[PATCH] drm/fb_helper: Determine proper crtc_index within drm_fb_helper_fb_dirty

[PATCH] drm/fb_helper: Determine proper crtc_index within drm_fb_helper_fb_dirty

[H. Nikolaus Schaller]

The call to drm_client_modeset_wait_for_vblank() currently hardcodes the
crtc_index to 0. This is incorrect when CRTC 0 is inactive and not
generating vblank interrupts.

When this occurs, the console is flooded with vblank timeout warnings:

[   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 behavior was observed on a PandaBoard-ES when only the HDMI port
(CRTC 1) is connected, while the LCD (CRTC 0) and DVI port (CRTC 2)
remain disconnected. The root cause is the hardcoded crtc_index = 0.

Fix this by locating the matching CRTCs. If multiple CRTCs match, the
last active one is selected. If no matching CRTC is found, the vblank
wait is skipped entirely.

Fixes: d8c4bddcd8bcb ("drm/fb-helper: Synchronize dirty worker with vblank")
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 1541fc8a9ac2d..70999cdc98b32 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -232,9 +232,23 @@ static void drm_fb_helper_fb_dirty(struct drm_fb_helper *helper)
 	struct drm_clip_rect clip_copy;
 	unsigned long flags;
 	int ret;
+	unsigned int crtc_index = 0;
+	struct drm_crtc *crtc;
 
 	mutex_lock(&helper->lock);
-	drm_client_modeset_wait_for_vblank(&helper->client, 0);
+
+	drm_for_each_crtc(crtc, dev) {
+		if (crtc->primary && crtc->primary->state &&
+		    crtc->primary->state->fb == helper->fb &&
+		    crtc->state && crtc->state->active) {
+			crtc_index = drm_crtc_index(crtc) + 1;
+			break;
+		}
+	}
+
+	if (crtc_index > 0)
+		drm_client_modeset_wait_for_vblank(&helper->client, crtc_index - 1);
+
 	mutex_unlock(&helper->lock);
 
 	if (drm_WARN_ON_ONCE(dev, !helper->funcs->fb_dirty))
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] drm/fb_helper: Determine proper crtc_index within drm_fb_helper_fb_dirty

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Lockless access to `crtc->state` and `crtc->primary->state` can lead to a Use-After-Free due to concurrent atomic commits.
- [High] The patch breaks vblank synchronization for legacy (non-atomic) DRM drivers.
- [Medium] The logic selects the first matching CRTC instead of the last active one, contradicting the commit message.
--

commit 0337b26b606b4d905d72ea2be31bcb984fe62c6e
Author: H. Nikolaus Schaller <hns@goldelico.com>

drm/fb_helper: Determine proper crtc_index within drm_fb_helper_fb_dirty

This commit updates the dirty framebuffer worker to search for a matching
active CRTC instead of hardcoding the wait for vblank to CRTC index 0. It
aims to avoid vblank timeouts when CRTC 0 is inactive.

> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
[ ... ]
> @@ -232,9 +232,23 @@ static void drm_fb_helper_fb_dirty(struct drm_fb_helper *helper)
>  	struct drm_clip_rect clip_copy;
>  	unsigned long flags;
>  	int ret;
> +	unsigned int crtc_index = 0;
> +	struct drm_crtc *crtc;
>  
>  	mutex_lock(&helper->lock);
> -	drm_client_modeset_wait_for_vblank(&helper->client, 0);
> +
> +	drm_for_each_crtc(crtc, dev) {
> +		if (crtc->primary && crtc->primary->state &&
> +		    crtc->primary->state->fb == helper->fb &&
> +		    crtc->state && crtc->state->active) {

[Severity: High]
Can this lockless access to crtc->state and crtc->primary->state result in a
use-after-free?

The drm_fb_helper_fb_dirty() function holds helper->lock, but does not hold
the DRM modeset locks required to protect these state pointers. During a
concurrent atomic modeset commit, the DRM core allocates new state objects,
swaps them into place, and frees the old objects via kfree().

If this code reads an old state pointer and gets preempted, could it then
dereference the pointer after it has been freed by the concurrent commit?

[Severity: High]
Does this break vblank synchronization for legacy non-atomic DRM drivers?

Legacy drivers do not use the atomic modesetting infrastructure, so their
crtc->state and crtc->primary->state pointers remain NULL. Since the condition
above requires these pointers to be non-NULL, the loop will skip the vblank
wait entirely for legacy drivers. This skips the intended rate-limiting
behavior and could lead to unbounded damage worker execution.

> +			crtc_index = drm_crtc_index(crtc) + 1;
> +			break;

[Severity: Medium]
The commit message states "If multiple CRTCs match, the last active one is
selected." Does the break statement here cause the first active CRTC to be
selected instead, bypassing the remaining matches in the list?

> +		}
> +	}
> +
> +	if (crtc_index > 0)
> +		drm_client_modeset_wait_for_vblank(&helper->client, crtc_index - 1);
> +
>  	mutex_unlock(&helper->lock);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/5118b37e579cec220f2d2b6ab0bb1ecfb67f0d5c.1781803617.git.hns@goldelico.com?part=1

