Re: [PATCH 12/13] drm/i915/vrr: Always use VRR timing generator for XELPD+

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 9/3/2024 6:55 PM, Ville Syrjälä wrote:
> On Mon, Sep 02, 2024 at 01:36:33PM +0530, Ankit Nautiyal wrote:
>> Currently VRR timing generator is used only when VRR is enabled by
>> userspace. From XELPD+, gradually move away from older timing
>> generator and use VRR timing generator for fixed refresh rate also.
>> In such a case, Flipline VMin and VMax all are set to the Vtotal of the
>> mode, which effectively makes the VRR timing generator work in
>> fixed refresh rate mode.
>>
>> v2: Use VRR Timing Generator from XELPD+ instead of MTL as it needs
>> Wa_14015406119.
>> v3: Set vrr.fixed during vrr_get_config (Mitul)
>> v4: Avoid setting vrr.fixed when vrr.cmrr is enabled.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_vrr.c | 61 +++++++++++++++---------
>>   1 file changed, 39 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index e01d4b4b8fa7..625728aff5a2 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -172,41 +172,54 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>>   	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
>>   		return;
>>   
>> -	crtc_state->vrr.in_range =
>> -		intel_vrr_is_in_range(connector, drm_mode_vrefresh(adjusted_mode));
>> -	if (!crtc_state->vrr.in_range)
>> -		return;
>> -
>>   	if (HAS_LRR(display))
>>   		crtc_state->update_lrr = true;
> We aren't supposed to do a LRR update unless the refresh rates are
> within the VRR range. So this needs to be moved as well.

Noted. Will move this in the other block.


>
>>   
>> -	vmin = DIV_ROUND_UP(adjusted_mode->crtc_clock * 1000,
>> -			    adjusted_mode->crtc_htotal * info->monitor_range.max_vfreq);
>> -	vmax = adjusted_mode->crtc_clock * 1000 /
>> -		(adjusted_mode->crtc_htotal * info->monitor_range.min_vfreq);
>> +	if (!crtc_state->uapi.vrr_enabled && DISPLAY_VER(display) >= 20) {
>> +		/*
>> +		 * for XELPD+ always go for VRR timing generator even for
>> +		 * fixed refresh rate.
>> +		 */
>> +		crtc_state->vrr.vmin = adjusted_mode->crtc_vtotal;
>> +		crtc_state->vrr.vmax = adjusted_mode->crtc_vtotal;
>> +		crtc_state->vrr.flipline = adjusted_mode->crtc_vtotal;
> Has the "flipline can't be below vmin+1" issue been changed in the hardware?

Need to check this, will get back on this.


>
>> +		crtc_state->vrr.fixed_rr = true;
>> +	} else {
>> +		crtc_state->vrr.in_range =
>> +			intel_vrr_is_in_range(connector, drm_mode_vrefresh(adjusted_mode));
>>   
>> -	vmin = max_t(int, vmin, adjusted_mode->crtc_vtotal);
>> -	vmax = max_t(int, vmax, adjusted_mode->crtc_vtotal);
>> +		if (!crtc_state->vrr.in_range)
>> +			return;
>>   
>> -	if (vmin >= vmax)
>> -		return;
>> +		vmin = DIV_ROUND_UP(adjusted_mode->crtc_clock * 1000,
>> +				    adjusted_mode->crtc_htotal * info->monitor_range.max_vfreq);
>> +		vmax = adjusted_mode->crtc_clock * 1000 /
>> +			(adjusted_mode->crtc_htotal * info->monitor_range.min_vfreq);
>>   
>> -	/*
>> -	 * flipline determines the min vblank length the hardware will
>> -	 * generate, and flipline>=vmin+1, hence we reduce vmin by one
>> -	 * to make sure we can get the actual min vblank length.
>> -	 */
>> -	crtc_state->vrr.vmin = vmin - 1;
>> -	crtc_state->vrr.vmax = vmax;
>> +		vmin = max_t(int, vmin, adjusted_mode->crtc_vtotal);
>> +		vmax = max_t(int, vmax, adjusted_mode->crtc_vtotal);
>> +
>> +		if (vmin >= vmax)
>> +			return;
>> +
>> +		/*
>> +		 * flipline determines the min vblank length the hardware will
>> +		 * generate, and flipline>=vmin+1, hence we reduce vmin by one
>> +		 * to make sure we can get the actual min vblank length.
>> +		 */
>> +		crtc_state->vrr.vmin = vmin - 1;
>> +		crtc_state->vrr.vmax = vmax;
>>   
>> -	crtc_state->vrr.flipline = crtc_state->vrr.vmin + 1;
>> +		crtc_state->vrr.flipline = crtc_state->vrr.vmin + 1;
>> +		crtc_state->vrr.fixed_rr = false;
>> +	}
>>   
>>   	/*
>>   	 * When panel is VRR capable and userspace has
>>   	 * not enabled adaptive sync mode then Fixed Average
>>   	 * Vtotal mode should be enabled.
>>   	 */
>> -	if (crtc_state->uapi.vrr_enabled) {
>> +	if (crtc_state->uapi.vrr_enabled || crtc_state->vrr.fixed_rr) {
>>   		crtc_state->vrr.enable = true;
>>   		crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
> Hmm. This is now a bit of a mess. We need to come up with a sane way to
> deal with the vblank timestamping code. Dunno if we want to make it so
> that we'd always use VRR timings or the non-VRR timings. Should be
> identical from HW POV so technically might not matter, apart from the
> software state tracking POV. And from that angle it seems to me that
> for the dynamic fixed vs. variable swithcing we might want to keep the
> code using the non-VRR timings for fixed refresh rate.

So do we set I915_MODE_FLAG_VRR only when actual vrr is used, (ie. when 
the vrr property is set)?


>
> There seems to other stuff amiss still:
> - We don't enable/disable the VRR timings generator early/late
>    in the modeset sequence?
> - How do we atomically switch between the fixed vs. variable
>    timings since we can't temporarily disable the VRR timing generator?

Yeah, with current changes vrr.enable is always true.
Perhaps I should have added a check for disabling(fixed_rr, ..) in 
intel_crtc_vrr_disabling.
Will get back on this as well.

MST + VRR is also one of the thing yet to be handled.


Thank you Ville, for the comments and guidance.

Regards,

Ankit


>
>>   	} else if (is_cmrr_frac_required(crtc_state) && is_edp) {
>> @@ -421,6 +434,10 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>>   						     TRANS_VRR_VMAX(display, cpu_transcoder)) + 1;
>>   		crtc_state->vrr.vmin = intel_de_read(display,
>>   						     TRANS_VRR_VMIN(display, cpu_transcoder)) + 1;
>> +		if (!crtc_state->cmrr.enable &&
>> +		    crtc_state->vrr.vmax == crtc_state->vrr.flipline &&
>> +		    crtc_state->vrr.vmin == crtc_state->vrr.flipline)
>> +			crtc_state->vrr.fixed_rr = true;
>>   	}
>>   
>>   	if (crtc_state->vrr.enable) {
>> -- 
>> 2.45.2