Changes to `vrr_enabled` property

[Ville Syrjälä]

On Thu, Sep 19, 2024 at 01:14:03PM -0500, Vidith Madhu wrote:
> Hi all,
> I'd like to discuss a potential opportunity to enhance the `vrr_enabled` property of `drm_crtc_state`. Currently, this property is a Boolean flag, but this exposes some ambiguity and potential disconnect between client and driver/display behavior. Namely, it's unclear to the client whether it must allow a modeset for changes to `vrr_enabled`. I understand that the DP spec mandates seamless VRR transitions, but there is no such requirement for HDMI. Therefore, it is possible for clients to toggle `vrr_enabled` without setting `allow_modeset`, when in fact the display requires a modeset and/or display blanking. There are multiple ways this might be handled, each of which has flaws:
> 
> 1. The driver proceeds with a modeset/blanking transition anyways, if the display requires it. As I understand, this is how amdgpu and i915 work [1, 2]. This is a problem because any Wayland compositor that has the "Automatic" option for VRR may transition in and out at unexpected moments, causing disruptive blanking effects.

i915 does not need a full modeset to toggle VRR. 
Originally we did, but no more.

> 
> 2. The driver initially forces all VRR-capable displays into VRR mode, and controls whether the refresh rate is fixed or variable on-the-fly. This is a problem because many displays have features like ULMB, motion interpolation, black frame insertion, etc.  that are unavailable when the display is in VRR mode. This is how the NVIDIA driver currently handles the situation; there have been user complaints about this [3].

Currently i915 always puts the display into "VRR mode"
(ie. just sets the "ignore MSA timings" bit in DPCD), but
I think we could change that to be set only VRR is actually
enabled (if displays are actually using that as an indication
to disable random features, which might not even be what [3]
is saying because it talks about g-sync and not vrr).

Still shouldn't require a full modeset IIRC. Though I haven't
checked what implications this would have for the adaptive sync
SDP.

No idea what HDMI VRR toggling would entail.

> 
> 3. The driver rejects the request. This is a uAPI violation, as currently `allow_modeset` is not required for changes to `vrr_enabled`.

The uapi does not specify that AFAIK.

IIRC I told the kwin people that they shouldn't depend on being able
to toggle VRR without a modeset but IIRC they said something along
the lines of "it's too hard to enable VRR already when lighting up
the display", hence you couldn't use i915+VRR+kwin until we
eliminated the full modeset from the driver.

> 
> I would suggest the following approach: Make `vrr_enabled` an enum of {TRUE, INACTIVE, FALSE}. FALSE would shut off VRR on the display (potentially with a modeset), and TRUE/INACTIVE would turn it on (again, potentially with modeset). INACTIVE would have the driver lock a fixed refresh rate while keeping VRR enabled on display. Transitioning between (TRUE/INACTIVE) and (FALSE) on any CRTC would require `allow_modeset` in `drm_atomic_state` to be set, else the request will fail. In addition, it would be the driver's responsibility to implement the distinction between TRUE and INACTIVE. With this change, clients are forced to declare their intentions clearly, and driver-side ambiguity is eliminated.

You would need to introduce a new property instead of breaking
the old one. Dunno if this would help kwin at all though, if
they couldn't set the current property at modeset time then
not sure how they could set the new property either. Shrug.

>  
> I would appreciate any feedback or discussion about this.
> 
> References:
> 1. https://gitlab.freedesktop.org/drm/amd/-/issues/2200#note_2160244
> 2. https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/7542#note_1643183
> 3. https://forums.developer.nvidia.com/t/cant-enable-ulmb-2-on-wayland/277478
> 
> Thanks,
> Vidith

-- 
Ville Syrjälä
Intel