[Intel-gfx] [PATCH] drm/i915: fix plane/cursor handling when runtime suspended
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Aug 15 18:55:42 CEST 2014
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 at linux.intel.com>:
> > On Thu, Aug 14, 2014 at 12:06:02PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni at 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: stable at vger.kernel.org
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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 ;)
>
> 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 at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
>
>
>
> --
> Paulo Zanoni
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list