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.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 wanted to read your reply if there's a good reason for it ;) > > 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. Just put before goto is best for this kind of thing IMO. > 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 > :) I figured since rpm get/put are a bit special (even the comment says so) that we'd have them exactly where needed. > > > > >> 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 -- Ville Syrjälä Intel OTC -- 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