diff mbox

[3/5] drm/vblank: Do not update vblank count if interrupts are already disabled.

Message ID 20180112215707.3084-3-dhinakaran.pandiyan@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dhinakaran Pandiyan Jan. 12, 2018, 9:57 p.m. UTC
Updating vblank counts requires register reads and these reads may not
return meaningful values if the device was in a low power state after
vblank interrupts were last disabled. So, update the count only if vblank
interrupts are enabled. Secondly, this means the registers should be read
before disabling vblank interrupts.

v2: Don't check vblank->enabled outside it's lock (Chris)

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Michel Dänzer <michel@daenzer.net>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

Rodrigo Vivi Jan. 19, 2018, 7:47 a.m. UTC | #1
On Fri, Jan 12, 2018 at 09:57:05PM +0000, Dhinakaran Pandiyan wrote:
> Updating vblank counts requires register reads and these reads may not
> return meaningful values if the device was in a low power state after
> vblank interrupts were last disabled. So, update the count only if vblank
> interrupts are enabled. Secondly, this means the registers should be read
> before disabling vblank interrupts.
> 
> v2: Don't check vblank->enabled outside it's lock (Chris)
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Michel Dänzer <michel@daenzer.net>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_vblank.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index f2bf1f5dbaa5..2559d2d7b907 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -347,23 +347,25 @@ void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
>  	spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
>  
>  	/*
> -	 * Only disable vblank interrupts if they're enabled. This avoids
> -	 * calling the ->disable_vblank() operation in atomic context with the
> -	 * hardware potentially runtime suspended.
> +	 * Update vblank count and disable vblank interrupts only if the
> +	 * interrupts were enabled. This avoids calling the ->disable_vblank()
> +	 * operation in atomic context with the hardware potentially runtime
> +	 * suspended.
>  	 */
> -	if (vblank->enabled) {
> -		__disable_vblank(dev, pipe);
> -		vblank->enabled = false;
> -	}
> +	if (!vblank->enabled)
> +		goto out;
>  
>  	/*
> -	 * Always update the count and timestamp to maintain the
> +	 * Update the count and timestamp to maintain the
>  	 * appearance that the counter has been ticking all along until
>  	 * this time. This makes the count account for the entire time
>  	 * between drm_crtc_vblank_on() and drm_crtc_vblank_off().
>  	 */

I feel that this entire comment can be simply removed now...
The approach looks good and right to me so you can use

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

but please ping Ville to take a look here since he introduced this approach with
4dfd64862ff8 ("drm: Use vblank timestamps to guesstimate how many vblanks were missed")

>  	drm_update_vblank_count(dev, pipe, false);
> +	__disable_vblank(dev, pipe);
> +	vblank->enabled = false;
>  
> +out:
>  	spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
>  }
>  
> -- 
> 2.11.0
>
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index f2bf1f5dbaa5..2559d2d7b907 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -347,23 +347,25 @@  void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)
 	spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
 
 	/*
-	 * Only disable vblank interrupts if they're enabled. This avoids
-	 * calling the ->disable_vblank() operation in atomic context with the
-	 * hardware potentially runtime suspended.
+	 * Update vblank count and disable vblank interrupts only if the
+	 * interrupts were enabled. This avoids calling the ->disable_vblank()
+	 * operation in atomic context with the hardware potentially runtime
+	 * suspended.
 	 */
-	if (vblank->enabled) {
-		__disable_vblank(dev, pipe);
-		vblank->enabled = false;
-	}
+	if (!vblank->enabled)
+		goto out;
 
 	/*
-	 * Always update the count and timestamp to maintain the
+	 * Update the count and timestamp to maintain the
 	 * appearance that the counter has been ticking all along until
 	 * this time. This makes the count account for the entire time
 	 * between drm_crtc_vblank_on() and drm_crtc_vblank_off().
 	 */
 	drm_update_vblank_count(dev, pipe, false);
+	__disable_vblank(dev, pipe);
+	vblank->enabled = false;
 
+out:
 	spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
 }