drm/i915: Clear fb_tracking.busy_bits also for synchronous flips
diff mbox

Message ID 1434619404-2870-1-git-send-email-daniel.vetter@ffwll.ch
State New
Headers show

Commit Message

Daniel Vetter June 18, 2015, 9:23 a.m. UTC
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.

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(-)

Comments

Paulo Zanoni June 23, 2015, 6:59 p.m. UTC | #1
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.

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
>
Daniel Vetter June 23, 2015, 8:52 p.m. UTC | #2
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

Patch
diff mbox

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;
+	/* 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);
+}