2014-08-15 5:39 GMT-03:00 Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>: > On Thu, Aug 14, 2014 at 12:06:02PM -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. >> >> 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: stable@xxxxxxxxxxxxxxx >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> >> --- >> 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 get/put calls in the wrong place, so having them wrap a wider piece of code could be better. Also, this is not like a mutex/spinlock get/put where we have to try to just get/put the smallest amount of things to not block another thread: if this code is running, it's very likely that something else is going to wake up the graphics driver anyway - and this is why my first version just wrapped the whole function. Anyway, since I couldn't imagine what style the code reviewers would prefer - I'm fine with both ways - I had to chose one, but I guess I chose the wrong one :) > >> 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 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > 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