| Message ID | 20211214195913.35735-1-matthew.brost@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Increment composite fence seqno | expand |
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
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.
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); > if (!fence_array) { > kfree(fences);
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);
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(-)