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

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

[Thomas Zimmermann]

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 */
+
+		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))
-- 
2.54.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Lockless dereference of atomic state pointers (`plane->state` and `crtc->state`) in `find_crtc_index_atomic` can lead to Use-After-Free during concurrent atomic commits.
--

commit 789de6442f862dfb55a96bcc0ed2477d4a69bfd2
Author: Thomas Zimmermann <tzimmermann@suse.de>
Subject: drm/fb-helper: Only consider active CRTCs for vblank sync

This patch modifies the fbdev emulation to only synchronize to the vblank of
an active CRTC. It introduces separate lookup paths for atomic and non-atomic
drivers to find the correct CRTC index.

Link: https://lore.kernel.org/dri-devel/1c9e0e24-9c4a-4259-8700-cf9e5fd60ca3@suse.de/
Closes: https://bugs.debian.org/1138033

> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 7b11a582f8ec4..cbf0a9a7b8e51 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)
[ ... ]
> +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 */

[Severity: Critical]
Is it safe to access plane->state here without holding the appropriate
plane locks?

Since dev->mode_config.mutex is held but plane->mutex is not, could a
concurrent atomic commit from another DRM master swap and free plane->state
while this function is executing?

> +
> +		crtc = plane_state->crtc;
> +		if (!crtc->state->active)
> +			continue;

[Severity: Critical]
If a use-after-free occurs on plane->state, could it lead to reading a fake
crtc pointer, dereferencing its state, and returning an arbitrary crtc->index?

> +		if (drm_WARN_ON_ONCE(dev, crtc->index > INT_MAX))
> +			continue; /* driver bug */
> +
> +		return crtc->index;
> +	}
> +
> +	return -EINVAL;
> +}
[ ... ]
>  static void drm_fb_helper_fb_dirty(struct drm_fb_helper *helper)
>  {
[ ... ]
>  	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);

[Severity: Critical]
Because drm_fb_helper_find_crtc_index() is called before checking if we still
own the DRM master (which happens inside drm_client_modeset_wait_for_vblank
via drm_master_internal_acquire), could an unprivileged user writing to
/dev/fb0 trigger this lookup concurrently with an active userspace
compositor's atomic commits, exposing the race condition mentioned above?

Furthermore, if an attacker can control the returned crtc_index, could it lead
to an out-of-bounds array read in drm_client_modeset_wait_for_vblank() when it
does:

    crtc = client->modesets[crtc_index].crtc;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622113434.682292-1-tzimmermann@suse.de?part=1

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

[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.

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

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

[Thomas Zimmermann]

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)