Re: [Intel-gfx] [PATCH] drm/i915: Increment composite fence seqno
On Tue, 14 Dec 2021, Matthew Brost 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
> ---
> 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);
--
Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH] drm/i915: Increment composite fence seqno
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
---
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
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);
Re: [Intel-gfx] [PATCH] drm/i915: Increment composite fence seqno
On Tue, Dec 14, 2021 at 10:17:48PM +0200, Jani Nikula wrote:
> On Tue, 14 Dec 2021, Matthew Brost wrote:
> > Increment composite fence seqno on each fence creation.
> >
> > Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf")
> > Signed-off-by: Matthew Brost
> > ---
> > 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);
>
> 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
Re: [Intel-gfx] [PATCH] drm/i915: Increment composite fence seqno
On Tue, 14 Dec 2021, Matthew Brost wrote:
> Increment composite fence seqno on each fence creation.
>
> Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf")
> Signed-off-by: Matthew Brost
> ---
> 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);
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.
--
Jani Nikula, Intel Open Source Graphics Center
[Intel-gfx] [PATCH] drm/i915: Increment composite fence seqno
Increment composite fence seqno on each fence creation.
Fixes: 544460c33821 ("drm/i915: Multi-BB execbuf")
Signed-off-by: Matthew Brost
---
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);
--
2.33.1
