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

[Daniel Vetter]

On Thu, Jul 02, 2015 at 04:41:32PM +0000, Vivi, Rodrigo wrote:
> 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.

See my review on the first round of this patch: dirtyfb _is_ a flush. If
it's not enough then there's some other problems still around.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch