Re: [Intel-gfx] [PATCH 7/7] drm/i915/perf: add flushing ioctl
On 11/03/2020 05:05, Dixit, Ashutosh wrote:
On Tue, 10 Mar 2020 13:44:30 -0700, Lionel Landwerlin wrote:
On 09/03/2020 21:51, Umesh Nerlige Ramappa wrote:
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
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
Signed-off-by: Umesh Nerlige Ramappa
[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!
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
I almost forgot why this would cause problem.
Checking the state of the buffer every time you call poll() will pretty
much guarantee you have at least one report to read every time.
So that would lead to lot more wakeups :(
The whole system has to stay "unidirectional" with either interrupts or
timeout driving the wakeups.
This additional ioctl is the only solution I could find to add one more
input to the wakeup mechanism.
Well, aren't we asking the app to sleep for time T and then call flush
(followed by read)? Then we might as well ask them to sleep for time T and
call poll? Or we can ask them set the hrtimer to T, skip the sleep and call
poll (followed by read)? Aren't these 3 mechanisms equivalent? To me the
last option seems to be the cleanest. Thanks!
I guess one thing that could work is to call oa_buffer_check() at the
top of i915_oa_read().
Maybe you proposed this in a previous email.
That way poll() doesn't trigger too many wake ups and right before
closing the FD will pull the remaining data.
One side effect of this is that read() might almost always return
something because the HW is faster to write the OA buffer than the app
can call read().
Still I think it still works out.
What do you think?
-Lionel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/perf: add flushing ioctl
On Tue, 10 Mar 2020 13:44:30 -0700, Lionel Landwerlin wrote:
>
> On 09/03/2020 21:51, Umesh Nerlige Ramappa wrote:
> > 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
> >>> >>
> >>> >> 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
> >>> >> Signed-off-by: Umesh Nerlige Ramappa
> >>>
> >>> > [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!
> >
> > 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
>
>
> I almost forgot why this would cause problem.
>
> Checking the state of the buffer every time you call poll() will pretty
> much guarantee you have at least one report to read every time.
>
> So that would lead to lot more wakeups :(
>
> The whole system has to stay "unidirectional" with either interrupts or
> timeout driving the wakeups.
>
> This additional ioctl is the only solution I could find to add one more
> input to the wakeup mechanism.
Well, aren't we asking the app to sleep for time T and then call flush
(followed by read)? Then we might as well ask them to sleep for time T and
call poll? Or we can ask them set the hrtimer to T, skip the sleep and call
poll (followed by read)? Aren't these 3 mechanisms equivalent? To me the
last option seems to be the cleanest. Thanks!
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/perf: add flushing ioctl
On 09/03/2020 21:51, Umesh Nerlige Ramappa wrote:
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
>>
>> 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
>> Signed-off-by: Umesh Nerlige Ramappa
> [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!
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
I almost forgot why this would cause problem.
Checking the state of the buffer every time you call poll() will pretty
much guarantee you have at least one report to read every time.
So that would lead to lot more wakeups :(
The whole system has to stay "unidirectional" with either interrupts or
timeout driving the wakeups.
This additional ioctl is the only solution I could find to add one more
input to the wakeup mechanism.
-Lionel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/perf: add flushing ioctl
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
>>
>> 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
>> Signed-off-by: Umesh Nerlige Ramappa
> [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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/perf: add flushing ioctl
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
>>
>> 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
>> Signed-off-by: Umesh Nerlige Ramappa
> [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!
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/perf: add flushing ioctl
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
> >>
> >> 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
> >> Signed-off-by: Umesh Nerlige Ramappa
> > [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!
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/perf: add flushing ioctl
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
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
Signed-off-by: Umesh Nerlige Ramappa
[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.
Changing that behavior will break some of the existing perf tests .
If any data is available, this new ioctl will wake up existing waiters
on poll()/select().
-Lionel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/perf: add flushing ioctl
On Tue, 03 Mar 2020 14:19:05 -0800, Umesh Nerlige Ramappa wrote:
>
> From: Lionel Landwerlin
>
> 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
> Signed-off-by: Umesh Nerlige Ramappa
[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.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 7/7] drm/i915/perf: add flushing ioctl
From: Lionel Landwerlin
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
Signed-off-by: Umesh Nerlige Ramappa
---
drivers/gpu/drm/i915/i915_perf.c | 18 ++
include/uapi/drm/i915_drm.h | 21 +
2 files changed, 39 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index ab41cba85b40..b6cb47e80b86 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -3221,6 +3221,18 @@ static void i915_perf_disable_locked(struct
i915_perf_stream *stream)
stream->ops->disable(stream);
}
+/**
+ * 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);
+}
+
static long i915_perf_config_locked(struct i915_perf_stream *stream,
unsigned long metrics_set)
{
@@ -3282,6 +3294,9 @@ static long i915_perf_ioctl_locked(struct
i915_perf_stream *stream,
return 0;
case I915_PERF_IOCTL_CONFIG:
return i915_perf_config_locked(stream, arg);
+ case I915_PERF_IOCTL_FLUSH_DATA:
+ i915_perf_flush_data(stream);
+ return 0;
}
return -EINVAL;
@@ -4551,6 +4566,9 @@ int i915_perf_ioctl_version(void)
*
* 5: Add DRM_I915_PERF_PROP_OA_ENABLE_INTERRUPT paramter to
*enable/disable interrupts in OA.
+*
+* 6: Add ioctl to flush OA data before reading.
+*I915_PERF_IOCTL_FLUSH_DATA
*/
return 5;
}
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index f609ff4ceccb..3fd6bb189248 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -2044,6 +2044,27 @@ struct drm_i915_perf_open_param {
*/
#define I915_PERF_IOCTL_CONFIG _IO('i', 0x2)
+/**
+ * Actively check the availability of data from a stream.
+ *
+ * A stream data availability can be driven by two types of events :
+ *
+ * - if enabled, the kernel's hrtimer checking the amount of available data
+ * in the OA buffer through head/tail registers.
+ *
+ * - if enabled, the OA unit's interrupt mechanism
+ *
+ * The kernel hrtimer incur a cost of running callback at fixed time
+ * intervals, while the OA interrupt might only happen rarely. In the
+ * situation where the application has disabled the kernel's hrtimer and only
+ * uses the OA interrupt to know about available data, the application can
+ * request an active check of the available OA data through this ioctl. This
+ * will make any data in the OA buffer available with either poll() or read().
+ *
+ * This ioctl is available in perf revision 6.
+ */
+#define I915_PERF_IOCTL_FLUSH_DATA _IO('i', 0x3)
+
/**
* Common to all i915 perf records
*/
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/perf: add flushing ioctl
Quoting Lionel Landwerlin (2019-02-04 17:30:12)
> On 22/01/2019 16:25, Joonas Lahtinen wrote:
> > Quoting Lionel Landwerlin (2019-01-16 17:36:22)
> >> 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.
> >>
> >> Signed-off-by: Lionel Landwerlin
> > Link to userspace changes?
> >
> > Regards, Joonas
>
>
> Hey Joonas,
>
>
> I'm about to make the changes in gputop for the high frequency sampling
> use case.
>
>
> One thing I would like to know is whether these new features should be
> reported through a flag.
>
> So far we haven't added any new option to the i915/perf driver since the
> initial upstreaming series.
>
>
> The way I'm currently detecting newly added parameters is by using
> trying to open the stream with a value that I know will report ENOENT
> rather than EINVAL when the feature is not available :
>
> https://github.com/djdeath/intel-gpu-tools/blob/wip/djdeath/oa-interrupts/tests/perf.c#L4345
>
> Is there a better way to do this?
I think I already responded in IRC, but here goes for e-mail archives.
A GETPARAM with rolling i915/perf version would probably be a justified
versioning interface.
Regards, Joonas
>
>
> Thanks,
>
>
> -Lionel
>
>
> >
> >> ---
> >> drivers/gpu/drm/i915/i915_perf.c | 17 +
> >> include/uapi/drm/i915_drm.h | 19 +++
> >> 2 files changed, 36 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_perf.c
> >> b/drivers/gpu/drm/i915/i915_perf.c
> >> index da721fce2543..6c98ffa2135e 100644
> >> --- a/drivers/gpu/drm/i915/i915_perf.c
> >> +++ b/drivers/gpu/drm/i915/i915_perf.c
> >> @@ -2433,6 +2433,20 @@ static void i915_perf_disable_locked(struct
> >> i915_perf_stream *stream)
> >> stream->ops->disable(stream);
> >> }
> >>
> >> +/**
> >> + * 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)
> >> +{
> >> + struct drm_i915_private *dev_priv = stream->dev_priv;
> >> +
> >> + dev_priv->perf.oa.pollin = oa_buffer_check(stream->dev_priv, true);
> >> +}
> >> +
> >> /**
> >>* i915_perf_ioctl - support ioctl() usage with i915 perf stream FDs
> >>* @stream: An i915 perf stream
> >> @@ -2456,6 +2470,9 @@ static long i915_perf_ioctl_locked(struct
> >> i915_perf_stream *stream,
> >> case I915_PERF_IOCTL_DISABLE:
> >> i915_perf_disable_locked(stream);
> >> return 0;
> >> + case I915_PERF_IOCTL_FLUSH_DATA:
> >> + i915_perf_flush_data(stream);
> >> + return 0;
> >> }
> >>
> >> return -EINVAL;
> >> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> >> index b6db5e544a35..0f0d20080572 100644
> >> --- a/include/uapi/drm/i915_drm.h
> >> +++ b/include/uapi/drm/i915_drm.h
> >> @@ -1594,6 +1594,25 @@ struct drm_i915_perf_open_param {
> >>*/
> >> #define I915_PERF_IOCTL_DISABLE_IO('i', 0x1)
> >>
> >> +/**
> >> + * Actively check the availability of data from a stream.
> >> + *
> >> + * A stream data availability can be driven by two types of events :
> >> + *
> >> + * - if enabled, the kernel's hrtimer checking the amount of available
> >> data
> >> + * in the OA buffer through head/tail registers.
> >> + *
> >> + * - if enabled, the OA unit's interrupt mechanism
> >> + *
> >> + * The kernel hrtimer incur a cost of running callback at fixed time
> >> + * intervals, while the OA interrupt might only happen rarely. In the
> >> + * situation where the application has disabled the kernel's hrtimer and
> >> only
> >> + * uses the OA interrupt to know about available data, the application can
> >> + * request an active check of the available OA data through this ioctl.
> >> This
> >> + * will make any data in the OA buffer available with either poll() or
> >> read().
> >> + */
> >> +#define I915_PERF_IOCTL_FLUSH_DATA _IO('i', 0x2)
> >> +
> >> /**
> >>* Common to all i915 perf records
> >>*/
> >> --
> >> 2.20.1
> >>
> >> ___
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
Re: [Intel-gfx] [PATCH 7/7] drm/i915/perf: add flushing ioctl
On 22/01/2019 16:25, Joonas Lahtinen wrote:
Quoting Lionel Landwerlin (2019-01-16 17:36:22)
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.
Signed-off-by: Lionel Landwerlin
Link to userspace changes?
Regards, Joonas
Hey Joonas,
I'm about to make the changes in gputop for the high frequency sampling
use case.
One thing I would like to know is whether these new features should be
reported through a flag.
So far we haven't added any new option to the i915/perf driver since the
initial upstreaming series.
The way I'm currently detecting newly added parameters is by using
trying to open the stream with a value that I know will report ENOENT
rather than EINVAL when the feature is not available :
https://github.com/djdeath/intel-gpu-tools/blob/wip/djdeath/oa-interrupts/tests/perf.c#L4345
Is there a better way to do this?
Thanks,
-Lionel
---
drivers/gpu/drm/i915/i915_perf.c | 17 +
include/uapi/drm/i915_drm.h | 19 +++
2 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index da721fce2543..6c98ffa2135e 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2433,6 +2433,20 @@ static void i915_perf_disable_locked(struct
i915_perf_stream *stream)
stream->ops->disable(stream);
}
+/**
+ * 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)
+{
+ struct drm_i915_private *dev_priv = stream->dev_priv;
+
+ dev_priv->perf.oa.pollin = oa_buffer_check(stream->dev_priv, true);
+}
+
/**
* i915_perf_ioctl - support ioctl() usage with i915 perf stream FDs
* @stream: An i915 perf stream
@@ -2456,6 +2470,9 @@ static long i915_perf_ioctl_locked(struct
i915_perf_stream *stream,
case I915_PERF_IOCTL_DISABLE:
i915_perf_disable_locked(stream);
return 0;
+ case I915_PERF_IOCTL_FLUSH_DATA:
+ i915_perf_flush_data(stream);
+ return 0;
}
return -EINVAL;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index b6db5e544a35..0f0d20080572 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1594,6 +1594,25 @@ struct drm_i915_perf_open_param {
*/
#define I915_PERF_IOCTL_DISABLE_IO('i', 0x1)
+/**
+ * Actively check the availability of data from a stream.
+ *
+ * A stream data availability can be driven by two types of events :
+ *
+ * - if enabled, the kernel's hrtimer checking the amount of available data
+ * in the OA buffer through head/tail registers.
+ *
+ * - if enabled, the OA unit's interrupt mechanism
+ *
+ * The kernel hrtimer incur a cost of running callback at fixed time
+ * intervals, while the OA interrupt might only happen rarely. In the
+ * situation where the application has disabled the kernel's hrtimer and only
+ * uses the OA interrupt to know about available data, the application can
+ * request an active check of the available OA data through this ioctl. This
+ * will make any data in the OA buffer available with either poll() or read().
+ */
+#define I915_PERF_IOCTL_FLUSH_DATA _IO('i', 0x2)
+
/**
* Common to all i915 perf records
*/
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/perf: add flushing ioctl
On 22/01/2019 16:25, Joonas Lahtinen wrote: Quoting Lionel Landwerlin (2019-01-16 17:36:22) 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. Signed-off-by: Lionel Landwerlin Link to userspace changes? Regards, Joonas Trying to get them uploaded to a branch on https://github.com/intel/metrics-discovery I'll let you know when it's available. -Lionel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915/perf: add flushing ioctl
Quoting Lionel Landwerlin (2019-01-16 17:36:22)
> 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.
>
> Signed-off-by: Lionel Landwerlin
Link to userspace changes?
Regards, Joonas
> ---
> drivers/gpu/drm/i915/i915_perf.c | 17 +
> include/uapi/drm/i915_drm.h | 19 +++
> 2 files changed, 36 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c
> b/drivers/gpu/drm/i915/i915_perf.c
> index da721fce2543..6c98ffa2135e 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -2433,6 +2433,20 @@ static void i915_perf_disable_locked(struct
> i915_perf_stream *stream)
> stream->ops->disable(stream);
> }
>
> +/**
> + * 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)
> +{
> + struct drm_i915_private *dev_priv = stream->dev_priv;
> +
> + dev_priv->perf.oa.pollin = oa_buffer_check(stream->dev_priv, true);
> +}
> +
> /**
> * i915_perf_ioctl - support ioctl() usage with i915 perf stream FDs
> * @stream: An i915 perf stream
> @@ -2456,6 +2470,9 @@ static long i915_perf_ioctl_locked(struct
> i915_perf_stream *stream,
> case I915_PERF_IOCTL_DISABLE:
> i915_perf_disable_locked(stream);
> return 0;
> + case I915_PERF_IOCTL_FLUSH_DATA:
> + i915_perf_flush_data(stream);
> + return 0;
> }
>
> return -EINVAL;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index b6db5e544a35..0f0d20080572 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1594,6 +1594,25 @@ struct drm_i915_perf_open_param {
> */
> #define I915_PERF_IOCTL_DISABLE_IO('i', 0x1)
>
> +/**
> + * Actively check the availability of data from a stream.
> + *
> + * A stream data availability can be driven by two types of events :
> + *
> + * - if enabled, the kernel's hrtimer checking the amount of available data
> + * in the OA buffer through head/tail registers.
> + *
> + * - if enabled, the OA unit's interrupt mechanism
> + *
> + * The kernel hrtimer incur a cost of running callback at fixed time
> + * intervals, while the OA interrupt might only happen rarely. In the
> + * situation where the application has disabled the kernel's hrtimer and only
> + * uses the OA interrupt to know about available data, the application can
> + * request an active check of the available OA data through this ioctl. This
> + * will make any data in the OA buffer available with either poll() or
> read().
> + */
> +#define I915_PERF_IOCTL_FLUSH_DATA _IO('i', 0x2)
> +
> /**
> * Common to all i915 perf records
> */
> --
> 2.20.1
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 7/7] drm/i915/perf: add flushing ioctl
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.
Signed-off-by: Lionel Landwerlin
---
drivers/gpu/drm/i915/i915_perf.c | 17 +
include/uapi/drm/i915_drm.h | 19 +++
2 files changed, 36 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index da721fce2543..6c98ffa2135e 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2433,6 +2433,20 @@ static void i915_perf_disable_locked(struct
i915_perf_stream *stream)
stream->ops->disable(stream);
}
+/**
+ * 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)
+{
+ struct drm_i915_private *dev_priv = stream->dev_priv;
+
+ dev_priv->perf.oa.pollin = oa_buffer_check(stream->dev_priv, true);
+}
+
/**
* i915_perf_ioctl - support ioctl() usage with i915 perf stream FDs
* @stream: An i915 perf stream
@@ -2456,6 +2470,9 @@ static long i915_perf_ioctl_locked(struct
i915_perf_stream *stream,
case I915_PERF_IOCTL_DISABLE:
i915_perf_disable_locked(stream);
return 0;
+ case I915_PERF_IOCTL_FLUSH_DATA:
+ i915_perf_flush_data(stream);
+ return 0;
}
return -EINVAL;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index b6db5e544a35..0f0d20080572 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -1594,6 +1594,25 @@ struct drm_i915_perf_open_param {
*/
#define I915_PERF_IOCTL_DISABLE_IO('i', 0x1)
+/**
+ * Actively check the availability of data from a stream.
+ *
+ * A stream data availability can be driven by two types of events :
+ *
+ * - if enabled, the kernel's hrtimer checking the amount of available data
+ * in the OA buffer through head/tail registers.
+ *
+ * - if enabled, the OA unit's interrupt mechanism
+ *
+ * The kernel hrtimer incur a cost of running callback at fixed time
+ * intervals, while the OA interrupt might only happen rarely. In the
+ * situation where the application has disabled the kernel's hrtimer and only
+ * uses the OA interrupt to know about available data, the application can
+ * request an active check of the available OA data through this ioctl. This
+ * will make any data in the OA buffer available with either poll() or read().
+ */
+#define I915_PERF_IOCTL_FLUSH_DATA _IO('i', 0x2)
+
/**
* Common to all i915 perf records
*/
--
2.20.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
