Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Jani Nikula]

On Fri, 15 Aug 2014, Paulo Zanoni przan...@gmail.com wrote:
 From: Paulo Zanoni paulo.r.zan...@intel.com

 If we're runtime suspended and try to use the plane interfaces, we
 will get a lot of WARNs saying we did the wrong thing.

 We need to get runtime PM references to pin the objects, and to
 change the fences. The pin functions are the ideal places for
 this, but intel_crtc_cursor_set_obj() doesn't call them, so we also
 have to add get/put calls inside it. There is no problem if we runtime
 suspend right after these functions are finished, because the
 registers written are forwarded to system memory.

 Note: for a complete fix of the cursor-dpms test case, we also need
 the patch named drm/i915: Don't try to enable cursor from setplane
 when crtc is disabled.

 v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
 v3: - Make get/put also surround the fence and unpin calls (Daniel and
   Ville).
 - Merge all the plane changes into a single patch since they're
   the same fix.
 - Add the comment requested by Daniel.
 v4: - Remove spurious whitespace (Ville).
 v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
   equivalent fix in another patch (Ville).
 v6: - Remove unpin chunk: it will be on a separate patch (Ville,
   Chris, Daniel).
 v7: - Same thing, new color.

 Testcase: igt/pm_rpm/cursor
 Testcase: igt/pm_rpm/cursor-dpms
 Testcase: igt/pm_rpm/legacy-planes
 Testcase: igt/pm_rpm/legacy-planes-dpms
 Testcase: igt/pm_rpm/universal-planes
 Testcase: igt/pm_rpm/universal-planes-dpms
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82603
 Cc: sta...@vger.kernel.org
 Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com

Pushed to drm-intel-fixes, thanks for the patch and review.

BR,
Jani.


 ---
  drivers/gpu/drm/i915/intel_display.c | 25 +
  1 file changed, 25 insertions(+)

 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index 3f8e037..15fe3eb 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -2201,6 +2201,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   if (need_vtd_wa(dev)  alignment  256 * 1024)
   alignment = 256 * 1024;
  
 + /*
 +  * Global gtt pte registers are special registers which actually forward
 +  * writes to a chunk of system memory. Which means that there is no risk
 +  * that the register values disappear as soon as we call
 +  * intel_runtime_pm_put(), so it is correct to wrap only the
 +  * pin/unpin/fence and not more.
 +  */
 + intel_runtime_pm_get(dev_priv);
 +
   dev_priv-mm.interruptible = false;
   ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
   if (ret)
 @@ -2218,12 +2227,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   i915_gem_object_pin_fence(obj);
  
   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return 0;
  
  err_unpin:
   i915_gem_object_unpin_from_display_plane(obj);
  err_interruptible:
   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return ret;
  }
  
 @@ -8253,6 +8264,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
uint32_t width, uint32_t height)
  {
   struct drm_device *dev = crtc-dev;
 + struct drm_i915_private *dev_priv = dev-dev_private;
   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
   enum pipe pipe = intel_crtc-pipe;
   unsigned old_width, stride;
 @@ -8292,6 +8304,15 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
   goto fail_locked;
   }
  
 + /*
 +  * Global gtt pte registers are special registers which actually
 +  * forward writes to a chunk of system memory. Which means that
 +  * there is no risk that the register values disappear as soon
 +  * as we call intel_runtime_pm_put(), so it is correct to wrap
 +  * only the pin/unpin/fence and not more.
 +  */
 + intel_runtime_pm_get(dev_priv);
 +
   /* Note that the w/a also requires 2 PTE of padding following
* the bo. We currently fill all unused PTE with the shadow
* page and so we should always have valid PTE following the
 @@ -8304,16 +8325,20 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
   ret = i915_gem_object_pin_to_display_plane(obj, alignment, 
 NULL);
   if (ret) {
   DRM_DEBUG_KMS(failed to move cursor bo into the 
 GTT\n);
 + intel_runtime_pm_put(dev_priv);
   goto fail_locked;
   }
  
   ret = i915_gem_object_put_fence(obj);
   if (ret) {
 

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Ville Syrjälä]

On Fri, Aug 15, 2014 at 03:59:32PM -0300, Paulo Zanoni wrote:
 From: Paulo Zanoni paulo.r.zan...@intel.com
 
 If we're runtime suspended and try to use the plane interfaces, we
 will get a lot of WARNs saying we did the wrong thing.
 
 We need to get runtime PM references to pin the objects, and to
 change the fences. The pin functions are the ideal places for
 this, but intel_crtc_cursor_set_obj() doesn't call them, so we also
 have to add get/put calls inside it. There is no problem if we runtime
 suspend right after these functions are finished, because the
 registers written are forwarded to system memory.
 
 Note: for a complete fix of the cursor-dpms test case, we also need
 the patch named drm/i915: Don't try to enable cursor from setplane
 when crtc is disabled.
 
 v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
 v3: - Make get/put also surround the fence and unpin calls (Daniel and
   Ville).
 - Merge all the plane changes into a single patch since they're
   the same fix.
 - Add the comment requested by Daniel.
 v4: - Remove spurious whitespace (Ville).
 v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
   equivalent fix in another patch (Ville).
 v6: - Remove unpin chunk: it will be on a separate patch (Ville,
   Chris, Daniel).
 v7: - Same thing, new color.
 
 Testcase: igt/pm_rpm/cursor
 Testcase: igt/pm_rpm/cursor-dpms
 Testcase: igt/pm_rpm/legacy-planes
 Testcase: igt/pm_rpm/legacy-planes-dpms
 Testcase: igt/pm_rpm/universal-planes
 Testcase: igt/pm_rpm/universal-planes-dpms
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82603
 Cc: sta...@vger.kernel.org
 Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com

Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com

 ---
  drivers/gpu/drm/i915/intel_display.c | 25 +
  1 file changed, 25 insertions(+)
 
 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index 3f8e037..15fe3eb 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -2201,6 +2201,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   if (need_vtd_wa(dev)  alignment  256 * 1024)
   alignment = 256 * 1024;
  
 + /*
 +  * Global gtt pte registers are special registers which actually forward
 +  * writes to a chunk of system memory. Which means that there is no risk
 +  * that the register values disappear as soon as we call
 +  * intel_runtime_pm_put(), so it is correct to wrap only the
 +  * pin/unpin/fence and not more.
 +  */
 + intel_runtime_pm_get(dev_priv);
 +
   dev_priv-mm.interruptible = false;
   ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
   if (ret)
 @@ -2218,12 +2227,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   i915_gem_object_pin_fence(obj);
  
   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return 0;
  
  err_unpin:
   i915_gem_object_unpin_from_display_plane(obj);
  err_interruptible:
   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return ret;
  }
  
 @@ -8253,6 +8264,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
uint32_t width, uint32_t height)
  {
   struct drm_device *dev = crtc-dev;
 + struct drm_i915_private *dev_priv = dev-dev_private;
   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
   enum pipe pipe = intel_crtc-pipe;
   unsigned old_width, stride;
 @@ -8292,6 +8304,15 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
   goto fail_locked;
   }
  
 + /*
 +  * Global gtt pte registers are special registers which actually
 +  * forward writes to a chunk of system memory. Which means that
 +  * there is no risk that the register values disappear as soon
 +  * as we call intel_runtime_pm_put(), so it is correct to wrap
 +  * only the pin/unpin/fence and not more.
 +  */
 + intel_runtime_pm_get(dev_priv);
 +
   /* Note that the w/a also requires 2 PTE of padding following
* the bo. We currently fill all unused PTE with the shadow
* page and so we should always have valid PTE following the
 @@ -8304,16 +8325,20 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
   ret = i915_gem_object_pin_to_display_plane(obj, alignment, 
 NULL);
   if (ret) {
   DRM_DEBUG_KMS(failed to move cursor bo into the 
 GTT\n);
 + intel_runtime_pm_put(dev_priv);
   goto fail_locked;
   }
  
   ret = i915_gem_object_put_fence(obj);
   if (ret) {

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Ville Syrjälä]

On Thu, Aug 14, 2014 at 12:06:02PM -0300, Paulo Zanoni wrote:
 From: Paulo Zanoni paulo.r.zan...@intel.com
 
 If we're runtime suspended and try to use the plane interfaces, we
 will get a lot of WARNs saying we did the wrong thing.
 
 We need to get runtime PM references to pin the objects, and to
 change the fences. The pin functions are the ideal places for
 this, but intel_crtc_cursor_set_obj() doesn't call them, so we also
 have to add get/put calls inside it. There is no problem if we runtime
 suspend right after these functions are finished, because the
 registers written are forwarded to system memory.
 
 Note: for a complete fix of the cursor-dpms test case, we also need
 the patch named drm/i915: Don't try to enable cursor from setplane
 when crtc is disabled.
 
 v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
 v3: - Make get/put also surround the fence and unpin calls (Daniel and
   Ville).
 - Merge all the plane changes into a single patch since they're
   the same fix.
 - Add the comment requested by Daniel.
 v4: - Remove spurious whitespace (Ville).
 v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
   equivalent fix in another patch (Ville).
 v6: - Remove unpin chunk: it will be on a separate patch (Ville,
   Chris, Daniel).
 
 Testcase: igt/pm_rpm/cursor
 Testcase: igt/pm_rpm/cursor-dpms
 Testcase: igt/pm_rpm/legacy-planes
 Testcase: igt/pm_rpm/legacy-planes-dpms
 Testcase: igt/pm_rpm/universal-planes
 Testcase: igt/pm_rpm/universal-planes-dpms
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
 Cc: sta...@vger.kernel.org
 Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
 ---
  drivers/gpu/drm/i915/intel_display.c | 27 +++
  1 file changed, 27 insertions(+)
 
 
 I talked with Daniel and we agreed to leave any possible fixes related with 
 the
 unpin functions to separate patches, possibly requiring separate IGT test
 cases.
 
 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index 3813526..17bc661 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   if (need_vtd_wa(dev)  alignment  256 * 1024)
   alignment = 256 * 1024;
  
 + /*
 +  * Global gtt pte registers are special registers which actually forward
 +  * writes to a chunk of system memory. Which means that there is no risk
 +  * that the register values disappear as soon as we call
 +  * intel_runtime_pm_put(), so it is correct to wrap only the
 +  * pin/unpin/fence and not more.
 +  */
 + intel_runtime_pm_get(dev_priv);
 +
   dev_priv-mm.interruptible = false;
   ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
   if (ret)
 @@ -2166,12 +2175,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   i915_gem_object_pin_fence(obj);
  
   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return 0;
  
  err_unpin:
   i915_gem_object_unpin_from_display_plane(obj);
  err_interruptible:
   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return ret;
  }
  
 @@ -8201,6 +8212,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
