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. Maxime
signature.asc
Description: PGP signature
