* Re: [Intel-gfx] [PATCH] drm/i915: Increment composite fence seqno
2021-12-14 19:59 [PATCH] drm/i915: Increment composite fence seqno Matthew Brost
@ 2021-12-14 20:17 ` Jani Nikula
2021-12-14 20:15 ` Matthew Brost
2021-12-23 21:32 ` John Harrison
2021-12-29 11:29 ` [Intel-gfx] " Jani Nikula
2 siblings, 1 reply; 5+ messages in thread
From: Jani Nikula @ 2021-12-14 20:17 UTC (permalink / raw)
To: Matthew Brost, intel-gfx, dri-devel
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);
> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915: Increment composite fence seqno
2021-12-14 20:17 ` [Intel-gfx] " Jani Nikula
@ 2021-12-14 20:15 ` Matthew Brost
0 siblings, 0 replies; 5+ messages in thread
From: Matthew Brost @ 2021-12-14 20:15 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx, dri-devel
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);
> > 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Increment composite fence seqno
2021-12-14 19:59 [PATCH] drm/i915: Increment composite fence seqno Matthew Brost
2021-12-14 20:17 ` [Intel-gfx] " Jani Nikula
@ 2021-12-23 21:32 ` John Harrison
2021-12-29 11:29 ` [Intel-gfx] " Jani Nikula
2 siblings, 0 replies; 5+ messages in thread
From: John Harrison @ 2021-12-23 21:32 UTC (permalink / raw)
To: Matthew Brost, intel-gfx, dri-devel; +Cc: daniele.ceraolospurio
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);
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [Intel-gfx] [PATCH] drm/i915: Increment composite fence seqno
2021-12-14 19:59 [PATCH] drm/i915: Increment composite fence seqno Matthew Brost
2021-12-14 20:17 ` [Intel-gfx] " Jani Nikula
2021-12-23 21:32 ` John Harrison
@ 2021-12-29 11:29 ` Jani Nikula
2 siblings, 0 replies; 5+ messages in thread
From: Jani Nikula @ 2021-12-29 11:29 UTC (permalink / raw)
To: Matthew Brost, intel-gfx, dri-devel
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);
> if (!fence_array) {
> kfree(fences);
--
Jani Nikula, Intel Open Source Graphics Center
^ permalink raw reply [flat|nested] 5+ messages in thread