On Wed, Mar 04, 2020 at 09:56:28PM -0800, Dixit, Ashutosh wrote:
On Wed, 04 Mar 2020 00:52:34 -0800, Lionel Landwerlin wrote:
On 04/03/2020 07:48, Dixit, Ashutosh wrote:
> On Tue, 03 Mar 2020 14:19:05 -0800, Umesh Nerlige Ramappa wrote:
>> From: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
>>
>> With the currently available parameters for the i915-perf stream,
>> there are still situations that are not well covered :
>>
>> If an application opens the stream with polling disable or at very low
>> frequency and OA interrupt enabled, no data will be available even
>> though somewhere between nothing and half of the OA buffer worth of
>> data might have landed in memory.
>>
>> To solve this issue we have a new flush ioctl on the perf stream that
>> forces the i915-perf driver to look at the state of the buffer when
>> called and makes any data available through both poll() & read() type
>> syscalls.
>>
>> v2: Version the ioctl (Joonas)
>> v3: Rebase (Umesh)
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx>
> [snip]
>
>> +/**
>> + * i915_perf_flush_data - handle `I915_PERF_IOCTL_FLUSH_DATA` ioctl
>> + * @stream: An enabled i915 perf stream
>> + *
>> + * The intention is to flush all the data available for reading from the OA
>> + * buffer
>> + */
>> +static void i915_perf_flush_data(struct i915_perf_stream *stream)
>> +{
>> + stream->pollin = oa_buffer_check(stream, true);
>> +}
> Since this function doesn't actually wake up any thread (which anyway can
> be done by sending a signal to the blocked thread), is the only purpose of
> this function to update OA buffer head/tail? But in that it is not clear
> why a separate ioctl should be created for this, can't the read() call
> itself call oa_buffer_check() to update the OA buffer head/tail?
>
> Again just trying to minimize uapi changes if possible.
Most applications will call read() after being notified by poll()/select()
that some data is available.
Correct this is the standard non blocking read behavior.
Changing that behavior will break some of the existing perf tests .
I am not suggesting changing that (that standard non blocking read
behavior).
If any data is available, this new ioctl will wake up existing waiters on
poll()/select().
The issue is we are not calling wake_up() in the above function to wake up
any blocked waiters. The ioctl will just update the OA buffer head/tail so
that (a) a subsequent blocking read will not block, or (b) a subsequent non
blocking read will return valid data (not -EAGAIN), or (c) a poll/select
will not block but return immediately saying data is available.
That is why it seems to me the ioctl is not required, updating the OA
buffer head/tail can be done as part of the read() (and the poll/select)
calls themselves.
We will investigate if this can be done and update the patches in the next
revision accordingly. Thanks!
resending (cc: Lionel)..
In this case, where we are trying to determine if there is any data in
the oa buffer before the next interrupt has fired, user could call poll
with a reasonable timeout to determine if data is available or not.
That would eliminate the need for the flush ioctl. Thoughts?
Thanks,
Umesh
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
