Re: [PATCH] drm/i915: fix plane/cursor handling when runtime suspended

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@linux.intel.com>:
> > On Thu, Aug 14, 2014 at 12:06:02PM -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.
> >>
> >> 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@vger.kernel.org
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC