[PATCH 1/2] drm/i915: Tighting frontbuffer tracking around flips

[PATCH 1/2] drm/i915: Tighting frontbuffer tracking around flips

[Daniel Vetter]

So I think I've spotted a small gap in the frontbuffer tracking
while discussing the logic with Paulo on irc:

1. Userspace schedules gpu rendering to the current frontbuffer.
This gets tracked in dev_priv->fb_tracking.busy_bits.

2. We pageflip a fully rendered buffer before the frontbuffer
rendering completes.

3. The request retiring will never clear busy_bits (since at retire
time the old frontbuffer won't have obj->frontbuffer_bits set), so
these bits now are stuck until someone again does a bit of frontbuffer
tracking.

If we clear stale busy_bits in flip_prepare this gap is closed.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_frontbuffer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index f74744c091cb..969af6e7b634 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -246,8 +246,9 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	mutex_lock(&dev_priv->fb_tracking.lock);
-	dev_priv->fb_tracking.flip_bits
-		|= frontbuffer_bits;
+	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);
 }
 
-- 
2.1.1

[PATCH 2/2] drm/i915: spelling fixes for frontbuffer tracking kerneldoc

[Daniel Vetter]

Oh well.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_drv.h         |  2 +-
 drivers/gpu/drm/i915/intel_frontbuffer.c | 13 ++++++-------
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 91e2b128c537..4e71ae5e3832 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -816,7 +816,7 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
 void intel_frontbuffer_flush(struct drm_device *dev,
 			     unsigned frontbuffer_bits);
 /**
- * intel_frontbuffer_flip - prepare frontbuffer flip
+ * intel_frontbuffer_flip - synchronous frontbuffer flip
  * @dev: DRM device
  * @frontbuffer_bits: frontbuffer plane tracking bits
  *
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index 969af6e7b634..437fa8072adf 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -28,7 +28,7 @@
  * DOC: frontbuffer tracking
  *
  * Many features require us to track changes to the currently active
- * frontbuffer, especially rendering targetted at the frontbuffer.
+ * frontbuffer, especially rendering targeted at the frontbuffer.
  *
  * To be able to do so GEM tracks frontbuffers using a bitmask for all possible
  * frontbuffer slots through i915_gem_track_fb(). The function in this file are
@@ -55,7 +55,7 @@
  * cancelled as soon as busyness is detected.
  *
  * Note that there's also an older frontbuffer activity tracking scheme which
- * just trackings general activity. This is done by the various mark_busy and
+ * just tracks general activity. This is done by the various mark_busy and
  * mark_idle functions. For display power management features using these
  * functions is deprecated and should be avoided.
  */
@@ -166,7 +166,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
  *
  * This function gets called every time rendering on the given planes has
  * completed and frontbuffer caching can be started again. Flushes will get
- * delayed if they're blocked by some oustanding asynchronous rendering.
+ * delayed if they're blocked by some outstanding asynchronous rendering.
  *
  * Can be called without any locks held.
  */
@@ -229,7 +229,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
 }
 
 /**
- * intel_frontbuffer_flip_prepare - prepare asnychronous frontbuffer flip
+ * intel_frontbuffer_flip_prepare - prepare asynchronous frontbuffer flip
  * @dev: DRM device
  * @frontbuffer_bits: frontbuffer plane tracking bits
  *
@@ -253,12 +253,12 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
 }
 
 /**
- * intel_frontbuffer_flip_complete - complete asynchronous frontbuffer flush
+ * intel_frontbuffer_flip_complete - complete asynchronous frontbuffer flip
  * @dev: DRM device
  * @frontbuffer_bits: frontbuffer plane tracking bits
  *
  * This function gets called after the flip has been latched and will complete
- * on the next vblank. It will execute the fush if it hasn't been cancalled yet.
+ * on the next vblank. It will execute the flush if it hasn't been cancalled yet.
  *
  * Can be called without any locks held.
  */
@@ -275,4 +275,3 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
 
 	intel_frontbuffer_flush(dev, frontbuffer_bits);
 }
-
-- 
2.1.1

Re: [PATCH 1/2] drm/i915: Tighting frontbuffer tracking around flips

[Paulo Zanoni]

2014-09-30 17:10 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> So I think I've spotted a small gap in the frontbuffer tracking
> while discussing the logic with Paulo on irc:
>
> 1. Userspace schedules gpu rendering to the current frontbuffer.
> This gets tracked in dev_priv->fb_tracking.busy_bits.
>
> 2. We pageflip a fully rendered buffer before the frontbuffer
> rendering completes.
>
> 3. The request retiring will never clear busy_bits (since at retire
> time the old frontbuffer won't have obj->frontbuffer_bits set), so
> these bits now are stuck until someone again does a bit of frontbuffer
> tracking.
>
> If we clear stale busy_bits in flip_prepare this gap is closed.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_frontbuffer.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index f74744c091cb..969af6e7b634 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -246,8 +246,9 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
>         struct drm_i915_private *dev_priv = dev->dev_private;
>
>         mutex_lock(&dev_priv->fb_tracking.lock);
> -       dev_priv->fb_tracking.flip_bits
> -               |= frontbuffer_bits;
> +       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);
>  }
>
> --
> 2.1.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/2] drm/i915: spelling fixes for frontbuffer tracking kerneldoc

[Paulo Zanoni]

2014-09-30 17:10 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Oh well.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_drv.h         |  2 +-
>  drivers/gpu/drm/i915/intel_frontbuffer.c | 13 ++++++-------
>  2 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 91e2b128c537..4e71ae5e3832 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -816,7 +816,7 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
>  void intel_frontbuffer_flush(struct drm_device *dev,
>                              unsigned frontbuffer_bits);
>  /**
> - * intel_frontbuffer_flip - prepare frontbuffer flip
> + * intel_frontbuffer_flip - synchronous frontbuffer flip
>   * @dev: DRM device
>   * @frontbuffer_bits: frontbuffer plane tracking bits
>   *
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index 969af6e7b634..437fa8072adf 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -28,7 +28,7 @@
>   * DOC: frontbuffer tracking
>   *
>   * Many features require us to track changes to the currently active
> - * frontbuffer, especially rendering targetted at the frontbuffer.
> + * frontbuffer, especially rendering targeted at the frontbuffer.
>   *
>   * To be able to do so GEM tracks frontbuffers using a bitmask for all possible
>   * frontbuffer slots through i915_gem_track_fb(). The function in this file are
> @@ -55,7 +55,7 @@
>   * cancelled as soon as busyness is detected.
>   *
>   * Note that there's also an older frontbuffer activity tracking scheme which
> - * just trackings general activity. This is done by the various mark_busy and
> + * just tracks general activity. This is done by the various mark_busy and
>   * mark_idle functions. For display power management features using these
>   * functions is deprecated and should be avoided.
>   */
> @@ -166,7 +166,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
>   *
>   * This function gets called every time rendering on the given planes has
>   * completed and frontbuffer caching can be started again. Flushes will get
> - * delayed if they're blocked by some oustanding asynchronous rendering.
> + * delayed if they're blocked by some outstanding asynchronous rendering.
>   *
>   * Can be called without any locks held.
>   */
> @@ -229,7 +229,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
>  }
>
>  /**
> - * intel_frontbuffer_flip_prepare - prepare asnychronous frontbuffer flip
> + * intel_frontbuffer_flip_prepare - prepare asynchronous frontbuffer flip
>   * @dev: DRM device
>   * @frontbuffer_bits: frontbuffer plane tracking bits
>   *
> @@ -253,12 +253,12 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
>  }
>
>  /**
> - * intel_frontbuffer_flip_complete - complete asynchronous frontbuffer flush
> + * intel_frontbuffer_flip_complete - complete asynchronous frontbuffer flip
>   * @dev: DRM device
>   * @frontbuffer_bits: frontbuffer plane tracking bits
>   *
>   * This function gets called after the flip has been latched and will complete
> - * on the next vblank. It will execute the fush if it hasn't been cancalled yet.
> + * on the next vblank. It will execute the flush if it hasn't been cancalled yet.

The word "cancalled" could also be fixed :)

With that: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>   *
>   * Can be called without any locks held.
>   */
> @@ -275,4 +275,3 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
>
>         intel_frontbuffer_flush(dev, frontbuffer_bits);
>  }
> -
> --
> 2.1.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/2] drm/i915: spelling fixes for frontbuffer tracking kerneldoc

