* [PATCH] drm/i915: Move fd_install after last use of fence
@ 2023-02-03 16:49 Rob Clark
2023-02-03 18:15 ` Rob Clark
0 siblings, 1 reply; 3+ messages in thread
From: Rob Clark @ 2023-02-03 16:49 UTC (permalink / raw)
To: dri-devel
Cc: Tvrtko Ursulin, Rob Clark, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, Tvrtko Ursulin, David Airlie, Daniel Vetter,
Matthew Auld, Andrzej Hajda, Niranjana Vishwanathapura,
Maarten Lankhorst, Nirmoy Das, Jason A. Donenfeld,
Thomas Hellström, intel-gfx, open list
From: Rob Clark <robdclark@chromium.org>
Because eb_composite_fence_create() drops the fence_array reference
after creation of the sync_file, only the sync_file holds a ref to the
fence. But fd_install() makes that reference visable to userspace, so
it must be the last thing we do with the fence.
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
index f266b68cf012..0f2e056c02dd 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
@@ -3476,38 +3476,38 @@ i915_gem_do_execbuffer(struct drm_device *dev,
err_request:
eb_requests_get(&eb);
err = eb_requests_add(&eb, err);
if (eb.fences)
signal_fence_array(&eb, eb.composite_fence ?
eb.composite_fence :
&eb.requests[0]->fence);
+ if (unlikely(eb.gem_context->syncobj)) {
+ drm_syncobj_replace_fence(eb.gem_context->syncobj,
+ eb.composite_fence ?
+ eb.composite_fence :
+ &eb.requests[0]->fence);
+ }
+
if (out_fence) {
if (err == 0) {
fd_install(out_fence_fd, out_fence->file);
args->rsvd2 &= GENMASK_ULL(31, 0); /* keep in-fence */
args->rsvd2 |= (u64)out_fence_fd << 32;
out_fence_fd = -1;
} else {
fput(out_fence->file);
}
}
- if (unlikely(eb.gem_context->syncobj)) {
- drm_syncobj_replace_fence(eb.gem_context->syncobj,
- eb.composite_fence ?
- eb.composite_fence :
- &eb.requests[0]->fence);
- }
-
if (!out_fence && eb.composite_fence)
dma_fence_put(eb.composite_fence);
eb_requests_put(&eb);
err_vma:
eb_release_vmas(&eb, true);
WARN_ON(err == -EDEADLK);
i915_gem_ww_ctx_fini(&eb.ww);
--
2.38.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/i915: Move fd_install after last use of fence
2023-02-03 16:49 [PATCH] drm/i915: Move fd_install after last use of fence Rob Clark
@ 2023-02-03 18:15 ` Rob Clark
2023-02-06 9:35 ` Tvrtko Ursulin
0 siblings, 1 reply; 3+ messages in thread
From: Rob Clark @ 2023-02-03 18:15 UTC (permalink / raw)
To: dri-devel
Cc: Tvrtko Ursulin, Rob Clark, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, Tvrtko Ursulin, David Airlie, Daniel Vetter,
Matthew Auld, Andrzej Hajda, Niranjana Vishwanathapura,
Maarten Lankhorst, Nirmoy Das, Jason A. Donenfeld,
Thomas Hellström, intel-gfx, open list, jason.ekstrand
On Fri, Feb 3, 2023 at 8:49 AM Rob Clark <robdclark@gmail.com> wrote:
>
> From: Rob Clark <robdclark@chromium.org>
>
> Because eb_composite_fence_create() drops the fence_array reference
> after creation of the sync_file, only the sync_file holds a ref to the
> fence. But fd_install() makes that reference visable to userspace, so
> it must be the last thing we do with the fence.
>
Fixes: 00dae4d3d35d ("drm/i915: Implement SINGLE_TIMELINE with a syncobj (v4)")
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> ---
> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> index f266b68cf012..0f2e056c02dd 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
> @@ -3476,38 +3476,38 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>
> err_request:
> eb_requests_get(&eb);
> err = eb_requests_add(&eb, err);
>
> if (eb.fences)
> signal_fence_array(&eb, eb.composite_fence ?
> eb.composite_fence :
> &eb.requests[0]->fence);
>
> + if (unlikely(eb.gem_context->syncobj)) {
> + drm_syncobj_replace_fence(eb.gem_context->syncobj,
> + eb.composite_fence ?
> + eb.composite_fence :
> + &eb.requests[0]->fence);
> + }
> +
> if (out_fence) {
> if (err == 0) {
> fd_install(out_fence_fd, out_fence->file);
> args->rsvd2 &= GENMASK_ULL(31, 0); /* keep in-fence */
> args->rsvd2 |= (u64)out_fence_fd << 32;
> out_fence_fd = -1;
> } else {
> fput(out_fence->file);
> }
> }
>
> - if (unlikely(eb.gem_context->syncobj)) {
> - drm_syncobj_replace_fence(eb.gem_context->syncobj,
> - eb.composite_fence ?
> - eb.composite_fence :
> - &eb.requests[0]->fence);
> - }
> -
> if (!out_fence && eb.composite_fence)
> dma_fence_put(eb.composite_fence);
>
> eb_requests_put(&eb);
>
> err_vma:
> eb_release_vmas(&eb, true);
> WARN_ON(err == -EDEADLK);
> i915_gem_ww_ctx_fini(&eb.ww);
>
> --
> 2.38.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/i915: Move fd_install after last use of fence
2023-02-03 18:15 ` Rob Clark
@ 2023-02-06 9:35 ` Tvrtko Ursulin
0 siblings, 0 replies; 3+ messages in thread
From: Tvrtko Ursulin @ 2023-02-06 9:35 UTC (permalink / raw)
To: Rob Clark, dri-devel, Matthew Brost
Cc: Tvrtko Ursulin, Rob Clark, Jani Nikula, Joonas Lahtinen,
Rodrigo Vivi, David Airlie, Daniel Vetter, Matthew Auld,
Andrzej Hajda, Niranjana Vishwanathapura, Maarten Lankhorst,
Nirmoy Das, Jason A. Donenfeld, Thomas Hellström, intel-gfx,
open list, jason.ekstrand
On 03/02/2023 18:15, Rob Clark wrote:
> On Fri, Feb 3, 2023 at 8:49 AM Rob Clark <robdclark@gmail.com> wrote:
>>
>> From: Rob Clark <robdclark@chromium.org>
>>
>> Because eb_composite_fence_create() drops the fence_array reference
>> after creation of the sync_file, only the sync_file holds a ref to the
>> fence. But fd_install() makes that reference visable to userspace, so
>> it must be the last thing we do with the fence.
>>
>
> Fixes: 00dae4d3d35d ("drm/i915: Implement SINGLE_TIMELINE with a syncobj (v4)")
This is correct and the fix looks good to me.
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
CI is green so I will merge it, thanks again for a fix Rob!
Followup up question for Matthew Brost however is whether the composite
fence flow could be simplified. This block here comes late in
i915_gem_do_execbuffer and may mislead the user the composite fence is
held to the end of the function:
if (!out_fence && eb.composite_fence)
dma_fence_put(eb.composite_fence);
Question is would it work to remove the !out_fence condition from here,
and remove "consumption" of the reference from eb_composite_fence_create
success path.
Regards,
Tvrtko
>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> index f266b68cf012..0f2e056c02dd 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> @@ -3476,38 +3476,38 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>>
>> err_request:
>> eb_requests_get(&eb);
>> err = eb_requests_add(&eb, err);
>>
>> if (eb.fences)
>> signal_fence_array(&eb, eb.composite_fence ?
>> eb.composite_fence :
>> &eb.requests[0]->fence);
>>
>> + if (unlikely(eb.gem_context->syncobj)) {
>> + drm_syncobj_replace_fence(eb.gem_context->syncobj,
>> + eb.composite_fence ?
>> + eb.composite_fence :
>> + &eb.requests[0]->fence);
>> + }
>> +
>> if (out_fence) {
>> if (err == 0) {
>> fd_install(out_fence_fd, out_fence->file);
>> args->rsvd2 &= GENMASK_ULL(31, 0); /* keep in-fence */
>> args->rsvd2 |= (u64)out_fence_fd << 32;
>> out_fence_fd = -1;
>> } else {
>> fput(out_fence->file);
>> }
>> }
>>
>> - if (unlikely(eb.gem_context->syncobj)) {
>> - drm_syncobj_replace_fence(eb.gem_context->syncobj,
>> - eb.composite_fence ?
>> - eb.composite_fence :
>> - &eb.requests[0]->fence);
>> - }
>> -
>> if (!out_fence && eb.composite_fence)
>> dma_fence_put(eb.composite_fence);
>>
>> eb_requests_put(&eb);
>>
>> err_vma:
>> eb_release_vmas(&eb, true);
>> WARN_ON(err == -EDEADLK);
>> i915_gem_ww_ctx_fini(&eb.ww);
>>
>> --
>> 2.38.1
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-02-06 9:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-03 16:49 [PATCH] drm/i915: Move fd_install after last use of fence Rob Clark
2023-02-03 18:15 ` Rob Clark
2023-02-06 9:35 ` Tvrtko Ursulin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox