On Mon Sep 14, 2026 at 8:11 PM CST, Thomas Zimmermann wrote:
> Hi
>
> Am 21.07.26 um 11:46 schrieb Ze Huang:
>> On Mon Jul 20, 2026 at 10:08 PM CST, Maxime Ripard wrote:
>>> 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
>>>>
>> Thank you Thomas and Maxime for the detailed explanations.
>>
>> I will follow the suggested pattern here:
>>
>>>> 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);
>
> Is there an update to this patch available?
Yes, here:
https://lore.kernel.org/all/20260727-drm-simple-kms-removal-v3-0-de36e534f...@oss.qualcomm.com/
>
>> IIUC, atomic_check is the last stage where valid to extend commit state.
>> After that, the state is fixed and helpers should only use
>> get_new_crtc_state() or get_old_crtc_state().
>>
>> My only remaining question is about the doc warning [1]:
>>
>> * WARNING: Drivers may only add new CRTC states to a @state if
>> * drm_atomic_state.allow_modeset is set, or if it's a driver-internal
>> * commit not created by userspace through an IOCTL call.
>
> I don't think this affects your work. If you just inline the existing
> simple-pipe it should be all good. (Or you duplicated existing bugs, but
> that doesn't make it worse either. :)
>
> Best regards
> Thomas
Thanks :)
>
>>
>> if we end up adding the CRTC state for a userspace non-modeset commit,
>> it could hit the requested/affected CRTC warning in
>> drm_atomic_check_only() [2]. That would still be more acceptable than
>> crashing, though.
>>
>> [1]
>> https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/drm_atomic.c#L355
>> [2]
>> https://elixir.bootlin.com/linux/v7.1.3/source/drivers/gpu/drm/drm_atomic.c#L1746
>>
>> Thanks,
>> Ze
>>
>>>> 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
>>
