[PATCH 1/2] drm: Mark up accesses of vblank->enabled outside of its spinlock

[PATCH 1/2] drm: Mark up accesses of vblank->enabled outside of its spinlock

[Chris Wilson]

Order the update to vblank->enabled after the timestamp is primed so
that a concurrent unlocked reader will only see the vblank->enabled with
the current timestamp.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/drm_irq.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 53a526c7b24d..4cc9352ab6a8 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -336,10 +336,8 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
 	 * calling the ->disable_vblank() operation in atomic context with the
 	 * hardware potentially runtime suspended.
 	 */
-	if (vblank->enabled) {
+	if (cmpxchg_relaxed(&vblank->enabled, true, false))
 		__disable_vblank(dev, pipe);
-		vblank->enabled = false;
-	}
 
 	/*
 	 * Always update the count and timestamp to maintain the
@@ -360,7 +358,7 @@ static void vblank_disable_fn(unsigned long arg)
 	unsigned long irqflags;
 
 	spin_lock_irqsave(&dev->vbl_lock, irqflags);
-	if (atomic_read(&vblank->refcount) == 0 && vblank->enabled) {
+	if (atomic_read(&vblank->refcount) == 0 && READ_ONCE(vblank->enabled)) {
 		DRM_DEBUG("disabling vblank on crtc %u\n", pipe);
 		vblank_disable_and_save(dev, pipe);
 	}
@@ -384,7 +382,7 @@ void drm_vblank_cleanup(struct drm_device *dev)
 	for (pipe = 0; pipe < dev->num_crtcs; pipe++) {
 		struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 
-		WARN_ON(vblank->enabled &&
+		WARN_ON(READ_ONCE(vblank->enabled) &&
 			drm_core_check_feature(dev, DRIVER_MODESET));
 
 		del_timer_sync(&vblank->disable_timer);
@@ -564,7 +562,7 @@ int drm_irq_uninstall(struct drm_device *dev)
 		for (i = 0; i < dev->num_crtcs; i++) {
 			struct drm_vblank_crtc *vblank = &dev->vblank[i];
 
-			if (!vblank->enabled)
+			if (!READ_ONCE(vblank->enabled))
 				continue;
 
 			WARN_ON(drm_core_check_feature(dev, DRIVER_MODESET));
@@ -1105,11 +1103,16 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
 		 */
 		ret = __enable_vblank(dev, pipe);
 		DRM_DEBUG("enabling vblank on crtc %u, ret: %d\n", pipe, ret);
-		if (ret)
+		if (ret) {
 			atomic_dec(&vblank->refcount);
-		else {
-			vblank->enabled = true;
+		} else {
 			drm_update_vblank_count(dev, pipe, 0);
+			/* drm_update_vblank_count() includes a wmb so we just
+			 * need to ensure that the compiler emits the write
+			 * to mark the vblank as enabled after the call
+			 * to drm_update_vblank_count().
+			 */
+			WRITE_ONCE(vblank->enabled, true);
 		}
 	}
 
