[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 at 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 at 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