| Message ID | 571DB9F8.7040903@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/25/2016 08:32 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) > Changes since v4: > - Add warning to docs when the function is useful. > - Add a WARN_ON when get_vblank_timestamp is unavailable. > - Use drm_vblank_count. > > 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> #v4 > Acked-by: David Airlie <airlied@linux.ie> #irc, v4 > --- > > Unfortunately WARN_ON(!dev->disable_vblank_immediate) doesn't work on gen2, > which is the reason this function is created. So I used > WARN_ON(!get_vblank_timestamp) instead. That's a weaker warning. I'd like to have the WARN_ON and the doc text to be more frightening/restrictive to discourage abuse. But can't you simply remove that !IS_GEN2 check now and always set dev->disable_vblank_immediate = true? The reason for that exception was that GEN2 doesn't have a hw vblank counter. But it has scanout pos based vblank timestamping, which i'd assume is well behaved. With the new scanout based vblank counter emulation in drm_update_vblank_count() since around Linux 4.4 you therefore essentially have a proper emulated vblank counter, so this should be safe. Ville will probably know. Otherwise you couldn't trust drm_accurate_vblank_count() here, because it depends on the same logic, no? -mario > > drivers/gpu/drm/drm_irq.c | 31 +++++++++++++++++++++++++++++++ > include/drm/drmP.h | 1 + > 2 files changed, 32 insertions(+) > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index 3c1a6f18e71c..d3124b67f4a5 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -303,6 +303,37 @@ 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. > + * > + * This is mostly useful for hardware that can obtain the scanout > + * position, but doesn't have a frame counter. > + */ > +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; > + > + WARN(!dev->driver->get_vblank_timestamp, > + "This function requires support for accurate vblank timestamps."); > + > + spin_lock_irqsave(&dev->vblank_time_lock, flags); > + > + drm_update_vblank_count(dev, pipe, 0); > + vblank = drm_vblank_count(dev, pipe); > + > + 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, >
Anyway, although i would have liked the stricter check and warning docs, the v4 patch is ok with me: Reviewed-by: Mario Kleiner <mario.kleiner.de@gmail.com> -mario On 04/25/2016 08:32 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) > Changes since v4: > - Add warning to docs when the function is useful. > - Add a WARN_ON when get_vblank_timestamp is unavailable. > - Use drm_vblank_count. > > 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> #v4 > Acked-by: David Airlie <airlied@linux.ie> #irc, v4 > --- > > Unfortunately WARN_ON(!dev->disable_vblank_immediate) doesn't work on gen2, > which is the reason this function is created. So I used > WARN_ON(!get_vblank_timestamp) instead. > > drivers/gpu/drm/drm_irq.c | 31 +++++++++++++++++++++++++++++++ > include/drm/drmP.h | 1 + > 2 files changed, 32 insertions(+) > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c > index 3c1a6f18e71c..d3124b67f4a5 100644 > --- a/drivers/gpu/drm/drm_irq.c > +++ b/drivers/gpu/drm/drm_irq.c > @@ -303,6 +303,37 @@ 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. > + * > + * This is mostly useful for hardware that can obtain the scanout > + * position, but doesn't have a frame counter. > + */ > +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; > + > + WARN(!dev->driver->get_vblank_timestamp, > + "This function requires support for accurate vblank timestamps."); > + > + spin_lock_irqsave(&dev->vblank_time_lock, flags); > + > + drm_update_vblank_count(dev, pipe, 0); > + vblank = drm_vblank_count(dev, pipe); > + > + 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, >
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 3c1a6f18e71c..d3124b67f4a5 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -303,6 +303,37 @@ 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. + * + * This is mostly useful for hardware that can obtain the scanout + * position, but doesn't have a frame counter. + */ +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; + + WARN(!dev->driver->get_vblank_timestamp, + "This function requires support for accurate vblank timestamps."); + + spin_lock_irqsave(&dev->vblank_time_lock, flags); + + drm_update_vblank_count(dev, pipe, 0); + vblank = drm_vblank_count(dev, pipe); + + 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,