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@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Acked-by: David Airlie <airlied@xxxxxxxx> #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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
