* [PATCH v3 1/2] dma-fence: allow signaling drivers to set fence timestamp @ 2021-01-13 19:52 Veera Sundaram Sankaran 2021-01-13 19:52 ` [PATCH v3 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event Veera Sundaram Sankaran 2021-01-14 1:16 ` [PATCH v3 1/2] dma-fence: allow signaling drivers to set fence timestamp John Stultz 0 siblings, 2 replies; 8+ messages in thread From: Veera Sundaram Sankaran @ 2021-01-13 19:52 UTC (permalink / raw) To: dri-devel, linux-media, sumit.semwal, gustavo, airlied, daniel, john.stultz Cc: Veera Sundaram Sankaran, abhinavk, pdhaval, sean Some drivers have hardware capability to get the precise HW timestamp of certain events based on which the fences are triggered. The delta between the event HW timestamp & current HW reference timestamp can be used to calculate the timestamp in kernel's CLOCK_MONOTONIC time domain. 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 Changes in v3: - Add timestamp domain information to commit-text and dma_fence_signal_timestamp documentation 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(-) diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 7475e09..b83e9fa 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 in kernel's CLOCK_MONOTONIC time domain * * 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 in kernel's CLOCK_MONOTONIC time domain + * + * 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, -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v3 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event 2021-01-13 19:52 [PATCH v3 1/2] dma-fence: allow signaling drivers to set fence timestamp Veera Sundaram Sankaran @ 2021-01-13 19:52 ` Veera Sundaram Sankaran 2021-01-14 1:29 ` John Stultz 2021-01-14 2:28 ` John Stultz 2021-01-14 1:16 ` [PATCH v3 1/2] dma-fence: allow signaling drivers to set fence timestamp John Stultz 1 sibling, 2 replies; 8+ messages in thread From: Veera Sundaram Sankaran @ 2021-01-13 19:52 UTC (permalink / raw) To: dri-devel, linux-media, sumit.semwal, gustavo, airlied, daniel, john.stultz Cc: Veera Sundaram Sankaran, abhinavk, pdhaval, sean 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 [1] uses the retire-fences as an alternative to vblank when frame-updates are in progress. Set the fence timestamp during send vblank event using a new drm_send_event_timestamp_locked variant to avoid discrepancies. [1] https://android.googlesource.com/platform/frameworks/native/+/master/ services/surfaceflinger/Scheduler/Scheduler.cpp#397 Changes in v2: - Use drm_send_event_timestamp_locked to update fence timestamp - add more information to commit text Changes in v3: - use same backend helper function for variants of drm_send_event to avoid code duplications Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org> --- drivers/gpu/drm/drm_file.c | 71 ++++++++++++++++++++++++++++++++++++-------- drivers/gpu/drm/drm_vblank.c | 9 +++++- include/drm/drm_file.h | 3 ++ 3 files changed, 70 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index 0ac4566..b8348ca 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -775,20 +775,19 @@ void drm_event_cancel_free(struct drm_device *dev, EXPORT_SYMBOL(drm_event_cancel_free); /** - * drm_send_event_locked - send DRM event to file descriptor + * drm_send_event_helper - send DRM event to file descriptor * @dev: DRM device * @e: DRM event to deliver + * @timestamp: timestamp to set for the fence event in kernel's CLOCK_MONOTONIC + * time domain * - * This function sends the event @e, initialized with drm_event_reserve_init(), - * to its associated userspace DRM file. Callers must already hold - * &drm_device.event_lock, see drm_send_event() for the unlocked version. - * - * Note that the core will take care of unlinking and disarming events when the - * corresponding DRM file is closed. Drivers need not worry about whether the - * DRM file for this event still exists and can call this function upon - * completion of the asynchronous work unconditionally. + * This helper function sends the event @e, initialized with + * drm_event_reserve_init(), to its associated userspace DRM file. + * The timestamp variant of dma_fence_signal is used when the caller + * sends a valid timestamp. */ -void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e) +void drm_send_event_helper(struct drm_device *dev, + struct drm_pending_event *e, ktime_t timestamp) { assert_spin_locked(&dev->event_lock); @@ -799,7 +798,10 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e) } if (e->fence) { - dma_fence_signal(e->fence); + if (timestamp) + dma_fence_signal_timestamp(e->fence, timestamp); + else + dma_fence_signal(e->fence); dma_fence_put(e->fence); } @@ -814,6 +816,51 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e) wake_up_interruptible_poll(&e->file_priv->event_wait, EPOLLIN | EPOLLRDNORM); } + +/** + * drm_send_event_timestamp_locked - send DRM event to file descriptor + * @dev: DRM device + * @e: DRM event to deliver + * @timestamp: timestamp to set for the fence event in kernel's CLOCK_MONOTONIC + * time domain + * + * This function sends the event @e, initialized with drm_event_reserve_init(), + * to its associated userspace DRM file. Callers must already hold + * &drm_device.event_lock. + * + * Note that the core will take care of unlinking and disarming events when the + * corresponding DRM file is closed. Drivers need not worry about whether the + * DRM file for this event still exists and can call this function upon + * completion of the asynchronous work unconditionally. + */ +void drm_send_event_timestamp_locked(struct drm_device *dev, + struct drm_pending_event *e, ktime_t timestamp) +{ + WARN_ON(!timestamp); + + drm_send_event_helper(dev, e, timestamp); + +} +EXPORT_SYMBOL(drm_send_event_timestamp_locked); + +/** + * drm_send_event_locked - send DRM event to file descriptor + * @dev: DRM device + * @e: DRM event to deliver + * + * This function sends the event @e, initialized with drm_event_reserve_init(), + * to its associated userspace DRM file. Callers must already hold + * &drm_device.event_lock, see drm_send_event() for the unlocked version. + * + * Note that the core will take care of unlinking and disarming events when the + * corresponding DRM file is closed. Drivers need not worry about whether the + * DRM file for this event still exists and can call this function upon + * completion of the asynchronous work unconditionally. + */ +void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e) +{ + drm_send_event_helper(dev, e, 0); +} EXPORT_SYMBOL(drm_send_event_locked); /** @@ -836,7 +883,7 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e) unsigned long irqflags; spin_lock_irqsave(&dev->event_lock, irqflags); - drm_send_event_locked(dev, e); + drm_send_event_helper(dev, e, 0); spin_unlock_irqrestore(&dev->event_lock, irqflags); } EXPORT_SYMBOL(drm_send_event); diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c index f135b79..286edbe 100644 --- a/drivers/gpu/drm/drm_vblank.c +++ b/drivers/gpu/drm/drm_vblank.c @@ -1000,7 +1000,14 @@ static void send_vblank_event(struct drm_device *dev, break; } trace_drm_vblank_event_delivered(e->base.file_priv, e->pipe, seq); - drm_send_event_locked(dev, &e->base); + /* + * Use the same timestamp for any associated fence signal to avoid + * mismatch in timestamps for vsync & fence events triggered by the + * same HW event. Frameworks like SurfaceFlinger in Android expects the + * retire-fence timestamp to match exactly with HW vsync as it uses it + * for its software vsync modeling. + */ + drm_send_event_timestamp_locked(dev, &e->base, now); } /** diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h index 716990b..b81b3bf 100644 --- a/include/drm/drm_file.h +++ b/include/drm/drm_file.h @@ -399,6 +399,9 @@ void drm_event_cancel_free(struct drm_device *dev, struct drm_pending_event *p); void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e); void drm_send_event(struct drm_device *dev, struct drm_pending_event *e); +void drm_send_event_timestamp_locked(struct drm_device *dev, + struct drm_pending_event *e, + ktime_t timestamp); struct file *mock_drm_getfile(struct drm_minor *minor, unsigned int flags); -- 2.7.4 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event 2021-01-13 19:52 ` [PATCH v3 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event Veera Sundaram Sankaran @ 2021-01-14 1:29 ` John Stultz 2021-01-14 2:28 ` John Stultz 1 sibling, 0 replies; 8+ messages in thread From: John Stultz @ 2021-01-14 1:29 UTC (permalink / raw) To: Veera Sundaram Sankaran Cc: David Airlie, Gustavo Padovan, dri-devel, pdhaval, abhinavk, Sean Paul, linux-media On Wed, Jan 13, 2021 at 11:52 AM Veera Sundaram Sankaran <veeras@codeaurora.org> 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 [1] uses the > retire-fences as an alternative to vblank when frame-updates are in > progress. Set the fence timestamp during send vblank event using a new > drm_send_event_timestamp_locked variant to avoid discrepancies. > > [1] https://android.googlesource.com/platform/frameworks/native/+/master/ > services/surfaceflinger/Scheduler/Scheduler.cpp#397 > > Changes in v2: > - Use drm_send_event_timestamp_locked to update fence timestamp > - add more information to commit text > > Changes in v3: > - use same backend helper function for variants of drm_send_event to > avoid code duplications > > Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org> Looks good. I appreciate you addressing my earlier comments and sending this out again! Reviewed-by: John Stultz <john.stultz@linaro.org> thanks -john _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event 2021-01-13 19:52 ` [PATCH v3 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event Veera Sundaram Sankaran 2021-01-14 1:29 ` John Stultz @ 2021-01-14 2:28 ` John Stultz 2021-01-14 20:54 ` veeras 1 sibling, 1 reply; 8+ messages in thread From: John Stultz @ 2021-01-14 2:28 UTC (permalink / raw) To: Veera Sundaram Sankaran Cc: David Airlie, Gustavo Padovan, dri-devel, pdhaval, abhinavk, Sean Paul, linux-media On Wed, Jan 13, 2021 at 11:52 AM Veera Sundaram Sankaran <veeras@codeaurora.org> 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 [1] uses the > retire-fences as an alternative to vblank when frame-updates are in > progress. Set the fence timestamp during send vblank event using a new > drm_send_event_timestamp_locked variant to avoid discrepancies. > > [1] https://android.googlesource.com/platform/frameworks/native/+/master/ > services/surfaceflinger/Scheduler/Scheduler.cpp#397 > > Changes in v2: > - Use drm_send_event_timestamp_locked to update fence timestamp > - add more information to commit text > > Changes in v3: > - use same backend helper function for variants of drm_send_event to > avoid code duplications > > Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org> > --- > drivers/gpu/drm/drm_file.c | 71 ++++++++++++++++++++++++++++++++++++-------- > drivers/gpu/drm/drm_vblank.c | 9 +++++- > include/drm/drm_file.h | 3 ++ > 3 files changed, 70 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c > index 0ac4566..b8348ca 100644 > --- a/drivers/gpu/drm/drm_file.c > +++ b/drivers/gpu/drm/drm_file.c > @@ -775,20 +775,19 @@ void drm_event_cancel_free(struct drm_device *dev, > EXPORT_SYMBOL(drm_event_cancel_free); > > /** > - * drm_send_event_locked - send DRM event to file descriptor > + * drm_send_event_helper - send DRM event to file descriptor > * @dev: DRM device > * @e: DRM event to deliver > + * @timestamp: timestamp to set for the fence event in kernel's CLOCK_MONOTONIC > + * time domain > * > - * This function sends the event @e, initialized with drm_event_reserve_init(), > - * to its associated userspace DRM file. Callers must already hold > - * &drm_device.event_lock, see drm_send_event() for the unlocked version. > - * > - * Note that the core will take care of unlinking and disarming events when the > - * corresponding DRM file is closed. Drivers need not worry about whether the > - * DRM file for this event still exists and can call this function upon > - * completion of the asynchronous work unconditionally. > + * This helper function sends the event @e, initialized with > + * drm_event_reserve_init(), to its associated userspace DRM file. > + * The timestamp variant of dma_fence_signal is used when the caller > + * sends a valid timestamp. > */ > -void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e) > +void drm_send_event_helper(struct drm_device *dev, > + struct drm_pending_event *e, ktime_t timestamp) > { > assert_spin_locked(&dev->event_lock); > > @@ -799,7 +798,10 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e) > } > > if (e->fence) { > - dma_fence_signal(e->fence); > + if (timestamp) > + dma_fence_signal_timestamp(e->fence, timestamp); > + else > + dma_fence_signal(e->fence); > dma_fence_put(e->fence); > } > > @@ -814,6 +816,51 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e) > wake_up_interruptible_poll(&e->file_priv->event_wait, > EPOLLIN | EPOLLRDNORM); > } > + > +/** > + * drm_send_event_timestamp_locked - send DRM event to file descriptor > + * @dev: DRM device > + * @e: DRM event to deliver > + * @timestamp: timestamp to set for the fence event in kernel's CLOCK_MONOTONIC > + * time domain > + * > + * This function sends the event @e, initialized with drm_event_reserve_init(), > + * to its associated userspace DRM file. Callers must already hold > + * &drm_device.event_lock. > + * > + * Note that the core will take care of unlinking and disarming events when the > + * corresponding DRM file is closed. Drivers need not worry about whether the > + * DRM file for this event still exists and can call this function upon > + * completion of the asynchronous work unconditionally. > + */ > +void drm_send_event_timestamp_locked(struct drm_device *dev, > + struct drm_pending_event *e, ktime_t timestamp) > +{ > + WARN_ON(!timestamp); > + > + drm_send_event_helper(dev, e, timestamp); > + > +} > +EXPORT_SYMBOL(drm_send_event_timestamp_locked); Hey Veera, So actually, after closer look at the testing I was doing, we seem to be hitting that WARN_ON right as the display first comes up (I see this on both db845c and HiKey960). It seems in drm_crtc_send_vblank_event(), if "now" is set by drm_vblank_count_and_time(), the first timestamp value we get from it seems to be 0. Let me know if you need any help reproducing and sorting this issue out. thanks -john _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event 2021-01-14 2:28 ` John Stultz @ 2021-01-14 20:54 ` veeras 2021-01-15 22:29 ` John Stultz 0 siblings, 1 reply; 8+ messages in thread From: veeras @ 2021-01-14 20:54 UTC (permalink / raw) To: John Stultz Cc: David Airlie, Gustavo Padovan, dri-devel, pdhaval, abhinavk, Sean Paul, linux-media On 2021-01-13 18:28, John Stultz wrote: > On Wed, Jan 13, 2021 at 11:52 AM Veera Sundaram Sankaran > <veeras@codeaurora.org> 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 [1] uses the >> retire-fences as an alternative to vblank when frame-updates are in >> progress. Set the fence timestamp during send vblank event using a new >> drm_send_event_timestamp_locked variant to avoid discrepancies. >> >> [1] >> https://android.googlesource.com/platform/frameworks/native/+/master/ >> services/surfaceflinger/Scheduler/Scheduler.cpp#397 >> >> Changes in v2: >> - Use drm_send_event_timestamp_locked to update fence timestamp >> - add more information to commit text >> >> Changes in v3: >> - use same backend helper function for variants of drm_send_event to >> avoid code duplications >> >> Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org> >> --- >> drivers/gpu/drm/drm_file.c | 71 >> ++++++++++++++++++++++++++++++++++++-------- >> drivers/gpu/drm/drm_vblank.c | 9 +++++- >> include/drm/drm_file.h | 3 ++ >> 3 files changed, 70 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c >> index 0ac4566..b8348ca 100644 >> --- a/drivers/gpu/drm/drm_file.c >> +++ b/drivers/gpu/drm/drm_file.c >> @@ -775,20 +775,19 @@ void drm_event_cancel_free(struct drm_device >> *dev, >> EXPORT_SYMBOL(drm_event_cancel_free); >> >> /** >> - * drm_send_event_locked - send DRM event to file descriptor >> + * drm_send_event_helper - send DRM event to file descriptor >> * @dev: DRM device >> * @e: DRM event to deliver >> + * @timestamp: timestamp to set for the fence event in kernel's >> CLOCK_MONOTONIC >> + * time domain >> * >> - * This function sends the event @e, initialized with >> drm_event_reserve_init(), >> - * to its associated userspace DRM file. Callers must already hold >> - * &drm_device.event_lock, see drm_send_event() for the unlocked >> version. >> - * >> - * Note that the core will take care of unlinking and disarming >> events when the >> - * corresponding DRM file is closed. Drivers need not worry about >> whether the >> - * DRM file for this event still exists and can call this function >> upon >> - * completion of the asynchronous work unconditionally. >> + * This helper function sends the event @e, initialized with >> + * drm_event_reserve_init(), to its associated userspace DRM file. >> + * The timestamp variant of dma_fence_signal is used when the caller >> + * sends a valid timestamp. >> */ >> -void drm_send_event_locked(struct drm_device *dev, struct >> drm_pending_event *e) >> +void drm_send_event_helper(struct drm_device *dev, >> + struct drm_pending_event *e, ktime_t >> timestamp) >> { >> assert_spin_locked(&dev->event_lock); >> >> @@ -799,7 +798,10 @@ void drm_send_event_locked(struct drm_device >> *dev, struct drm_pending_event *e) >> } >> >> if (e->fence) { >> - dma_fence_signal(e->fence); >> + if (timestamp) >> + dma_fence_signal_timestamp(e->fence, >> timestamp); >> + else >> + dma_fence_signal(e->fence); >> dma_fence_put(e->fence); >> } >> >> @@ -814,6 +816,51 @@ void drm_send_event_locked(struct drm_device >> *dev, struct drm_pending_event *e) >> wake_up_interruptible_poll(&e->file_priv->event_wait, >> EPOLLIN | EPOLLRDNORM); >> } >> + >> +/** >> + * drm_send_event_timestamp_locked - send DRM event to file >> descriptor >> + * @dev: DRM device >> + * @e: DRM event to deliver >> + * @timestamp: timestamp to set for the fence event in kernel's >> CLOCK_MONOTONIC >> + * time domain >> + * >> + * This function sends the event @e, initialized with >> drm_event_reserve_init(), >> + * to its associated userspace DRM file. Callers must already hold >> + * &drm_device.event_lock. >> + * >> + * Note that the core will take care of unlinking and disarming >> events when the >> + * corresponding DRM file is closed. Drivers need not worry about >> whether the >> + * DRM file for this event still exists and can call this function >> upon >> + * completion of the asynchronous work unconditionally. >> + */ >> +void drm_send_event_timestamp_locked(struct drm_device *dev, >> + struct drm_pending_event *e, ktime_t >> timestamp) >> +{ >> + WARN_ON(!timestamp); >> + >> + drm_send_event_helper(dev, e, timestamp); >> + >> +} >> +EXPORT_SYMBOL(drm_send_event_timestamp_locked); > > > Hey Veera, > So actually, after closer look at the testing I was doing, we seem > to be hitting that WARN_ON right as the display first comes up (I see > this on both db845c and HiKey960). > It seems in drm_crtc_send_vblank_event(), if "now" is set by > drm_vblank_count_and_time(), the first timestamp value we get from it > seems to be 0. > > Let me know if you need any help reproducing and sorting this issue > out. > Hi John, Thanks for the review and testing. Since vblank timestamp being set to 0 is acceptable currently, I think it would be safe to remove the WARN_ON in drm_send_event_timestamp_locked(), so the same would be applied to the corresponding fences as well. Especially with high precision vblank timestamping enabled, driver is free to set and there is no timestamp validation within drm. This is a separate work and can be decoupled from this patch. Or we can use the appropriate drm_send_event variant from send_vblank_event() based on the availability of "now" timestamp. Please let me know your opinion, I will modify the patch accordingly. Thanks, Veera > thanks > -john _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event 2021-01-14 20:54 ` veeras @ 2021-01-15 22:29 ` John Stultz 2021-01-16 1:12 ` veeras 0 siblings, 1 reply; 8+ messages in thread From: John Stultz @ 2021-01-15 22:29 UTC (permalink / raw) To: Veera Sundaram Sankaran Cc: David Airlie, Gustavo Padovan, dri-devel, pdhaval, abhinavk, Sean Paul, linux-media On Thu, Jan 14, 2021 at 12:54 PM <veeras@codeaurora.org> wrote: > > On 2021-01-13 18:28, John Stultz wrote: > > On Wed, Jan 13, 2021 at 11:52 AM Veera Sundaram Sankaran > > <veeras@codeaurora.org> 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 [1] uses the > >> retire-fences as an alternative to vblank when frame-updates are in > >> progress. Set the fence timestamp during send vblank event using a new > >> drm_send_event_timestamp_locked variant to avoid discrepancies. > >> > >> [1] > >> https://android.googlesource.com/platform/frameworks/native/+/master/ > >> services/surfaceflinger/Scheduler/Scheduler.cpp#397 > >> > >> Changes in v2: > >> - Use drm_send_event_timestamp_locked to update fence timestamp > >> - add more information to commit text > >> > >> Changes in v3: > >> - use same backend helper function for variants of drm_send_event to > >> avoid code duplications > >> > >> Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org> > >> --- > >> drivers/gpu/drm/drm_file.c | 71 > >> ++++++++++++++++++++++++++++++++++++-------- > >> drivers/gpu/drm/drm_vblank.c | 9 +++++- > >> include/drm/drm_file.h | 3 ++ > >> 3 files changed, 70 insertions(+), 13 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c > >> index 0ac4566..b8348ca 100644 > >> --- a/drivers/gpu/drm/drm_file.c > >> +++ b/drivers/gpu/drm/drm_file.c > >> @@ -775,20 +775,19 @@ void drm_event_cancel_free(struct drm_device > >> *dev, > >> EXPORT_SYMBOL(drm_event_cancel_free); > >> > >> /** > >> - * drm_send_event_locked - send DRM event to file descriptor > >> + * drm_send_event_helper - send DRM event to file descriptor > >> * @dev: DRM device > >> * @e: DRM event to deliver > >> + * @timestamp: timestamp to set for the fence event in kernel's > >> CLOCK_MONOTONIC > >> + * time domain > >> * > >> - * This function sends the event @e, initialized with > >> drm_event_reserve_init(), > >> - * to its associated userspace DRM file. Callers must already hold > >> - * &drm_device.event_lock, see drm_send_event() for the unlocked > >> version. > >> - * > >> - * Note that the core will take care of unlinking and disarming > >> events when the > >> - * corresponding DRM file is closed. Drivers need not worry about > >> whether the > >> - * DRM file for this event still exists and can call this function > >> upon > >> - * completion of the asynchronous work unconditionally. > >> + * This helper function sends the event @e, initialized with > >> + * drm_event_reserve_init(), to its associated userspace DRM file. > >> + * The timestamp variant of dma_fence_signal is used when the caller > >> + * sends a valid timestamp. > >> */ > >> -void drm_send_event_locked(struct drm_device *dev, struct > >> drm_pending_event *e) > >> +void drm_send_event_helper(struct drm_device *dev, > >> + struct drm_pending_event *e, ktime_t > >> timestamp) > >> { > >> assert_spin_locked(&dev->event_lock); > >> > >> @@ -799,7 +798,10 @@ void drm_send_event_locked(struct drm_device > >> *dev, struct drm_pending_event *e) > >> } > >> > >> if (e->fence) { > >> - dma_fence_signal(e->fence); > >> + if (timestamp) > >> + dma_fence_signal_timestamp(e->fence, > >> timestamp); > >> + else > >> + dma_fence_signal(e->fence); > >> dma_fence_put(e->fence); > >> } > >> > >> @@ -814,6 +816,51 @@ void drm_send_event_locked(struct drm_device > >> *dev, struct drm_pending_event *e) > >> wake_up_interruptible_poll(&e->file_priv->event_wait, > >> EPOLLIN | EPOLLRDNORM); > >> } > >> + > >> +/** > >> + * drm_send_event_timestamp_locked - send DRM event to file > >> descriptor > >> + * @dev: DRM device > >> + * @e: DRM event to deliver > >> + * @timestamp: timestamp to set for the fence event in kernel's > >> CLOCK_MONOTONIC > >> + * time domain > >> + * > >> + * This function sends the event @e, initialized with > >> drm_event_reserve_init(), > >> + * to its associated userspace DRM file. Callers must already hold > >> + * &drm_device.event_lock. > >> + * > >> + * Note that the core will take care of unlinking and disarming > >> events when the > >> + * corresponding DRM file is closed. Drivers need not worry about > >> whether the > >> + * DRM file for this event still exists and can call this function > >> upon > >> + * completion of the asynchronous work unconditionally. > >> + */ > >> +void drm_send_event_timestamp_locked(struct drm_device *dev, > >> + struct drm_pending_event *e, ktime_t > >> timestamp) > >> +{ > >> + WARN_ON(!timestamp); > >> + > >> + drm_send_event_helper(dev, e, timestamp); > >> + > >> +} > >> +EXPORT_SYMBOL(drm_send_event_timestamp_locked); > > > > > > Hey Veera, > > So actually, after closer look at the testing I was doing, we seem > > to be hitting that WARN_ON right as the display first comes up (I see > > this on both db845c and HiKey960). > > It seems in drm_crtc_send_vblank_event(), if "now" is set by > > drm_vblank_count_and_time(), the first timestamp value we get from it > > seems to be 0. > > > > Let me know if you need any help reproducing and sorting this issue > > out. > > > Hi John, > Thanks for the review and testing. > Since vblank timestamp being set to 0 is acceptable currently, I think > it would > be safe to remove the WARN_ON in drm_send_event_timestamp_locked(), so > the same > would be applied to the corresponding fences as well. Especially with > high > precision vblank timestamping enabled, driver is free to set and there > is no > timestamp validation within drm. This is a separate work and can be > decoupled > from this patch. Or we can use the appropriate drm_send_event variant > from send_vblank_event() based on the availability of "now" timestamp. > Please let me know your opinion, I will modify the patch accordingly. Not sure if I know the vblank details well enough to really recommend a path. I suspect there needs to be some special case handling of the initialization state so the first vblank isn't at time 0, but I'd need to dig more to understand it. I'd guess removing the warning is ok for now, but it does seem like something needs to eventually be fixed. thanks -john _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event 2021-01-15 22:29 ` John Stultz @ 2021-01-16 1:12 ` veeras 0 siblings, 0 replies; 8+ messages in thread From: veeras @ 2021-01-16 1:12 UTC (permalink / raw) To: John Stultz Cc: David Airlie, Gustavo Padovan, dri-devel, pdhaval, abhinavk, Sean Paul, linux-media On 2021-01-15 14:29, John Stultz wrote: > On Thu, Jan 14, 2021 at 12:54 PM <veeras@codeaurora.org> wrote: >> >> On 2021-01-13 18:28, John Stultz wrote: >> > On Wed, Jan 13, 2021 at 11:52 AM Veera Sundaram Sankaran >> > <veeras@codeaurora.org> 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 [1] uses the >> >> retire-fences as an alternative to vblank when frame-updates are in >> >> progress. Set the fence timestamp during send vblank event using a new >> >> drm_send_event_timestamp_locked variant to avoid discrepancies. >> >> >> >> [1] >> >> https://android.googlesource.com/platform/frameworks/native/+/master/ >> >> services/surfaceflinger/Scheduler/Scheduler.cpp#397 >> >> >> >> Changes in v2: >> >> - Use drm_send_event_timestamp_locked to update fence timestamp >> >> - add more information to commit text >> >> >> >> Changes in v3: >> >> - use same backend helper function for variants of drm_send_event to >> >> avoid code duplications >> >> >> >> Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org> >> >> --- >> >> drivers/gpu/drm/drm_file.c | 71 >> >> ++++++++++++++++++++++++++++++++++++-------- >> >> drivers/gpu/drm/drm_vblank.c | 9 +++++- >> >> include/drm/drm_file.h | 3 ++ >> >> 3 files changed, 70 insertions(+), 13 deletions(-) >> >> >> >> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c >> >> index 0ac4566..b8348ca 100644 >> >> --- a/drivers/gpu/drm/drm_file.c >> >> +++ b/drivers/gpu/drm/drm_file.c >> >> @@ -775,20 +775,19 @@ void drm_event_cancel_free(struct drm_device >> >> *dev, >> >> EXPORT_SYMBOL(drm_event_cancel_free); >> >> >> >> /** >> >> - * drm_send_event_locked - send DRM event to file descriptor >> >> + * drm_send_event_helper - send DRM event to file descriptor >> >> * @dev: DRM device >> >> * @e: DRM event to deliver >> >> + * @timestamp: timestamp to set for the fence event in kernel's >> >> CLOCK_MONOTONIC >> >> + * time domain >> >> * >> >> - * This function sends the event @e, initialized with >> >> drm_event_reserve_init(), >> >> - * to its associated userspace DRM file. Callers must already hold >> >> - * &drm_device.event_lock, see drm_send_event() for the unlocked >> >> version. >> >> - * >> >> - * Note that the core will take care of unlinking and disarming >> >> events when the >> >> - * corresponding DRM file is closed. Drivers need not worry about >> >> whether the >> >> - * DRM file for this event still exists and can call this function >> >> upon >> >> - * completion of the asynchronous work unconditionally. >> >> + * This helper function sends the event @e, initialized with >> >> + * drm_event_reserve_init(), to its associated userspace DRM file. >> >> + * The timestamp variant of dma_fence_signal is used when the caller >> >> + * sends a valid timestamp. >> >> */ >> >> -void drm_send_event_locked(struct drm_device *dev, struct >> >> drm_pending_event *e) >> >> +void drm_send_event_helper(struct drm_device *dev, >> >> + struct drm_pending_event *e, ktime_t >> >> timestamp) >> >> { >> >> assert_spin_locked(&dev->event_lock); >> >> >> >> @@ -799,7 +798,10 @@ void drm_send_event_locked(struct drm_device >> >> *dev, struct drm_pending_event *e) >> >> } >> >> >> >> if (e->fence) { >> >> - dma_fence_signal(e->fence); >> >> + if (timestamp) >> >> + dma_fence_signal_timestamp(e->fence, >> >> timestamp); >> >> + else >> >> + dma_fence_signal(e->fence); >> >> dma_fence_put(e->fence); >> >> } >> >> >> >> @@ -814,6 +816,51 @@ void drm_send_event_locked(struct drm_device >> >> *dev, struct drm_pending_event *e) >> >> wake_up_interruptible_poll(&e->file_priv->event_wait, >> >> EPOLLIN | EPOLLRDNORM); >> >> } >> >> + >> >> +/** >> >> + * drm_send_event_timestamp_locked - send DRM event to file >> >> descriptor >> >> + * @dev: DRM device >> >> + * @e: DRM event to deliver >> >> + * @timestamp: timestamp to set for the fence event in kernel's >> >> CLOCK_MONOTONIC >> >> + * time domain >> >> + * >> >> + * This function sends the event @e, initialized with >> >> drm_event_reserve_init(), >> >> + * to its associated userspace DRM file. Callers must already hold >> >> + * &drm_device.event_lock. >> >> + * >> >> + * Note that the core will take care of unlinking and disarming >> >> events when the >> >> + * corresponding DRM file is closed. Drivers need not worry about >> >> whether the >> >> + * DRM file for this event still exists and can call this function >> >> upon >> >> + * completion of the asynchronous work unconditionally. >> >> + */ >> >> +void drm_send_event_timestamp_locked(struct drm_device *dev, >> >> + struct drm_pending_event *e, ktime_t >> >> timestamp) >> >> +{ >> >> + WARN_ON(!timestamp); >> >> + >> >> + drm_send_event_helper(dev, e, timestamp); >> >> + >> >> +} >> >> +EXPORT_SYMBOL(drm_send_event_timestamp_locked); >> > >> > >> > Hey Veera, >> > So actually, after closer look at the testing I was doing, we seem >> > to be hitting that WARN_ON right as the display first comes up (I see >> > this on both db845c and HiKey960). >> > It seems in drm_crtc_send_vblank_event(), if "now" is set by >> > drm_vblank_count_and_time(), the first timestamp value we get from it >> > seems to be 0. >> > >> > Let me know if you need any help reproducing and sorting this issue >> > out. >> > >> Hi John, >> Thanks for the review and testing. >> Since vblank timestamp being set to 0 is acceptable currently, I think >> it would >> be safe to remove the WARN_ON in drm_send_event_timestamp_locked(), so >> the same >> would be applied to the corresponding fences as well. Especially with >> high >> precision vblank timestamping enabled, driver is free to set and there >> is no >> timestamp validation within drm. This is a separate work and can be >> decoupled >> from this patch. Or we can use the appropriate drm_send_event variant >> from send_vblank_event() based on the availability of "now" timestamp. >> Please let me know your opinion, I will modify the patch accordingly. > > > Not sure if I know the vblank details well enough to really recommend a > path. > I suspect there needs to be some special case handling of the > initialization state so the first vblank isn't at time 0, but I'd need > to dig more to understand it. > > I'd guess removing the warning is ok for now, but it does seem like > something needs to eventually be fixed. > Thanks John. Agree that path might need a fix. Updated the current patch with warn removed. Please take a look. Thanks, Veera > thanks > -john _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/2] dma-fence: allow signaling drivers to set fence timestamp 2021-01-13 19:52 [PATCH v3 1/2] dma-fence: allow signaling drivers to set fence timestamp Veera Sundaram Sankaran 2021-01-13 19:52 ` [PATCH v3 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event Veera Sundaram Sankaran @ 2021-01-14 1:16 ` John Stultz 1 sibling, 0 replies; 8+ messages in thread From: John Stultz @ 2021-01-14 1:16 UTC (permalink / raw) To: Veera Sundaram Sankaran Cc: David Airlie, Gustavo Padovan, dri-devel, pdhaval, abhinavk, Sean Paul, linux-media On Wed, Jan 13, 2021 at 11:52 AM Veera Sundaram Sankaran <veeras@codeaurora.org> wrote: > Some drivers have hardware capability to get the precise HW timestamp > of certain events based on which the fences are triggered. The delta > between the event HW timestamp & current HW reference timestamp can > be used to calculate the timestamp in kernel's CLOCK_MONOTONIC time > domain. 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 > > Changes in v3: > - Add timestamp domain information to commit-text and > dma_fence_signal_timestamp documentation > > Signed-off-by: Veera Sundaram Sankaran <veeras@codeaurora.org> Looks ok to me, also did some brief testing w/ AOSP and didn't see any regressions. Reviewed-by: John Stultz <john.stultz@linaro.org> thanks -john _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-16 10:45 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-13 19:52 [PATCH v3 1/2] dma-fence: allow signaling drivers to set fence timestamp Veera Sundaram Sankaran 2021-01-13 19:52 ` [PATCH v3 2/2] drm/drm_vblank: set the dma-fence timestamp during send_vblank_event Veera Sundaram Sankaran 2021-01-14 1:29 ` John Stultz 2021-01-14 2:28 ` John Stultz 2021-01-14 20:54 ` veeras 2021-01-15 22:29 ` John Stultz 2021-01-16 1:12 ` veeras 2021-01-14 1:16 ` [PATCH v3 1/2] dma-fence: allow signaling drivers to set fence timestamp John Stultz
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).