Hi,
On 18/2/26 16:04, Thomas Zimmermann wrote:
> 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.
Ah - gotcha. I'll do some testing with the conditional removed.
The worry about changing command sequences was a bit pedantic anyway -
DPMS would have caused an instant oops in the current tree and nobody else
has reported anything yet.
Would y'all be happy to take a v5 afterwards? Apologies for all the
iterations here :/
Thanks,
Shenghao
