Increment composite fence seqno on each fence creation.
Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf") Signed-off-by: Matthew Brost matthew.brost@intel.com --- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 2213f7b613da..96cf8361b017 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -3113,7 +3113,7 @@ eb_composite_fence_create(struct i915_execbuffer *eb, int out_fence_fd) fence_array = dma_fence_array_create(eb->num_batches, fences, eb->context->parallel.fence_context, - eb->context->parallel.seqno, + eb->context->parallel.seqno++, false); if (!fence_array) { kfree(fences);
On Tue, 14 Dec 2021, Matthew Brost matthew.brost@intel.com wrote:
Increment composite fence seqno on each fence creation.
Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf") Signed-off-by: Matthew Brost matthew.brost@intel.com
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 2213f7b613da..96cf8361b017 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -3113,7 +3113,7 @@ eb_composite_fence_create(struct i915_execbuffer *eb, int out_fence_fd) fence_array = dma_fence_array_create(eb->num_batches, fences, eb->context->parallel.fence_context,
eb->context->parallel.seqno,
eb->context->parallel.seqno++, false);
I have no idea what's going on, but the feeling I get from "code smells" just in this small snippet is that the seqno++ does not take the error path here into account.
BR, Jani.
On Tue, Dec 14, 2021 at 10:17:48PM +0200, Jani Nikula wrote:
On Tue, 14 Dec 2021, Matthew Brost matthew.brost@intel.com wrote:
Increment composite fence seqno on each fence creation.
Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf") Signed-off-by: Matthew Brost matthew.brost@intel.com
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 2213f7b613da..96cf8361b017 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -3113,7 +3113,7 @@ eb_composite_fence_create(struct i915_execbuffer *eb, int out_fence_fd) fence_array = dma_fence_array_create(eb->num_batches, fences, eb->context->parallel.fence_context,
eb->context->parallel.seqno,
eb->context->parallel.seqno++, false);I have no idea what's going on, but the feeling I get from "code smells" just in this small snippet is that the seqno++ does not take the error path here into account.
It does not take the error path into account, but it completely fine to skip seqno numbers. As long as next valid seqno is greater than the last valid seqno we should be fine.
Matt
BR, Jani.
-- Jani Nikula, Intel Open Source Graphics Center
On 12/14/2021 11:59, Matthew Brost wrote:
Increment composite fence seqno on each fence creation.
Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf") Signed-off-by: Matthew Brost matthew.brost@intel.com
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 2213f7b613da..96cf8361b017 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -3113,7 +3113,7 @@ eb_composite_fence_create(struct i915_execbuffer *eb, int out_fence_fd) fence_array = dma_fence_array_create(eb->num_batches, fences, eb->context->parallel.fence_context,
eb->context->parallel.seqno,
eb->context->parallel.seqno++,
As is, this change looks good. So: Reviewed-by: John Harrison John.C.Harrison@Intel.com
However, just spotted that dma_fence_array_create() takes the seqno value as an 'unsigned' even though it passes it on to an underlying dma-fence as a u64 (and all other dma-fence code uses u64 seqnos). That should probably be fixed (and our context::parallel::seqno changed from u32 to u64).
John.
false);if (!fence_array) { kfree(fences);
On Tue, 14 Dec 2021, Matthew Brost matthew.brost@intel.com wrote:
Increment composite fence seqno on each fence creation.
For future reference, this commit message is not enough. Both the subject line and the commit message just repeat what I can see from the code, i.e. *what* is being done, but there is not a hint on the *why* here.
BR, Jani.
Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf") Signed-off-by: Matthew Brost matthew.brost@intel.com
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 2213f7b613da..96cf8361b017 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -3113,7 +3113,7 @@ eb_composite_fence_create(struct i915_execbuffer *eb, int out_fence_fd) fence_array = dma_fence_array_create(eb->num_batches, fences, eb->context->parallel.fence_context,
eb->context->parallel.seqno,
eb->context->parallel.seqno++, false);
dri-devel@lists.freedesktop.org
-
Jani Nikula -
John Harrison -
Matthew Brost