2014-08-11 11:42 GMT-03:00 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>: > 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@xxxxxxxxxxxxxxx>: >> > On Wed, Aug 06, 2014 at 06:26:01PM -0300, Paulo Zanoni wrote: >> >> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> >> >> >> 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@xxxxxxxxxxxxxxx >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> >> --- >> >> 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 -- Paulo Zanoni -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html