On 2023-06-30 08:00, Christian König wrote:
> Some Android CTS is testing if the signaling time keeps consistent
> during merges.
>
> v2: use the current time if the fence is still in the signaling path and
> the timestamp not yet available.
> v3: improve comment, fix one more case to use the correct timestamp
>
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> ---
> drivers/dma-buf/dma-fence-unwrap.c | 26 ++++++++++++++++++++++----
> drivers/dma-buf/dma-fence.c | 5 +++--
> drivers/gpu/drm/drm_syncobj.c | 2 +-
> include/linux/dma-fence.h | 2 +-
> 4 files changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence-unwrap.c b/drivers/dma-buf/dma-fence-unwrap.c
> index 7002bca792ff..c625bb2b5d56 100644
> --- a/drivers/dma-buf/dma-fence-unwrap.c
> +++ b/drivers/dma-buf/dma-fence-unwrap.c
> @@ -66,18 +66,36 @@ 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 we couldn't find a pending fence just return a private signaled
> + * fence with the timestamp of the last signaled one.
> + */
> if (count == 0)
> - return dma_fence_get_stub();
> + return dma_fence_allocate_private_stub(timestamp);
>
Hi Christian,
Thank you for clarifying the justification of this patch in the patch description,
and adding the comment before "if (count == 0)"--it's clearer now.
Reviewed-by: Luben Tuikov <luben.tuikov@xxxxxxx>
Thanks again for sending a v3 of this patch--it does make it clearer now. Feel
free to push this patch in.
---
Silly question perhaps:
Could we not have returned an existing (signalled) fence with
the wanted timestamp (when count == 0), as opposed to allocating a stub? Maybe
allocation should be avoided?
--
Regards,
Luben
> array = kmalloc_array(count, sizeof(*array), GFP_KERNEL);
> if (!array)
> @@ -138,7 +156,7 @@ struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences,
> } while (tmp);
>
> if (count == 0) {
> - tmp = dma_fence_get_stub();
> + tmp = dma_fence_allocate_private_stub(ktime_get());
> goto return_tmp;
> }
>
> 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;
