On Mon, Jul 20, 2026 at 04:38:26PM +0200, Thomas Zimmermann wrote:
> Am 20.07.26 um 16:08 schrieb Maxime Ripard:
> > On Mon, Jul 20, 2026 at 02:36:19PM +0200, Thomas Zimmermann wrote:
> > > Hi
> > >
> > > Am 18.07.26 um 21:12 schrieb Ze Huang:
> > > [...]
> > > > > > - if (event) {
> > > > > > - crtc->state->event = NULL;
> > > > > > +static int imx_lcdc_plane_helper_atomic_check(struct drm_plane
> > > > > > *plane,
> > > > > > + struct drm_atomic_commit
> > > > > > *commit)
> > > > > > +{
> > > > > > + struct drm_plane_state *plane_state =
> > > > > > drm_atomic_get_new_plane_state(commit, plane);
> > > > > > + struct drm_crtc *crtc = plane_state->crtc;
> > > > > > + struct drm_crtc_state *crtc_state = NULL;
> > > > > > - spin_lock_irq(&crtc->dev->event_lock);
> > > > > > + if (crtc)
> > > > > > + crtc_state = drm_atomic_get_new_crtc_state(commit,
> > > > > > crtc);
> > > > > > - if (crtc->state->active && drm_crtc_vblank_get(crtc) ==
> > > > > > 0)
> > > > > > - drm_crtc_arm_vblank_event(crtc, event);
> > > > > > - else
> > > > > > - drm_crtc_send_vblank_event(crtc, event);
> > > > > > + return drm_atomic_helper_check_plane_state(plane_state,
> > > > > > crtc_state,
> > > > > [Severity: High]
> > > > > Can fetching the CRTC state this way cause a kernel panic in the core
> > > > > atomic
> > > > > helpers?
> > > > >
> > > > > If userspace submits a commit that modifies only plane properties
> > > > > without
> > > > > affecting the CRTC, the CRTC is not implicitly added to the atomic
> > > > > commit
> > > > > state.
> > > > >
> > > > > In this scenario, drm_atomic_get_new_crtc_state() returns NULL.
> > > > > Passing this
> > > > > NULL pointer directly to drm_atomic_helper_check_plane_state() causes
> > > > > a crash
> > > > > when it dereferences crtc_state->enable.
> > > > >
> > > > > Could this use drm_atomic_get_crtc_state() instead to ensure the
> > > > > state is
> > > > > brought into the commit if it is missing?
> > > > I think it is fine here; I'll just copy the pattern from [1].
> > > >
> > > > [1]
> > > > https://elixir.bootlin.com/linux/v7.1.2/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L487
> > > It could be that there's a long standing problem in the overall logic. Not
> > > having a CRTC (and hence crtc_state) should also mean !fb, so we'd return
> > > at
> > > [1]. If we have a CRTC on the plane but pass a crtc_state of NULL, we
> > > could
> > > get a panic at [2], where it does crtc_state->crtc. I'm not aware of any
> > > bug reports about this problem, but it's still an issue.
> > >
> > > A number of drivers get this wrong by using
> > > drm_atomic_helper_get_new_crtc_state(). The bot suggests to use
> > > drm_atomic_helper_get_crtc_state() instead. This helper also returns the
> > > new state. But if there's no new state, it duplicates the CRTC's existing
> > > state. That's a bit of an overhead, but probably not an issue. Several
> > > drivers use this helper, but also get it wrong. They tend to return early
> > > in
> > > the case of !crtc or !fb without calling _check_plane_state(). See [3] and
> > > [4] for examples.
> > >
> > > I think, going with the bot's suggestion to use
> > > drm_atomic_helper_get_crtc_state() might be the best resolution for now.
> > > It
> > > still needs a crtc pointer, so the pattern is
> > >
> > > crtc_state = NULL
> > > if (plane_state->crtc)
> > > crtc_state = drm_atomic_helper_get_crtc_state(plane_state->crtc)
> > >
> > > _check_plane_state(plane_state, crtc_state);
> > >
> > > And in this case, _check_plane_state() should work correctly. But you can
> > > only use _get_crtc_state() in the atomic_check helpers! In the
> > > atomic_update, atomic_enable, etc helpers, it's too late for the helper to
> > > copy the CRTC state.
> > >
> > > I think some other DRM dev should look over this as well. It's one of the
> > > trickier things in DRM to get right.
> > drm_atomic_helper_get_crtc_state is safe in atomic_check. It's
> > everything after that must use either get_new_crtc_state or
> > get_old_crtc_state, as the global state cannot be modified anymore.
>
> Thanks a lot for confirming.
>
> Wrt. the logic in plane atomic_check, we might have to fix a number of
> drivers. As I outlined above, some use _get_new_crtc_state(), some use
> _get_crtc_state() incorrectly. But that's for another series.Sigh... I removed all of them a couple of years ago, I guess some crept back in. Maybe we should warn loudly if it happens? Maxime
signature.asc
Description: PGP signature
