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

[Vivi, Rodrigo]

On Mon, 2015-07-06 at 19:56 +0200, Daniel Vetter wrote:
> On Mon, Jul 06, 2015 at 02:11:55PM -0300, Paulo Zanoni wrote:
> > 2015-07-06 13:43 GMT-03:00 Vivi, Rodrigo <rodrigo.vivi at intel.com>:
> > > On Fri, 2015-07-03 at 09:10 +0200, Daniel Vetter wrote:
> > >> 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.
> > >
> > > Well, the flush itself in the way it is defined is to flush frontbuffer
> > > bits and put power saving features back to work. PSR flush for instance
> > > doesn't exit on every flush. Only in the cases that we know HW tracking
> > > doesn't work.
> > >
> > > One possibility would be change PSR to respect that all flush always is
> > > invalidate (exit) + flush fb_bits. But I'm against this since PSR on
> > > core platforms doesn't have SW trackking ext and it wasn't implemented
> > > to be fully enabled/disabled every time.
> > >
> > > Another idea on this line is to make flushs know origins or at least a
> > > boolean to separate cases where flush must be handled as flush bits +
> > > continue working or invalidate + flush bits + start working again
> > >
> > > Please let me know what you think.
> > >
> > > But putting flushs on this dirty callback is currently useless because
> > > flush just tells psr to continue working without getting screen updates.
> > >
> > > Well, this is the most important discussion for now, but also even we
> > > change flush interface or only in PSR I'm not sure this will work.
> > >
> > > This dirty case happens in the middle of fbdev and when it gives control
> > > back to fbcon psr would be working without no one else invalidate or
> > > flushing it and we would miss screen updates anyway.
> > > But in this case I understand this is a hack and if we put the
> > > invalidate there I should also put a FIXME XXX explaining that...
> > 
> > Last week I discussed some of this with Rodrigo on IRC and we have
> > different interpretations on what the frontbuffer tracking calls mean.
> > Daniel, can you please clarify a few things?
> > 
> > One of the things I've discussed with Rodrigo is that I consider
> > invalidate+flush to be equivalent to just a flush call, while Rodrigo
> > sees both cases as different. Daniel, can you please clarify the
> > intentions of the frontbuffer tracking infrastructure here?
> 
> Yeah essentially flush without an invalidate is just a instantaneous
> invalidate+flush. invalidate just means "someone started to frontbuffer
> render and I have no idea at all when that will stop". Flush means
> "someone has done some frontbuffer rendering, make sure those updates land
> on the screen". So yeah Paulo's idea is correct.

Thanks for the clarification...

> 
> Problem is now that when we get a flip we also call flush, and for psr on
> big core that's probably not what we want since it can handle flips
> correctly, but not real frontbuffer flushes.
> 
> > One contradiction I see with the current code is that PSR does nothing
> > on flush() for BDW because it believes the HW tracking, but it does
> > psr_exit() on invalidate for every single case. If it relies on the HW
> > tracking, shouldn't it also do nothing on invalidate()? If it doesn't
> > rely on HW tracking, shouldn't it do something on flush()? If it
> > relies on just a few specific pieces of the HW tracking, shouldn't it
> > check enum fb_op_origin? The conclusion here is that the PSR code is
> > completely relying on having invalidate() calls before the flush() for
> > cases where it needs help from software.
> 
> Calling invalidate before flush isn't imo the right fix since usually
> (except when you have working hw tracking like fbc for gtt mmaps) you need
> to stop psr/fbc/whatever on invalidate. Whereas on flush you just need to
> make sure screen contents get to the screen.
> 
> I think the best option would be to wire up the fb_op_origin for flushes
> too, and then filter out ORIGIN_FLIP for psr (except for sprites on hsw)
> on big core.

Ok, this approach here works here when I have splash screen taking place
and X coming right after it. However we still have one issue that is
when it goes back to console.

fbdev on set_par invalidated front buffer, but then splash screen came
and flushed it, psr started working, then control gets back to fbdev and
nothing invalidates more so from this point on we miss many (if not all)
screen updates.

So we are going to a endless path where we could have many cases where
fbdev will be flushed with no reliable way to get invalidated again.

So, right now I just see 3 paths:
1. invalidate instead of flush on fb dirty callback just to make sure
everything gets invalidated when using dumb ioctls
2. change fbdev and all its clients to make a proper use of a dirty.
3. change fbdev, fbcon to propagate console switch (KD_TEXT info) where
we would have a way to know exactly who is in control
4. Introdoce IOCTLs for PSR and enable/disable it on DDX when we know
DDX has control.

I really don't like the idea to change fbdev scructures and its
clients...

Please let me know if you have other/better ideas and also what path you
prefer...


Thanks,
Rodrigo.
>  
> > Also, we've been discussing a boot splash screen, and, according to
> > what you said, it's supposed to be using the dumb interfaces, right?
> > So I see that the dumb mmap is just a gem_mmap() to our driver. But
> > PSR is supposed to get disabled after GTT mmaps. Why is it not
> > disabled in this case? That is because we don't get the invalidate
> > call after the mmap, right? If we require an invalidate on the dirty
> > ioctl, it means that PSR is enabled even though it is GTT mmapped...
> > That's supposed to be wrong, right? Shouldn't the mmap IOCTL force an
> > invalidate?
> 
> Yeah dumb mmap users are supposed to call dirtyfb after each draw call.
> 
> i915 userspace could start doing the same to mark the gtt mmap invalidate
> as done since we don't have any point where we stop it right now with an
> i915 ioctl. And to be able to work together with set_domain(GTT) it really
> needs to be a flush in ->dirty()
> -Daniel