diff mbox

[1/5] drm/vblank: Do not update vblank counts if vblanks are already disabled.

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

Commit Message

Dhinakaran Pandiyan Jan. 3, 2018, 8:39 p.m. UTC
Updating the vblank counts requires register reads and these reads may not
return meaningful values after the vblank interrupts are disabled as the
device may go to low power state. An additional change would be to allow
the driver to save the vblank counts before entering a low power state, but
that's for the future.

Also, disable vblanks after reading the HW counter in the case where
_crtc_vblank_off() is disabling vblanks.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

Comments

Chris Wilson Jan. 3, 2018, 8:53 p.m. UTC | #1
Quoting Dhinakaran Pandiyan (2018-01-03 20:39:57)
> Updating the vblank counts requires register reads and these reads may not
> return meaningful values after the vblank interrupts are disabled as the
> device may go to low power state. An additional change would be to allow
> the driver to save the vblank counts before entering a low power state, but
> that's for the future.
> 
> Also, disable vblanks after reading the HW counter in the case where
> _crtc_vblank_off() is disabling vblanks.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/drm_vblank.c | 23 +++++++++--------------
>  1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 32d9bcf5be7f..7eee82c06ed8 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -347,23 +347,14 @@ 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.
> -        */
> -       if (vblank->enabled) {
> -               __disable_vblank(dev, pipe);
> -               vblank->enabled = false;
> -       }
> -
> -       /*
> -        * 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;
>         spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
>  }
>  
> @@ -1122,8 +1113,12 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
>                       pipe, vblank->enabled, vblank->inmodeset);
>  
>         /* Avoid redundant vblank disables without previous
> -        * drm_crtc_vblank_on(). */
> -       if (drm_core_check_feature(dev, DRIVER_ATOMIC) || !vblank->inmodeset)
> +        * drm_crtc_vblank_on() and only disable them if they're enabled. This
> +        * avoids calling the ->disable_vblank() operation in atomic context
> +        * with the hardware potentially runtime suspended.
> +        */
> +       if ((drm_core_check_feature(dev, DRIVER_ATOMIC) || !vblank->inmodeset) &&
> +           vblank->enabled)

Outside of the spinlock protecting vblank->enabled.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 32d9bcf5be7f..7eee82c06ed8 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -347,23 +347,14 @@  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.
-	 */
-	if (vblank->enabled) {
-		__disable_vblank(dev, pipe);
-		vblank->enabled = false;
-	}
-
-	/*
-	 * 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;
 	spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
 }
 
@@ -1122,8 +1113,12 @@  void drm_crtc_vblank_off(struct drm_crtc *crtc)
 		      pipe, vblank->enabled, vblank->inmodeset);
 
 	/* Avoid redundant vblank disables without previous
-	 * drm_crtc_vblank_on(). */
-	if (drm_core_check_feature(dev, DRIVER_ATOMIC) || !vblank->inmodeset)
+	 * drm_crtc_vblank_on() and only disable them if they're enabled. This
+	 * avoids calling the ->disable_vblank() operation in atomic context
+	 * with the hardware potentially runtime suspended.
+	 */
+	if ((drm_core_check_feature(dev, DRIVER_ATOMIC) || !vblank->inmodeset) &&
+	    vblank->enabled)
 		drm_vblank_disable_and_save(dev, pipe);
 
 	wake_up(&vblank->queue);