[Intel-gfx] [PATCH v2] drm: Add high-precision time to vblank trace event

[Heinrich Fink]

On Mon, 23 Sep 2019 at 14:06, Daniel Vetter <daniel at ffwll.ch> wrote:
>
> On Sat, Sep 21, 2019 at 10:33 AM Heinrich Fink <heinrich.fink at daqri.com> wrote:
> >
> > On Tue, 3 Sep 2019 at 11:53, Daniel Vetter <daniel at ffwll.ch> wrote:
> > >
> > > On Tue, Sep 03, 2019 at 11:19:19AM +0200, Heinrich Fink wrote:
> > > > On Tue, 3 Sep 2019 at 09:46, Daniel Vetter <daniel at ffwll.ch> wrote:
> > > > >
> > > > > On Mon, Sep 02, 2019 at 04:24:12PM +0200, Heinrich Fink wrote:
> > > > > > Store the timestamp of the current vblank in the new field 'time' of the
> > > > > > vblank trace event. If the timestamp is calculated by a driver that
> > > > > > supports high-precision vblank timing, set the field 'high-prec' to
> > > > > > 'true'.
> > > > > >
> > > > > > User space can now access actual hardware vblank times via the tracing
> > > > > > infrastructure. Tracing applications (such as GPUVis, see [0] for
> > > > > > related discussion), can use the newly added information to conduct a
> > > > > > more accurate analysis of display timing.
> > > > > >
> > > > > > v2 Fix author name (missing last name)
> > > > > >
> > > > > > [0] https://github.com/mikesart/gpuvis/issues/30
> > > > > >
> > > > > > Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > > > > Signed-off-by: Heinrich Fink <heinrich.fink at daqri.com>
> > > > >
> > > > > Applied, thanks.
> > > >
> > > > thanks! One question: After sending v2, I got an email from patchwork
> > > > pointing at some failed style checks (CHECK:PARENTHESIS_ALIGNMENT,
> > > > CHECK:COMPARISON_TO_NULL). Just so I know for the future, are these
> > > > checks mandatory to be addressed? I haven't had a chance to address
> > > > them yet. FWIW, linux-tree/scripts/checkpatch.pl did not complain.
> > >
> > > It's the same script, but I think CI uses some different options/flags. I
> > > generally ignore these, but also generally good to stick to the style.
> > >
> > > $ dim checkpatch
> > >
> > > in our maintainer-tools should give you the drm flavoured checkpatch.
> > > -Daniel
> >
> > Apologies if that's a basic question, but at which point is this patch
> > landing upstream? I am monitoring the 5.4 merge window and couldn't
> > figure out what the stages are for this patch to get there. Is there
> > anything that is still left to do from my side?
>
> It missed the 5.4 feature cut-off just barely:
>
> commit 6914f8eb64f9de5308e2968968145cf6eb304025
> Author: Heinrich Fink <heinrich.fink at daqri.com>
> Date:   Mon Sep 2 16:24:12 2019 +0200
>
>     drm: Add high-precision time to vblank trace event
>
> is your patch. Should land in 5.5 (and will show up in linux-next
> after -rc1 is tagged).

Thanks for the explanation, I missed that this landed in drm-misc.

cheers, Heinrich

> -Daniel
>
> >
> > Thanks, Heinrich
> >
> > >
> > > >
> > > > - Heinrich
> > > >
> > > > > -Daniel
> > > > >
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_trace.h  | 14 ++++++++++----
> > > > > >  drivers/gpu/drm/drm_vblank.c |  3 ++-
> > > > > >  2 files changed, 12 insertions(+), 5 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/drm_trace.h b/drivers/gpu/drm/drm_trace.h
> > > > > > index 471eb927474b..11c6dd577e8e 100644
> > > > > > --- a/drivers/gpu/drm/drm_trace.h
> > > > > > +++ b/drivers/gpu/drm/drm_trace.h
> > > > > > @@ -13,17 +13,23 @@ struct drm_file;
> > > > > >  #define TRACE_INCLUDE_FILE drm_trace
> > > > > >
> > > > > >  TRACE_EVENT(drm_vblank_event,
> > > > > > -         TP_PROTO(int crtc, unsigned int seq),
> > > > > > -         TP_ARGS(crtc, seq),
> > > > > > +         TP_PROTO(int crtc, unsigned int seq, ktime_t time, bool high_prec),
> > > > > > +         TP_ARGS(crtc, seq, time, high_prec),
> > > > > >           TP_STRUCT__entry(
> > > > > >                   __field(int, crtc)
> > > > > >                   __field(unsigned int, seq)
> > > > > > +                 __field(ktime_t, time)
> > > > > > +                 __field(bool, high_prec)
> > > > > >                   ),
> > > > > >           TP_fast_assign(
> > > > > >                   __entry->crtc = crtc;
> > > > > >                   __entry->seq = seq;
> > > > > > -                 ),
> > > > > > -         TP_printk("crtc=%d, seq=%u", __entry->crtc, __entry->seq)
> > > > > > +                 __entry->time = time;
> > > > > > +                 __entry->high_prec = high_prec;
> > > > > > +                     ),
> > > > > > +         TP_printk("crtc=%d, seq=%u, time=%lld, high-prec=%s",
> > > > > > +                     __entry->crtc, __entry->seq, __entry->time,
> > > > > > +                     __entry->high_prec ? "true" : "false")
> > > > > >  );
> > > > > >
> > > > > >  TRACE_EVENT(drm_vblank_event_queued,
> > > > > > diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> > > > > > index fd1fbc77871f..c99feda25dea 100644
> > > > > > --- a/drivers/gpu/drm/drm_vblank.c
> > > > > > +++ b/drivers/gpu/drm/drm_vblank.c
> > > > > > @@ -1731,7 +1731,8 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
> > > > > >               send_vblank_event(dev, e, seq, now);
> > > > > >       }
> > > > > >
> > > > > > -     trace_drm_vblank_event(pipe, seq);
> > > > > > +     trace_drm_vblank_event(pipe, seq, now,
> > > > > > +                     dev->driver->get_vblank_timestamp != NULL);
> > > > > >  }
> > > > > >
> > > > > >  /**
> > > > > > --
> > > > > > 2.23.0.rc1
> > > > > >
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch