Re: [PATCH 02/20] drm/i915:vrr: Separate out functions to compute vmin and vmax

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

On 2/26/2025 6:29 PM, Ville Syrjälä wrote:
> On Mon, Feb 24, 2025 at 11:46:59AM +0530, Ankit Nautiyal wrote:
>> Make helpers to compute vmin and vmax.
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/display/intel_vrr.c | 39 +++++++++++++++++++-----
>>   1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
>> index 106bfaf6649b..a435b8d5b631 100644
>> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
>> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
>> @@ -222,6 +222,35 @@ cmrr_get_vtotal(struct intel_crtc_state *crtc_state, bool video_mode_required)
>>   	return vtotal;
>>   }
>>   
>> +static
>> +int intel_vrr_compute_vmin(struct intel_connector *connector,
>> +			   struct drm_display_mode *adjusted_mode)
> Make the adjusted mode const
>
>> +{
>> +	int vmin;
>> +	const struct drm_display_info *info = &connector->base.display_info;
> I generally prefer to order these approximately by the line length
> in descending order. So swapping these would look better to me.
> Same in the vmax counterpart.
>
>> +
>> +	vmin = DIV_ROUND_UP(adjusted_mode->crtc_clock * 1000,
>> +			    adjusted_mode->crtc_htotal * info->monitor_range.max_vfreq);
>> +	vmin = max_t(int, vmin, adjusted_mode->crtc_vtotal);
>> +
>> +	return vmin;
>> +}
>> +
>> +static
>> +int intel_vrr_compute_vmax(struct intel_connector *connector,
>> +			   struct drm_display_mode *adjusted_mode)
> adjusted_mode should be const here as well
>
>> +{
>> +	int vmax;
>> +	const struct drm_display_info *info = &connector->base.display_info;
>> +
>> +	vmax = adjusted_mode->crtc_clock * 1000 /
>> +		(adjusted_mode->crtc_htotal * info->monitor_range.min_vfreq);
>> +
> extra newline here but not in the vmin counterpart
>
>> +	vmax = max_t(int, vmax, adjusted_mode->crtc_vtotal);
>> +
>> +	return vmax;
>> +}
>> +
> With those sorted this is
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks for the review and comments. I agree with the suggested changes 
and will fix these in next version.


Regards,

Ankit

>
>>   void
>>   intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>>   			 struct drm_connector_state *conn_state)
>> @@ -232,7 +261,6 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>>   	struct intel_dp *intel_dp = intel_attached_dp(connector);
>>   	bool is_edp = intel_dp_is_edp(intel_dp);
>>   	struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>> -	const struct drm_display_info *info = &connector->base.display_info;
>>   	int vmin, vmax;
>>   
>>   	/*
>> @@ -253,13 +281,8 @@ intel_vrr_compute_config(struct intel_crtc_state *crtc_state,
>>   	if (HAS_LRR(display))
>>   		crtc_state->update_lrr = true;
>>   
>> -	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);
>> -
>> -	vmin = max_t(int, vmin, adjusted_mode->crtc_vtotal);
>> -	vmax = max_t(int, vmax, adjusted_mode->crtc_vtotal);
>> +	vmin = intel_vrr_compute_vmin(connector, adjusted_mode);
>> +	vmax = intel_vrr_compute_vmax(connector, adjusted_mode);
>>   
>>   	if (vmin >= vmax)
>>   		return;
>> -- 
>> 2.45.2