Re: [PATCH 02/16] drm/i915/vrr: Avoid reading vrr.enable based on fixed_rr check

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Fri, Mar 21, 2025 at 11:10:05PM +0530, Nautiyal, Ankit K wrote:
> 
> On 3/21/2025 11:04 PM, Ville Syrjälä wrote:
> > On Tue, Mar 18, 2025 at 01:05:26PM +0530, Ankit Nautiyal wrote:
> >> Currently, vrr.enable is intended only for variable refresh rate timings.
> >> At this point, we do not set fixed refresh rate timings, but the GOP can,
> >> which creates a problem during the readback of vrr.enable.
> >>
> >> The GOP enables the VRR timing generator with fixed timings, while the
> >> driver only recognizes the VRR timing generator as enabled with
> >> variable timings. This discrepancy causes an issue due to the
> >> fixed refresh rate check during readback. Since the VRR timing generator
> >> is enabled and we do not support fixed timings, the readback should set
> >> vrr.enable so that the driver can disable the VRR timing generator.
> >> However, the current check does not allow this.
> >>
> >> Therefore, remove the fixed refresh rate check during readback.
> >>
> >> Fixes: 27217f9d1856 ("drm/i915/vrr: Track vrr.enable only for variable timing")
> >> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/display/intel_vrr.c | 3 +--
> >>   1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> >> index aa65a6933ddb..6bdcdfed4b9b 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> >> @@ -657,8 +657,7 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
> >>   		}
> >>   	}
> >>   
> >> -	crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE &&
> >> -				 !intel_vrr_is_fixed_rr(crtc_state);
> >> +	crtc_state->vrr.enable = trans_vrr_ctl & VRR_CTL_VRR_ENABLE;
> > Doesn't this break the state checker when we use the VRR timing
> > generator for fixed refresh modes?
> 
> Yes right, but currently those changes are not merged yet.
> 
> I have something like this [1], when we add support for VRR TG with 
> fixed refresh rate.
> 
> [1] https://patchwork.freedesktop.org/patch/643470/?series=134383&rev=16

Yeah, just saw it.

Given that, this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> 
> Regards,
> 
> Ankit
> 
> 
> >
> >>   
> >>   	/*
> >>   	 * #TODO: For Both VRR and CMRR the flag I915_MODE_FLAG_VRR is set for mode_flags.
> >> -- 
> >> 2.45.2

-- 
Ville Syrjälä
Intel