uint32_t width, uint32_t height)
  {
   struct drm_device *dev = crtc-dev;
 + struct drm_i915_private *dev_priv = dev-dev_private;
   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
   enum pipe pipe = intel_crtc-pipe;
   unsigned old_width, stride;
 @@ -8231,6 +8243,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
  
   /* we only need to pin inside GTT if cursor is non-phy */
   mutex_lock(dev-struct_mutex);
 +
 + /*
 +  * Global gtt pte registers are special registers which actually forward
 +  * writes to a chunk of system memory. Which means that there is no risk
 +  * that the register values disappear as soon as we call
 +  * intel_runtime_pm_put(), so it is correct to wrap only the
 +  * pin/unpin/fence and not more.
 +  */
 + intel_runtime_pm_get(dev_priv);
 +

Only the !cursor_needs_physical case need runtime pm get/put. I thought
the calls were there originally, but I guess they got moved out. I
suppose it's not a huge deal either way, but the current approach does
give the reader the wrong impression that the unpin and frontbuffer
tracking would also need a rpm reference.

   if (!INTEL_INFO(dev)-cursor_needs_physical) {
   unsigned alignment;
  
 @@ -8280,6 +8302,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
  
   i915_gem_track_fb(intel_crtc-cursor_bo, obj,
 INTEL_FRONTBUFFER_CURSOR(pipe));
 +
 + if (obj)
 + intel_runtime_pm_put(dev_priv);
 +
   

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Paulo Zanoni]

2014-08-15 5:39 GMT-03:00 Ville Syrjälä ville.syrj...@linux.intel.com:
 On Thu, Aug 14, 2014 at 12:06:02PM -0300, Paulo Zanoni wrote:
 From: Paulo Zanoni paulo.r.zan...@intel.com

 If we're runtime suspended and try to use the plane interfaces, we
 will get a lot of WARNs saying we did the wrong thing.

 We need to get runtime PM references to pin the objects, and to
 change the fences. The pin functions are the ideal places for
 this, but intel_crtc_cursor_set_obj() doesn't call them, so we also
 have to add get/put calls inside it. There is no problem if we runtime
 suspend right after these functions are finished, because the
 registers written are forwarded to system memory.

 Note: for a complete fix of the cursor-dpms test case, we also need
 the patch named drm/i915: Don't try to enable cursor from setplane
 when crtc is disabled.

 v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
 v3: - Make get/put also surround the fence and unpin calls (Daniel and
   Ville).
 - Merge all the plane changes into a single patch since they're
   the same fix.
 - Add the comment requested by Daniel.
 v4: - Remove spurious whitespace (Ville).
 v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
   equivalent fix in another patch (Ville).
 v6: - Remove unpin chunk: it will be on a separate patch (Ville,
   Chris, Daniel).

 Testcase: igt/pm_rpm/cursor
 Testcase: igt/pm_rpm/cursor-dpms
 Testcase: igt/pm_rpm/legacy-planes
 Testcase: igt/pm_rpm/legacy-planes-dpms
 Testcase: igt/pm_rpm/universal-planes
 Testcase: igt/pm_rpm/universal-planes-dpms
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
 Cc: sta...@vger.kernel.org
 Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
 ---
  drivers/gpu/drm/i915/intel_display.c | 27 +++
  1 file changed, 27 insertions(+)


 I talked with Daniel and we agreed to leave any possible fixes related with 
 the
 unpin functions to separate patches, possibly requiring separate IGT test
 cases.

 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index 3813526..17bc661 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   if (need_vtd_wa(dev)  alignment  256 * 1024)
   alignment = 256 * 1024;

 + /*
 +  * Global gtt pte registers are special registers which actually 
 forward
 +  * writes to a chunk of system memory. Which means that there is no 
 risk
 +  * that the register values disappear as soon as we call
 +  * intel_runtime_pm_put(), so it is correct to wrap only the
 +  * pin/unpin/fence and not more.
 +  */
 + intel_runtime_pm_get(dev_priv);
 +
   dev_priv-mm.interruptible = false;
   ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
   if (ret)
 @@ -2166,12 +2175,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   i915_gem_object_pin_fence(obj);

   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return 0;

  err_unpin:
   i915_gem_object_unpin_from_display_plane(obj);
  err_interruptible:
   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return ret;
  }

 @@ -8201,6 +8212,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
uint32_t width, uint32_t height)
  {
   struct drm_device *dev = crtc-dev;
 + struct drm_i915_private *dev_priv = dev-dev_private;
   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
   enum pipe pipe = intel_crtc-pipe;
   unsigned old_width, stride;
 @@ -8231,6 +8243,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,

   /* we only need to pin inside GTT if cursor is non-phy */
   mutex_lock(dev-struct_mutex);
 +
 + /*
 +  * Global gtt pte registers are special registers which actually 
 forward
 +  * writes to a chunk of system memory. Which means that there is no 
 risk
 +  * that the register values disappear as soon as we call
 +  * intel_runtime_pm_put(), so it is correct to wrap only the
 +  * pin/unpin/fence and not more.
 +  */
 + intel_runtime_pm_get(dev_priv);
 +

 Only the !cursor_needs_physical case need runtime pm get/put. I thought
 the calls were there originally, but I guess they got moved out. I
 suppose it's not a huge deal either way, but the current approach does
 give the reader the wrong impression that the unpin and frontbuffer
 tracking would also need a rpm reference.

It's not a huge deal either way, yet you don't give a reviewed-by tag :)

I thought about just putting the get/put inside cursor_needs_physical,
but then I'd need a new bool variable to track whether we need to put
or not at the failure code paths. And we have so much code churn that
maybe additional changes could leave the 

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Ville Syrjälä]

On Fri, Aug 15, 2014 at 01:47:18PM -0300, Paulo Zanoni wrote:
 2014-08-15 5:39 GMT-03:00 Ville Syrjälä ville.syrj...@linux.intel.com:
  On Thu, Aug 14, 2014 at 12:06:02PM -0300, Paulo Zanoni wrote:
  From: Paulo Zanoni paulo.r.zan...@intel.com
 
  If we're runtime suspended and try to use the plane interfaces, we
  will get a lot of WARNs saying we did the wrong thing.
 
  We need to get runtime PM references to pin the objects, and to
  change the fences. The pin functions are the ideal places for
  this, but intel_crtc_cursor_set_obj() doesn't call them, so we also
  have to add get/put calls inside it. There is no problem if we runtime
  suspend right after these functions are finished, because the
  registers written are forwarded to system memory.
 
  Note: for a complete fix of the cursor-dpms test case, we also need
  the patch named drm/i915: Don't try to enable cursor from setplane
  when crtc is disabled.
 
  v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
  v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
  - Merge all the plane changes into a single patch since they're
the same fix.
  - Add the comment requested by Daniel.
  v4: - Remove spurious whitespace (Ville).
  v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
equivalent fix in another patch (Ville).
  v6: - Remove unpin chunk: it will be on a separate patch (Ville,
Chris, Daniel).
 
  Testcase: igt/pm_rpm/cursor
  Testcase: igt/pm_rpm/cursor-dpms
  Testcase: igt/pm_rpm/legacy-planes
  Testcase: igt/pm_rpm/legacy-planes-dpms
  Testcase: igt/pm_rpm/universal-planes
  Testcase: igt/pm_rpm/universal-planes-dpms
  Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
  Cc: sta...@vger.kernel.org
  Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
  ---
   drivers/gpu/drm/i915/intel_display.c | 27 +++
   1 file changed, 27 insertions(+)
 
 
  I talked with Daniel and we agreed to leave any possible fixes related 
  with the
  unpin functions to separate patches, possibly requiring separate IGT test
  cases.
 
  diff --git a/drivers/gpu/drm/i915/intel_display.c 
  b/drivers/gpu/drm/i915/intel_display.c
  index 3813526..17bc661 100644
  --- a/drivers/gpu/drm/i915/intel_display.c
  +++ b/drivers/gpu/drm/i915/intel_display.c
  @@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
if (need_vtd_wa(dev)  alignment  256 * 1024)
alignment = 256 * 1024;
 
  + /*
  +  * Global gtt pte registers are special registers which actually 
  forward
  +  * writes to a chunk of system memory. Which means that there is no 
  risk
  +  * that the register values disappear as soon as we call
  +  * intel_runtime_pm_put(), so it is correct to wrap only the
  +  * pin/unpin/fence and not more.
  +  */
  + intel_runtime_pm_get(dev_priv);
  +
dev_priv-mm.interruptible = false;
ret = i915_gem_object_pin_to_display_plane(obj, alignment, 
  pipelined);
if (ret)
  @@ -2166,12 +2175,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
i915_gem_object_pin_fence(obj);
 
dev_priv-mm.interruptible = true;
  + intel_runtime_pm_put(dev_priv);
return 0;
 
   err_unpin:
i915_gem_object_unpin_from_display_plane(obj);
   err_interruptible:
dev_priv-mm.interruptible = true;
  + intel_runtime_pm_put(dev_priv);
return ret;
   }
 
  @@ -8201,6 +8212,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
  *crtc,
 uint32_t width, uint32_t height)
   {
struct drm_device *dev = crtc-dev;
  + struct drm_i915_private *dev_priv = dev-dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
enum pipe pipe = intel_crtc-pipe;
unsigned old_width, stride;
  @@ -8231,6 +8243,16 @@ static int intel_crtc_cursor_set_obj(struct 
  drm_crtc *crtc,
 
/* we only need to pin inside GTT if cursor is non-phy */
mutex_lock(dev-struct_mutex);
  +
  + /*
  +  * Global gtt pte registers are special registers which actually 
  forward
  +  * writes to a chunk of system memory. Which means that there is no 
  risk
  +  * that the register values disappear as soon as we call
  +  * intel_runtime_pm_put(), so it is correct to wrap only the
  +  * pin/unpin/fence and not more.
  +  */
  + intel_runtime_pm_get(dev_priv);
  +
 
  Only the !cursor_needs_physical case need runtime pm get/put. I thought
  the calls were there originally, but I guess they got moved out. I
  suppose it's not a huge deal either way, but the current approach does
  give the reader the wrong impression that the unpin and frontbuffer
  tracking would also need a rpm reference.
 
 It's not a huge deal either way, yet you don't give a reviewed-by tag :)

I wanted to read your reply if there's a good reason for it ;)

 
 

[Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Paulo Zanoni]

From: Paulo Zanoni paulo.r.zan...@intel.com

If we're runtime suspended and try to use the plane interfaces, we
will get a lot of WARNs saying we did the wrong thing.

We need to get runtime PM references to pin the objects, and to
change the fences. The pin functions are the ideal places for
this, but intel_crtc_cursor_set_obj() doesn't call them, so we also
have to add get/put calls inside it. There is no problem if we runtime
suspend right after these functions are finished, because the
registers written are forwarded to system memory.

Note: for a complete fix of the cursor-dpms test case, we also need
the patch named drm/i915: Don't try to enable cursor from setplane
when crtc is disabled.

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
  Ville).
