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

[Rob Clark <rob.clark@linaro.org>]

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);

	/* 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);
	}

	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.

BR,
-R

> --Imre
>
>>
>> BR,
>> -R
>>
>> > diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
>> > index 7ddef8f..55c014a 100644
>> > --- a/drivers/gpu/drm/radeon/radeon_display.c
>> > +++ b/drivers/gpu/drm/radeon/radeon_display.c
>> > @@ -272,7 +272,6 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
>> >         struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[crtc_id];
>> >         struct radeon_unpin_work *work;
>> >         struct drm_pending_vblank_event *e;
>> > -       struct timeval now;
>> >         unsigned long flags;
>> >         u32 update_pending;
>> >         int vpos, hpos;
>> > @@ -329,10 +328,13 @@ void radeon_crtc_handle_flip(struct radeon_device *rdev, int crtc_id)
>> >
>> >         /* wakeup userspace */
>> >         if (work->event) {
>> > +               struct drm_vblank_time now;
>> > +               u32 seq;
>> > +
>> >                 e = work->event;
>> > -               e->event.sequence = drm_vblank_count_and_time(rdev->ddev, crtc_id, &now);
>> > -               e->event.tv_sec = now.tv_sec;
>> > -               e->event.tv_usec = now.tv_usec;
>> > +               seq = drm_vblank_count_and_time(rdev->ddev, crtc_id, &now);
>> > +               drm_set_event_seq_and_time(&e->event, e->timestamp_raw,
>> > +                                          seq, &now);
>> >                 list_add_tail(&e->base.link, &e->base.file_priv->event_list);
>> >                 wake_up_interruptible(&e->base.file_priv->event_wait);
>> >         }
>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel