Quoting Ville Syrjala (2021-02-09 02:19:17) > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > 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@xxxxxxxxxxxxxxx > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx> > 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@xxxxxxxxxxxxxxx> Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > --- > 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
