Hi
Am 20.07.26 um 17:06 schrieb Maxime Ripard:
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 think the handling of atomic state is non-trivial and not easily
discoverable. So yeah. Once sorted out, there should probably be a
WARN_ONCE of some kind.
Best regards
Thomas
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)
