Re: [PATCH 1/6] drm: No-Op redundant calls to drm_vblank_off()

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Feb 08, 2016 at 02:13:24AM +0100, Mario Kleiner wrote:
> Otherwise if a kms driver calls into drm_vblank_off() more than once
> before calling drm_vblank_on() again, the redundant calls to
> vblank_disable_and_save() will call drm_update_vblank_count()
> while hw vblank counters and vblank timestamping are in a undefined
> state during modesets, dpms off etc.
> 
> At least with the legacy drm helpers it is not unusual to
> get multiple calls to drm_vblank_off and drm_vblank_on, e.g.,
> half a dozen calls to drm_vblank_off and two calls to drm_vblank_on
> were observed on radeon-kms during dpms-off -> dpms-on transition.
> 
> Fixes large jumps of the software maintained vblank counter due to
> the hardware vblank counter resetting to zero during dpms off or
> modeset, e.g., if radeon-kms is modified to use drm_vblank_off/on
> instead of drm_vblank_pre/post_modeset().
> 
> This fixes a regression caused by the changes made to
> drm_update_vblank_count() in Linux 4.4.
> 
> Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> Cc: <stable@vger.kernel.org> # 4.4+
> Cc: michel@daenzer.net
> Cc: vbabka@suse.cz
> Cc: ville.syrjala@linux.intel.com
> Cc: daniel.vetter@ffwll.ch
> Cc: dri-devel@lists.freedesktop.org
> Cc: alexander.deucher@amd.com
> Cc: christian.koenig@amd.com
> ---
>  drivers/gpu/drm/drm_irq.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 607f493..bcb8528 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1313,7 +1313,13 @@ void drm_vblank_off(struct drm_device *dev, unsigned int pipe)
>  	spin_lock_irqsave(&dev->event_lock, irqflags);
>  
>  	spin_lock(&dev->vbl_lock);
> -	vblank_disable_and_save(dev, pipe);
> +	DRM_DEBUG_VBL("crtc %d, vblank enabled %d, inmodeset %d\n",
> +		      pipe, vblank->enabled, vblank->inmodeset);
> +
> +	/* Avoid redundant vblank disables without previous drm_vblank_on(). */
> +	if (!vblank->inmodeset)

Since atomic drivers shouldn't suck so badly at this, maybe

	if (DRIVER_ATOMIC || !vblank->inmodeset)

instead? That way the flexibility would be constraint to old drivers that
need it because legacy crtc helpers suck. With that change:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +		vblank_disable_and_save(dev, pipe);
> +
>  	wake_up(&vblank->queue);
>  
>  	/*
> @@ -1415,6 +1421,9 @@ void drm_vblank_on(struct drm_device *dev, unsigned int pipe)
>  		return;
>  
>  	spin_lock_irqsave(&dev->vbl_lock, irqflags);
> +	DRM_DEBUG_VBL("crtc %d, vblank enabled %d, inmodeset %d\n",
> +		      pipe, vblank->enabled, vblank->inmodeset);
> +
>  	/* Drop our private "prevent drm_vblank_get" refcount */
>  	if (vblank->inmodeset) {
>  		atomic_dec(&vblank->refcount);
> -- 
> 1.9.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch