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

Ville Syrjälä
2014-08-11T14:42:09Z

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