Re: [PATCH] drm/fb_helper: Determine proper crtc_index within drm_fb_helper_fb_dirty

[H. Nikolaus Schaller]

> Am 18.06.2026 um 19:43 schrieb sashiko-bot@kernel.org:
> 
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] Lockless access to `crtc->state` and `crtc->primary->state` can lead to a Use-After-Free due to concurrent atomic commits.
> - [High] The patch breaks vblank synchronization for legacy (non-atomic) DRM drivers.
> - [Medium] The logic selects the first matching CRTC instead of the last active one, contradicting the commit message.
> --
> 
> commit 0337b26b606b4d905d72ea2be31bcb984fe62c6e
> Author: H. Nikolaus Schaller <hns@goldelico.com>
> 
> drm/fb_helper: Determine proper crtc_index within drm_fb_helper_fb_dirty
> 
> This commit updates the dirty framebuffer worker to search for a matching
> active CRTC instead of hardcoding the wait for vblank to CRTC index 0. It
> aims to avoid vblank timeouts when CRTC 0 is inactive.
> 
>> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
> [ ... ]
>> @@ -232,9 +232,23 @@ static void drm_fb_helper_fb_dirty(struct drm_fb_helper *helper)
>> struct drm_clip_rect clip_copy;
>> unsigned long flags;
>> int ret;
>> + unsigned int crtc_index = 0;
>> + struct drm_crtc *crtc;
>> 
>> mutex_lock(&helper->lock);
>> - drm_client_modeset_wait_for_vblank(&helper->client, 0);
>> +
>> + drm_for_each_crtc(crtc, dev) {
>> + if (crtc->primary && crtc->primary->state &&
>> +    crtc->primary->state->fb == helper->fb &&
>> +    crtc->state && crtc->state->active) {
> 
> [Severity: High]
> Can this lockless access to crtc->state and crtc->primary->state result in a
> use-after-free?

No idea...

> 
> The drm_fb_helper_fb_dirty() function holds helper->lock, but does not hold
> the DRM modeset locks required to protect these state pointers. During a

Which locks are exactly recommended to add? What is their name/pointer?

> concurrent atomic modeset commit, the DRM core allocates new state objects,
> swaps them into place, and frees the old objects via kfree().
> 
> If this code reads an old state pointer and gets preempted, could it then
> dereference the pointer after it has been freed by the concurrent commit?
> 
> [Severity: High]
> Does this break vblank synchronization for legacy non-atomic DRM drivers?

> 
> Legacy drivers do not use the atomic modesetting infrastructure, so their
> crtc->state and crtc->primary->state pointers remain NULL. Since the condition
> above requires these pointers to be non-NULL, the loop will skip the vblank
> wait entirely for legacy drivers. This skips the intended rate-limiting
> behavior and could lead to unbounded damage worker execution.

What is the solution? And is this important.

Before d8c4bddcd8bcb ("drm/fb-helper: Synchronize dirty worker with vblank")

there was also no rate-limiting behavior.

> 
>> + crtc_index = drm_crtc_index(crtc) + 1;
>> + break;
> 
> [Severity: Medium]
> The commit message states "If multiple CRTCs match, the last active one is
> selected." Does the break statement here cause the first active CRTC to be
> selected instead, bypassing the remaining matches in the list?

Yes, that is correct. The description is indeed wrong.

> 
>> + }
>> + }
>> +
>> + if (crtc_index > 0)
>> + drm_client_modeset_wait_for_vblank(&helper->client, crtc_index - 1);
>> +
>> mutex_unlock(&helper->lock);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/5118b37e579cec220f2d2b6ab0bb1ecfb67f0d5c.1781803617.git.hns@goldelico.com?part=1