On Mon, 10 Nov 2025, Ville Syrjälä <[email protected]> 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 <[email protected]>
>> ---
>> 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
