Re: [PATCH v1 1/3] drm: Add passive_vrr properties for passive/desktop VRR

2026-09-01 Thread Tomasz Pakuła 
On Tue, 2026-09-01 at 15:12 -0400, Fangzhi Zuo wrote:
> Add the PASSIVE_VRR_DISABLED atomic CRTC property

Michel Dänzer already commented on this:
https://lore.kernel.org/amd-gfx/[email protected]/

"Having a negation term like "disabled" in the property name can be
confusing (as it involves double negation when the property value is 0)
and is better avoided."

To this day, I'm not sure how to better handle this, without the driver
just forcing VRR on it's own. Consequently, this will enforce ALLM so
probably there is a benefit to leaving this configurable and somehow
default for HDMI.

IDK, smarter people can probably deal with this, just wanted to forward
the comments I received previously.

On windows, AMD driver just doesn't care and forces VRR signaling all
the time, without a way to disable it.

Re: [PATCH v1 1/3] drm: Add passive_vrr properties for passive/desktop VRR

2026-09-01 Thread sashiko-bot 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) 
to consider:
- [Low] Contradictory kerneldoc for `passive_vrr_disabled` in `struct 
drm_crtc_state` and multiple spelling errors in comments.
--

commit 870f0fcb955a90bd32f228b223d6193abfa63290
Author: Tomasz Pakuła 
Subject: drm: Add passive_vrr properties for passive/desktop VRR

This commit adds atomic CRTC and immutable connector properties to support
passive VRR for desktop usage on HDMI sinks. This aims to avoid blanking or
flickering on VRR entry and exit by keeping the sink in a variable-refresh
state during fixed refresh use.

> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 9d820a2a87ce4..c429105172b0d 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
[ ... ]
> @@ -2386,6 +2396,17 @@ EXPORT_SYMBOL(drm_mode_create_scaling_mode_property);
>   *
>   *   The driver may place further restrictions within these minimum
>   *   and maximum bounds.
> + *
> + * "PASSIVE_VRR_DISABLED":
> + *   Default &drm_crtc boolean property that notifies the driver that the
> + *   VRR singalling should be disabled in fixed refresh rate scenarios.
> + *   Functionally, psssive vrr works the same as VRR_ENABLED == false

[Severity: Low]
This isn't a bug, but there are a couple of spelling errors in the uAPI
documentation here: "singalling" should be "signalling", and "psssive"
should be "passive".

> + *   but works around displays blanking (mainly HDMI) that do not support
> + *   seamless VRR transitions. Also helps with brightness flickering during
> + *   VRR transitions.
> + *
> + *   Passive VRR mode is not that useful for DP/eDP sinks where seamless VRR
> + *   transitions are enforced by the standard.
>   */
[ ... ]
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index c6dbe8b7db9ee..642a374f403e6 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -311,6 +311,15 @@ struct drm_crtc_state {
>*/
>   bool vrr_enabled;
>  
> + /**
> +  * @passive_vrr_disabled:
> +  *
> +  * Indicates if variable refresh rate on desktop should be enabled for
> +  * the CRTC. Support for the requested state will depend on driver and
> +  * hardware capabiltiy - lacking support is not treated as failure.
> +  */
> + bool passive_vrr_disabled;
> +

[Severity: Low]
This isn't a bug, but the documentation states this field "Indicates if
variable refresh rate on desktop should be enabled", which contradicts the
variable name passive_vrr_disabled. Should this description read "disabled"
instead?

Also, there is a spelling error in "capabiltiy" (capability).

-- 
Sashiko AI review · 
https://sashiko.dev/#/patchset/[email protected]?part=1