| Message ID | 1605205643-12746-2-git-send-email-veeras@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,1/2] dma-fence: allow signaling drivers to set fence timestamp | expand |
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. Aside from documenting why we want to do this I think this all looks reasonable. -Daniel > > 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); > } > -- > 2.7.4 >
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. 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@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 >>
On Thu, Nov 19, 2020 at 2:26 AM <veeras@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@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 > >>
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); }
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(+)