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

[Nautiyal, Ankit K]

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.nauti...@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.syrj...@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