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;
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); }
On Thu, Nov 12, 2020 at 10:27:23AM -0800, Veera Sundaram Sankaran wrote:
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
On 2020-11-13 12:45, Daniel Vetter wrote:
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/service... Will update the commit-text with the links as part of V2 patch.
Thanks, Veera
On Thu, Nov 19, 2020 at 2:26 AM veeras@codeaurora.org wrote:
Yeah sounds good. Might be good to mention that Android requires this in the code comment too, since it's potentially confusing. -Daniel
On Wed, Nov 18, 2020 at 9:28 PM veeras@codeaurora.org wrote:
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...
Thanks, Daniel
On Thu, Nov 12, 2020 at 7:27 PM Veera Sundaram Sankaran veeras@codeaurora.org wrote:
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
On 2020-11-19 03:58, Daniel Vetter wrote:
@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&archi... Thanks, Veera
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Veera Sundaram Sankaran -
veeras＠codeaurora.org