On Fri, Jan 12, 2018 at 09:57:05PM +, 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
> Cc: Daniel Vetter
> Cc: Michel Dänzer
> Signed-off-by: Dhinakaran Pandiyan
> ---
> 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
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
