| Message ID | 20230929104725.2358-1-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-buf: add dma_fence_timestamp helper | expand |
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 >
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,