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

[Daniel Vetter]

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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch