Re: [PATCH v2 01/13] drm/i915/vrr: Refactor vmin/vmax stuff

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

On 4/23/2025 12:19 PM, Nautiyal, Ankit K wrote:
>
> On 4/21/2025 9:18 PM, Mitul Golani wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Refactor vmin/vmax functions for better computation.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_vrr.c | 41 +++++++++++-------------
>>   1 file changed, 19 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c 
>> b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index c6565baf815a..adfd231eb578 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -146,37 +146,41 @@ static int intel_vrr_vblank_exit_length(const 
>> struct intel_crtc_state *crtc_stat
>>           return crtc_state->vrr.pipeline_full + 
>> crtc_state->framestart_delay + 1;
>>   }
>>   -int intel_vrr_vmin_vtotal(const struct intel_crtc_state *crtc_state)
>> +static int intel_vrr_vtotal(const struct intel_crtc_state 
>> *crtc_state, int vmin_vmax)
>>   {
>>       struct intel_display *display = to_intel_display(crtc_state);
>>   -    /* Min vblank actually determined by flipline */
>>       if (DISPLAY_VER(display) >= 13)
>> -        return intel_vrr_vmin_flipline(crtc_state);
>> +        return vmin_vmax;
>>       else
>> -        return intel_vrr_vmin_flipline(crtc_state) +
>> -            intel_vrr_real_vblank_delay(crtc_state);
>> +        return vmin_vmax + intel_vrr_real_vblank_delay(crtc_state);
>>   }
>>   -int intel_vrr_vmax_vtotal(const struct intel_crtc_state *crtc_state)
>> +static int intel_vrr_vblank_start(const struct intel_crtc_state 
>> *crtc_state,
>> +                  int vmin_vmax)
>>   {
>> -    struct intel_display *display = to_intel_display(crtc_state);
>> +    return intel_vrr_vtotal(crtc_state, vmin_vmax) -
>> +            intel_vrr_vblank_exit_length(crtc_state);
>> +}
>>   -    if (DISPLAY_VER(display) >= 13)
>> -        return crtc_state->vrr.vmax;
>> -    else
>> -        return crtc_state->vrr.vmax +
>> -            intel_vrr_real_vblank_delay(crtc_state);
>> +int intel_vrr_vmin_vtotal(const struct intel_crtc_state *crtc_state)
>> +{
>> +    return intel_vrr_vtotal(crtc_state, 
>> intel_vrr_vmin_flipline(crtc_state));
>> +}
>> +
>> +int intel_vrr_vmax_vtotal(const struct intel_crtc_state *crtc_state)
>> +{
>> +    return intel_vrr_vtotal(crtc_state, crtc_state->vrr.vmax);
>>   }
>>     int intel_vrr_vmin_vblank_start(const struct intel_crtc_state 
>> *crtc_state)
>>   {
>> -    return intel_vrr_vmin_vtotal(crtc_state) - 
>> intel_vrr_vblank_exit_length(crtc_state);
>> +    return intel_vrr_vblank_start(crtc_state, 
>> intel_vrr_vmin_flipline(crtc_state));
>>   }
>>     int intel_vrr_vmax_vblank_start(const struct intel_crtc_state 
>> *crtc_state)
>>   {
>> -    return intel_vrr_vmax_vtotal(crtc_state) - 
>> intel_vrr_vblank_exit_length(crtc_state);
>> +    return intel_vrr_vblank_start(crtc_state, crtc_state->vrr.vmax);
>>   }
>>     static bool
>> @@ -257,14 +261,7 @@ void intel_vrr_compute_vrr_timings(struct 
>> intel_crtc_state *crtc_state)
>>   static
>>   int intel_vrr_fixed_rr_vtotal(const struct intel_crtc_state 
>> *crtc_state)
>>   {
>> -    struct intel_display *display = to_intel_display(crtc_state);
>> -    int crtc_vtotal = crtc_state->hw.adjusted_mode.crtc_vtotal;
>> -
>> -    if (DISPLAY_VER(display) >= 13)
>> -        return crtc_vtotal;
>> -    else
>> -        return crtc_vtotal -
>> -            intel_vrr_real_vblank_delay(crtc_state);
>> +    return intel_vrr_vtotal(crtc_state, 
>> crtc_state->hw.adjusted_mode.crtc_vtotal);
>
> This seems to be off for Display < 13.
>
> If I see it correctly, instead of subtracting we are now adding 
> intel_vrr_real_vblank_delay(crtc_state);


I think the vblank delay should be added for platforms < 13, and not 
subtracted.

Perhaps need a separate fix for this.

Regards,

Ankit

>
>
> Can this be rechecked? or I am missing something?
>
> Rest of the changes seem fine.
>
>
> Regards,
>
> Ankit
>
>
>
>>   }
>>     static