Re: [PATCH 01/19] drm/core: Add drm_accurate_vblank_count, v4.

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Apr 25, 2016 at 06:35:48AM +0200, Mario Kleiner wrote:
> Sorry for the late review, but see below...
> 
> On 04/19/2016 09:52 AM, Maarten Lankhorst wrote:
> > This function is useful for gen2 intel devices which have no frame
> > counter, but need a way to determine the current vblank count without
> > racing with the vblank interrupt handler.
> >
> > intel_pipe_update_start checks if no vblank interrupt will occur
> > during vblank evasion, but cannot check whether the vblank handler has
> > run to completion. This function uses the timestamps to determine
> > when the last vblank has happened, and interpolates from there.
> >
> > Changes since v1:
> > - Take vblank_time_lock and don't use drm_vblank_count_and_time.
> > Changes since v2:
> > - Don't return time of last vblank.
> > Changes since v3:
> > - Change pipe to unsigned int. (Ville)
> > - Remove unused documentation for tv_ret. (kbuild)
> >
> > Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Acked-by: David Airlie <airlied@linux.ie> #irc
> > ---
> >   drivers/gpu/drm/drm_irq.c | 26 ++++++++++++++++++++++++++
> >   include/drm/drmP.h        |  1 +
> >   2 files changed, 27 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 3c1a6f18e71c..f1bda13562da 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -303,6 +303,32 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
> >   	store_vblank(dev, pipe, diff, &t_vblank, cur_vblank);
> >   }
> >
> > +/**
> > + * drm_accurate_vblank_count - retrieve the master vblank counter
> > + * @crtc: which counter to retrieve
> > + *
> > + * This function is similar to @drm_crtc_vblank_count but this
> > + * function interpolates to handle a race with vblank irq's.
> > + */
> > +
> > +u32 drm_accurate_vblank_count(struct drm_crtc *crtc)
> > +{
> > +	struct drm_device *dev = crtc->dev;
> > +	unsigned int pipe = drm_crtc_index(crtc);
> > +	u32 vblank;
> > +	unsigned long flags;
> > +
> 
> This function is rather dangerous to use on any driver that doesn't have 
> precise vblank timestamping, or doesn't have the guarantee that hw 
> vblank counters (if there are any) and timestamps update exactly at 
> leading edge of vblank, so i think we need some WARN() here and maybe 
> much less encouraging docs to avoid this being called from incapable kms 
> drivers or in general code.
> 
> - If the driver doesn't have precise scanoutpos based timestamping each 
> call into drm_update_vblank_count from non-irq context will reset the 
> vblank timestamps to zero, so clients will only receive invalid 
> timestamps if this is frequently used. Also bogus vblank counts
> 
> Atm. only i915 Intel, AMD, NVidia desktop for >= NV-50, maybe nouveau 
> driven Tegra parts, and some modern Adrenos (msm/mdp-5 - i assume from 
> the code?) support this reliably.
> 
> - If the drivers scanoutpos timestamps and/or vblank counter don't 
> increment at leading edge we will get funny off-by-one problems with 
> vblank counters. That's why we normally only call 
> drm_update_vblank_count() from vblank irq on such parts - the only safe 
> place to avoid off-by-one problems, and limit vblank disable/enable to 
> only at most once every 5 seconds to reduce the problems caused by 
> off-by-one errors.
> 
> Which restricts the list to only the above parts, maybe minus Adreno 
> where i don't know if it obeys the "leading edge" rule or not.
> 
> So on most SoC's one must not use this function.
> 
> WARN_ON(!dev->vblank_disable_immediate, "This function is unsafe on this 
> driver.");
> 
> would probably prevent the worst abuse, unless drivers lie about 
> vblank_disable_immediate. Not sure how much this was checked for msm / 
> Adreno? At least drm_vblank_init() only allows vblank_disable_immediate 
> if the driver at least implements proper timestamping.
> 
> Not sure how much general use this function will have outside Intel 
> gen-2 with the restrictions on safe use?

This (or something like it) needs to be used also in generic vblank
wait functions. Currently those are racy.

> 
> > +	spin_lock_irqsave(&dev->vblank_time_lock, flags);
> > +
> > +	drm_update_vblank_count(dev, pipe, 0);
> > +	vblank = dev->vblank[pipe].count;
> 
> Could do vblank = drm_vblank_count(dev, pipe); instead, given that we 
> avoid open coding this in most places.
> 
> -mario
> 
> > +
> > +	spin_unlock_irqrestore(&dev->vblank_time_lock, flags);
> > +
> > +	return vblank;
> > +}
> > +EXPORT_SYMBOL(drm_accurate_vblank_count);
> > +
> >   /*
> >    * Disable vblank irq's on crtc, make sure that last vblank count
> >    * of hardware and corresponding consistent software vblank counter
> > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > index 005202ea5900..90527c41cd5a 100644
> > --- a/include/drm/drmP.h
> > +++ b/include/drm/drmP.h
> > @@ -995,6 +995,7 @@ extern void drm_crtc_vblank_off(struct drm_crtc *crtc);
> >   extern void drm_crtc_vblank_reset(struct drm_crtc *crtc);
> >   extern void drm_crtc_vblank_on(struct drm_crtc *crtc);
> >   extern void drm_vblank_cleanup(struct drm_device *dev);
> > +extern u32 drm_accurate_vblank_count(struct drm_crtc *crtc);
> >   extern u32 drm_vblank_no_hw_counter(struct drm_device *dev, unsigned int pipe);
> >
> >   extern int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
> >

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx