Re: [PATCH 2/8] drm/i915/vrr: s/intel_vrr_compute_config_late/intel_vrr_compute_guardband

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

On 10/10/2025 8:23 PM, Ville Syrjälä wrote:
> On Thu, Oct 09, 2025 at 02:30:56PM +0530, Ankit Nautiyal wrote:
>> The helper intel_vrr_compute_config_late() practically just computes the
>> guardband. Rename intel_vrr_compute_config_late() to
>> intel_vrr_compute_guardband().
>>
>> Since we are going to compute the guardband and then move the
>> vblank_start for optmizing guardband move it to
>> intel_crtc_compute_config() which handles such changes.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_display.c | 4 ++--
>>   drivers/gpu/drm/i915/display/intel_vrr.c     | 2 +-
>>   drivers/gpu/drm/i915/display/intel_vrr.h     | 2 +-
>>   3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>> index b57efd870774..cd499e58bed3 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>> @@ -2414,6 +2414,8 @@ static int intel_crtc_compute_config(struct intel_atomic_state *state,
>>   	if (ret)
>>   		return ret;
>>   
>> +	intel_vrr_compute_guardband(crtc_state);
>> +
>>   	ret = intel_dpll_crtc_compute_clock(state, crtc);
> Hmm. The intel_dpll_crtc_compute_clock() probably needs to move to the
> very start of the function, so that we'll have an accurate clock for the
> eventual guardband calculations. In fact my plan has been to move it
> into .compute_config() entirely, but I haven't had time to revisit
> this topic in a while :/
>
> For easier bisectability I'd do that move first as a separate patch.

Ohh I missed that, will move intel_dpll_crtc_compute_clock() in the 
beginning of the function.


>
>>   	if (ret)
>>   		return ret;
> The other thing we have here is intel_crtc_compute_pipe_mode(). I have
> a feeling I didn't consider the joiner aspect at all with the prefill
> helpers. We might need the pipe_mode for the guardband calculations.
> I'll have to have a look at what I did there and think a bit more about
> how the joiner affects that stuff.

Hmm in that case I guess will move intel_vrr_compute_guardband() at the 
last or perhaps atleast after intel_crtc_compute_pipe_mode().


>
>
> And the other thing I haven't considered at all is MSO. Right now
> adjusted_mode will contain the per-segment timings with MSO which,
> now that I think about it again, migth be a bad idea (my idea IIRC).
> Eg. adjusted_mode based linetime calculations will be skewed by the
> overlap included in the segement timings.
>
> We may have to rethink the MSO apporoach to keep the full timings in
> adjusted_mode and either introduce yet another mode for the per-segment
> timings, or perhaps just do the full<->segment conversions as needed
> (set_transcoder_timings()+its readout, compute_m_n(), maybe some other
> places as well?).

I still look into the series for pre-fill, but yes now I can see how 
joiner and MSO need to be accounted for the prefill computation.

Regards,

Ankit


>
>> @@ -4722,8 +4724,6 @@ intel_modeset_pipe_config_late(struct intel_atomic_state *state,
>>   	struct drm_connector *connector;
>>   	int i;
>>   
>> -	intel_vrr_compute_config_late(crtc_state);
>> -
>>   	for_each_new_connector_in_state(&state->base, connector,
>>   					conn_state, i) {
>>   		struct intel_encoder *encoder =
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index 4bc14b5e685f..8d71d7dc9d12 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -433,7 +433,7 @@ intel_vrr_max_guardband(struct intel_crtc_state *crtc_state)
>>   		   intel_vrr_max_vblank_guardband(crtc_state));
>>   }
>>   
>> -void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state)
>> +void intel_vrr_compute_guardband(struct intel_crtc_state *crtc_state)
>>   {
>>   	struct intel_display *display = to_intel_display(crtc_state);
>>   	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.h b/drivers/gpu/drm/i915/display/intel_vrr.h
>> index 7317f8730089..bc9044621635 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.h
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.h
>> @@ -21,7 +21,7 @@ bool intel_vrr_possible(const struct intel_crtc_state *crtc_state);
>>   void intel_vrr_check_modeset(struct intel_atomic_state *state);
>>   void intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>>   			      struct drm_connector_state *conn_state);
>> -void intel_vrr_compute_config_late(struct intel_crtc_state *crtc_state);
>> +void intel_vrr_compute_guardband(struct intel_crtc_state *crtc_state);
>>   void intel_vrr_set_transcoder_timings(const struct intel_crtc_state *crtc_state);
>>   void intel_vrr_enable(const struct intel_crtc_state *crtc_state);
>>   void intel_vrr_send_push(struct intel_dsb *dsb,
>> -- 
>> 2.45.2