Re: [PATCH 2/2] drm/i915/display: Avoid use of VTOTAL.Vtotal bits

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

On 3/26/2025 10:39 PM, Ville Syrjälä wrote:
> On Wed, Mar 26, 2025 at 09:33:21PM +0530, Ankit Nautiyal wrote:
>> For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
>> bits are not required. Since the support for these bits is going to
>> be deprecated in upcoming platforms, avoid writing these bits for the
>> platforms that do not use legacy Timing Generator.
>>
>> Since for these platforms TRAN_VMIN is always filled with crtc_vtotal,
>> use TRAN_VRR_VMIN to get the vtotal for adjusted_mode.
>>
>> v2: Avoid having a helper for manipulating VTOTAL register, and instead
>> just make the change where required. (Ville)
>> v3: Set `crtc_vtotal` instead of working with the bits directly (Ville).
>> Use intel_vrr_vmin_vtotal() to set the vtotal during readout. (Ville)
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display.c | 31 +++++++++++++++++++-
>>   drivers/gpu/drm/i915/display/intel_vrr.c     | 10 +++++++
>>   2 files changed, 40 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index 0db1cd4fc963..6796dd0307a6 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -2698,9 +2698,19 @@ static void intel_set_transcoder_timings(const struct intel_crtc_state *crtc_sta
>>   		       HSYNC_START(adjusted_mode->crtc_hsync_start - 1) |
>>   		       HSYNC_END(adjusted_mode->crtc_hsync_end - 1));
>>   
>> +	/*
>> +	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
>> +	 * bits are not required. Since the support for these bits is going to
>> +	 * be deprecated in upcoming platforms, avoid writing these bits for the
>> +	 * platforms that do not use legacy Timing Generator.
>> +	 */
>> +	if (intel_vrr_always_use_vrr_tg(display))
>> +		crtc_vtotal = 1;
>> +
>>   	intel_de_write(display, TRANS_VTOTAL(display, cpu_transcoder),
>>   		       VACTIVE(crtc_vdisplay - 1) |
>>   		       VTOTAL(crtc_vtotal - 1));
>> +
> spurious whitespace change
>
>>   	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
>>   		       VBLANK_START(crtc_vblank_start - 1) |
>>   		       VBLANK_END(crtc_vblank_end - 1));
>> @@ -2758,6 +2768,15 @@ static void intel_set_transcoder_timings_lrr(const struct intel_crtc_state *crtc
>>   	intel_de_write(display, TRANS_VBLANK(display, cpu_transcoder),
>>   		       VBLANK_START(crtc_vblank_start - 1) |
>>   		       VBLANK_END(crtc_vblank_end - 1));
>> +	/*
>> +	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
>> +	 * bits are not required. Since the support for these bits is going to
>> +	 * be deprecated in upcoming platforms, avoid writing these bits for the
>> +	 * platforms that do not use legacy Timing Generator.
>> +	 */
>> +	if (intel_vrr_always_use_vrr_tg(display))
>> +		crtc_vtotal = 1;
>> +
>>   	/*
>>   	 * The double buffer latch point for TRANS_VTOTAL
>>   	 * is the transcoder's undelayed vblank.
>> @@ -2827,7 +2846,17 @@ static void intel_get_transcoder_timings(struct intel_crtc *crtc,
>>   
>>   	tmp = intel_de_read(display, TRANS_VTOTAL(display, cpu_transcoder));
>>   	adjusted_mode->crtc_vdisplay = REG_FIELD_GET(VACTIVE_MASK, tmp) + 1;
>> -	adjusted_mode->crtc_vtotal = REG_FIELD_GET(VTOTAL_MASK, tmp) + 1;
>> +
>> +	/*
>> +	 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
>> +	 * bits are not filled. The value for adjusted_mode->crtc_vtotal is read
>> +	 * from VRR_VMIN register in intel_vrr_get_config.
>> +	 * Just set this to 0 here.
>> +	 */
>> +	if (intel_vrr_always_use_vrr_tg(display))
> This one either needs the transcoder_has_vrr() check, or we could just
> keep on blindly reading this anyway, and let intel_vrr_get_config()
> overwrite it afterwards. That's kinda how we deal with
> TRANS_SET_CONTEXT_LATENCY as well.

Understood. I think will just let vtotal be filled here and for 
platforms that always use VRR TG it will get overwritten in 
intel_vrr_get_config.

Regards,

Ankit

>
>> +		adjusted_mode->crtc_vtotal = 0;
>> +	else
>> +		adjusted_mode->crtc_vtotal = REG_FIELD_GET(VTOTAL_MASK, tmp) + 1;
>>   
>>   	/* FIXME TGL+ DSI transcoders have this! */
>>   	if (!transcoder_is_dsi(cpu_transcoder)) {
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index 414f93851059..7359d66fc091 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -708,6 +708,16 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>>   		crtc_state->vrr.vmin = intel_de_read(display,
>>   						     TRANS_VRR_VMIN(display, cpu_transcoder)) + 1;
>>   
>> +		/*
>> +		 * For platforms that always use VRR Timing Generator, the VTOTAL.Vtotal
>> +		 * bits are not filled. Since for these platforms TRAN_VMIN is always
>> +		 * filled with crtc_vtotal, use TRAN_VRR_VMIN to get the vtotal for
>> +		 * adjusted_mode.
>> +		 */
>> +		if (intel_vrr_always_use_vrr_tg(display))
>> +			crtc_state->hw.adjusted_mode.crtc_vtotal =
>> +				intel_vrr_vmin_vtotal(crtc_state);
>> +
>>   		if (HAS_AS_SDP(display)) {
>>   			trans_vrr_vsync =
>>   				intel_de_read(display,
>> -- 
>> 2.45.2