[Intel-gfx] [RFC] drm/i915: Temporarily go realtime when polling PCODE

[Tvrtko Ursulin]

On 22/02/2017 09:13, Imre Deak wrote:
> On Wed, Feb 22, 2017 at 07:52:01AM +0000, Tvrtko Ursulin wrote:
>>
>> On 21/02/2017 18:48, Imre Deak wrote:
>>> On Tue, Feb 21, 2017 at 05:01:58PM +0000, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>
>>>> Elevate task scheduling policy to realtime when polling on PCODE
>>>> to guarantee a good poll rate before falling back to busy wait.
>>>>
>>>> We only do this for tasks with normal policy and priority in
>>>> order  to simplify policy restore and also assuming that for
>>>> tasks which either made themselves low or high priority it makes
>>>> less sense to do so.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>> Cc: Imre Deak <imre.deak at intel.com>
>>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>>> ---
>>>> This was my idea as mentioned in the other thread.
>>>>
>>>> Deadline scheduling policy seems trickier to restore from so
>>>> I thought SCHED_FIFO should be good enough.
>>>>
>>>> Briefly tested but couldn't reproduce the timeout condition.
>>>
>>> Hm, I thought you wanted this instead of the preempt-disable poll. The
>>> first preempt-enable poll is what's based on the spec, which only
>>> requires two requests 3ms apart, so no requirement on the number of
>>> requests there. That works most of the time and the preempt-disable part
>>> is needed only rarely. So do we want to increase the priority for the
>>> normal case?
>>
>> So we end up in the busy loop case less often or never? (By polling better
>> in the sleeping loop.) It is possible I got this completely wrong mind you.
>> I was just going by what is written in this thread - that the problem is the
>> sleeping loop sometimes does not run the COND often enough, or enough times.
>
> Yes, but that means we also raise the priority for the usual case. That
> would make the first loop a similar busy loop to what we want to avoid,
> running that always. What I hope is that this is a problem in the PCODE
> firmware that will get solved eventually, so we don't need the WA; hence
> argued about keeping any WA separate.

Having read the spec I think I see both sides now.

Spec is actually suggesting we should busy-retry the pcode request for 
3ms in this case.

It doesn't say how many retries we are supposed to do and how it 
internally operates, which makes me unsure if our first more relaxed 
polling is perhaps causing or contributing to the issue.

One thing where we don't follow the spec is the timeout for the 
GEN6_PCODE_READY poll which spec says should be 150us and not 500ms. I 
don't know if this timeout was trigger in the bug reports? If not then 
it is not the direct issue. But could be a contributing one, so the 
question is why we decided to do it and shouldn't we change this one to 
the 150us busy wait instead (add wait_for_register_fw_us)?

Another thing is the 10-20us retry for the top level PCODE retry - spec 
does not mention we should wait before retrying so is this our decision 
to be nicer to the system?

In either case, if the poll for GEN6_PCODE_READY is >2us (busy spin 
limit before going to sleeping poll), and the higher level PCODE retry 
ends up much longer than the 10-20us written in the code, first due 
hardware taking longer than 2us to respond, and both due overall CPU 
load and scheduling latencies, we would be drifting away from what is 
prescribed in the spec.

But regardless, the fact that the fallback busy loop needs up to 34ms as 
well makes the last bit from the above a bit uncertain. Only if the 
non-compliant polling we do somehow confuses the hardware and then we 
end up having to busy poll longer than we normally would. Probably unlikely.

Regards,

Tvrtko