[PATCH v3 13/15] drm/i915/vrr: Restructure VRR enablement bit

[Nautiyal, Ankit K]

On 4/28/2025 11:50 AM, Mitul Golani wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> Restructure bit for VRR enablement.
>
> --v2:
> - Separate multiple enablement from one patch.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani at intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_vrr.c | 25 ++++++++++++------------
>   1 file changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 54b91c2a0a87..86b858222b6e 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -603,6 +603,7 @@ void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
>   {
>   	struct intel_display *display = to_intel_display(crtc_state);
>   	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
> +	u32 ctl;
>   
>   	if (!crtc_state->vrr.enable)
>   		return;
> @@ -617,16 +618,11 @@ void intel_vrr_enable(const struct intel_crtc_state *crtc_state)
>   	intel_de_write(display, TRANS_PUSH(display, cpu_transcoder),
>   		       TRANS_PUSH_EN);
>   
> -	if (!intel_vrr_always_use_vrr_tg(display)) {
> -		if (crtc_state->cmrr.enable) {
> -			intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
> -				       VRR_CTL_VRR_ENABLE | VRR_CTL_CMRR_ENABLE |
> -				       trans_vrr_ctl(crtc_state));
> -		} else {
> -			intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
> -				       VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state));
> -		}
> -	}
> +	ctl = VRR_CTL_VRR_ENABLE | trans_vrr_ctl(crtc_state);
> +	if (crtc_state->cmrr.enable)
> +		ctl |= VRR_CTL_CMRR_ENABLE;
> +
> +	intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), ctl);
>   
>   	if (HAS_VRR_DC_BALANCE(display) && crtc_state->vrr.dc_balance.enable) {
>   		intel_de_write(display, PIPEDMC_DCB_VMIN(display, cpu_transcoder),
> @@ -650,6 +646,7 @@ void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
>   {
>   	struct intel_display *display = to_intel_display(old_crtc_state);
>   	enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
> +	u32 ctl;
>   
>   	if (!old_crtc_state->vrr.enable)
>   		return;
> @@ -665,9 +662,13 @@ void intel_vrr_disable(const struct intel_crtc_state *old_crtc_state)
>   		intel_de_write(display, PIPEDMC_DCB_VBLANK(display, cpu_transcoder), 0);
>   	}
>   
> +	ctl = trans_vrr_ctl(old_crtc_state);
> +	if (intel_vrr_always_use_vrr_tg(display))
> +		ctl |= VRR_CTL_VRR_ENABLE;
> +
> +	intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder), ctl);
> +

This seems to introduce an extra write for TRANS_VRR_CTL for cases where 
we always want to enabled VRR TG.

Though there shouldn't be any difference, as the timing generator is 
already enabled with guardband and flipline_en bits set.

IMHO it would be good to mention the same in commit message.

In any case change looks alright to me.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>


Regards,

Ankit

>   	if (!intel_vrr_always_use_vrr_tg(display)) {
> -		intel_de_write(display, TRANS_VRR_CTL(display, cpu_transcoder),
> -			       trans_vrr_ctl(old_crtc_state));
>   		intel_de_wait_for_clear(display,
>   					TRANS_VRR_STATUS(display, cpu_transcoder),
>   					VRR_STATUS_VRR_EN_LIVE, 1000);