[Intel-gfx] [PATCH 7/7] drm/i915/perf: add flushing ioctl

[Umesh Nerlige Ramappa]

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 at intel.com>
>> >>
>> >> 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 at intel.com>
>> >> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
>> > [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