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@xxxxxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Michel Dänzer <michel@xxxxxxxxxxx>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
