Hi
Am 21.07.26 um 11:46 schrieb Ze Huang:
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);
Is there an update to this patch available?
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.
I don't think this affects your work. If you just inline the existing
simple-pipe it should be all good. (Or you duplicated existing bugs, but
that doesn't make it worse either. :)
Best regards
Thomas
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
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)
