[Intel-gfx] [PATCH 2/3] drm/i915: Fix overlay frontbuffer tracking

[Chris Wilson]

Quoting Ville Syrjala (2021-02-09 02:19:17)
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> We don't have a persistent fb holding a reference to the frontbuffer
> object, so every time we do the get+put we throw the frontbuffer object
> immediately away. And so the next time around we get a pristine
> frontbuffer object with bits==0 even for the old vma. This confuses
> the frontbuffer tracking code which understandably expects the old
> frontbuffer to have the overlay's bit set.
> 
> Fix this by hanging on to the frontbuffer reference until the next
> flip. And just to make this a bit more clear let's track the frontbuffer
> explicitly instead of just grabbing it via the old vma.
> 
> Cc: stable at vger.kernel.org
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/1136
> Fixes: da42104f589d ("drm/i915: Hold reference to intel_frontbuffer as we track activity")

Maybe more apropos, same kernel though
Fixes: 8e7cb1799b4f ("drm/i915: Extract intel_frontbuffer active tracking")

Ok, so this definitely used to be swapping between the
obj->frontbuffer_bits and so used to have a persistent reference.
Keeping the frontbuffer tracking with the overlay makes even more sense.

> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>

> ---
>  drivers/gpu/drm/i915/display/intel_overlay.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_overlay.c b/drivers/gpu/drm/i915/display/intel_overlay.c
> index 9c0113f15b58..ef8f44f5e751 100644
> --- a/drivers/gpu/drm/i915/display/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/display/intel_overlay.c
> @@ -183,6 +183,7 @@ struct intel_overlay {
>         struct intel_crtc *crtc;
>         struct i915_vma *vma;
>         struct i915_vma *old_vma;
> +       struct intel_frontbuffer *frontbuffer;
>         bool active;
>         bool pfit_active;
>         u32 pfit_vscale_ratio; /* shifted-point number, (1<<12) == 1.0 */
> @@ -283,21 +284,19 @@ static void intel_overlay_flip_prepare(struct intel_overlay *overlay,
>                                        struct i915_vma *vma)
>  {
>         enum pipe pipe = overlay->crtc->pipe;
> -       struct intel_frontbuffer *from = NULL, *to = NULL;
> +       struct intel_frontbuffer *frontbuffer = NULL;
>  
>         drm_WARN_ON(&overlay->i915->drm, overlay->old_vma);
>  
> -       if (overlay->vma)
> -               from = intel_frontbuffer_get(overlay->vma->obj);
>         if (vma)
> -               to = intel_frontbuffer_get(vma->obj);
> +               frontbuffer = intel_frontbuffer_get(vma->obj);
>  
> -       intel_frontbuffer_track(from, to, INTEL_FRONTBUFFER_OVERLAY(pipe));
> +       intel_frontbuffer_track(overlay->frontbuffer, frontbuffer,
> +                               INTEL_FRONTBUFFER_OVERLAY(pipe));
>  
> -       if (to)
> -               intel_frontbuffer_put(to);
> -       if (from)
> -               intel_frontbuffer_put(from);
> +       if (overlay->frontbuffer)
> +               intel_frontbuffer_put(overlay->frontbuffer);
> +       overlay->frontbuffer = frontbuffer;

And this will drop the ref on overlay->frontbuffer as we flip to NULL on
shutdown.

Now if only someone still had the code to expose sprites instead of
overlays.
-Chris