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

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

On Mon, 10 Nov 2025, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 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.

I don't disagree, but getting that done is a bit more involved than what
I have time for right now. Need to think.

In the mean time, pushed the drm_crtc_vblank_crtc() related patches in
the series, and left the drm_crtc_vblank_waitqueue() ones to simmer.

Thanks for the reviews.

BR,
Jani.


>
> [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));

-- 
Jani Nikula, Intel