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

Imre Deak imre.deak at intel.com
Fri Oct 5 17:49:16 PDT 2012


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




More information about the dri-devel mailing list