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

[Rodrigo Vivi]

On Wed, Mar 5, 2014 at 2:46 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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

I'm not sure I fully got your idea/plan here.

Should I try a psr_exit with delayed back at dirtyfb? and create a new
test case?
Or this it self isn't enough?

Btw, i'm using daily a hsw ult with psr enable and a kde environment
using the same psr_exit strategy here for baytrail with psr enabled
for more than 2 weeks without any issue so far.

> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br