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

[Thomas Zimmermann <tzimmermann@suse.de>]

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)