[Intel-gfx] [PATCH 10/11] drm/i915: PSR Baytrail: Force exit by inactivating it.

[Daniel Vetter]

On Mon, Mar 03, 2014 at 01:09:36PM -0300, Rodrigo Vivi wrote:
> On Sat, Mar 1, 2014 at 6:10 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> > On Sat, Mar 01, 2014 at 03:29:41PM -0300, Rodrigo Vivi wrote:
> >> On Sat, Mar 1, 2014 at 5:45 AM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> >> > On Fri, Feb 28, 2014 at 08:44:45PM -0300, Rodrigo Vivi wrote:
> >> >> Baytrail cannot easily detect screen updates and force PSR exit.
> >> >> So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy}
> >> >> and update to enable it back it later with a delayed workqueue.
> >> >
> >> > Why are we not checking if the object being accessed is indeed being
> >> > used for PSR? In set-domain, you only care about writes.
> >>
> >> Ok, so here you are suggesting that I on trigger psr_exit if it is
> >> write domain, right?
> >> I agree.
> >>
> >> > sw-finish and
> >> > busy are too late for psr_exit, the damage has already been done and
> >> > presumably the content may already be corrupted?
> >>
> >> I also agree but it is better late than never... or than letting
> >> userspace fully control the psr exit.
> >> Most of the cases are coverred by psr_exit at set_domain.
> >> The remaining cases where userspace set domain once, sleep for while
> >> (like 10 seconds) and than write something
> >> was coverred by my test that checked crc and it was different from crc
> >> base already.
> >>
> >> > Can you please explain
> >> > that it is safe to do an psr_exit after the damage is already in the scanount
> >> > based on the panel refresh cycle.
> >>
> >> The perfect solution is the hardware tracking the changes and doing
> >> psr_exit by itself,
> >> but unfortunatelly se don't have this scenario so we can live with what we have.
> >>
> >> If the idea is to allow userspace to let kernel know when to exit psr
> >> I'm in favor of a more generic
> >> ioctl called pre_damage to or a front_buffer_rend something to warn
> >> when some damage will occur
> >> and we can disable psr, fbc and do whatever we want before the damage
> >>
> >> if you prefer I can remove this patch and add the patch with ioclts
> >> that let psr_exit full control to userspace.
> >> These patches are ready already. I just really don't like this option
> >> because I don't like the idea to depend
> >> on userspace to get this hardware feature working.
> >
> > This is the level of detail that I want in the changelog and in nearby
> > comments. Keeping track of the weaknesses of the implementation is
> > vital.
> 
> agree
> 
> > For example there are problems with this approach if userspace flips
> > between two GTT mmapped buffers, it currently has no reason to call
> > set-domain again (and it never calls sw-finish along this path). So PSR
> > is enabled again, and userspace may continue to directly write into the
> > scanout without the kernel ever detecting and issuing the psr_exit().
> 
> I see...
> 
> > I do not believe there is a way to change the coherency semantics of the
> > GTT domain without userspace being aware, or else run afoul of a popular
> > corner case.
> 
> so, what do you suggest?
> ioctls for exit?

We have it, it's called dirtyfb. But we need to keep the current userspace
stuff mostly working first, hence all this trouble. We can fix the gtt
trouble with a delayed work (one vblank or so) which
a) does a psr_exit
b) shots down gtt mappings of the offending frontbuffer

Then we can intercept the next write in gem_fault.

Of course that will be horrible if we don't track correctly which buffers
are actually relevant for psr.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch