On Tue, Jul 21, 2026 at 11:37:32AM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 21.07.26 um 11:28 schrieb Maxime Ripard:
> > On Mon, Jul 20, 2026 at 05:06:41PM +0200, Maxime Ripard wrote:
> > > 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?
> > I started to look into it for drm_atomic_helper_get_crtc_state and I
> > cannot find an occurence of this in drm-misc-next. Do you have a
> > specific example in mind?
>
> It's called drm_atomic_get_crtc_state(). [1] :)Yeah, sorry :) > I've not evaluated all calls, but there are cases like [2], which > returns early from !fb before running the whole > drm_atomic_helper_check_plane_state() machinery. It should have at > least set plane_state->visible to false. Better would be to call > drm_atomic_helper_check_plane_state() in any case. Oh right, I misunderstood what you were saying then. I thought you were saying that there's drm_atomic_get_crtc_state() calls in !atomic_check, which doesn't seem to be the case. Maxime
signature.asc
Description: PGP signature
