Re: [Intel-gfx] [PATCH v4 1/4] drm/i915/fbc: Clear frontbuffer busy bits on flip

["Coelho, Luciano" <luciano.coelho@intel.com>]

On Mon, 2023-09-04 at 08:40 +0000, Hogander, Jouni wrote:
> On Mon, 2023-09-04 at 07:25 +0000, Coelho, Luciano wrote:
> > Hi Jouni,
> > 
> > On Fri, 2023-09-01 at 12:34 +0300, Jouni Högander wrote:
> > > We are planning to move flush performed from work queue. This
> > > means it is possible to have invalidate -> flip -> flush sequence.
> > > Handle this by clearing possible busy bits on flip.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++----
> > >  1 file changed, 2 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > index 1c6d467cec26..817e5784660b 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > @@ -1307,11 +1307,9 @@ static void __intel_fbc_post_update(struct
> > > intel_fbc *fbc)
> > >         lockdep_assert_held(&fbc->lock);
> > >  
> > >         fbc->flip_pending = false;
> > > +       fbc->busy_bits = 0;
> > >  
> > > -       if (!fbc->busy_bits)
> > > -               intel_fbc_activate(fbc);
> > > -       else
> > > -               intel_fbc_deactivate(fbc, "frontbuffer write");
> > > +       intel_fbc_activate(fbc);
> > 
> > Can you explain why the call to intel_fbc_deactivate() is not needed
> > here anymore? I think it would be a good idea to explain that in the
> > commit message.  Or, at least, an explanation about it here, so it's
> > documented. ;)
> 
> We are clearing fbc->busy_bits -> I.e. if(!fbc->busy_bits) is always
> taken :
> 
> Post plane update is called at the end of the flip. If you consider
> case where busy_bits != 0 at this point: it means someone have
> initiated frontbuffer write (invalidate) which is not yet completed
> (flush from workqueue). That flush pending in workqueue is not valid
> anymore as there was a flip and the buffer which was frontbuffer is not
> a frontbuffer anymore. Even if the same buffer would be used when doing
> a flip the atomic commit would take care of flushing the buffer towards
> fbc. Also waiting for dma fences is take caren by the atomic commit
> code.

Thanks for the explanation! It makes sense.

So you have my:

Reviewed-by: Luca Coelho <luciano.coelho@intel.com>

--
Cheers,
Luca.