Re: [PATCH 23/24] drm/vblank: reduce pipe checks

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 10, 2025 at 06:17:41PM +0200, Jani Nikula wrote:
> Now that drm_vblank_crtc() is the only place that indexes dev->vblank[],
> and its usage has reduced considerably, add the primary pipe
> out-of-bounds check there, and return NULL. Expect callers to check it
> and act accordingly.
> 
> In drm_crtc_vblank_crtc(), warn and return NULL, and let it go boom. If
> the crtc->pipe is out of bounds, it's a driver error that needs to be
> fixed.
> 
> Remove superfluous pipe checks all around.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_vblank.c | 36 +++++++++++++++---------------------
>  1 file changed, 15 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
> index 44fb8d225485..7829e64e42b4 100644
> --- a/drivers/gpu/drm/drm_vblank.c
> +++ b/drivers/gpu/drm/drm_vblank.c
> @@ -177,13 +177,22 @@ MODULE_PARM_DESC(timestamp_precision_usec, "Max. error on timestamps [usecs]");
>  static struct drm_vblank_crtc *
>  drm_vblank_crtc(struct drm_device *dev, unsigned int pipe)
>  {
> +	if (pipe >= dev->num_crtcs)
> +		return NULL;
> +
>  	return &dev->vblank[pipe];
>  }
>  
>  struct drm_vblank_crtc *
>  drm_crtc_vblank_crtc(struct drm_crtc *crtc)
>  {
> -	return drm_vblank_crtc(crtc->dev, drm_crtc_index(crtc));
> +	struct drm_vblank_crtc *vblank;
> +
> +	vblank = drm_vblank_crtc(crtc->dev, drm_crtc_index(crtc));
> +	if (drm_WARN_ON(crtc->dev, !vblank))
> +		return NULL;
> +
> +	return vblank;
>  }
>  EXPORT_SYMBOL(drm_crtc_vblank_crtc);
>  
> @@ -631,7 +640,6 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>  				     const struct drm_display_mode *mode)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	unsigned int pipe = drm_crtc_index(crtc);
>  	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>  	int linedur_ns = 0, framedur_ns = 0;
>  	int dotclock = mode->crtc_clock;
> @@ -639,9 +647,6 @@ void drm_calc_timestamping_constants(struct drm_crtc *crtc,
>  	if (!drm_dev_has_vblank(dev))
>  		return;

I belive this at least gets called from the atomic helpers even
for drivers that don't have vblank support. In which case the
drm_crtc_vblank_crtc() call would have to be done after the
drm_dev_has_vblank() check or else you'll get spurious WARNs.

I don't remember if there are other cases like this as well.

>  
> -	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
> -		return;
> -
>  	/* Valid dotclock? */
>  	if (dotclock > 0) {
>  		int frame_size = mode->crtc_htotal * mode->crtc_vtotal;
> @@ -724,11 +729,6 @@ drm_crtc_vblank_helper_get_vblank_timestamp_internal(
>  	int vpos, hpos, i;
>  	int delta_ns, duration_ns;
>  
> -	if (pipe >= dev->num_crtcs) {
> -		drm_err(dev, "Invalid crtc %u\n", pipe);
> -		return false;
> -	}
> -
>  	/* Scanout position query not supported? Should not happen. */
>  	if (!get_scanout_position) {
>  		drm_err(dev, "Called from CRTC w/o get_scanout_position()!?\n");
> @@ -1339,9 +1339,6 @@ void drm_crtc_vblank_off(struct drm_crtc *crtc)
>  	ktime_t now;
>  	u64 seq;
>  
> -	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
> -		return;
> -
>  	/*
>  	 * Grab event_lock early to prevent vblank work from being scheduled
>  	 * while we're in the middle of shutting down vblank interrupts
> @@ -1480,9 +1477,6 @@ void drm_crtc_vblank_on_config(struct drm_crtc *crtc,
>  	unsigned int pipe = drm_crtc_index(crtc);
>  	struct drm_vblank_crtc *vblank = drm_crtc_vblank_crtc(crtc);
>  
> -	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
> -		return;
> -
>  	spin_lock_irq(&dev->vbl_lock);
>  	drm_dbg_vbl(dev, "crtc %d, vblank enabled %d, inmodeset %d\n",
>  		    pipe, vblank->enabled, vblank->inmodeset);
> @@ -1764,10 +1758,9 @@ int drm_wait_vblank_ioctl(struct drm_device *dev, void *data,
>  		pipe = pipe_index;
>  	}
>  
> -	if (pipe >= dev->num_crtcs)
> -		return -EINVAL;
> -
>  	vblank = drm_vblank_crtc(dev, pipe);
> +	if (!vblank)
> +		return -EINVAL;
>  
>  	/* If the counter is currently enabled and accurate, short-circuit
>  	 * queries to return the cached timestamp of the last vblank.
> @@ -1902,14 +1895,15 @@ static void drm_handle_vblank_events(struct drm_vblank_crtc *vblank)
>   */
>  bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
>  {
> -	struct drm_vblank_crtc *vblank = drm_vblank_crtc(dev, pipe);
> +	struct drm_vblank_crtc *vblank;
>  	unsigned long irqflags;
>  	bool disable_irq;
>  
>  	if (drm_WARN_ON_ONCE(dev, !drm_dev_has_vblank(dev)))
>  		return false;
>  
> -	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
> +	vblank = drm_vblank_crtc(dev, pipe);
> +	if (drm_WARN_ON(dev, !vblank))
>  		return false;
>  
>  	spin_lock_irqsave(&dev->event_lock, irqflags);
> -- 
> 2.47.3

-- 
Ville Syrjälä
Intel