From: Ville Syrjälä ville.syrjala@linux.intel.com
I'm going to require vblank interrupts during modeset in i915 soon, however the drm vblank code won't currently allow it. It simply refuses to re-enable the vblank interrupts whenever they were disable and the refcount is already elevated. That means anyone holding a vblank reference across a modeset/dpms cycle can prevent the driver from operating correctly.
The same problem can also cause wait for vblank ioctls issued soon after a modeset to fail. I've cooked up a few i-g-t/kms_flip subtests to excercise this.
I don't want to be responsible for potentially breaking every other drm driver, so I've made the new behaviour opt in. If the driver uses the new way of doing things, vblank interrupts (and ioctls) are allowed everywhere except between drm_vblank_off() and drm_vblanl_on(). Any wait for vblank ioctl currently pending will get completed when drm_vblank_off() is called, including blocking waits which previously could have just sat there until the timeout expired.
I also fixed the vblank disable timer to act predictably when multiple crtcs are involved , and I hoovered up an old patch from Peter Hurley to skip the double irqsave in drm_vblank_get().
Peter Hurley (1): drm: Use correct spinlock flavor in drm_vblank_get()
Ville Syrjälä (4): drm: Make the vblank disable timer per-crtc drm: Allow the driver to reject vblank requests only when it really has the vblank interrupts disabled drm: Allow reenabling of vblank interrupts even if refcount>0 drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races
drivers/gpu/drm/armada/armada_crtc.c | 2 +- drivers/gpu/drm/drm_irq.c | 78 ++++++++++++++++++++++---------- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 2 +- drivers/gpu/drm/gma500/gma_display.c | 2 +- drivers/gpu/drm/i915/i915_dma.c | 6 +++ drivers/gpu/drm/i915/intel_display.c | 23 ++++++---- drivers/gpu/drm/tegra/dc.c | 2 +- include/drm/drmP.h | 14 +++++- 8 files changed, 91 insertions(+), 38 deletions(-)
From: Peter Hurley peter@hurleysoftware.com
The irq flags state is already established by the outer spin_lock_irqsave(); re-disabling irqs is redundant.
Signed-off-by: Peter Hurley peter@hurleysoftware.com --- drivers/gpu/drm/drm_irq.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index c2676b5..baa12e7 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -882,13 +882,13 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc) */ int drm_vblank_get(struct drm_device *dev, int crtc) { - unsigned long irqflags, irqflags2; + unsigned long irqflags; int ret = 0;
spin_lock_irqsave(&dev->vbl_lock, irqflags); /* Going from 0->1 means we have to enable interrupts again */ if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) { - spin_lock_irqsave(&dev->vblank_time_lock, irqflags2); + spin_lock(&dev->vblank_time_lock); if (!dev->vblank[crtc].enabled) { /* Enable vblank irqs under vblank_time_lock protection. * All vblank count & timestamp updates are held off @@ -906,7 +906,7 @@ int drm_vblank_get(struct drm_device *dev, int crtc) drm_update_vblank_count(dev, crtc); } } - spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags2); + spin_unlock(&dev->vblank_time_lock); } else { if (!dev->vblank[crtc].enabled) { atomic_dec(&dev->vblank[crtc].refcount);
On Fri, 21 Feb 2014 21:03:31 +0200 ville.syrjala@linux.intel.com wrote:
From: Peter Hurley peter@hurleysoftware.com
The irq flags state is already established by the outer spin_lock_irqsave(); re-disabling irqs is redundant.
Signed-off-by: Peter Hurley peter@hurleysoftware.com
drivers/gpu/drm/drm_irq.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index c2676b5..baa12e7 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -882,13 +882,13 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc) */ int drm_vblank_get(struct drm_device *dev, int crtc) {
- unsigned long irqflags, irqflags2;
unsigned long irqflags; int ret = 0;
spin_lock_irqsave(&dev->vbl_lock, irqflags); /* Going from 0->1 means we have to enable interrupts again */ if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) {
spin_lock_irqsave(&dev->vblank_time_lock, irqflags2);
spin_lock(&dev->vblank_time_lock);@@ -906,7 +906,7 @@ int drm_vblank_get(struct drm_device *dev, int crtc) drm_update_vblank_count(dev, crtc); } }
spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags2);
spin_unlock(&dev->vblank_time_lock);
Reviewed-by: Jesse Barnes jbarnes@virtuousgeek.org
From: Ville Syrjälä ville.syrjala@linux.intel.com
Currently there's one per-device vblank disable timer, and it gets reset wheneven the vblank refcount for any crtc drops to zero. That means that one crtc could accidentally be keeping the vblank interrupts for other crtcs enabled even if there are no users for them. Make the disable timer per-crtc to avoid this issue.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 40 +++++++++++++++++++++------------------- include/drm/drmP.h | 4 +++- 2 files changed, 24 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index baa12e7..3211158 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -167,33 +167,34 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
static void vblank_disable_fn(unsigned long arg) { - struct drm_device *dev = (struct drm_device *)arg; + struct drm_vblank_crtc *vblank = (void *)arg; + struct drm_device *dev = vblank->dev; unsigned long irqflags; - int i; + int crtc = vblank->crtc;
if (!dev->vblank_disable_allowed) return;
- for (i = 0; i < dev->num_crtcs; i++) { - spin_lock_irqsave(&dev->vbl_lock, irqflags); - if (atomic_read(&dev->vblank[i].refcount) == 0 && - dev->vblank[i].enabled) { - DRM_DEBUG("disabling vblank on crtc %d\n", i); - vblank_disable_and_save(dev, i); - } - spin_unlock_irqrestore(&dev->vbl_lock, irqflags); + spin_lock_irqsave(&dev->vbl_lock, irqflags); + if (atomic_read(&vblank->refcount) == 0 && vblank->enabled) { + DRM_DEBUG("disabling vblank on crtc %d\n", crtc); + vblank_disable_and_save(dev, crtc); } + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); }
void drm_vblank_cleanup(struct drm_device *dev) { + int crtc; + /* Bail if the driver didn't call drm_vblank_init() */ if (dev->num_crtcs == 0) return;
- del_timer_sync(&dev->vblank_disable_timer); - - vblank_disable_fn((unsigned long)dev); + for (crtc = 0; crtc < dev->num_crtcs; crtc++) { + del_timer_sync(&dev->vblank[crtc].disable_timer); + vblank_disable_fn((unsigned long)&dev->vblank[crtc]); + }
kfree(dev->vblank);
@@ -205,8 +206,6 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs) { int i, ret = -ENOMEM;
- setup_timer(&dev->vblank_disable_timer, vblank_disable_fn, - (unsigned long)dev); spin_lock_init(&dev->vbl_lock); spin_lock_init(&dev->vblank_time_lock);
@@ -216,8 +215,13 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs) if (!dev->vblank) goto err;
- for (i = 0; i < num_crtcs; i++) + for (i = 0; i < num_crtcs; i++) { + dev->vblank[i].dev = dev; + dev->vblank[i].crtc = i; init_waitqueue_head(&dev->vblank[i].queue); + setup_timer(&dev->vblank[i].disable_timer, vblank_disable_fn, + (unsigned long)&dev->vblank[i]); + }
DRM_INFO("Supports vblank timestamp caching Rev 2 (21.10.2013).\n");
@@ -934,7 +938,7 @@ void drm_vblank_put(struct drm_device *dev, int crtc) /* Last user schedules interrupt disable */ if (atomic_dec_and_test(&dev->vblank[crtc].refcount) && (drm_vblank_offdelay > 0)) - mod_timer(&dev->vblank_disable_timer, + mod_timer(&dev->vblank[crtc].disable_timer, jiffies + ((drm_vblank_offdelay * HZ)/1000)); } EXPORT_SYMBOL(drm_vblank_put); @@ -943,8 +947,6 @@ EXPORT_SYMBOL(drm_vblank_put); * drm_vblank_off - disable vblank events on a CRTC * @dev: DRM device * @crtc: CRTC in question - * - * Caller must hold event lock. */ void drm_vblank_off(struct drm_device *dev, int crtc) { diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 04a7f31..f974da9 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1077,14 +1077,17 @@ struct drm_pending_vblank_event { };
struct drm_vblank_crtc { + struct drm_device *dev; /* pointer to the drm_device */ wait_queue_head_t queue; /**< VBLANK wait queue */ struct timeval time[DRM_VBLANKTIME_RBSIZE]; /**< timestamp of current count */ + struct timer_list disable_timer; /* delayed disable timer */ atomic_t count; /**< number of VBLANK interrupts */ atomic_t refcount; /* number of users of vblank interruptsper crtc */ u32 last; /* protected by dev->vbl_lock, used */ /* for wraparound handling */ u32 last_wait; /* Last vblank seqno waited per CRTC */ unsigned int inmodeset; /* Display driver is setting mode */ + int crtc; /* crtc index */ bool enabled; /* so we don't call enable more than once per disable */ }; @@ -1157,7 +1160,6 @@ struct drm_device {
spinlock_t vblank_time_lock; /**< Protects vblank count and time updates during vblank enable/disable */ spinlock_t vbl_lock; - struct timer_list vblank_disable_timer;
u32 max_vblank_count; /**< size of vblank counter register */
On Fri, 21 Feb 2014 21:03:32 +0200 ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Currently there's one per-device vblank disable timer, and it gets reset wheneven the vblank refcount for any crtc drops to zero. That means that one crtc could accidentally be keeping the vblank interrupts for other crtcs enabled even if there are no users for them. Make the disable timer per-crtc to avoid this issue.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_irq.c | 40 +++++++++++++++++++++------------------- include/drm/drmP.h | 4 +++- 2 files changed, 24 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index baa12e7..3211158 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -167,33 +167,34 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
static void vblank_disable_fn(unsigned long arg) {
- struct drm_device *dev = (struct drm_device *)arg;
- struct drm_vblank_crtc *vblank = (void *)arg;
- struct drm_device *dev = vblank->dev; unsigned long irqflags;
- int i;
int crtc = vblank->crtc;
if (!dev->vblank_disable_allowed) return;
- for (i = 0; i < dev->num_crtcs; i++) {
spin_lock_irqsave(&dev->vbl_lock, irqflags);if (atomic_read(&dev->vblank[i].refcount) == 0 &&dev->vblank[i].enabled) {DRM_DEBUG("disabling vblank on crtc %d\n", i);vblank_disable_and_save(dev, i);}spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
- spin_lock_irqsave(&dev->vbl_lock, irqflags);
- if (atomic_read(&vblank->refcount) == 0 && vblank->enabled) {
DRM_DEBUG("disabling vblank on crtc %d\n", crtc);vblank_disable_and_save(dev, crtc);- spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
}
void drm_vblank_cleanup(struct drm_device *dev) {
- int crtc;
- /* Bail if the driver didn't call drm_vblank_init() */ if (dev->num_crtcs == 0) return;
- del_timer_sync(&dev->vblank_disable_timer);
- vblank_disable_fn((unsigned long)dev);
for (crtc = 0; crtc < dev->num_crtcs; crtc++) {
del_timer_sync(&dev->vblank[crtc].disable_timer);vblank_disable_fn((unsigned long)&dev->vblank[crtc]);}
kfree(dev->vblank);
@@ -205,8 +206,6 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs) { int i, ret = -ENOMEM;
- setup_timer(&dev->vblank_disable_timer, vblank_disable_fn,
(unsigned long)dev);@@ -216,8 +215,13 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs) if (!dev->vblank) goto err;
- for (i = 0; i < num_crtcs; i++)
for (i = 0; i < num_crtcs; i++) {
dev->vblank[i].dev = dev;dev->vblank[i].crtc = i;init_waitqueue_head(&dev->vblank[i].queue);
setup_timer(&dev->vblank[i].disable_timer, vblank_disable_fn,(unsigned long)&dev->vblank[i]);}
DRM_INFO("Supports vblank timestamp caching Rev 2 (21.10.2013).\n");
@@ -934,7 +938,7 @@ void drm_vblank_put(struct drm_device *dev, int crtc) /* Last user schedules interrupt disable */ if (atomic_dec_and_test(&dev->vblank[crtc].refcount) && (drm_vblank_offdelay > 0))
mod_timer(&dev->vblank_disable_timer,
mod_timer(&dev->vblank[crtc].disable_timer, jiffies + ((drm_vblank_offdelay * HZ)/1000));} EXPORT_SYMBOL(drm_vblank_put); @@ -943,8 +947,6 @@ EXPORT_SYMBOL(drm_vblank_put);
- drm_vblank_off - disable vblank events on a CRTC
- @dev: DRM device
- @crtc: CRTC in question
*/
- Caller must hold event lock.
void drm_vblank_off(struct drm_device *dev, int crtc) { diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 04a7f31..f974da9 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1077,14 +1077,17 @@ struct drm_pending_vblank_event { };
struct drm_vblank_crtc {
- struct drm_device *dev; /* pointer to the drm_device */ wait_queue_head_t queue; /**< VBLANK wait queue */ struct timeval time[DRM_VBLANKTIME_RBSIZE]; /**< timestamp of current count */
- struct timer_list disable_timer; /* delayed disable timer */ atomic_t count; /**< number of VBLANK interrupts */ atomic_t refcount; /* number of users of vblank interruptsper crtc */ u32 last; /* protected by dev->vbl_lock, used */ /* for wraparound handling */ u32 last_wait; /* Last vblank seqno waited per CRTC */ unsigned int inmodeset; /* Display driver is setting mode */
- int crtc; /* crtc index */ bool enabled; /* so we don't call enable more than once per disable */
}; @@ -1157,7 +1160,6 @@ struct drm_device {
spinlock_t vblank_time_lock; /**< Protects vblank count and time updates during vblank enable/disable */ spinlock_t vbl_lock;
struct timer_list vblank_disable_timer;
u32 max_vblank_count; /**< size of vblank counter register */
Yeah this looks like a good fix.
Reviewed-by: Jesse Barnes jbarnes@virtuousgeek.org
From: Ville Syrjälä ville.syrjala@linux.intel.com
Allow the driver to specify whether all new vblank requests after drm_vblank_off() should be rejected. And add a counterpart called drm_vblank_on() which will again allow vblank requests to come in.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/armada/armada_crtc.c | 2 +- drivers/gpu/drm/drm_irq.c | 29 ++++++++++++++++++++++++++++- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 2 +- drivers/gpu/drm/gma500/gma_display.c | 2 +- drivers/gpu/drm/i915/intel_display.c | 6 +++--- drivers/gpu/drm/tegra/dc.c | 2 +- include/drm/drmP.h | 4 +++- 7 files changed, 38 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index d8e3982..74317b2 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -257,7 +257,7 @@ static void armada_drm_vblank_off(struct armada_crtc *dcrtc) * Tell the DRM core that vblank IRQs aren't going to happen for * a while. This cleans up any pending vblank events for us. */ - drm_vblank_off(dev, dcrtc->num); + drm_vblank_off(dev, dcrtc->num, false);
/* Handle any pending flip event. */ spin_lock_irq(&dev->event_lock); diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 3211158..6e5d820 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -890,6 +890,12 @@ int drm_vblank_get(struct drm_device *dev, int crtc) int ret = 0;
spin_lock_irqsave(&dev->vbl_lock, irqflags); + + if (dev->vblank[crtc].reject) { + ret = -EINVAL; + goto out; + } + /* Going from 0->1 means we have to enable interrupts again */ if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) { spin_lock(&dev->vblank_time_lock); @@ -917,6 +923,8 @@ int drm_vblank_get(struct drm_device *dev, int crtc) ret = -EINVAL; } } + + out: spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
return ret; @@ -947,8 +955,9 @@ EXPORT_SYMBOL(drm_vblank_put); * drm_vblank_off - disable vblank events on a CRTC * @dev: DRM device * @crtc: CRTC in question + * @reject: reject drm_vblank_get() until drm_vblank_on() has been called? */ -void drm_vblank_off(struct drm_device *dev, int crtc) +void drm_vblank_off(struct drm_device *dev, int crtc, bool reject) { struct drm_pending_vblank_event *e, *t; struct timeval now; @@ -956,6 +965,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) unsigned int seq;
spin_lock_irqsave(&dev->vbl_lock, irqflags); + dev->vblank[crtc].reject = reject; vblank_disable_and_save(dev, crtc); wake_up(&dev->vblank[crtc].queue);
@@ -979,6 +989,22 @@ void drm_vblank_off(struct drm_device *dev, int crtc) } EXPORT_SYMBOL(drm_vblank_off);
+ +/** + * drm_vblank_on - enable vblank events on a CRTC + * @dev: DRM device + * @crtc: CRTC in question + */ +void drm_vblank_on(struct drm_device *dev, int crtc) +{ + unsigned long irqflags; + + spin_lock_irqsave(&dev->vbl_lock, irqflags); + dev->vblank[crtc].reject = false; + spin_unlock_irqrestore(&dev->vbl_lock, irqflags); +} +EXPORT_SYMBOL(drm_vblank_on); + /** * drm_vblank_pre_modeset - account for vblanks across mode sets * @dev: DRM device @@ -1224,6 +1250,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, DRM_WAIT_ON(ret, dev->vblank[crtc].queue, 3 * HZ, (((drm_vblank_count(dev, crtc) - vblwait->request.sequence) <= (1 << 23)) || + dev->vblank[crtc].reject || !dev->irq_enabled));
if (ret != -EINTR) { diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index ebc0150..e2d6b9d 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -68,7 +68,7 @@ static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode) /* wait for the completion of page flip. */ wait_event(exynos_crtc->pending_flip_queue, atomic_read(&exynos_crtc->pending_flip) == 0); - drm_vblank_off(crtc->dev, exynos_crtc->pipe); + drm_vblank_off(crtc->dev, exynos_crtc->pipe, false); }
exynos_drm_fn_encoder(crtc, &mode, exynos_drm_encoder_crtc_dpms); diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c index 386de2c..ff18220 100644 --- a/drivers/gpu/drm/gma500/gma_display.c +++ b/drivers/gpu/drm/gma500/gma_display.c @@ -281,7 +281,7 @@ void gma_crtc_dpms(struct drm_crtc *crtc, int mode) REG_WRITE(VGACNTRL, VGA_DISP_DISABLE);
/* Turn off vblank interrupts */ - drm_vblank_off(dev, pipe); + drm_vblank_off(dev, pipe, false);
/* Wait for vblank for the disable to take effect */ gma_wait_for_vblank(dev); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f19e6ea..bab0d08 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3643,7 +3643,7 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc) int plane = intel_crtc->plane;
intel_crtc_wait_for_pending_flips(crtc); - drm_vblank_off(dev, pipe); + drm_vblank_off(dev, pipe, false);
/* FBC must be disabled before disabling the plane on HSW. */ if (dev_priv->fbc.plane == plane) @@ -3774,7 +3774,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) encoder->disable(encoder);
intel_crtc_wait_for_pending_flips(crtc); - drm_vblank_off(dev, pipe); + drm_vblank_off(dev, pipe, false);
if (dev_priv->fbc.plane == plane) intel_disable_fbc(dev); @@ -4239,7 +4239,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
/* Give the overlay scaler a chance to disable if it's on this pipe */ intel_crtc_wait_for_pending_flips(crtc); - drm_vblank_off(dev, pipe); + drm_vblank_off(dev, pipe, false);
if (dev_priv->fbc.plane == plane) intel_disable_fbc(dev); diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 9336006..480bfec 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -324,7 +324,7 @@ static void tegra_crtc_disable(struct drm_crtc *crtc) } }
- drm_vblank_off(drm, dc->pipe); + drm_vblank_off(drm, dc->pipe, false); }
static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index f974da9..ee40483 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1090,6 +1090,7 @@ struct drm_vblank_crtc { int crtc; /* crtc index */ bool enabled; /* so we don't call enable more than once per disable */ + bool reject; /* reject drm_vblank_get()? */ };
/** @@ -1400,7 +1401,8 @@ extern void drm_send_vblank_event(struct drm_device *dev, int crtc, extern bool drm_handle_vblank(struct drm_device *dev, int crtc); extern int drm_vblank_get(struct drm_device *dev, int crtc); extern void drm_vblank_put(struct drm_device *dev, int crtc); -extern void drm_vblank_off(struct drm_device *dev, int crtc); +extern void drm_vblank_off(struct drm_device *dev, int crtc, bool reject); +extern void drm_vblank_on(struct drm_device *dev, int crtc); extern void drm_vblank_cleanup(struct drm_device *dev); extern u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc, struct timeval *tvblank, unsigned flags);
On Fri, 21 Feb 2014 21:03:33 +0200 ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Allow the driver to specify whether all new vblank requests after drm_vblank_off() should be rejected. And add a counterpart called drm_vblank_on() which will again allow vblank requests to come in.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/armada/armada_crtc.c | 2 +- drivers/gpu/drm/drm_irq.c | 29 ++++++++++++++++++++++++++++- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 2 +- drivers/gpu/drm/gma500/gma_display.c | 2 +- drivers/gpu/drm/i915/intel_display.c | 6 +++--- drivers/gpu/drm/tegra/dc.c | 2 +- include/drm/drmP.h | 4 +++- 7 files changed, 38 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index d8e3982..74317b2 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -257,7 +257,7 @@ static void armada_drm_vblank_off(struct armada_crtc *dcrtc) * Tell the DRM core that vblank IRQs aren't going to happen for * a while. This cleans up any pending vblank events for us. */
- drm_vblank_off(dev, dcrtc->num);
drm_vblank_off(dev, dcrtc->num, false);
/* Handle any pending flip event. */ spin_lock_irq(&dev->event_lock);
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 3211158..6e5d820 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -890,6 +890,12 @@ int drm_vblank_get(struct drm_device *dev, int crtc) int ret = 0;
spin_lock_irqsave(&dev->vbl_lock, irqflags);
- if (dev->vblank[crtc].reject) {
ret = -EINVAL;goto out;- }
- /* Going from 0->1 means we have to enable interrupts again */ if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) { spin_lock(&dev->vblank_time_lock);
@@ -917,6 +923,8 @@ int drm_vblank_get(struct drm_device *dev, int crtc) ret = -EINVAL; } }
out: spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
return ret;
@@ -947,8 +955,9 @@ EXPORT_SYMBOL(drm_vblank_put);
- drm_vblank_off - disable vblank events on a CRTC
- @dev: DRM device
- @crtc: CRTC in question
*/
- @reject: reject drm_vblank_get() until drm_vblank_on() has been called?
-void drm_vblank_off(struct drm_device *dev, int crtc) +void drm_vblank_off(struct drm_device *dev, int crtc, bool reject) { struct drm_pending_vblank_event *e, *t; struct timeval now; @@ -956,6 +965,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) unsigned int seq;
spin_lock_irqsave(&dev->vbl_lock, irqflags);
- dev->vblank[crtc].reject = reject; vblank_disable_and_save(dev, crtc); wake_up(&dev->vblank[crtc].queue);
@@ -979,6 +989,22 @@ void drm_vblank_off(struct drm_device *dev, int crtc) } EXPORT_SYMBOL(drm_vblank_off);
+/**
- drm_vblank_on - enable vblank events on a CRTC
- @dev: DRM device
- @crtc: CRTC in question
- */
+void drm_vblank_on(struct drm_device *dev, int crtc) +{
- unsigned long irqflags;
- spin_lock_irqsave(&dev->vbl_lock, irqflags);
- dev->vblank[crtc].reject = false;
- spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+} +EXPORT_SYMBOL(drm_vblank_on);
/**
- drm_vblank_pre_modeset - account for vblanks across mode sets
- @dev: DRM device
@@ -1224,6 +1250,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, DRM_WAIT_ON(ret, dev->vblank[crtc].queue, 3 * HZ, (((drm_vblank_count(dev, crtc) - vblwait->request.sequence) <= (1 << 23)) ||
dev->vblank[crtc].reject || !dev->irq_enabled));if (ret != -EINTR) {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index ebc0150..e2d6b9d 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -68,7 +68,7 @@ static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode) /* wait for the completion of page flip. */ wait_event(exynos_crtc->pending_flip_queue, atomic_read(&exynos_crtc->pending_flip) == 0);
drm_vblank_off(crtc->dev, exynos_crtc->pipe);
drm_vblank_off(crtc->dev, exynos_crtc->pipe, false);}
exynos_drm_fn_encoder(crtc, &mode, exynos_drm_encoder_crtc_dpms);
diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c index 386de2c..ff18220 100644 --- a/drivers/gpu/drm/gma500/gma_display.c +++ b/drivers/gpu/drm/gma500/gma_display.c @@ -281,7 +281,7 @@ void gma_crtc_dpms(struct drm_crtc *crtc, int mode) REG_WRITE(VGACNTRL, VGA_DISP_DISABLE);
/* Turn off vblank interrupts */
drm_vblank_off(dev, pipe);
drm_vblank_off(dev, pipe, false);/* Wait for vblank for the disable to take effect */ gma_wait_for_vblank(dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f19e6ea..bab0d08 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3643,7 +3643,7 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc) int plane = intel_crtc->plane;
intel_crtc_wait_for_pending_flips(crtc);
- drm_vblank_off(dev, pipe);
drm_vblank_off(dev, pipe, false);
/* FBC must be disabled before disabling the plane on HSW. */ if (dev_priv->fbc.plane == plane)
@@ -3774,7 +3774,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) encoder->disable(encoder);
intel_crtc_wait_for_pending_flips(crtc);
- drm_vblank_off(dev, pipe);
drm_vblank_off(dev, pipe, false);
if (dev_priv->fbc.plane == plane) intel_disable_fbc(dev);
@@ -4239,7 +4239,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
/* Give the overlay scaler a chance to disable if it's on this pipe */ intel_crtc_wait_for_pending_flips(crtc);
- drm_vblank_off(dev, pipe);
drm_vblank_off(dev, pipe, false);
if (dev_priv->fbc.plane == plane) intel_disable_fbc(dev);
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 9336006..480bfec 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -324,7 +324,7 @@ static void tegra_crtc_disable(struct drm_crtc *crtc) } }
- drm_vblank_off(drm, dc->pipe);
- drm_vblank_off(drm, dc->pipe, false);
}
static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index f974da9..ee40483 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1090,6 +1090,7 @@ struct drm_vblank_crtc { int crtc; /* crtc index */ bool enabled; /* so we don't call enable more than once per disable */
- bool reject; /* reject drm_vblank_get()? */
};
/** @@ -1400,7 +1401,8 @@ extern void drm_send_vblank_event(struct drm_device *dev, int crtc, extern bool drm_handle_vblank(struct drm_device *dev, int crtc); extern int drm_vblank_get(struct drm_device *dev, int crtc); extern void drm_vblank_put(struct drm_device *dev, int crtc); -extern void drm_vblank_off(struct drm_device *dev, int crtc); +extern void drm_vblank_off(struct drm_device *dev, int crtc, bool reject); +extern void drm_vblank_on(struct drm_device *dev, int crtc); extern void drm_vblank_cleanup(struct drm_device *dev); extern u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc, struct timeval *tvblank, unsigned flags);
Reviewed-by: Jesse Barnes jbarnes@virtuousgeek.org
On Fri, Feb 21, 2014 at 09:03:33PM +0200, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Allow the driver to specify whether all new vblank requests after drm_vblank_off() should be rejected. And add a counterpart called drm_vblank_on() which will again allow vblank requests to come in.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Not really happy about this - drm_irq.c is already a giant mess, adding more driver-specific hacks doesn't help. I think we need a few bits of polish on top of your code:
- Add stern warnings to pre/post_modeset that they're inherently racy.
- Add calls to drm_vblank_off to every driver lacking them. Put it at the beginning of their crtc disable functions expect when there's a call to pre_modeset. Then it should be right after that.
- Sprinkle calls to drm_vblank_on over all drivers. Put them at the end of their crtc enable functions except when there's a call to post_modeset. Then put it right before that.
- Rip out the reject argument again and make it the default. If we have drm_vblank_off everywhere then all old vblank waits should complete and there's no userspace yet out there which races a modeset with vblank ioctl calls. Then only issue would be userspace which does vblank waits on disabled ioctls which a) is buggy b) we can easily fix with a driver quirk flag if _really_ needed.
This way the drm_irq.c mess will at least converge a bit and so should help generic display servers (like Wayland) a lot.
I can volunteer for this if you want to punt on it yourself. -Daniel
drivers/gpu/drm/armada/armada_crtc.c | 2 +- drivers/gpu/drm/drm_irq.c | 29 ++++++++++++++++++++++++++++- drivers/gpu/drm/exynos/exynos_drm_crtc.c | 2 +- drivers/gpu/drm/gma500/gma_display.c | 2 +- drivers/gpu/drm/i915/intel_display.c | 6 +++--- drivers/gpu/drm/tegra/dc.c | 2 +- include/drm/drmP.h | 4 +++- 7 files changed, 38 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/armada/armada_crtc.c b/drivers/gpu/drm/armada/armada_crtc.c index d8e3982..74317b2 100644 --- a/drivers/gpu/drm/armada/armada_crtc.c +++ b/drivers/gpu/drm/armada/armada_crtc.c @@ -257,7 +257,7 @@ static void armada_drm_vblank_off(struct armada_crtc *dcrtc) * Tell the DRM core that vblank IRQs aren't going to happen for * a while. This cleans up any pending vblank events for us. */
- drm_vblank_off(dev, dcrtc->num);
drm_vblank_off(dev, dcrtc->num, false);
/* Handle any pending flip event. */ spin_lock_irq(&dev->event_lock);
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 3211158..6e5d820 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -890,6 +890,12 @@ int drm_vblank_get(struct drm_device *dev, int crtc) int ret = 0;
spin_lock_irqsave(&dev->vbl_lock, irqflags);
- if (dev->vblank[crtc].reject) {
ret = -EINVAL;goto out;- }
- /* Going from 0->1 means we have to enable interrupts again */ if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) { spin_lock(&dev->vblank_time_lock);
@@ -917,6 +923,8 @@ int drm_vblank_get(struct drm_device *dev, int crtc) ret = -EINVAL; } }
out: spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
return ret;
@@ -947,8 +955,9 @@ EXPORT_SYMBOL(drm_vblank_put);
- drm_vblank_off - disable vblank events on a CRTC
- @dev: DRM device
- @crtc: CRTC in question
*/
- @reject: reject drm_vblank_get() until drm_vblank_on() has been called?
-void drm_vblank_off(struct drm_device *dev, int crtc) +void drm_vblank_off(struct drm_device *dev, int crtc, bool reject) { struct drm_pending_vblank_event *e, *t; struct timeval now; @@ -956,6 +965,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc) unsigned int seq;
spin_lock_irqsave(&dev->vbl_lock, irqflags);
- dev->vblank[crtc].reject = reject; vblank_disable_and_save(dev, crtc); wake_up(&dev->vblank[crtc].queue);
@@ -979,6 +989,22 @@ void drm_vblank_off(struct drm_device *dev, int crtc) } EXPORT_SYMBOL(drm_vblank_off);
+/**
- drm_vblank_on - enable vblank events on a CRTC
- @dev: DRM device
- @crtc: CRTC in question
- */
+void drm_vblank_on(struct drm_device *dev, int crtc) +{
- unsigned long irqflags;
- spin_lock_irqsave(&dev->vbl_lock, irqflags);
- dev->vblank[crtc].reject = false;
- spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+} +EXPORT_SYMBOL(drm_vblank_on);
/**
- drm_vblank_pre_modeset - account for vblanks across mode sets
- @dev: DRM device
@@ -1224,6 +1250,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data, DRM_WAIT_ON(ret, dev->vblank[crtc].queue, 3 * HZ, (((drm_vblank_count(dev, crtc) - vblwait->request.sequence) <= (1 << 23)) ||
dev->vblank[crtc].reject || !dev->irq_enabled));if (ret != -EINTR) {
diff --git a/drivers/gpu/drm/exynos/exynos_drm_crtc.c b/drivers/gpu/drm/exynos/exynos_drm_crtc.c index ebc0150..e2d6b9d 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_crtc.c +++ b/drivers/gpu/drm/exynos/exynos_drm_crtc.c @@ -68,7 +68,7 @@ static void exynos_drm_crtc_dpms(struct drm_crtc *crtc, int mode) /* wait for the completion of page flip. */ wait_event(exynos_crtc->pending_flip_queue, atomic_read(&exynos_crtc->pending_flip) == 0);
drm_vblank_off(crtc->dev, exynos_crtc->pipe);
drm_vblank_off(crtc->dev, exynos_crtc->pipe, false);}
exynos_drm_fn_encoder(crtc, &mode, exynos_drm_encoder_crtc_dpms);
diff --git a/drivers/gpu/drm/gma500/gma_display.c b/drivers/gpu/drm/gma500/gma_display.c index 386de2c..ff18220 100644 --- a/drivers/gpu/drm/gma500/gma_display.c +++ b/drivers/gpu/drm/gma500/gma_display.c @@ -281,7 +281,7 @@ void gma_crtc_dpms(struct drm_crtc *crtc, int mode) REG_WRITE(VGACNTRL, VGA_DISP_DISABLE);
/* Turn off vblank interrupts */
drm_vblank_off(dev, pipe);
drm_vblank_off(dev, pipe, false);/* Wait for vblank for the disable to take effect */ gma_wait_for_vblank(dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f19e6ea..bab0d08 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3643,7 +3643,7 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc) int plane = intel_crtc->plane;
intel_crtc_wait_for_pending_flips(crtc);
- drm_vblank_off(dev, pipe);
drm_vblank_off(dev, pipe, false);
/* FBC must be disabled before disabling the plane on HSW. */ if (dev_priv->fbc.plane == plane)
@@ -3774,7 +3774,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) encoder->disable(encoder);
intel_crtc_wait_for_pending_flips(crtc);
- drm_vblank_off(dev, pipe);
drm_vblank_off(dev, pipe, false);
if (dev_priv->fbc.plane == plane) intel_disable_fbc(dev);
@@ -4239,7 +4239,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
/* Give the overlay scaler a chance to disable if it's on this pipe */ intel_crtc_wait_for_pending_flips(crtc);
- drm_vblank_off(dev, pipe);
drm_vblank_off(dev, pipe, false);
if (dev_priv->fbc.plane == plane) intel_disable_fbc(dev);
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index 9336006..480bfec 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -324,7 +324,7 @@ static void tegra_crtc_disable(struct drm_crtc *crtc) } }
- drm_vblank_off(drm, dc->pipe);
- drm_vblank_off(drm, dc->pipe, false);
}
static bool tegra_crtc_mode_fixup(struct drm_crtc *crtc, diff --git a/include/drm/drmP.h b/include/drm/drmP.h index f974da9..ee40483 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1090,6 +1090,7 @@ struct drm_vblank_crtc { int crtc; /* crtc index */ bool enabled; /* so we don't call enable more than once per disable */
- bool reject; /* reject drm_vblank_get()? */
};
/** @@ -1400,7 +1401,8 @@ extern void drm_send_vblank_event(struct drm_device *dev, int crtc, extern bool drm_handle_vblank(struct drm_device *dev, int crtc); extern int drm_vblank_get(struct drm_device *dev, int crtc); extern void drm_vblank_put(struct drm_device *dev, int crtc); -extern void drm_vblank_off(struct drm_device *dev, int crtc); +extern void drm_vblank_off(struct drm_device *dev, int crtc, bool reject); +extern void drm_vblank_on(struct drm_device *dev, int crtc); extern void drm_vblank_cleanup(struct drm_device *dev); extern u32 drm_get_last_vbltimestamp(struct drm_device *dev, int crtc, struct timeval *tvblank, unsigned flags); -- 1.8.3.2
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Mar 04, 2014 at 10:24:54AM +0100, Daniel Vetter wrote:
On Fri, Feb 21, 2014 at 09:03:33PM +0200, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Allow the driver to specify whether all new vblank requests after drm_vblank_off() should be rejected. And add a counterpart called drm_vblank_on() which will again allow vblank requests to come in.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Not really happy about this - drm_irq.c is already a giant mess, adding more driver-specific hacks doesn't help. I think we need a few bits of polish on top of your code:
Add stern warnings to pre/post_modeset that they're inherently racy.
Add calls to drm_vblank_off to every driver lacking them. Put it at the beginning of their crtc disable functions expect when there's a call to pre_modeset. Then it should be right after that.
Sprinkle calls to drm_vblank_on over all drivers. Put them at the end of their crtc enable functions except when there's a call to post_modeset. Then put it right before that.
Rip out the reject argument again and make it the default. If we have drm_vblank_off everywhere then all old vblank waits should complete and there's no userspace yet out there which races a modeset with vblank ioctl calls. Then only issue would be userspace which does vblank waits on disabled ioctls which a) is buggy b) we can easily fix with a driver quirk flag if _really_ needed.
This way the drm_irq.c mess will at least converge a bit and so should help generic display servers (like Wayland) a lot.
I can volunteer for this if you want to punt on it yourself.
Much appreciated. I'll punt.
My hope was that other people would fix their own mess after seeing how i915 did it, and then we could rip out the crap, but if you're feeling the urge to do it all upfront I certainly won't object.
On Wed, Mar 05, 2014 at 02:38:25PM +0200, Ville Syrjälä wrote:
On Tue, Mar 04, 2014 at 10:24:54AM +0100, Daniel Vetter wrote:
On Fri, Feb 21, 2014 at 09:03:33PM +0200, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Allow the driver to specify whether all new vblank requests after drm_vblank_off() should be rejected. And add a counterpart called drm_vblank_on() which will again allow vblank requests to come in.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
Not really happy about this - drm_irq.c is already a giant mess, adding more driver-specific hacks doesn't help. I think we need a few bits of polish on top of your code:
Add stern warnings to pre/post_modeset that they're inherently racy.
Add calls to drm_vblank_off to every driver lacking them. Put it at the beginning of their crtc disable functions expect when there's a call to pre_modeset. Then it should be right after that.
Sprinkle calls to drm_vblank_on over all drivers. Put them at the end of their crtc enable functions except when there's a call to post_modeset. Then put it right before that.
Rip out the reject argument again and make it the default. If we have drm_vblank_off everywhere then all old vblank waits should complete and there's no userspace yet out there which races a modeset with vblank ioctl calls. Then only issue would be userspace which does vblank waits on disabled ioctls which a) is buggy b) we can easily fix with a driver quirk flag if _really_ needed.
This way the drm_irq.c mess will at least converge a bit and so should help generic display servers (like Wayland) a lot.
I can volunteer for this if you want to punt on it yourself.
Much appreciated. I'll punt.
My hope was that other people would fix their own mess after seeing how i915 did it, and then we could rip out the crap, but if you're feeling the urge to do it all upfront I certainly won't object.
My experience tells me that the only way to fix a cross-driver mess is to simple charge ahead. If driver maintainers are asleep they'll end up with a broken driver, but that usually gets their attention. Pleas and praying don't ;-) -Daniel
From: Ville Syrjälä ville.syrjala@linux.intel.com
If someone holds a vblank reference across the modeset, and after/during the modeset someone tries to grab a vblank reference, the current code won't re-enable the vblank interrupts. That's not good, so instead allow the driver to choose whether drm_vblank_get() should always enable the interrupts regardless of the refcount.
Combined with the drm_vblank_off/drm_vblank_on reject mechanism, this can also be used to allow drivers to use vblank interrupts during modeset, whether or not someone is currently holding a vblank reference.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 3 ++- include/drm/drmP.h | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 6e5d820..d613b6f 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -897,7 +897,8 @@ int drm_vblank_get(struct drm_device *dev, int crtc) }
/* Going from 0->1 means we have to enable interrupts again */ - if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) { + if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1 || + dev->vblank_always_enable_on_get) { spin_lock(&dev->vblank_time_lock); if (!dev->vblank[crtc].enabled) { /* Enable vblank irqs under vblank_time_lock protection. diff --git a/include/drm/drmP.h b/include/drm/drmP.h index ee40483..3eca0ee 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1156,6 +1156,12 @@ struct drm_device { */ bool vblank_disable_allowed;
+ /* + * Should a non-rejected drm_vblank_get() always enable the + * vblank interrupt regardless of the current refcount? + */ + bool vblank_always_enable_on_get; + /* array of size num_crtcs */ struct drm_vblank_crtc *vblank;
On Fri, 21 Feb 2014 21:03:34 +0200 ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
If someone holds a vblank reference across the modeset, and after/during the modeset someone tries to grab a vblank reference, the current code won't re-enable the vblank interrupts. That's not good, so instead allow the driver to choose whether drm_vblank_get() should always enable the interrupts regardless of the refcount.
Combined with the drm_vblank_off/drm_vblank_on reject mechanism, this can also be used to allow drivers to use vblank interrupts during modeset, whether or not someone is currently holding a vblank reference.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_irq.c | 3 ++- include/drm/drmP.h | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 6e5d820..d613b6f 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -897,7 +897,8 @@ int drm_vblank_get(struct drm_device *dev, int crtc) }
/* Going from 0->1 means we have to enable interrupts again */
- if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) {
- if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1 ||
dev->vblank_always_enable_on_get) {diff --git a/include/drm/drmP.h b/include/drm/drmP.h index ee40483..3eca0ee 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1156,6 +1156,12 @@ struct drm_device { */ bool vblank_disable_allowed;
- /*
* Should a non-rejected drm_vblank_get() always enable the* vblank interrupt regardless of the current refcount?*/- bool vblank_always_enable_on_get;
- /* array of size num_crtcs */ struct drm_vblank_crtc *vblank;
This seems like the sort of thing it would be good to have a test for... I'm surprised we haven't hit it yet. But in looking at the code I don't see where we'd re-enable things properly in this situation, so I guess it's a real bug.
Reviewed-by: Jesse Barnes jbarnes@virtuousgeek.org
On Fri, Feb 21, 2014 at 09:03:34PM +0200, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
If someone holds a vblank reference across the modeset, and after/during the modeset someone tries to grab a vblank reference, the current code won't re-enable the vblank interrupts. That's not good, so instead allow the driver to choose whether drm_vblank_get() should always enable the interrupts regardless of the refcount.
Combined with the drm_vblank_off/drm_vblank_on reject mechanism, this can also be used to allow drivers to use vblank interrupts during modeset, whether or not someone is currently holding a vblank reference.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_irq.c | 3 ++- include/drm/drmP.h | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 6e5d820..d613b6f 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -897,7 +897,8 @@ int drm_vblank_get(struct drm_device *dev, int crtc) }
/* Going from 0->1 means we have to enable interrupts again */
- if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) {
- if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1 ||
dev->vblank_always_enable_on_get) {diff --git a/include/drm/drmP.h b/include/drm/drmP.h index ee40483..3eca0ee 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1156,6 +1156,12 @@ struct drm_device { */ bool vblank_disable_allowed;
- /*
* Should a non-rejected drm_vblank_get() always enable the* vblank interrupt regardless of the current refcount?*/- bool vblank_always_enable_on_get;
Nack for this hack. Why can't drm_vblank_on not just re-enable the vblank interrupt if we still have a vblank reference? -Daniel
- /* array of size num_crtcs */ struct drm_vblank_crtc *vblank;
-- 1.8.3.2
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Mar 04, 2014 at 10:16:02AM +0100, Daniel Vetter wrote:
On Fri, Feb 21, 2014 at 09:03:34PM +0200, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
If someone holds a vblank reference across the modeset, and after/during the modeset someone tries to grab a vblank reference, the current code won't re-enable the vblank interrupts. That's not good, so instead allow the driver to choose whether drm_vblank_get() should always enable the interrupts regardless of the refcount.
Combined with the drm_vblank_off/drm_vblank_on reject mechanism, this can also be used to allow drivers to use vblank interrupts during modeset, whether or not someone is currently holding a vblank reference.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/drm_irq.c | 3 ++- include/drm/drmP.h | 6 ++++++ 2 files changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 6e5d820..d613b6f 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -897,7 +897,8 @@ int drm_vblank_get(struct drm_device *dev, int crtc) }
/* Going from 0->1 means we have to enable interrupts again */
- if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) {
- if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1 ||
dev->vblank_always_enable_on_get) {diff --git a/include/drm/drmP.h b/include/drm/drmP.h index ee40483..3eca0ee 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1156,6 +1156,12 @@ struct drm_device { */ bool vblank_disable_allowed;
- /*
* Should a non-rejected drm_vblank_get() always enable the* vblank interrupt regardless of the current refcount?*/- bool vblank_always_enable_on_get;
Nack for this hack. Why can't drm_vblank_on not just re-enable the vblank interrupt if we still have a vblank reference?
Hmm. Yeah that seems like a nicer way to go about it.
From: Ville Syrjälä ville.syrjala@linux.intel.com
Tell the drm core vblank code to reject drm_vblank_get()s only between drm_vblank_off() and drm_vblank_on() calls, and sprinkle the appropriate drm_vblank_on() calls to the .crtc_enable() hooks. At this time I kept the off calls in their current position, and added the on calls to the end of .crtc_enable(). Later on these will be moved inwards a bit to allow vblank interrupts during plane enable/disable steps.
We can kill of the drm_vblank_{pre,post}_modeset() calls since those are there simply to make drm_vblank_get() fail during a modeset. The way they do it is by grabbing a vblank reference, and after drm_vblank_off() gets called this will results in drm_vblank_get() failing due to the elevated refcount while vblank interrupts are disabled. Unfortunately this means there's no point during modeset where the behaviour can be restored back to the normal state until the vblank refcount drops to 0. There's no gurantee of that happening even after the modeset has completed, so simply dropping the drm_vblank_{pre,post}_modeset() calls is the best option. The new reject mechanism will take care of things in a much more consistent and race free manner.
Testcase: igt/kms_flip/{dpms,modeset}-vs-vblank-race Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/i915_dma.c | 6 ++++++ drivers/gpu/drm/i915/intel_display.c | 23 +++++++++++++++-------- 2 files changed, 21 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 7688abc..d5e27bb 100644 --- a/drivers/gpu/drm/i915/i915_dma.c +++ b/drivers/gpu/drm/i915/i915_dma.c @@ -1293,6 +1293,12 @@ static int i915_load_modeset_init(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; int ret;
+ /* + * Allow the use of vblank interrupts during modeset, + * and make the vblank code behaviour more consistent + */ + dev->vblank_always_enable_on_get = true; + ret = intel_parse_bios(dev); if (ret) DRM_INFO("failed to find VBIOS tables\n"); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index bab0d08..2933540 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3607,6 +3607,8 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) * happening. */ intel_wait_for_vblank(dev, intel_crtc->pipe); + + drm_vblank_on(dev, pipe); }
/* IPS only exists on ULT machines and is tied to pipe A. */ @@ -3632,6 +3634,8 @@ static void haswell_crtc_enable_planes(struct drm_crtc *crtc) mutex_lock(&dev->struct_mutex); intel_update_fbc(dev); mutex_unlock(&dev->struct_mutex); + + drm_vblank_on(dev, pipe); }
static void haswell_crtc_disable_planes(struct drm_crtc *crtc) @@ -3643,7 +3647,7 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc) int plane = intel_crtc->plane;
intel_crtc_wait_for_pending_flips(crtc); - drm_vblank_off(dev, pipe, false); + drm_vblank_off(dev, pipe, true);
/* FBC must be disabled before disabling the plane on HSW. */ if (dev_priv->fbc.plane == plane) @@ -3774,7 +3778,7 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) encoder->disable(encoder);
intel_crtc_wait_for_pending_flips(crtc); - drm_vblank_off(dev, pipe, false); + drm_vblank_off(dev, pipe, true);
if (dev_priv->fbc.plane == plane) intel_disable_fbc(dev); @@ -4160,6 +4164,8 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
for_each_encoder_on_crtc(dev, crtc, encoder) encoder->enable(encoder); + + drm_vblank_on(dev, pipe); }
static void i9xx_crtc_enable(struct drm_crtc *crtc) @@ -4205,6 +4211,8 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
for_each_encoder_on_crtc(dev, crtc, encoder) encoder->enable(encoder); + + drm_vblank_on(dev, pipe); }
static void i9xx_pfit_disable(struct intel_crtc *crtc) @@ -4239,7 +4247,7 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
/* Give the overlay scaler a chance to disable if it's on this pipe */ intel_crtc_wait_for_pending_flips(crtc); - drm_vblank_off(dev, pipe, false); + drm_vblank_off(dev, pipe, true);
if (dev_priv->fbc.plane == plane) intel_disable_fbc(dev); @@ -7035,15 +7043,10 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc, struct intel_encoder *encoder; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_display_mode *mode = &intel_crtc->config.requested_mode; - int pipe = intel_crtc->pipe; int ret;
- drm_vblank_pre_modeset(dev, pipe); - ret = dev_priv->display.crtc_mode_set(crtc, x, y, fb);
- drm_vblank_post_modeset(dev, pipe); - if (ret != 0) return ret;
@@ -11380,6 +11383,10 @@ void intel_modeset_setup_hw_state(struct drm_device *dev, crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); intel_sanitize_crtc(crtc); intel_dump_pipe_config(crtc, &crtc->config, "[setup_hw_state]"); + if (crtc->active) + drm_vblank_on(dev, crtc->pipe); + else + drm_vblank_off(dev, crtc->pipe, true); }
for (i = 0; i < dev_priv->num_shared_dpll; i++) {
On Fre, 2014-02-21 at 21:03 +0200, ville.syrjala@linux.intel.com wrote:
We can kill of the drm_vblank_{pre,post}_modeset() calls since those are there simply to make drm_vblank_get() fail during a modeset.
Actually, their original purpose was to keep the DRM vblank counter consistent across modesets, assuming the modeset resets the hardware vblank counter.
On Mon, Feb 24, 2014 at 12:48:55PM +0900, Michel Dänzer wrote:
On Fre, 2014-02-21 at 21:03 +0200, ville.syrjala@linux.intel.com wrote:
We can kill of the drm_vblank_{pre,post}_modeset() calls since those are there simply to make drm_vblank_get() fail during a modeset.
Actually, their original purpose was to keep the DRM vblank counter consistent across modesets, assuming the modeset resets the hardware vblank counter.
I see. Well, actually I really don't. The code is too funky for me to tell what it actually ends up doing. The obvious way would be to resample the hardware counter at drm_vblank_post_modeset(), which the code certainly doesn't do. But maybe it did something sensible in the past.
On Mon, 2014-02-24 at 14:11 +0200, Ville Syrjälä wrote:
On Mon, Feb 24, 2014 at 12:48:55PM +0900, Michel Dänzer wrote:
On Fre, 2014-02-21 at 21:03 +0200, ville.syrjala@linux.intel.com wrote:
We can kill of the drm_vblank_{pre,post}_modeset() calls since those are there simply to make drm_vblank_get() fail during a modeset.
Actually, their original purpose was to keep the DRM vblank counter consistent across modesets, assuming the modeset resets the hardware vblank counter.
I see. Well, actually I really don't. The code is too funky for me to tell what it actually ends up doing. The obvious way would be to resample the hardware counter at drm_vblank_post_modeset(), which the code certainly doesn't do. But maybe it did something sensible in the past.
When the pre/post-modeset hooks were originally added, it worked like this: the pre-modeset hook enabled the vblank interrupt, which updated the DRM vblank counter from the driver/HW counter. The post-modeset hook disabled the vblank interrupt again, which recorded the post-modeset driver/HW counter value.
But the vblank code has changed a lot since then, not sure it still works like that.
On Tue, 25 Feb 2014 11:58:26 +0900 Michel Dänzer michel@daenzer.net wrote:
On Mon, 2014-02-24 at 14:11 +0200, Ville Syrjälä wrote:
On Mon, Feb 24, 2014 at 12:48:55PM +0900, Michel Dänzer wrote:
On Fre, 2014-02-21 at 21:03 +0200, ville.syrjala@linux.intel.com wrote:
We can kill of the drm_vblank_{pre,post}_modeset() calls since those are there simply to make drm_vblank_get() fail during a modeset.
Actually, their original purpose was to keep the DRM vblank counter consistent across modesets, assuming the modeset resets the hardware vblank counter.
I see. Well, actually I really don't. The code is too funky for me to tell what it actually ends up doing. The obvious way would be to resample the hardware counter at drm_vblank_post_modeset(), which the code certainly doesn't do. But maybe it did something sensible in the past.
When the pre/post-modeset hooks were originally added, it worked like this: the pre-modeset hook enabled the vblank interrupt, which updated the DRM vblank counter from the driver/HW counter. The post-modeset hook disabled the vblank interrupt again, which recorded the post-modeset driver/HW counter value.
But the vblank code has changed a lot since then, not sure it still works like that.
Yeah it's changed a bit. I think Rob added the latest bits there. They were originally in place to support both UMS and KMS drivers, which is as ugly as it sounds.
As long as nothing breaks (vblank vs dpms, vblank vs modeset, vblank vs high precision timestamps, vblank vs refcount) I think your code is ok.
Cc'ing Mario for another opinion too. This makes me nervous but it seems ok.
I think you should update the docbook (with examples) as well so other driver writers will know how to use this stuff.
Reviewed-by: Jesse Barnes jbarnes@virtuousgeek.org
On Tue, Feb 25, 2014 at 11:58:26AM +0900, Michel Dänzer wrote:
On Mon, 2014-02-24 at 14:11 +0200, Ville Syrjälä wrote:
On Mon, Feb 24, 2014 at 12:48:55PM +0900, Michel Dänzer wrote:
On Fre, 2014-02-21 at 21:03 +0200, ville.syrjala@linux.intel.com wrote:
We can kill of the drm_vblank_{pre,post}_modeset() calls since those are there simply to make drm_vblank_get() fail during a modeset.
Actually, their original purpose was to keep the DRM vblank counter consistent across modesets, assuming the modeset resets the hardware vblank counter.
I see. Well, actually I really don't. The code is too funky for me to tell what it actually ends up doing. The obvious way would be to resample the hardware counter at drm_vblank_post_modeset(), which the code certainly doesn't do. But maybe it did something sensible in the past.
When the pre/post-modeset hooks were originally added, it worked like this: the pre-modeset hook enabled the vblank interrupt, which updated the DRM vblank counter from the driver/HW counter. The post-modeset hook disabled the vblank interrupt again, which recorded the post-modeset driver/HW counter value.
But the vblank code has changed a lot since then, not sure it still works like that.
It still works like that, but there's two fundamental issues with this trick: - There's a race where the vblank state is fubar right between the completion of the modeset and before the first vblank happened. - It doesn't work across suspend/resume since no one re-enables the vblank interrupt.
Cheers, Daniel
Digging out an ooold post of Daniel's...
On 04.03.2014 18:13, Daniel Vetter wrote:
On Tue, Feb 25, 2014 at 11:58:26AM +0900, Michel Dänzer wrote:
When the pre/post-modeset hooks were originally added, it worked like this: the pre-modeset hook enabled the vblank interrupt, which updated the DRM vblank counter from the driver/HW counter. The post-modeset hook disabled the vblank interrupt again, which recorded the post-modeset driver/HW counter value.
But the vblank code has changed a lot since then, not sure it still works like that.
It still works like that, but there's two fundamental issues with this trick:
- There's a race where the vblank state is fubar right between the completion of the modeset and before the first vblank happened.
Can you provide more details about that? You mentioned on IRC that sometimes 'bogus' DRM vblank counter values are returned to userspace. The most likely cause of that would be drm_vblank_pre_modeset() being called too late, i.e. after the hardware counter was reset. (Or if you're reducing / eliminating the vblank disable timer, possibly the vblank interrupt getting disabled too early, i.e. before the hardware counter was reset)
Speaking of reducing or disabling the vblank disable timer, that should be possible with drm_vblank_pre/post_modeset() as well.
- It doesn't work across suspend/resume since no one re-enables the vblank interrupt.
That sounds like a driver bug to me. The driver should re-enable the hardware interrupt on resume if the vblank interrupt is currently enabled by the DRM core. This seems to work fine for me with radeon.
So, I'm afraid I'm still not convinced that the new vblank_off/on() functions and the ever-increasing series of fixes for problems related to them are the right (let alone only) solution for your problems.
On Wed, May 28, 2014 at 06:12:54PM +0900, Michel Dänzer wrote:
Digging out an ooold post of Daniel's...
On 04.03.2014 18:13, Daniel Vetter wrote:
On Tue, Feb 25, 2014 at 11:58:26AM +0900, Michel Dänzer wrote:
When the pre/post-modeset hooks were originally added, it worked like this: the pre-modeset hook enabled the vblank interrupt, which updated the DRM vblank counter from the driver/HW counter. The post-modeset hook disabled the vblank interrupt again, which recorded the post-modeset driver/HW counter value.
But the vblank code has changed a lot since then, not sure it still works like that.
It still works like that, but there's two fundamental issues with this trick:
- There's a race where the vblank state is fubar right between the completion of the modeset and before the first vblank happened.
Can you provide more details about that? You mentioned on IRC that sometimes 'bogus' DRM vblank counter values are returned to userspace. The most likely cause of that would be drm_vblank_pre_modeset() being called too late, i.e. after the hardware counter was reset. (Or if you're reducing / eliminating the vblank disable timer, possibly the vblank interrupt getting disabled too early, i.e. before the hardware counter was reset)
The hardware counter reset is a problem: 1. vblank_disable_and_save() updates .last 2. modeset/dpms/suspend (hw counter is reset) 3. drm_vblank_get() -> cur_vblank-.last == garbage
The lack of drm_vblank_on() is a problem: 1. drm_vblank_get() 2. drm_vblank_off() 3. modeset/dpms/suspend 4. drm_vblank_get() -> -EINVAL
Another issue: 1. drm_vblank_get() 2. drm_vblank_put() 3. disable timer expires which updates .last ... 4. drm_vblank_off() updates .last again 5. modeset/dpms/suspend 6. drm_vblank_get() -> sequence number doesn't account for the time between 3. and 4. I suppose this isn't a big issue, but I don't like leaking implementation details (the timer delay) into the sequence number.
Now this last one should actually work with the current drm_vblank_pre_modeset() since it does a drm_vblank_get() which will apply the cur_vblank-.last diff, but it also enables the vblank interrupt which is entirely pointless, and also wrong on Intel hardware (well, if we didn't have drm_vblank_off()). Our docs say that we shouldn't have the vblank interrupt enabled+unmasked while the pipe is off.
Anyway it's not a very obvious way to do things. Ie. you're doing the drm_vblank_get() not because you actually want vblank interrupts, but because you want the side effects. I usually prefer straightforward code to magic.
Speaking of reducing or disabling the vblank disable timer, that should be possible with drm_vblank_pre/post_modeset() as well.
I get the impression that you're a bit hung up on the names :) We could rename the off/on to pre/post_modeset if you like, but then someone gets to audit all the other drivers. That someone isn't going to be me.
- It doesn't work across suspend/resume since no one re-enables the vblank interrupt.
That sounds like a driver bug to me. The driver should re-enable the hardware interrupt on resume if the vblank interrupt is currently enabled by the DRM core.
The interrupt is not enabled due to drm_vblank_off(). There's nothing to undo that which is why I added drm_vblank_on().
On 28.05.2014 20:19, Ville Syrjälä wrote:
On Wed, May 28, 2014 at 06:12:54PM +0900, Michel Dänzer wrote:
Digging out an ooold post of Daniel's...
On 04.03.2014 18:13, Daniel Vetter wrote:
On Tue, Feb 25, 2014 at 11:58:26AM +0900, Michel Dänzer wrote:
When the pre/post-modeset hooks were originally added, it worked like this: the pre-modeset hook enabled the vblank interrupt, which updated the DRM vblank counter from the driver/HW counter. The post-modeset hook disabled the vblank interrupt again, which recorded the post-modeset driver/HW counter value.
But the vblank code has changed a lot since then, not sure it still works like that.
It still works like that, but there's two fundamental issues with this trick:
- There's a race where the vblank state is fubar right between the completion of the modeset and before the first vblank happened.
Can you provide more details about that? You mentioned on IRC that sometimes 'bogus' DRM vblank counter values are returned to userspace. The most likely cause of that would be drm_vblank_pre_modeset() being called too late, i.e. after the hardware counter was reset. (Or if you're reducing / eliminating the vblank disable timer, possibly the vblank interrupt getting disabled too early, i.e. before the hardware counter was reset)
The hardware counter reset is a problem:
- vblank_disable_and_save() updates .last
- modeset/dpms/suspend (hw counter is reset)
- drm_vblank_get() -> cur_vblank-.last == garbage
The lack of drm_vblank_on() is a problem:
- drm_vblank_get()
- drm_vblank_off()
- modeset/dpms/suspend
- drm_vblank_get() -> -EINVAL
I'd summarize these as 'drm_vblank_off() considered harmful'.
Another issue:
- drm_vblank_get()
- drm_vblank_put()
- disable timer expires which updates .last
... 4. drm_vblank_off() updates .last again 5. modeset/dpms/suspend 6. drm_vblank_get() -> sequence number doesn't account for the time between 3. and 4. I suppose this isn't a big issue, but I don't like leaking implementation details (the timer delay) into the sequence number.
Yes, I guess drm_vblank_off() shouldn't call vblank_disable_and_save() if vblank is already disabled.
Now this last one should actually work with the current drm_vblank_pre_modeset() since it does a drm_vblank_get() which will apply the cur_vblank-.last diff, but it also enables the vblank interrupt which is entirely pointless, and also wrong on Intel hardware (well, if we didn't have drm_vblank_off()). Our docs say that we shouldn't have the vblank interrupt enabled+unmasked while the pipe is off.
That's a driver implementation detail. The driver isn't required to keep the hardware interrupt enabled all the time between the enable_vblank() and disable_vblank() hook calls. The DRM core just wants drm_handle_vblank() to be called for any vertical blank periods between them.
Anyway it's not a very obvious way to do things. Ie. you're doing the drm_vblank_get() not because you actually want vblank interrupts, but because you want the side effects.
No, that's not the only reason. It's also so that drm_handle_vblank() is called for any vertical blank periods occurring while the hardware counter might reset, so the DRM vblank counter gets incremented independently from the hardware counter.
Speaking of reducing or disabling the vblank disable timer, that should be possible with drm_vblank_pre/post_modeset() as well.
I get the impression that you're a bit hung up on the names :)
Not at all. In fact, the pre/post_modeset names are slightly misleading, as they're not only about modesetting but about preventing the DRM vblank counter from jumping due to hardware counter jumps.
We could rename the off/on to pre/post_modeset if you like, but then someone gets to audit all the other drivers. That someone isn't going to be me.
I appreciate your caution wrt other drivers, but I'm worried that having a second mechanism for keeping the DRM vblank counter consistent might cause subtle problems for drivers using the existing mechanism anyway.
On Thu, May 29, 2014 at 6:11 AM, Michel Dänzer michel@daenzer.net wrote:
We could rename the off/on to pre/post_modeset if you like, but then someone gets to audit all the other drivers. That someone isn't going to be me.
I appreciate your caution wrt other drivers, but I'm worried that having a second mechanism for keeping the DRM vblank counter consistent might cause subtle problems for drivers using the existing mechanism anyway.
I think that risk is rather low. Only i915 has been stupid enough to use both drm_vblank_off + pre/post_modeset in the normal crtc enable/disable functions. Everyone else uses either or the other, except for tegra which uses pre/post in the ->prepare/commit hooks and off in the crtc->disable callback. So should still get consistent results (since after ->disable vblank consistency stops mattering).
The reason we do all this is that we want to kill the delayed vblank put for i915, which is possible if you have a hw vblank counter. There's apparently more stuff wrong here still, so while we wrestle around probably best to leave other drivers out. I guess once this is all done I have to roll it out across all drivers so that we have consistent behaviour again ... -Daniel
On 29.05.2014 19:56, Daniel Vetter wrote:
On Thu, May 29, 2014 at 6:11 AM, Michel Dänzer michel@daenzer.net wrote:
We could rename the off/on to pre/post_modeset if you like, but then someone gets to audit all the other drivers. That someone isn't going to be me.
I appreciate your caution wrt other drivers, but I'm worried that having a second mechanism for keeping the DRM vblank counter consistent might cause subtle problems for drivers using the existing mechanism anyway.
I think that risk is rather low. Only i915 has been stupid enough to use both drm_vblank_off + pre/post_modeset in the normal crtc enable/disable functions. Everyone else uses either or the other, except for tegra which uses pre/post in the ->prepare/commit hooks and off in the crtc->disable callback. So should still get consistent results (since after ->disable vblank consistency stops mattering).
The reason we do all this is that we want to kill the delayed vblank put for i915, which is possible if you have a hw vblank counter.
That should also be possible with pre/post_modeset. (Not sure eliminating it completely is a good idea for the reasons you mentioned before, but at least decreasing it significantly should be no problem. I think even dropping the default a lot for all drivers should be rather low risk)
There's apparently more stuff wrong here still, so while we wrestle around probably best to leave other drivers out. I guess once this is all done I have to roll it out across all drivers so that we have consistent behaviour again ...
Since AFAICT radeon isn't affected by the problem drm_vblank_off() was supposed to solve originally, I'd prefer if it didn't start using that and potentially suffer from all the trouble it seems to cause.
On Fri, Feb 21, 2014 at 09:03:35PM +0200, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Tell the drm core vblank code to reject drm_vblank_get()s only between drm_vblank_off() and drm_vblank_on() calls, and sprinkle the appropriate drm_vblank_on() calls to the .crtc_enable() hooks. At this time I kept the off calls in their current position, and added the on calls to the end of .crtc_enable(). Later on these will be moved inwards a bit to allow vblank interrupts during plane enable/disable steps.
We can kill of the drm_vblank_{pre,post}_modeset() calls since those are there simply to make drm_vblank_get() fail during a modeset. The way they do it is by grabbing a vblank reference, and after drm_vblank_off() gets called this will results in drm_vblank_get() failing due to the elevated refcount while vblank interrupts are disabled. Unfortunately this means there's no point during modeset where the behaviour can be restored back to the normal state until the vblank refcount drops to 0. There's no gurantee of that happening even after the modeset has completed, so simply dropping the drm_vblank_{pre,post}_modeset() calls is the best option. The new reject mechanism will take care of things in a much more consistent and race free manner.
Testcase: igt/kms_flip/{dpms,modeset}-vs-vblank-race
QA hit the new tests and filed a bug.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75593
<snip>
On Fri, Feb 28, 2014 at 10:56:20AM +0200, Ville Syrjälä wrote:
On Fri, Feb 21, 2014 at 09:03:35PM +0200, ville.syrjala@linux.intel.com wrote:
From: Ville Syrjälä ville.syrjala@linux.intel.com
Tell the drm core vblank code to reject drm_vblank_get()s only between drm_vblank_off() and drm_vblank_on() calls, and sprinkle the appropriate drm_vblank_on() calls to the .crtc_enable() hooks. At this time I kept the off calls in their current position, and added the on calls to the end of .crtc_enable(). Later on these will be moved inwards a bit to allow vblank interrupts during plane enable/disable steps.
We can kill of the drm_vblank_{pre,post}_modeset() calls since those are there simply to make drm_vblank_get() fail during a modeset. The way they do it is by grabbing a vblank reference, and after drm_vblank_off() gets called this will results in drm_vblank_get() failing due to the elevated refcount while vblank interrupts are disabled. Unfortunately this means there's no point during modeset where the behaviour can be restored back to the normal state until the vblank refcount drops to 0. There's no gurantee of that happening even after the modeset has completed, so simply dropping the drm_vblank_{pre,post}_modeset() calls is the best option. The new reject mechanism will take care of things in a much more consistent and race free manner.
Testcase: igt/kms_flip/{dpms,modeset}-vs-vblank-race
QA hit the new tests and filed a bug.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75593
I have another test request since you've just fixed this bug:
Across suspend/resume the vblank state restoring through the pre/post_modeset hacks isn't just racy but flat-out doesn't work. Keith Packard stumbled over this when working on his present extension. So I think since you've (likely) also fixed this, we also should have a testcase for it. -Daniel
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Jesse Barnes -
Michel Dänzer -
Ville Syrjälä -
ville.syrjala＠linux.intel.com