- Merge all the plane changes into a single patch since they're
  the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).
v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
  equivalent fix in another patch (Ville).
v6: - Remove unpin chunk: it will be on a separate patch (Ville,
  Chris, Daniel).
v7: - Same thing, new color.

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82603
Cc: sta...@vger.kernel.org
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
---
 drivers/gpu/drm/i915/intel_display.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 3f8e037..15fe3eb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2201,6 +2201,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
if (need_vtd_wa(dev)  alignment  256 * 1024)
alignment = 256 * 1024;
 
+   /*
+* Global gtt pte registers are special registers which actually forward
+* writes to a chunk of system memory. Which means that there is no risk
+* that the register values disappear as soon as we call
+* intel_runtime_pm_put(), so it is correct to wrap only the
+* pin/unpin/fence and not more.
+*/
+   intel_runtime_pm_get(dev_priv);
+
dev_priv-mm.interruptible = false;
ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
if (ret)
@@ -2218,12 +2227,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
i915_gem_object_pin_fence(obj);
 
dev_priv-mm.interruptible = true;
+   intel_runtime_pm_put(dev_priv);
return 0;
 
 err_unpin:
i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
dev_priv-mm.interruptible = true;
+   intel_runtime_pm_put(dev_priv);
return ret;
 }
 
@@ -8253,6 +8264,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
 uint32_t width, uint32_t height)
 {
struct drm_device *dev = crtc-dev;
+   struct drm_i915_private *dev_priv = dev-dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
enum pipe pipe = intel_crtc-pipe;
unsigned old_width, stride;
@@ -8292,6 +8304,15 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
goto fail_locked;
}
 
+   /*
+* Global gtt pte registers are special registers which actually
+* forward writes to a chunk of system memory. Which means that
+* there is no risk that the register values disappear as soon
+* as we call intel_runtime_pm_put(), so it is correct to wrap
+* only the pin/unpin/fence and not more.
+*/
+   intel_runtime_pm_get(dev_priv);
+
/* Note that the w/a also requires 2 PTE of padding following
 * the bo. We currently fill all unused PTE with the shadow
 * page and so we should always have valid PTE following the
@@ -8304,16 +8325,20 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
ret = i915_gem_object_pin_to_display_plane(obj, alignment, 
NULL);
if (ret) {
DRM_DEBUG_KMS(failed to move cursor bo into the 
GTT\n);
+   intel_runtime_pm_put(dev_priv);
goto fail_locked;
}
 
ret = i915_gem_object_put_fence(obj);
if (ret) {
DRM_DEBUG_KMS(failed to release fence for cursor);
+   intel_runtime_pm_put(dev_priv);
goto 

[Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Paulo Zanoni]

From: Paulo Zanoni paulo.r.zan...@intel.com

If we're runtime suspended and try to use the plane interfaces, we
will get a lot of WARNs saying we did the wrong thing.

We need to get runtime PM references to pin the objects, and to
change the fences. The pin functions are the ideal places for
this, but intel_crtc_cursor_set_obj() doesn't call them, so we also
have to add get/put calls inside it. There is no problem if we runtime
suspend right after these functions are finished, because the
registers written are forwarded to system memory.

Note: for a complete fix of the cursor-dpms test case, we also need
the patch named drm/i915: Don't try to enable cursor from setplane
when crtc is disabled.

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
  Ville).
- Merge all the plane changes into a single patch since they're
  the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).
v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
  equivalent fix in another patch (Ville).
v6: - Remove unpin chunk: it will be on a separate patch (Ville,
  Chris, Daniel).

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
Cc: sta...@vger.kernel.org
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
---
 drivers/gpu/drm/i915/intel_display.c | 27 +++
 1 file changed, 27 insertions(+)


I talked with Daniel and we agreed to leave any possible fixes related with the
unpin functions to separate patches, possibly requiring separate IGT test
cases.

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 3813526..17bc661 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
if (need_vtd_wa(dev)  alignment  256 * 1024)
alignment = 256 * 1024;
 
+   /*
+* Global gtt pte registers are special registers which actually forward
+* writes to a chunk of system memory. Which means that there is no risk
+* that the register values disappear as soon as we call
+* intel_runtime_pm_put(), so it is correct to wrap only the
+* pin/unpin/fence and not more.
+*/
+   intel_runtime_pm_get(dev_priv);
+
dev_priv-mm.interruptible = false;
ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
if (ret)
@@ -2166,12 +2175,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
i915_gem_object_pin_fence(obj);
 
dev_priv-mm.interruptible = true;
+   intel_runtime_pm_put(dev_priv);
return 0;
 
 err_unpin:
i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
dev_priv-mm.interruptible = true;
+   intel_runtime_pm_put(dev_priv);
return ret;
 }
 
@@ -8201,6 +8212,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
 uint32_t width, uint32_t height)
 {
struct drm_device *dev = crtc-dev;
+   struct drm_i915_private *dev_priv = dev-dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
enum pipe pipe = intel_crtc-pipe;
unsigned old_width, stride;
@@ -8231,6 +8243,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
 
/* we only need to pin inside GTT if cursor is non-phy */
mutex_lock(dev-struct_mutex);
+
+   /*
+* Global gtt pte registers are special registers which actually forward
+* writes to a chunk of system memory. Which means that there is no risk
+* that the register values disappear as soon as we call
+* intel_runtime_pm_put(), so it is correct to wrap only the
+* pin/unpin/fence and not more.
+*/
+   intel_runtime_pm_get(dev_priv);
+
if (!INTEL_INFO(dev)-cursor_needs_physical) {
unsigned alignment;
 
@@ -8280,6 +8302,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
 
i915_gem_track_fb(intel_crtc-cursor_bo, obj,
  INTEL_FRONTBUFFER_CURSOR(pipe));
+
+   if (obj)
+   intel_runtime_pm_put(dev_priv);
+
mutex_unlock(dev-struct_mutex);
 
old_width = intel_crtc-cursor_width;
@@ -8301,6 +8327,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
 fail_unpin:
i915_gem_object_unpin_from_display_plane(obj);
 fail_locked:
+   intel_runtime_pm_put(dev_priv);
mutex_unlock(dev-struct_mutex);
 fail:
drm_gem_object_unreference_unlocked(obj-base);
-- 
2.0.1

___

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Paulo Zanoni]

2014-08-14 12:06 GMT-03:00 Paulo Zanoni przan...@gmail.com:
 From: Paulo Zanoni paulo.r.zan...@intel.com

 If we're runtime suspended and try to use the plane interfaces, we
 will get a lot of WARNs saying we did the wrong thing.

 We need to get runtime PM references to pin the objects, and to
 change the fences. The pin functions are the ideal places for
 this, but intel_crtc_cursor_set_obj() doesn't call them, so we also
 have to add get/put calls inside it. There is no problem if we runtime
 suspend right after these functions are finished, because the
 registers written are forwarded to system memory.

 Note: for a complete fix of the cursor-dpms test case, we also need
 the patch named drm/i915: Don't try to enable cursor from setplane
 when crtc is disabled.

 v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
 v3: - Make get/put also surround the fence and unpin calls (Daniel and
   Ville).
 - Merge all the plane changes into a single patch since they're
   the same fix.
 - Add the comment requested by Daniel.
 v4: - Remove spurious whitespace (Ville).
 v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
   equivalent fix in another patch (Ville).
 v6: - Remove unpin chunk: it will be on a separate patch (Ville,
   Chris, Daniel).

 Testcase: igt/pm_rpm/cursor
 Testcase: igt/pm_rpm/cursor-dpms
 Testcase: igt/pm_rpm/legacy-planes
 Testcase: igt/pm_rpm/legacy-planes-dpms
 Testcase: igt/pm_rpm/universal-planes
 Testcase: igt/pm_rpm/universal-planes-dpms
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645

And now we also have a report from QA:
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=82603

 Cc: sta...@vger.kernel.org
 Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
 ---
  drivers/gpu/drm/i915/intel_display.c | 27 +++
  1 file changed, 27 insertions(+)


 I talked with Daniel and we agreed to leave any possible fixes related with 
 the
 unpin functions to separate patches, possibly requiring separate IGT test
 cases.

 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index 3813526..17bc661 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 if (need_vtd_wa(dev)  alignment  256 * 1024)
 alignment = 256 * 1024;

 +   /*
 +* Global gtt pte registers are special registers which actually 
 forward
 +* writes to a chunk of system memory. Which means that there is no 
 risk
 +* that the register values disappear as soon as we call
 +* intel_runtime_pm_put(), so it is correct to wrap only the
 +* pin/unpin/fence and not more.
 +*/
 +   intel_runtime_pm_get(dev_priv);
 +
 dev_priv-mm.interruptible = false;
 ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
 if (ret)
 @@ -2166,12 +2175,14 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 i915_gem_object_pin_fence(obj);

 dev_priv-mm.interruptible = true;
 +   intel_runtime_pm_put(dev_priv);
 return 0;

  err_unpin:
 i915_gem_object_unpin_from_display_plane(obj);
  err_interruptible:
 dev_priv-mm.interruptible = true;
 +   intel_runtime_pm_put(dev_priv);
 return ret;
  }

 @@ -8201,6 +8212,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
  uint32_t width, uint32_t height)
  {
 struct drm_device *dev = crtc-dev;
 +   struct drm_i915_private *dev_priv = dev-dev_private;
 struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 enum pipe pipe = intel_crtc-pipe;
 unsigned old_width, stride;
 @@ -8231,6 +8243,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,

 /* we only need to pin inside GTT if cursor is non-phy */
 mutex_lock(dev-struct_mutex);
 +
 +   /*
 +* Global gtt pte registers are special registers which actually 
 forward
 +* writes to a chunk of system memory. Which means that there is no 
 risk
 +* that the register values disappear as soon as we call
 +* intel_runtime_pm_put(), so it is correct to wrap only the
 +* pin/unpin/fence and not more.
 +*/
 +   intel_runtime_pm_get(dev_priv);
 +
 if (!INTEL_INFO(dev)-cursor_needs_physical) {
 unsigned alignment;

 @@ -8280,6 +8302,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,

 i915_gem_track_fb(intel_crtc-cursor_bo, obj,
   INTEL_FRONTBUFFER_CURSOR(pipe));
 +
 +   if (obj)
 +   intel_runtime_pm_put(dev_priv);
 +
 mutex_unlock(dev-struct_mutex);

 old_width = intel_crtc-cursor_width;
 @@ -8301,6 +8327,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
  

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Daniel Vetter]

On Tue, Aug 12, 2014 at 09:51:13PM +0100, Chris Wilson wrote:
 On Tue, Aug 12, 2014 at 10:37:21PM +0200, Daniel Vetter wrote:
  On Tue, Aug 12, 2014 at 10:30 PM, Chris Wilson ch...@chris-wilson.co.uk 
  wrote:
   On Tue, Aug 12, 2014 at 10:19:20PM +0200, Daniel Vetter wrote:
   On Tue, Aug 12, 2014 at 9:33 PM, Paulo Zanoni przan...@gmail.com wrote:
2014-08-12 16:28 GMT-03:00 Chris Wilson ch...@chris-wilson.co.uk:
On Tue, Aug 12, 2014 at 04:12:38PM -0300, Paulo Zanoni wrote:
But we just get/put RPM around this function, not for the whole time
while the object is pinned.
   
Ah misread, saw pin-get, unpin-put and assumed the symmetry. But why
unpin then? It doesn't touch any hardware registers.
   
Only because Daniel asked it on a conversation we had on IRC, and I
automatically assumed the patch would be rejected if I didn't include
it :)
   
Since both you and VIlle pointed that out, I should probably submit
yet another version, without the unpin part, and let Daniel choose
which one to merge...
  
   Hm, I've indeed forgotten about the lazy unbinding. But that poses the
   question about the final bo unref. For example:
   1) create bo, gtt mmap it to force it into existence (and into the 
   global gtt)
   2) unmap binding
   3) wait for rpm entry
   4) unref bo ... causing pte writes for the global gtt unbinding while
   runtime suspended or not?
  
   boom or not boom?
  
   Maybe the bug is simply in a different function ;-)
  
   Yes. If you get serious about it, you will want to move the lazy stuff
   into its own workqueue to be run the next time the device is awake.
  
  4b) shrinker happens and unbinds (potentially purgeable) buffer objects.
  
  In that case I don't think the core mm would be happy if we'd
  indefinitely delay this until someone wiggles the mouse.
 
 You underestimate just how much we can delay it ;-) But for your next
 trick, you could unbind the buffer without touching the ptes since the
 gpu is not using those pages... Diminishing returns I guess.

That's actually something I've considered - on gen6+ we don't use the
global gtt any more for rendering, so it's fully isolated from whatever
userspace can get at. Well ignoring an icky regression from full ppgtt for
the aliasing ppgtt binding rules.

So we /could/ just leave the stale pte hanging in the air forever. But I'm
not sure whether we want to do that for general robustness reasons.
Clearing all ptes on device wake-up isn't an option since it takes too
much time, and delaying the clearing doesn't look like worth it from a
complexity pov.

  Especially if
  the compositor wants that memory to render the frame it needs to
  switch everything on again ...
 
 But's true without rpm anyway. It would need to enable the device to
 render, whether or not the system is thrashing.

Yeah, which is also why I don't think just waking the device in the 
shrinker/bo_free
callback is harmful - very likely we didn't wake it for nothing anyway. Oh
any my scenario can be fixed with some software rendering into an Xshm or
so, and if you assume something else running overnight has thrashed all
the memory to swap.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Ville Syrjälä]

On Mon, Aug 11, 2014 at 02:57:44PM -0300, Paulo Zanoni wrote:
 2014-08-11 11:42 GMT-03:00 Ville Syrjälä ville.syrj...@linux.intel.com:
  On Mon, Aug 11, 2014 at 11:29:21AM -0300, Paulo Zanoni wrote:
  2014-08-11 8:32 GMT-03:00 Ville Syrjälä ville.syrj...@linux.intel.com:
   On Wed, Aug 06, 2014 at 06:26:01PM -0300, Paulo Zanoni wrote:
   From: Paulo Zanoni paulo.r.zan...@intel.com
  
   If we're runtime suspended and try to use the plane interfaces, we
   will get a lot of WARNs saying we did the wrong thing.
  
   For intel_crtc_update_cursor(), all we need to do is return if the
   CRTC is not active, since writing the registers won't really have any
   effect if the screen is not visible, and we will write the registers
   later when enabling the screen.
  
   For all the other cases, we need to get runtime PM references to
   pin/unpin the objects, and to change the fences. The pin/unpin
   functions are the ideal places for this, but
   intel_crtc_cursor_set_obj() doesn't call them, so we also have to add
   get/put calls inside it. There is no problem if we runtime suspend
   right after these functions are finished, because the registers
   weitten are forwarded to system memory.
  
   v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
   v3: - Make get/put also surround the fence and unpin calls (Daniel and
 Ville).
   - Merge all the plane changes into a single patch since they're
 the same fix.
   - Add the comment requested by Daniel.
  
   Testcase: igt/pm_rpm/cursor
   Testcase: igt/pm_rpm/cursor-dpms
   Testcase: igt/pm_rpm/legacy-planes
   Testcase: igt/pm_rpm/legacy-planes-dpms
   Testcase: igt/pm_rpm/universal-planes
   Testcase: igt/pm_rpm/universal-planes-dpms
   Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
   Cc: sta...@vger.kernel.org
   Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
   ---
drivers/gpu/drm/i915/intel_display.c | 39 
   +++-
1 file changed, 38 insertions(+), 1 deletion(-)
  
   diff --git a/drivers/gpu/drm/i915/intel_display.c 
   b/drivers/gpu/drm/i915/intel_display.c
   index 4f659eb..a86d67c 100644
   --- a/drivers/gpu/drm/i915/intel_display.c
   +++ b/drivers/gpu/drm/i915/intel_display.c
   @@ -2212,6 +2212,15 @@ intel_pin_and_fence_fb_obj(struct drm_device 
   *dev,
 if (need_vtd_wa(dev)  alignment  256 * 1024)
 alignment = 256 * 1024;
  
   + /*
   +  * Global gtt pte registers are special registers which actually 
   forward
   +  * writes to a chunk of system memory. Which means that there is 
   no risk
   +  * that the register values disappear as soon as we call
   +  * intel_runtime_pm_put(), so it is correct to wrap only the
   +  * pin/unpin/fence and not more.
   +  */
   + intel_runtime_pm_get(dev_priv);
   +
 dev_priv-mm.interruptible = false;
 ret = i915_gem_object_pin_to_display_plane(obj, alignment, 
   pipelined);
 if (ret)
   @@ -2229,21 +2238,30 @@ intel_pin_and_fence_fb_obj(struct drm_device 
   *dev,
 i915_gem_object_pin_fence(obj);
  
 dev_priv-mm.interruptible = true;
   + intel_runtime_pm_put(dev_priv);
 return 0;
  
err_unpin:
 i915_gem_object_unpin_from_display_plane(obj);
err_interruptible:
 dev_priv-mm.interruptible = true;
   + intel_runtime_pm_put(dev_priv);
 return ret;
}
  
void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
{
   - WARN_ON(!mutex_is_locked(obj-base.dev-struct_mutex));
   + struct drm_device *dev = obj-base.dev;
   + struct drm_i915_private *dev_priv = dev-dev_private;
   +
   + WARN_ON(!mutex_is_locked(dev-struct_mutex));
   +
   + intel_runtime_pm_get(dev_priv);
  
 i915_gem_object_unpin_fence(obj);
 i915_gem_object_unpin_from_display_plane(obj);
   +
   + intel_runtime_pm_put(dev_priv);
  
   I don't think we touch the hardware during unpin so these aren't
   strictly needed.
  
 
  Daniel requested them.
 
 
}
  
/* Computes the linear offset to the base tile and adjusts x, y. bytes 
   per pixel
   @@ -8285,6 +8303,9 @@ static void intel_crtc_update_cursor(struct 
   drm_crtc *crtc,
 if (base == 0  intel_crtc-cursor_base == 0)
 return;
  
   + if (!intel_crtc-active)
   + return;
   +
  
   Did you actually manage to get by the base==0 check above with a
   disabled pipe? I don't think this should happen.
 
  Yes, since we enabled runtime suspend during DPMS. Remember that
  crtc-active != crtc-enabled.
 
  Then I think there's a bug somewhere a bit earlier. We should consider
  the cursor to be invisible when crtc-active==false. That's how we deal
  with all other planes.
 
 Why? I am not very familiar with the cursor code and I don't see
 what's the problem here.

Just feels like duct tape for something that should have been caught
by some earlier piece of code.

[Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Paulo Zanoni]

From: Paulo Zanoni paulo.r.zan...@intel.com

If we're runtime suspended and try to use the plane interfaces, we
will get a lot of WARNs saying we did the wrong thing.

We need to get runtime PM references to pin/unpin the objects, and to
change the fences. The pin/unpin functions are the ideal places for
this, but intel_crtc_cursor_set_obj() doesn't call them, so we also
have to add get/put calls inside it. There is no problem if we runtime
suspend right after these functions are finished, because the
registers written are forwarded to system memory.

Note: for a complete fix of the cursor-dpms test case, we also need
the patch named drm/i915: Don't try to enable cursor from setplane
when crtc is disabled.

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
  Ville).
- Merge all the plane changes into a single patch since they're
  the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).
v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
  equivalent fix in another patch (Ville).

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
Cc: sta...@vger.kernel.org
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
---
 drivers/gpu/drm/i915/intel_display.c | 35 ++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index a1cf052..2db9e06 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
if (need_vtd_wa(dev)  alignment  256 * 1024)
alignment = 256 * 1024;
 
+   /*
+* Global gtt pte registers are special registers which actually forward
+* writes to a chunk of system memory. Which means that there is no risk
+* that the register values disappear as soon as we call
+* intel_runtime_pm_put(), so it is correct to wrap only the
+* pin/unpin/fence and not more.
+*/
+   intel_runtime_pm_get(dev_priv);
+
dev_priv-mm.interruptible = false;
ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
if (ret)
@@ -2166,21 +2175,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
i915_gem_object_pin_fence(obj);
 
dev_priv-mm.interruptible = true;
+   intel_runtime_pm_put(dev_priv);
return 0;
 
 err_unpin:
i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
dev_priv-mm.interruptible = true;
+   intel_runtime_pm_put(dev_priv);
return ret;
 }
 
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
 {
-   WARN_ON(!mutex_is_locked(obj-base.dev-struct_mutex));
+   struct drm_device *dev = obj-base.dev;
+   struct drm_i915_private *dev_priv = dev-dev_private;
+
+   WARN_ON(!mutex_is_locked(dev-struct_mutex));
+
+   intel_runtime_pm_get(dev_priv);
 
i915_gem_object_unpin_fence(obj);
i915_gem_object_unpin_from_display_plane(obj);
+
+   intel_runtime_pm_put(dev_priv);
 }
 
 /* Computes the linear offset to the base tile and adjusts x, y. bytes per 
pixel
@@ -8170,6 +8188,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
 
/* we only need to pin inside GTT if cursor is non-phy */
mutex_lock(dev-struct_mutex);
+
+   /*
+* Global gtt pte registers are special registers which actually forward
+* writes to a chunk of system memory. Which means that there is no risk
+* that the register values disappear as soon as we call
+* intel_runtime_pm_put(), so it is correct to wrap only the
+* pin/unpin/fence and not more.
+*/
+   intel_runtime_pm_get(dev_priv);
+
if (!INTEL_INFO(dev)-cursor_needs_physical) {
unsigned alignment;
 
@@ -8219,6 +8247,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
 
i915_gem_track_fb(intel_crtc-cursor_bo, obj,
  INTEL_FRONTBUFFER_CURSOR(pipe));
+
+   if (obj)
+   intel_runtime_pm_put(dev_priv);
+
mutex_unlock(dev-struct_mutex);
 
old_width = intel_crtc-cursor_width;
@@ -8240,6 +8272,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
 fail_unpin:
i915_gem_object_unpin_from_display_plane(obj);
 fail_locked:
+   intel_runtime_pm_put(dev_priv);
mutex_unlock(dev-struct_mutex);
 fail:
drm_gem_object_unreference_unlocked(obj-base);
-- 
2.0.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Chris Wilson]

On Tue, Aug 12, 2014 at 03:55:12PM -0300, Paulo Zanoni wrote:
 From: Paulo Zanoni paulo.r.zan...@intel.com
 
 If we're runtime suspended and try to use the plane interfaces, we
 will get a lot of WARNs saying we did the wrong thing.
 
 We need to get runtime PM references to pin/unpin the objects, and to
 change the fences. The pin/unpin functions are the ideal places for
 this, but intel_crtc_cursor_set_obj() doesn't call them, so we also
 have to add get/put calls inside it. There is no problem if we runtime
 suspend right after these functions are finished, because the
 registers written are forwarded to system memory.
 
 Note: for a complete fix of the cursor-dpms test case, we also need
 the patch named drm/i915: Don't try to enable cursor from setplane
 when crtc is disabled.
 
 v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
 v3: - Make get/put also surround the fence and unpin calls (Daniel and
   Ville).
 - Merge all the plane changes into a single patch since they're
   the same fix.
 - Add the comment requested by Daniel.
 v4: - Remove spurious whitespace (Ville).
 v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
   equivalent fix in another patch (Ville).
 
 Testcase: igt/pm_rpm/cursor
 Testcase: igt/pm_rpm/cursor-dpms
 Testcase: igt/pm_rpm/legacy-planes
 Testcase: igt/pm_rpm/legacy-planes-dpms
 Testcase: igt/pm_rpm/universal-planes
 Testcase: igt/pm_rpm/universal-planes-dpms
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
 Cc: sta...@vger.kernel.org
 Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
 ---
  drivers/gpu/drm/i915/intel_display.c | 35 ++-
  1 file changed, 34 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index a1cf052..2db9e06 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   if (need_vtd_wa(dev)  alignment  256 * 1024)
   alignment = 256 * 1024;
  
 + /*
 +  * Global gtt pte registers are special registers which actually forward
 +  * writes to a chunk of system memory. Which means that there is no risk
 +  * that the register values disappear as soon as we call
 +  * intel_runtime_pm_put(), so it is correct to wrap only the
 +  * pin/unpin/fence and not more.
 +  */
 + intel_runtime_pm_get(dev_priv);
 +
   dev_priv-mm.interruptible = false;
   ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
   if (ret)
 @@ -2166,21 +2175,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   i915_gem_object_pin_fence(obj);
  
   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return 0;
  
  err_unpin:
   i915_gem_object_unpin_from_display_plane(obj);
  err_interruptible:
   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return ret;
  }
  
  void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
  {
 - WARN_ON(!mutex_is_locked(obj-base.dev-struct_mutex));
 + struct drm_device *dev = obj-base.dev;
 + struct drm_i915_private *dev_priv = dev-dev_private;
 +
 + WARN_ON(!mutex_is_locked(dev-struct_mutex));
 +
 + intel_runtime_pm_get(dev_priv);
  
   i915_gem_object_unpin_fence(obj);
   i915_gem_object_unpin_from_display_plane(obj);
 +
 + intel_runtime_pm_put(dev_priv);
  }

framebuffer objects are pinned for a very long time, and the fbcon is
permanently pinned. This should have the effect of disabling rpm
entirely.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Paulo Zanoni]

2014-08-12 16:09 GMT-03:00 Chris Wilson ch...@chris-wilson.co.uk:
 On Tue, Aug 12, 2014 at 03:55:12PM -0300, Paulo Zanoni wrote:
 From: Paulo Zanoni paulo.r.zan...@intel.com

 If we're runtime suspended and try to use the plane interfaces, we
 will get a lot of WARNs saying we did the wrong thing.

 We need to get runtime PM references to pin/unpin the objects, and to
 change the fences. The pin/unpin functions are the ideal places for
 this, but intel_crtc_cursor_set_obj() doesn't call them, so we also
 have to add get/put calls inside it. There is no problem if we runtime
 suspend right after these functions are finished, because the
 registers written are forwarded to system memory.

 Note: for a complete fix of the cursor-dpms test case, we also need
 the patch named drm/i915: Don't try to enable cursor from setplane
 when crtc is disabled.

 v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
 v3: - Make get/put also surround the fence and unpin calls (Daniel and
   Ville).
 - Merge all the plane changes into a single patch since they're
   the same fix.
 - Add the comment requested by Daniel.
 v4: - Remove spurious whitespace (Ville).
 v5: - Remove intel_crtc_update_cursor() chunk since Ville did an
   equivalent fix in another patch (Ville).

 Testcase: igt/pm_rpm/cursor
 Testcase: igt/pm_rpm/cursor-dpms
 Testcase: igt/pm_rpm/legacy-planes
 Testcase: igt/pm_rpm/legacy-planes-dpms
 Testcase: igt/pm_rpm/universal-planes
 Testcase: igt/pm_rpm/universal-planes-dpms
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
 Cc: sta...@vger.kernel.org
 Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
 ---
  drivers/gpu/drm/i915/intel_display.c | 35 
 ++-
  1 file changed, 34 insertions(+), 1 deletion(-)

 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index a1cf052..2db9e06 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   if (need_vtd_wa(dev)  alignment  256 * 1024)
   alignment = 256 * 1024;

 + /*
 +  * Global gtt pte registers are special registers which actually 
 forward
 +  * writes to a chunk of system memory. Which means that there is no 
 risk
 +  * that the register values disappear as soon as we call
 +  * intel_runtime_pm_put(), so it is correct to wrap only the
 +  * pin/unpin/fence and not more.
 +  */
 + intel_runtime_pm_get(dev_priv);
 +
   dev_priv-mm.interruptible = false;
   ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
   if (ret)
 @@ -2166,21 +2175,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   i915_gem_object_pin_fence(obj);

   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return 0;

  err_unpin:
   i915_gem_object_unpin_from_display_plane(obj);
  err_interruptible:
   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return ret;
  }

  void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
  {
 - WARN_ON(!mutex_is_locked(obj-base.dev-struct_mutex));
 + struct drm_device *dev = obj-base.dev;
 + struct drm_i915_private *dev_priv = dev-dev_private;
 +
 + WARN_ON(!mutex_is_locked(dev-struct_mutex));
 +
 + intel_runtime_pm_get(dev_priv);

   i915_gem_object_unpin_fence(obj);
   i915_gem_object_unpin_from_display_plane(obj);
 +
 + intel_runtime_pm_put(dev_priv);
  }

 framebuffer objects are pinned for a very long time, and the fbcon is
 permanently pinned. This should have the effect of disabling rpm
 entirely.

But we just get/put RPM around this function, not for the whole time
while the object is pinned.

 -Chris

 --
 Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Chris Wilson]

On Tue, Aug 12, 2014 at 04:12:38PM -0300, Paulo Zanoni wrote:
 But we just get/put RPM around this function, not for the whole time
 while the object is pinned.

Ah misread, saw pin-get, unpin-put and assumed the symmetry. But why
unpin then? It doesn't touch any hardware registers.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Paulo Zanoni]

2014-08-12 16:28 GMT-03:00 Chris Wilson ch...@chris-wilson.co.uk:
 On Tue, Aug 12, 2014 at 04:12:38PM -0300, Paulo Zanoni wrote:
 But we just get/put RPM around this function, not for the whole time
 while the object is pinned.

 Ah misread, saw pin-get, unpin-put and assumed the symmetry. But why
 unpin then? It doesn't touch any hardware registers.

Only because Daniel asked it on a conversation we had on IRC, and I
automatically assumed the patch would be rejected if I didn't include
it :)

Since both you and VIlle pointed that out, I should probably submit
yet another version, without the unpin part, and let Daniel choose
which one to merge...

 -Chris

 --
 Chris Wilson, Intel Open Source Technology Centre



-- 
Paulo Zanoni
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Daniel Vetter]

On Tue, Aug 12, 2014 at 9:33 PM, Paulo Zanoni przan...@gmail.com wrote:
 2014-08-12 16:28 GMT-03:00 Chris Wilson ch...@chris-wilson.co.uk:
 On Tue, Aug 12, 2014 at 04:12:38PM -0300, Paulo Zanoni wrote:
 But we just get/put RPM around this function, not for the whole time
 while the object is pinned.

 Ah misread, saw pin-get, unpin-put and assumed the symmetry. But why
 unpin then? It doesn't touch any hardware registers.

 Only because Daniel asked it on a conversation we had on IRC, and I
 automatically assumed the patch would be rejected if I didn't include
 it :)

 Since both you and VIlle pointed that out, I should probably submit
 yet another version, without the unpin part, and let Daniel choose
 which one to merge...

Hm, I've indeed forgotten about the lazy unbinding. But that poses the
question about the final bo unref. For example:
1) create bo, gtt mmap it to force it into existence (and into the global gtt)
2) unmap binding
3) wait for rpm entry
4) unref bo ... causing pte writes for the global gtt unbinding while
runtime suspended or not?

boom or not boom?

Maybe the bug is simply in a different function ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Chris Wilson]

On Tue, Aug 12, 2014 at 10:19:20PM +0200, Daniel Vetter wrote:
 On Tue, Aug 12, 2014 at 9:33 PM, Paulo Zanoni przan...@gmail.com wrote:
  2014-08-12 16:28 GMT-03:00 Chris Wilson ch...@chris-wilson.co.uk:
  On Tue, Aug 12, 2014 at 04:12:38PM -0300, Paulo Zanoni wrote:
  But we just get/put RPM around this function, not for the whole time
  while the object is pinned.
 
  Ah misread, saw pin-get, unpin-put and assumed the symmetry. But why
  unpin then? It doesn't touch any hardware registers.
 
  Only because Daniel asked it on a conversation we had on IRC, and I
  automatically assumed the patch would be rejected if I didn't include
  it :)
 
  Since both you and VIlle pointed that out, I should probably submit
  yet another version, without the unpin part, and let Daniel choose
  which one to merge...
 
 Hm, I've indeed forgotten about the lazy unbinding. But that poses the
 question about the final bo unref. For example:
 1) create bo, gtt mmap it to force it into existence (and into the global gtt)
 2) unmap binding
 3) wait for rpm entry
 4) unref bo ... causing pte writes for the global gtt unbinding while
 runtime suspended or not?
 
 boom or not boom?
 
 Maybe the bug is simply in a different function ;-)

Yes. If you get serious about it, you will want to move the lazy stuff
into its own workqueue to be run the next time the device is awake.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Daniel Vetter]

On Tue, Aug 12, 2014 at 10:30 PM, Chris Wilson ch...@chris-wilson.co.uk wrote:
 On Tue, Aug 12, 2014 at 10:19:20PM +0200, Daniel Vetter wrote:
 On Tue, Aug 12, 2014 at 9:33 PM, Paulo Zanoni przan...@gmail.com wrote:
  2014-08-12 16:28 GMT-03:00 Chris Wilson ch...@chris-wilson.co.uk:
  On Tue, Aug 12, 2014 at 04:12:38PM -0300, Paulo Zanoni wrote:
  But we just get/put RPM around this function, not for the whole time
  while the object is pinned.
 
  Ah misread, saw pin-get, unpin-put and assumed the symmetry. But why
  unpin then? It doesn't touch any hardware registers.
 
  Only because Daniel asked it on a conversation we had on IRC, and I
  automatically assumed the patch would be rejected if I didn't include
  it :)
 
  Since both you and VIlle pointed that out, I should probably submit
  yet another version, without the unpin part, and let Daniel choose
  which one to merge...

 Hm, I've indeed forgotten about the lazy unbinding. But that poses the
 question about the final bo unref. For example:
 1) create bo, gtt mmap it to force it into existence (and into the global 
 gtt)
 2) unmap binding
 3) wait for rpm entry
 4) unref bo ... causing pte writes for the global gtt unbinding while
 runtime suspended or not?

 boom or not boom?

 Maybe the bug is simply in a different function ;-)

 Yes. If you get serious about it, you will want to move the lazy stuff
 into its own workqueue to be run the next time the device is awake.

4b) shrinker happens and unbinds (potentially purgeable) buffer objects.

In that case I don't think the core mm would be happy if we'd
indefinitely delay this until someone wiggles the mouse. Especially if
the compositor wants that memory to render the frame it needs to
switch everything on again ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Chris Wilson]

On Tue, Aug 12, 2014 at 10:37:21PM +0200, Daniel Vetter wrote:
 On Tue, Aug 12, 2014 at 10:30 PM, Chris Wilson ch...@chris-wilson.co.uk 
 wrote:
  On Tue, Aug 12, 2014 at 10:19:20PM +0200, Daniel Vetter wrote:
  On Tue, Aug 12, 2014 at 9:33 PM, Paulo Zanoni przan...@gmail.com wrote:
   2014-08-12 16:28 GMT-03:00 Chris Wilson ch...@chris-wilson.co.uk:
   On Tue, Aug 12, 2014 at 04:12:38PM -0300, Paulo Zanoni wrote:
   But we just get/put RPM around this function, not for the whole time
   while the object is pinned.
  
   Ah misread, saw pin-get, unpin-put and assumed the symmetry. But why
   unpin then? It doesn't touch any hardware registers.
  
   Only because Daniel asked it on a conversation we had on IRC, and I
   automatically assumed the patch would be rejected if I didn't include
   it :)
  
   Since both you and VIlle pointed that out, I should probably submit
   yet another version, without the unpin part, and let Daniel choose
   which one to merge...
 
  Hm, I've indeed forgotten about the lazy unbinding. But that poses the
  question about the final bo unref. For example:
  1) create bo, gtt mmap it to force it into existence (and into the global 
  gtt)
  2) unmap binding
  3) wait for rpm entry
  4) unref bo ... causing pte writes for the global gtt unbinding while
  runtime suspended or not?
 
  boom or not boom?
 
  Maybe the bug is simply in a different function ;-)
 
  Yes. If you get serious about it, you will want to move the lazy stuff
  into its own workqueue to be run the next time the device is awake.
 
 4b) shrinker happens and unbinds (potentially purgeable) buffer objects.
 
 In that case I don't think the core mm would be happy if we'd
 indefinitely delay this until someone wiggles the mouse.

You underestimate just how much we can delay it ;-) But for your next
trick, you could unbind the buffer without touching the ptes since the
gpu is not using those pages... Diminishing returns I guess.

 Especially if
 the compositor wants that memory to render the frame it needs to
 switch everything on again ...

But's true without rpm anyway. It would need to enable the device to
render, whether or not the system is thrashing.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Ville Syrjälä]

On Wed, Aug 06, 2014 at 06:26:01PM -0300, Paulo Zanoni wrote:
 From: Paulo Zanoni paulo.r.zan...@intel.com
 
 If we're runtime suspended and try to use the plane interfaces, we
 will get a lot of WARNs saying we did the wrong thing.
 
 For intel_crtc_update_cursor(), all we need to do is return if the
 CRTC is not active, since writing the registers won't really have any
 effect if the screen is not visible, and we will write the registers
 later when enabling the screen.
 
 For all the other cases, we need to get runtime PM references to
 pin/unpin the objects, and to change the fences. The pin/unpin
 functions are the ideal places for this, but
 intel_crtc_cursor_set_obj() doesn't call them, so we also have to add
 get/put calls inside it. There is no problem if we runtime suspend
 right after these functions are finished, because the registers
 weitten are forwarded to system memory.
 
 v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
 v3: - Make get/put also surround the fence and unpin calls (Daniel and
   Ville).
 - Merge all the plane changes into a single patch since they're
   the same fix.
 - Add the comment requested by Daniel.
 
 Testcase: igt/pm_rpm/cursor
 Testcase: igt/pm_rpm/cursor-dpms
 Testcase: igt/pm_rpm/legacy-planes
 Testcase: igt/pm_rpm/legacy-planes-dpms
 Testcase: igt/pm_rpm/universal-planes
 Testcase: igt/pm_rpm/universal-planes-dpms
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
 Cc: sta...@vger.kernel.org
 Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
 ---
  drivers/gpu/drm/i915/intel_display.c | 39 
 +++-
  1 file changed, 38 insertions(+), 1 deletion(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index 4f659eb..a86d67c 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -2212,6 +2212,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   if (need_vtd_wa(dev)  alignment  256 * 1024)
   alignment = 256 * 1024;
  
 + /*
 +  * Global gtt pte registers are special registers which actually forward
 +  * writes to a chunk of system memory. Which means that there is no risk
 +  * that the register values disappear as soon as we call
 +  * intel_runtime_pm_put(), so it is correct to wrap only the
 +  * pin/unpin/fence and not more.
 +  */
 + intel_runtime_pm_get(dev_priv);
 +
   dev_priv-mm.interruptible = false;
   ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
   if (ret)
 @@ -2229,21 +2238,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   i915_gem_object_pin_fence(obj);
  
   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return 0;
  
  err_unpin:
   i915_gem_object_unpin_from_display_plane(obj);
  err_interruptible:
   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return ret;
  }
  
  void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
  {
 - WARN_ON(!mutex_is_locked(obj-base.dev-struct_mutex));
 + struct drm_device *dev = obj-base.dev;
 + struct drm_i915_private *dev_priv = dev-dev_private;
 +
 + WARN_ON(!mutex_is_locked(dev-struct_mutex));
 +
 + intel_runtime_pm_get(dev_priv);
  
   i915_gem_object_unpin_fence(obj);
   i915_gem_object_unpin_from_display_plane(obj);
 +
 + intel_runtime_pm_put(dev_priv);

I don't think we touch the hardware during unpin so these aren't
strictly needed.

  }
  
  /* Computes the linear offset to the base tile and adjusts x, y. bytes per 
 pixel
 @@ -8285,6 +8303,9 @@ static void intel_crtc_update_cursor(struct drm_crtc 
 *crtc,
   if (base == 0  intel_crtc-cursor_base == 0)
   return;
  
 + if (!intel_crtc-active)
 + return;
 +

Did you actually manage to get by the base==0 check above with a
disabled pipe? I don't think this should happen.

   I915_WRITE(CURPOS(pipe), pos);
  
   if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
 @@ -8340,9 +8361,20 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,
  
   /* we only need to pin inside GTT if cursor is non-phy */
   mutex_lock(dev-struct_mutex);
 +
 + /*
 +  * Global gtt pte registers are special registers which actually forward
 +  * writes to a chunk of system memory. Which means that there is no risk
 +  * that the register values disappear as soon as we call
 +  * intel_runtime_pm_put(), so it is correct to wrap only the
 +  * pin/unpin/fence and not more.
 +  */
 + intel_runtime_pm_get(dev_priv);
 +
   if (!INTEL_INFO(dev)-cursor_needs_physical) {
   unsigned alignment;
  
 +

Spurious whitespace

And now that I look at our cursor code I see we're doing dubious stuff
like unpinning the old bo before even writing the cursor registers to
use the new bo (really we should 

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Paulo Zanoni]

2014-08-11 8:32 GMT-03:00 Ville Syrjälä ville.syrj...@linux.intel.com:
 On Wed, Aug 06, 2014 at 06:26:01PM -0300, Paulo Zanoni wrote:
 From: Paulo Zanoni paulo.r.zan...@intel.com

 If we're runtime suspended and try to use the plane interfaces, we
 will get a lot of WARNs saying we did the wrong thing.

 For intel_crtc_update_cursor(), all we need to do is return if the
 CRTC is not active, since writing the registers won't really have any
 effect if the screen is not visible, and we will write the registers
 later when enabling the screen.

 For all the other cases, we need to get runtime PM references to
 pin/unpin the objects, and to change the fences. The pin/unpin
 functions are the ideal places for this, but
 intel_crtc_cursor_set_obj() doesn't call them, so we also have to add
 get/put calls inside it. There is no problem if we runtime suspend
 right after these functions are finished, because the registers
 weitten are forwarded to system memory.

 v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
 v3: - Make get/put also surround the fence and unpin calls (Daniel and
   Ville).
 - Merge all the plane changes into a single patch since they're
   the same fix.
 - Add the comment requested by Daniel.

 Testcase: igt/pm_rpm/cursor
 Testcase: igt/pm_rpm/cursor-dpms
 Testcase: igt/pm_rpm/legacy-planes
 Testcase: igt/pm_rpm/legacy-planes-dpms
 Testcase: igt/pm_rpm/universal-planes
 Testcase: igt/pm_rpm/universal-planes-dpms
 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
 Cc: sta...@vger.kernel.org
 Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
 ---
  drivers/gpu/drm/i915/intel_display.c | 39 
 +++-
  1 file changed, 38 insertions(+), 1 deletion(-)

 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index 4f659eb..a86d67c 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -2212,6 +2212,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   if (need_vtd_wa(dev)  alignment  256 * 1024)
   alignment = 256 * 1024;

 + /*
 +  * Global gtt pte registers are special registers which actually 
 forward
 +  * writes to a chunk of system memory. Which means that there is no 
 risk
 +  * that the register values disappear as soon as we call
 +  * intel_runtime_pm_put(), so it is correct to wrap only the
 +  * pin/unpin/fence and not more.
 +  */
 + intel_runtime_pm_get(dev_priv);
 +
   dev_priv-mm.interruptible = false;
   ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
   if (ret)
 @@ -2229,21 +2238,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
   i915_gem_object_pin_fence(obj);

   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return 0;

  err_unpin:
   i915_gem_object_unpin_from_display_plane(obj);
  err_interruptible:
   dev_priv-mm.interruptible = true;
 + intel_runtime_pm_put(dev_priv);
   return ret;
  }

  void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
  {
 - WARN_ON(!mutex_is_locked(obj-base.dev-struct_mutex));
 + struct drm_device *dev = obj-base.dev;
 + struct drm_i915_private *dev_priv = dev-dev_private;
 +
 + WARN_ON(!mutex_is_locked(dev-struct_mutex));
 +
 + intel_runtime_pm_get(dev_priv);

   i915_gem_object_unpin_fence(obj);
   i915_gem_object_unpin_from_display_plane(obj);
 +
 + intel_runtime_pm_put(dev_priv);

 I don't think we touch the hardware during unpin so these aren't
 strictly needed.


Daniel requested them.


  }

  /* Computes the linear offset to the base tile and adjusts x, y. bytes per 
 pixel
 @@ -8285,6 +8303,9 @@ static void intel_crtc_update_cursor(struct drm_crtc 
 *crtc,
   if (base == 0  intel_crtc-cursor_base == 0)
   return;

 + if (!intel_crtc-active)
 + return;
 +

 Did you actually manage to get by the base==0 check above with a
 disabled pipe? I don't think this should happen.

Yes, since we enabled runtime suspend during DPMS. Remember that
crtc-active != crtc-enabled.

To hit this return, just run sudo ./pm_rpm --run-subtest cursor-dpms.



   I915_WRITE(CURPOS(pipe), pos);

   if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
 @@ -8340,9 +8361,20 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
 *crtc,

   /* we only need to pin inside GTT if cursor is non-phy */
   mutex_lock(dev-struct_mutex);
 +
 + /*
 +  * Global gtt pte registers are special registers which actually 
 forward
 +  * writes to a chunk of system memory. Which means that there is no 
 risk
 +  * that the register values disappear as soon as we call
 +  * intel_runtime_pm_put(), so it is correct to wrap only the
 +  * pin/unpin/fence and not more.
 +  */
 + intel_runtime_pm_get(dev_priv);
 +
   if 

[Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Paulo Zanoni]

From: Paulo Zanoni paulo.r.zan...@intel.com

If we're runtime suspended and try to use the plane interfaces, we
will get a lot of WARNs saying we did the wrong thing.

For intel_crtc_update_cursor(), all we need to do is return if the
CRTC is not active, since writing the registers won't really have any
effect if the screen is not visible, and we will write the registers
later when enabling the screen.

For all the other cases, we need to get runtime PM references to
pin/unpin the objects, and to change the fences. The pin/unpin
functions are the ideal places for this, but
intel_crtc_cursor_set_obj() doesn't call them, so we also have to add
get/put calls inside it. There is no problem if we runtime suspend
right after these functions are finished, because the registers
weitten are forwarded to system memory.

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
  Ville).
- Merge all the plane changes into a single patch since they're
  the same fix.
- Add the comment requested by Daniel.
v4: - Remove spurious whitespace (Ville).

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
Cc: sta...@vger.kernel.org
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
---
 drivers/gpu/drm/i915/intel_display.c | 38 +++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index e9b578e..4e1c957 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2149,6 +2149,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
if (need_vtd_wa(dev)  alignment  256 * 1024)
alignment = 256 * 1024;
 
+   /*
+* Global gtt pte registers are special registers which actually forward
+* writes to a chunk of system memory. Which means that there is no risk
+* that the register values disappear as soon as we call
+* intel_runtime_pm_put(), so it is correct to wrap only the
+* pin/unpin/fence and not more.
+*/
+   intel_runtime_pm_get(dev_priv);
+
dev_priv-mm.interruptible = false;
ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
if (ret)
@@ -2166,21 +2175,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
i915_gem_object_pin_fence(obj);
 
dev_priv-mm.interruptible = true;
+   intel_runtime_pm_put(dev_priv);
return 0;
 
 err_unpin:
i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
dev_priv-mm.interruptible = true;
+   intel_runtime_pm_put(dev_priv);
return ret;
 }
 
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
 {
-   WARN_ON(!mutex_is_locked(obj-base.dev-struct_mutex));
+   struct drm_device *dev = obj-base.dev;
+   struct drm_i915_private *dev_priv = dev-dev_private;
+
+   WARN_ON(!mutex_is_locked(dev-struct_mutex));
+
+   intel_runtime_pm_get(dev_priv);
 
i915_gem_object_unpin_fence(obj);
i915_gem_object_unpin_from_display_plane(obj);
+
+   intel_runtime_pm_put(dev_priv);
 }
 
 /* Computes the linear offset to the base tile and adjusts x, y. bytes per 
pixel
@@ -8154,6 +8172,9 @@ static void intel_crtc_update_cursor(struct drm_crtc 
*crtc,
if (base == 0  intel_crtc-cursor_base == 0)
return;
 
+   if (!intel_crtc-active)
+   return;
+
I915_WRITE(CURPOS(pipe), pos);
 
if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
@@ -8209,6 +8230,16 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
 
/* we only need to pin inside GTT if cursor is non-phy */
mutex_lock(dev-struct_mutex);
+
+   /*
+* Global gtt pte registers are special registers which actually forward
+* writes to a chunk of system memory. Which means that there is no risk
+* that the register values disappear as soon as we call
+* intel_runtime_pm_put(), so it is correct to wrap only the
+* pin/unpin/fence and not more.
+*/
+   intel_runtime_pm_get(dev_priv);
+
if (!INTEL_INFO(dev)-cursor_needs_physical) {
unsigned alignment;
 
@@ -8261,6 +8292,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
 
i915_gem_track_fb(intel_crtc-cursor_bo, obj,
  INTEL_FRONTBUFFER_CURSOR(pipe));
+
+   if (obj)
+   intel_runtime_pm_put(dev_priv);
+
mutex_unlock(dev-struct_mutex);
 
old_width = intel_crtc-cursor_width;
@@ -8282,6 +8317,7 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
 fail_unpin:
  

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Ville Syrjälä]

On Mon, Aug 11, 2014 at 11:29:21AM -0300, Paulo Zanoni wrote:
 2014-08-11 8:32 GMT-03:00 Ville Syrjälä ville.syrj...@linux.intel.com:
  On Wed, Aug 06, 2014 at 06:26:01PM -0300, Paulo Zanoni wrote:
  From: Paulo Zanoni paulo.r.zan...@intel.com
 
  If we're runtime suspended and try to use the plane interfaces, we
  will get a lot of WARNs saying we did the wrong thing.
 
  For intel_crtc_update_cursor(), all we need to do is return if the
  CRTC is not active, since writing the registers won't really have any
  effect if the screen is not visible, and we will write the registers
  later when enabling the screen.
 
  For all the other cases, we need to get runtime PM references to
  pin/unpin the objects, and to change the fences. The pin/unpin
  functions are the ideal places for this, but
  intel_crtc_cursor_set_obj() doesn't call them, so we also have to add
  get/put calls inside it. There is no problem if we runtime suspend
  right after these functions are finished, because the registers
  weitten are forwarded to system memory.
 
  v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
  v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
  - Merge all the plane changes into a single patch since they're
the same fix.
  - Add the comment requested by Daniel.
 
  Testcase: igt/pm_rpm/cursor
  Testcase: igt/pm_rpm/cursor-dpms
  Testcase: igt/pm_rpm/legacy-planes
  Testcase: igt/pm_rpm/legacy-planes-dpms
  Testcase: igt/pm_rpm/universal-planes
  Testcase: igt/pm_rpm/universal-planes-dpms
  Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
  Cc: sta...@vger.kernel.org
  Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
  ---
   drivers/gpu/drm/i915/intel_display.c | 39 
  +++-
   1 file changed, 38 insertions(+), 1 deletion(-)
 
  diff --git a/drivers/gpu/drm/i915/intel_display.c 
  b/drivers/gpu/drm/i915/intel_display.c
  index 4f659eb..a86d67c 100644
  --- a/drivers/gpu/drm/i915/intel_display.c
  +++ b/drivers/gpu/drm/i915/intel_display.c
  @@ -2212,6 +2212,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
if (need_vtd_wa(dev)  alignment  256 * 1024)
alignment = 256 * 1024;
 
  + /*
  +  * Global gtt pte registers are special registers which actually 
  forward
  +  * writes to a chunk of system memory. Which means that there is no 
  risk
  +  * that the register values disappear as soon as we call
  +  * intel_runtime_pm_put(), so it is correct to wrap only the
  +  * pin/unpin/fence and not more.
  +  */
  + intel_runtime_pm_get(dev_priv);
  +
dev_priv-mm.interruptible = false;
ret = i915_gem_object_pin_to_display_plane(obj, alignment, 
  pipelined);
if (ret)
  @@ -2229,21 +2238,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
i915_gem_object_pin_fence(obj);
 
dev_priv-mm.interruptible = true;
  + intel_runtime_pm_put(dev_priv);
return 0;
 
   err_unpin:
i915_gem_object_unpin_from_display_plane(obj);
   err_interruptible:
dev_priv-mm.interruptible = true;
  + intel_runtime_pm_put(dev_priv);