[Daniel Vetter]

On Tue, Sep 30, 2014 at 05:34:44PM -0300, Paulo Zanoni wrote:
> 2014-09-30 17:10 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > Oh well.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h         |  2 +-
> >  drivers/gpu/drm/i915/intel_frontbuffer.c | 13 ++++++-------
> >  2 files changed, 7 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 91e2b128c537..4e71ae5e3832 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -816,7 +816,7 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
> >  void intel_frontbuffer_flush(struct drm_device *dev,
> >                              unsigned frontbuffer_bits);
> >  /**
> > - * intel_frontbuffer_flip - prepare frontbuffer flip
> > + * intel_frontbuffer_flip - synchronous frontbuffer flip
> >   * @dev: DRM device
> >   * @frontbuffer_bits: frontbuffer plane tracking bits
> >   *
> > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > index 969af6e7b634..437fa8072adf 100644
> > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > @@ -28,7 +28,7 @@
> >   * DOC: frontbuffer tracking
> >   *
> >   * Many features require us to track changes to the currently active
> > - * frontbuffer, especially rendering targetted at the frontbuffer.
> > + * frontbuffer, especially rendering targeted at the frontbuffer.
> >   *
> >   * To be able to do so GEM tracks frontbuffers using a bitmask for all possible
> >   * frontbuffer slots through i915_gem_track_fb(). The function in this file are
> > @@ -55,7 +55,7 @@
> >   * cancelled as soon as busyness is detected.
> >   *
> >   * Note that there's also an older frontbuffer activity tracking scheme which
> > - * just trackings general activity. This is done by the various mark_busy and
> > + * just tracks general activity. This is done by the various mark_busy and
> >   * mark_idle functions. For display power management features using these
> >   * functions is deprecated and should be avoided.
> >   */
> > @@ -166,7 +166,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
> >   *
> >   * This function gets called every time rendering on the given planes has
> >   * completed and frontbuffer caching can be started again. Flushes will get
> > - * delayed if they're blocked by some oustanding asynchronous rendering.
> > + * delayed if they're blocked by some outstanding asynchronous rendering.
> >   *
> >   * Can be called without any locks held.
> >   */
> > @@ -229,7 +229,7 @@ void intel_fb_obj_flush(struct drm_i915_gem_object *obj,
> >  }
> >
> >  /**
> > - * intel_frontbuffer_flip_prepare - prepare asnychronous frontbuffer flip
> > + * intel_frontbuffer_flip_prepare - prepare asynchronous frontbuffer flip
> >   * @dev: DRM device
> >   * @frontbuffer_bits: frontbuffer plane tracking bits
> >   *
> > @@ -253,12 +253,12 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
> >  }
> >
> >  /**
> > - * intel_frontbuffer_flip_complete - complete asynchronous frontbuffer flush
> > + * intel_frontbuffer_flip_complete - complete asynchronous frontbuffer flip
> >   * @dev: DRM device
> >   * @frontbuffer_bits: frontbuffer plane tracking bits
> >   *
> >   * This function gets called after the flip has been latched and will complete
> > - * on the next vblank. It will execute the fush if it hasn't been cancalled yet.
> > + * on the next vblank. It will execute the flush if it hasn't been cancalled yet.
> 
> The word "cancalled" could also be fixed :)

Fixed.
> 
> With that: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Thanks for your review, both patches applied.
-Daniel

> 
> >   *
> >   * Can be called without any locks held.
> >   */
> > @@ -275,4 +275,3 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
> >
> >         intel_frontbuffer_flush(dev, frontbuffer_bits);
> >  }
> > -
> > --
> > 2.1.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch