Re: [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.syrj...@linux.intel.com>

Restructure bit for VRR enablement.

--v2:
- Separate multiple enablement from one patch.

Signed-off-by: Ville Syrjälä <ville.syrj...@linux.intel.com>
Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.gol...@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.nauti...@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);