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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2021-01-13 18:28, John Stultz wrote:
On Wed, Jan 13, 2021 at 11:52 AM Veera Sundaram Sankaran
<veeras@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux