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.
Best regards
Thomas
[1]
https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/drm_atomic_helper.c#L912
[2]
https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/drm_atomic_helper.c#L907
[3]
https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/mediatek/mtk_plane.c#L230
[4]
https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/tidss/tidss_plane.c#L31
--
--
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)
