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

[Paulo Zanoni]

2014-08-11 11:42 GMT-03:00 Ville Syrjälä <ville.syrjala at 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 at linux.intel.com>:
>> > On Wed, Aug 06, 2014 at 06:26:01PM -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.
>> >>
>> >> 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 at vger.kernel.org
>> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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. Please explain more: what exactly is the
problem, where is it and what is your suggested solution? 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