Re: [PATCH v3] drm/gud: fix NULL crtc dereference on display disable

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 18.02.26 um 05:22 schrieb Shenghao Yang:
> Hi,
>
> Thanks for taking a look!
>
> On 6/2/26 15:52, Thomas Zimmermann wrote:
>
>>>    +void gud_crtc_atomic_enable(struct drm_crtc *crtc,
>>> +               struct drm_atomic_state *state)
>>> +{
>>> +    struct drm_device *drm = crtc->dev;
>>> +    struct gud_device *gdrm = to_gud_device(drm);
>>> +    int idx;
>>> +
>>> +    if (!drm_dev_enter(drm, &idx))
>>> +        return;
>>> +
>>> +    if (crtc->state->mode_changed || crtc->state->connectors_changed) {
>> I think you can do this unconditionally. Atomic_enable is supposed to be a full modeset and take some time.  For simple pageflips, this function doesn't run at all.
>>
> I was trying this with proptest and toggling DPMS - the atomic_disable() and
> atomic_enable() functions get called in that path even if the mode doesn't
> change.
>
> The driver in 6.12 LTS (before the atomic changes) didn't send
> SET_CONTROLLER_ENABLE commands on DPMS changes either, so I thought it'd be
> safer to do the same here.

Leave it as it is if you like. But in DRM, we don't have real DPMS 
support for atomic modesetting. The DPMS on state is a full 
atomic_enable and the off and blanking states are atomic_disable IIRC. 
Doing that would be the correct handling.

Best regards
Thomas

>
>>> +        gud_usb_set_u8(gdrm, GUD_REQ_SET_CONTROLLER_ENABLE, 1);
>>> +        gud_usb_set(gdrm, GUD_REQ_SET_STATE_COMMIT, 0, NULL, 0);
>>> +    }
>>> +
>>> +    gud_usb_set_u8(gdrm, GUD_REQ_SET_DISPLAY_ENABLE, crtc->state->active);
>> Why not write '1' unconditionally? IIRC the active bit is always true in atomic_enable.
>>
> Yup looking at drm_atomic_helper_commit_crtc_enable() the call to
> atomic_enable() is skipped if !active - I'll sort this in v4!
>
> drm_atomic_helper_commit_crtc_enable(struct drm_device *dev, struct drm_atomic_state *state)
> {
> ...
> 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> ...
> 		if (!new_crtc_state->active)
> 			continue;
> }
>
> Shenghao
>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)