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

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 10.11.25 um 13:51 schrieb Jani Nikula:
> 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.

Please also merge the rest of the series. These patches are an 
improvement to open-coding the access to the fields.

Best regards
Thomas

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

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