diff mbox series

[RESEND,v2,1/2] dma-fence: allow signaling drivers to set fence timestamp

Message ID 1609962554-13872-1-git-send-email-veeras@codeaurora.org
State Superseded
Headers show
Series [RESEND,v2,1/2] dma-fence: allow signaling drivers to set fence timestamp | expand

Commit Message

Veera Sundaram Sankaran Jan. 6, 2021, 7:49 p.m. UTC 
Some drivers have hardware capability to get the precise timestamp of
certain events based on which the fences are triggered. This allows it to
set accurate timestamp factoring out any software and IRQ latencies. Add
a timestamp variant of fence signal function, dma_fence_signal_timestamp
to allow drivers to update the precise timestamp for fences.

Changes in v2:
- Add a new fence signal variant instead of modifying fence struct

Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org>
---
 drivers/dma-buf/dma-fence.c | 70 ++++++++++++++++++++++++++++++++++++++++-----
 include/linux/dma-fence.h   |  3 ++
 2 files changed, 66 insertions(+), 7 deletions(-)

Comments

John Stultz Jan. 8, 2021, 7:55 p.m. UTC | #1 
On Thu, Jan 7, 2021 at 12:53 AM Veera Sundaram Sankaran
<veeras@codeaurora.org> wrote:
>
> Some drivers have hardware capability to get the precise timestamp of
> certain events based on which the fences are triggered. This allows it to
> set accurate timestamp factoring out any software and IRQ latencies. Add
> a timestamp variant of fence signal function, dma_fence_signal_timestamp
> to allow drivers to update the precise timestamp for fences.
>

So, on quick review, this seems mostly sane. Though, it might be good
to add some more detail about how the hardware timestamping fits into
the kernel's CLOCK_MONOTONIC time domain.

I just want to make sure this interface isn't abused to jam raw
hardware-domain timestamps into the fence->timestamp, causing the
meaning or time-domain of the fence->timestamp to be unclear or
inconsistent.

It may be useful to add an additional patch to the documentation
around the dma_fence structure to make the timestamp field semantics
more explicit and avoid confusion?

thanks
-john
Veera Sundaram Sankaran Jan. 9, 2021, midnight UTC | #2 
On 2021-01-08 11:55, John Stultz wrote:
> On Thu, Jan 7, 2021 at 12:53 AM Veera Sundaram Sankaran

> <veeras@codeaurora.org> wrote:

>> 

>> Some drivers have hardware capability to get the precise timestamp of

>> certain events based on which the fences are triggered. This allows it 

>> to

>> set accurate timestamp factoring out any software and IRQ latencies. 

>> Add

>> a timestamp variant of fence signal function, 

>> dma_fence_signal_timestamp

>> to allow drivers to update the precise timestamp for fences.

>> 

> 

> So, on quick review, this seems mostly sane. Though, it might be good

> to add some more detail about how the hardware timestamping fits into

> the kernel's CLOCK_MONOTONIC time domain.

> 

> I just want to make sure this interface isn't abused to jam raw

> hardware-domain timestamps into the fence->timestamp, causing the

> meaning or time-domain of the fence->timestamp to be unclear or

> inconsistent.

> 

> It may be useful to add an additional patch to the documentation

> around the dma_fence structure to make the timestamp field semantics

> more explicit and avoid confusion?


Thanks for the comments. Sure, let me add more information in the
commit-text about the HW timestamp conversion to kernel time-domain.
Will explicitly mention the timestamp domain expected as part of the
new dma_fence_signal_timestamp api documentation, since that would
be the only place the timestamp would be set externally from drivers.
On top of it, do suggest if still documentation on dma_fence struct
would be required.

Thanks,
Veera

> 

> thanks

> -john
diff mbox series

Patch

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 7475e09..8887c5a 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -312,22 +312,25 @@  void __dma_fence_might_wait(void)
 
 
 /**
- * dma_fence_signal_locked - signal completion of a fence
+ * dma_fence_signal_timestamp_locked - signal completion of a fence
  * @fence: the fence to signal
+ * @timestamp: fence signal timestamp
  *
  * Signal completion for software callbacks on a fence, this will unblock
  * dma_fence_wait() calls and run all the callbacks added with
  * dma_fence_add_callback(). Can be called multiple times, but since a fence
  * can only go from the unsignaled to the signaled state and not back, it will
- * only be effective the first time.
+ * only be effective the first time. Set the timestamp provided as the fence
+ * signal timestamp.
  *
- * Unlike dma_fence_signal(), this function must be called with &dma_fence.lock
- * held.
+ * Unlike dma_fence_signal_timestamp(), this function must be called with
+ * &dma_fence.lock held.
  *
  * Returns 0 on success and a negative error value when @fence has been
  * signalled already.
  */
-int dma_fence_signal_locked(struct dma_fence *fence)
+int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
+			ktime_t timestamp)
 {
 	struct dma_fence_cb *cur, *tmp;
 	struct list_head cb_list;
@@ -341,7 +344,7 @@  int dma_fence_signal_locked(struct dma_fence *fence)
 	/* Stash the cb_list before replacing it with the timestamp */
 	list_replace(&fence->cb_list, &cb_list);
 
-	fence->timestamp = ktime_get();
+	fence->timestamp = timestamp;
 	set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
 	trace_dma_fence_signaled(fence);
 
@@ -352,6 +355,59 @@  int dma_fence_signal_locked(struct dma_fence *fence)
 
 	return 0;
 }
+EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
+
+/**
+ * dma_fence_signal_timestamp - signal completion of a fence
+ * @fence: the fence to signal
+ * @timestamp: fence signal timestamp
+ *
+ * Signal completion for software callbacks on a fence, this will unblock
+ * dma_fence_wait() calls and run all the callbacks added with
+ * dma_fence_add_callback(). Can be called multiple times, but since a fence
+ * can only go from the unsignaled to the signaled state and not back, it will
+ * only be effective the first time. Set the timestamp provided as the fence
+ * signal timestamp.
+ *
+ * Returns 0 on success and a negative error value when @fence has been
+ * signalled already.
+ */
+int dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp)
+{
+	unsigned long flags;
+	int ret;
+
+	if (!fence)
+		return -EINVAL;
+
+	spin_lock_irqsave(fence->lock, flags);
+	ret = dma_fence_signal_timestamp_locked(fence, timestamp);
+	spin_unlock_irqrestore(fence->lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL(dma_fence_signal_timestamp);
+
+/**
+ * dma_fence_signal_locked - signal completion of a fence
+ * @fence: the fence to signal
+ *
+ * Signal completion for software callbacks on a fence, this will unblock
+ * dma_fence_wait() calls and run all the callbacks added with
+ * dma_fence_add_callback(). Can be called multiple times, but since a fence
+ * can only go from the unsignaled to the signaled state and not back, it will
+ * only be effective the first time.
+ *
+ * Unlike dma_fence_signal(), this function must be called with &dma_fence.lock
+ * held.
+ *
+ * Returns 0 on success and a negative error value when @fence has been
+ * signalled already.
+ */
+int dma_fence_signal_locked(struct dma_fence *fence)
+{
+	return dma_fence_signal_timestamp_locked(fence, ktime_get());
+}
 EXPORT_SYMBOL(dma_fence_signal_locked);
 
 /**
@@ -379,7 +435,7 @@  int dma_fence_signal(struct dma_fence *fence)
 	tmp = dma_fence_begin_signalling();
 
 	spin_lock_irqsave(fence->lock, flags);
-	ret = dma_fence_signal_locked(fence);
+	ret = dma_fence_signal_timestamp_locked(fence, ktime_get());
 	spin_unlock_irqrestore(fence->lock, flags);
 
 	dma_fence_end_signalling(tmp);
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 09e23ad..9f12efa 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -372,6 +372,9 @@  static inline void __dma_fence_might_wait(void) {}
 
 int dma_fence_signal(struct dma_fence *fence);
 int dma_fence_signal_locked(struct dma_fence *fence);
+int dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp);
+int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
+				      ktime_t timestamp);
 signed long dma_fence_default_wait(struct dma_fence *fence,
 				   bool intr, signed long timeout);
 int dma_fence_add_callback(struct dma_fence *fence,