| Message ID | 20230623090856.110456-1-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-buf: keep the signaling time of merged fences v2 | expand |
On Fri, Jun 23, 2023 at 5:09 AM Christian König <ckoenig.leichtzumerken@gmail.com> wrote: > > Some Android CTS is testing for that. > > v2: use the current time if the fence is still in the signaling path and > the timestamp not yet available. > > Signed-off-by: Christian König <christian.koenig@amd.com> Acked-by: Alex Deucher <alexander.deucher@amd.com> > --- > drivers/dma-buf/dma-fence-unwrap.c | 20 +++++++++++++++++--- > drivers/dma-buf/dma-fence.c | 5 +++-- > drivers/gpu/drm/drm_syncobj.c | 2 +- > include/linux/dma-fence.h | 2 +- > 4 files changed, 22 insertions(+), 7 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence-unwrap.c b/drivers/dma-buf/dma-fence-unwrap.c > index 7002bca792ff..46c2d3a474cd 100644 > --- a/drivers/dma-buf/dma-fence-unwrap.c > +++ b/drivers/dma-buf/dma-fence-unwrap.c > @@ -66,18 +66,32 @@ struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences, > { > struct dma_fence_array *result; > struct dma_fence *tmp, **array; > + ktime_t timestamp; > unsigned int i; > size_t count; > > count = 0; > + timestamp = ns_to_ktime(0); > for (i = 0; i < num_fences; ++i) { > - dma_fence_unwrap_for_each(tmp, &iter[i], fences[i]) > - if (!dma_fence_is_signaled(tmp)) > + dma_fence_unwrap_for_each(tmp, &iter[i], fences[i]) { > + if (!dma_fence_is_signaled(tmp)) { > ++count; > + } else if (test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, > + &tmp->flags)) { > + if (ktime_after(tmp->timestamp, timestamp)) > + timestamp = tmp->timestamp; > + } else { > + /* > + * Use the current time if the fence is > + * currently signaling. > + */ > + timestamp = ktime_get(); > + } > + } > } > > if (count == 0) > - return dma_fence_get_stub(); > + return dma_fence_allocate_private_stub(timestamp); > > array = kmalloc_array(count, sizeof(*array), GFP_KERNEL); > if (!array) > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index f177c56269bb..ad076f208760 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -150,10 +150,11 @@ EXPORT_SYMBOL(dma_fence_get_stub); > > /** > * dma_fence_allocate_private_stub - return a private, signaled fence > + * @timestamp: timestamp when the fence was signaled > * > * Return a newly allocated and signaled stub fence. > */ > -struct dma_fence *dma_fence_allocate_private_stub(void) > +struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp) > { > struct dma_fence *fence; > > @@ -169,7 +170,7 @@ struct dma_fence *dma_fence_allocate_private_stub(void) > set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, > &fence->flags); > > - dma_fence_signal(fence); > + dma_fence_signal_timestamp(fence, timestamp); > > return fence; > } > diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c > index 0c2be8360525..04589a35eb09 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -353,7 +353,7 @@ EXPORT_SYMBOL(drm_syncobj_replace_fence); > */ > static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) > { > - struct dma_fence *fence = dma_fence_allocate_private_stub(); > + struct dma_fence *fence = dma_fence_allocate_private_stub(ktime_get()); > > if (IS_ERR(fence)) > return PTR_ERR(fence); > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index d54b595a0fe0..0d678e9a7b24 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -606,7 +606,7 @@ static inline signed long dma_fence_wait(struct dma_fence *fence, bool intr) > void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline); > > struct dma_fence *dma_fence_get_stub(void); > -struct dma_fence *dma_fence_allocate_private_stub(void); > +struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp); > u64 dma_fence_context_alloc(unsigned num); > > extern const struct dma_fence_ops dma_fence_array_ops; > -- > 2.34.1 >
On 2023-06-23 05:08, Christian König wrote: > Some Android CTS is testing for that. > It's not entirely clear what "that" is, other than by the subject title of the patch. Something like "Record and return the signalling time of merged fences, as well as regular fences, since some Android CTS(?) is testing for this time." would be very helpful as a commit description. > v2: use the current time if the fence is still in the signaling path and > the timestamp not yet available. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/dma-fence-unwrap.c | 20 +++++++++++++++++--- > drivers/dma-buf/dma-fence.c | 5 +++-- > drivers/gpu/drm/drm_syncobj.c | 2 +- > include/linux/dma-fence.h | 2 +- > 4 files changed, 22 insertions(+), 7 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence-unwrap.c b/drivers/dma-buf/dma-fence-unwrap.c > index 7002bca792ff..46c2d3a474cd 100644 > --- a/drivers/dma-buf/dma-fence-unwrap.c > +++ b/drivers/dma-buf/dma-fence-unwrap.c > @@ -66,18 +66,32 @@ struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences, > { > struct dma_fence_array *result; > struct dma_fence *tmp, **array; > + ktime_t timestamp; > unsigned int i; > size_t count; > > count = 0; > + timestamp = ns_to_ktime(0); > for (i = 0; i < num_fences; ++i) { > - dma_fence_unwrap_for_each(tmp, &iter[i], fences[i]) > - if (!dma_fence_is_signaled(tmp)) > + dma_fence_unwrap_for_each(tmp, &iter[i], fences[i]) { > + if (!dma_fence_is_signaled(tmp)) { > ++count; > + } else if (test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, > + &tmp->flags)) { > + if (ktime_after(tmp->timestamp, timestamp)) > + timestamp = tmp->timestamp; > + } else { > + /* > + * Use the current time if the fence is > + * currently signaling. > + */ > + timestamp = ktime_get(); > + } > + } > } > > if (count == 0) Just before this "if" I would've added a comment to describe what exactly is being checked and happening, since working out the negated if (!dma_fence_is_signaled(tmp)) ++count; is a bit cryptic. Perhaps something like, /* If all fences have signalled, then return a private * signalled fence. */ if (count == 0) And I can see that "count" is being reused down below in the logic of this function, but perhaps using another variable named "signalled", and then counting positive conditional, and then comparing, /* If all fences have signalled, then return a private * signalled fence. */ if (signalled == num_fences) ... Would've made the code clearer. The compiler would optimize the use of a second variable to the same register anyway. A moot point now perhaps, but we should keep note for future submissions.
diff --git a/drivers/dma-buf/dma-fence-unwrap.c b/drivers/dma-buf/dma-fence-unwrap.c index 7002bca792ff..46c2d3a474cd 100644 --- a/drivers/dma-buf/dma-fence-unwrap.c +++ b/drivers/dma-buf/dma-fence-unwrap.c @@ -66,18 +66,32 @@ struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences, { struct dma_fence_array *result; struct dma_fence *tmp, **array; + ktime_t timestamp; unsigned int i; size_t count; count = 0; + timestamp = ns_to_ktime(0); for (i = 0; i < num_fences; ++i) { - dma_fence_unwrap_for_each(tmp, &iter[i], fences[i]) - if (!dma_fence_is_signaled(tmp)) + dma_fence_unwrap_for_each(tmp, &iter[i], fences[i]) { + if (!dma_fence_is_signaled(tmp)) { ++count; + } else if (test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, + &tmp->flags)) { + if (ktime_after(tmp->timestamp, timestamp)) + timestamp = tmp->timestamp; + } else { + /* + * Use the current time if the fence is + * currently signaling. + */ + timestamp = ktime_get(); + } + } } if (count == 0) - return dma_fence_get_stub(); + return dma_fence_allocate_private_stub(timestamp); array = kmalloc_array(count, sizeof(*array), GFP_KERNEL); if (!array) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index f177c56269bb..ad076f208760 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -150,10 +150,11 @@ EXPORT_SYMBOL(dma_fence_get_stub); /** * dma_fence_allocate_private_stub - return a private, signaled fence + * @timestamp: timestamp when the fence was signaled * * Return a newly allocated and signaled stub fence. */ -struct dma_fence *dma_fence_allocate_private_stub(void) +struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp) { struct dma_fence *fence; @@ -169,7 +170,7 @@ struct dma_fence *dma_fence_allocate_private_stub(void) set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags); - dma_fence_signal(fence); + dma_fence_signal_timestamp(fence, timestamp); return fence; } diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 0c2be8360525..04589a35eb09 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -353,7 +353,7 @@ EXPORT_SYMBOL(drm_syncobj_replace_fence); */ static int drm_syncobj_assign_null_handle(struct drm_syncobj *syncobj) { - struct dma_fence *fence = dma_fence_allocate_private_stub(); + struct dma_fence *fence = dma_fence_allocate_private_stub(ktime_get()); if (IS_ERR(fence)) return PTR_ERR(fence); diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index d54b595a0fe0..0d678e9a7b24 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -606,7 +606,7 @@ static inline signed long dma_fence_wait(struct dma_fence *fence, bool intr) void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline); struct dma_fence *dma_fence_get_stub(void); -struct dma_fence *dma_fence_allocate_private_stub(void); +struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp); u64 dma_fence_context_alloc(unsigned num); extern const struct dma_fence_ops dma_fence_array_ops;
Some Android CTS is testing for that. v2: use the current time if the fence is still in the signaling path and the timestamp not yet available. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-fence-unwrap.c | 20 +++++++++++++++++--- drivers/dma-buf/dma-fence.c | 5 +++-- drivers/gpu/drm/drm_syncobj.c | 2 +- include/linux/dma-fence.h | 2 +- 4 files changed, 22 insertions(+), 7 deletions(-)