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.
>> + 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
