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

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

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

with struct timeval changing to struct drm_vblank_time with my changes.

I can do this if you agree - unless you have something ready.

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