All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
To: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>,
	matthew.auld@intel.com,
	"Christian König" <christian.koenig@amd.com>
Subject: [PATCH v4 2/4] drm/i915: remove questionable fence optimization during copy
Date: Tue, 21 Dec 2021 21:00:48 +0100	[thread overview]
Message-ID: <20211221200050.436316-3-thomas.hellstrom@linux.intel.com> (raw)
In-Reply-To: <20211221200050.436316-1-thomas.hellstrom@linux.intel.com>

From: Christian König <christian.koenig@amd.com>

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>
Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 43 +++++++-------------
 1 file changed, 14 insertions(+), 29 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 62a6fd2d127d..5dbaccf3f201 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
@@ -195,19 +195,14 @@ static int i915_deps_add_dependency(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;
@@ -634,11 +629,7 @@ prev_deps(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx,
 
 	ret = i915_deps_add_dependency(deps, bo->moving, ctx);
 	if (!ret)
-		/*
-		 * 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);
 
 	return ret;
 }
@@ -768,22 +759,21 @@ int i915_gem_obj_copy_ttm(struct drm_i915_gem_object *dst,
 	struct i915_refct_sgt *dst_rsgt;
 	struct dma_fence *copy_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;
 
@@ -798,12 +788,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.31.1


WARNING: multiple messages have this Message-ID (diff)
From: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>
To: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Cc: "Thomas Hellström" <thomas.hellstrom@linux.intel.com>,
	matthew.auld@intel.com,
	"Christian König" <christian.koenig@amd.com>
Subject: [Intel-gfx] [PATCH v4 2/4] drm/i915: remove questionable fence optimization during copy
Date: Tue, 21 Dec 2021 21:00:48 +0100	[thread overview]
Message-ID: <20211221200050.436316-3-thomas.hellstrom@linux.intel.com> (raw)
In-Reply-To: <20211221200050.436316-1-thomas.hellstrom@linux.intel.com>

From: Christian König <christian.koenig@amd.com>

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>
Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c | 43 +++++++-------------
 1 file changed, 14 insertions(+), 29 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 62a6fd2d127d..5dbaccf3f201 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm_move.c
@@ -195,19 +195,14 @@ static int i915_deps_add_dependency(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;
@@ -634,11 +629,7 @@ prev_deps(struct ttm_buffer_object *bo, struct ttm_operation_ctx *ctx,
 
 	ret = i915_deps_add_dependency(deps, bo->moving, ctx);
 	if (!ret)
-		/*
-		 * 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);
 
 	return ret;
 }
@@ -768,22 +759,21 @@ int i915_gem_obj_copy_ttm(struct drm_i915_gem_object *dst,
 	struct i915_refct_sgt *dst_rsgt;
 	struct dma_fence *copy_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;
 
@@ -798,12 +788,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.31.1


  parent reply	other threads:[~2021-12-21 20:01 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-21 20:00 [PATCH v4 0/4] drm/i915: Asynchronous vma unbinding part1 Thomas Hellström
2021-12-21 20:00 ` [Intel-gfx] " Thomas Hellström
2021-12-21 20:00 ` [PATCH v4 1/4] drm/i915: Avoid using the i915_fence_array when collecting dependencies Thomas Hellström
2021-12-21 20:00   ` [Intel-gfx] " Thomas Hellström
2021-12-21 20:00 ` Thomas Hellström [this message]
2021-12-21 20:00   ` [Intel-gfx] [PATCH v4 2/4] drm/i915: remove questionable fence optimization during copy Thomas Hellström
2021-12-21 20:00 ` [PATCH v4 3/4] drm/i915: Break out the i915_deps utility Thomas Hellström
2021-12-21 20:00   ` [Intel-gfx] " Thomas Hellström
2021-12-21 20:00 ` [PATCH v4 4/4] drm/i915: Require the vm mutex for i915_vma_bind() Thomas Hellström
2021-12-21 20:00   ` [Intel-gfx] " Thomas Hellström
2021-12-21 22:44 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Asynchronous vma unbinding part1 Patchwork
2021-12-21 22:46 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2021-12-21 23:14 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2021-12-22  3:42 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211221200050.436316-3-thomas.hellstrom@linux.intel.com \
    --to=thomas.hellstrom@linux.intel.com \
    --cc=christian.koenig@amd.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=matthew.auld@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.