Re: [PATCH 1/2] drm/vblank: allow dynamic per-crtc vblank off delay

[Daniel Vetter <daniel.vetter@ffwll.ch>]

On Mon, Jul 08, 2024 at 04:29:06PM -0400, Hamza Mahfooz wrote:
> We would like to be able to adjust the vblank off delay dynamically for
> a given CRTC. Since, it will allow drivers to apply static screen
> optimizations more quickly and consequently allow users to benefit more
> so from the power savings afforded by the aforementioned optimizations.
> 
> Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com>

So the vblank off delay is a hack to work around driver bugs/races, where
immediately disabling the vblank interrupt might result in
under/overcounting vblanks. Which really confuses compositors. The timeout
is chosen to be long enough that any miscounting should never end up
confusing userspace, so you can't just change it as a power optimization.

The right way to fix this is to work your driver so that you can set
vblank_disable_immediate = true, which amdgpu does for at least some
generations. The right fix here is to make sure you have that enabled on
all platforms correctly (which means the vblank interrupt race needs to be
properly fixed in driver code, which requires some deep hw knowledge of
when exactly the various registers update an interrupts fire). The
kerneldoc for vblank_disable_immedate and the other pieces it references
should expain the details.

tldr; nak on this approach.

Cheers, Sima

> ---
>  drivers/gpu/drm/drm_vblank.c | 31 ++++++++++++++++++++++++++-----
>  include/drm/drm_vblank.h     |  7 +++++++
>  2 files changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 702a12bc93bd..4f475131a092 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -529,6 +529,7 @@ int drm_vblank_init(struct drm_device *dev, unsigned int num_crtcs)
>  
>  		vblank->dev = dev;
>  		vblank->pipe = i;
> +		vblank->offdelay_ms = drm_vblank_offdelay;
>  		init_waitqueue_head(&vblank->queue);
>  		timer_setup(&vblank->disable_timer, vblank_disable_fn, 0);
>  		seqlock_init(&vblank->seqlock);
> @@ -1229,6 +1230,7 @@ EXPORT_SYMBOL(drm_crtc_vblank_get);
>  void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
>  {
>  	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> +	int vblank_offdelay = vblank->offdelay_ms;
>  
>  	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
>  		return;
> @@ -1238,13 +1240,13 @@ void drm_vblank_put(struct drm_device *dev, unsigned int pipe)
>  
>  	/* Last user schedules interrupt disable */
>  	if (atomic_dec_and_test(&vblank->refcount)) {
> -		if (drm_vblank_offdelay == 0)
> +		if (!vblank_offdelay)
>  			return;
> -		else if (drm_vblank_offdelay < 0)
> +		else if (vblank_offdelay < 0)
>  			vblank_disable_fn(&vblank->disable_timer);
>  		else if (!dev->vblank_disable_immediate)
>  			mod_timer(&vblank->disable_timer,
> -				  jiffies + ((drm_vblank_offdelay * HZ)/1000));
> +				  jiffies + ((vblank_offdelay * HZ) / 1000));
>  	}
>  }
>  
> @@ -1455,6 +1457,25 @@ void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
>  }
>  EXPORT_SYMBOL(drm_crtc_set_max_vblank_count);
>  
> +/**
> + * drm_crtc_set_vblank_offdelay - configure the vblank off delay value
> + * @crtc: CRTC in question
> + * @offdelay: off delay value
> + *
> + * If used, must be called before drm_vblank_on().
> + */
> +void drm_crtc_set_vblank_offdelay(struct drm_crtc *crtc, int offdelay)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	unsigned int pipe = drm_crtc_index(crtc);
> +	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
> +
> +	drm_WARN_ON(dev, dev->vblank_disable_immediate);
> +
> +	vblank->offdelay_ms = offdelay;
> +}
> +EXPORT_SYMBOL(drm_crtc_set_vblank_offdelay);
> +
>  /**
>   * drm_crtc_vblank_on - enable vblank events on a CRTC
>   * @crtc: CRTC in question
> @@ -1490,7 +1511,7 @@ void drm_crtc_vblank_on(struct drm_crtc *crtc)
>  	 * re-enable interrupts if there are users left, or the
>  	 * user wishes vblank interrupts to be enabled all the time.
>  	 */
> -	if (atomic_read(&vblank->refcount) != 0 || drm_vblank_offdelay == 0)
> +	if (atomic_read(&vblank->refcount) != 0 || !vblank->offdelay_ms)
>  		drm_WARN_ON(dev, drm_vblank_enable(dev, pipe));
>  	spin_unlock_irq(&dev->vbl_lock);
>  }
> @@ -1909,7 +1930,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
>  	 * drm_handle_vblank_events) so that the timestamp is always accurate.
>  	 */
>  	disable_irq = (dev->vblank_disable_immediate &&
> -		       drm_vblank_offdelay > 0 &&
> +		       vblank->offdelay_ms > 0 &&
>  		       !atomic_read(&vblank->refcount));
>  
>  	drm_handle_vblank_events(dev, pipe);
> diff --git a/include/drm/drm_vblank.h b/include/drm/drm_vblank.h
> index 7f3957943dd1..f92f28b816af 100644
> --- a/include/drm/drm_vblank.h
> +++ b/include/drm/drm_vblank.h
> @@ -187,6 +187,12 @@ struct drm_vblank_crtc {
>  	 */
>  	int linedur_ns;
>  
> +	/**
> +	 * @offdelay_ms: Vblank off delay in ms, used to determine how long
> +	 * @disable_timer waits before disabling.
> +	 */
> +	int offdelay_ms;
> +
>  	/**
>  	 * @hwmode:
>  	 *
> @@ -255,6 +261,7 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>  wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc);
>  void drm_crtc_set_max_vblank_count(struct drm_crtc *crtc,
>  				   u32 max_vblank_count);
> +void drm_crtc_set_vblank_offdelay(struct drm_crtc *crtc, int offdelay);
>  
>  /*
>   * Helpers for struct drm_crtc_funcs
> -- 
> 2.45.1
> 

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