diff mbox

[1/4] drm/i915: Make fb user dirty operation to invalidate frontbuffer

Message ID 1435610686-2249-1-git-send-email-rodrigo.vivi@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rodrigo Vivi June 29, 2015, 8:44 p.m. UTC
This patch introduces a frontbuffer invalidation on dirty fb
user callback.

It is mainly used for DIRTYFB drm ioctl, but can be extended
for fbdev use on following patch.

This patch itself already solves the biggest PSR known issue, that is
missed screen updates during boot, mainly when there is a splash
screen involved like plymouth.

Plymoth will do a modeset over ioctl that flushes frontbuffer
tracking and PSR gets back to work while it cannot track the
screen updates and exit properly. However plymouth also uses
a dirtyfb ioctl whenever updating the screen. So let's use it
to invalidate PSR back again.

This patch also introduces the ORIGIN_FB_DIRTY to frontbuffer tracking.
The reason is that whenever using this invalidate path we don't need to
keep continuously invalidating the frontbuffer for every call. One call
between flips is enough to keep frontbuffer tracking invalidated and
let all users aware. If a sync or async flip completed it means that we
probably can flush everything and enable powersavings features back.
If this isn't the case on the next dirty call we invalidate it again
until next flip.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          |  2 ++
 drivers/gpu/drm/i915/intel_display.c     | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_frontbuffer.c | 18 ++++++++++++++++++
 3 files changed, 38 insertions(+)

Comments

Daniel Vetter June 30, 2015, 6:55 a.m. UTC | #1
On Mon, Jun 29, 2015 at 01:44:43PM -0700, Rodrigo Vivi wrote:
> This patch introduces a frontbuffer invalidation on dirty fb
> user callback.
> 
> It is mainly used for DIRTYFB drm ioctl, but can be extended
> for fbdev use on following patch.
> 
> This patch itself already solves the biggest PSR known issue, that is
> missed screen updates during boot, mainly when there is a splash
> screen involved like plymouth.
> 
> Plymoth will do a modeset over ioctl that flushes frontbuffer
> tracking and PSR gets back to work while it cannot track the
> screen updates and exit properly. However plymouth also uses
> a dirtyfb ioctl whenever updating the screen. So let's use it
> to invalidate PSR back again.
> 
> This patch also introduces the ORIGIN_FB_DIRTY to frontbuffer tracking.
> The reason is that whenever using this invalidate path we don't need to
> keep continuously invalidating the frontbuffer for every call. One call
> between flips is enough to keep frontbuffer tracking invalidated and
> let all users aware. If a sync or async flip completed it means that we
> probably can flush everything and enable powersavings features back.
> If this isn't the case on the next dirty call we invalidate it again
> until next flip.

Sounds like we need yet another testcase in the frontbuffer tracking test
from Paulo for this case, i.e.

- Allocate a dumb buffer.
- Mmap dumb buffer (both using the dumb bo ioctls, not the i915 ones).
- Do modeset using that buffer.
- Check that drawing using that mmap + dirtyfb works correctly.

Bunch more comments on the implementation below.

> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h          |  2 ++
>  drivers/gpu/drm/i915/intel_display.c     | 18 ++++++++++++++++++
>  drivers/gpu/drm/i915/intel_frontbuffer.c | 18 ++++++++++++++++++
>  3 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ea9caf2..e0591d3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -889,6 +889,7 @@ enum fb_op_origin {
>  	ORIGIN_CPU,
>  	ORIGIN_CS,
>  	ORIGIN_FLIP,
> +	ORIGIN_FB_DIRTY,
>  };
>  
>  struct i915_fbc {
> @@ -1628,6 +1629,7 @@ struct i915_frontbuffer_tracking {
>  	 */
>  	unsigned busy_bits;
>  	unsigned flip_bits;
> +	bool fb_dirty;
>  };
>  
>  struct i915_wa_reg {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 01eaab8..19c2ab3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14330,9 +14330,27 @@ static int intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,
>  	return drm_gem_handle_create(file, &obj->base, handle);
>  }
>  
> +static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
> +					       struct drm_file *file,
> +					       unsigned flags, unsigned color,
> +					       struct drm_clip_rect *clips,
> +					       unsigned num_clips)
> +{
> +	struct drm_device *dev = fb->dev;
> +	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> +	struct drm_i915_gem_object *obj = intel_fb->obj;
> +
> +	mutex_lock(&dev->struct_mutex);
> +	intel_fb_obj_invalidate(obj, ORIGIN_FB_DIRTY);
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return 0;
> +}
> +
>  static const struct drm_framebuffer_funcs intel_fb_funcs = {
>  	.destroy = intel_user_framebuffer_destroy,
>  	.create_handle = intel_user_framebuffer_create_handle,
> +	.dirty = intel_user_framebuffer_dirty,
>  };
>  
>  static
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index 6e90e2b..329b6fc 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -81,12 +81,28 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
>  {
>  	struct drm_device *dev = obj->base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	bool fb_dirty;
>  
>  	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>  
>  	if (!obj->frontbuffer_bits)
>  		return;
>  
> +	/*
> +	 * We just invalidate the frontbuffer on the first dirty and keep
> +	 * it dirty and invalid until next flip.
> +	 */
> +	if (origin == ORIGIN_FB_DIRTY) {

ORIGIN_FB_DIRTY == ORIGIN_GTT, at least on the hw side. Except that
dirty_fb actually is a flush (it's supposed to be done _after_ some
drawing has been done).

I don't think we need to add more tracking state for this, or at least I
don't understand exactly why we need all those fb_dirty state.

> +		mutex_lock(&dev_priv->fb_tracking.lock);
> +		fb_dirty = dev_priv->fb_tracking.fb_dirty;
> +		dev_priv->fb_tracking.fb_dirty = true;
> +		mutex_unlock(&dev_priv->fb_tracking.lock);
> +
> +		if (fb_dirty)
> +			return;
> +		DRM_ERROR("PSR FBT invalidate dirty\n");
> +	}
> +
>  	if (origin == ORIGIN_CS) {
>  		mutex_lock(&dev_priv->fb_tracking.lock);
>  		dev_priv->fb_tracking.busy_bits
> @@ -207,6 +223,7 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
>  	mutex_lock(&dev_priv->fb_tracking.lock);
> +	dev_priv->fb_tracking.fb_dirty = false;
>  	/* Mask any cancelled flips. */
>  	frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
>  	dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
> @@ -233,6 +250,7 @@ void intel_frontbuffer_flip(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
>  	mutex_lock(&dev_priv->fb_tracking.lock);
> +	dev_priv->fb_tracking.fb_dirty = false;
>  	/* 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.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Rodrigo Vivi June 30, 2015, 10:41 p.m. UTC | #2
On Mon, Jun 29, 2015 at 11:53 PM Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Jun 29, 2015 at 01:44:43PM -0700, Rodrigo Vivi wrote:
> > This patch introduces a frontbuffer invalidation on dirty fb
> > user callback.
> >
> > It is mainly used for DIRTYFB drm ioctl, but can be extended
> > for fbdev use on following patch.
> >
> > This patch itself already solves the biggest PSR known issue, that is
> > missed screen updates during boot, mainly when there is a splash
> > screen involved like plymouth.
> >
> > Plymoth will do a modeset over ioctl that flushes frontbuffer
> > tracking and PSR gets back to work while it cannot track the
> > screen updates and exit properly. However plymouth also uses
> > a dirtyfb ioctl whenever updating the screen. So let's use it
> > to invalidate PSR back again.
> >
> > This patch also introduces the ORIGIN_FB_DIRTY to frontbuffer tracking.
> > The reason is that whenever using this invalidate path we don't need to
> > keep continuously invalidating the frontbuffer for every call. One call
> > between flips is enough to keep frontbuffer tracking invalidated and
> > let all users aware. If a sync or async flip completed it means that we
> > probably can flush everything and enable powersavings features back.
> > If this isn't the case on the next dirty call we invalidate it again
> > until next flip.
>
> Sounds like we need yet another testcase in the frontbuffer tracking test
> from Paulo for this case, i.e.
>
> - Allocate a dumb buffer.
> - Mmap dumb buffer (both using the dumb bo ioctls, not the i915 ones).
> - Do modeset using that buffer.
> - Check that drawing using that mmap + dirtyfb works correctly.
>
> Bunch more comments on the implementation below.
>
> >
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h          |  2 ++
> >  drivers/gpu/drm/i915/intel_display.c     | 18 ++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_frontbuffer.c | 18 ++++++++++++++++++
> >  3 files changed, 38 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> > index ea9caf2..e0591d3 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -889,6 +889,7 @@ enum fb_op_origin {
> >       ORIGIN_CPU,
> >       ORIGIN_CS,
> >       ORIGIN_FLIP,
> > +     ORIGIN_FB_DIRTY,
> >  };
> >
> >  struct i915_fbc {
> > @@ -1628,6 +1629,7 @@ struct i915_frontbuffer_tracking {
> >        */
> >       unsigned busy_bits;
> >       unsigned flip_bits;
> > +     bool fb_dirty;
> >  };
> >
> >  struct i915_wa_reg {
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> > index 01eaab8..19c2ab3 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14330,9 +14330,27 @@ static int
> intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,
> >       return drm_gem_handle_create(file, &obj->base, handle);
> >  }
> >
> > +static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
> > +                                            struct drm_file *file,
> > +                                            unsigned flags, unsigned
> color,
> > +                                            struct drm_clip_rect *clips,
> > +                                            unsigned num_clips)
> > +{
> > +     struct drm_device *dev = fb->dev;
> > +     struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
> > +     struct drm_i915_gem_object *obj = intel_fb->obj;
> > +
> > +     mutex_lock(&dev->struct_mutex);
> > +     intel_fb_obj_invalidate(obj, ORIGIN_FB_DIRTY);
> > +     mutex_unlock(&dev->struct_mutex);
> > +
> > +     return 0;
> > +}
> > +
> >  static const struct drm_framebuffer_funcs intel_fb_funcs = {
> >       .destroy = intel_user_framebuffer_destroy,
> >       .create_handle = intel_user_framebuffer_create_handle,
> > +     .dirty = intel_user_framebuffer_dirty,
> >  };
> >
> >  static
> > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c
> b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > index 6e90e2b..329b6fc 100644
> > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > @@ -81,12 +81,28 @@ void intel_fb_obj_invalidate(struct
> drm_i915_gem_object *obj,
> >  {
> >       struct drm_device *dev = obj->base.dev;
> >       struct drm_i915_private *dev_priv = to_i915(dev);
> > +     bool fb_dirty;
> >
> >       WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> >
> >       if (!obj->frontbuffer_bits)
> >               return;
> >
> > +     /*
> > +      * We just invalidate the frontbuffer on the first dirty and keep
> > +      * it dirty and invalid until next flip.
> > +      */
> > +     if (origin == ORIGIN_FB_DIRTY) {
>
> ORIGIN_FB_DIRTY == ORIGIN_GTT, at least on the hw side. Except that
> dirty_fb actually is a flush (it's supposed to be done _after_ some
> drawing has been done).
>
> I don't think we need to add more tracking state for this, or at least I
> don't understand exactly why we need all those fb_dirty state.
>

I agree.. Just created this to be more generic and use for fbdev that could
call for every single screen update. So in this case we would want to
minimize the amount of invalidation... but forget about it....


>
> > +             mutex_lock(&dev_priv->fb_tracking.lock);
> > +             fb_dirty = dev_priv->fb_tracking.fb_dirty;
> > +             dev_priv->fb_tracking.fb_dirty = true;
> > +             mutex_unlock(&dev_priv->fb_tracking.lock);
> > +
> > +             if (fb_dirty)
> > +                     return;
> > +             DRM_ERROR("PSR FBT invalidate dirty\n");
> > +     }
> > +
> >       if (origin == ORIGIN_CS) {
> >               mutex_lock(&dev_priv->fb_tracking.lock);
> >               dev_priv->fb_tracking.busy_bits
> > @@ -207,6 +223,7 @@ void intel_frontbuffer_flip_complete(struct
> drm_device *dev,
> >       struct drm_i915_private *dev_priv = to_i915(dev);
> >
> >       mutex_lock(&dev_priv->fb_tracking.lock);
> > +     dev_priv->fb_tracking.fb_dirty = false;
> >       /* Mask any cancelled flips. */
> >       frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
> >       dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
> > @@ -233,6 +250,7 @@ void intel_frontbuffer_flip(struct drm_device *dev,
> >       struct drm_i915_private *dev_priv = to_i915(dev);
> >
> >       mutex_lock(&dev_priv->fb_tracking.lock);
> > +     dev_priv->fb_tracking.fb_dirty = false;
> >       /* 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.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> 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
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ea9caf2..e0591d3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -889,6 +889,7 @@  enum fb_op_origin {
 	ORIGIN_CPU,
 	ORIGIN_CS,
 	ORIGIN_FLIP,
+	ORIGIN_FB_DIRTY,
 };
 
 struct i915_fbc {
@@ -1628,6 +1629,7 @@  struct i915_frontbuffer_tracking {
 	 */
 	unsigned busy_bits;
 	unsigned flip_bits;
+	bool fb_dirty;
 };
 
 struct i915_wa_reg {
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 01eaab8..19c2ab3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14330,9 +14330,27 @@  static int intel_user_framebuffer_create_handle(struct drm_framebuffer *fb,
 	return drm_gem_handle_create(file, &obj->base, handle);
 }
 
+static int intel_user_framebuffer_dirty(struct drm_framebuffer *fb,
+					       struct drm_file *file,
+					       unsigned flags, unsigned color,
+					       struct drm_clip_rect *clips,
+					       unsigned num_clips)
+{
+	struct drm_device *dev = fb->dev;
+	struct intel_framebuffer *intel_fb = to_intel_framebuffer(fb);
+	struct drm_i915_gem_object *obj = intel_fb->obj;
+
+	mutex_lock(&dev->struct_mutex);
+	intel_fb_obj_invalidate(obj, ORIGIN_FB_DIRTY);
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
+
 static const struct drm_framebuffer_funcs intel_fb_funcs = {
 	.destroy = intel_user_framebuffer_destroy,
 	.create_handle = intel_user_framebuffer_create_handle,
+	.dirty = intel_user_framebuffer_dirty,
 };
 
 static
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index 6e90e2b..329b6fc 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -81,12 +81,28 @@  void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
+	bool fb_dirty;
 
 	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 
 	if (!obj->frontbuffer_bits)
 		return;
 
+	/*
+	 * We just invalidate the frontbuffer on the first dirty and keep
+	 * it dirty and invalid until next flip.
+	 */
+	if (origin == ORIGIN_FB_DIRTY) {
+		mutex_lock(&dev_priv->fb_tracking.lock);
+		fb_dirty = dev_priv->fb_tracking.fb_dirty;
+		dev_priv->fb_tracking.fb_dirty = true;
+		mutex_unlock(&dev_priv->fb_tracking.lock);
+
+		if (fb_dirty)
+			return;
+		DRM_ERROR("PSR FBT invalidate dirty\n");
+	}
+
 	if (origin == ORIGIN_CS) {
 		mutex_lock(&dev_priv->fb_tracking.lock);
 		dev_priv->fb_tracking.busy_bits
@@ -207,6 +223,7 @@  void intel_frontbuffer_flip_complete(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
 	mutex_lock(&dev_priv->fb_tracking.lock);
+	dev_priv->fb_tracking.fb_dirty = false;
 	/* Mask any cancelled flips. */
 	frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
 	dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
@@ -233,6 +250,7 @@  void intel_frontbuffer_flip(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
 	mutex_lock(&dev_priv->fb_tracking.lock);
+	dev_priv->fb_tracking.fb_dirty = false;
 	/* Remove stale busy bits due to the old buffer. */
 	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
 	mutex_unlock(&dev_priv->fb_tracking.lock);