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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel