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

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

[Veera Sundaram Sankaran]

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.
Move the timestamp parameter out of union in dma_fence struct to allow
signaling drivers to set it. If the parameter is not set, ktime_get is
used to set the current time to fence timestamp during dma_fence_signal.

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

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 43624b4..7cef49a 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2012 Canonical Ltd
  * Copyright (C) 2012 Texas Instruments
+ * Copyright (c) 2020 The Linux Foundation. All rights reserved.
  *
  * Authors:
  * Rob Clark <robdclark@gmail.com>
@@ -329,7 +330,6 @@ void __dma_fence_might_wait(void)
 int dma_fence_signal_locked(struct dma_fence *fence)
 {
 	struct dma_fence_cb *cur, *tmp;
-	struct list_head cb_list;
 
 	lockdep_assert_held(fence->lock);
 
@@ -337,16 +337,18 @@ int dma_fence_signal_locked(struct dma_fence *fence)
 				      &fence->flags)))
 		return -EINVAL;
 
-	/* Stash the cb_list before replacing it with the timestamp */
-	list_replace(&fence->cb_list, &cb_list);
-
-	fence->timestamp = ktime_get();
+	/* set current time, if not set by signaling driver */
+	if (!fence->timestamp)
+		fence->timestamp = ktime_get();
 	set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
 	trace_dma_fence_signaled(fence);
 
-	list_for_each_entry_safe(cur, tmp, &cb_list, node) {
-		INIT_LIST_HEAD(&cur->node);
-		cur->func(fence, cur);
+	if (!list_empty(&fence->cb_list)) {
+		list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {
+			INIT_LIST_HEAD(&cur->node);
+			cur->func(fence, cur);
+		}
+		INIT_LIST_HEAD(&fence->cb_list);
 	}
 
 	return 0;
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 09e23ad..a9eebaf 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -4,6 +4,7 @@
  *
  * Copyright (C) 2012 Canonical Ltd
  * Copyright (C) 2012 Texas Instruments
+ * Copyright (c) 2020 The Linux Foundation. All rights reserved.
  *
  * Authors:
  * Rob Clark <robdclark@gmail.com>
@@ -70,26 +71,16 @@ struct dma_fence {
 	 * release the fence it is unused. No one should be adding to the
 	 * cb_list that they don't themselves hold a reference for.
 	 *
-	 * The lifetime of the timestamp is similarly tied to both the
-	 * rcu freelist and the cb_list. The timestamp is only set upon
-	 * signaling while simultaneously notifying the cb_list. Ergo, we
-	 * only use either the cb_list of timestamp. Upon destruction,
-	 * neither are accessible, and so we can use the rcu. This means
-	 * that the cb_list is *only* valid until the signal bit is set,
-	 * and to read either you *must* hold a reference to the fence,
-	 * and not just the rcu_read_lock.
-	 *
 	 * Listed in chronological order.
 	 */
 	union {
 		struct list_head cb_list;
-		/* @cb_list replaced by @timestamp on dma_fence_signal() */
-		ktime_t timestamp;
-		/* @timestamp replaced by @rcu on dma_fence_release() */
+		/* @cb_list replaced by @rcu on dma_fence_release() */
 		struct rcu_head rcu;
 	};
 	u64 context;
 	u64 seqno;
+	ktime_t timestamp;
 	unsigned long flags;
 	struct kref refcount;
 	int error;
-- 
2.7.4

[PATCH RESEND 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event

[Veera Sundaram Sankaran]

The explicit out-fences in crtc are signaled as part of vblank event,
indicating all framebuffers present on the Atomic Commit request are
scanned out on the screen. Though the fence signal and the vblank event
notification happens at the same time, triggered by the same hardware
vsync event, the timestamp set in both are different. With drivers
supporting precise vblank timestamp the difference between the two
timestamps would be even higher. This might have an impact on use-mode
frameworks using these fence timestamps for purposes other than simple
buffer usage. For instance, the Android framework uses the retire-fences
as an alternative to vblank when frame-updates are in progress Set the
fence timestamp during send vblank event to avoid discrepancies.

Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org>
---
 drivers/gpu/drm/drm_vblank.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index b18e1ef..b38e50c 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -24,6 +24,7 @@
  * OTHER DEALINGS IN THE SOFTWARE.
  */
 
+#include <linux/dma-fence.h>
 #include <linux/export.h>
 #include <linux/kthread.h>
 #include <linux/moduleparam.h>
@@ -999,6 +1000,14 @@ static void send_vblank_event(struct drm_device *dev,
 		e->event.seq.time_ns = ktime_to_ns(now);
 		break;
 	}
+
+	/*
+	 * update fence timestamp with the same vblank timestamp as both
+	 * are signaled by the same event
+	 */
+	if (e->base.fence)
+		e->base.fence->timestamp = now;
+
 	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
 	drm_send_event_locked(dev, &e->base);
 }
-- 
2.7.4

Re: [PATCH RESEND 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event

[Daniel Vetter]

On Thu, Nov 12, 2020 at 10:27:23AM -0800, Veera Sundaram Sankaran wrote:
> The explicit out-fences in crtc are signaled as part of vblank event,
> indicating all framebuffers present on the Atomic Commit request are
> scanned out on the screen. Though the fence signal and the vblank event
> notification happens at the same time, triggered by the same hardware
> vsync event, the timestamp set in both are different. With drivers
> supporting precise vblank timestamp the difference between the two
> timestamps would be even higher. This might have an impact on use-mode
> frameworks using these fence timestamps for purposes other than simple
> buffer usage. For instance, the Android framework uses the retire-fences
> as an alternative to vblank when frame-updates are in progress Set the
> fence timestamp during send vblank event to avoid discrepancies.

I think a reference to the exact source code in android that does this
would be really useful. Something in drm_hwcomposer or whatever is doing
this.

Aside from documenting why we want to do this I think this all looks
reasonable.
-Daniel

> 
> Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org>
> ---
>  drivers/gpu/drm/drm_vblank.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index b18e1ef..b38e50c 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -24,6 +24,7 @@
>   * OTHER DEALINGS IN THE SOFTWARE.
>   */
>  
> +#include <linux/dma-fence.h>
>  #include <linux/export.h>
>  #include <linux/kthread.h>
>  #include <linux/moduleparam.h>
> @@ -999,6 +1000,14 @@ static void send_vblank_event(struct drm_device *dev,
>  		e->event.seq.time_ns = ktime_to_ns(now);
>  		break;
>  	}
> +
> +	/*
> +	 * update fence timestamp with the same vblank timestamp as both
> +	 * are signaled by the same event
> +	 */
> +	if (e->base.fence)
> +		e->base.fence->timestamp = now;
> +
>  	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
>  	drm_send_event_locked(dev, &e->base);
>  }
> -- 
> 2.7.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[veeras]

On 2020-11-12 10:27, Veera Sundaram Sankaran 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.
> Move the timestamp parameter out of union in dma_fence struct to allow
> signaling drivers to set it. If the parameter is not set, ktime_get is
> used to set the current time to fence timestamp during 
> dma_fence_signal.
> 

@Sumit Semwal / @Gustavo Padovan,
Can you please help in reviewing this change as it falls in dma-fence 
files.
Thanks,
Veera

> Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org>
> ---
>  drivers/dma-buf/dma-fence.c | 18 ++++++++++--------
>  include/linux/dma-fence.h   | 15 +++------------
>  2 files changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 43624b4..7cef49a 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (C) 2012 Canonical Ltd
>   * Copyright (C) 2012 Texas Instruments
> + * Copyright (c) 2020 The Linux Foundation. All rights reserved.
>   *
>   * Authors:
>   * Rob Clark <robdclark@gmail.com>
> @@ -329,7 +330,6 @@ void __dma_fence_might_wait(void)
>  int dma_fence_signal_locked(struct dma_fence *fence)
>  {
>  	struct dma_fence_cb *cur, *tmp;
> -	struct list_head cb_list;
> 
>  	lockdep_assert_held(fence->lock);
> 
> @@ -337,16 +337,18 @@ int dma_fence_signal_locked(struct dma_fence 
> *fence)
>  				      &fence->flags)))
>  		return -EINVAL;
> 
> -	/* Stash the cb_list before replacing it with the timestamp */
> -	list_replace(&fence->cb_list, &cb_list);
> -
> -	fence->timestamp = ktime_get();
> +	/* set current time, if not set by signaling driver */
> +	if (!fence->timestamp)
> +		fence->timestamp = ktime_get();
>  	set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
>  	trace_dma_fence_signaled(fence);
> 
> -	list_for_each_entry_safe(cur, tmp, &cb_list, node) {
> -		INIT_LIST_HEAD(&cur->node);
> -		cur->func(fence, cur);
> +	if (!list_empty(&fence->cb_list)) {
> +		list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {
> +			INIT_LIST_HEAD(&cur->node);
> +			cur->func(fence, cur);
> +		}
> +		INIT_LIST_HEAD(&fence->cb_list);
>  	}
> 
>  	return 0;
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 09e23ad..a9eebaf 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (C) 2012 Canonical Ltd
>   * Copyright (C) 2012 Texas Instruments
> + * Copyright (c) 2020 The Linux Foundation. All rights reserved.
>   *
>   * Authors:
>   * Rob Clark <robdclark@gmail.com>
> @@ -70,26 +71,16 @@ struct dma_fence {
>  	 * release the fence it is unused. No one should be adding to the
>  	 * cb_list that they don't themselves hold a reference for.
>  	 *
> -	 * The lifetime of the timestamp is similarly tied to both the
> -	 * rcu freelist and the cb_list. The timestamp is only set upon
> -	 * signaling while simultaneously notifying the cb_list. Ergo, we
> -	 * only use either the cb_list of timestamp. Upon destruction,
> -	 * neither are accessible, and so we can use the rcu. This means
> -	 * that the cb_list is *only* valid until the signal bit is set,
> -	 * and to read either you *must* hold a reference to the fence,
> -	 * and not just the rcu_read_lock.
> -	 *
>  	 * Listed in chronological order.
>  	 */
>  	union {
>  		struct list_head cb_list;
> -		/* @cb_list replaced by @timestamp on dma_fence_signal() */
> -		ktime_t timestamp;
> -		/* @timestamp replaced by @rcu on dma_fence_release() */
> +		/* @cb_list replaced by @rcu on dma_fence_release() */
>  		struct rcu_head rcu;
>  	};
>  	u64 context;
>  	u64 seqno;
> +	ktime_t timestamp;
>  	unsigned long flags;
>  	struct kref refcount;
>  	int error;

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

[Daniel Vetter]

On Wed, Nov 18, 2020 at 9:28 PM <veeras@codeaurora.org> wrote:
>
> On 2020-11-12 10:27, Veera Sundaram Sankaran 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.
> > Move the timestamp parameter out of union in dma_fence struct to allow
> > signaling drivers to set it. If the parameter is not set, ktime_get is
> > used to set the current time to fence timestamp during
> > dma_fence_signal.
> >
>
> @Sumit Semwal / @Gustavo Padovan,
> Can you please help in reviewing this change as it falls in dma-fence
> files.

Please answer my question on the 2nd patch. This is new uabi
behaviour, we have a bunch of requirements for this kind of work:

https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements

Thanks, Daniel

> Thanks,
> Veera
>
> > Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org>
> > ---
> >  drivers/dma-buf/dma-fence.c | 18 ++++++++++--------
> >  include/linux/dma-fence.h   | 15 +++------------
> >  2 files changed, 13 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 43624b4..7cef49a 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -4,6 +4,7 @@
> >   *
> >   * Copyright (C) 2012 Canonical Ltd
> >   * Copyright (C) 2012 Texas Instruments
> > + * Copyright (c) 2020 The Linux Foundation. All rights reserved.
> >   *
> >   * Authors:
> >   * Rob Clark <robdclark@gmail.com>
> > @@ -329,7 +330,6 @@ void __dma_fence_might_wait(void)
> >  int dma_fence_signal_locked(struct dma_fence *fence)
> >  {
> >       struct dma_fence_cb *cur, *tmp;
> > -     struct list_head cb_list;
> >
> >       lockdep_assert_held(fence->lock);
> >
> > @@ -337,16 +337,18 @@ int dma_fence_signal_locked(struct dma_fence
> > *fence)
> >                                     &fence->flags)))
> >               return -EINVAL;
> >
> > -     /* Stash the cb_list before replacing it with the timestamp */
> > -     list_replace(&fence->cb_list, &cb_list);
> > -
> > -     fence->timestamp = ktime_get();
> > +     /* set current time, if not set by signaling driver */
> > +     if (!fence->timestamp)
> > +             fence->timestamp = ktime_get();
> >       set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
> >       trace_dma_fence_signaled(fence);
> >
> > -     list_for_each_entry_safe(cur, tmp, &cb_list, node) {
> > -             INIT_LIST_HEAD(&cur->node);
> > -             cur->func(fence, cur);
> > +     if (!list_empty(&fence->cb_list)) {
> > +             list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {
> > +                     INIT_LIST_HEAD(&cur->node);
> > +                     cur->func(fence, cur);
> > +             }
> > +             INIT_LIST_HEAD(&fence->cb_list);
> >       }
> >
> >       return 0;
> > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> > index 09e23ad..a9eebaf 100644
> > --- a/include/linux/dma-fence.h
> > +++ b/include/linux/dma-fence.h
> > @@ -4,6 +4,7 @@
> >   *
> >   * Copyright (C) 2012 Canonical Ltd
> >   * Copyright (C) 2012 Texas Instruments
> > + * Copyright (c) 2020 The Linux Foundation. All rights reserved.
> >   *
> >   * Authors:
> >   * Rob Clark <robdclark@gmail.com>
> > @@ -70,26 +71,16 @@ struct dma_fence {
> >        * release the fence it is unused. No one should be adding to the
> >        * cb_list that they don't themselves hold a reference for.
> >        *
> > -      * The lifetime of the timestamp is similarly tied to both the
> > -      * rcu freelist and the cb_list. The timestamp is only set upon
> > -      * signaling while simultaneously notifying the cb_list. Ergo, we
> > -      * only use either the cb_list of timestamp. Upon destruction,
> > -      * neither are accessible, and so we can use the rcu. This means
> > -      * that the cb_list is *only* valid until the signal bit is set,
> > -      * and to read either you *must* hold a reference to the fence,
> > -      * and not just the rcu_read_lock.
> > -      *
> >        * Listed in chronological order.
> >        */
> >       union {
> >               struct list_head cb_list;
> > -             /* @cb_list replaced by @timestamp on dma_fence_signal() */
> > -             ktime_t timestamp;
> > -             /* @timestamp replaced by @rcu on dma_fence_release() */
> > +             /* @cb_list replaced by @rcu on dma_fence_release() */
> >               struct rcu_head rcu;
> >       };
> >       u64 context;
> >       u64 seqno;
> > +     ktime_t timestamp;
> >       unsigned long flags;
> >       struct kref refcount;
> >       int error;



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH RESEND 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event

[veeras]

On 2020-11-13 12:45, Daniel Vetter wrote:
> On Thu, Nov 12, 2020 at 10:27:23AM -0800, Veera Sundaram Sankaran 
> wrote:
>> The explicit out-fences in crtc are signaled as part of vblank event,
>> indicating all framebuffers present on the Atomic Commit request are
>> scanned out on the screen. Though the fence signal and the vblank 
>> event
>> notification happens at the same time, triggered by the same hardware
>> vsync event, the timestamp set in both are different. With drivers
>> supporting precise vblank timestamp the difference between the two
>> timestamps would be even higher. This might have an impact on use-mode
>> frameworks using these fence timestamps for purposes other than simple
>> buffer usage. For instance, the Android framework uses the 
>> retire-fences
>> as an alternative to vblank when frame-updates are in progress Set the
>> fence timestamp during send vblank event to avoid discrepancies.
> 
> I think a reference to the exact source code in android that does this
> would be really useful. Something in drm_hwcomposer or whatever is 
> doing
> this.
> 

Thanks for the review. Sorry for not getting back earlier, was waiting
for the review on [patch 1/2], so that both comments can be addressed 
together.
Here is the reference for Android expecting retire-fence timestamp to
match exactly with hardware vsync as it is used for the dispsync model.

Usage: https://source.android.com/devices/graphics/implement-vsync
Code: 
https://android.googlesource.com/platform/frameworks/native/+/master/services/surfaceflinger/Scheduler/Scheduler.cpp#397
Will update the commit-text with the links as part of V2 patch.

Thanks,
Veera

> Aside from documenting why we want to do this I think this all looks
> reasonable.
> -Daniel
> 
>> 
>> Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org>
>> ---
>>  drivers/gpu/drm/drm_vblank.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/drm_vblank.c 
>> b/drivers/gpu/drm/drm_vblank.c
>> index b18e1ef..b38e50c 100644
>> --- a/drivers/gpu/drm/drm_vblank.c
>> +++ b/drivers/gpu/drm/drm_vblank.c
>> @@ -24,6 +24,7 @@
>>   * OTHER DEALINGS IN THE SOFTWARE.
>>   */
>> 
>> +#include <linux/dma-fence.h>
>>  #include <linux/export.h>
>>  #include <linux/kthread.h>
>>  #include <linux/moduleparam.h>
>> @@ -999,6 +1000,14 @@ static void send_vblank_event(struct drm_device 
>> *dev,
>>  		e->event.seq.time_ns = ktime_to_ns(now);
>>  		break;
>>  	}
>> +
>> +	/*
>> +	 * update fence timestamp with the same vblank timestamp as both
>> +	 * are signaled by the same event
>> +	 */
>> +	if (e->base.fence)
>> +		e->base.fence->timestamp = now;
>> +
>>  	trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
>>  	drm_send_event_locked(dev, &e->base);
>>  }
>> --
>> 2.7.4
>> 

Re: [PATCH RESEND 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event

[Daniel Vetter]

On Thu, Nov 19, 2020 at 2:26 AM <veeras@codeaurora.org> wrote:
>
> On 2020-11-13 12:45, Daniel Vetter wrote:
> > On Thu, Nov 12, 2020 at 10:27:23AM -0800, Veera Sundaram Sankaran
> > wrote:
> >> The explicit out-fences in crtc are signaled as part of vblank event,
> >> indicating all framebuffers present on the Atomic Commit request are
> >> scanned out on the screen. Though the fence signal and the vblank
> >> event
> >> notification happens at the same time, triggered by the same hardware
> >> vsync event, the timestamp set in both are different. With drivers
> >> supporting precise vblank timestamp the difference between the two
> >> timestamps would be even higher. This might have an impact on use-mode
> >> frameworks using these fence timestamps for purposes other than simple
> >> buffer usage. For instance, the Android framework uses the
> >> retire-fences
> >> as an alternative to vblank when frame-updates are in progress Set the
> >> fence timestamp during send vblank event to avoid discrepancies.
> >
> > I think a reference to the exact source code in android that does this
> > would be really useful. Something in drm_hwcomposer or whatever is
> > doing
> > this.
> >
>
> Thanks for the review. Sorry for not getting back earlier, was waiting
> for the review on [patch 1/2], so that both comments can be addressed
> together.
> Here is the reference for Android expecting retire-fence timestamp to
> match exactly with hardware vsync as it is used for the dispsync model.
>
> Usage: https://source.android.com/devices/graphics/implement-vsync
> Code:
> https://android.googlesource.com/platform/frameworks/native/+/master/services/surfaceflinger/Scheduler/Scheduler.cpp#397
> Will update the commit-text with the links as part of V2 patch.

Yeah sounds good. Might be good to mention that Android requires this
in the code comment too, since it's potentially confusing.
-Daniel

> Thanks,
> Veera
>
> > Aside from documenting why we want to do this I think this all looks
> > reasonable.
> > -Daniel
> >
> >>
> >> Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org>
> >> ---
> >>  drivers/gpu/drm/drm_vblank.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_vblank.c
> >> b/drivers/gpu/drm/drm_vblank.c
> >> index b18e1ef..b38e50c 100644
> >> --- a/drivers/gpu/drm/drm_vblank.c
> >> +++ b/drivers/gpu/drm/drm_vblank.c
> >> @@ -24,6 +24,7 @@
> >>   * OTHER DEALINGS IN THE SOFTWARE.
> >>   */
> >>
> >> +#include <linux/dma-fence.h>
> >>  #include <linux/export.h>
> >>  #include <linux/kthread.h>
> >>  #include <linux/moduleparam.h>
> >> @@ -999,6 +1000,14 @@ static void send_vblank_event(struct drm_device
> >> *dev,
> >>              e->event.seq.time_ns = ktime_to_ns(now);
> >>              break;
> >>      }
> >> +
> >> +    /*
> >> +     * update fence timestamp with the same vblank timestamp as both
> >> +     * are signaled by the same event
> >> +     */
> >> +    if (e->base.fence)
> >> +            e->base.fence->timestamp = now;
> >> +
> >>      trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq);
> >>      drm_send_event_locked(dev, &e->base);
> >>  }
> >> --
> >> 2.7.4
> >>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Daniel Vetter]

On Thu, Nov 12, 2020 at 7:27 PM 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.
> Move the timestamp parameter out of union in dma_fence struct to allow
> signaling drivers to set it. If the parameter is not set, ktime_get is
> used to set the current time to fence timestamp during dma_fence_signal.
>
> Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org>

So with they "why?" question fully resolved, I think this is a bit too
much a hack. I think much better if we pass the timestamp explicitly,
in a new dma_fence_signal_timestamp variant. That means a bit more
work, but I think it will handle this special case cleaner.

Also means we need to wire the timestamp through the entire call stack
on the drm side too. So we need a drm_send_event_locked_timestamp
variant too for send_vblank_event.
-Daniel

> ---
>  drivers/dma-buf/dma-fence.c | 18 ++++++++++--------
>  include/linux/dma-fence.h   | 15 +++------------
>  2 files changed, 13 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 43624b4..7cef49a 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (C) 2012 Canonical Ltd
>   * Copyright (C) 2012 Texas Instruments
> + * Copyright (c) 2020 The Linux Foundation. All rights reserved.
>   *
>   * Authors:
>   * Rob Clark <robdclark@gmail.com>
> @@ -329,7 +330,6 @@ void __dma_fence_might_wait(void)
>  int dma_fence_signal_locked(struct dma_fence *fence)
>  {
>         struct dma_fence_cb *cur, *tmp;
> -       struct list_head cb_list;
>
>         lockdep_assert_held(fence->lock);
>
> @@ -337,16 +337,18 @@ int dma_fence_signal_locked(struct dma_fence *fence)
>                                       &fence->flags)))
>                 return -EINVAL;
>
> -       /* Stash the cb_list before replacing it with the timestamp */
> -       list_replace(&fence->cb_list, &cb_list);
> -
> -       fence->timestamp = ktime_get();
> +       /* set current time, if not set by signaling driver */
> +       if (!fence->timestamp)
> +               fence->timestamp = ktime_get();
>         set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
>         trace_dma_fence_signaled(fence);
>
> -       list_for_each_entry_safe(cur, tmp, &cb_list, node) {
> -               INIT_LIST_HEAD(&cur->node);
> -               cur->func(fence, cur);
> +       if (!list_empty(&fence->cb_list)) {
> +               list_for_each_entry_safe(cur, tmp, &fence->cb_list, node) {
> +                       INIT_LIST_HEAD(&cur->node);
> +                       cur->func(fence, cur);
> +               }
> +               INIT_LIST_HEAD(&fence->cb_list);
>         }
>
>         return 0;
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 09e23ad..a9eebaf 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -4,6 +4,7 @@
>   *
>   * Copyright (C) 2012 Canonical Ltd
>   * Copyright (C) 2012 Texas Instruments
> + * Copyright (c) 2020 The Linux Foundation. All rights reserved.
>   *
>   * Authors:
>   * Rob Clark <robdclark@gmail.com>
> @@ -70,26 +71,16 @@ struct dma_fence {
>          * release the fence it is unused. No one should be adding to the
>          * cb_list that they don't themselves hold a reference for.
>          *
> -        * The lifetime of the timestamp is similarly tied to both the
> -        * rcu freelist and the cb_list. The timestamp is only set upon
> -        * signaling while simultaneously notifying the cb_list. Ergo, we
> -        * only use either the cb_list of timestamp. Upon destruction,
> -        * neither are accessible, and so we can use the rcu. This means
> -        * that the cb_list is *only* valid until the signal bit is set,
> -        * and to read either you *must* hold a reference to the fence,
> -        * and not just the rcu_read_lock.
> -        *
>          * Listed in chronological order.
>          */
>         union {
>                 struct list_head cb_list;
> -               /* @cb_list replaced by @timestamp on dma_fence_signal() */
> -               ktime_t timestamp;
> -               /* @timestamp replaced by @rcu on dma_fence_release() */
> +               /* @cb_list replaced by @rcu on dma_fence_release() */
>                 struct rcu_head rcu;
>         };
>         u64 context;
>         u64 seqno;
> +       ktime_t timestamp;
>         unsigned long flags;
>         struct kref refcount;
>         int error;
> --
> 2.7.4
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[veeras]

On 2020-11-19 03:58, Daniel Vetter wrote:
> On Thu, Nov 12, 2020 at 7:27 PM 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.
>> Move the timestamp parameter out of union in dma_fence struct to allow
>> signaling drivers to set it. If the parameter is not set, ktime_get is
>> used to set the current time to fence timestamp during 
>> dma_fence_signal.
>> 
>> Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org>
> 
> So with they "why?" question fully resolved, I think this is a bit too
> much a hack. I think much better if we pass the timestamp explicitly,
> in a new dma_fence_signal_timestamp variant. That means a bit more
> work, but I think it will handle this special case cleaner.
> 
> Also means we need to wire the timestamp through the entire call stack
> on the drm side too. So we need a drm_send_event_locked_timestamp
> variant too for send_vblank_event.
> -Daniel
> 

@Sumit Semwal, @Gustavo Padovan, @Steven Rostedt
Can you please help in getting review for the v2 patches.
I have addressed the earlier comments from Daniel Vetter.
https://patchwork.kernel.org/project/dri-devel/list/?series=388881&archive=both
Thanks,
Veera

>> ---
>>  drivers/dma-buf/dma-fence.c | 18 ++++++++++--------
>>  include/linux/dma-fence.h   | 15 +++------------
>>  2 files changed, 13 insertions(+), 20 deletions(-)
>> 
>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>> index 43624b4..7cef49a 100644
>> --- a/drivers/dma-buf/dma-fence.c
>> +++ b/drivers/dma-buf/dma-fence.c
>> @@ -4,6 +4,7 @@
>>   *
>>   * Copyright (C) 2012 Canonical Ltd
>>   * Copyright (C) 2012 Texas Instruments
>> + * Copyright (c) 2020 The Linux Foundation. All rights reserved.
>>   *
>>   * Authors:
>>   * Rob Clark <robdclark@gmail.com>
>> @@ -329,7 +330,6 @@ void __dma_fence_might_wait(void)
>>  int dma_fence_signal_locked(struct dma_fence *fence)
>>  {
>>         struct dma_fence_cb *cur, *tmp;
>> -       struct list_head cb_list;
>> 
>>         lockdep_assert_held(fence->lock);
>> 
>> @@ -337,16 +337,18 @@ int dma_fence_signal_locked(struct dma_fence 
>> *fence)
>>                                       &fence->flags)))
>>                 return -EINVAL;
>> 
>> -       /* Stash the cb_list before replacing it with the timestamp */
>> -       list_replace(&fence->cb_list, &cb_list);
>> -
>> -       fence->timestamp = ktime_get();
>> +       /* set current time, if not set by signaling driver */
>> +       if (!fence->timestamp)
>> +               fence->timestamp = ktime_get();
>>         set_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT, &fence->flags);
>>         trace_dma_fence_signaled(fence);
>> 
>> -       list_for_each_entry_safe(cur, tmp, &cb_list, node) {
>> -               INIT_LIST_HEAD(&cur->node);
>> -               cur->func(fence, cur);
>> +       if (!list_empty(&fence->cb_list)) {
>> +               list_for_each_entry_safe(cur, tmp, &fence->cb_list, 
>> node) {
>> +                       INIT_LIST_HEAD(&cur->node);
>> +                       cur->func(fence, cur);
>> +               }
>> +               INIT_LIST_HEAD(&fence->cb_list);
>>         }
>> 
>>         return 0;
>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>> index 09e23ad..a9eebaf 100644
>> --- a/include/linux/dma-fence.h
>> +++ b/include/linux/dma-fence.h
>> @@ -4,6 +4,7 @@
>>   *
>>   * Copyright (C) 2012 Canonical Ltd
>>   * Copyright (C) 2012 Texas Instruments
>> + * Copyright (c) 2020 The Linux Foundation. All rights reserved.
>>   *
>>   * Authors:
>>   * Rob Clark <robdclark@gmail.com>
>> @@ -70,26 +71,16 @@ struct dma_fence {
>>          * release the fence it is unused. No one should be adding to 
>> the
>>          * cb_list that they don't themselves hold a reference for.
>>          *
>> -        * The lifetime of the timestamp is similarly tied to both the
>> -        * rcu freelist and the cb_list. The timestamp is only set 
>> upon
>> -        * signaling while simultaneously notifying the cb_list. Ergo, 
>> we
>> -        * only use either the cb_list of timestamp. Upon destruction,
>> -        * neither are accessible, and so we can use the rcu. This 
>> means
>> -        * that the cb_list is *only* valid until the signal bit is 
>> set,
>> -        * and to read either you *must* hold a reference to the 
>> fence,
>> -        * and not just the rcu_read_lock.
>> -        *
>>          * Listed in chronological order.
>>          */
>>         union {
>>                 struct list_head cb_list;
>> -               /* @cb_list replaced by @timestamp on 
>> dma_fence_signal() */
>> -               ktime_t timestamp;
>> -               /* @timestamp replaced by @rcu on dma_fence_release() 
>> */
>> +               /* @cb_list replaced by @rcu on dma_fence_release() */
>>                 struct rcu_head rcu;
>>         };
>>         u64 context;
>>         u64 seqno;
>> +       ktime_t timestamp;
>>         unsigned long flags;
>>         struct kref refcount;
>>         int error;
>> --
>> 2.7.4
>>