* [PATCH] dma-buf: add dma_fence_timestamp helper
@ 2023-09-29 10:47 Christian König
2023-10-02 15:49 ` Alex Deucher
0 siblings, 1 reply; 2+ messages in thread
From: Christian König @ 2023-09-29 10:47 UTC (permalink / raw)
To: dri-devel, Yunxiang.Li
When a fence signals there is a very small race window where the timestamp
isn't updated yet. sync_file solves this by busy waiting for the
timestamp to appear, but on other ocassions didn't handled this
correctly.
Provide a dma_fence_timestamp() helper function for this and use it in
all appropriate cases.
Another alternative would be to grab the spinlock when that happens.
v2 by teddy: add a wait parameter to wait for the timestamp to show up, in case
the accurate timestamp is needed and/or the timestamp is not based on
ktime (e.g. hw timestamp)
v3 chk: drop the parameter again for unified handling
Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
Signed-off-by: Christian König <christian.koenig@amd.com>
Fixes: 1774baa64f93 ("drm/scheduler: Change scheduled fence track v2")
---
drivers/dma-buf/dma-fence-unwrap.c | 13 ++++---------
drivers/dma-buf/sync_file.c | 9 +++------
drivers/gpu/drm/scheduler/sched_main.c | 2 +-
include/linux/dma-fence.h | 19 +++++++++++++++++++
4 files changed, 27 insertions(+), 16 deletions(-)
diff --git a/drivers/dma-buf/dma-fence-unwrap.c b/drivers/dma-buf/dma-fence-unwrap.c
index c625bb2b5d56..628af51c81af 100644
--- a/drivers/dma-buf/dma-fence-unwrap.c
+++ b/drivers/dma-buf/dma-fence-unwrap.c
@@ -76,16 +76,11 @@ struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences,
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();
+ ktime_t t = dma_fence_timestamp(tmp);
+
+ if (ktime_after(t, timestamp))
+ timestamp = t;
}
}
}
diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
index af57799c86ce..2e9a316c596a 100644
--- a/drivers/dma-buf/sync_file.c
+++ b/drivers/dma-buf/sync_file.c
@@ -268,13 +268,10 @@ static int sync_fill_fence_info(struct dma_fence *fence,
sizeof(info->driver_name));
info->status = dma_fence_get_status(fence);
- while (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) &&
- !test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags))
- cpu_relax();
info->timestamp_ns =
- test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags) ?
- ktime_to_ns(fence->timestamp) :
- ktime_set(0, 0);
+ dma_fence_is_signaled(fence) ?
+ ktime_to_ns(dma_fence_timestamp(fence)) :
+ ktime_set(0, 0);
return info->status;
}
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 506371c42745..5a3a622fc672 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -929,7 +929,7 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
if (next) {
next->s_fence->scheduled.timestamp =
- job->s_fence->finished.timestamp;
+ dma_fence_timestamp(&job->s_fence->finished);
/* start TO timer for next job */
drm_sched_start_timeout(sched);
}
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 0d678e9a7b24..ebe78bd3d121 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -568,6 +568,25 @@ static inline void dma_fence_set_error(struct dma_fence *fence,
fence->error = error;
}
+/**
+ * dma_fence_timestamp - helper to get the completion timestamp of a fence
+ * @fence: fence to get the timestamp from.
+ *
+ * After a fence is signaled the timestamp is updated with the signaling time,
+ * but setting the timestamp can race with tasks waiting for the signaling. This
+ * helper busy waits for the correct timestamp to appear.
+ */
+static inline ktime_t dma_fence_timestamp(struct dma_fence *fence)
+{
+ if (WARN_ON(!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)))
+ return ktime_get();
+
+ while (!test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags))
+ cpu_relax();
+
+ return fence->timestamp;
+}
+
signed long dma_fence_wait_timeout(struct dma_fence *,
bool intr, signed long timeout);
signed long dma_fence_wait_any_timeout(struct dma_fence **fences,
--
2.34.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] dma-buf: add dma_fence_timestamp helper
2023-09-29 10:47 [PATCH] dma-buf: add dma_fence_timestamp helper Christian König
@ 2023-10-02 15:49 ` Alex Deucher
0 siblings, 0 replies; 2+ messages in thread
From: Alex Deucher @ 2023-10-02 15:49 UTC (permalink / raw)
To: Christian König; +Cc: Yunxiang.Li, dri-devel
On Sat, Sep 30, 2023 at 7:06 PM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> When a fence signals there is a very small race window where the timestamp
> isn't updated yet. sync_file solves this by busy waiting for the
> timestamp to appear, but on other ocassions didn't handled this
> correctly.
>
> Provide a dma_fence_timestamp() helper function for this and use it in
> all appropriate cases.
>
> Another alternative would be to grab the spinlock when that happens.
>
> v2 by teddy: add a wait parameter to wait for the timestamp to show up, in case
> the accurate timestamp is needed and/or the timestamp is not based on
> ktime (e.g. hw timestamp)
> v3 chk: drop the parameter again for unified handling
>
> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Fixes: 1774baa64f93 ("drm/scheduler: Change scheduled fence track v2")
Looks correct to me.
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
> ---
> drivers/dma-buf/dma-fence-unwrap.c | 13 ++++---------
> drivers/dma-buf/sync_file.c | 9 +++------
> drivers/gpu/drm/scheduler/sched_main.c | 2 +-
> include/linux/dma-fence.h | 19 +++++++++++++++++++
> 4 files changed, 27 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence-unwrap.c b/drivers/dma-buf/dma-fence-unwrap.c
> index c625bb2b5d56..628af51c81af 100644
> --- a/drivers/dma-buf/dma-fence-unwrap.c
> +++ b/drivers/dma-buf/dma-fence-unwrap.c
> @@ -76,16 +76,11 @@ struct dma_fence *__dma_fence_unwrap_merge(unsigned int num_fences,
> 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();
> + ktime_t t = dma_fence_timestamp(tmp);
> +
> + if (ktime_after(t, timestamp))
> + timestamp = t;
> }
> }
> }
> diff --git a/drivers/dma-buf/sync_file.c b/drivers/dma-buf/sync_file.c
> index af57799c86ce..2e9a316c596a 100644
> --- a/drivers/dma-buf/sync_file.c
> +++ b/drivers/dma-buf/sync_file.c
> @@ -268,13 +268,10 @@ static int sync_fill_fence_info(struct dma_fence *fence,
> sizeof(info->driver_name));
>
> info->status = dma_fence_get_status(fence);
> - while (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) &&
> - !test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags))
> - cpu_relax();
> info->timestamp_ns =
> - test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags) ?
> - ktime_to_ns(fence->timestamp) :
> - ktime_set(0, 0);
> + dma_fence_is_signaled(fence) ?
> + ktime_to_ns(dma_fence_timestamp(fence)) :
> + ktime_set(0, 0);
>
> return info->status;
> }
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 506371c42745..5a3a622fc672 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -929,7 +929,7 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
>
> if (next) {
> next->s_fence->scheduled.timestamp =
> - job->s_fence->finished.timestamp;
> + dma_fence_timestamp(&job->s_fence->finished);
> /* start TO timer for next job */
> drm_sched_start_timeout(sched);
> }
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 0d678e9a7b24..ebe78bd3d121 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -568,6 +568,25 @@ static inline void dma_fence_set_error(struct dma_fence *fence,
> fence->error = error;
> }
>
> +/**
> + * dma_fence_timestamp - helper to get the completion timestamp of a fence
> + * @fence: fence to get the timestamp from.
> + *
> + * After a fence is signaled the timestamp is updated with the signaling time,
> + * but setting the timestamp can race with tasks waiting for the signaling. This
> + * helper busy waits for the correct timestamp to appear.
> + */
> +static inline ktime_t dma_fence_timestamp(struct dma_fence *fence)
> +{
> + if (WARN_ON(!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)))
> + return ktime_get();
> +
> + while (!test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags))
> + cpu_relax();
> +
> + return fence->timestamp;
> +}
> +
> signed long dma_fence_wait_timeout(struct dma_fence *,
> bool intr, signed long timeout);
> signed long dma_fence_wait_any_timeout(struct dma_fence **fences,
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-10-02 15:49 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-29 10:47 [PATCH] dma-buf: add dma_fence_timestamp helper Christian König
2023-10-02 15:49 ` Alex Deucher
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).