Re: [PATCH 06/28] drm/i915/vrr: Fill VRR timing generator mode for CMRR and VRR

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

On 2/13/2025 2:36 AM, Ville Syrjälä wrote:
> On Mon, Feb 03, 2025 at 06:08:18PM +0530, Ankit Nautiyal wrote:
>> Fill vrr.mode during compute_config and update intel_vrr_get_config() to
>> read vrr.mode based on CMRR and VRR enable conditions.
> This vrr.mode thing still feels like a bit of a distraction at this
> point in the series. If we canskip this stuff for now I think we
> should be able to get this done more simply.

Alright, so I will avoid introducing vrr.mode at this series.

Do you see any value to introduce this later?


>
> The other thing that seems to complicate things is the attempt at doing
> something to intel_crtc_update_active_timings(). I think it'll be
> easier to just essentially pretend that things are running with the
> legacy timing generator when using fixed refresh rate. That way
> we don't have to touch intel_crtc_update_active_timings() at all.
Okay will change this thing.
>
> So basically just:
> - add the vmin/vmax/flipline reprogramming to vrr_disable() and vrr_enable()
> - make the timing generator enable/disable optional in those places
> - add the stuff to the modeset sequence to program the initial
>    timings (ie. fixed timings) and enable/disable the timing generator
> - also update to the new fixed timings in intel_set_transcoder_timings_lrr()

Alright, I was missing this thing.


> - adjust readout to not set vrr.enable when vmax==vmin_flipline

So essentially vrr.enable will track if variable timings are used or not.

Means we will not track if vrr timing generator is enable or not, but 
just if its running with variable timings.


>
> I think that should work perfectly fine for adl+. icl/tgl will be
> slightly wrong due to the weird extra scanline delay after
> vactive, but I don't think we should really have to care about that
> since we aren't going to enable this on those platforms anyway.

Alright. Will make changes and send new version.

Perhaps I will come back if I face any issues.

Thanks for the comments and suggestions.


Regards,

Ankit


>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display.c | 1 +
>>   drivers/gpu/drm/i915/display/intel_vrr.c     | 5 +++++
>>   2 files changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index a6383ddde871..9cff080d4ff9 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -5803,6 +5803,7 @@ intel_pipe_config_compare(const struct intel_crtc_state *current_config,
>>   
>>   	if (!fastset) {
>>   		PIPE_CONF_CHECK_BOOL(vrr.enable);
>> +		PIPE_CONF_CHECK_X(vrr.mode);
>>   		PIPE_CONF_CHECK_I(vrr.vmin);
>>   		PIPE_CONF_CHECK_I(vrr.vmax);
>>   		PIPE_CONF_CHECK_I(vrr.flipline);
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index 6f314e209e96..ded5466c5214 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -227,6 +227,7 @@ void intel_vrr_compute_cmrr_timings(struct intel_crtc_state *crtc_state)
>>   {
>>   	crtc_state->vrr.enable = true;
>>   	crtc_state->cmrr.enable = true;
>> +	crtc_state->vrr.mode = INTEL_VRRTG_MODE_CMRR;
>>   	/*
>>   	 * TODO: Compute precise target refresh rate to determine
>>   	 * if video_mode_required should be true. Currently set to
>> @@ -243,6 +244,7 @@ static
>>   void intel_vrr_compute_vrr_timings(struct intel_crtc_state *crtc_state)
>>   {
>>   	crtc_state->vrr.enable = true;
>> +	crtc_state->vrr.mode = INTEL_VRRTG_MODE_VRR;
>>   	crtc_state->mode_flags |= I915_MODE_FLAG_VRR;
>>   }
>>   
>> @@ -506,12 +508,15 @@ void intel_vrr_get_config(struct intel_crtc_state *crtc_state)
>>   
>>   	if (HAS_CMRR(display) && trans_vrr_ctl & VRR_CTL_CMRR_ENABLE) {
>>   		crtc_state->cmrr.enable = true;
>> +		crtc_state->vrr.mode = INTEL_VRRTG_MODE_CMRR;
>>   		crtc_state->cmrr.cmrr_n =
>>   			intel_de_read64_2x32(display, TRANS_CMRR_N_LO(display, cpu_transcoder),
>>   					     TRANS_CMRR_N_HI(display, cpu_transcoder));
>>   		crtc_state->cmrr.cmrr_m =
>>   			intel_de_read64_2x32(display, TRANS_CMRR_M_LO(display, cpu_transcoder),
>>   					     TRANS_CMRR_M_HI(display, cpu_transcoder));
>> +	} else if (trans_vrr_ctl & VRR_CTL_VRR_ENABLE) {
>> +		crtc_state->vrr.mode = INTEL_VRRTG_MODE_VRR;
>>   	}
>>   
>>   	if (DISPLAY_VER(display) >= 13)
>> -- 
>> 2.45.2