From: Ville Syrjälä ville.syrjala@linux.intel.com
This series has a few goals: - make timestamps more accurate by not using rounded pixel/line durations - avoid infinite loops in vblank code - use the timestamps to guesttimate the number of missed vblanks, in case there's no hardware frame counter, or it can't be trusted - fix bogus vblank counts/timstamps when the vblank counter incerements by an even number
Also available in my git repo (with a few hacks on top): git://github.com/vsyrjala/linux.git vblank_ts_diff_5
Ville Syrjälä (11): drm: s/int crtc/unsigned int pipe/ straggles drm: Move timestamping constants into drm_vblank_crtc drm: Stop using linedur_ns and pixeldur_ns for vblank timestamps drm: Kill pixeldur_ns drm/i915: Fix vblank count variable types drm: Pass flags to drm_update_vblank_count() drm: Limit the number of .get_vblank_counter() retries drm: Clean up drm_calc_vbltimestamp_from_scanoutpos() vbl_status drm: store_vblank() is never called with NULL timestamp drm: Use vblank timestamps to guesstimate how many vblanks were missed drm: Fix vblank timestamp races
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +- drivers/gpu/drm/drm_irq.c | 328 ++++++++++++++---------------- drivers/gpu/drm/i915/i915_irq.c | 5 +- drivers/gpu/drm/i915/intel_drv.h | 4 +- drivers/gpu/drm/nouveau/nouveau_display.c | 8 +- drivers/gpu/drm/nouveau/nouveau_display.h | 3 +- drivers/gpu/drm/radeon/radeon_display.c | 14 +- drivers/gpu/drm/radeon/radeon_drv.c | 5 +- drivers/gpu/drm/radeon/radeon_kms.c | 2 +- drivers/gpu/drm/radeon/radeon_mode.h | 5 +- drivers/gpu/drm/radeon/radeon_pm.c | 4 +- include/drm/drmP.h | 21 +- include/drm/drm_crtc.h | 6 - 13 files changed, 194 insertions(+), 213 deletions(-)
From: Ville Syrjälä ville.syrjala@linux.intel.com
Finish the recent replacement of 'int pipe' with 'unsigned int pipe'
Cc: Thierry Reding treding@nvidia.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 22d207e..8df4133 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -74,11 +74,11 @@ module_param_named(vblankoffdelay, drm_vblank_offdelay, int, 0600); module_param_named(timestamp_precision_usec, drm_timestamp_precision, int, 0600); module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
-static void store_vblank(struct drm_device *dev, int crtc, +static void store_vblank(struct drm_device *dev, unsigned int pipe, u32 vblank_count_inc, struct timeval *t_vblank) { - struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; + struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; u32 tslot;
assert_spin_locked(&dev->vblank_time_lock); @@ -88,7 +88,7 @@ static void store_vblank(struct drm_device *dev, int crtc, * the latching of vblank->count below. */ tslot = vblank->count + vblank_count_inc; - vblanktimestamp(dev, crtc, tslot) = *t_vblank; + vblanktimestamp(dev, pipe, tslot) = *t_vblank; }
/* @@ -110,7 +110,7 @@ static void store_vblank(struct drm_device *dev, int crtc, * @pipe: counter to update * * Call back into the driver to update the appropriate vblank counter - * (specified by @crtc). Deal with wraparound, if it occurred, and + * (specified by @pipe). Deal with wraparound, if it occurred, and * update the last read value so we can deal with wraparound on the next * call if necessary. * @@ -1154,8 +1154,8 @@ EXPORT_SYMBOL(drm_crtc_vblank_put); * @dev: DRM device * @pipe: CRTC index * - * This waits for one vblank to pass on @crtc, using the irq driver interfaces. - * It is a failure to call this when the vblank irq for @crtc is disabled, e.g. + * This waits for one vblank to pass on @pipe, using the irq driver interfaces. + * It is a failure to call this when the vblank irq for @pipe is disabled, e.g. * due to lack of driver support or because the crtc is off. */ void drm_wait_one_vblank(struct drm_device *dev, unsigned int pipe) @@ -1288,8 +1288,8 @@ void drm_crtc_vblank_reset(struct drm_crtc *drm_crtc) { struct drm_device *dev = drm_crtc->dev; unsigned long irqflags; - int crtc = drm_crtc_index(drm_crtc); - struct drm_vblank_crtc *vblank = &dev->vblank[crtc]; + unsigned int pipe = drm_crtc_index(drm_crtc); + struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
spin_lock_irqsave(&dev->vbl_lock, irqflags); /*
On Mon, Sep 14, 2015 at 10:43:42PM +0300, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Finish the recent replacement of 'int pipe' with 'unsigned int pipe'
Cc: Thierry Reding treding@nvidia.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_irq.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
Looks good to me. I have a bunch more that do a similar conversion but end up being rather invasive because they change the driver API and callbacks. I'll see if I can find the time to rebase and send them out later today.
This patch:
Reviewed-by: Thierry Reding treding@nvidia.com
On Tue, Sep 15, 2015 at 09:29:41AM +0200, Thierry Reding wrote:
On Mon, Sep 14, 2015 at 10:43:42PM +0300, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Finish the recent replacement of 'int pipe' with 'unsigned int pipe'
Cc: Thierry Reding treding@nvidia.com Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_irq.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
Looks good to me. I have a bunch more that do a similar conversion but end up being rather invasive because they change the driver API and callbacks. I'll see if I can find the time to rebase and send them out later today.
This patch:
Reviewed-by: Thierry Reding treding@nvidia.com
Queued for -next, thanks for the patch. -Daniel
From: Ville Syrjälä ville.syrjala@linux.intel.com
Collect the timestamping constants alongside the rest of the relevant stuff under drm_vblank_crtc.
We can now get rid of the 'refcrtc' parameter to drm_calc_vbltimestamp_from_scanoutpos().
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 2 +- drivers/gpu/drm/drm_irq.c | 16 ++++++++-------- drivers/gpu/drm/i915/i915_irq.c | 1 - drivers/gpu/drm/nouveau/nouveau_display.c | 5 +++-- drivers/gpu/drm/radeon/radeon_kms.c | 2 +- include/drm/drmP.h | 4 +++- include/drm/drm_crtc.h | 6 ------ 7 files changed, 16 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c index 2236793..ecfa703 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c @@ -681,7 +681,7 @@ int amdgpu_get_vblank_timestamp_kms(struct drm_device *dev, int crtc, /* Helper routine in DRM core does all the work: */ return drm_calc_vbltimestamp_from_scanoutpos(dev, crtc, max_error, vblank_time, flags, - drmcrtc, &drmcrtc->hwmode); + &drmcrtc->hwmode); }
const struct drm_ioctl_desc amdgpu_ioctls_kms[] = { diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 8df4133..6b2fefd 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -603,6 +603,7 @@ int drm_control(struct drm_device *dev, void *data, void drm_calc_timestamping_constants(struct drm_crtc *crtc, const struct drm_display_mode *mode) { + struct drm_vblank_crtc *vblank = &crtc->dev->vblank[drm_crtc_index(crtc)]; int linedur_ns = 0, pixeldur_ns = 0, framedur_ns = 0; int dotclock = mode->crtc_clock;
@@ -628,9 +629,9 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc, DRM_ERROR("crtc %u: Can't calculate constants, dotclock = 0!\n", crtc->base.id);
- crtc->pixeldur_ns = pixeldur_ns; - crtc->linedur_ns = linedur_ns; - crtc->framedur_ns = framedur_ns; + vblank->pixeldur_ns = pixeldur_ns; + vblank->linedur_ns = linedur_ns; + vblank->framedur_ns = framedur_ns;
DRM_DEBUG("crtc %u: hwmode: htotal %d, vtotal %d, vdisplay %d\n", crtc->base.id, mode->crtc_htotal, @@ -651,7 +652,6 @@ EXPORT_SYMBOL(drm_calc_timestamping_constants); * @flags: Flags to pass to driver: * 0 = Default, * DRM_CALLED_FROM_VBLIRQ = If function is called from vbl IRQ handler - * @refcrtc: CRTC which defines scanout timing * @mode: mode which defines the scanout timings * * Implements calculation of exact vblank timestamps from given drm_display_mode @@ -692,9 +692,9 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, int *max_error, struct timeval *vblank_time, unsigned flags, - const struct drm_crtc *refcrtc, const struct drm_display_mode *mode) { + struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; struct timeval tv_etime; ktime_t stime, etime; int vbl_status; @@ -714,9 +714,9 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, }
/* Durations of frames, lines, pixels in nanoseconds. */ - framedur_ns = refcrtc->framedur_ns; - linedur_ns = refcrtc->linedur_ns; - pixeldur_ns = refcrtc->pixeldur_ns; + framedur_ns = vblank->framedur_ns; + linedur_ns = vblank->linedur_ns; + pixeldur_ns = vblank->pixeldur_ns;
/* If mode timing undefined, just return as no-op: * Happens during initial modesetting of a crtc. diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 90bc6c2..4aee725 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -867,7 +867,6 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe, /* Helper routine in DRM core does all the work: */ return drm_calc_vbltimestamp_from_scanoutpos(dev, pipe, max_error, vblank_time, flags, - crtc, &crtc->hwmode); }
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index cc6c228..425515f 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -103,6 +103,7 @@ nouveau_display_scanoutpos_head(struct drm_crtc *crtc, int *vpos, int *hpos, .base.head = nouveau_crtc(crtc)->index, }; struct nouveau_display *disp = nouveau_display(crtc->dev); + struct drm_vblank_crtc *vblank = &crtc->dev->vblank[drm_crtc_index(crtc)]; int ret, retry = 1;
do { @@ -116,7 +117,7 @@ nouveau_display_scanoutpos_head(struct drm_crtc *crtc, int *vpos, int *hpos, break; }
- if (retry) ndelay(crtc->linedur_ns); + if (retry) ndelay(vblank->linedur_ns); } while (retry--);
*hpos = args.scan.hline; @@ -155,7 +156,7 @@ nouveau_display_vblstamp(struct drm_device *dev, int head, int *max_error, list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { if (nouveau_crtc(crtc)->index == head) { return drm_calc_vbltimestamp_from_scanoutpos(dev, - head, max_error, time, flags, crtc, + head, max_error, time, flags, &crtc->hwmode); } } diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c index 4a119c2..fd9da28 100644 --- a/drivers/gpu/drm/radeon/radeon_kms.c +++ b/drivers/gpu/drm/radeon/radeon_kms.c @@ -841,7 +841,7 @@ int radeon_get_vblank_timestamp_kms(struct drm_device *dev, int crtc, /* Helper routine in DRM core does all the work: */ return drm_calc_vbltimestamp_from_scanoutpos(dev, crtc, max_error, vblank_time, flags, - drmcrtc, &drmcrtc->hwmode); + &drmcrtc->hwmode); }
#define KMS_INVALID_IOCTL(name) \ diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 8b5ce7c..2998867 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -701,6 +701,9 @@ struct drm_vblank_crtc { u32 last_wait; /* Last vblank seqno waited per CRTC */ unsigned int inmodeset; /* Display driver is setting mode */ unsigned int pipe; /* crtc index */ + int framedur_ns; /* frame/field duration in ns */ + int linedur_ns; /* line duration in ns */ + int pixeldur_ns; /* pixel duration in ns */ bool enabled; /* so we don't call enable more than once per disable */ }; @@ -951,7 +954,6 @@ extern int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, unsigned int pipe, int *max_error, struct timeval *vblank_time, unsigned flags, - const struct drm_crtc *refcrtc, const struct drm_display_mode *mode); extern void drm_calc_timestamping_constants(struct drm_crtc *crtc, const struct drm_display_mode *mode); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c0366e9..2294061 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -413,9 +413,6 @@ struct drm_crtc_funcs { * @funcs: CRTC control functions * @gamma_size: size of gamma ramp * @gamma_store: gamma ramp values - * @framedur_ns: precise frame timing - * @linedur_ns: precise line timing - * @pixeldur_ns: precise pixel timing * @helper_private: mid-layer private data * @properties: property tracking for this CRTC * @state: current atomic state for this CRTC @@ -468,9 +465,6 @@ struct drm_crtc { uint32_t gamma_size; uint16_t *gamma_store;
- /* Constants needed for precise vblank and swap timestamping. */ - int framedur_ns, linedur_ns, pixeldur_ns; - /* if you are using the helper */ const void *helper_private;
Op 14-09-15 om 21:43 schreef ville.syrjala@linux.intel.com:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Collect the timestamping constants alongside the rest of the relevant stuff under drm_vblank_crtc.
We can now get rid of the 'refcrtc' parameter to drm_calc_vbltimestamp_from_scanoutpos().
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
That's a much better place. :-)
I reviewed the whole series and didn't find anything obvious wrong, but didn't compile or runtime test it.
For the whole series: Acked-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com
From: Ville Syrjälä ville.syrjala@linux.intel.com
linedur_ns, and especially pixeldur_ns are becoming rather inaccurate to be used for the vblank timestamp correction. With 4k@60 the pixel duration is already below 2ns, so the amount of error due to the truncation to nanoseconds is introducing quite a bit of error.
We can avoid such problems if we instead calculate the timestamp delta_ns directly from the dislay timings, avoiding the use of these intermediate truncated values.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 19 ++++++++----------- drivers/gpu/drm/i915/i915_irq.c | 4 ++-- drivers/gpu/drm/nouveau/nouveau_display.c | 3 ++- drivers/gpu/drm/nouveau/nouveau_display.h | 3 ++- drivers/gpu/drm/radeon/radeon_display.c | 14 ++++++++------ drivers/gpu/drm/radeon/radeon_drv.c | 5 +++-- drivers/gpu/drm/radeon/radeon_mode.h | 5 +++-- drivers/gpu/drm/radeon/radeon_pm.c | 4 +++- include/drm/drmP.h | 6 ++++-- 9 files changed, 35 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 6b2fefd..9fab333 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -694,12 +694,11 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, unsigned flags, const struct drm_display_mode *mode) { - struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; struct timeval tv_etime; ktime_t stime, etime; int vbl_status; int vpos, hpos, i; - int framedur_ns, linedur_ns, pixeldur_ns, delta_ns, duration_ns; + int delta_ns, duration_ns; bool invbl;
if (pipe >= dev->num_crtcs) { @@ -713,15 +712,10 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, return -EIO; }
- /* Durations of frames, lines, pixels in nanoseconds. */ - framedur_ns = vblank->framedur_ns; - linedur_ns = vblank->linedur_ns; - pixeldur_ns = vblank->pixeldur_ns; - /* If mode timing undefined, just return as no-op: * Happens during initial modesetting of a crtc. */ - if (framedur_ns == 0) { + if (mode->crtc_clock == 0) { DRM_DEBUG("crtc %u: Noop due to uninitialized mode.\n", pipe); return -EAGAIN; } @@ -738,8 +732,10 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, * Get vertical and horizontal scanout position vpos, hpos, * and bounding timestamps stime, etime, pre/post query. */ - vbl_status = dev->driver->get_scanout_position(dev, pipe, flags, &vpos, - &hpos, &stime, &etime); + vbl_status = dev->driver->get_scanout_position(dev, pipe, flags, + &vpos, &hpos, + &stime, &etime, + mode);
/* Return as no-op if scanout query unsupported or failed. */ if (!(vbl_status & DRM_SCANOUTPOS_VALID)) { @@ -776,7 +772,8 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, * since start of scanout at first display scanline. delta_ns * can be negative if start of scanout hasn't happened yet. */ - delta_ns = vpos * linedur_ns + hpos * pixeldur_ns; + delta_ns = div_s64(1000000LL * (vpos * mode->crtc_htotal + hpos), + mode->crtc_clock);
if (!drm_timestamp_monotonic) etime = ktime_mono_to_real(etime); diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 4aee725..77e8718 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -705,12 +705,12 @@ static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
static int i915_get_crtc_scanoutpos(struct drm_device *dev, int pipe, unsigned int flags, int *vpos, int *hpos, - ktime_t *stime, ktime_t *etime) + ktime_t *stime, ktime_t *etime, + const struct drm_display_mode *mode) { struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - const struct drm_display_mode *mode = &intel_crtc->base.hwmode; int position; int vbl_start, vbl_end, hsync_start, htotal, vtotal; bool in_vbl = true; diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index 425515f..a82c3cb 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -133,7 +133,8 @@ nouveau_display_scanoutpos_head(struct drm_crtc *crtc, int *vpos, int *hpos,
int nouveau_display_scanoutpos(struct drm_device *dev, int head, unsigned int flags, - int *vpos, int *hpos, ktime_t *stime, ktime_t *etime) + int *vpos, int *hpos, ktime_t *stime, ktime_t *etime, + const struct drm_display_mode *mode) { struct drm_crtc *crtc;
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h index a6213e2..4182d21 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.h +++ b/drivers/gpu/drm/nouveau/nouveau_display.h @@ -68,7 +68,8 @@ void nouveau_display_resume(struct drm_device *dev, bool runtime); int nouveau_display_vblank_enable(struct drm_device *, int); void nouveau_display_vblank_disable(struct drm_device *, int); int nouveau_display_scanoutpos(struct drm_device *, int, unsigned int, - int *, int *, ktime_t *, ktime_t *); + int *, int *, ktime_t *, ktime_t *, + const struct drm_display_mode *); int nouveau_display_vblstamp(struct drm_device *, int, int *, struct timeval *, unsigned);
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c index d2e9e9e..0503af7 100644 --- a/drivers/gpu/drm/radeon/radeon_display.c +++ b/drivers/gpu/drm/radeon/radeon_display.c @@ -323,7 +323,8 @@ void radeon_crtc_handle_vblank(struct radeon_device *rdev, int crtc_id) */ if (update_pending && (DRM_SCANOUTPOS_VALID & radeon_get_crtc_scanoutpos(rdev->ddev, crtc_id, 0, - &vpos, &hpos, NULL, NULL)) && + &vpos, &hpos, NULL, NULL, + &rdev->mode_info.crtcs[crtc_id]->base.hwmode)) && ((vpos >= (99 * rdev->mode_info.crtcs[crtc_id]->base.hwmode.crtc_vdisplay)/100) || (vpos < 0 && !ASIC_IS_AVIVO(rdev)))) { /* crtc didn't flip in this target vblank interval, @@ -1799,7 +1800,8 @@ bool radeon_crtc_scaling_mode_fixup(struct drm_crtc *crtc, * */ int radeon_get_crtc_scanoutpos(struct drm_device *dev, int crtc, unsigned int flags, - int *vpos, int *hpos, ktime_t *stime, ktime_t *etime) + int *vpos, int *hpos, ktime_t *stime, ktime_t *etime, + const struct drm_display_mode *mode) { u32 stat_crtc = 0, vbl = 0, position = 0; int vbl_start, vbl_end, vtotal, ret = 0; @@ -1914,7 +1916,7 @@ int radeon_get_crtc_scanoutpos(struct drm_device *dev, int crtc, unsigned int fl } else { /* No: Fake something reasonable which gives at least ok results. */ - vbl_start = rdev->mode_info.crtcs[crtc]->base.hwmode.crtc_vdisplay; + vbl_start = mode->crtc_vdisplay; vbl_end = 0; }
@@ -1930,7 +1932,7 @@ int radeon_get_crtc_scanoutpos(struct drm_device *dev, int crtc, unsigned int fl
/* Inside "upper part" of vblank area? Apply corrective offset if so: */ if (in_vbl && (*vpos >= vbl_start)) { - vtotal = rdev->mode_info.crtcs[crtc]->base.hwmode.crtc_vtotal; + vtotal = mode->crtc_vtotal; *vpos = *vpos - vtotal; }
@@ -1952,8 +1954,8 @@ int radeon_get_crtc_scanoutpos(struct drm_device *dev, int crtc, unsigned int fl * We only do this if DRM_CALLED_FROM_VBLIRQ. */ if ((flags & DRM_CALLED_FROM_VBLIRQ) && !in_vbl) { - vbl_start = rdev->mode_info.crtcs[crtc]->base.hwmode.crtc_vdisplay; - vtotal = rdev->mode_info.crtcs[crtc]->base.hwmode.crtc_vtotal; + vbl_start = mode->crtc_vdisplay; + vtotal = mode->crtc_vtotal;
if (vbl_start - *vpos < vtotal / 100) { *vpos -= vtotal; diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c index 5751446..e30c1d7 100644 --- a/drivers/gpu/drm/radeon/radeon_drv.c +++ b/drivers/gpu/drm/radeon/radeon_drv.c @@ -126,8 +126,9 @@ struct dma_buf *radeon_gem_prime_export(struct drm_device *dev, int flags); extern int radeon_get_crtc_scanoutpos(struct drm_device *dev, int crtc, unsigned int flags, - int *vpos, int *hpos, ktime_t *stime, - ktime_t *etime); + int *vpos, int *hpos, + ktime_t *stime, ktime_t *etime, + const struct drm_display_mode *mode); extern bool radeon_is_px(struct drm_device *dev); extern const struct drm_ioctl_desc radeon_ioctls_kms[]; extern int radeon_max_kms_ioctl; diff --git a/drivers/gpu/drm/radeon/radeon_mode.h b/drivers/gpu/drm/radeon/radeon_mode.h index aecc3e3..2317d04 100644 --- a/drivers/gpu/drm/radeon/radeon_mode.h +++ b/drivers/gpu/drm/radeon/radeon_mode.h @@ -876,8 +876,9 @@ extern void radeon_cursor_reset(struct drm_crtc *crtc);
extern int radeon_get_crtc_scanoutpos(struct drm_device *dev, int crtc, unsigned int flags, - int *vpos, int *hpos, ktime_t *stime, - ktime_t *etime); + int *vpos, int *hpos, + ktime_t *stime, ktime_t *etime, + const struct drm_display_mode *mode);
extern bool radeon_combios_check_hardcoded_edid(struct radeon_device *rdev); extern struct edid * diff --git a/drivers/gpu/drm/radeon/radeon_pm.c b/drivers/gpu/drm/radeon/radeon_pm.c index 05751f3..10f4c12 100644 --- a/drivers/gpu/drm/radeon/radeon_pm.c +++ b/drivers/gpu/drm/radeon/radeon_pm.c @@ -1733,7 +1733,9 @@ static bool radeon_pm_in_vbl(struct radeon_device *rdev) */ for (crtc = 0; (crtc < rdev->num_crtc) && in_vbl; crtc++) { if (rdev->pm.active_crtcs & (1 << crtc)) { - vbl_status = radeon_get_crtc_scanoutpos(rdev->ddev, crtc, 0, &vpos, &hpos, NULL, NULL); + vbl_status = radeon_get_crtc_scanoutpos(rdev->ddev, crtc, 0, + &vpos, &hpos, NULL, NULL, + &rdev->mode_info.crtcs[crtc]->base.hwmode); if ((vbl_status & DRM_SCANOUTPOS_VALID) && !(vbl_status & DRM_SCANOUTPOS_IN_VBLANK)) in_vbl = false; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 2998867..b2a95e7 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -482,6 +482,7 @@ struct drm_driver { * scanout position query. Can be NULL to skip timestamp. * \param *etime Target location for timestamp taken immediately after * scanout position query. Can be NULL to skip timestamp. + * \param mode Current display timings. * * Returns vpos as a positive number while in active scanout area. * Returns vpos as a negative number inside vblank, counting the number @@ -499,8 +500,9 @@ struct drm_driver { */ int (*get_scanout_position) (struct drm_device *dev, int crtc, unsigned int flags, - int *vpos, int *hpos, ktime_t *stime, - ktime_t *etime); + int *vpos, int *hpos, + ktime_t *stime, ktime_t *etime, + const struct drm_display_mode *mode);
/** * Called by \c drm_get_last_vbltimestamp. Should return a precise
From: Ville Syrjälä ville.syrjala@linux.intel.com
pixeldur_ns is now unsued, so kill it from drm_vblank_crtc. framedur_ns is also currently unused but we will have use for it in the near future so leave it be. linedur_ns is still used by nouveau for some internal delays.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 13 +++++-------- include/drm/drmP.h | 1 - 2 files changed, 5 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 9fab333..ac17602 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -604,7 +604,7 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc, const struct drm_display_mode *mode) { struct drm_vblank_crtc *vblank = &crtc->dev->vblank[drm_crtc_index(crtc)]; - int linedur_ns = 0, pixeldur_ns = 0, framedur_ns = 0; + int linedur_ns = 0, framedur_ns = 0; int dotclock = mode->crtc_clock;
/* Valid dotclock? */ @@ -613,10 +613,9 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
/* * Convert scanline length in pixels and video - * dot clock to line duration, frame duration - * and pixel duration in nanoseconds: + * dot clock to line duration and frame duration + * in nanoseconds: */ - pixeldur_ns = 1000000 / dotclock; linedur_ns = div_u64((u64) mode->crtc_htotal * 1000000, dotclock); framedur_ns = div_u64((u64) frame_size * 1000000, dotclock);
@@ -629,16 +628,14 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc, DRM_ERROR("crtc %u: Can't calculate constants, dotclock = 0!\n", crtc->base.id);
- vblank->pixeldur_ns = pixeldur_ns; vblank->linedur_ns = linedur_ns; vblank->framedur_ns = framedur_ns;
DRM_DEBUG("crtc %u: hwmode: htotal %d, vtotal %d, vdisplay %d\n", crtc->base.id, mode->crtc_htotal, mode->crtc_vtotal, mode->crtc_vdisplay); - DRM_DEBUG("crtc %u: clock %d kHz framedur %d linedur %d, pixeldur %d\n", - crtc->base.id, dotclock, framedur_ns, - linedur_ns, pixeldur_ns); + DRM_DEBUG("crtc %u: clock %d kHz framedur %d linedur %d\n", + crtc->base.id, dotclock, framedur_ns, linedur_ns); } EXPORT_SYMBOL(drm_calc_timestamping_constants);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index b2a95e7..6717a7d 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -705,7 +705,6 @@ struct drm_vblank_crtc { unsigned int pipe; /* crtc index */ int framedur_ns; /* frame/field duration in ns */ int linedur_ns; /* line duration in ns */ - int pixeldur_ns; /* pixel duration in ns */ bool enabled; /* so we don't call enable more than once per disable */ };
From: Ville Syrjälä ville.syrjala@linux.intel.com
The vblank counts are u32 so make flip_queued_vblank and flip_ready_vblank u32 as well.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/intel_drv.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 02a755a..0475f89 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -862,8 +862,8 @@ struct intel_unpin_work { u32 flip_count; u32 gtt_offset; struct drm_i915_gem_request *flip_queued_req; - int flip_queued_vblank; - int flip_ready_vblank; + u32 flip_queued_vblank; + u32 flip_ready_vblank; bool enable_stall_check; };
From: Ville Syrjälä ville.syrjala@linux.intel.com
We'll soon have use for the 'flags' in drm_update_vblank_count() so pass it in.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index ac17602..0353224 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -120,7 +120,8 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe, * Note: caller must hold dev->vbl_lock since this reads & writes * device vblank fields. */ -static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe) +static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, + unsigned long flags) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; u32 cur_vblank, diff; @@ -141,7 +142,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe) */ do { cur_vblank = dev->driver->get_vblank_counter(dev, pipe); - rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, 0); + rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, flags); } while (cur_vblank != dev->driver->get_vblank_counter(dev, pipe));
/* Deal with counter wrap */ @@ -207,7 +208,7 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe) */ if (!vblank->enabled && drm_get_last_vbltimestamp(dev, pipe, &tvblank, 0)) { - drm_update_vblank_count(dev, pipe); + drm_update_vblank_count(dev, pipe, 0); spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags); return; } @@ -1027,7 +1028,7 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe) atomic_dec(&vblank->refcount); else { vblank->enabled = true; - drm_update_vblank_count(dev, pipe); + drm_update_vblank_count(dev, pipe, 0); } }
From: Ville Syrjälä ville.syrjala@linux.intel.com
Pontential infinite loops in the vblank code are a bad idea. Add some limits.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 0353224..93fe582 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -127,6 +127,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, u32 cur_vblank, diff; bool rc; struct timeval t_vblank; + int count = DRM_TIMESTAMP_MAXRETRIES;
/* * Interrupts were disabled prior to this call, so deal with counter @@ -143,7 +144,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, do { cur_vblank = dev->driver->get_vblank_counter(dev, pipe); rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, flags); - } while (cur_vblank != dev->driver->get_vblank_counter(dev, pipe)); + } while (cur_vblank != dev->driver->get_vblank_counter(dev, pipe) && --count > 0);
/* Deal with counter wrap */ diff = cur_vblank - vblank->last; @@ -914,6 +915,7 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe, struct timeval *vblanktime) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; + int count = DRM_TIMESTAMP_MAXRETRIES; u32 cur_vblank;
if (WARN_ON(pipe >= dev->num_crtcs)) @@ -929,7 +931,7 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe, smp_rmb(); *vblanktime = vblanktimestamp(dev, pipe, cur_vblank); smp_rmb(); - } while (cur_vblank != vblank->count); + } while (cur_vblank != vblank->count && --count > 0);
return cur_vblank; }
From: Ville Syrjälä ville.syrjala@linux.intel.com
Avoid confusion and don't use 'vbl_status' as both the .get_scanout_position() return value and the return value from drm_calc_vbltimestamp_from_scanoutpos().
While at it make 'vbl_status' unsigned and print it as hex in the debug prints since it's a bitmask.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 93fe582..aad4f1d 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -695,10 +695,10 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, { struct timeval tv_etime; ktime_t stime, etime; - int vbl_status; + unsigned int vbl_status; + int ret = DRM_VBLANKTIME_SCANOUTPOS_METHOD; int vpos, hpos, i; int delta_ns, duration_ns; - bool invbl;
if (pipe >= dev->num_crtcs) { DRM_ERROR("Invalid crtc %u\n", pipe); @@ -738,7 +738,7 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev,
/* Return as no-op if scanout query unsupported or failed. */ if (!(vbl_status & DRM_SCANOUTPOS_VALID)) { - DRM_DEBUG("crtc %u : scanoutpos query failed [%d].\n", + DRM_DEBUG("crtc %u : scanoutpos query failed [0x%x].\n", pipe, vbl_status); return -EIO; } @@ -765,7 +765,8 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, * within vblank area, counting down the number of lines until * start of scanout. */ - invbl = vbl_status & DRM_SCANOUTPOS_IN_VBLANK; + if (vbl_status & DRM_SCANOUTPOS_IN_VBLANK) + ret |= DRM_VBLANKTIME_IN_VBLANK;
/* Convert scanout position into elapsed time at raw_time query * since start of scanout at first display scanline. delta_ns @@ -788,17 +789,13 @@ int drm_calc_vbltimestamp_from_scanoutpos(struct drm_device *dev, etime = ktime_sub_ns(etime, delta_ns); *vblank_time = ktime_to_timeval(etime);
- DRM_DEBUG("crtc %u : v %d p(%d,%d)@ %ld.%ld -> %ld.%ld [e %d us, %d rep]\n", - pipe, (int)vbl_status, hpos, vpos, + DRM_DEBUG("crtc %u : v 0x%x p(%d,%d)@ %ld.%ld -> %ld.%ld [e %d us, %d rep]\n", + pipe, vbl_status, hpos, vpos, (long)tv_etime.tv_sec, (long)tv_etime.tv_usec, (long)vblank_time->tv_sec, (long)vblank_time->tv_usec, duration_ns/1000, i);
- vbl_status = DRM_VBLANKTIME_SCANOUTPOS_METHOD; - if (invbl) - vbl_status |= DRM_VBLANKTIME_IN_VBLANK; - - return vbl_status; + return ret; } EXPORT_SYMBOL(drm_calc_vbltimestamp_from_scanoutpos);
From: Ville Syrjälä ville.syrjala@linux.intel.com
Remove the NULL 't_vblank' checks from store_vblank() since that will never happen.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index aad4f1d..07b0cb1 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -83,13 +83,11 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe,
assert_spin_locked(&dev->vblank_time_lock);
- if (t_vblank) { - /* All writers hold the spinlock, but readers are serialized by - * the latching of vblank->count below. - */ - tslot = vblank->count + vblank_count_inc; - vblanktimestamp(dev, pipe, tslot) = *t_vblank; - } + /* All writers hold the spinlock, but readers are serialized by + * the latching of vblank->count below. + */ + tslot = vblank->count + vblank_count_inc; + vblanktimestamp(dev, pipe, tslot) = *t_vblank;
/* * vblank timestamp updates are protected on the write side with
From: Ville Syrjälä ville.syrjala@linux.intel.com
When lacking am accurate hardware frame counter, we can fall back to using the vblank timestamps to guesstimagte how many vblanks have elapsed since the last time the vblank counter was updated.
Take the oppostunity to unify the vblank_disable_and_save() and drm_handle_vblank_events() to call the same function (drm_update_vblank_count()) to perform the vblank updates.
If the hardware/driver has an accurate frame counter use it instead of the timestamp based guesstimate. If the hardware/driver has neither a frame counter nor acurate vblank timestamps, we fall back to assuming that each drm_handle_vblank_events() should increment the vblank count by one.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 206 ++++++++++++++++++++-------------------------- 1 file changed, 91 insertions(+), 115 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 07b0cb1..88fbee4 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -76,13 +76,15 @@ module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
static void store_vblank(struct drm_device *dev, unsigned int pipe, u32 vblank_count_inc, - struct timeval *t_vblank) + struct timeval *t_vblank, u32 last) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; u32 tslot;
assert_spin_locked(&dev->vblank_time_lock);
+ vblank->last = last; + /* All writers hold the spinlock, but readers are serialized by * the latching of vblank->count below. */ @@ -103,6 +105,54 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe, }
/** + * drm_reset_vblank_timestamp - reset the last timestamp to the last vblank + * @dev: DRM device + * @pipe: index of CRTC for which to reset the timestamp + * + * Reset the stored timestamp for the current vblank count to correspond + * to the last vblank occurred. + * + * Only to be called from drm_vblank_on(). + * + * Note: caller must hold dev->vbl_lock since this reads & writes + * device vblank fields. + */ +static void drm_reset_vblank_timestamp(struct drm_device *dev, unsigned int pipe) +{ + u32 cur_vblank; + bool rc; + struct timeval t_vblank; + int count = DRM_TIMESTAMP_MAXRETRIES; + + spin_lock(&dev->vblank_time_lock); + + /* + * sample the current counter to avoid random jumps + * when drm_vblank_enable() applies the diff + */ + do { + cur_vblank = dev->driver->get_vblank_counter(dev, pipe); + rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, 0); + } while (cur_vblank != dev->driver->get_vblank_counter(dev, pipe) && --count > 0); + + /* + * Only reinitialize corresponding vblank timestamp if high-precision query + * available and didn't fail. Otherwise reinitialize delayed at next vblank + * interrupt and assign 0 for now, to mark the vblanktimestamp as invalid. + */ + if (!rc) + t_vblank = (struct timeval) {0, 0}; + + /* + * +1 to make sure user will never see the same + * vblank counter value before and after a modeset + */ + store_vblank(dev, pipe, 1, &t_vblank, cur_vblank); + + spin_unlock(&dev->vblank_time_lock); +} + +/** * drm_update_vblank_count - update the master vblank counter * @dev: DRM device * @pipe: counter to update @@ -126,6 +176,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, bool rc; struct timeval t_vblank; int count = DRM_TIMESTAMP_MAXRETRIES; + int framedur_ns = vblank->framedur_ns;
/* * Interrupts were disabled prior to this call, so deal with counter @@ -144,20 +195,40 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, flags); } while (cur_vblank != dev->driver->get_vblank_counter(dev, pipe) && --count > 0);
- /* Deal with counter wrap */ - diff = cur_vblank - vblank->last; - if (cur_vblank < vblank->last) { - diff += dev->max_vblank_count + 1; + if (dev->max_vblank_count != 0) { + /* trust the hw counter when it's around */ + diff = (cur_vblank - vblank->last) & dev->max_vblank_count; + } else if (rc && framedur_ns) { + const struct timeval *t_old; + u64 diff_ns; + + t_old = &vblanktimestamp(dev, pipe, vblank->count); + diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old);
- DRM_DEBUG("last_vblank[%u]=0x%x, cur_vblank=0x%x => diff=0x%x\n", - pipe, vblank->last, cur_vblank, diff); + /* + * Figure out how many vblanks we've missed based + * on the difference in the timestamps and the + * frame/field duration. + */ + diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns); + + if (diff == 0 && flags & DRM_CALLED_FROM_VBLIRQ) + DRM_DEBUG("crtc %u: Redundant vblirq ignored." + " diff_ns = %lld, framedur_ns = %d)\n", + pipe, (long long) diff_ns, framedur_ns); + } else { + /* some kind of default for drivers w/o accurate vbl timestamping */ + diff = (flags & DRM_CALLED_FROM_VBLIRQ) != 0; }
- DRM_DEBUG("updating vblank count on crtc %u, missed %d\n", - pipe, diff); + DRM_DEBUG("updating vblank count on crtc %u:" + " current=%u, diff=%u, hw=%u hw_last=%u\n", + pipe, vblank->count, diff, cur_vblank, vblank->last);
- if (diff == 0) + if (diff == 0) { + WARN_ON_ONCE(cur_vblank != vblank->last); return; + }
/* * Only reinitialize corresponding vblank timestamp if high-precision query @@ -167,7 +238,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, if (!rc) t_vblank = (struct timeval) {0, 0};
- store_vblank(dev, pipe, diff, &t_vblank); + store_vblank(dev, pipe, diff, &t_vblank, cur_vblank); }
/* @@ -180,11 +251,6 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; unsigned long irqflags; - u32 vblcount; - s64 diff_ns; - bool vblrc; - struct timeval tvblank; - int count = DRM_TIMESTAMP_MAXRETRIES;
/* Prevent vblank irq processing while disabling vblank irqs, * so no updates of timestamps or count can happen after we've @@ -193,26 +259,6 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe) spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
/* - * If the vblank interrupt was already disabled update the count - * and timestamp to maintain the appearance that the counter - * has been ticking all along until this time. This makes the - * count account for the entire time between drm_vblank_on() and - * drm_vblank_off(). - * - * But only do this if precise vblank timestamps are available. - * Otherwise we might read a totally bogus timestamp since drivers - * lacking precise timestamp support rely upon sampling the system clock - * at vblank interrupt time. Which obviously won't work out well if the - * vblank interrupt is disabled. - */ - if (!vblank->enabled && - drm_get_last_vbltimestamp(dev, pipe, &tvblank, 0)) { - drm_update_vblank_count(dev, pipe, 0); - spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags); - return; - } - - /* * Only disable vblank interrupts if they're enabled. This avoids * calling the ->disable_vblank() operation in atomic context with the * hardware potentially runtime suspended. @@ -222,47 +268,13 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe) vblank->enabled = false; }
- /* No further vblank irq's will be processed after - * this point. Get current hardware vblank count and - * vblank timestamp, repeat until they are consistent. - * - * FIXME: There is still a race condition here and in - * drm_update_vblank_count() which can cause off-by-one - * reinitialization of software vblank counter. If gpu - * vblank counter doesn't increment exactly at the leading - * edge of a vblank interval, then we can lose 1 count if - * we happen to execute between start of vblank and the - * delayed gpu counter increment. - */ - do { - vblank->last = dev->driver->get_vblank_counter(dev, pipe); - vblrc = drm_get_last_vbltimestamp(dev, pipe, &tvblank, 0); - } while (vblank->last != dev->driver->get_vblank_counter(dev, pipe) && (--count) && vblrc); - - if (!count) - vblrc = 0; - - /* Compute time difference to stored timestamp of last vblank - * as updated by last invocation of drm_handle_vblank() in vblank irq. - */ - vblcount = vblank->count; - diff_ns = timeval_to_ns(&tvblank) - - timeval_to_ns(&vblanktimestamp(dev, pipe, vblcount)); - - /* If there is at least 1 msec difference between the last stored - * timestamp and tvblank, then we are currently executing our - * disable inside a new vblank interval, the tvblank timestamp - * corresponds to this new vblank interval and the irq handler - * for this vblank didn't run yet and won't run due to our disable. - * Therefore we need to do the job of drm_handle_vblank() and - * increment the vblank counter by one to account for this vblank. - * - * Skip this step if there isn't any high precision timestamp - * available. In that case we can't account for this and just - * hope for the best. + /* + * Always update the count and timestamp to maintain the + * appearance that the counter has been ticking all along until + * this time. This makes the count account for the entire time + * between drm_vblank_on() and drm_vblank_off(). */ - if (vblrc && (abs64(diff_ns) > 1000000)) - store_vblank(dev, pipe, 1, &tvblank); + drm_update_vblank_count(dev, pipe, 0);
spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags); } @@ -1325,16 +1337,8 @@ void drm_vblank_on(struct drm_device *dev, unsigned int pipe) vblank->inmodeset = 0; }
- /* - * sample the current counter to avoid random jumps - * when drm_vblank_enable() applies the diff - * - * -1 to make sure user will never see the same - * vblank counter value before and after a modeset - */ - vblank->last = - (dev->driver->get_vblank_counter(dev, pipe) - 1) & - dev->max_vblank_count; + drm_reset_vblank_timestamp(dev, pipe); + /* * re-enable interrupts if there are users left, or the * user wishes vblank interrupts to be enabled all the time. @@ -1717,9 +1721,6 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe) bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; - u32 vblcount; - s64 diff_ns; - struct timeval tvblank; unsigned long irqflags;
if (WARN_ON_ONCE(!dev->num_crtcs)) @@ -1743,32 +1744,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe) return false; }
- /* Fetch corresponding timestamp for this vblank interval from - * driver and store it in proper slot of timestamp ringbuffer. - */ - - /* Get current timestamp and count. */ - vblcount = vblank->count; - drm_get_last_vbltimestamp(dev, pipe, &tvblank, DRM_CALLED_FROM_VBLIRQ); - - /* Compute time difference to timestamp of last vblank */ - diff_ns = timeval_to_ns(&tvblank) - - timeval_to_ns(&vblanktimestamp(dev, pipe, vblcount)); - - /* Update vblank timestamp and count if at least - * DRM_REDUNDANT_VBLIRQ_THRESH_NS nanoseconds - * difference between last stored timestamp and current - * timestamp. A smaller difference means basically - * identical timestamps. Happens if this vblank has - * been already processed and this is a redundant call, - * e.g., due to spurious vblank interrupts. We need to - * ignore those for accounting. - */ - if (abs64(diff_ns) > DRM_REDUNDANT_VBLIRQ_THRESH_NS) - store_vblank(dev, pipe, 1, &tvblank); - else - DRM_DEBUG("crtc %u: Redundant vblirq ignored. diff_ns = %d\n", - pipe, (int) diff_ns); + drm_update_vblank_count(dev, pipe, DRM_CALLED_FROM_VBLIRQ);
spin_unlock(&dev->vblank_time_lock);
On 15.09.2015 04:43, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
When lacking am accurate hardware frame counter, we can fall back to using the vblank timestamps to guesstimagte how many vblanks have elapsed since the last time the vblank counter was updated.
Take the oppostunity to unify the vblank_disable_and_save() and drm_handle_vblank_events() to call the same function (drm_update_vblank_count()) to perform the vblank updates.
It would be nice to keep the drm_update_vblank_count unification separate. As it is, it's very hard to keep track of which parts of the patch are for each logical change.
BTW, I think the fact that I was hitting the problem fixed by 209e4dbc ("drm/vblank: Use u32 consistently for vblank counters") within a few days indicates that there's another bug which causes the counter to jump forward with drm_vblank_on/off(). It may not manifest itself with current Intel hardware because that has a full 32-bit hardware frame counter, turning the related calculations into no-ops. I haven't had time to investigate this further yet.
On Mon, Sep 14, 2015 at 10:43:51PM +0300, ville.syrjala@linux.intel.com wrote: [...]
@@ -167,7 +238,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, if (!rc) t_vblank = (struct timeval) {0, 0};
- store_vblank(dev, pipe, diff, &t_vblank);
- store_vblank(dev, pipe, diff, &t_vblank, cur_vblank);
I think clearing the t_vblank is now wrong here. This breaks weston on Tegra (and I think all other drivers that don't implement hardware VBLANK timestamps). The reason is that if ->get_vblank_timestamp() isn't implemented, rc will always be false at this point, hence resulting in a zero VBLANK timestamp being reported to userspace.
In fact the comment, reproduced here because it's not in the patch context:
/* * Only reinitialize corresponding vblank timestamp if high-precision query * available and didn't fail. Otherwise reinitialize delayed at next vblank * interrupt and assign 0 for now, to mark the vblanktimestamp as invalid. */
is no longer accurate because this function is now called at the VBLANK interrupt. If you want to keep invalidating timestamps for drivers that implement VBLANK timestamp queries I think the condition needs to be changed to something like:
if (dev->get_vblank_timestamp && !rc)
and the comment updated to reflect the truth. Alternatively this code can be removed, though I guess having the monotonic timestamp fallback would be kind of missing the point.
I've tested either change and they both restore weston on Tegra.
Thierry
From: Ville Syrjälä ville.syrjala@linux.intel.com
The vblank timestamp ringbuffer only has two entries, so if the vblank->count is incremented by an even number readers may end up seeing the new vblank timestamp alongside the old vblank counter value.
Fix the problem by storing the vblank counter in a ringbuffer as well, and always increment the ringbuffer "slot" by one when storing a new timestamp+counter pair.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 40 ++++++++++++++++++++++++---------------- include/drm/drmP.h | 12 ++++++------ 2 files changed, 30 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 88fbee4..8de236a 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -43,8 +43,10 @@ #include <linux/export.h>
/* Access macro for slots in vblank timestamp ringbuffer. */ -#define vblanktimestamp(dev, pipe, count) \ - ((dev)->vblank[pipe].time[(count) % DRM_VBLANKTIME_RBSIZE]) +#define vblanktimestamp(dev, pipe, slot) \ + ((dev)->vblank[pipe].time[(slot) % DRM_VBLANK_RBSIZE]) +#define vblankcount(dev, pipe, slot) \ + ((dev)->vblank[pipe].count[(slot) % DRM_VBLANK_RBSIZE])
/* Retry timestamp calculation up to 3 times to satisfy * drm_timestamp_precision before giving up. @@ -76,20 +78,23 @@ module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
static void store_vblank(struct drm_device *dev, unsigned int pipe, u32 vblank_count_inc, - struct timeval *t_vblank, u32 last) + const struct timeval *t_vblank, u32 last) { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; - u32 tslot; + u32 slot;
assert_spin_locked(&dev->vblank_time_lock);
+ slot = vblank->slot + 1; + vblank->last = last;
/* All writers hold the spinlock, but readers are serialized by - * the latching of vblank->count below. + * the latching of vblank->slot below. */ - tslot = vblank->count + vblank_count_inc; - vblanktimestamp(dev, pipe, tslot) = *t_vblank; + vblankcount(dev, pipe, slot) = + vblankcount(dev, pipe, vblank->slot) + vblank_count_inc; + vblanktimestamp(dev, pipe, slot) = *t_vblank;
/* * vblank timestamp updates are protected on the write side with @@ -100,7 +105,7 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe, * The read-side barriers for this are in drm_vblank_count_and_time. */ smp_wmb(); - vblank->count += vblank_count_inc; + vblank->slot = slot; smp_wmb(); }
@@ -202,7 +207,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, const struct timeval *t_old; u64 diff_ns;
- t_old = &vblanktimestamp(dev, pipe, vblank->count); + t_old = &vblanktimestamp(dev, pipe, vblank->slot); diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old);
/* @@ -223,7 +228,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
DRM_DEBUG("updating vblank count on crtc %u:" " current=%u, diff=%u, hw=%u hw_last=%u\n", - pipe, vblank->count, diff, cur_vblank, vblank->last); + pipe, vblankcount(dev, pipe, vblank->slot), + diff, cur_vblank, vblank->last);
if (diff == 0) { WARN_ON_ONCE(cur_vblank != vblank->last); @@ -883,7 +889,7 @@ u32 drm_vblank_count(struct drm_device *dev, int pipe) if (WARN_ON(pipe >= dev->num_crtcs)) return 0;
- return vblank->count; + return vblankcount(dev, pipe, vblank->slot); } EXPORT_SYMBOL(drm_vblank_count);
@@ -923,7 +929,8 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe, { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; int count = DRM_TIMESTAMP_MAXRETRIES; - u32 cur_vblank; + u32 vblankcount; + u32 slot;
if (WARN_ON(pipe >= dev->num_crtcs)) return 0; @@ -934,13 +941,14 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe, * This works like a seqlock. The write-side barriers are in store_vblank. */ do { - cur_vblank = vblank->count; + slot = vblank->slot; smp_rmb(); - *vblanktime = vblanktimestamp(dev, pipe, cur_vblank); + *vblanktime = vblanktimestamp(dev, pipe, slot); + vblankcount = vblankcount(dev, pipe, slot); smp_rmb(); - } while (cur_vblank != vblank->count && --count > 0); + } while (slot != vblank->slot && --count > 0);
- return cur_vblank; + return vblankcount; } EXPORT_SYMBOL(drm_vblank_count_and_time);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 6717a7d..9de9fde 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -374,10 +374,10 @@ struct drm_master { void *driver_priv; };
-/* Size of ringbuffer for vblank timestamps. Just double-buffer +/* Size of ringbuffer for vblank counts/timestamps. Just double-buffer * in initial implementation. */ -#define DRM_VBLANKTIME_RBSIZE 2 +#define DRM_VBLANK_RBSIZE 2
/* Flags and return codes for get_vblank_timestamp() driver function. */ #define DRM_CALLED_FROM_VBLIRQ 1 @@ -692,10 +692,10 @@ struct drm_vblank_crtc { wait_queue_head_t queue; /**< VBLANK wait queue */ struct timer_list disable_timer; /* delayed disable timer */
- /* vblank counter, protected by dev->vblank_time_lock for writes */ - u32 count; - /* vblank timestamps, protected by dev->vblank_time_lock for writes */ - struct timeval time[DRM_VBLANKTIME_RBSIZE]; + u32 slot; + /* vblank timestamps and counter, protected by dev->vblank_time_lock for writes */ + struct timeval time[DRM_VBLANK_RBSIZE]; + u32 count[DRM_VBLANK_RBSIZE];
atomic_t refcount; /* number of users of vblank interruptsper crtc */ u32 last; /* protected by dev->vbl_lock, used */
On Mon, Sep 14, 2015 at 10:43:52PM +0300, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
The vblank timestamp ringbuffer only has two entries, so if the vblank->count is incremented by an even number readers may end up seeing the new vblank timestamp alongside the old vblank counter value.
Fix the problem by storing the vblank counter in a ringbuffer as well, and always increment the ringbuffer "slot" by one when storing a new timestamp+counter pair.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Imo if we bother with this we might as well just switch over to using full-blown seqlocks. They internally use a two-stage update which means race-free even with just one copy of the data we protect. Also more standardized to boot.
Series looks good otherwise, I'll wait for Maarten to r-b it and then pull it in. -Daniel
drivers/gpu/drm/drm_irq.c | 40 ++++++++++++++++++++++++---------------- include/drm/drmP.h | 12 ++++++------ 2 files changed, 30 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 88fbee4..8de236a 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -43,8 +43,10 @@ #include <linux/export.h>
/* Access macro for slots in vblank timestamp ringbuffer. */ -#define vblanktimestamp(dev, pipe, count) \
- ((dev)->vblank[pipe].time[(count) % DRM_VBLANKTIME_RBSIZE])
+#define vblanktimestamp(dev, pipe, slot) \
- ((dev)->vblank[pipe].time[(slot) % DRM_VBLANK_RBSIZE])
+#define vblankcount(dev, pipe, slot) \
- ((dev)->vblank[pipe].count[(slot) % DRM_VBLANK_RBSIZE])
/* Retry timestamp calculation up to 3 times to satisfy
- drm_timestamp_precision before giving up.
@@ -76,20 +78,23 @@ module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
static void store_vblank(struct drm_device *dev, unsigned int pipe, u32 vblank_count_inc,
struct timeval *t_vblank, u32 last)
const struct timeval *t_vblank, u32 last){ struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
- u32 tslot;
u32 slot;
assert_spin_locked(&dev->vblank_time_lock);
slot = vblank->slot + 1;
vblank->last = last;
/* All writers hold the spinlock, but readers are serialized by
* the latching of vblank->count below.
* the latching of vblank->slot below.
- tslot = vblank->count + vblank_count_inc;
- vblanktimestamp(dev, pipe, tslot) = *t_vblank;
vblankcount(dev, pipe, slot) =
vblankcount(dev, pipe, vblank->slot) + vblank_count_inc;vblanktimestamp(dev, pipe, slot) = *t_vblank;
/*
- vblank timestamp updates are protected on the write side with
@@ -100,7 +105,7 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe, * The read-side barriers for this are in drm_vblank_count_and_time. */ smp_wmb();
- vblank->count += vblank_count_inc;
- vblank->slot = slot; smp_wmb();
}
@@ -202,7 +207,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, const struct timeval *t_old; u64 diff_ns;
t_old = &vblanktimestamp(dev, pipe, vblank->count);
t_old = &vblanktimestamp(dev, pipe, vblank->slot);diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old);
/*
@@ -223,7 +228,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
DRM_DEBUG("updating vblank count on crtc %u:" " current=%u, diff=%u, hw=%u hw_last=%u\n",
pipe, vblank->count, diff, cur_vblank, vblank->last);
pipe, vblankcount(dev, pipe, vblank->slot),diff, cur_vblank, vblank->last);if (diff == 0) { WARN_ON_ONCE(cur_vblank != vblank->last);
@@ -883,7 +889,7 @@ u32 drm_vblank_count(struct drm_device *dev, int pipe) if (WARN_ON(pipe >= dev->num_crtcs)) return 0;
- return vblank->count;
- return vblankcount(dev, pipe, vblank->slot);
} EXPORT_SYMBOL(drm_vblank_count);
@@ -923,7 +929,8 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe, { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; int count = DRM_TIMESTAMP_MAXRETRIES;
- u32 cur_vblank;
u32 vblankcount;
u32 slot;
if (WARN_ON(pipe >= dev->num_crtcs)) return 0;
@@ -934,13 +941,14 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe, * This works like a seqlock. The write-side barriers are in store_vblank. */ do {
cur_vblank = vblank->count;
slot = vblank->slot;
*vblanktime = vblanktimestamp(dev, pipe, cur_vblank);
*vblanktime = vblanktimestamp(dev, pipe, slot);vblankcount = vblankcount(dev, pipe, slot);
- } while (cur_vblank != vblank->count && --count > 0);
- } while (slot != vblank->slot && --count > 0);
- return cur_vblank;
- return vblankcount;
} EXPORT_SYMBOL(drm_vblank_count_and_time);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 6717a7d..9de9fde 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -374,10 +374,10 @@ struct drm_master { void *driver_priv; };
-/* Size of ringbuffer for vblank timestamps. Just double-buffer +/* Size of ringbuffer for vblank counts/timestamps. Just double-buffer
- in initial implementation.
*/ -#define DRM_VBLANKTIME_RBSIZE 2 +#define DRM_VBLANK_RBSIZE 2
/* Flags and return codes for get_vblank_timestamp() driver function. */ #define DRM_CALLED_FROM_VBLIRQ 1 @@ -692,10 +692,10 @@ struct drm_vblank_crtc { wait_queue_head_t queue; /**< VBLANK wait queue */ struct timer_list disable_timer; /* delayed disable timer */
- /* vblank counter, protected by dev->vblank_time_lock for writes */
- u32 count;
- /* vblank timestamps, protected by dev->vblank_time_lock for writes */
- struct timeval time[DRM_VBLANKTIME_RBSIZE];
u32 slot;
/* vblank timestamps and counter, protected by dev->vblank_time_lock for writes */
struct timeval time[DRM_VBLANK_RBSIZE];
u32 count[DRM_VBLANK_RBSIZE];
atomic_t refcount; /* number of users of vblank interruptsper crtc */ u32 last; /* protected by dev->vbl_lock, used */
-- 2.4.6
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 22-09-15 om 11:10 schreef Daniel Vetter:
On Mon, Sep 14, 2015 at 10:43:52PM +0300, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
The vblank timestamp ringbuffer only has two entries, so if the vblank->count is incremented by an even number readers may end up seeing the new vblank timestamp alongside the old vblank counter value.
Fix the problem by storing the vblank counter in a ringbuffer as well, and always increment the ringbuffer "slot" by one when storing a new timestamp+counter pair.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Imo if we bother with this we might as well just switch over to using full-blown seqlocks. They internally use a two-stage update which means race-free even with just one copy of the data we protect. Also more standardized to boot.
Series looks good otherwise, I'll wait for Maarten to r-b it and then pull it in.
R-b for 1-10.
On Tue, Sep 22, 2015 at 01:15:01PM +0200, Maarten Lankhorst wrote:
Op 22-09-15 om 11:10 schreef Daniel Vetter:
On Mon, Sep 14, 2015 at 10:43:52PM +0300, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
The vblank timestamp ringbuffer only has two entries, so if the vblank->count is incremented by an even number readers may end up seeing the new vblank timestamp alongside the old vblank counter value.
Fix the problem by storing the vblank counter in a ringbuffer as well, and always increment the ringbuffer "slot" by one when storing a new timestamp+counter pair.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Imo if we bother with this we might as well just switch over to using full-blown seqlocks. They internally use a two-stage update which means race-free even with just one copy of the data we protect. Also more standardized to boot.
Series looks good otherwise, I'll wait for Maarten to r-b it and then pull it in.
R-b for 1-10.
Merged to drm-misc, thanks. -Daniel
On Tue, Sep 22, 2015 at 11:10:50AM +0200, Daniel Vetter wrote:
On Mon, Sep 14, 2015 at 10:43:52PM +0300, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
The vblank timestamp ringbuffer only has two entries, so if the vblank->count is incremented by an even number readers may end up seeing the new vblank timestamp alongside the old vblank counter value.
Fix the problem by storing the vblank counter in a ringbuffer as well, and always increment the ringbuffer "slot" by one when storing a new timestamp+counter pair.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Imo if we bother with this we might as well just switch over to using full-blown seqlocks. They internally use a two-stage update which means race-free even with just one copy of the data we protect. Also more standardized to boot.
Any volunteers for that? I don't have time to start redoing this right now.
Series looks good otherwise, I'll wait for Maarten to r-b it and then pull it in. -Daniel
drivers/gpu/drm/drm_irq.c | 40 ++++++++++++++++++++++++---------------- include/drm/drmP.h | 12 ++++++------ 2 files changed, 30 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 88fbee4..8de236a 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -43,8 +43,10 @@ #include <linux/export.h>
/* Access macro for slots in vblank timestamp ringbuffer. */ -#define vblanktimestamp(dev, pipe, count) \
- ((dev)->vblank[pipe].time[(count) % DRM_VBLANKTIME_RBSIZE])
+#define vblanktimestamp(dev, pipe, slot) \
- ((dev)->vblank[pipe].time[(slot) % DRM_VBLANK_RBSIZE])
+#define vblankcount(dev, pipe, slot) \
- ((dev)->vblank[pipe].count[(slot) % DRM_VBLANK_RBSIZE])
/* Retry timestamp calculation up to 3 times to satisfy
- drm_timestamp_precision before giving up.
@@ -76,20 +78,23 @@ module_param_named(timestamp_monotonic, drm_timestamp_monotonic, int, 0600);
static void store_vblank(struct drm_device *dev, unsigned int pipe, u32 vblank_count_inc,
struct timeval *t_vblank, u32 last)
const struct timeval *t_vblank, u32 last){ struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
- u32 tslot;
u32 slot;
assert_spin_locked(&dev->vblank_time_lock);
slot = vblank->slot + 1;
vblank->last = last;
/* All writers hold the spinlock, but readers are serialized by
* the latching of vblank->count below.
* the latching of vblank->slot below.
- tslot = vblank->count + vblank_count_inc;
- vblanktimestamp(dev, pipe, tslot) = *t_vblank;
vblankcount(dev, pipe, slot) =
vblankcount(dev, pipe, vblank->slot) + vblank_count_inc;vblanktimestamp(dev, pipe, slot) = *t_vblank;
/*
- vblank timestamp updates are protected on the write side with
@@ -100,7 +105,7 @@ static void store_vblank(struct drm_device *dev, unsigned int pipe, * The read-side barriers for this are in drm_vblank_count_and_time. */ smp_wmb();
- vblank->count += vblank_count_inc;
- vblank->slot = slot; smp_wmb();
}
@@ -202,7 +207,7 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe, const struct timeval *t_old; u64 diff_ns;
t_old = &vblanktimestamp(dev, pipe, vblank->count);
t_old = &vblanktimestamp(dev, pipe, vblank->slot);diff_ns = timeval_to_ns(&t_vblank) - timeval_to_ns(t_old);
/*
@@ -223,7 +228,8 @@ static void drm_update_vblank_count(struct drm_device *dev, unsigned int pipe,
DRM_DEBUG("updating vblank count on crtc %u:" " current=%u, diff=%u, hw=%u hw_last=%u\n",
pipe, vblank->count, diff, cur_vblank, vblank->last);
pipe, vblankcount(dev, pipe, vblank->slot),diff, cur_vblank, vblank->last);if (diff == 0) { WARN_ON_ONCE(cur_vblank != vblank->last);
@@ -883,7 +889,7 @@ u32 drm_vblank_count(struct drm_device *dev, int pipe) if (WARN_ON(pipe >= dev->num_crtcs)) return 0;
- return vblank->count;
- return vblankcount(dev, pipe, vblank->slot);
} EXPORT_SYMBOL(drm_vblank_count);
@@ -923,7 +929,8 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe, { struct drm_vblank_crtc *vblank = &dev->vblank[pipe]; int count = DRM_TIMESTAMP_MAXRETRIES;
- u32 cur_vblank;
u32 vblankcount;
u32 slot;
if (WARN_ON(pipe >= dev->num_crtcs)) return 0;
@@ -934,13 +941,14 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, unsigned int pipe, * This works like a seqlock. The write-side barriers are in store_vblank. */ do {
cur_vblank = vblank->count;
slot = vblank->slot;
*vblanktime = vblanktimestamp(dev, pipe, cur_vblank);
*vblanktime = vblanktimestamp(dev, pipe, slot);vblankcount = vblankcount(dev, pipe, slot);
- } while (cur_vblank != vblank->count && --count > 0);
- } while (slot != vblank->slot && --count > 0);
- return cur_vblank;
- return vblankcount;
} EXPORT_SYMBOL(drm_vblank_count_and_time);
diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 6717a7d..9de9fde 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -374,10 +374,10 @@ struct drm_master { void *driver_priv; };
-/* Size of ringbuffer for vblank timestamps. Just double-buffer +/* Size of ringbuffer for vblank counts/timestamps. Just double-buffer
- in initial implementation.
*/ -#define DRM_VBLANKTIME_RBSIZE 2 +#define DRM_VBLANK_RBSIZE 2
/* Flags and return codes for get_vblank_timestamp() driver function. */ #define DRM_CALLED_FROM_VBLIRQ 1 @@ -692,10 +692,10 @@ struct drm_vblank_crtc { wait_queue_head_t queue; /**< VBLANK wait queue */ struct timer_list disable_timer; /* delayed disable timer */
- /* vblank counter, protected by dev->vblank_time_lock for writes */
- u32 count;
- /* vblank timestamps, protected by dev->vblank_time_lock for writes */
- struct timeval time[DRM_VBLANKTIME_RBSIZE];
u32 slot;
/* vblank timestamps and counter, protected by dev->vblank_time_lock for writes */
struct timeval time[DRM_VBLANK_RBSIZE];
u32 count[DRM_VBLANK_RBSIZE];
atomic_t refcount; /* number of users of vblank interruptsper crtc */ u32 last; /* protected by dev->vbl_lock, used */
-- 2.4.6
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
On Tue, Sep 22, 2015 at 03:36:44PM +0300, Ville Syrjälä wrote:
On Tue, Sep 22, 2015 at 11:10:50AM +0200, Daniel Vetter wrote:
On Mon, Sep 14, 2015 at 10:43:52PM +0300, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
The vblank timestamp ringbuffer only has two entries, so if the vblank->count is incremented by an even number readers may end up seeing the new vblank timestamp alongside the old vblank counter value.
Fix the problem by storing the vblank counter in a ringbuffer as well, and always increment the ringbuffer "slot" by one when storing a new timestamp+counter pair.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Imo if we bother with this we might as well just switch over to using full-blown seqlocks. They internally use a two-stage update which means race-free even with just one copy of the data we protect. Also more standardized to boot.
Any volunteers for that? I don't have time to start redoing this right now.
I guess we can keep it as a low-hanging thing for now, didn't seem to have annoyed anyone yet. It should be fairly simple since all the vblank counter access goes through the 2 functions I created a while back while polishing the barriers a bit. -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Maarten Lankhorst -
Michel Dänzer -
Thierry Reding -
Thierry Reding -
Ville Syrjälä -
ville.syrjala＠linux.intel.com