Re: [PATCH 5/6] drm: Introduce drm_crtc_vblank_sanitize_counter.

From: Daniel Vetter <daniel@ffwll.ch>
To: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 5/6] drm: Introduce drm_crtc_vblank_sanitize_counter.
Date: Thu, 4 Aug 2016 10:32:58 +0200	[thread overview]
Message-ID: <20160804083258.GX6232@phenom.ffwll.local> (raw)
In-Reply-To: <1470260019-6173-6-git-send-email-rodrigo.vivi@intel.com>

On Wed, Aug 03, 2016 at 02:33:38PM -0700, Rodrigo Vivi wrote:
> In modern systems there are situations that you can let the screen
> enabled and sleep or shut off a most of the display controler.
> 
> In situations like this the vblank hw counter can be reset.
> When this happens everything in the system gets crazy by
> the big count.
> 
> So, the right approach is to make sure we are avoiding any
> runtime suspend when vblank is enabled but also restore
> drm crtc vblank counter to a state we can rely.
> 
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/drm_irq.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_irq.h     |  1 +
>  2 files changed, 47 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index d0d1dde..7320836 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -265,6 +265,52 @@ u32 drm_accurate_vblank_count(struct drm_crtc *crtc)
>  }
>  EXPORT_SYMBOL(drm_accurate_vblank_count);
>  
> +/**
> + * drm_crtc_vblank_sanitize_counter - Sanitize vblank counter
> + * @crtc: which counter to sanitize
> + *
> + * This function returns drm crtc vblank counter to the latest
> + * known counter.
> + *
> + * This function is useful for runtime suspend where the hw
> + * counter or timer might reset.
> + *
> + * Use this only when there is no risk of the runtime suspend
> + * reseting hardware counter and before enabling vblank irq.

I think this needs to be a bit more precise, and should reference the
prepare/unprepare hooks.

> + */
> +void drm_crtc_vblank_sanitize_counter(struct drm_crtc *crtc)

sanitize_counter is a bit an unclear function name. I think resync_counter
would be better, but still not good really.

> +{
> +	struct drm_device *dev = crtc->dev;
> +	unsigned int pipe = drm_crtc_index(crtc);
> +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> +	unsigned long irqflags;
> +	struct timeval t_vblank;
> +	int count = DRM_TIMESTAMP_MAXRETRIES;
> +	u32 cur_vblank;
> +	bool rc;
> +
> +	spin_lock_irqsave(&dev->vblank_time_lock, irqflags);
> +
> +	if (vblank->enabled) {

If we put the sanitize call like I suggested, this would be a driver bug.
We don't want to sanitize it when it's already sanitized and nothing
could have corrupted it meanwhile, and especially not when the vblank
counter is in use. Should be a WARN_ON, not debug level.

> +		DRM_DEBUG_VBL("Skip sanitize - Vblank is enabled on pipe %d\n",
> +			      pipe);
> +		goto unlock;
> +	}
> +
> +	do {
> +		cur_vblank = dev->driver->get_vblank_counter(dev, pipe);
> +		rc = drm_get_last_vbltimestamp(dev, pipe, &t_vblank, 0);
> +	} while (cur_vblank != dev->driver->get_vblank_counter(dev, pipe) && --count > 0);
> +
> +	if (!rc)
> +		t_vblank = (struct timeval) {0, 0};
> +
> +	store_vblank(dev, pipe, 0, &t_vblank, cur_vblank);

We also need to sufficiently fake the missed vblanks by comparing the last
timestamp with the new one and dividing the elapsed time by the frame
time. Ville has added such logic to already to make sure we correctly
account for the time between drm_vblank_on and drm_vblank_off when the
vblank counter is not running. Please reuse that code to make that
adjustment, instead of open-code it.

> +unlock:
> +	spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
> +}
> +EXPORT_SYMBOL(drm_crtc_vblank_sanitize_counter);
> +
>  /*
>   * Disable vblank irq's on crtc, make sure that last vblank count
>   * of hardware and corresponding consistent software vblank counter
> diff --git a/include/drm/drm_irq.h b/include/drm/drm_irq.h
> index 93a9e9d..55c419a 100644
> --- a/include/drm/drm_irq.h
> +++ b/include/drm/drm_irq.h
> @@ -160,6 +160,7 @@ extern u32 drm_vblank_count(struct drm_device *dev, unsigned int pipe);
>  extern u32 drm_crtc_vblank_count(struct drm_crtc *crtc);
>  extern u32 drm_crtc_vblank_count_and_time(struct drm_crtc *crtc,
>  					  struct timeval *vblanktime);
> +extern void drm_crtc_vblank_sanitize_counter(struct drm_crtc *crtc);
>  extern void drm_crtc_send_vblank_event(struct drm_crtc *crtc,
>  				       struct drm_pending_vblank_event *e);
>  extern void drm_crtc_arm_vblank_event(struct drm_crtc *crtc,
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx