Re: [RFC 4/4] drm: add support for raw monotonic vblank timestamps

[Imre Deak <imre.deak@intel.com>]

On Fri, 2012-10-05 at 18:09 -0600, Rob Clark wrote:
> On Fri, Oct 5, 2012 at 5:41 PM, Imre Deak <imre.deak@intel.com> wrote:
> > On Fri, 2012-10-05 at 16:18 -0600, Rob Clark wrote:
> >> On Fri, Oct 5, 2012 at 7:37 AM, Imre Deak <imre.deak@intel.com> wrote:
> >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> > index ab1ef15..056e810 100644
> >> > --- a/drivers/gpu/drm/i915/intel_display.c
> >> > +++ b/drivers/gpu/drm/i915/intel_display.c
> >> > @@ -6247,7 +6247,6 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
> >> >         struct intel_unpin_work *work;
> >> >         struct drm_i915_gem_object *obj;
> >> >         struct drm_pending_vblank_event *e;
> >> > -       struct timeval tvbl;
> >> >         unsigned long flags;
> >> >
> >> >         /* Ignore early vblank irqs */
> >> > @@ -6264,12 +6263,13 @@ static void do_intel_finish_page_flip(struct drm_device *dev,
> >> >         intel_crtc->unpin_work = NULL;
> >> >
> >> >         if (work->event) {
> >> > -               e = work->event;
> >> > -               e->event.sequence = drm_vblank_count_and_time(dev, intel_crtc->pipe, &tvbl);
> >> > -
> >> > -               e->event.tv_sec = tvbl.tv_sec;
> >> > -               e->event.tv_usec = tvbl.tv_usec;
> >> > +               struct drm_vblank_time tvbl;
> >> > +               u32 seq;
> >> >
> >> > +               e = work->event;
> >> > +               seq = drm_vblank_count_and_time(dev, intel_crtc->pipe, &tvbl);
> >> > +               drm_set_event_seq_and_time(&e->event, e->timestamp_raw, seq,
> >> > +                                          &tvbl);
> >> >                 list_add_tail(&e->base.link,
> >> >                               &e->base.file_priv->event_list);
> >> >                 wake_up_interruptible(&e->base.file_priv->event_wait);
> >>
> >>
> >> btw, I wonder if we could just have a helper like:
> >>
> >> int drm_send_page_flip_event(struct drm_device *dev, int crtc,
> >>               struct drm_pending_vblank_event *event);
> >>
> >> Since most drivers have pretty much the same code for sending vblank
> >> event after a page flip..
> >>
> >> I guess not strictly related to monotonic timestamps, but it has been
> >> a cleanup that I've been meaning to do for a while.. and I guess if
> >> this was done first it wouldn't mean touching each driver (as much) to
> >> add support for monotonic timestamps.
> >
> > Good idea, we should do this.
> >
> > But if we want to reduce the diff from my changes then the proto should
> > rather be:
> >
> > int drm_send_page_flip_event(struct drm_device *dev, int crtc,
> >                struct drm_pending_vblank_event *event,
> >                int seq, struct timeval *now);
> 
> do we need 'seq' and 'now'?  I was sort of thinking that could all be
> internal to the send_page_flip_event() fxn.. ie. like:
> 
> ---------
> void drm_send_page_flip_event(struct drm_device *dev, int pipe,
> 		struct drm_crtc *crtc, struct drm_pending_vblank_event *event)
> {
> 	struct timeval tnow, tvbl;
> 
> 	do_gettimeofday(&tnow);
> 
> 	event->event.sequence = drm_vblank_count_and_time(dev, pipe, &tvbl);

Ah, ok, you are right. I originally thought of using this helper in
drm_handle_vblank_events too, but now I realized it's not quite the same
sequence there.

> 
> 	/* Called before vblank count and timestamps have
> 	 * been updated for the vblank interval of flip
> 	 * completion? Need to increment vblank count and
> 	 * add one videorefresh duration to returned timestamp
> 	 * to account for this. We assume this happened if we
> 	 * get called over 0.9 frame durations after the last
> 	 * timestamped vblank.
> 	 *
> 	 * This calculation can not be used with vrefresh rates
> 	 * below 5Hz (10Hz to be on the safe side) without
> 	 * promoting to 64 integers.
> 	 */
> 	if (10 * (timeval_to_ns(&tnow) - timeval_to_ns(&tvbl)) >
> 	    9 * crtc->framedur_ns) {
> 		event->event.sequence++;
> 		tvbl = ns_to_timeval(timeval_to_ns(&tvbl) +
> 				     crtc->framedur_ns);
> 	}

This has been recently removed by danvet's "drm/i915: don't frob the
vblank ts in finish_page_flip", though not yet committed, so we can do
away with it here too.

> 	event->event.tv_sec = tvbl.tv_sec;
> 	event->event.tv_usec = tvbl.tv_usec;
> 
> 	list_add_tail(&event->base.link,
> 		      &event->base.file_priv->event_list);
> 	wake_up_interruptible(&event->base.file_priv->event_wait);
> }


> ---------
> 
> well, this would be the pre-monotonic timestamp version.. and it is a
> bit weird to have to pass in both crtc ptr and pipe #.. I don't see
> anything obvious in drm core that links pipe # and crtc ptr, although
> userspace seems to make this assumption about order of crtc's.  But I
> think that if() statement is only in i915 driver.. I assume because
> you don't know if this will get called before or after
> drm_handle_vblank()?  I guess that shouldn't hurt for other drivers.
> 
> then each driver would just have something like:
> 
> ---------
> 	if (work->event)
> 		drm_send_page_flip_event(dev, intel_crtc->pipe, crtc, work->event);
> ---------
> 
> 
> > with struct timeval changing to struct drm_vblank_time with my changes.
> >
> > I can do this if you agree - unless you have something ready.
> 
> I've locally split out this into a helper fxn in omapdrm, but haven't
> got around to pushing it to core and updating other drivers.  If you
> want I can send a patch, but I guess it is easier to just include
> something like this in your patchset.  I'm ok either way.

I think the easiest is if you post your updated patch since it can be
anyway applied independently. I'll then rebase my changes on top of
that.

Thanks,
Imre