[Intel-gfx] [PATCH 5/7] drm/i915: handle interrupts from the OA unit

[Lionel Landwerlin]

On 16/01/2019 16:05, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2019-01-16 15:58:00)
>> On 16/01/2019 15:52, Chris Wilson wrote:
>>> Quoting Lionel Landwerlin (2019-01-16 15:36:20)
>>>> @@ -1877,6 +1883,21 @@ struct drm_i915_private {
>>>>                           wait_queue_head_t poll_wq;
>>>>                           bool pollin;
>>>>    
>>>> +                       /**
>>>> +                        * Atomic counter incremented by the interrupt
>>>> +                        * handling code for each OA half full interrupt
>>>> +                        * received.
>>>> +                        */
>>>> +                       atomic64_t half_full_count;
>>>> +
>>>> +                       /**
>>>> +                        * Copy of the atomic half_full_count that was last
>>>> +                        * processed in the i915-perf driver. If both counters
>>>> +                        * differ, there is data available to read in the OA
>>>> +                        * buffer.
>>>> +                        */
>>>> +                       u64 half_full_count_last;
>>> Eh? But why a relatively expensive atomic64. You only need one bit, and
>>> reading the tail pointer from WB memory should just be cheap. You should
>>> be able to sample the perf ringbuffer pointers very cheaply... What am I
>>> missing?
>>> -Chris
>>>
>> Fair comment.
>>
>> The thing is with this series there are 2 mechanism that notify the poll_wq.
>>
>> One is the hrtimer that kicks in at regular interval and polls the
>> register with the workaround.
>>
>> The other is the interrupt which doesn't read the registers and workaround.
> What's the complication with the workaround?


It's a bit more than just looking at registers, we actually have to look 
at the content of the buffer to figure out what landed in memory.

The register values are not synchronized with the memory writes...


There is a comment in the code (i915_perf_poll_locked) about not 
checking the register after each wakeup because that may be a very hot path.

The atomic64 sounded like a lesser evil.


>
>> Maybe a better way to do it would be to have 2 wait queues, one triggers
>> a workqueue for the oa_buffer_check after the interrupt, the other
>> notifies the existing poll_wq.
> Ok, I was expecting that both the hrtimer, interrupt and general signal
> handling (spurious wakeups) fed into the same mechanism to sample the
> ringbuffer and notify the client if ready. (And I presume that we sample
> from the client's process context anyway to save on the context switch.)


Correct, the client only deals with the output of oa_buffer_check().

The interrupt is just missing the oa_buffer_check() step which maybe we 
can do with a 2 stage thing.

Either timer+buffer_check -> wakeup

Or interrupt-> workqueue+buffer_check -> wakeup


-

Lionel


> -Chris
>