[PATCH 0/3] drm: Allow vblank interrupts during modeset and make the code less racy, take two
From: Ville Syrjälä ville.syrjala@linux.intel.com
This series is a continuation of the earlier attempt [1]. The first two patches from that series are still needed, but since they were unchanged I didn't include them here. The rest of the original series should be scrapped.
This time I dropped the 'reject' and 'vblank_always_enable_on_get' and instead did things like Daniel suggested. This does change the behaviour of blocking vblank waits for all drivers to return when the vblank interrupt gets disabled, whereas the old series made everything purely opt-in. To fix up the remaining drivers fully, someone needs to go and sprinkle the drm_vblank_on() calls there. Daniel volunteered to do that, so I'm not going to bother.
[1] http://lists.freedesktop.org/archives/dri-devel/2014-February/054372.html
Ville Syrjälä (3): drm: Make blocking vblank wait return when the vblank interrupts get disabled drm: Add drm_vblank_on() drm/i915: Use drm_vblank_on() to re-enable vblank interrupts
drivers/gpu/drm/drm_irq.c | 73 ++++++++++++++++++++++++++---------- drivers/gpu/drm/i915/intel_display.c | 13 ++++--- include/drm/drmP.h | 1 + 3 files changed, 63 insertions(+), 24 deletions(-)
From: Ville Syrjälä ville.syrjala@linux.intel.com
If there's a blocking vblank wait in progress while the vblank interrupt gets disabled, the current code will just let the vblank wait time out. Instead make it return immediately when vblank interrupts get disabled.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 3211158..6c6a81b 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1224,6 +1224,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].enabled || !dev->irq_enabled));
if (ret != -EINTR) {
From: Ville Syrjälä ville.syrjala@linux.intel.com
drm_vblank_off() will turn off vblank interrupts, but as long as the refcount is elevated drm_vblank_get() will not re-enable them. This is a problem is someone is holding a vblank reference while a modeset is happening, and the driver requires vblank interrupt to work during that time.
Add drm_vblank_on() as a counterpart to drm_vblank_off() which will re-enabled vblank interrupts if the refcount is already elevated. This will allow drivers to choose the specific places in the modeset sequence at which vblank interrupts get disabled and enabled.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_irq.c | 72 ++++++++++++++++++++++++++++++++++------------- include/drm/drmP.h | 1 + 2 files changed, 54 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index 6c6a81b..15f45b9 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -874,6 +874,41 @@ static void drm_update_vblank_count(struct drm_device *dev, int crtc) }
/** + * drm_vblank_enable - enable the vblank interrupt on a CRTC + * @dev: DRM device + * @crtc: CRTC in question + */ +static int drm_vblank_enable(struct drm_device *dev, int crtc) +{ + int ret = 0; + + assert_spin_locked(&dev->vbl_lock); + + 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 + * until we are done reinitializing master counter and + * timestamps. Filtercode in drm_handle_vblank() will + * prevent double-accounting of same vblank interval. + */ + ret = dev->driver->enable_vblank(dev, crtc); + DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n", crtc, ret); + if (ret) + atomic_dec(&dev->vblank[crtc].refcount); + else { + dev->vblank[crtc].enabled = true; + drm_update_vblank_count(dev, crtc); + } + } + + spin_unlock(&dev->vblank_time_lock); + + return ret; +} + +/** * drm_vblank_get - get a reference count on vblank events * @dev: DRM device * @crtc: which CRTC to own @@ -892,25 +927,7 @@ int drm_vblank_get(struct drm_device *dev, int crtc) 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(&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 - * until we are done reinitializing master counter and - * timestamps. Filtercode in drm_handle_vblank() will - * prevent double-accounting of same vblank interval. - */ - ret = dev->driver->enable_vblank(dev, crtc); - DRM_DEBUG("enabling vblank on crtc %d, ret: %d\n", - crtc, ret); - if (ret) - atomic_dec(&dev->vblank[crtc].refcount); - else { - dev->vblank[crtc].enabled = true; - drm_update_vblank_count(dev, crtc); - } - } - spin_unlock(&dev->vblank_time_lock); + ret = drm_vblank_enable(dev, crtc); } else { if (!dev->vblank[crtc].enabled) { atomic_dec(&dev->vblank[crtc].refcount); @@ -980,6 +997,23 @@ 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); + /* re-enable interrupts if there's are users left */ + if (atomic_read(&dev->vblank[crtc].refcount) != 0) + WARN_ON(drm_vblank_enable(dev, crtc)); + 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 * @crtc: CRTC in question diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 1b04298..72dff8b 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -1404,6 +1404,7 @@ 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_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);
From: Ville Syrjälä ville.syrjala@linux.intel.com
Sprinkle drm_vblank_on() calls to the .crtc_enable() callbacks. Also drop the drm_vblank_{pre,post}_modeset() calls since those are now useless.
Testcase: igt/kms_flip/{dpms,modeset}-vs-vblank-race Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/i915/intel_display.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7e8bfd8..87895dd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3612,6 +3612,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. */ @@ -3637,6 +3639,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) @@ -4235,6 +4239,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) @@ -4280,6 +4286,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) @@ -7014,15 +7022,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;
dri-devel@lists.freedesktop.org
-
ville.syrjala＠linux.intel.com