Re: [PATCH] drm/i915: Clear fb_tracking.busy_bits also for synchronous flips

From: Daniel Vetter <daniel@ffwll.ch>
To: Paulo Zanoni <przanoni@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH] drm/i915: Clear fb_tracking.busy_bits also for synchronous flips
Date: Tue, 23 Jun 2015 22:52:49 +0200	[thread overview]
Message-ID: <20150623205248.GK25769@phenom.ffwll.local> (raw)
In-Reply-To: <CA+gsUGRxAp3mCRJpJoa9fbaYzHKSGZ1NHuQsyAXaHz4t+P7yLw@mail.gmail.com>

On Tue, Jun 23, 2015 at 03:59:24PM -0300, Paulo Zanoni wrote:
> 2015-06-18 6:23 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > The current/old frontbuffer might still have gpu frontbuffer rendering
> > pending. But once flipped it won't have the corresponding frontbuffer
> > bits any more and hence the request retire function won't ever clear
> > the corresponding busy bits. The async flip tracking (with the
> > flip_prepare and flip_complete functions) already does this, but
> > somehow I've forgotten to do this for synchronous flips.
> >
> > Note that we don't track outstanding rendering of the new framebuffer
> > with busy_bits since all our plane update code waits for previous
> > rendering to complete before displaying a new buffer. Hence a new
> > buffer will never be busy.
> 
> Steps to reproduce:
> - Boot your machine to the display manager
> - Kill it, returning to fbcon
> - Try to run any subtest from igt/tests/kms_frontbuffer_tracking: they
> all fail because FBC never gets enabled
> - Do any blt/render operation to the frontbuffer
> - Now the tests will succeed
> 
> I also submitted a new test for this, that doesn't require the steps
> above, so we can also add:
> Testcase: igt/kms_frontbuffer_tracking/fbc-modesetfrombusy
> 
> More below:
> 
> >
> > v2: Drop the spurious inline Ville spotted.
> >
> > Reported-by: Paulo Zanoni <przanoni@gmail.com>
> > Cc: Paulo Zanoni <przanoni@gmail.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> > Paulo promised to also extend kms_frontbuffer_tracking with flip vs.
> > busy buffer tests.
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h         | 18 ++----------------
> >  drivers/gpu/drm/i915/intel_frontbuffer.c | 26 ++++++++++++++++++++++++++
> >  2 files changed, 28 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index bcafefcf048b..b7c69460fb20 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -963,23 +963,9 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
> >                                      unsigned frontbuffer_bits);
> >  void intel_frontbuffer_flush(struct drm_device *dev,
> >                              unsigned frontbuffer_bits);
> > -/**
> > - * intel_frontbuffer_flip - synchronous frontbuffer flip
> > - * @dev: DRM device
> > - * @frontbuffer_bits: frontbuffer plane tracking bits
> > - *
> > - * This function gets called after scheduling a flip on @obj. This is for
> > - * synchronous plane updates which will happen on the next vblank and which will
> > - * not get delayed by pending gpu rendering.
> > - *
> > - * Can be called without any locks held.
> > - */
> > -static inline
> > +inline
> >  void intel_frontbuffer_flip(struct drm_device *dev,
> > -                           unsigned frontbuffer_bits)
> > -{
> > -       intel_frontbuffer_flush(dev, frontbuffer_bits);
> > -}
> > +                           unsigned frontbuffer_bits);
> >
> >  unsigned int intel_fb_align_height(struct drm_device *dev,
> >                                    unsigned int height,
> > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > index 57095f54c1f2..c3559fde25e5 100644
> > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > @@ -270,3 +270,29 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
> >
> >         intel_frontbuffer_flush(dev, frontbuffer_bits);
> >  }
> > +
> > +/**
> > + * intel_frontbuffer_flip - synchronous frontbuffer flip
> > + * @dev: DRM device
> > + * @frontbuffer_bits: frontbuffer plane tracking bits
> > + *
> > + * This function gets called after scheduling a flip on @obj. This is for
> > + * synchronous plane updates which will happen on the next vblank and which will
> > + * not get delayed by pending gpu rendering.
> > + *
> > + * Can be called without any locks held.
> > + */
> > +
> > +void intel_frontbuffer_flip(struct drm_device *dev,
> > +                           unsigned frontbuffer_bits)
> > +{
> > +       struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +       mutex_lock(&dev_priv->fb_tracking.lock);
> > +       dev_priv->fb_tracking.flip_bits |= frontbuffer_bits;
> 
> I don't understand the purpose of the line above. Is this really
> necessary? It seems that flip_bits is only necessary for the
> asynchronous flip case, where someone schedules a flip then draws on
> the buffer, so we don't flush the buffer at flip_complete. We do
> unconditional flush at intel_frontbuffer_flip(), so it seems like we
> shouldn't be touching flip_bits here.

Indeed this was bogus, but luckily it was harmless (keeping track of an
async flip that will never happen isn't a problem) so either version
worked. But I removed it when merging the patch, thanks for spotting this.
-Daniel

> 
> I tested both this version and the version without the flip_bits
> change, and both appear to solve the problem I can reproduce, so for
> both:
> Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> And, with the flip_bits problem explained or fixed:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> > +       /* Remove stale busy bits due to the old buffer. */
> > +       dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
> > +       mutex_unlock(&dev_priv->fb_tracking.lock);
> > +
> > +       intel_frontbuffer_flush(dev, frontbuffer_bits);
> > +}
> > --
> > 2.1.0
> >
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx