Re: [PATCH v2 1/3] drm/syncobj: call drm_syncobj_fence_add_wait when WAIT_AVAILABLE flag is set
On Thursday, February 22nd, 2024 at 11:34, Simon Ser wrote: > On Thursday, February 22nd, 2024 at 11:32, Simon Ser cont...@emersion.fr > wrote: > > > Thanks, pushed to drm-misc-next! > > As I write this, I realize I should've pushed the first patch to > drm-misc-fixes instead… Sorry about the fuss… > > Sima, Dave, what is the right thing to do here? Push a duplicate commit > to drm-misc-fixes? (I know that's not a great thing to do…) I've cherry-picked the fix to drm-misc-fixes with Maxime's ACK.
Re: [PATCH v2 1/3] drm/syncobj: call drm_syncobj_fence_add_wait when WAIT_AVAILABLE flag is set
On Thursday, February 22nd, 2024 at 11:32, Simon Ser wrote: > Thanks, pushed to drm-misc-next! As I write this, I realize I should've pushed the first patch to drm-misc-fixes instead… Sorry about the fuss… Sima, Dave, what is the right thing to do here? Push a duplicate commit to drm-misc-fixes? (I know that's not a great thing to do…)
Re: [PATCH v2 1/3] drm/syncobj: call drm_syncobj_fence_add_wait when WAIT_AVAILABLE flag is set
Sorry for the delay. The first patch is also Reviewed-by me, the rest is Acked-by because I only skimmed through them. Thanks, pushed to drm-misc-next! Please do ping about patches when they slip through the cracks.
Re: [PATCH v2 1/3] drm/syncobj: call drm_syncobj_fence_add_wait when WAIT_AVAILABLE flag is set
Can someone pick these up into misc?
Dave.
On Thu, 22 Feb 2024 at 04:48, Erik Kurzinger wrote:
>
> It looks these these patches have still not been merged after one month, is
> there anything more that needs to be done for this to happen?
>
> On 1/25/24 10:12, Daniel Vetter wrote:
> > On Fri, Jan 19, 2024 at 08:32:06AM -0800, Erik Kurzinger wrote:
> >> When waiting for a syncobj timeline point whose fence has not yet been
> >> submitted with the WAIT_FOR_SUBMIT flag, a callback is registered using
> >> drm_syncobj_fence_add_wait and the thread is put to sleep until the
> >> timeout expires. If the fence is submitted before then,
> >> drm_syncobj_add_point will wake up the sleeping thread immediately which
> >> will proceed to wait for the fence to be signaled.
> >>
> >> However, if the WAIT_AVAILABLE flag is used instead,
> >> drm_syncobj_fence_add_wait won't get called, meaning the waiting thread
> >> will always sleep for the full timeout duration, even if the fence gets
> >> submitted earlier. If it turns out that the fence *has* been submitted
> >> by the time it eventually wakes up, it will still indicate to userspace
> >> that the wait completed successfully (it won't return -ETIME), but it
> >> will have taken much longer than it should have.
> >>
> >> To fix this, we must call drm_syncobj_fence_add_wait if *either* the
> >> WAIT_FOR_SUBMIT flag or the WAIT_AVAILABLE flag is set. The only
> >> difference being that with WAIT_FOR_SUBMIT we will also wait for the
> >> fence to be signaled after it has been submitted while with
> >> WAIT_AVAILABLE we will return immediately.
> >>
> >> IGT test patch:
> >> https://lists.freedesktop.org/archives/igt-dev/2024-January/067537.html
> >>
> >> v1 -> v2: adjust lockdep_assert_none_held_once condition
> >>
> >> Fixes: 01d6c3578379 ("drm/syncobj: add support for timeline point wait v8")
> >> Signed-off-by: Erik Kurzinger
> >
> > Yeah I think this series catches now all the corner cases I spotted in v1.
> > On the series:
> >
> > Reviewed-by: Daniel Vetter
> >> ---
> >> drivers/gpu/drm/drm_syncobj.c | 6 --
> >> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> >> index 94ebc71e5be5..97be8b140599 100644
> >> --- a/drivers/gpu/drm/drm_syncobj.c
> >> +++ b/drivers/gpu/drm/drm_syncobj.c
> >> @@ -1058,7 +1058,8 @@ static signed long
> >> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
> >> uint64_t *points;
> >> uint32_t signaled_count, i;
> >>
> >> -if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT)
> >> +if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
> >> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE))
> >> lockdep_assert_none_held_once();
> >>
> >> points = kmalloc_array(count, sizeof(*points), GFP_KERNEL);
> >> @@ -1127,7 +1128,8 @@ static signed long
> >> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
> >> * fallthough and try a 0 timeout wait!
> >> */
> >>
> >> -if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
> >> +if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
> >> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE)) {
> >> for (i = 0; i < count; ++i)
> >> drm_syncobj_fence_add_wait(syncobjs[i], [i]);
> >> }
> >> --
> >> 2.43.0
> >>
> >
>
Re: [PATCH v2 1/3] drm/syncobj: call drm_syncobj_fence_add_wait when WAIT_AVAILABLE flag is set
It looks these these patches have still not been merged after one month, is
there anything more that needs to be done for this to happen?
On 1/25/24 10:12, Daniel Vetter wrote:
> On Fri, Jan 19, 2024 at 08:32:06AM -0800, Erik Kurzinger wrote:
>> When waiting for a syncobj timeline point whose fence has not yet been
>> submitted with the WAIT_FOR_SUBMIT flag, a callback is registered using
>> drm_syncobj_fence_add_wait and the thread is put to sleep until the
>> timeout expires. If the fence is submitted before then,
>> drm_syncobj_add_point will wake up the sleeping thread immediately which
>> will proceed to wait for the fence to be signaled.
>>
>> However, if the WAIT_AVAILABLE flag is used instead,
>> drm_syncobj_fence_add_wait won't get called, meaning the waiting thread
>> will always sleep for the full timeout duration, even if the fence gets
>> submitted earlier. If it turns out that the fence *has* been submitted
>> by the time it eventually wakes up, it will still indicate to userspace
>> that the wait completed successfully (it won't return -ETIME), but it
>> will have taken much longer than it should have.
>>
>> To fix this, we must call drm_syncobj_fence_add_wait if *either* the
>> WAIT_FOR_SUBMIT flag or the WAIT_AVAILABLE flag is set. The only
>> difference being that with WAIT_FOR_SUBMIT we will also wait for the
>> fence to be signaled after it has been submitted while with
>> WAIT_AVAILABLE we will return immediately.
>>
>> IGT test patch:
>> https://lists.freedesktop.org/archives/igt-dev/2024-January/067537.html
>>
>> v1 -> v2: adjust lockdep_assert_none_held_once condition
>>
>> Fixes: 01d6c3578379 ("drm/syncobj: add support for timeline point wait v8")
>> Signed-off-by: Erik Kurzinger
>
> Yeah I think this series catches now all the corner cases I spotted in v1.
> On the series:
>
> Reviewed-by: Daniel Vetter
>> ---
>> drivers/gpu/drm/drm_syncobj.c | 6 --
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>> index 94ebc71e5be5..97be8b140599 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -1058,7 +1058,8 @@ static signed long
>> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>> uint64_t *points;
>> uint32_t signaled_count, i;
>>
>> -if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT)
>> +if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
>> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE))
>> lockdep_assert_none_held_once();
>>
>> points = kmalloc_array(count, sizeof(*points), GFP_KERNEL);
>> @@ -1127,7 +1128,8 @@ static signed long
>> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>> * fallthough and try a 0 timeout wait!
>> */
>>
>> -if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
>> +if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
>> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE)) {
>> for (i = 0; i < count; ++i)
>> drm_syncobj_fence_add_wait(syncobjs[i], [i]);
>> }
>> --
>> 2.43.0
>>
>
Re: [PATCH v2 1/3] drm/syncobj: call drm_syncobj_fence_add_wait when WAIT_AVAILABLE flag is set
On Fri, Jan 19, 2024 at 08:32:06AM -0800, Erik Kurzinger wrote:
> When waiting for a syncobj timeline point whose fence has not yet been
> submitted with the WAIT_FOR_SUBMIT flag, a callback is registered using
> drm_syncobj_fence_add_wait and the thread is put to sleep until the
> timeout expires. If the fence is submitted before then,
> drm_syncobj_add_point will wake up the sleeping thread immediately which
> will proceed to wait for the fence to be signaled.
>
> However, if the WAIT_AVAILABLE flag is used instead,
> drm_syncobj_fence_add_wait won't get called, meaning the waiting thread
> will always sleep for the full timeout duration, even if the fence gets
> submitted earlier. If it turns out that the fence *has* been submitted
> by the time it eventually wakes up, it will still indicate to userspace
> that the wait completed successfully (it won't return -ETIME), but it
> will have taken much longer than it should have.
>
> To fix this, we must call drm_syncobj_fence_add_wait if *either* the
> WAIT_FOR_SUBMIT flag or the WAIT_AVAILABLE flag is set. The only
> difference being that with WAIT_FOR_SUBMIT we will also wait for the
> fence to be signaled after it has been submitted while with
> WAIT_AVAILABLE we will return immediately.
>
> IGT test patch:
> https://lists.freedesktop.org/archives/igt-dev/2024-January/067537.html
>
> v1 -> v2: adjust lockdep_assert_none_held_once condition
>
> Fixes: 01d6c3578379 ("drm/syncobj: add support for timeline point wait v8")
> Signed-off-by: Erik Kurzinger
Yeah I think this series catches now all the corner cases I spotted in v1.
On the series:
Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_syncobj.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index 94ebc71e5be5..97be8b140599 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -1058,7 +1058,8 @@ static signed long
> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
> uint64_t *points;
> uint32_t signaled_count, i;
>
> - if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT)
> + if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE))
> lockdep_assert_none_held_once();
>
> points = kmalloc_array(count, sizeof(*points), GFP_KERNEL);
> @@ -1127,7 +1128,8 @@ static signed long
> drm_syncobj_array_wait_timeout(struct drm_syncobj **syncobjs,
>* fallthough and try a 0 timeout wait!
>*/
>
> - if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
> + if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
> + DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE)) {
> for (i = 0; i < count; ++i)
> drm_syncobj_fence_add_wait(syncobjs[i], [i]);
> }
> --
> 2.43.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH v2 1/3] drm/syncobj: call drm_syncobj_fence_add_wait when WAIT_AVAILABLE flag is set
When waiting for a syncobj timeline point whose fence has not yet been
submitted with the WAIT_FOR_SUBMIT flag, a callback is registered using
drm_syncobj_fence_add_wait and the thread is put to sleep until the
timeout expires. If the fence is submitted before then,
drm_syncobj_add_point will wake up the sleeping thread immediately which
will proceed to wait for the fence to be signaled.
However, if the WAIT_AVAILABLE flag is used instead,
drm_syncobj_fence_add_wait won't get called, meaning the waiting thread
will always sleep for the full timeout duration, even if the fence gets
submitted earlier. If it turns out that the fence *has* been submitted
by the time it eventually wakes up, it will still indicate to userspace
that the wait completed successfully (it won't return -ETIME), but it
will have taken much longer than it should have.
To fix this, we must call drm_syncobj_fence_add_wait if *either* the
WAIT_FOR_SUBMIT flag or the WAIT_AVAILABLE flag is set. The only
difference being that with WAIT_FOR_SUBMIT we will also wait for the
fence to be signaled after it has been submitted while with
WAIT_AVAILABLE we will return immediately.
IGT test patch:
https://lists.freedesktop.org/archives/igt-dev/2024-January/067537.html
v1 -> v2: adjust lockdep_assert_none_held_once condition
Fixes: 01d6c3578379 ("drm/syncobj: add support for timeline point wait v8")
Signed-off-by: Erik Kurzinger
---
drivers/gpu/drm/drm_syncobj.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index 94ebc71e5be5..97be8b140599 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1058,7 +1058,8 @@ static signed long drm_syncobj_array_wait_timeout(struct
drm_syncobj **syncobjs,
uint64_t *points;
uint32_t signaled_count, i;
- if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT)
+ if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
+DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE))
lockdep_assert_none_held_once();
points = kmalloc_array(count, sizeof(*points), GFP_KERNEL);
@@ -1127,7 +1128,8 @@ static signed long drm_syncobj_array_wait_timeout(struct
drm_syncobj **syncobjs,
* fallthough and try a 0 timeout wait!
*/
- if (flags & DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT) {
+ if (flags & (DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT |
+DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE)) {
for (i = 0; i < count; ++i)
drm_syncobj_fence_add_wait(syncobjs[i], [i]);
}
--
2.43.0
