在 2026-06-22一的 11:11 +0200，Thomas Zimmermann写道： > Hi > > Am 19.06.26 um 18:59 schrieb Icenowy Zheng: > > 在 2026-06-19五的 10:50 +0200，Thomas Zimmermann写道： > > > (cc Icenowy) > > > > > > Hi, > > > > > > there are a number of similar reports. Does the attached patch > > > fix > > > the > > > problem. It picks up what you do, but in a more correct way. > > Yes, this patch looks okay, but I wonder whether the crtc index > > query > > should be done in drm_client_modeset_wait_for_vblank() instead. > > There's another call to this helper from the ioctl helper. It has a > lengthy comment about using CRTC 0 only and how these indices don't > make > much sense with fbdev. I'll look into generalizing the code a bit,
I think both codepaths are based on the same (wrong) assumption that CRTC 0 is always the first used CRTC, so that codepath should also be fixed; integrating the logic into drm_client_modeset_wait_for_vblank() will not affect drivers that use CRTC 0 first, but will make drivers that have fixed CRTC preference work with FBIO_WAITFORVSYNC too (which currently do not work). Thanks, Icenowy > but > I'd rather not fully integrate the logic into > drm_client_modeset_wait_for_vblank(). > > Best regards > Thomas > > > > > I will test this patch this weekend. > > > > Thanks, > > Icenow > > > > > Best regards > > > Thomas > > > > > > Am 18.06.26 um 19:56 schrieb H. Nikolaus Schaller: > > > > > Am 18.06.2026 um 19:43 schrieb [email protected]: > > > > > > > > > > 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 <[email protected]> > > > > > > > > > > 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....@goldelico.com?part=1
