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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
