Re: [PATCH 14/15] drm/i915/panel: Refactor helper to get highest fixed mode

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Sep 11, 2025 at 08:15:53AM +0530, Ankit Nautiyal wrote:
> Refactor intel_panel_highest_mode() to return the fixed mode with the
> highest pixel clock, removing the fallback to the adjusted mode. This makes
> the function semantics clearer and better suited for future use cases where
> fallback is not desirable.
> 
> Update the caller in intel_dp_mode_clock() to handle the NULL case
> explicitly by falling back to the adjusted mode's crtc_clock. This also
> addresses the existing FIXME comment about ambiguity between clock and
> crtc_clock, by using mode->clock for fixed modes and mode->crtc_clock for
> adjusted modes.
> 
> v2: Avoid introducing a new function and refactor existing one instead.
> (Jani).
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Reviewed-by: Mitul Golani <mitulkumar.ajitkumar.golani@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c    | 14 +++++++++-----
>  drivers/gpu/drm/i915/display/intel_panel.c | 11 +++++------
>  drivers/gpu/drm/i915/display/intel_panel.h |  3 +--
>  3 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 83c46e4680b3..f74ac98062d4 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1760,11 +1760,15 @@ static int intel_dp_mode_clock(const struct intel_crtc_state *crtc_state,
>  	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
>  
> -	/* FIXME a bit of a mess wrt clock vs. crtc_clock */
> -	if (has_seamless_m_n(connector))
> -		return intel_panel_highest_mode(connector, adjusted_mode)->clock;
> -	else
> -		return adjusted_mode->crtc_clock;
> +	if (has_seamless_m_n(connector)) {
> +		const struct drm_display_mode *highest_mode;
> +
> +		highest_mode = intel_panel_highest_mode(connector);
> +		if (highest_mode)
> +			return highest_mode->clock;
> +	}
> +
> +	return adjusted_mode->crtc_clock;
>  }
>  
>  /* Optimize link config in order: max bpp, min clock, min lanes */
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.c b/drivers/gpu/drm/i915/display/intel_panel.c
> index 2a20aaaaac39..ac0f04073ecb 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.c
> +++ b/drivers/gpu/drm/i915/display/intel_panel.c
> @@ -144,18 +144,17 @@ intel_panel_downclock_mode(struct intel_connector *connector,
>  }
>  
>  const struct drm_display_mode *
> -intel_panel_highest_mode(struct intel_connector *connector,
> -			 const struct drm_display_mode *adjusted_mode)
> +intel_panel_highest_mode(struct intel_connector *connector)
>  {
> -	const struct drm_display_mode *fixed_mode, *best_mode = adjusted_mode;
> +	const struct drm_display_mode *fixed_mode, *highest_mode = NULL;
>  
>  	/* pick the fixed_mode that has the highest clock */
>  	list_for_each_entry(fixed_mode, &connector->panel.fixed_modes, head) {
> -		if (fixed_mode->clock > best_mode->clock)
> -			best_mode = fixed_mode;
> +		if (!highest_mode || fixed_mode->clock > highest_mode->clock)
> +			highest_mode = fixed_mode;

This looks broken now as it will always return some kind of mode
from the list, but whether or not it's better than the adjusted_mode
is no logner guaranteed.

>  	}
>  
> -	return best_mode;
> +	return highest_mode;
>  }
>  
>  int intel_panel_get_modes(struct intel_connector *connector)
> diff --git a/drivers/gpu/drm/i915/display/intel_panel.h b/drivers/gpu/drm/i915/display/intel_panel.h
> index 56a6412cf0fb..8a17600e46a3 100644
> --- a/drivers/gpu/drm/i915/display/intel_panel.h
> +++ b/drivers/gpu/drm/i915/display/intel_panel.h
> @@ -37,8 +37,7 @@ const struct drm_display_mode *
>  intel_panel_downclock_mode(struct intel_connector *connector,
>  			   const struct drm_display_mode *adjusted_mode);
>  const struct drm_display_mode *
> -intel_panel_highest_mode(struct intel_connector *connector,
> -			 const struct drm_display_mode *adjusted_mode);
> +intel_panel_highest_mode(struct intel_connector *connector);
>  int intel_panel_get_modes(struct intel_connector *connector);
>  enum drrs_type intel_panel_drrs_type(struct intel_connector *connector);
>  enum drm_mode_status
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel