* [Intel-gfx] [PATCH] drm/i915: remove questionable fence optimization during copy
@ 2021-12-21 14:07 Christian König
2021-12-21 15:47 ` Thomas Hellström
2022-01-10 13:27 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for " Patchwork
0 siblings, 2 replies; 4+ messages in thread
From: Christian König @ 2021-12-21 14:07 UTC (permalink / raw)
To: thomas.hellstrom, daniel, intel-gfx
First of all as discussed multiple times now kernel copies *must* always wait
for all fences in a BO before actually doing the copy. This is mandatory.
Additional to that drop the handling when there can't be a shared slot
allocated on the source BO and just properly return an error code. Otherwise
this code path would only be tested under out of memory conditions.
Signed-off-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 39 +++++++-------------
1 file changed, 14 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
index 80df9f592407..798f310d710c 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
@@ -250,19 +250,14 @@ static struct dma_fence *i915_deps_to_fence(struct i915_deps *deps,
}
static int i915_deps_add_resv(struct i915_deps *deps, struct dma_resv *resv,
- bool all, const bool no_excl,
const struct ttm_operation_ctx *ctx)
{
struct dma_resv_iter iter;
struct dma_fence *fence;
+ int ret;
dma_resv_assert_held(resv);
- dma_resv_for_each_fence(&iter, resv, all, fence) {
- int ret;
-
- if (no_excl && dma_resv_iter_is_exclusive(&iter))
- continue;
-
+ dma_resv_for_each_fence(&iter, resv, true, fence) {
ret = i915_deps_add_dependency(deps, fence, ctx);
if (ret)
return ret;
@@ -698,7 +693,7 @@ static struct dma_fence *prev_fence(struct ttm_buffer_object *bo,
* TODO: Only await excl fence here, and shared fences before
* signaling the migration fence.
*/
- ret = i915_deps_add_resv(&deps, bo->base.resv, true, false, ctx);
+ ret = i915_deps_add_resv(&deps, bo->base.resv, ctx);
if (ret)
return ERR_PTR(ret);
@@ -828,22 +823,21 @@ int i915_gem_obj_copy_ttm(struct drm_i915_gem_object *dst,
struct i915_refct_sgt *dst_rsgt;
struct dma_fence *copy_fence, *dep_fence;
struct i915_deps deps;
- int ret, shared_err;
+ int ret;
assert_object_held(dst);
assert_object_held(src);
i915_deps_init(&deps, GFP_KERNEL | __GFP_NORETRY | __GFP_NOWARN);
- /*
- * We plan to add a shared fence only for the source. If that
- * fails, we await all source fences before commencing
- * the copy instead of only the exclusive.
- */
- shared_err = dma_resv_reserve_shared(src_bo->base.resv, 1);
- ret = i915_deps_add_resv(&deps, dst_bo->base.resv, true, false, &ctx);
- if (!ret)
- ret = i915_deps_add_resv(&deps, src_bo->base.resv,
- !!shared_err, false, &ctx);
+ ret = dma_resv_reserve_shared(src_bo->base.resv, 1);
+ if (ret)
+ return ret;
+
+ ret = i915_deps_add_resv(&deps, dst_bo->base.resv, &ctx);
+ if (ret)
+ return ret;
+
+ ret = i915_deps_add_resv(&deps, src_bo->base.resv, &ctx);
if (ret)
return ret;
@@ -861,12 +855,7 @@ int i915_gem_obj_copy_ttm(struct drm_i915_gem_object *dst,
return PTR_ERR_OR_ZERO(copy_fence);
dma_resv_add_excl_fence(dst_bo->base.resv, copy_fence);
-
- /* If we failed to reserve a shared slot, add an exclusive fence */
- if (shared_err)
- dma_resv_add_excl_fence(src_bo->base.resv, copy_fence);
- else
- dma_resv_add_shared_fence(src_bo->base.resv, copy_fence);
+ dma_resv_add_shared_fence(src_bo->base.resv, copy_fence);
dma_fence_put(copy_fence);
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: remove questionable fence optimization during copy
2021-12-21 14:07 [Intel-gfx] [PATCH] drm/i915: remove questionable fence optimization during copy Christian König
@ 2021-12-21 15:47 ` Thomas Hellström
2021-12-22 7:47 ` Christian König
2022-01-10 13:27 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for " Patchwork
1 sibling, 1 reply; 4+ messages in thread
From: Thomas Hellström @ 2021-12-21 15:47 UTC (permalink / raw)
To: Christian König, daniel, intel-gfx
Hi, Christian,
On Tue, 2021-12-21 at 15:07 +0100, Christian König wrote:
> First of all as discussed multiple times now kernel copies *must*
> always wait
> for all fences in a BO before actually doing the copy. This is
> mandatory.
This patch looks ok to me.
Regarding the discussion I was just awaiting Daniel's reply from
yesterday:
https://lists.freedesktop.org/archives/intel-gfx/2021-December/285717.html
since his earlier reply
https://lists.freedesktop.org/archives/intel-gfx/2021-December/285717.html
contradicted your previous reply
https://lists.freedesktop.org/archives/intel-gfx/2021-December/284467.html
That confirmed all writes had to add an exclusive fence, and confirmed
that starting the blit early was ok.
So I was left a bit confused as to what the rules really were.
So now if I understand both of you correctly, writers that want to opt
out of implicit syncing do *not* need to add an exclusive fence. Is
that correct?
>
> Additional to that drop the handling when there can't be a shared
> slot
> allocated on the source BO and just properly return an error code.
> Otherwise
> this code path would only be tested under out of memory conditions.
Good point.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Ok if I add this to drm-intel-gt-next?
/Thomas
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: remove questionable fence optimization during copy
2021-12-21 15:47 ` Thomas Hellström
@ 2021-12-22 7:47 ` Christian König
0 siblings, 0 replies; 4+ messages in thread
From: Christian König @ 2021-12-22 7:47 UTC (permalink / raw)
To: Thomas Hellström, daniel, intel-gfx
Am 21.12.21 um 16:47 schrieb Thomas Hellström:
> Hi, Christian,
>
> On Tue, 2021-12-21 at 15:07 +0100, Christian König wrote:
>> First of all as discussed multiple times now kernel copies *must*
>> always wait
>> for all fences in a BO before actually doing the copy. This is
>> mandatory.
> This patch looks ok to me.
>
> Regarding the discussion I was just awaiting Daniel's reply from
> yesterday:
>
> https://lists.freedesktop.org/archives/intel-gfx/2021-December/285717.html
>
> since his earlier reply
>
> https://lists.freedesktop.org/archives/intel-gfx/2021-December/285717.html
>
> contradicted your previous reply
>
> https://lists.freedesktop.org/archives/intel-gfx/2021-December/284467.html
>
> That confirmed all writes had to add an exclusive fence, and confirmed
> that starting the blit early was ok.
>
> So I was left a bit confused as to what the rules really were.
>
> So now if I understand both of you correctly, writers that want to opt
> out of implicit syncing do *not* need to add an exclusive fence. Is
> that correct?
Yes, that's a good summary of the problem.
>> Additional to that drop the handling when there can't be a shared
>> slot
>> allocated on the source BO and just properly return an error code.
>> Otherwise
>> this code path would only be tested under out of memory conditions.
> Good point.
>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
>
> Ok if I add this to drm-intel-gt-next?
Please go ahead.
Thanks,
Christian.
>
> /Thomas
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Intel-gfx] ✗ Fi.CI.BUILD: failure for drm/i915: remove questionable fence optimization during copy
2021-12-21 14:07 [Intel-gfx] [PATCH] drm/i915: remove questionable fence optimization during copy Christian König
2021-12-21 15:47 ` Thomas Hellström
@ 2022-01-10 13:27 ` Patchwork
1 sibling, 0 replies; 4+ messages in thread
From: Patchwork @ 2022-01-10 13:27 UTC (permalink / raw)
To: Christian König; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: remove questionable fence optimization during copy
URL : https://patchwork.freedesktop.org/series/98679/
State : failure
== Summary ==
Applying: drm/i915: remove questionable fence optimization during copy
Using index info to reconstruct a base tree...
M drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 drm/i915: remove questionable fence optimization during copy
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-01-10 13:27 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-21 14:07 [Intel-gfx] [PATCH] drm/i915: remove questionable fence optimization during copy Christian König
2021-12-21 15:47 ` Thomas Hellström
2021-12-22 7:47 ` Christian König
2022-01-10 13:27 ` [Intel-gfx] ✗ Fi.CI.BUILD: failure for " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.