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

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 10 Nov 2025, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 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.

Good catch! Yeah, not all places can be converted to struct
drm_vblank_crtc. I need to go through these.

There are a handful of places now that grab the vblank pointer (even
with NULL dev->vblank) and check has vblank afterwards, which is a bit
iffy. And actually mislead me here.

BR,
Jani.

>
>>  
>> -	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

-- 
Jani Nikula, Intel