1. 22 Aug, 2016 8 commits
    • Matt Roper's avatar
      drm/i915/gen9: Only copy WM results for changed pipes to skl_hw · 2722efb9
      Matt Roper authored
      When we write watermark values to the hardware, those values are stored
      in dev_priv->wm.skl_hw.  However with recent watermark changes, the
      results structure we're copying from only contains valid watermark and
      DDB values for the pipes that are actually changing; the values for
      other pipes remain 0.  Thus a blind copy of the entire skl_wm_values
      structure will clobber the values for unchanged pipes...we need to be
      more selective and only copy over the values for the changing pipes.
      
      This mistake was hidden until recently due to another bug that caused us
      to erroneously re-calculate watermarks for all active pipes rather than
      changing pipes.  Only when that bug was fixed was the impact of this bug
      discovered (e.g., modesets failing with "Requested display configuration
      exceeds system watermark limitations" messages and leaving watermarks
      non-functional, even ones initiated by intel_fbdev_restore_mode).
      
      Changes since v1:
       - Add a function for copying a pipe's wm values
         (skl_copy_wm_for_pipe()) so we can reuse this later
      
      Fixes: 734fa01f ("drm/i915/gen9: Calculate watermarks during atomic 'check' (v2)")
      Fixes: 9b613022 ("drm/i915/gen9: Re-allocate DDB only for changed pipes")
      Signed-off-by: default avatarMatt Roper <matthew.d.roper@intel.com>
      Signed-off-by: default avatarLyude <cpaul@redhat.com>
      Reviewed-by: default avatarMatt Roper <matthew.d.roper@intel.com>
      Cc: stable@vger.kernel.org
      Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
      Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
      Cc: Daniel Vetter <daniel.vetter@intel.com>
      Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
      Cc: Hans de Goede <hdegoede@redhat.com>
      Signed-off-by: default avatarMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
      Link: http://patchwork.freedesktop.org/patch/msgid/1471463761-26796-4-git-send-email-cpaul@redhat.com
      2722efb9
    • Lyude's avatar
      drm/i915/skl: Add support for the SAGV, fix underrun hangs · 656d1b89
      Lyude authored
      Since the watermark calculations for Skylake are still broken, we're apt
      to hitting underruns very easily under multi-monitor configurations.
      While it would be lovely if this was fixed, it's not. Another problem
      that's been coming from this however, is the mysterious issue of
      underruns causing full system hangs. An easy way to reproduce this with
      a skylake system:
      
      - Get a laptop with a skylake GPU, and hook up two external monitors to
        it
      - Move the cursor from the built-in LCD to one of the external displays
        as quickly as you can
      - You'll get a few pipe underruns, and eventually the entire system will
        just freeze.
      
      After doing a lot of investigation and reading through the bspec, I
      found the existence of the SAGV, which is responsible for adjusting the
      system agent voltage and clock frequencies depending on how much power
      we need. According to the bspec:
      
      "The display engine access to system memory is blocked during the
       adjustment time. SAGV defaults to enabled. Software must use the
       GT-driver pcode mailbox to disable SAGV when the display engine is not
       able to tolerate the blocking time."
      
      The rest of the bspec goes on to explain that software can simply leave
      the SAGV enabled, and disable it when we use interlaced pipes/have more
      then one pipe active.
      
      Sure enough, with this patchset the system hangs resulting from pipe
      underruns on Skylake have completely vanished on my T460s. Additionally,
      the bspec mentions turning off the SAGV	with more then one pipe enabled
      as a workaround for display underruns. While this patch doesn't entirely
      fix that, it looks like it does improve the situation a little bit so
      it's likely this is going to be required to make watermarks on Skylake
      fully functional.
      
      This will still need additional work in the future: we shouldn't be
      enabling the SAGV if any of the currently enabled planes can't enable WM
      levels that introduce latencies >= 30 µs.
      
      Changes since v11:
       - Add skl_can_enable_sagv()
       - Make sure we don't enable SAGV when not all planes can enable
         watermarks >= the SAGV engine block time. I was originally going to
         save this for later, but I recently managed to run into a machine
         that was having problems with a single pipe configuration + SAGV.
       - Make comparisons to I915_SKL_SAGV_NOT_CONTROLLED explicit
       - Change I915_SAGV_DYNAMIC_FREQ to I915_SAGV_ENABLE
       - Move printks outside of mutexes
       - Don't print error messages twice
      Changes since v10:
       - Apparently sandybridge_pcode_read actually writes values and reads
         them back, despite it's misleading function name. This means we've
         been doing this mostly wrong and have been writing garbage to the
         SAGV control. Because of this, we no longer attempt to read the SAGV
         status during initialization (since there are no helpers for this).
       - mlankhorst noticed that this patch was breaking on some very early
         pre-release Skylake machines, which apparently don't allow you to
         disable the SAGV. To prevent machines from failing tests due to SAGV
         errors, if the first time we try to control the SAGV results in the
         mailbox indicating an invalid command, we just disable future attempts
         to control the SAGV state by setting dev_priv->skl_sagv_status to
         I915_SKL_SAGV_NOT_CONTROLLED and make a note of it in dmesg.
       - Move mutex_unlock() a little higher in skl_enable_sagv(). This
         doesn't actually fix anything, but lets us release the lock a little
         sooner since we're finished with it.
      Changes since v9:
       - Only enable/disable sagv on Skylake
      Changes since v8:
       - Add intel_state->modeset guard to the conditional for
         skl_enable_sagv()
      Changes since v7:
       - Remove GEN9_SAGV_LOW_FREQ, replace with GEN9_SAGV_IS_ENABLED (that's
         all we use it for anyway)
       - Use GEN9_SAGV_IS_ENABLED instead of 0x1 for clarification
       - Fix a styling error that snuck past me
      Changes since v6:
       - Protect skl_enable_sagv() with intel_state->modeset conditional in
         intel_atomic_commit_tail()
      Changes since v5:
       - Don't use is_power_of_2. Makes things confusing
       - Don't use the old state to figure out whether or not to
         enable/disable the sagv, use the new one
       - Split the loop in skl_disable_sagv into it's own function
       - Move skl_sagv_enable/disable() calls into intel_atomic_commit_tail()
      Changes since v4:
       - Use is_power_of_2 against active_crtcs to check whether we have > 1
         pipe enabled
       - Fix skl_sagv_get_hw_state(): (temp & 0x1) indicates disabled, 0x0
         enabled
       - Call skl_sagv_enable/disable() from pre/post-plane updates
      Changes since v3:
       - Use time_before() to compare timeout to jiffies
      Changes since v2:
       - Really apply minor style nitpicks to patch this time
      Changes since v1:
       - Added comments about this probably being one of the requirements to
         fixing Skylake's watermark issues
       - Minor style nitpicks from Matt Roper
       - Disable these functions on Broxton, since it doesn't have an SAGV
      Signed-off-by: default avatarLyude <cpaul@redhat.com>
      Cc: Matt Roper <matthew.d.roper@intel.com>
      Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
      Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
      Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
      Cc: stable@vger.kernel.org
      Signed-off-by: default avatarMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
      Link: http://patchwork.freedesktop.org/patch/msgid/1471463761-26796-3-git-send-email-cpaul@redhat.com
      [mlankhorst: ENOSYS -> ENXIO, whitespace fixes]
      656d1b89
    • Lyude's avatar
      drm/i915/gen6+: Interpret mailbox error flags · 87660502
      Lyude authored
      In order to add proper support for the SAGV, we need to be able to know
      what the cause of a failure to change the SAGV through the pcode mailbox
      was. The reasoning for this is that some very early pre-release Skylake
      machines don't actually allow you to control the SAGV on them, and
      indicate an invalid mailbox command was sent.
      
      This also might come in handy in the future for debugging.
      
      Changes since v1:
       - Add functions for interpreting gen6 mailbox error codes along with
         gen7+ error codes, and actually interpret those codes properly
       - Renamed patch to reflect new behavior
      Signed-off-by: default avatarLyude <cpaul@redhat.com>
      Cc: Matt Roper <matthew.d.roper@intel.com>
      Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
      Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
      Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
      Cc: stable@vger.kernel.org
      Signed-off-by: default avatarMaarten Lankhorst <maarten.lankhorst@linux.intel.com>
      Link: http://patchwork.freedesktop.org/patch/msgid/1471463761-26796-2-git-send-email-cpaul@redhat.com
      [mlankhorst: -ENOSYS -> -ENXIO for checkpatch]
      87660502
    • Paulo Zanoni's avatar
      drm/i915: Call intel_fbc_pre_update() after pinning the new pageflip · 1f061316
      Paulo Zanoni authored
      intel_fbc_pre_update() depends upon the new state being already pinned
      in place in the Global GTT (primarily for both fencing which wants both
      an offset and a fence register, if assigned). This requires the call to
      intel_fbc_pre_update() be after intel_pin_and_fence_fb() - but commit
      e8216e50 ("drm/i915/fbc: call intel_fbc_pre_update earlier during
      page flips") moved the code way too much up in its attempt to call it
      before the page flip.
      
      v2 (from Paulo):
       - Point the original bad commit.
       - Add a comment to maybe prevent further regressions.
      
      Fixes: e8216e50 ("drm/i915/fbc: call intel_fbc_pre_update earlier...")
      Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
      Signed-off-by: default avatarPaulo Zanoni <paulo.r.zanoni@intel.com>
      Cc: Daniel Vetter <daniel.vetter@intel.com>
      Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
      Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
      Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
      Cc: drm-intel-fixes@lists.freedesktop.org
      Reviewed-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
      Link: http://patchwork.freedesktop.org/patch/msgid/1471462904-842-1-git-send-email-paulo.r.zanoni@intel.com
      Cc: stable@vger.kernel.org
      1f061316
    • Daniel Vetter's avatar
      drm/i915: Ensure consistent control flow __i915_gem_active_get_rcu · c75870d8
      Daniel Vetter authored
      This issue here is (I think) purely theoretical, since a compiler
      would need to be especially foolish to recompute the value of
      i915_gem_request_completed right after it was already used. Hence the
      additional barrier() is also not really a restriction.
      
      But I believe this to be at least permissible, and since our rcu
      trickery is a beast it's worth to annotate all the corner cases.
      Chris proposed to instead just wrap a READ_ONCE around
      request->fence.seqno in i915_gem_request_completed. But that has a
      measurable impact on code size, and everywhere we hold a full
      reference to the underlying request it's also not needed. And
      personally I'd like to have just enough barriers and locking needed
      for correctness, but not more - it makes it much easier in the future
      to understand what's going on.
      
      Since the busy ioctl has now fully embraced it's races there's no
      point annotating it there too. We really only need it in
      active_get_rcu, since that function _must_ deliver a correct snapshot
      of the active fences (and not chase something else).
      
      v2: Polish the comment a bit more (Chris).
      
      Cc: Chris Wilson <chris@chris-wilson.co.uk>
      Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
      Reviewed-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
      Signed-off-by: default avatarDaniel Vetter <daniel.vetter@intel.com>
      Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
      Link: http://patchwork.freedesktop.org/patch/msgid/1471856122-466-1-git-send-email-daniel.vetter@ffwll.ch
      c75870d8
    • Chris Wilson's avatar
      drm/i915: Stop marking the unaccessible scratch page as UC · 14daa63b
      Chris Wilson authored
      Since by design, if not entirely by practice, nothing is allowed to
      access the scratch page we use to background fill the VM, then we do not
      need to ensure that it is coherent between the CPU and GPU.
      set_pages_uc() does a stop_machine() after changing the PAT, and that
      significantly impacts upon context creation throughput.
      Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
      Link: http://patchwork.freedesktop.org/patch/msgid/20160822074431.26872-1-chris@chris-wilson.co.ukReviewed-by: default avatarJoonas Lahtinen <joonas.lahtinen@linux.intel.com>
      14daa63b
    • Chris Wilson's avatar
      drm/i915: Restore debugfs/i915_gem_gtt back to its former glory · 5f4b091a
      Chris Wilson authored
      The passed in flag that distinguishes i915_gem_pin_display from
      i915_gem_gtt is from node->info_ent->data not the data function
      parameter.
      
      Fixes: 6da84829 ("drm/i915: Focus debugfs/i915_gem_pinned to show...")
      Signed-off-by: default avatarChris Wilson <chris@chris-wilson.co.uk>
      Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
      Link: http://patchwork.freedesktop.org/patch/msgid/20160819115625.17688-1-chris@chris-wilson.co.ukReviewed-by: default avatarJoonas Lahtinen <joonas.lahtinen@linux.intel.com>
      5f4b091a
    • Daniel Vetter's avatar
      d5d0804f
  2. 19 Aug, 2016 7 commits
  3. 18 Aug, 2016 25 commits