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

[Rodrigo Vivi]

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?

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre



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