| Message ID | 1407360362-1774-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 06, 2014 at 06:26:01PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@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: stable@vger.kernel.org > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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 wait for vblank before unpinning). But that's just something else we have to fix at some point. > 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 = intel_crtc->cursor_width; > @@ -8413,6 +8449,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 > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2014-08-11 8:32 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>: > On Wed, Aug 06, 2014 at 06:26:01PM -0300, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@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: stable@vger.kernel.org >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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_INFO(dev)->cursor_needs_physical) { >> unsigned alignment; >> >> + > > Spurious whitespace Oops... Will fix. > > 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 wait for vblank before unpinning). But > that's just something else we have to fix at some point. Yeah, these should be in other patches. Thanks, Paulo > >> 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 = intel_crtc->cursor_width; >> @@ -8413,6 +8449,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 >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
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.syrjala@linux.intel.com>: > > On Wed, Aug 06, 2014 at 06:26:01PM -0300, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@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: stable@vger.kernel.org > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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.
2014-08-11 11:42 GMT-03:00 Ville Syrjälä <ville.syrjala@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.syrjala@linux.intel.com>: >> > On Wed, Aug 06, 2014 at 06:26:01PM -0300, Paulo Zanoni wrote: >> >> From: Paulo Zanoni <paulo.r.zanoni@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: stable@vger.kernel.org >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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 look at the backtrace, I see that we are already calling these functions through drm_mode_cursor_universal(), so this should already be handling the cursor plane in the same way it handles the other planes. Also, I invoke Daniel to discuss here since he's the one that introduced the difference between ->active and ->enabled. > > -- > Ville Syrjälä > Intel OTC
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 = intel_crtc->cursor_width; @@ -8413,6 +8449,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);