[PATCH v2] drm/syncobj: ensure progress for syncobj queries

[PATCH v2] drm/syncobj: ensure progress for syncobj queries

[Chia-I Wu]

Userspace might poll a syncobj with the query ioctl.  Call
dma_fence_enable_sw_signaling to ensure dma_fence_is_signaled returns
true in finite time.

Fixes: 27b575a9aa2f ("drm/syncobj: add timeline payload query ioctl v6")
Signed-off-by: Chia-I Wu <olvaffe@gmail.com>

---

v2: add Signed-off-by and Fixes tags
---
 drivers/gpu/drm/drm_syncobj.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 4fcfc0b9b386c..58c5593c897a2 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1689,6 +1689,9 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
 			    DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) {
 				point = fence->seqno;
 			} else {
+				/* ensure forward progress */
+				dma_fence_enable_sw_signaling(fence);
+
 				dma_fence_chain_for_each(iter, fence) {
 					if (iter->context != fence->context) {
 						dma_fence_put(iter);
-- 
2.47.0.105.g07ac214952-goog

Re: [PATCH v2] drm/syncobj: ensure progress for syncobj queries

[Christian König]

Am 22.10.24 um 18:18 schrieb Chia-I Wu:
> Userspace might poll a syncobj with the query ioctl.  Call
> dma_fence_enable_sw_signaling to ensure dma_fence_is_signaled returns
> true in finite time.

Wait a second, just querying the fence status is absolutely not 
guaranteed to return true in finite time. That is well documented on the 
dma_fence() object.

When you want to poll on signaling from userspace you really need to 
call poll or the wait IOCTL with a zero timeout. That will also return 
immediately but should enable signaling while doing that.

So just querying the status should absolutely *not* enable signaling. 
That's an intentional separation.

Regards,
Christian.

>
> Fixes: 27b575a9aa2f ("drm/syncobj: add timeline payload query ioctl v6")
> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
>
> ---
>
> v2: add Signed-off-by and Fixes tags
> ---
>   drivers/gpu/drm/drm_syncobj.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 4fcfc0b9b386c..58c5593c897a2 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -1689,6 +1689,9 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
>   			    DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) {
>   				point = fence->seqno;
>   			} else {
> +				/* ensure forward progress */
> +				dma_fence_enable_sw_signaling(fence);
> +
>   				dma_fence_chain_for_each(iter, fence) {
>   					if (iter->context != fence->context) {
>   						dma_fence_put(iter);

Re: [PATCH v2] drm/syncobj: ensure progress for syncobj queries

[Chia-I Wu]

On Tue, Oct 22, 2024 at 9:53 AM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 22.10.24 um 18:18 schrieb Chia-I Wu:
> > Userspace might poll a syncobj with the query ioctl.  Call
> > dma_fence_enable_sw_signaling to ensure dma_fence_is_signaled returns
> > true in finite time.
>
> Wait a second, just querying the fence status is absolutely not
> guaranteed to return true in finite time. That is well documented on the
> dma_fence() object.
>
> When you want to poll on signaling from userspace you really need to
> call poll or the wait IOCTL with a zero timeout. That will also return
> immediately but should enable signaling while doing that.
>
> So just querying the status should absolutely *not* enable signaling.
> That's an intentional separation.
I think it depends on what semantics DRM_IOCTL_SYNCOBJ_QUERY should have.

If DRM_IOCTL_SYNCOBJ_QUERY is mainly for vulkan timeline semaphores,
it is a bit heavy if userspace has to do a
DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT before a query.


>
> Regards,
> Christian.
>
> >
> > Fixes: 27b575a9aa2f ("drm/syncobj: add timeline payload query ioctl v6")
> > Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> >
> > ---
> >
> > v2: add Signed-off-by and Fixes tags
> > ---
> >   drivers/gpu/drm/drm_syncobj.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> > index 4fcfc0b9b386c..58c5593c897a2 100644
> > --- a/drivers/gpu/drm/drm_syncobj.c
> > +++ b/drivers/gpu/drm/drm_syncobj.c
> > @@ -1689,6 +1689,9 @@ int drm_syncobj_query_ioctl(struct drm_device *dev, void *data,
> >                           DRM_SYNCOBJ_QUERY_FLAGS_LAST_SUBMITTED) {
> >                               point = fence->seqno;
> >                       } else {
> > +                             /* ensure forward progress */
> > +                             dma_fence_enable_sw_signaling(fence);
> > +
> >                               dma_fence_chain_for_each(iter, fence) {
> >                                       if (iter->context != fence->context) {
> >                                               dma_fence_put(iter);
>

Re: [PATCH v2] drm/syncobj: ensure progress for syncobj queries

[Chia-I Wu]

On Tue, Oct 22, 2024 at 10:24 AM Chia-I Wu <olvaffe@gmail.com> wrote:
>
> On Tue, Oct 22, 2024 at 9:53 AM Christian König
> <christian.koenig@amd.com> wrote:
> >
> > Am 22.10.24 um 18:18 schrieb Chia-I Wu:
> > > Userspace might poll a syncobj with the query ioctl.  Call
> > > dma_fence_enable_sw_signaling to ensure dma_fence_is_signaled returns
> > > true in finite time.
> >
> > Wait a second, just querying the fence status is absolutely not
> > guaranteed to return true in finite time. That is well documented on the
> > dma_fence() object.
> >
> > When you want to poll on signaling from userspace you really need to
> > call poll or the wait IOCTL with a zero timeout. That will also return
> > immediately but should enable signaling while doing that.
> >
> > So just querying the status should absolutely *not* enable signaling.
> > That's an intentional separation.
> I think it depends on what semantics DRM_IOCTL_SYNCOBJ_QUERY should have.
>
> If DRM_IOCTL_SYNCOBJ_QUERY is mainly for vulkan timeline semaphores,
> it is a bit heavy if userspace has to do a
> DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT before a query.

I filed a Mesa issue,
https://gitlab.freedesktop.org/mesa/mesa/-/issues/12094, and Faith
suggested a kernel-side fix as well.  Should we reconsider this?

Re: [PATCH v2] drm/syncobj: ensure progress for syncobj queries

[Chia-I Wu]

On Mon, Nov 4, 2024 at 11:32 PM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 04.11.24 um 22:32 schrieb Chia-I Wu:
>
> On Tue, Oct 22, 2024 at 10:24 AM Chia-I Wu <olvaffe@gmail.com> wrote:
>
> On Tue, Oct 22, 2024 at 9:53 AM Christian König
> <christian.koenig@amd.com> wrote:
>
> Am 22.10.24 um 18:18 schrieb Chia-I Wu:
>
> Userspace might poll a syncobj with the query ioctl.  Call
> dma_fence_enable_sw_signaling to ensure dma_fence_is_signaled returns
> true in finite time.
>
> Wait a second, just querying the fence status is absolutely not
> guaranteed to return true in finite time. That is well documented on the
> dma_fence() object.
>
> When you want to poll on signaling from userspace you really need to
> call poll or the wait IOCTL with a zero timeout. That will also return
> immediately but should enable signaling while doing that.
>
> So just querying the status should absolutely *not* enable signaling.
> That's an intentional separation.
>
> I think it depends on what semantics DRM_IOCTL_SYNCOBJ_QUERY should have.
>
>
> Well that's what I pointed out. The behavior of the QUERY IOCTL is based on the behavior of the dma_fence and the later is documented to do exactly what it currently does.
>
> If DRM_IOCTL_SYNCOBJ_QUERY is mainly for vulkan timeline semaphores,
> it is a bit heavy if userspace has to do a
> DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT before a query.
>
>
> Maybe you misunderstood me, you *only* have to call DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT and *not* _QUERY.
>
> The underlying dma_fence_wait_timeout() function is extra optimized so that zero timeout has only minimal overhead.
>
> This overhead is actually lower than _QUERY because that one actually queries the driver for the current status while _WAIT just assumes that the driver will signal the fence when ready from an interrupt.

The context here is that vkGetSemaphoreCounterValue calls QUERY to get
the timeline value.  WAIT does not replace QUERY.

Taking a step back, in the binary (singled/unsignaled) case, a WAIT
with zero timeout can get the up-to-date status.  But in the timeline
case, there is no direct way to get the up-to-date status if QUERY
must strictly be a wrapper for dma_fence_is_signaled.  It comes back
to what was QUERY designed for and can we change it?


>
> I filed a Mesa issue,
> https://gitlab.freedesktop.org/mesa/mesa/-/issues/12094, and Faith
> suggested a kernel-side fix as well.  Should we reconsider this?
>
>
> Wait a second, you might have an even bigger misconception here. The difference between waiting and querying is usually intentional!
>
> This is done so that for example on mobile devices you don't need to enable device interrupts, but rather query in defined intervals.
>
> This is a very common design pattern and while I don't know the wording of the Vulkan timeline extension it's quite likely that this is the intended use case.
Yeah, there are Vulkan CTS tests that query timeline semaphores
repeatedly for progress.  Those tests can fail because mesa translates
the queries directly to the QUERY ioctl.

As things are, enable_signaling is a requirement to query up-to-date
status no matter the syncobj is binary or a timeline.  Whether
enable_signaling enables interrupts or not depends on how the specific
fences are implemented.


>
> Regards,
> Christian.

Re: [PATCH v2] drm/syncobj: ensure progress for syncobj queries

[Boris Brezillon]

On Tue, 5 Nov 2024 09:56:22 -0800
Chia-I Wu <olvaffe@gmail.com> wrote:

> On Mon, Nov 4, 2024 at 11:32 PM Christian König
> <christian.koenig@amd.com> wrote:
> >
> > Am 04.11.24 um 22:32 schrieb Chia-I Wu:
> >
> > On Tue, Oct 22, 2024 at 10:24 AM Chia-I Wu <olvaffe@gmail.com> wrote:
> >
> > On Tue, Oct 22, 2024 at 9:53 AM Christian König
> > <christian.koenig@amd.com> wrote:
> >
> > Am 22.10.24 um 18:18 schrieb Chia-I Wu:
> >
> > Userspace might poll a syncobj with the query ioctl.  Call
> > dma_fence_enable_sw_signaling to ensure dma_fence_is_signaled returns
> > true in finite time.
> >
> > Wait a second, just querying the fence status is absolutely not
> > guaranteed to return true in finite time. That is well documented on the
> > dma_fence() object.
> >
> > When you want to poll on signaling from userspace you really need to
> > call poll or the wait IOCTL with a zero timeout. That will also return
> > immediately but should enable signaling while doing that.
> >
> > So just querying the status should absolutely *not* enable signaling.
> > That's an intentional separation.
> >
> > I think it depends on what semantics DRM_IOCTL_SYNCOBJ_QUERY should have.
> >
> >
> > Well that's what I pointed out. The behavior of the QUERY IOCTL is based on the behavior of the dma_fence and the later is documented to do exactly what it currently does.
> >
> > If DRM_IOCTL_SYNCOBJ_QUERY is mainly for vulkan timeline semaphores,
> > it is a bit heavy if userspace has to do a
> > DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT before a query.
> >
> >
> > Maybe you misunderstood me, you *only* have to call DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT and *not* _QUERY.
> >
> > The underlying dma_fence_wait_timeout() function is extra optimized so that zero timeout has only minimal overhead.
> >
> > This overhead is actually lower than _QUERY because that one actually queries the driver for the current status while _WAIT just assumes that the driver will signal the fence when ready from an interrupt.  
> 
> The context here is that vkGetSemaphoreCounterValue calls QUERY to get
> the timeline value.  WAIT does not replace QUERY.
> 
> Taking a step back, in the binary (singled/unsignaled) case, a WAIT
> with zero timeout can get the up-to-date status.  But in the timeline
> case, there is no direct way to get the up-to-date status if QUERY
> must strictly be a wrapper for dma_fence_is_signaled.  It comes back
> to what was QUERY designed for and can we change it?
> 
> 
> >
> > I filed a Mesa issue,
> > https://gitlab.freedesktop.org/mesa/mesa/-/issues/12094, and Faith
> > suggested a kernel-side fix as well.  Should we reconsider this?
> >
> >
> > Wait a second, you might have an even bigger misconception here. The difference between waiting and querying is usually intentional!
> >
> > This is done so that for example on mobile devices you don't need to enable device interrupts, but rather query in defined intervals.
> >
> > This is a very common design pattern and while I don't know the wording of the Vulkan timeline extension it's quite likely that this is the intended use case.  
> Yeah, there are Vulkan CTS tests that query timeline semaphores
> repeatedly for progress.  Those tests can fail because mesa translates
> the queries directly to the QUERY ioctl.
> 
> As things are, enable_signaling is a requirement to query up-to-date
> status no matter the syncobj is binary or a timeline.

I kinda agree with Chia-I here. What's the point of querying a timeline
syncobj if what we get in return is an outdated sync point? I get that
the overhead of enabling signalling exists, but if we stand on this
position, that means the QUERY ioctl is not suitable for
vkGetSemaphoreCounterValue() unless we first add a
WAIT(sync_point=0,timeout=0) to make sure signalling is enabled on all
fences contained by the dma_fence_chain backing the timeline syncobj.
And this has to be done for each vkGetSemaphoreCounterValue(), because
new sync points don't have signalling enabled by default.

At the very least, we should add a new DRM_SYNCOBJ_QUERY_FLAGS_ flag
(DRM_SYNCOBJ_QUERY_FLAGS_REFRESH_STATE?) to combine the
enable_signalling and query operations in a single ioctl. If we go
for this approach, that means mesa has to support both cases, and pick
the most optimal one if the kernel supports it.

Re: [PATCH v2] drm/syncobj: ensure progress for syncobj queries

[Boris Brezillon]

On Wed, 6 Nov 2024 13:44:20 +0100
Christian König <christian.koenig@amd.com> wrote:

> Am 06.11.24 um 09:14 schrieb Boris Brezillon:
> > On Tue, 5 Nov 2024 09:56:22 -0800
> > Chia-I Wu<olvaffe@gmail.com>  wrote:
> >  
> >> On Mon, Nov 4, 2024 at 11:32 PM Christian König
> >> <christian.koenig@amd.com>  wrote:  
> >>> Am 04.11.24 um 22:32 schrieb Chia-I Wu:
> >>>
> >>> On Tue, Oct 22, 2024 at 10:24 AM Chia-I Wu<olvaffe@gmail.com>  wrote:
> >>>
> >>> On Tue, Oct 22, 2024 at 9:53 AM Christian König
> >>> <christian.koenig@amd.com>  wrote:
> >>>
> >>> Am 22.10.24 um 18:18 schrieb Chia-I Wu:
> >>>
> >>> Userspace might poll a syncobj with the query ioctl.  Call
> >>> dma_fence_enable_sw_signaling to ensure dma_fence_is_signaled returns
> >>> true in finite time.
> >>>
> >>> Wait a second, just querying the fence status is absolutely not
> >>> guaranteed to return true in finite time. That is well documented on the
> >>> dma_fence() object.
> >>>
> >>> When you want to poll on signaling from userspace you really need to
> >>> call poll or the wait IOCTL with a zero timeout. That will also return
> >>> immediately but should enable signaling while doing that.
> >>>
> >>> So just querying the status should absolutely *not* enable signaling.
> >>> That's an intentional separation.
> >>>
> >>> I think it depends on what semantics DRM_IOCTL_SYNCOBJ_QUERY should have.
> >>>
> >>>
> >>> Well that's what I pointed out. The behavior of the QUERY IOCTL is based on the behavior of the dma_fence and the later is documented to do exactly what it currently does.
> >>>
> >>> If DRM_IOCTL_SYNCOBJ_QUERY is mainly for vulkan timeline semaphores,
> >>> it is a bit heavy if userspace has to do a
> >>> DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT before a query.
> >>>
> >>>
> >>> Maybe you misunderstood me, you *only* have to call DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT and *not* _QUERY.
> >>>
> >>> The underlying dma_fence_wait_timeout() function is extra optimized so that zero timeout has only minimal overhead.
> >>>
> >>> This overhead is actually lower than _QUERY because that one actually queries the driver for the current status while _WAIT just assumes that the driver will signal the fence when ready from an interrupt.  
> >> The context here is that vkGetSemaphoreCounterValue calls QUERY to get
> >> the timeline value.  WAIT does not replace QUERY.  
> 
> Oh, that is a really good argument.
> 
> >> Taking a step back, in the binary (singled/unsignaled) case, a WAIT
> >> with zero timeout can get the up-to-date status.  But in the timeline
> >> case, there is no direct way to get the up-to-date status if QUERY
> >> must strictly be a wrapper for dma_fence_is_signaled.  It comes back
> >> to what was QUERY designed for and can we change it?  
> 
> Yeah that is a really good point. If _QUERY implements a different 
> functionality than _WAIT than the usual distinction between polling and 
> interrupt based approach can't be applied here.
> 
> >>> I filed a Mesa issue,
> >>> https://gitlab.freedesktop.org/mesa/mesa/-/issues/12094, and Faith
> >>> suggested a kernel-side fix as well.  Should we reconsider this?
> >>>
> >>>
> >>> Wait a second, you might have an even bigger misconception here. The difference between waiting and querying is usually intentional!
> >>>
> >>> This is done so that for example on mobile devices you don't need to enable device interrupts, but rather query in defined intervals.
> >>>
> >>> This is a very common design pattern and while I don't know the wording of the Vulkan timeline extension it's quite likely that this is the intended use case.  
> >> Yeah, there are Vulkan CTS tests that query timeline semaphores
> >> repeatedly for progress.  Those tests can fail because mesa translates
> >> the queries directly to the QUERY ioctl.
> >>
> >> As things are, enable_signaling is a requirement to query up-to-date
> >> status no matter the syncobj is binary or a timeline.  
> > I kinda agree with Chia-I here. What's the point of querying a timeline
> > syncobj if what we get in return is an outdated sync point? I get that
> > the overhead of enabling signalling exists, but if we stand on this
> > position, that means the QUERY ioctl is not suitable for
> > vkGetSemaphoreCounterValue() unless we first add a
> > WAIT(sync_point=0,timeout=0) to make sure signalling is enabled on all
> > fences contained by the dma_fence_chain backing the timeline syncobj.
> > And this has to be done for each vkGetSemaphoreCounterValue(), because
> > new sync points don't have signalling enabled by default.  
> 
> The common driver design I know from other operating systems is that you 
> either poll without enabling interrupts or you enable interrupts and 
> wait for the async operation to complete.

The problem is that we don't really poll if we call ::signaled() on a
dma_fence_array. The test is based on the state the container was at
creation time. The only way to poll a fence_array right now is to
enable signalling. So maybe that's the problem we should solve here:
make dma_fence_array_signaled() iterate over the fences instead of
checking ::num_pending and returning false if it's not zero (see the
diff at the end of this email).

> 
> That distinction is really important for things like mobile devices 
> where interrupts are rather power costly.

Sure, I get the importance of keeping interrupts disabled for
power-savings.

> 
> > At the very least, we should add a new DRM_SYNCOBJ_QUERY_FLAGS_ flag
> > (DRM_SYNCOBJ_QUERY_FLAGS_REFRESH_STATE?) to combine the
> > enable_signalling and query operations in a single ioctl. If we go
> > for this approach, that means mesa has to support both cases, and pick
> > the most optimal one if the kernel supports it.  
> 
> Another problem we have here is that dma_fence_enable_signaling() can 
> mean two different things, maybe the documentation is a bit confusing:
> 
> 1) Enabling interrupts so that we don't need to call the 
> ops->is_signaled() driver callback.
> 
> 2) Asking preemption fences to actually signal because memory management 
> wants to do something.

Uh, I wasn't aware of (2). If it's document somewhere, I probably missed
that part.

> 
> So when this ops->is_signaled() callback is implemented it shouldn't be 
> necessary to enable signaling to query the state.

Agree on that, see my comment about fence_array() not necessarily doing
the right thing here.

> 
> To summarize: When you call _QUERY you shouldn't get an outdated sync 
> point. When you do this then there is something wrong with the backend 
> driver.

If by backend, you mean the dma_fence_array implementation, you're probably
right.

> 
> Regards,
> Christian.


--->8---
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c
index 8a08ffde31e7..c9222a065954 100644
--- a/drivers/dma-buf/dma-fence-array.c
+++ b/drivers/dma-buf/dma-fence-array.c
@@ -104,8 +104,22 @@ static bool dma_fence_array_signaled(struct dma_fence *fence)
 {
        struct dma_fence_array *array = to_dma_fence_array(fence);
 
-       if (atomic_read(&array->num_pending) > 0)
-               return false;
+       if (atomic_read(&array->num_pending) > 0) {
+               struct dma_fence *subfence;
+               unsigned i;
+
+               /*
+                * If signaling is enabled, we don't need to iterate over
+                * fences to get the state, we can rely on num_pending > 0.
+                */
+               if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags))
+                       return false;
+
+               dma_fence_array_for_each(subfence, i, fence) {
+                       if (!dma_fence_is_signaled(subfence))
+                               return false;
+               }
+       }
 
        dma_fence_array_clear_pending_error(array);
        return true;