Re: [PATCH 05/15] drm/i915/dp: Add helper to compute num pipes required

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

On Wed, Sep 18, 2024 at 08:13:33PM +0530, Ankit Nautiyal wrote:
> Add a helper to compute the number of pipes required.
> This will depend on whether the joiner is required or is forced through
> the debugfs. If no joiner is required the helper returns 1.
> 
> v2:
> -Return 1 if no joiner is required. (Ville)
> -Change the suffix from joined_pipes to num_pipes. (Ville)
> -Use number of pipes while calculating joined_pipe masks and
> max_dotclk. (Ville)
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c     | 51 +++++++++++++--------
>  drivers/gpu/drm/i915/display/intel_dp.h     |  6 +--
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 23 ++++------
>  3 files changed, 44 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 2e35a81fa6d1..96ad048b68cf 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1270,17 +1270,35 @@ intel_dp_mode_valid_downstream(struct intel_connector *connector,
>  	return MODE_OK;
>  }
>  
> -bool intel_dp_need_joiner(struct intel_dp *intel_dp,
> -			  struct intel_connector *connector,
> -			  int hdisplay, int clock)
> +static
> +bool intel_dp_needs_bigjoiner(struct intel_dp *intel_dp,
> +			      struct intel_connector *connector,
> +			      int hdisplay, int clock)
>  {
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>  
>  	if (!intel_dp_has_joiner(intel_dp))
>  		return false;
>  
> -	return clock > i915->display.cdclk.max_dotclk_freq || hdisplay > 5120 ||
> -	       connector->force_joined_pipes == 2;
> +	return clock > i915->display.cdclk.max_dotclk_freq || hdisplay > 5120;
> +}
> +
> +int intel_dp_compute_num_pipes(struct intel_dp *intel_dp,
> +			       struct intel_connector *connector,
> +			       int hdisplay, int clock)

intel_dp_compute_num_joined_pipes() to be a bit more specfic?
Or even just intel_dp_num_joined_pipes()?

> +{
> +	switch (connector->force_joined_pipes) {
> +	case 2:
> +		return connector->force_joined_pipes;
> +	default:
> +		MISSING_CASE(connector->force_joined_pipes);
> +		fallthrough;
> +	case 0:
> +		if (intel_dp_needs_bigjoiner(intel_dp, connector, hdisplay, clock))
> +			return 2;
> +	}
> +
> +	return 1;

I think that could be simplified to just:
{
	if (force_joined_pipes)
		return force_joined_pipes;
	
	if (need_bigjoiner())
		return 2;
	
	return 1;
}

Apart from that this looks good.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  }
>  
>  bool intel_dp_has_dsc(const struct intel_connector *connector)
> @@ -1317,7 +1335,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>  	u16 dsc_max_compressed_bpp = 0;
>  	u8 dsc_slice_count = 0;
>  	enum drm_mode_status status;
> -	bool dsc = false, joiner = false;
> +	bool dsc = false;
>  	int num_joined_pipes;
>  
>  	status = intel_cpu_transcoder_mode_valid(dev_priv, mode);
> @@ -1339,13 +1357,9 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>  		target_clock = fixed_mode->clock;
>  	}
>  
> -	if (intel_dp_need_joiner(intel_dp, connector,
> -				 mode->hdisplay, target_clock)) {
> -		joiner = true;
> -		max_dotclk *= 2;
> -	}
> -
> -	num_joined_pipes = joiner ? 2 : 1;
> +	num_joined_pipes = intel_dp_compute_num_pipes(intel_dp, connector,
> +						      mode->hdisplay, target_clock);
> +	max_dotclk *= num_joined_pipes;
>  
>  	if (target_clock > max_dotclk)
>  		return MODE_CLOCK_HIGH;
> @@ -2552,12 +2566,11 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  	    !intel_dp_supports_fec(intel_dp, connector, pipe_config))
>  		return -EINVAL;
>  
> -	if (intel_dp_need_joiner(intel_dp, connector,
> -				 adjusted_mode->crtc_hdisplay,
> -				 adjusted_mode->crtc_clock))
> -		pipe_config->joiner_pipes = GENMASK(crtc->pipe + 1, crtc->pipe);
> -
> -	num_joined_pipes = intel_crtc_num_joined_pipes(pipe_config);
> +	num_joined_pipes = intel_dp_compute_num_pipes(intel_dp, connector,
> +						      adjusted_mode->crtc_hdisplay,
> +						      adjusted_mode->crtc_clock);
> +	if (num_joined_pipes > 1)
> +		pipe_config->joiner_pipes = GENMASK(crtc->pipe + num_joined_pipes - 1, crtc->pipe);
>  
>  	joiner_needs_dsc = intel_dp_joiner_needs_dsc(i915, num_joined_pipes);
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> index a0a31fb64716..d72ca99e3a1c 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -154,9 +154,9 @@ int intel_dp_dsc_sink_max_compressed_bpp(const struct intel_connector *connector
>  u8 intel_dp_dsc_get_slice_count(const struct intel_connector *connector,
>  				int mode_clock, int mode_hdisplay,
>  				int num_joined_pipes);
> -bool intel_dp_need_joiner(struct intel_dp *intel_dp,
> -			  struct intel_connector *connector,
> -			  int hdisplay, int clock);
> +int intel_dp_compute_num_pipes(struct intel_dp *intel_dp,
> +			       struct intel_connector *connector,
> +			       int hdisplay, int clock);
>  
>  static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
>  {
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 7debefd4a0d6..df380f6ee76c 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -580,12 +580,11 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLSCAN)
>  		return -EINVAL;
>  
> -	if (intel_dp_need_joiner(intel_dp, connector,
> -				 adjusted_mode->crtc_hdisplay,
> -				 adjusted_mode->crtc_clock))
> -		pipe_config->joiner_pipes = GENMASK(crtc->pipe + 1, crtc->pipe);
> -
> -	num_joined_pipes = intel_crtc_num_joined_pipes(pipe_config);
> +	num_joined_pipes = intel_dp_compute_num_pipes(intel_dp, connector,
> +						      adjusted_mode->crtc_hdisplay,
> +						      adjusted_mode->crtc_clock);
> +	if (num_joined_pipes > 1)
> +		pipe_config->joiner_pipes = GENMASK(crtc->pipe + num_joined_pipes - 1, crtc->pipe);
>  
>  	pipe_config->sink_format = INTEL_OUTPUT_FORMAT_RGB;
>  	pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
> @@ -1427,7 +1426,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>  	int max_dotclk = to_i915(connector->dev)->display.cdclk.max_dotclk_freq;
>  	int max_rate, mode_rate, max_lanes, max_link_clock;
>  	int ret;
> -	bool dsc = false, joiner = false;
> +	bool dsc = false;
>  	u16 dsc_max_compressed_bpp = 0;
>  	u8 dsc_slice_count = 0;
>  	int target_clock = mode->clock;
> @@ -1471,13 +1470,9 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>  	 *   corresponding link capabilities of the sink) in case the
>  	 *   stream is uncompressed for it by the last branch device.
>  	 */
> -	if (intel_dp_need_joiner(intel_dp, intel_connector,
> -				 mode->hdisplay, target_clock)) {
> -		joiner = true;
> -		max_dotclk *= 2;
> -	}
> -
> -	num_joined_pipes = joiner ? 2 : 1;
> +	num_joined_pipes = intel_dp_compute_num_pipes(intel_dp, intel_connector,
> +						      mode->hdisplay, target_clock);
> +	max_dotclk *= num_joined_pipes;
>  
>  	ret = drm_modeset_lock(&mgr->base.lock, ctx);
>  	if (ret)
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel