Re: [PATCH 2/6] drm/atomic: use drm_crtc_vblank_waitqueue()

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 07.11.25 um 12:04 schrieb Jani Nikula:
> We have drm_crtc_vblank_waitqueue() to get the wait_queue_head_t pointer
> for a vblank. Use it instead of poking at dev->vblank[] directly.
>
> Due to the macro maze of wait_event_timeout() that uses the address-of
> operator on the argument, we have to pass it in with the indirection
> operator.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by Thomas Zimmermann <tzimmermann@suse.de>

But... drm_crtc_vblank_waitqueue() is a terrible interface IMHO, as it 
exports internal details of the vblank implementation.

I wonder if the existing users at [1] and [2] couldn't be replaced with 
a common vblank helper.

And there's drm_wait_one_vblank() [3] and the waiting that's being fixed 
here [4]. The latter looks like [3] but with multiple CRTC waiting for 
their next vblank. I'd say this could be a single implementation within 
the vblank code.

[1] 
https://elixir.bootlin.com/linux/v6.18-rc4/source/drivers/gpu/drm/i915/display/intel_display_rps.c#L73
[2] 
https://elixir.bootlin.com/linux/v6.18-rc4/source/drivers/gpu/drm/i915/display/intel_vblank.c#L715
[3] 
https://elixir.bootlin.com/linux/v6.18-rc4/source/drivers/gpu/drm/drm_vblank.c#L1304
[4] 
https://elixir.bootlin.com/linux/v6.18-rc4/source/drivers/gpu/drm/drm_atomic_helper.c#L1837

Best regards
Thomas

> ---
>   drivers/gpu/drm/drm_atomic_helper.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 5a473a274ff0..e641fcf8c568 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1831,10 +1831,12 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device *dev,
>   	}
>   
>   	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
> +		wait_queue_head_t *queue = drm_crtc_vblank_waitqueue(crtc);
> +
>   		if (!(crtc_mask & drm_crtc_mask(crtc)))
>   			continue;
>   
> -		ret = wait_event_timeout(dev->vblank[i].queue,
> +		ret = wait_event_timeout(*queue,
>   					 state->crtcs[i].last_vblank_count !=
>   						drm_crtc_vblank_count(crtc),
>   					 msecs_to_jiffies(100));

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)