Re: [PATCH v2] drm/imx: replace struct drm_simple_display_pipe with regular atomic helpers

[Maxime Ripard <mripard@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4425 bytes --]

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?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]