On Mon Jul 20, 2026 at 10:08 PM CST, Maxime Ripard wrote:
> 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
>>
Thank you Thomas and Maxime for the detailed explanations.
I will follow the suggested pattern here:
>> 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);
IIUC, atomic_check is the last stage where valid to extend commit state.
After that, the state is fixed and helpers should only use
get_new_crtc_state() or get_old_crtc_state().
My only remaining question is about the doc warning [1]:
* WARNING: Drivers may only add new CRTC states to a @state if
* drm_atomic_state.allow_modeset is set, or if it's a driver-internal
* commit not created by userspace through an IOCTL call.
if we end up adding the CRTC state for a userspace non-modeset commit,
it could hit the requested/affected CRTC warning in
drm_atomic_check_only() [2]. That would still be more acceptable than
crashing, though.
[1]
https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/drm_atomic.c#L355
[2]
https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/drm_atomic.c#L1746
Thanks,
Ze
>>
>> 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
