Re: [PATCH v2] drm/fb-helper: Only consider active CRTCs for vblank sync

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 22.06.26 um 15:34 schrieb Jani Nikula:
> On Mon, 22 Jun 2026, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Only synchronize fbdev output to the vblank of an active CRTC. Go over
>> the list of CRTCs and pick the first that matches. Fixes warnings as
>> the one shown below
>>
>> [   77.201354] WARNING: drivers/gpu/drm/drm_vblank.c:1320 at drm_crtc_wait_one_vblank+0x194/0x1cc [drm], CPU#1: kworker/1:7/1867
>> [   77.201354] omapdrm omapdrm.0: [drm] vblank wait timed out on crtc 0
>>
>> This currently happens if the fbdev output is not on CRTC 0.
>>
>> Atomic and non-atomic drivers require distinct code paths. As for other
>> fbdev operations, implement both and select the correct one at runtime.
>>
>> Not finding an active CRTC is not a bug. Do not wait in this case, but
>> flush the display update as before.
>>
>> v2:
>> - move look-up code into separate helper
>> - support drivers with legacy modesetting
>> v1:
>> - see https://lore.kernel.org/dri-devel/1c9e0e24-9c4a-4259-8700-cf9e5fd60ca3@suse.de/
>>
>> Co-authored-by: H. Nikolaus Schaller <hns@goldelico.com>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Fixes: d8c4bddcd8bcb ("drm/fb-helper: Synchronize dirty worker with vblank")
>> Tested-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
>> Closes: https://bugs.debian.org/1138033
>> Cc: <stable@vger.kernel.org> # v6.19+
>> ---
>>   drivers/gpu/drm/drm_fb_helper.c | 71 ++++++++++++++++++++++++++++++++-
>>   1 file changed, 70 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> index 7b11a582f8ec..cbf0a9a7b8e5 100644
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
>> @@ -225,16 +225,85 @@ static void drm_fb_helper_resume_worker(struct work_struct *work)
>>   	console_unlock();
>>   }
>>   
>> +static int find_crtc_index_atomic(struct drm_fb_helper *helper)
>> +{
>> +	struct drm_device *dev = helper->dev;
>> +	struct drm_plane *plane;
>> +
>> +	drm_for_each_plane(plane, dev) {
>> +		const struct drm_plane_state *plane_state;
>> +		const struct drm_crtc *crtc;
>> +
>> +		if (plane->type != DRM_PLANE_TYPE_PRIMARY)
>> +			continue;
>> +
>> +		plane_state = plane->state;
>> +		if (plane_state->fb != helper->fb || !plane_state->crtc)
>> +			continue; /* plane doesn't display fbdev emulation */
>> +
>> +		crtc = plane_state->crtc;
>> +		if (!crtc->state->active)
>> +			continue;
>> +		if (drm_WARN_ON_ONCE(dev, crtc->index > INT_MAX))
>> +			continue; /* driver bug */
> I take it this is here because crtc->index is unsigned, and this
> function returns int so you can have negative error codes. Ditto the
> other function below.
>
> I feel like this is something that should be checked once somewhere, if
> that. I think adding arbitrary checks like this invites more arbitrary
> checks everywhere. crtc->index is supposed to be invariant over the
> lifetime of the CRTC.

Ok, makes sense.

Best regards
Thomas

>
> BR,
> Jani.
>
>> +
>> +		return crtc->index;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int find_crtc_index_legacy(struct drm_fb_helper *helper)
>> +{
>> +	struct drm_device *dev = helper->dev;
>> +	struct drm_crtc *crtc;
>> +
>> +	drm_for_each_crtc(crtc, dev) {
>> +		struct drm_plane *plane = crtc->primary;
>> +
>> +		if (!crtc->enabled)
>> +			continue;
>> +		if (!plane || plane->fb != helper->fb)
>> +			continue; /* CRTC doesn't display fbdev emulation */
>> +		if (drm_WARN_ON_ONCE(dev, crtc->index > INT_MAX))
>> +			continue; /* driver bug */
>> +
>> +		return crtc->index;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +
>> +static int drm_fb_helper_find_crtc_index(struct drm_fb_helper *helper)
>> +{
>> +	struct drm_device *dev = helper->dev;
>> +	int crtc_index;
>> +
>> +	mutex_lock(&dev->mode_config.mutex);
>> +
>> +	if (drm_drv_uses_atomic_modeset(dev))
>> +		crtc_index = find_crtc_index_atomic(helper);
>> +	else
>> +		crtc_index = find_crtc_index_legacy(helper);
>> +
>> +	mutex_unlock(&dev->mode_config.mutex);
>> +
>> +	return crtc_index;
>> +}
>> +
>>   static void drm_fb_helper_fb_dirty(struct drm_fb_helper *helper)
>>   {
>>   	struct drm_device *dev = helper->dev;
>>   	struct drm_clip_rect *clip = &helper->damage_clip;
>>   	struct drm_clip_rect clip_copy;
>> +	int crtc_index;
>>   	unsigned long flags;
>>   	int ret;
>>   
>>   	mutex_lock(&helper->lock);
>> -	drm_client_modeset_wait_for_vblank(&helper->client, 0);
>> +	crtc_index = drm_fb_helper_find_crtc_index(helper);
>> +	if (crtc_index >= 0)
>> +		drm_client_modeset_wait_for_vblank(&helper->client, crtc_index);
>>   	mutex_unlock(&helper->lock);
>>   
>>   	if (drm_WARN_ON_ONCE(dev, !helper->funcs->fb_dirty))

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