return ret;
   }
 
   void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
   {
  - WARN_ON(!mutex_is_locked(obj-base.dev-struct_mutex));
  + struct drm_device *dev = obj-base.dev;
  + struct drm_i915_private *dev_priv = dev-dev_private;
  +
  + WARN_ON(!mutex_is_locked(dev-struct_mutex));
  +
  + intel_runtime_pm_get(dev_priv);
 
i915_gem_object_unpin_fence(obj);
i915_gem_object_unpin_from_display_plane(obj);
  +
  + intel_runtime_pm_put(dev_priv);
 
  I don't think we touch the hardware during unpin so these aren't
  strictly needed.
 
 
 Daniel requested them.
 
 
   }
 
   /* Computes the linear offset to the base tile and adjusts x, y. bytes 
  per pixel
  @@ -8285,6 +8303,9 @@ static void intel_crtc_update_cursor(struct drm_crtc 
  *crtc,
if (base == 0  intel_crtc-cursor_base == 0)
return;
 
  + if (!intel_crtc-active)
  + return;
  +
 
  Did you actually manage to get by the base==0 check above with a
  disabled pipe? I don't think this should happen.
 
 Yes, since we enabled runtime suspend during DPMS. Remember that
 crtc-active != crtc-enabled.

Then I think there's a bug somewhere a bit earlier. We should consider
the cursor to be invisible when crtc-active==false. That's how we deal
with all other planes.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Paulo Zanoni]

2014-08-11 11:42 GMT-03:00 Ville Syrjälä ville.syrj...@linux.intel.com:
 On Mon, Aug 11, 2014 at 11:29:21AM -0300, Paulo Zanoni wrote:
 2014-08-11 8:32 GMT-03:00 Ville Syrjälä ville.syrj...@linux.intel.com:
  On Wed, Aug 06, 2014 at 06:26:01PM -0300, Paulo Zanoni wrote:
  From: Paulo Zanoni paulo.r.zan...@intel.com
 
  If we're runtime suspended and try to use the plane interfaces, we
  will get a lot of WARNs saying we did the wrong thing.
 
  For intel_crtc_update_cursor(), all we need to do is return if the
  CRTC is not active, since writing the registers won't really have any
  effect if the screen is not visible, and we will write the registers
  later when enabling the screen.
 
  For all the other cases, we need to get runtime PM references to
  pin/unpin the objects, and to change the fences. The pin/unpin
  functions are the ideal places for this, but
  intel_crtc_cursor_set_obj() doesn't call them, so we also have to add
  get/put calls inside it. There is no problem if we runtime suspend
  right after these functions are finished, because the registers
  weitten are forwarded to system memory.
 
  v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
  v3: - Make get/put also surround the fence and unpin calls (Daniel and
Ville).
  - Merge all the plane changes into a single patch since they're
the same fix.
  - Add the comment requested by Daniel.
 
  Testcase: igt/pm_rpm/cursor
  Testcase: igt/pm_rpm/cursor-dpms
  Testcase: igt/pm_rpm/legacy-planes
  Testcase: igt/pm_rpm/legacy-planes-dpms
  Testcase: igt/pm_rpm/universal-planes
  Testcase: igt/pm_rpm/universal-planes-dpms
  Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
  Cc: sta...@vger.kernel.org
  Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
  ---
   drivers/gpu/drm/i915/intel_display.c | 39 
  +++-
   1 file changed, 38 insertions(+), 1 deletion(-)
 
  diff --git a/drivers/gpu/drm/i915/intel_display.c 
  b/drivers/gpu/drm/i915/intel_display.c
  index 4f659eb..a86d67c 100644
  --- a/drivers/gpu/drm/i915/intel_display.c
  +++ b/drivers/gpu/drm/i915/intel_display.c
  @@ -2212,6 +2212,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
if (need_vtd_wa(dev)  alignment  256 * 1024)
alignment = 256 * 1024;
 
  + /*
  +  * Global gtt pte registers are special registers which actually 
  forward
  +  * writes to a chunk of system memory. Which means that there is no 
  risk
  +  * that the register values disappear as soon as we call
  +  * intel_runtime_pm_put(), so it is correct to wrap only the
  +  * pin/unpin/fence and not more.
  +  */
  + intel_runtime_pm_get(dev_priv);
  +
dev_priv-mm.interruptible = false;
ret = i915_gem_object_pin_to_display_plane(obj, alignment, 
  pipelined);
if (ret)
  @@ -2229,21 +2238,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
i915_gem_object_pin_fence(obj);
 
dev_priv-mm.interruptible = true;
  + intel_runtime_pm_put(dev_priv);
return 0;
 
   err_unpin:
i915_gem_object_unpin_from_display_plane(obj);
   err_interruptible:
dev_priv-mm.interruptible = true;
  + intel_runtime_pm_put(dev_priv);
return ret;
   }
 
   void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
   {
  - WARN_ON(!mutex_is_locked(obj-base.dev-struct_mutex));
  + struct drm_device *dev = obj-base.dev;
  + struct drm_i915_private *dev_priv = dev-dev_private;
  +
  + WARN_ON(!mutex_is_locked(dev-struct_mutex));
  +
  + intel_runtime_pm_get(dev_priv);
 
i915_gem_object_unpin_fence(obj);
i915_gem_object_unpin_from_display_plane(obj);
  +
  + intel_runtime_pm_put(dev_priv);
 
  I don't think we touch the hardware during unpin so these aren't
  strictly needed.
 

 Daniel requested them.


   }
 
   /* Computes the linear offset to the base tile and adjusts x, y. bytes 
  per pixel
  @@ -8285,6 +8303,9 @@ static void intel_crtc_update_cursor(struct 
  drm_crtc *crtc,
if (base == 0  intel_crtc-cursor_base == 0)
return;
 
  + if (!intel_crtc-active)
  + return;
  +
 
  Did you actually manage to get by the base==0 check above with a
  disabled pipe? I don't think this should happen.

 Yes, since we enabled runtime suspend during DPMS. Remember that
 crtc-active != crtc-enabled.

 Then I think there's a bug somewhere a bit earlier. We should consider
 the cursor to be invisible when crtc-active==false. That's how we deal
 with all other planes.

Why? I am not very familiar with the cursor code and I don't see
what's the problem here. Please explain more: what exactly is the
problem, where is it and what is your suggested solution? Also, notice
that this is a patch to -stable, so I don't think we should block this
fix based on complicated rework ideas, or something that won't really
apply to kernels older than 5 hours.

When I 

[Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

[Paulo Zanoni]

From: Paulo Zanoni paulo.r.zan...@intel.com

If we're runtime suspended and try to use the plane interfaces, we
will get a lot of WARNs saying we did the wrong thing.

For intel_crtc_update_cursor(), all we need to do is return if the
CRTC is not active, since writing the registers won't really have any
effect if the screen is not visible, and we will write the registers
later when enabling the screen.

For all the other cases, we need to get runtime PM references to
pin/unpin the objects, and to change the fences. The pin/unpin
functions are the ideal places for this, but
intel_crtc_cursor_set_obj() doesn't call them, so we also have to add
get/put calls inside it. There is no problem if we runtime suspend
right after these functions are finished, because the registers
weitten are forwarded to system memory.

v2: - Narrow the put/get calls on intel_crtc_cursor_set_obj() (Daniel)
v3: - Make get/put also surround the fence and unpin calls (Daniel and
  Ville).
- Merge all the plane changes into a single patch since they're
  the same fix.
- Add the comment requested by Daniel.

Testcase: igt/pm_rpm/cursor
Testcase: igt/pm_rpm/cursor-dpms
Testcase: igt/pm_rpm/legacy-planes
Testcase: igt/pm_rpm/legacy-planes-dpms
Testcase: igt/pm_rpm/universal-planes
Testcase: igt/pm_rpm/universal-planes-dpms
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81645
Cc: sta...@vger.kernel.org
Signed-off-by: Paulo Zanoni paulo.r.zan...@intel.com
---
 drivers/gpu/drm/i915/intel_display.c | 39 +++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 4f659eb..a86d67c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2212,6 +2212,15 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
if (need_vtd_wa(dev)  alignment  256 * 1024)
alignment = 256 * 1024;
 
+   /*
+* Global gtt pte registers are special registers which actually forward
+* writes to a chunk of system memory. Which means that there is no risk
+* that the register values disappear as soon as we call
+* intel_runtime_pm_put(), so it is correct to wrap only the
+* pin/unpin/fence and not more.
+*/
+   intel_runtime_pm_get(dev_priv);
+
dev_priv-mm.interruptible = false;
ret = i915_gem_object_pin_to_display_plane(obj, alignment, pipelined);
if (ret)
@@ -2229,21 +2238,30 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
i915_gem_object_pin_fence(obj);
 
dev_priv-mm.interruptible = true;
+   intel_runtime_pm_put(dev_priv);
return 0;
 
 err_unpin:
i915_gem_object_unpin_from_display_plane(obj);
 err_interruptible:
dev_priv-mm.interruptible = true;
+   intel_runtime_pm_put(dev_priv);
return ret;
 }
 
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
 {
-   WARN_ON(!mutex_is_locked(obj-base.dev-struct_mutex));
+   struct drm_device *dev = obj-base.dev;
+   struct drm_i915_private *dev_priv = dev-dev_private;
+
+   WARN_ON(!mutex_is_locked(dev-struct_mutex));
+
+   intel_runtime_pm_get(dev_priv);
 
i915_gem_object_unpin_fence(obj);
i915_gem_object_unpin_from_display_plane(obj);
+
+   intel_runtime_pm_put(dev_priv);
 }
 
 /* Computes the linear offset to the base tile and adjusts x, y. bytes per 
pixel
@@ -8285,6 +8303,9 @@ static void intel_crtc_update_cursor(struct drm_crtc 
*crtc,
if (base == 0  intel_crtc-cursor_base == 0)
return;
 
+   if (!intel_crtc-active)
+   return;
+
I915_WRITE(CURPOS(pipe), pos);
 
if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev))
@@ -8340,9 +8361,20 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
 
/* we only need to pin inside GTT if cursor is non-phy */
mutex_lock(dev-struct_mutex);
+
+   /*
+* Global gtt pte registers are special registers which actually forward
+* writes to a chunk of system memory. Which means that there is no risk
+* that the register values disappear as soon as we call
+* intel_runtime_pm_put(), so it is correct to wrap only the
+* pin/unpin/fence and not more.
+*/
+   intel_runtime_pm_get(dev_priv);
+
if (!INTEL_INFO(dev)-cursor_needs_physical) {
unsigned alignment;
 
+
if (obj-tiling_mode) {
DRM_DEBUG_KMS(cursor cannot be tiled\n);
ret = -EINVAL;
@@ -8392,6 +8424,10 @@ static int intel_crtc_cursor_set_obj(struct drm_crtc 
*crtc,
 
i915_gem_track_fb(intel_crtc-cursor_bo, obj,
  INTEL_FRONTBUFFER_CURSOR(pipe));
+
+   if (obj)
+   intel_runtime_pm_put(dev_priv);
+
mutex_unlock(dev-struct_mutex);
 
old_width =