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

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

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@linux.intel.com>:
> > On Wed, Aug 06, 2014 at 06:26:01PM -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.
> >>
> >> 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@vger.kernel.org
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  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.

-- 
Ville Syrjälä
Intel OTC