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

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

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.

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

-- 
Jani Nikula, Intel