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

[Vivi, Rodrigo]

On Thu, 2015-07-02 at 13:03 -0300, Paulo Zanoni wrote:
> 2015-06-30 20:42 GMT-03:00 Rodrigo Vivi <rodrigo.vivi at intel.com>:
> > Let's do a frontbuffer invalidation on dirty fb.
> > To be used for DIRTYFB drm ioctl.
> >
> > This patch 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.
> >
> > v2: Remove ORIGIN_FB_DIRTY and use ORIGIN_GTT instead since dirty
> >     callback is just called after few screen updates and not on
> >     everyone as pointed by Daniel.
> >
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 724b0e3..b55b1b6 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14393,9 +14393,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)
> 
> You don't need the white space on the lines above, just the tabs.
> 
> > +{
> > +       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_GTT);
> 
> As far as I understood from my brief investigation, the dirty IOCTL
> just says "hey, I'm done writing on the memory, you can flush things
> now", and if the user writes on the buffer again later, it will need
> to call the dirty IOCTL again. So shouldn't we call
> intel_fb_obj_flush(obj, false) here? It seems this was also pointed by
> Daniel on the first review. It would be better because it would allow
> us to actually keep PSR/FBC enabled.

The flush caused by the dumb modeset ioctl is exactly what I want to
revert here.

Well, I just tested to double check here and flush makes me to miss
screen updates. (triple checked with retired = true as well just in
case)

fbdev comes to place and invalidated frontbuffer, then plymouth does a
flush that makes psr start working when it should continue disabled.
Continue flushing it doesn't solve the problem, just ratify it.

But beside the issue that it is solving I don't believe we want is a
flush anyway. There is something writing directly to frontbuffer with no
invalidation. The dirty call is supposed to be a damage call that
actually tells something on the screen got written and needs to be
updated if it hasn't still. In our cause this is exactly the frontbuffer
invalidate, not the flush.

The flush place would be after it really stopped doing something, but
since I don't trust it I prefer to let it invalidated until next flip.

> 
> Also, don't forget that switching to intel_fb_obj_flush() will allow
> you to kill the struct_mutex lock.
> 
> BTW, I'm also looking forward to see the IGT test suggested by Daniel :)
> 
> > +       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
> > --
> > 2.1.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
>