Re: [PATCH 04/19] drm/i915/dp: Add helper to compute num pipes joined

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

On Wed, Sep 11, 2024 at 06:43:34PM +0530, Ankit Nautiyal wrote:
> Add a helper to compute the number of pipes to be joined.
> The num of pipes joined will depend on whether the joiner is required or
> is forced through the debugfs.
> 
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c     | 46 +++++++++++++++------
>  drivers/gpu/drm/i915/display/intel_dp.h     |  6 +--
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 14 ++++---
>  3 files changed, 46 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 5c30d4488141..333624fcebd7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1261,17 +1261,33 @@ 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;
> +}
>  
> -	return clock > i915->display.cdclk.max_dotclk_freq || hdisplay > 5120 ||
> -	       connector->force_joined_pipes == 2;
> +int intel_dp_compute_joiner_pipes(struct intel_dp *intel_dp,
> +				  struct intel_connector *connector,
> +				  int hdisplay, int clock)

s/joiner_pipes/num_pipes/ would seem a more appropriate name
if we want to make i return 1 for the non-joiner cases...

> +{
> +	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_has_joiner(intel_dp) &&
> +		    intel_dp_needs_bigjoiner(intel_dp, connector, hdisplay, clock))
> +			return 2;
> +	}
> +
> +	return 0;

... which I thought we did, but here we are returning 0?

>  }
>  
>  bool intel_dp_has_dsc(const struct intel_connector *connector)
> @@ -1309,6 +1325,7 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>  	u8 dsc_slice_count = 0;
>  	enum drm_mode_status status;
>  	bool dsc = false, joiner = false;
> +	int num_joined_pipes;
>  
>  	status = intel_cpu_transcoder_mode_valid(dev_priv, mode);
>  	if (status != MODE_OK)
> @@ -1329,11 +1346,14 @@ 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)) {
> +	num_joined_pipes = intel_dp_compute_joiner_pipes(intel_dp, connector,
> +							 mode->hdisplay, target_clock);
> +
> +	if (num_joined_pipes == 2) {
>  		joiner = true;
>  		max_dotclk *= 2;
>  	}

This could become nan unconditional
	max_dotclock *= num_pipes;
if we did the return 1 for non-joiner cases.

> +
>  	if (target_clock > max_dotclk)
>  		return MODE_CLOCK_HIGH;
>  
> @@ -2523,15 +2543,17 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  	struct link_config_limits limits;
>  	bool dsc_needed, joiner_needs_dsc;
> +	int num_joined_pipes;
>  	int ret = 0;
>  
>  	if (pipe_config->fec_enable &&
>  	    !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))
> +	num_joined_pipes = intel_dp_compute_joiner_pipes(intel_dp, connector,
> +							 adjusted_mode->crtc_hdisplay,
> +							 adjusted_mode->crtc_clock);
> +	if (num_joined_pipes == 2)
>  		pipe_config->joiner_pipes = GENMASK(crtc->pipe + 1, crtc->pipe);

This could be a bit more generic with something like:
	if (num_pipes > 1)
		joiner_pipes = GENNMASK(pipe + num_pipes - 1, pipe);

But maybe you're doing some that stuff later. I'll keep reading...

>  
>  	joiner_needs_dsc = intel_dp_joiner_needs_dsc(i915, pipe_config->joiner_pipes);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> index 1b9aaddd8c35..bc9a82d82df2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -153,9 +153,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,
>  				bool bigjoiner);
> -bool intel_dp_need_joiner(struct intel_dp *intel_dp,
> -			  struct intel_connector *connector,
> -			  int hdisplay, int clock);
> +int intel_dp_compute_joiner_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 15541932b809..383b3e38df52 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -568,6 +568,7 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  		&pipe_config->hw.adjusted_mode;
>  	struct link_config_limits limits;
>  	bool dsc_needed, joiner_needs_dsc;
> +	int num_joined_pipes;
>  	int ret = 0;
>  
>  	if (pipe_config->fec_enable &&
> @@ -577,9 +578,10 @@ 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))
> +	num_joined_pipes = intel_dp_compute_joiner_pipes(intel_dp, connector,
> +							 adjusted_mode->crtc_hdisplay,
> +							 adjusted_mode->crtc_clock);
> +	if (num_joined_pipes == 2)
>  		pipe_config->joiner_pipes = GENMASK(crtc->pipe + 1, crtc->pipe);
>  
>  	pipe_config->sink_format = INTEL_OUTPUT_FORMAT_RGB;
> @@ -1426,6 +1428,7 @@ intel_dp_mst_mode_valid_ctx(struct drm_connector *connector,
>  	u16 dsc_max_compressed_bpp = 0;
>  	u8 dsc_slice_count = 0;
>  	int target_clock = mode->clock;
> +	int num_joined_pipes;
>  
>  	if (drm_connector_is_unregistered(connector)) {
>  		*status = MODE_ERROR;
> @@ -1465,8 +1468,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)) {
> +	num_joined_pipes = intel_dp_compute_joiner_pipes(intel_dp, intel_connector,
> +							 mode->hdisplay, target_clock);
> +	if (num_joined_pipes == 2) {
>  		joiner = true;
>  		max_dotclk *= 2;
>  	}
> -- 
> 2.45.2

-- 
Ville Syrjälä
Intel