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

[John Stultz <john.stultz@linaro.org>]

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