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)