@@ -1148,7 +1151,7 @@ static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
 	if (atomic_add_return(1, &vblank->refcount) == 1) {
 		ret = drm_vblank_enable(dev, pipe);
 	} else {
-		if (!vblank->enabled) {
+		if (!READ_ONCE(vblank->enabled)) {
 			atomic_dec(&vblank->refcount);
 			ret = -EINVAL;
 		}
@@ -1517,7 +1520,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
 	 * vblank disable, so no need for further locking.  The reference from
 	 * drm_vblank_get() protects against vblank disable from another source.
 	 */
-	if (!vblank->enabled) {
+	if (!READ_ONCE(vblank->enabled)) {
 		ret = -EINVAL;
 		goto err_unlock;
 	}
@@ -1644,7 +1647,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 		DRM_WAIT_ON(ret, vblank->queue, 3 * HZ,
 			    (((drm_vblank_count(dev, pipe) -
 			       vblwait->request.sequence) <= (1 << 23)) ||
-			     !vblank->enabled ||
+			     !READ_ONCE(vblank->enabled) ||
 			     !dev->irq_enabled));
 	}
 
@@ -1714,6 +1717,9 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
 	if (WARN_ON(pipe >= dev->num_crtcs))
 		return false;
 
+	if (!READ_ONCE(vblank->enabled))
+		return false;
+
 	spin_lock_irqsave(&dev->event_lock, irqflags);
 
 	/* Need timestamp lock to prevent concurrent execution with
-- 
2.11.0

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

[PATCH 2/2] drm: Peek at the current counter/timestamp for vblank queries

[Chris Wilson]

Bypass all the spinlocks and return the last timestamp and counter from
the last vblank if the driver delcares that it is accurate (and stable
across on/off), and the vblank is currently enabled.

This is dependent upon the both the hardware and driver to provide the
proper barriers to facilitate reading our bookkeeping outside of the
vblank interrupt and outside of the explicit vblank locks.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Michel Dänzer <michel@daenzer.net>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Dave Airlie <airlied@redhat.com>,
Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
---
 drivers/gpu/drm/drm_irq.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 4cc9352ab6a8..b4bd361a2bcc 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1562,6 +1562,17 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
 	return ret;
 }
 
+static bool drm_wait_vblank_is_query(union drm_wait_vblank *vblwait)
+{
+	if (vblwait->request.sequence)
+		return false;
+
+	return _DRM_VBLANK_RELATIVE ==
+		(vblwait->request.type & (_DRM_VBLANK_TYPES_MASK |
+					  _DRM_VBLANK_EVENT |
+					  _DRM_VBLANK_NEXTONMISS));
+}
+
 /*
  * Wait for VBLANK.
  *
@@ -1611,6 +1622,21 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 
 	vblank = &dev->vblank[pipe];
 
+	/* If the counter is currently enabled and accurate, short-circuit queries
+	 * to return the cached timestamp of the last vblank.
+	 */
+	if (dev->vblank_disable_immediate &&
+	    drm_wait_vblank_is_query(vblwait) &&
+	    vblank->enabled) {
+		struct timeval now;
+
+		vblwait->reply.sequence =
+			drm_vblank_count_and_time(dev, pipe, &now);
+		vblwait->reply.tval_sec = now.tv_sec;
+		vblwait->reply.tval_usec = now.tv_usec;
+		return 0;
+	}
+
 	ret = drm_vblank_get(dev, pipe);
 	if (ret) {
 		DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
-- 
2.11.0

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

✗ Fi.CI.BAT: failure for series starting with [1/2] drm: Mark up accesses of vblank->enabled outside of its spinlock

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm: Mark up accesses of vblank->enabled outside of its spinlock
URL   : https://patchwork.freedesktop.org/series/21410/
State : failure

== Summary ==

Series 21410v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/21410/revisions/1/mbox/

Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                pass       -> FAIL       (fi-snb-2600) fdo#100007
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-bxt-t5700) fdo#100125
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (fi-skl-6770hq)
Test pm_rps:
        Subgroup basic-api:
                pass       -> INCOMPLETE (fi-skl-6770hq)

fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 456s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 573s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 533s
fi-bxt-t5700     total:278  pass:257  dwarn:1   dfail:0   fail:0   skip:20  time: 552s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 498s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 505s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 435s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 432s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 444s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 505s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 476s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 466s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 488s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 596s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 480s
fi-skl-6770hq    total:243  pass:232  dwarn:1   dfail:0   fail:0   skip:9   time: 0s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 547s
fi-snb-2600      total:278  pass:248  dwarn:0   dfail:0   fail:1   skip:29  time: 421s

99bd31bdd4d2c4c07a9eef33c9c68611a315c5b0 drm-tip: 2017y-03m-16d-22h-22m-27s UTC integration manifest
38f5f83 drm: Peek at the current counter/timestamp for vblank queries
1d381f8 drm: Mark up accesses of vblank->enabled outside of its spinlock

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4211/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: Mark up accesses of vblank->enabled outside of its spinlock

[Ville Syrjälä]

On Thu, Mar 16, 2017 at 11:47:48PM +0000, Chris Wilson wrote:
> Order the update to vblank->enabled after the timestamp is primed so
> that a concurrent unlocked reader will only see the vblank->enabled with
> the current timestamp.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_irq.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 53a526c7b24d..4cc9352ab6a8 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -336,10 +336,8 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
>  	 * calling the ->disable_vblank() operation in atomic context with the
>  	 * hardware potentially runtime suspended.
>  	 */
> -	if (vblank->enabled) {
> +	if (cmpxchg_relaxed(&vblank->enabled, true, false))
>  		__disable_vblank(dev, pipe);
> -		vblank->enabled = false;
> -	}
>  
>  	/*
>  	 * Always update the count and timestamp to maintain the
> @@ -360,7 +358,7 @@ static void vblank_disable_fn(unsigned long arg)
>  	unsigned long irqflags;
>  
>  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> -	if (atomic_read(&vblank->refcount) == 0 && vblank->enabled) {
> +	if (atomic_read(&vblank->refcount) == 0 && READ_ONCE(vblank->enabled)) {

Hmm. Aren't most of these accesses inside the lock? Looks like you're
marking everything READ/WRITE_ONCE()?

>  		DRM_DEBUG("disabling vblank on crtc %u\n", pipe);
>  		vblank_disable_and_save(dev, pipe);
>  	}
> @@ -384,7 +382,7 @@ void drm_vblank_cleanup(struct drm_device *dev)
>  	for (pipe = 0; pipe < dev->num_crtcs; pipe++) {
>  		struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>  
> -		WARN_ON(vblank->enabled &&
> +		WARN_ON(READ_ONCE(vblank->enabled) &&
>  			drm_core_check_feature(dev, DRIVER_MODESET));
>  
>  		del_timer_sync(&vblank->disable_timer);
> @@ -564,7 +562,7 @@ int drm_irq_uninstall(struct drm_device *dev)
>  		for (i = 0; i < dev->num_crtcs; i++) {
>  			struct drm_vblank_crtc *vblank = &dev->vblank[i];
>  
> -			if (!vblank->enabled)
> +			if (!READ_ONCE(vblank->enabled))
>  				continue;
>  
>  			WARN_ON(drm_core_check_feature(dev, DRIVER_MODESET));
> @@ -1105,11 +1103,16 @@ static int drm_vblank_enable(struct drm_device *dev, unsigned int pipe)
>  		 */
>  		ret = __enable_vblank(dev, pipe);
>  		DRM_DEBUG("enabling vblank on crtc %u, ret: %d\n", pipe, ret);
> -		if (ret)
> +		if (ret) {
>  			atomic_dec(&vblank->refcount);
> -		else {
> -			vblank->enabled = true;
> +		} else {
>  			drm_update_vblank_count(dev, pipe, 0);
> +			/* drm_update_vblank_count() includes a wmb so we just
> +			 * need to ensure that the compiler emits the write
> +			 * to mark the vblank as enabled after the call
> +			 * to drm_update_vblank_count().
> +			 */
> +			WRITE_ONCE(vblank->enabled, true);

Ordering+barrier looks correct to me.

>  		}
>  	}
>  
> @@ -1148,7 +1151,7 @@ static int drm_vblank_get(struct drm_device *dev, unsigned int pipe)
>  	if (atomic_add_return(1, &vblank->refcount) == 1) {
>  		ret = drm_vblank_enable(dev, pipe);
>  	} else {
> -		if (!vblank->enabled) {
> +		if (!READ_ONCE(vblank->enabled)) {
>  			atomic_dec(&vblank->refcount);
>  			ret = -EINVAL;
>  		}
> @@ -1517,7 +1520,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  	 * vblank disable, so no need for further locking.  The reference from
>  	 * drm_vblank_get() protects against vblank disable from another source.
>  	 */
> -	if (!vblank->enabled) {
> +	if (!READ_ONCE(vblank->enabled)) {
>  		ret = -EINVAL;
>  		goto err_unlock;
>  	}
> @@ -1644,7 +1647,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
>  		DRM_WAIT_ON(ret, vblank->queue, 3 * HZ,
>  			    (((drm_vblank_count(dev, pipe) -
>  			       vblwait->request.sequence) <= (1 << 23)) ||
> -			     !vblank->enabled ||
> +			     !READ_ONCE(vblank->enabled) ||
>  			     !dev->irq_enabled));
>  	}
>  
> @@ -1714,6 +1717,9 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
>  	if (WARN_ON(pipe >= dev->num_crtcs))
>  		return false;
>  
> +	if (!READ_ONCE(vblank->enabled))
> +		return false;

This to me looks like it could theoretically cause us to
miss an interrupt.

1. enable_irq()
2. drm_update_vblank_count()
3. irq fires
4. drm_handle_vblank() doesn't do anything
5. enabled=true

> +
>  	spin_lock_irqsave(&dev->event_lock, irqflags);
>  
>  	/* Need timestamp lock to prevent concurrent execution with
> -- 
> 2.11.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/2] drm: Peek at the current counter/timestamp for vblank queries

[Ville Syrjälä]

On Thu, Mar 16, 2017 at 11:47:49PM +0000, Chris Wilson wrote:
> Bypass all the spinlocks and return the last timestamp and counter from
> the last vblank if the driver delcares that it is accurate (and stable
> across on/off), and the vblank is currently enabled.
> 
> This is dependent upon the both the hardware and driver to provide the
> proper barriers to facilitate reading our bookkeeping outside of the
> vblank interrupt and outside of the explicit vblank locks.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Michel Dänzer <michel@daenzer.net>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Dave Airlie <airlied@redhat.com>,
> Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
> ---
>  drivers/gpu/drm/drm_irq.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 4cc9352ab6a8..b4bd361a2bcc 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1562,6 +1562,17 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
>  	return ret;
>  }
>  
> +static bool drm_wait_vblank_is_query(union drm_wait_vblank *vblwait)
> +{
> +	if (vblwait->request.sequence)
> +		return false;
> +
> +	return _DRM_VBLANK_RELATIVE ==
> +		(vblwait->request.type & (_DRM_VBLANK_TYPES_MASK |
> +					  _DRM_VBLANK_EVENT |
> +					  _DRM_VBLANK_NEXTONMISS));
> +}
> +
>  /*
>   * Wait for VBLANK.
>   *
> @@ -1611,6 +1622,21 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
>  
>  	vblank = &dev->vblank[pipe];
>  
> +	/* If the counter is currently enabled and accurate, short-circuit queries
> +	 * to return the cached timestamp of the last vblank.
> +	 */
> +	if (dev->vblank_disable_immediate &&
> +	    drm_wait_vblank_is_query(vblwait) &&
> +	    vblank->enabled) {
> +		struct timeval now;
> +

Do we want a comment here as well stating that the seqlock
already has the rmb?

> +		vblwait->reply.sequence =
> +			drm_vblank_count_and_time(dev, pipe, &now);
> +		vblwait->reply.tval_sec = now.tv_sec;
> +		vblwait->reply.tval_usec = now.tv_usec;
> +		return 0;
> +	}
> +
>  	ret = drm_vblank_get(dev, pipe);
>  	if (ret) {
>  		DRM_DEBUG("crtc %d failed to acquire vblank counter, %d\n", pipe, ret);
> -- 
> 2.11.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: Mark up accesses of vblank->enabled outside of its spinlock

[Chris Wilson]

On Fri, Mar 17, 2017 at 11:47:51AM +0200, Ville Syrjälä wrote:
> On Thu, Mar 16, 2017 at 11:47:48PM +0000, Chris Wilson wrote:
> > @@ -360,7 +358,7 @@ static void vblank_disable_fn(unsigned long arg)
> >  	unsigned long irqflags;
> >  
> >  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > -	if (atomic_read(&vblank->refcount) == 0 && vblank->enabled) {
> > +	if (atomic_read(&vblank->refcount) == 0 && READ_ONCE(vblank->enabled)) {
> 
> Hmm. Aren't most of these accesses inside the lock? Looks like you're
> marking everything READ/WRITE_ONCE()?

There's like 3 different locks here. Afaict, the correct one for
serialising vblank->enabled was dev->vblank_time_lock. Every access
outside of that lock, I marked up as READ_ONCE.

Oh, you are using vbl_lock as the barrier? That's not as clear for
disable:

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 53a526c7b24d..f447ed07ef95 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -325,6 +325,8 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
        struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
        unsigned long irqflags;
 
+       assert_spin_locked(&dev->vbl_lock);
+
        /* Prevent vblank irq processing while disabling vblank irqs,
         * so no updates of timestamps or count can happen after we've
         * disabled. Needed to prevent races in case of delayed irq's.


> > @@ -1714,6 +1717,9 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
> >  	if (WARN_ON(pipe >= dev->num_crtcs))
> >  		return false;
> >  
> > +	if (!READ_ONCE(vblank->enabled))
> > +		return false;
> 
> This to me looks like it could theoretically cause us to
> miss an interrupt.
> 
> 1. enable_irq()
> 2. drm_update_vblank_count()
> 3. irq fires
> 4. drm_handle_vblank() doesn't do anything
> 5. enabled=true

Sure. There's a danger you miss the irq anyway, and so the last action
after enabling the interrupt should be to process any completed events -
that's implicitly handled by enabling the interrupt in advance of adding
the first event. In the scenario above, there should be nothing to do.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: Mark up accesses of vblank->enabled outside of its spinlock

[Ville Syrjälä]

On Fri, Mar 17, 2017 at 10:19:51AM +0000, Chris Wilson wrote:
> On Fri, Mar 17, 2017 at 11:47:51AM +0200, Ville Syrjälä wrote:
> > On Thu, Mar 16, 2017 at 11:47:48PM +0000, Chris Wilson wrote:
> > > @@ -360,7 +358,7 @@ static void vblank_disable_fn(unsigned long arg)
> > >  	unsigned long irqflags;
> > >  
> > >  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > > -	if (atomic_read(&vblank->refcount) == 0 && vblank->enabled) {
> > > +	if (atomic_read(&vblank->refcount) == 0 && READ_ONCE(vblank->enabled)) {
> > 
> > Hmm. Aren't most of these accesses inside the lock? Looks like you're
> > marking everything READ/WRITE_ONCE()?
> 
> There's like 3 different locks here. Afaict, the correct one for
> serialising vblank->enabled was dev->vblank_time_lock. Every access
> outside of that lock, I marked up as READ_ONCE.
> 
> Oh, you are using vbl_lock as the barrier? That's not as clear for
> disable:
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 53a526c7b24d..f447ed07ef95 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -325,6 +325,8 @@ static void vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
>         struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
>         unsigned long irqflags;
>  
> +       assert_spin_locked(&dev->vbl_lock);
> +
>         /* Prevent vblank irq processing while disabling vblank irqs,
>          * so no updates of timestamps or count can happen after we've
>          * disabled. Needed to prevent races in case of delayed irq's.
> 
> 
> > > @@ -1714,6 +1717,9 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
> > >  	if (WARN_ON(pipe >= dev->num_crtcs))
> > >  		return false;
> > >  
> > > +	if (!READ_ONCE(vblank->enabled))
> > > +		return false;
> > 
> > This to me looks like it could theoretically cause us to
> > miss an interrupt.
> > 
> > 1. enable_irq()
> > 2. drm_update_vblank_count()
> > 3. irq fires
> > 4. drm_handle_vblank() doesn't do anything
> > 5. enabled=true
> 
> Sure. There's a danger you miss the irq anyway, and so the last action
> after enabling the interrupt should be to process any completed events -
> that's implicitly handled by enabling the interrupt in advance of adding
> the first event. In the scenario above, there should be nothing to do.

That's a slightly different scenario since you're only thinking about 
actually enabling the interrupt vs. calling drm_update_vblank_count().
That is fine as is. But with the extra 'enabled' check in the interrupt
handler you're effectively reversing that order to enable the interrupt
after drm_update_vblank_count(). Hence we can lose an interrupt now.

Of course we would eventually get another interrupt, and thanks to the
hw frame counter and whatnot we wouldn't actually lose our place entirely,
but we would see the counter jump by two frames when we actually handle
the interrupt.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm: Peek at the current counter/timestamp for vblank queries

[Chris Wilson]

On Fri, Mar 17, 2017 at 11:49:32AM +0200, Ville Syrjälä wrote:
> On Thu, Mar 16, 2017 at 11:47:49PM +0000, Chris Wilson wrote:
> > Bypass all the spinlocks and return the last timestamp and counter from
> > the last vblank if the driver delcares that it is accurate (and stable
> > across on/off), and the vblank is currently enabled.
> > 
> > This is dependent upon the both the hardware and driver to provide the
> > proper barriers to facilitate reading our bookkeeping outside of the
> > vblank interrupt and outside of the explicit vblank locks.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Michel Dänzer <michel@daenzer.net>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Dave Airlie <airlied@redhat.com>,
> > Cc: Mario Kleiner <mario.kleiner.de@gmail.com>
> > ---
> >  drivers/gpu/drm/drm_irq.c | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 4cc9352ab6a8..b4bd361a2bcc 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -1562,6 +1562,17 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
> >  	return ret;
> >  }
> >  
> > +static bool drm_wait_vblank_is_query(union drm_wait_vblank *vblwait)
> > +{
> > +	if (vblwait->request.sequence)
> > +		return false;
> > +
> > +	return _DRM_VBLANK_RELATIVE ==
> > +		(vblwait->request.type & (_DRM_VBLANK_TYPES_MASK |
> > +					  _DRM_VBLANK_EVENT |
> > +					  _DRM_VBLANK_NEXTONMISS));
> > +}
> > +
> >  /*
> >   * Wait for VBLANK.
> >   *
> > @@ -1611,6 +1622,21 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
> >  
> >  	vblank = &dev->vblank[pipe];
> >  
> > +	/* If the counter is currently enabled and accurate, short-circuit queries
> > +	 * to return the cached timestamp of the last vblank.
> > +	 */
> > +	if (dev->vblank_disable_immediate &&
> > +	    drm_wait_vblank_is_query(vblwait) &&
> > +	    vblank->enabled) {
> > +		struct timeval now;
> > +
> 
> Do we want a comment here as well stating that the seqlock
> already has the rmb?

I didn't find it enlightening. Added READ_ONCE(vblank->enabled).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx