[Intel-gfx] [PATCH 1/3] drm/i915: Make fb user dirty operation to invalidate frontbuffer
Daniel Vetter
daniel at ffwll.ch
Wed Jul 8 02:05:45 PDT 2015
On Tue, Jul 07, 2015 at 08:23:46PM +0000, Vivi, Rodrigo wrote:
> On Tue, 2015-07-07 at 13:24 +0200, Daniel Vetter wrote:
> > On Mon, Jul 06, 2015 at 11:19:03PM +0000, Vivi, Rodrigo wrote:
> > > 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...
> >
> > As I mentioned we're not the only ones with exactly this problem, udl &
> > qxl have the same need for calling dirtyfb all over the place from fbdev
> > hooks. qxl seems to have the more complete solution using a workqueue.
>
> Yes, I agree. I just want to block PSR on this fbdev rework that is not
> simple, involve many maintainers and it is not the main PSR use case.
>
> >
> > What I'm still wondering about though is why we don't get an fbdev
> > invalidate when switching back to the console. boot splash should be doing
> > it's drawing on its own buffer, fbcon would switch back (which means a
> > set_par call) and that should invalidate. I have no idea why we don't get
> > this call in your setup ...
>
> You were right.... I wasn't relying on set_par but it is actually called
> on fbcon switch back, but the invalidate was happening only on the first
> time because write_domain was already GTT. So with the following patch
> we can solve the case where I press "Esc" key on the middle of splash
> screen:
> http://cgit.freedesktop.org/~vivijim/drm-intel/commit/?h=psr&id=ce7226054085080c10e53b82743a76e0593b785a
Oops I've accidentally broken this even worse with
commit 031b698a77a70a6c394568034437b5486a44e868
Author: Daniel Vetter <daniel.vetter at ffwll.ch>
Date: Fri Jun 26 19:35:16 2015 +0200
drm/i915: Unconditionally do fb tracking invalidate in set_domain
This commit fixes this exact bug (already in gtt write domain), but
unfortunately only for the set_domain ioctl, not for fbcon.
We really just need an fb invalidate in there, no need for the if.
> But I was facing also another issue with crypto pass and without any
> splash and trace lead me to fbdev_restore_mode so another hack:
> http://cgit.freedesktop.org/~vivijim/drm-intel/commit/?h=psr&id=9a7c3f7853a327a82551e335b6056b5e5362004a
Excellent catch, we definitely need that one too.
-Daniel
> All (an more) in
> http://cgit.freedesktop.org/~vivijim/drm-intel/log/?h=psr
> for now... while I'm not sure a flicker on fedora reported by community
> is not reliably solved:
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1236303
>
> Thanks,
> Rodrigo.
>
>
>
>
>
> > -Daniel
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list