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: 3642 bytes --]

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.

Maxime

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