[PATCH 0/5] drm: Allow vblank interrupts during modeset and make the code less racy

[PATCH 0/5] drm: Allow vblank interrupts during modeset and make the code less racy

[ville.syrjala]

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(-)

-- 
1.8.3.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/5] drm: Use correct spinlock flavor in drm_vblank_get()

[ville.syrjala]

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);
-- 
1.8.3.2

[PATCH 2/5] drm: Make the vblank disable timer per-crtc

[ville.syrjala]

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 */
 
-- 
1.8.3.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/5] drm: Allow the driver to reject vblank requests only when it really has the vblank interrupts disabled

[ville.syrjala]

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);
-- 
1.8.3.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/5] drm: Allow reenabling of vblank interrupts even if refcount>0

[ville.syrjala]

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;
 
-- 
1.8.3.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races

[ville.syrjala]

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++) {
-- 
1.8.3.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 1/5] drm: Use correct spinlock flavor in drm_vblank_get()

[Jesse Barnes]

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);
>  		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);

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/5] drm: Make the vblank disable timer per-crtc

[Jesse Barnes]

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);
>  	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 */
>  

Yeah this looks like a good fix.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm: Allow the driver to reject vblank requests only when it really has the vblank interrupts disabled

[Jesse Barnes]

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>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm: Allow reenabling of vblank interrupts even if refcount>0

[Jesse Barnes]

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) {
>  		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;
>  

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>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races

[Jesse Barnes]

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>

-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races

[Ville Syrjälä]

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>

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/5] drm: Allow reenabling of vblank interrupts even if refcount>0

[Daniel Vetter]

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) {
>  		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;

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/5] drm: Allow the driver to reject vblank requests only when it really has the vblank interrupts disabled

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/5] drm: Allow reenabling of vblank interrupts even if refcount>0

[Ville Syrjälä]

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) {
> >  		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;
> 
> 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.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/5] drm: Allow the driver to reject vblank requests only when it really has the vblank interrupts disabled

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/5] drm: Allow the driver to reject vblank requests only when it really has the vblank interrupts disabled

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer

Re: [Intel-gfx] [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races

[Ville Syrjälä]

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().

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races

[Michel Dänzer]

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:
> 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

I'd summarize these as 'drm_vblank_off() considered harmful'.


> 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.

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.


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer

Re: [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Allow vblank interrupts during modeset and eliminate some vblank races

[Michel Dänzer]

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.


-- 
Earthling Michel Dänzer            |                  http://www.amd.com
Libre software enthusiast          |                Mesa and X developer
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx