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

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

On Mon, Aug 11, 2014 at 02:57:44PM -0300, Paulo Zanoni wrote:
> 2014-08-11 11:42 GMT-03:00 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.
> 
> Why? I am not very familiar with the cursor code and I don't see
> what's the problem here.

Just feels like duct tape for something that should have been caught
by some earlier piece of code.

> Please explain more: what exactly is the
> problem, where is it and what is your suggested solution?

I think this ought to fix it, and is exactly what we do with other
planes:

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e9b578e..c9f733d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11699,8 +11699,8 @@ intel_cursor_plane_update(struct drm_plane *plane, struct drm_crtc *crtc,
        };
        const struct drm_rect clip = {
                /* integer pixels */
-               .x2 = intel_crtc->config.pipe_src_w,
-               .y2 = intel_crtc->config.pipe_src_h,
+               .x2 = intel_crtc->active ? intel_crtc->config.pipe_src_w : 0,
+               .y2 = intel_crtc->active ? intel_crtc->config.pipe_src_h : 0,
        };
        bool visible;
        int ret;

> Also, notice
> that this is a patch to -stable, so I don't think we should block this
> fix based on complicated rework ideas, or something that won't really
> apply to kernels older than 5 hours.
> 
> When I look at the backtrace, I see that we are already calling these
> functions through drm_mode_cursor_universal(), so this should already
> be handling the cursor plane in the same way it handles the other
> planes.
> 
> Also, I invoke Daniel to discuss here since he's the one that
> introduced the difference between ->active and ->enabled.
> 
> >
> > --
> > Ville Syrjälä
> > Intel OTC
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Ville Syrjälä
Intel OTC