[12/13] drm/i915/dp: Get optimal link config to have best compressed bpp

Commit Message

Nautiyal, Ankit K May 12, 2023, 6:24 a.m. UTC

Currently, we take the max lane, rate and pipe bpp, to get the maximum
compressed bpp possible. We then set the output bpp to this value.
This patch provides support to have max bpp, min rate and min lanes,
that can support the min compressed bpp.

v2:
-Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
-Fix the checks for limits->max/min_bpp while iterating over list of
 valid DSC bpcs. (Stan)

v3:
-Refactor the code to have pipe bpp/compressed bpp computation and slice
count calculation separately for different cases.

v4:
-Separate the pipe_bpp calculation for eDP and DP.

Signed-off-by: Ankit Nautiyal <[e-mail]>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
 1 file changed, 245 insertions(+), 60 deletions(-)

Comments

Stanislav Lisovskiy May 16, 2023, 10:43 a.m. UTC | #1

On Fri, May 12, 2023 at 11:54:16AM +0530, Ankit Nautiyal wrote:
> Currently, we take the max lane, rate and pipe bpp, to get the maximum
> compressed bpp possible. We then set the output bpp to this value.
> This patch provides support to have max bpp, min rate and min lanes,
> that can support the min compressed bpp.
> 
> v2:
> -Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
> -Fix the checks for limits->max/min_bpp while iterating over list of
>  valid DSC bpcs. (Stan)
> 
> v3:
> -Refactor the code to have pipe bpp/compressed bpp computation and slice
> count calculation separately for different cases.
> 
> v4:
> -Separate the pipe_bpp calculation for eDP and DP.
> 
> Signed-off-by: Ankit Nautiyal <[e-mail]>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
>  1 file changed, 245 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 39e2bf3d738d..578320220c9a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1642,6 +1642,209 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
>  	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
>  }
>  
> +static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
> +				 const struct drm_display_mode *adjusted_mode)
> +{
> +	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
> +	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
> +
> +	return mode_rate <= link_avail;
> +}
> +
> +static int dsc_compute_link_config(struct intel_dp *intel_dp,
> +				   struct intel_crtc_state *pipe_config,
> +				   struct link_config_limits *limits,
> +				   int pipe_bpp,
> +				   u16 compressed_bpp,
> +				   int timeslots)
> +{
> +	const struct drm_display_mode *adjusted_mode =
> +		&pipe_config->hw.adjusted_mode;
> +	int link_rate, lane_count;
> +	int dsc_max_bpp;
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +	int i;
> +
> +	for (i = 0; i < intel_dp->num_common_rates; i++) {
> +		link_rate = intel_dp_common_rate(intel_dp, i);
> +		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
> +			continue;
> +
> +		for (lane_count = limits->min_lane_count;
> +		     lane_count <= limits->max_lane_count;
> +		     lane_count <<= 1) {
> +			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> +									  link_rate,
> +									  lane_count,
> +									  adjusted_mode->crtc_clock,
> +									  adjusted_mode->crtc_hdisplay,
> +									  pipe_config->bigjoiner_pipes,
> +									  pipe_config->output_format,
> +									  pipe_bpp, timeslots);
> +			/*
> +			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> +			 * supported PPS value can be 63.9375 and with the further
> +			 * mention that bpp should be programmed double the target bpp
> +			 * restricting our target bpp to be 31.9375 at max
> +			 */
> +			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> +				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
> +
> +			if (compressed_bpp > dsc_max_bpp)
> +				continue;
> +
> +			if (!is_dsc_bw_sufficient(link_rate, lane_count,
> +						  compressed_bpp, adjusted_mode))
> +				continue;
> +
> +			pipe_config->lane_count = lane_count;
> +			pipe_config->port_clock = link_rate;
> +
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static
> +u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
> +					    struct intel_crtc_state *pipe_config,
> +					    int bpc)
> +{
> +	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
> +
> +	if (max_bppx16)
> +		return max_bppx16;
> +	/*
> +	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
> +	 * values as given in spec Table 2-157 DP v2.0
> +	 */
> +	switch (pipe_config->output_format) {
> +	case INTEL_OUTPUT_FORMAT_RGB:
> +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> +		return (3 * bpc) << 4;
> +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> +		return (3 * (bpc / 2)) << 4;
> +	default:
> +		MISSING_CASE(pipe_config->output_format);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
> +{
> +	switch (pipe_config->output_format) {
> +	case INTEL_OUTPUT_FORMAT_RGB:
> +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> +		return 8 << 4;
> +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> +		return 6 << 4;
> +	default:
> +		MISSING_CASE(pipe_config->output_format);
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
> +				      struct intel_crtc_state *pipe_config,
> +				      struct link_config_limits *limits,
> +				      int pipe_bpp,
> +				      int timeslots)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +	u16 compressed_bpp;
> +	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
> +	int ret;
> +
> +	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
> +	if (DISPLAY_VER(dev_priv) <= 12)
> +		dsc_src_max_bpp = 23;
> +	else
> +		dsc_src_max_bpp = 27;

I would may be added some comment about what are those "23/27" numbers or
may be even created some self-explanatory #define constants for those.
Otherwise in a couple of years, even we ourselves might not be able to recall
immediately where those numbers are taken from.

Otherwise looks good to me,

Reviewed-by: Stanislav Lisovskiy <[e-mail]>

> +	dsc_sink_max_bpp =
> +		intel_dp_dsc_max_sink_compressed_bppx16(intel_dp,
> +							pipe_config, pipe_bpp / 3) >> 4;
> +
> +	dsc_max_bpp = dsc_sink_max_bpp ? min(dsc_sink_max_bpp, dsc_src_max_bpp) : dsc_src_max_bpp;
> +
> +	/* Compressed BPP should be less than the Input DSC bpp */
> +	dsc_max_bpp = min(dsc_max_bpp, pipe_bpp - 1);
> +
> +	for (compressed_bpp = dsc_max_bpp;
> +	     compressed_bpp >= dsc_min_bpp;
> +	     compressed_bpp--) {
> +		ret = dsc_compute_link_config(intel_dp,
> +					      pipe_config,
> +					      limits,
> +					      pipe_bpp,
> +					      compressed_bpp,
> +					      timeslots);
> +		if (ret == 0) {
> +			pipe_config->dsc.compressed_bpp = compressed_bpp;
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int intel_dp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
> +					 struct intel_crtc_state *pipe_config,
> +					 struct drm_connector_state *conn_state,
> +					 struct link_config_limits *limits,
> +					 int timeslots)
> +{
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +	u8 dsc_bpc[3] = {0};
> +	u8 dsc_max_bpc, dsc_max_bpp;
> +	u8 dsc_min_bpc, dsc_min_bpp;
> +	u8 max_req_bpc = conn_state->max_requested_bpc;
> +	int i, bpp, ret;
> +	int num_bpc = drm_dp_dsc_sink_supported_input_bpcs(intel_dp->dsc_dpcd,
> +							   dsc_bpc);
> +
> +	/* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */
> +	if (DISPLAY_VER(i915) >= 12)
> +		dsc_max_bpc = min_t(u8, 12, max_req_bpc);
> +	else
> +		dsc_max_bpc = min_t(u8, 10, max_req_bpc);
> +
> +	dsc_max_bpp = min(dsc_max_bpc * 3, limits->max_bpp);
> +
> +	/* Min DSC Input BPC for ICL+ is 8 */
> +	dsc_min_bpc = 8;
> +	dsc_min_bpp = max(dsc_min_bpc * 3, limits->min_bpp);
> +
> +	/*
> +	 * Get the maximum DSC bpc that will be supported by any valid
> +	 * link configuration and compressed bpp.
> +	 */
> +	for (i = 0; i < num_bpc; i++) {
> +		bpp = dsc_bpc[i] * 3;
> +
> +		if (bpp < dsc_min_bpp)
> +			break;
> +
> +		if (bpp > dsc_max_bpp)
> +			continue;
> +
> +		ret = dsc_compute_compressed_bpp(intel_dp, pipe_config,
> +						 limits, bpp, timeslots);
> +		if (ret == 0) {
> +			pipe_config->pipe_bpp = bpp;
> +
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static
>  bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
>  {
> @@ -1649,6 +1852,31 @@ bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
>  	return (pipe_bpp < 8 * 3);
>  }
>  
> +static int intel_edp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
> +					  struct intel_crtc_state *pipe_config,
> +					  struct drm_connector_state *conn_state,
> +					  struct link_config_limits *limits)
> +{
> +	/* For eDP use max bpp that can be supported with DSC. */
> +	int pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
> +						    conn_state->max_requested_bpc);
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +
> +	if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
> +		drm_dbg_kms(&i915->drm,
> +			    "No DSC support for less than 8bpc\n");
> +		return -EINVAL;
> +	}
> +	pipe_config->pipe_bpp = pipe_bpp;
> +	pipe_config->port_clock = limits->max_rate;
> +	pipe_config->lane_count = limits->max_lane_count;
> +	pipe_config->dsc.compressed_bpp =
> +		min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
> +		      pipe_config->pipe_bpp);
> +
> +	return 0;
> +}
> +
>  int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  				struct intel_crtc_state *pipe_config,
>  				struct drm_connector_state *conn_state,
> @@ -1671,6 +1899,12 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
>  		return -EINVAL;
>  
> +	/*
> +	 * compute pipe bpp is set to false for DP MST DSC case
> +	 * and compressed_bpp is calculated same time once
> +	 * vpci timeslots are allocated, because overall bpp
> +	 * calculation procedure is bit different for MST case.
> +	 */
>  	if (compute_pipe_bpp) {
>  		int pipe_bpp;
>  		int forced_bpp = intel_dp->force_dsc_bpc * 3;
> @@ -1683,31 +1917,25 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  				 forced_bpp && !is_dsc_pipe_bpp_sufficient(forced_bpp),
>  				 "Cannot force dsc bpc:%d, due to dsc bpc limits\n",
>  				 intel_dp->force_dsc_bpc);
> -
> -			pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
> -								conn_state->max_requested_bpc);
> -
> -			if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
> -				drm_dbg_kms(&dev_priv->drm, "No DSC support for less than 8bpc\n");
> -				return -EINVAL;
> +			if (intel_dp_is_edp(intel_dp))
> +				ret = intel_edp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
> +								     conn_state, limits);
> +			else
> +				ret = intel_dp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
> +								    conn_state, limits, timeslots);
> +			if (ret) {
> +				drm_dbg_kms(&dev_priv->drm,
> +					    "No Valid pipe bpp for given mode ret = %d\n", ret);
> +				return ret;
>  			}
>  		}
> -
> -		pipe_config->pipe_bpp = pipe_bpp;
>  	}
>  
> -	/*
> -	 * For now enable DSC for max link rate, max lane count.
> -	 * Optimize this later for the minimum possible link rate/lane count
> -	 * with DSC enabled for the requested mode.
> -	 */
>  	pipe_config->port_clock = limits->max_rate;
>  	pipe_config->lane_count = limits->max_lane_count;
>  
> +	/* Calculate Slice count */
>  	if (intel_dp_is_edp(intel_dp)) {
> -		pipe_config->dsc.compressed_bpp =
> -			min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
> -			      pipe_config->pipe_bpp);
>  		pipe_config->dsc.slice_count =
>  			drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
>  							true);
> @@ -1717,37 +1945,8 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  			return -EINVAL;
>  		}
>  	} else {
> -		u16 dsc_max_compressed_bpp = 0;
>  		u8 dsc_dp_slice_count;
>  
> -		if (compute_pipe_bpp) {
> -			dsc_max_compressed_bpp =
> -				intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> -								    pipe_config->port_clock,
> -								    pipe_config->lane_count,
> -								    adjusted_mode->crtc_clock,
> -								    adjusted_mode->crtc_hdisplay,
> -								    pipe_config->bigjoiner_pipes,
> -								    pipe_config->output_format,
> -								    pipe_config->pipe_bpp,
> -								    timeslots);
> -			/*
> -			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> -			 * supported PPS value can be 63.9375 and with the further
> -			 * mention that bpp should be programmed double the target bpp
> -			 * restricting our target bpp to be 31.9375 at max
> -			 */
> -			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> -				dsc_max_compressed_bpp = min_t(u16,
> -							       dsc_max_compressed_bpp,
> -							       31);
> -
> -			if (!dsc_max_compressed_bpp) {
> -				drm_dbg_kms(&dev_priv->drm,
> -					    "Compressed BPP not supported\n");
> -				return -EINVAL;
> -			}
> -		}
>  		dsc_dp_slice_count =
>  			intel_dp_dsc_get_slice_count(intel_dp,
>  						     adjusted_mode->crtc_clock,
> @@ -1759,20 +1958,6 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  			return -EINVAL;
>  		}
>  
> -		/*
> -		 * compute pipe bpp is set to false for DP MST DSC case
> -		 * and compressed_bpp is calculated same time once
> -		 * vpci timeslots are allocated, because overall bpp
> -		 * calculation procedure is bit different for MST case.
> -		 */
> -		if (compute_pipe_bpp) {
> -			u16 output_bpp = intel_dp_output_bpp(pipe_config->output_format,
> -							     pipe_config->pipe_bpp);
> -
> -			pipe_config->dsc.compressed_bpp = min_t(u16,
> -								dsc_max_compressed_bpp,
> -								output_bpp);
> -		}
>  		pipe_config->dsc.slice_count = dsc_dp_slice_count;
>  	}
>  	/*
> -- 
> 2.25.1
>

Ville Syrjälä May 16, 2023, 11:40 a.m. UTC | #2

On Tue, May 16, 2023 at 01:43:44PM +0300, Lisovskiy, Stanislav wrote:
> On Fri, May 12, 2023 at 11:54:16AM +0530, Ankit Nautiyal wrote:
> > Currently, we take the max lane, rate and pipe bpp, to get the maximum
> > compressed bpp possible. We then set the output bpp to this value.
> > This patch provides support to have max bpp, min rate and min lanes,
> > that can support the min compressed bpp.
> > 
> > v2:
> > -Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
> > -Fix the checks for limits->max/min_bpp while iterating over list of
> >  valid DSC bpcs. (Stan)
> > 
> > v3:
> > -Refactor the code to have pipe bpp/compressed bpp computation and slice
> > count calculation separately for different cases.
> > 
> > v4:
> > -Separate the pipe_bpp calculation for eDP and DP.
> > 
> > Signed-off-by: Ankit Nautiyal <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
> >  1 file changed, 245 insertions(+), 60 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index 39e2bf3d738d..578320220c9a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -1642,6 +1642,209 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
> >  	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
> >  }
> >  
> > +static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
> > +				 const struct drm_display_mode *adjusted_mode)
> > +{
> > +	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
> > +	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
> > +
> > +	return mode_rate <= link_avail;
> > +}
> > +
> > +static int dsc_compute_link_config(struct intel_dp *intel_dp,
> > +				   struct intel_crtc_state *pipe_config,
> > +				   struct link_config_limits *limits,
> > +				   int pipe_bpp,
> > +				   u16 compressed_bpp,
> > +				   int timeslots)
> > +{
> > +	const struct drm_display_mode *adjusted_mode =
> > +		&pipe_config->hw.adjusted_mode;
> > +	int link_rate, lane_count;
> > +	int dsc_max_bpp;
> > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +	int i;
> > +
> > +	for (i = 0; i < intel_dp->num_common_rates; i++) {
> > +		link_rate = intel_dp_common_rate(intel_dp, i);
> > +		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
> > +			continue;
> > +
> > +		for (lane_count = limits->min_lane_count;
> > +		     lane_count <= limits->max_lane_count;
> > +		     lane_count <<= 1) {
> > +			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> > +									  link_rate,
> > +									  lane_count,
> > +									  adjusted_mode->crtc_clock,
> > +									  adjusted_mode->crtc_hdisplay,
> > +									  pipe_config->bigjoiner_pipes,
> > +									  pipe_config->output_format,
> > +									  pipe_bpp, timeslots);
> > +			/*
> > +			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> > +			 * supported PPS value can be 63.9375 and with the further
> > +			 * mention that bpp should be programmed double the target bpp
> > +			 * restricting our target bpp to be 31.9375 at max
> > +			 */
> > +			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > +				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
> > +
> > +			if (compressed_bpp > dsc_max_bpp)
> > +				continue;
> > +
> > +			if (!is_dsc_bw_sufficient(link_rate, lane_count,
> > +						  compressed_bpp, adjusted_mode))
> > +				continue;
> > +
> > +			pipe_config->lane_count = lane_count;
> > +			pipe_config->port_clock = link_rate;
> > +
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static
> > +u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
> > +					    struct intel_crtc_state *pipe_config,
> > +					    int bpc)
> > +{
> > +	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
> > +
> > +	if (max_bppx16)
> > +		return max_bppx16;
> > +	/*
> > +	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
> > +	 * values as given in spec Table 2-157 DP v2.0
> > +	 */
> > +	switch (pipe_config->output_format) {
> > +	case INTEL_OUTPUT_FORMAT_RGB:
> > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > +		return (3 * bpc) << 4;
> > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > +		return (3 * (bpc / 2)) << 4;
> > +	default:
> > +		MISSING_CASE(pipe_config->output_format);
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
> > +{
> > +	switch (pipe_config->output_format) {
> > +	case INTEL_OUTPUT_FORMAT_RGB:
> > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > +		return 8 << 4;
> > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > +		return 6 << 4;
> > +	default:
> > +		MISSING_CASE(pipe_config->output_format);
> > +		break;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
> > +				      struct intel_crtc_state *pipe_config,
> > +				      struct link_config_limits *limits,
> > +				      int pipe_bpp,
> > +				      int timeslots)
> > +{
> > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +	u16 compressed_bpp;
> > +	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
> > +	int ret;
> > +
> > +	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
> > +	if (DISPLAY_VER(dev_priv) <= 12)
> > +		dsc_src_max_bpp = 23;
> > +	else
> > +		dsc_src_max_bpp = 27;
> 
> I would may be added some comment about what are those "23/27" numbers or
> may be even created some self-explanatory #define constants for those.

I dislike defines like that. They are single use so don't actually
do anything in terms of avoiding typoes and other accidental
mismatches, and people always seem put them in some random place
(eg. top of file) so then it takes extra work to find them.

The best approach IMO is to just use functions with good names.
Eg. in this case we could just have a full set of clear functions:
dsc_{sink,source}_{min,max}_bpp() or something along those line.

> Otherwise in a couple of years, even we ourselves might not be able to recall
> immediately where those numbers are taken from.
> 
> Otherwise looks good to me,
> 
> Reviewed-by: Stanislav Lisovskiy <[e-mail]>
> 
> > +	dsc_sink_max_bpp =
> > +		intel_dp_dsc_max_sink_compressed_bppx16(intel_dp,
> > +							pipe_config, pipe_bpp / 3) >> 4;
> > +
> > +	dsc_max_bpp = dsc_sink_max_bpp ? min(dsc_sink_max_bpp, dsc_src_max_bpp) : dsc_src_max_bpp;
> > +
> > +	/* Compressed BPP should be less than the Input DSC bpp */
> > +	dsc_max_bpp = min(dsc_max_bpp, pipe_bpp - 1);
> > +
> > +	for (compressed_bpp = dsc_max_bpp;
> > +	     compressed_bpp >= dsc_min_bpp;
> > +	     compressed_bpp--) {
> > +		ret = dsc_compute_link_config(intel_dp,
> > +					      pipe_config,
> > +					      limits,
> > +					      pipe_bpp,
> > +					      compressed_bpp,
> > +					      timeslots);
> > +		if (ret == 0) {
> > +			pipe_config->dsc.compressed_bpp = compressed_bpp;
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int intel_dp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
> > +					 struct intel_crtc_state *pipe_config,
> > +					 struct drm_connector_state *conn_state,
> > +					 struct link_config_limits *limits,
> > +					 int timeslots)
> > +{
> > +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > +	u8 dsc_bpc[3] = {0};
> > +	u8 dsc_max_bpc, dsc_max_bpp;
> > +	u8 dsc_min_bpc, dsc_min_bpp;
> > +	u8 max_req_bpc = conn_state->max_requested_bpc;
> > +	int i, bpp, ret;
> > +	int num_bpc = drm_dp_dsc_sink_supported_input_bpcs(intel_dp->dsc_dpcd,
> > +							   dsc_bpc);
> > +
> > +	/* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */
> > +	if (DISPLAY_VER(i915) >= 12)
> > +		dsc_max_bpc = min_t(u8, 12, max_req_bpc);
> > +	else
> > +		dsc_max_bpc = min_t(u8, 10, max_req_bpc);
> > +
> > +	dsc_max_bpp = min(dsc_max_bpc * 3, limits->max_bpp);
> > +
> > +	/* Min DSC Input BPC for ICL+ is 8 */
> > +	dsc_min_bpc = 8;
> > +	dsc_min_bpp = max(dsc_min_bpc * 3, limits->min_bpp);
> > +
> > +	/*
> > +	 * Get the maximum DSC bpc that will be supported by any valid
> > +	 * link configuration and compressed bpp.
> > +	 */
> > +	for (i = 0; i < num_bpc; i++) {
> > +		bpp = dsc_bpc[i] * 3;
> > +
> > +		if (bpp < dsc_min_bpp)
> > +			break;
> > +
> > +		if (bpp > dsc_max_bpp)
> > +			continue;
> > +
> > +		ret = dsc_compute_compressed_bpp(intel_dp, pipe_config,
> > +						 limits, bpp, timeslots);
> > +		if (ret == 0) {
> > +			pipe_config->pipe_bpp = bpp;
> > +
> > +			return 0;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> >  static
> >  bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
> >  {
> > @@ -1649,6 +1852,31 @@ bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
> >  	return (pipe_bpp < 8 * 3);
> >  }
> >  
> > +static int intel_edp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
> > +					  struct intel_crtc_state *pipe_config,
> > +					  struct drm_connector_state *conn_state,
> > +					  struct link_config_limits *limits)
> > +{
> > +	/* For eDP use max bpp that can be supported with DSC. */
> > +	int pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
> > +						    conn_state->max_requested_bpc);
> > +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > +
> > +	if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
> > +		drm_dbg_kms(&i915->drm,
> > +			    "No DSC support for less than 8bpc\n");
> > +		return -EINVAL;
> > +	}
> > +	pipe_config->pipe_bpp = pipe_bpp;
> > +	pipe_config->port_clock = limits->max_rate;
> > +	pipe_config->lane_count = limits->max_lane_count;
> > +	pipe_config->dsc.compressed_bpp =
> > +		min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
> > +		      pipe_config->pipe_bpp);
> > +
> > +	return 0;
> > +}
> > +
> >  int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >  				struct intel_crtc_state *pipe_config,
> >  				struct drm_connector_state *conn_state,
> > @@ -1671,6 +1899,12 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >  	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
> >  		return -EINVAL;
> >  
> > +	/*
> > +	 * compute pipe bpp is set to false for DP MST DSC case
> > +	 * and compressed_bpp is calculated same time once
> > +	 * vpci timeslots are allocated, because overall bpp
> > +	 * calculation procedure is bit different for MST case.
> > +	 */
> >  	if (compute_pipe_bpp) {
> >  		int pipe_bpp;
> >  		int forced_bpp = intel_dp->force_dsc_bpc * 3;
> > @@ -1683,31 +1917,25 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >  				 forced_bpp && !is_dsc_pipe_bpp_sufficient(forced_bpp),
> >  				 "Cannot force dsc bpc:%d, due to dsc bpc limits\n",
> >  				 intel_dp->force_dsc_bpc);
> > -
> > -			pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
> > -								conn_state->max_requested_bpc);
> > -
> > -			if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
> > -				drm_dbg_kms(&dev_priv->drm, "No DSC support for less than 8bpc\n");
> > -				return -EINVAL;
> > +			if (intel_dp_is_edp(intel_dp))
> > +				ret = intel_edp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
> > +								     conn_state, limits);
> > +			else
> > +				ret = intel_dp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
> > +								    conn_state, limits, timeslots);
> > +			if (ret) {
> > +				drm_dbg_kms(&dev_priv->drm,
> > +					    "No Valid pipe bpp for given mode ret = %d\n", ret);
> > +				return ret;
> >  			}
> >  		}
> > -
> > -		pipe_config->pipe_bpp = pipe_bpp;
> >  	}
> >  
> > -	/*
> > -	 * For now enable DSC for max link rate, max lane count.
> > -	 * Optimize this later for the minimum possible link rate/lane count
> > -	 * with DSC enabled for the requested mode.
> > -	 */
> >  	pipe_config->port_clock = limits->max_rate;
> >  	pipe_config->lane_count = limits->max_lane_count;
> >  
> > +	/* Calculate Slice count */
> >  	if (intel_dp_is_edp(intel_dp)) {
> > -		pipe_config->dsc.compressed_bpp =
> > -			min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
> > -			      pipe_config->pipe_bpp);
> >  		pipe_config->dsc.slice_count =
> >  			drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
> >  							true);
> > @@ -1717,37 +1945,8 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >  			return -EINVAL;
> >  		}
> >  	} else {
> > -		u16 dsc_max_compressed_bpp = 0;
> >  		u8 dsc_dp_slice_count;
> >  
> > -		if (compute_pipe_bpp) {
> > -			dsc_max_compressed_bpp =
> > -				intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> > -								    pipe_config->port_clock,
> > -								    pipe_config->lane_count,
> > -								    adjusted_mode->crtc_clock,
> > -								    adjusted_mode->crtc_hdisplay,
> > -								    pipe_config->bigjoiner_pipes,
> > -								    pipe_config->output_format,
> > -								    pipe_config->pipe_bpp,
> > -								    timeslots);
> > -			/*
> > -			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> > -			 * supported PPS value can be 63.9375 and with the further
> > -			 * mention that bpp should be programmed double the target bpp
> > -			 * restricting our target bpp to be 31.9375 at max
> > -			 */
> > -			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > -				dsc_max_compressed_bpp = min_t(u16,
> > -							       dsc_max_compressed_bpp,
> > -							       31);
> > -
> > -			if (!dsc_max_compressed_bpp) {
> > -				drm_dbg_kms(&dev_priv->drm,
> > -					    "Compressed BPP not supported\n");
> > -				return -EINVAL;
> > -			}
> > -		}
> >  		dsc_dp_slice_count =
> >  			intel_dp_dsc_get_slice_count(intel_dp,
> >  						     adjusted_mode->crtc_clock,
> > @@ -1759,20 +1958,6 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >  			return -EINVAL;
> >  		}
> >  
> > -		/*
> > -		 * compute pipe bpp is set to false for DP MST DSC case
> > -		 * and compressed_bpp is calculated same time once
> > -		 * vpci timeslots are allocated, because overall bpp
> > -		 * calculation procedure is bit different for MST case.
> > -		 */
> > -		if (compute_pipe_bpp) {
> > -			u16 output_bpp = intel_dp_output_bpp(pipe_config->output_format,
> > -							     pipe_config->pipe_bpp);
> > -
> > -			pipe_config->dsc.compressed_bpp = min_t(u16,
> > -								dsc_max_compressed_bpp,
> > -								output_bpp);
> > -		}
> >  		pipe_config->dsc.slice_count = dsc_dp_slice_count;
> >  	}
> >  	/*
> > -- 
> > 2.25.1
> >

Nautiyal, Ankit K May 18, 2023, 1:25 p.m. UTC | #3

Thanks Stan and Ville for the review comments.

I agree can have some documentation about the values used, instead of 
magic numbers.

Also, Ville's approach for dsc_{sink,source}_{min,max}_bpp() seems good, 
and that can be used as helpers in MST case too.

Will add the changes in the next version of the patch.


Thanks & Regards,

Ankit


On 5/16/2023 5:10 PM, Ville Syrjälä wrote:
> On Tue, May 16, 2023 at 01:43:44PM +0300, Lisovskiy, Stanislav wrote:
>> On Fri, May 12, 2023 at 11:54:16AM +0530, Ankit Nautiyal wrote:
>>> Currently, we take the max lane, rate and pipe bpp, to get the maximum
>>> compressed bpp possible. We then set the output bpp to this value.
>>> This patch provides support to have max bpp, min rate and min lanes,
>>> that can support the min compressed bpp.
>>>
>>> v2:
>>> -Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
>>> -Fix the checks for limits->max/min_bpp while iterating over list of
>>>   valid DSC bpcs. (Stan)
>>>
>>> v3:
>>> -Refactor the code to have pipe bpp/compressed bpp computation and slice
>>> count calculation separately for different cases.
>>>
>>> v4:
>>> -Separate the pipe_bpp calculation for eDP and DP.
>>>
>>> Signed-off-by: Ankit Nautiyal <[e-mail]>
>>> ---
>>>   drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
>>>   1 file changed, 245 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>>> index 39e2bf3d738d..578320220c9a 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>>> @@ -1642,6 +1642,209 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
>>>   	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
>>>   }
>>>   
>>> +static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
>>> +				 const struct drm_display_mode *adjusted_mode)
>>> +{
>>> +	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
>>> +	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
>>> +
>>> +	return mode_rate <= link_avail;
>>> +}
>>> +
>>> +static int dsc_compute_link_config(struct intel_dp *intel_dp,
>>> +				   struct intel_crtc_state *pipe_config,
>>> +				   struct link_config_limits *limits,
>>> +				   int pipe_bpp,
>>> +				   u16 compressed_bpp,
>>> +				   int timeslots)
>>> +{
>>> +	const struct drm_display_mode *adjusted_mode =
>>> +		&pipe_config->hw.adjusted_mode;
>>> +	int link_rate, lane_count;
>>> +	int dsc_max_bpp;
>>> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>>> +	int i;
>>> +
>>> +	for (i = 0; i < intel_dp->num_common_rates; i++) {
>>> +		link_rate = intel_dp_common_rate(intel_dp, i);
>>> +		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
>>> +			continue;
>>> +
>>> +		for (lane_count = limits->min_lane_count;
>>> +		     lane_count <= limits->max_lane_count;
>>> +		     lane_count <<= 1) {
>>> +			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
>>> +									  link_rate,
>>> +									  lane_count,
>>> +									  adjusted_mode->crtc_clock,
>>> +									  adjusted_mode->crtc_hdisplay,
>>> +									  pipe_config->bigjoiner_pipes,
>>> +									  pipe_config->output_format,
>>> +									  pipe_bpp, timeslots);
>>> +			/*
>>> +			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
>>> +			 * supported PPS value can be 63.9375 and with the further
>>> +			 * mention that bpp should be programmed double the target bpp
>>> +			 * restricting our target bpp to be 31.9375 at max
>>> +			 */
>>> +			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
>>> +				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
>>> +
>>> +			if (compressed_bpp > dsc_max_bpp)
>>> +				continue;
>>> +
>>> +			if (!is_dsc_bw_sufficient(link_rate, lane_count,
>>> +						  compressed_bpp, adjusted_mode))
>>> +				continue;
>>> +
>>> +			pipe_config->lane_count = lane_count;
>>> +			pipe_config->port_clock = link_rate;
>>> +
>>> +			return 0;
>>> +		}
>>> +	}
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static
>>> +u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
>>> +					    struct intel_crtc_state *pipe_config,
>>> +					    int bpc)
>>> +{
>>> +	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
>>> +
>>> +	if (max_bppx16)
>>> +		return max_bppx16;
>>> +	/*
>>> +	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
>>> +	 * values as given in spec Table 2-157 DP v2.0
>>> +	 */
>>> +	switch (pipe_config->output_format) {
>>> +	case INTEL_OUTPUT_FORMAT_RGB:
>>> +	case INTEL_OUTPUT_FORMAT_YCBCR444:
>>> +		return (3 * bpc) << 4;
>>> +	case INTEL_OUTPUT_FORMAT_YCBCR420:
>>> +		return (3 * (bpc / 2)) << 4;
>>> +	default:
>>> +		MISSING_CASE(pipe_config->output_format);
>>> +		break;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
>>> +{
>>> +	switch (pipe_config->output_format) {
>>> +	case INTEL_OUTPUT_FORMAT_RGB:
>>> +	case INTEL_OUTPUT_FORMAT_YCBCR444:
>>> +		return 8 << 4;
>>> +	case INTEL_OUTPUT_FORMAT_YCBCR420:
>>> +		return 6 << 4;
>>> +	default:
>>> +		MISSING_CASE(pipe_config->output_format);
>>> +		break;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
>>> +				      struct intel_crtc_state *pipe_config,
>>> +				      struct link_config_limits *limits,
>>> +				      int pipe_bpp,
>>> +				      int timeslots)
>>> +{
>>> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>>> +	u16 compressed_bpp;
>>> +	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
>>> +	int ret;
>>> +
>>> +	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
>>> +	if (DISPLAY_VER(dev_priv) <= 12)
>>> +		dsc_src_max_bpp = 23;
>>> +	else
>>> +		dsc_src_max_bpp = 27;
>> I would may be added some comment about what are those "23/27" numbers or
>> may be even created some self-explanatory #define constants for those.
> I dislike defines like that. They are single use so don't actually
> do anything in terms of avoiding typoes and other accidental
> mismatches, and people always seem put them in some random place
> (eg. top of file) so then it takes extra work to find them.
>
> The best approach IMO is to just use functions with good names.
> Eg. in this case we could just have a full set of clear functions:
> dsc_{sink,source}_{min,max}_bpp() or something along those line.
>
>> Otherwise in a couple of years, even we ourselves might not be able to recall
>> immediately where those numbers are taken from.
>>
>> Otherwise looks good to me,
>>
>> Reviewed-by: Stanislav Lisovskiy <[e-mail]>
>>
>>> +	dsc_sink_max_bpp =
>>> +		intel_dp_dsc_max_sink_compressed_bppx16(intel_dp,
>>> +							pipe_config, pipe_bpp / 3) >> 4;
>>> +
>>> +	dsc_max_bpp = dsc_sink_max_bpp ? min(dsc_sink_max_bpp, dsc_src_max_bpp) : dsc_src_max_bpp;
>>> +
>>> +	/* Compressed BPP should be less than the Input DSC bpp */
>>> +	dsc_max_bpp = min(dsc_max_bpp, pipe_bpp - 1);
>>> +
>>> +	for (compressed_bpp = dsc_max_bpp;
>>> +	     compressed_bpp >= dsc_min_bpp;
>>> +	     compressed_bpp--) {
>>> +		ret = dsc_compute_link_config(intel_dp,
>>> +					      pipe_config,
>>> +					      limits,
>>> +					      pipe_bpp,
>>> +					      compressed_bpp,
>>> +					      timeslots);
>>> +		if (ret == 0) {
>>> +			pipe_config->dsc.compressed_bpp = compressed_bpp;
>>> +			return 0;
>>> +		}
>>> +	}
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>> +static int intel_dp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
>>> +					 struct intel_crtc_state *pipe_config,
>>> +					 struct drm_connector_state *conn_state,
>>> +					 struct link_config_limits *limits,
>>> +					 int timeslots)
>>> +{
>>> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>> +	u8 dsc_bpc[3] = {0};
>>> +	u8 dsc_max_bpc, dsc_max_bpp;
>>> +	u8 dsc_min_bpc, dsc_min_bpp;
>>> +	u8 max_req_bpc = conn_state->max_requested_bpc;
>>> +	int i, bpp, ret;
>>> +	int num_bpc = drm_dp_dsc_sink_supported_input_bpcs(intel_dp->dsc_dpcd,
>>> +							   dsc_bpc);
>>> +
>>> +	/* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */
>>> +	if (DISPLAY_VER(i915) >= 12)
>>> +		dsc_max_bpc = min_t(u8, 12, max_req_bpc);
>>> +	else
>>> +		dsc_max_bpc = min_t(u8, 10, max_req_bpc);
>>> +
>>> +	dsc_max_bpp = min(dsc_max_bpc * 3, limits->max_bpp);
>>> +
>>> +	/* Min DSC Input BPC for ICL+ is 8 */
>>> +	dsc_min_bpc = 8;
>>> +	dsc_min_bpp = max(dsc_min_bpc * 3, limits->min_bpp);
>>> +
>>> +	/*
>>> +	 * Get the maximum DSC bpc that will be supported by any valid
>>> +	 * link configuration and compressed bpp.
>>> +	 */
>>> +	for (i = 0; i < num_bpc; i++) {
>>> +		bpp = dsc_bpc[i] * 3;
>>> +
>>> +		if (bpp < dsc_min_bpp)
>>> +			break;
>>> +
>>> +		if (bpp > dsc_max_bpp)
>>> +			continue;
>>> +
>>> +		ret = dsc_compute_compressed_bpp(intel_dp, pipe_config,
>>> +						 limits, bpp, timeslots);
>>> +		if (ret == 0) {
>>> +			pipe_config->pipe_bpp = bpp;
>>> +
>>> +			return 0;
>>> +		}
>>> +	}
>>> +
>>> +	return -EINVAL;
>>> +}
>>> +
>>>   static
>>>   bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
>>>   {
>>> @@ -1649,6 +1852,31 @@ bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
>>>   	return (pipe_bpp < 8 * 3);
>>>   }
>>>   
>>> +static int intel_edp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
>>> +					  struct intel_crtc_state *pipe_config,
>>> +					  struct drm_connector_state *conn_state,
>>> +					  struct link_config_limits *limits)
>>> +{
>>> +	/* For eDP use max bpp that can be supported with DSC. */
>>> +	int pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
>>> +						    conn_state->max_requested_bpc);
>>> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>>> +
>>> +	if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
>>> +		drm_dbg_kms(&i915->drm,
>>> +			    "No DSC support for less than 8bpc\n");
>>> +		return -EINVAL;
>>> +	}
>>> +	pipe_config->pipe_bpp = pipe_bpp;
>>> +	pipe_config->port_clock = limits->max_rate;
>>> +	pipe_config->lane_count = limits->max_lane_count;
>>> +	pipe_config->dsc.compressed_bpp =
>>> +		min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
>>> +		      pipe_config->pipe_bpp);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>   int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>>   				struct intel_crtc_state *pipe_config,
>>>   				struct drm_connector_state *conn_state,
>>> @@ -1671,6 +1899,12 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>>   	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
>>>   		return -EINVAL;
>>>   
>>> +	/*
>>> +	 * compute pipe bpp is set to false for DP MST DSC case
>>> +	 * and compressed_bpp is calculated same time once
>>> +	 * vpci timeslots are allocated, because overall bpp
>>> +	 * calculation procedure is bit different for MST case.
>>> +	 */
>>>   	if (compute_pipe_bpp) {
>>>   		int pipe_bpp;
>>>   		int forced_bpp = intel_dp->force_dsc_bpc * 3;
>>> @@ -1683,31 +1917,25 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>>   				 forced_bpp && !is_dsc_pipe_bpp_sufficient(forced_bpp),
>>>   				 "Cannot force dsc bpc:%d, due to dsc bpc limits\n",
>>>   				 intel_dp->force_dsc_bpc);
>>> -
>>> -			pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
>>> -								conn_state->max_requested_bpc);
>>> -
>>> -			if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
>>> -				drm_dbg_kms(&dev_priv->drm, "No DSC support for less than 8bpc\n");
>>> -				return -EINVAL;
>>> +			if (intel_dp_is_edp(intel_dp))
>>> +				ret = intel_edp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
>>> +								     conn_state, limits);
>>> +			else
>>> +				ret = intel_dp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
>>> +								    conn_state, limits, timeslots);
>>> +			if (ret) {
>>> +				drm_dbg_kms(&dev_priv->drm,
>>> +					    "No Valid pipe bpp for given mode ret = %d\n", ret);
>>> +				return ret;
>>>   			}
>>>   		}
>>> -
>>> -		pipe_config->pipe_bpp = pipe_bpp;
>>>   	}
>>>   
>>> -	/*
>>> -	 * For now enable DSC for max link rate, max lane count.
>>> -	 * Optimize this later for the minimum possible link rate/lane count
>>> -	 * with DSC enabled for the requested mode.
>>> -	 */
>>>   	pipe_config->port_clock = limits->max_rate;
>>>   	pipe_config->lane_count = limits->max_lane_count;
>>>   
>>> +	/* Calculate Slice count */
>>>   	if (intel_dp_is_edp(intel_dp)) {
>>> -		pipe_config->dsc.compressed_bpp =
>>> -			min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
>>> -			      pipe_config->pipe_bpp);
>>>   		pipe_config->dsc.slice_count =
>>>   			drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
>>>   							true);
>>> @@ -1717,37 +1945,8 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>>   			return -EINVAL;
>>>   		}
>>>   	} else {
>>> -		u16 dsc_max_compressed_bpp = 0;
>>>   		u8 dsc_dp_slice_count;
>>>   
>>> -		if (compute_pipe_bpp) {
>>> -			dsc_max_compressed_bpp =
>>> -				intel_dp_dsc_get_max_compressed_bpp(dev_priv,
>>> -								    pipe_config->port_clock,
>>> -								    pipe_config->lane_count,
>>> -								    adjusted_mode->crtc_clock,
>>> -								    adjusted_mode->crtc_hdisplay,
>>> -								    pipe_config->bigjoiner_pipes,
>>> -								    pipe_config->output_format,
>>> -								    pipe_config->pipe_bpp,
>>> -								    timeslots);
>>> -			/*
>>> -			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
>>> -			 * supported PPS value can be 63.9375 and with the further
>>> -			 * mention that bpp should be programmed double the target bpp
>>> -			 * restricting our target bpp to be 31.9375 at max
>>> -			 */
>>> -			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
>>> -				dsc_max_compressed_bpp = min_t(u16,
>>> -							       dsc_max_compressed_bpp,
>>> -							       31);
>>> -
>>> -			if (!dsc_max_compressed_bpp) {
>>> -				drm_dbg_kms(&dev_priv->drm,
>>> -					    "Compressed BPP not supported\n");
>>> -				return -EINVAL;
>>> -			}
>>> -		}
>>>   		dsc_dp_slice_count =
>>>   			intel_dp_dsc_get_slice_count(intel_dp,
>>>   						     adjusted_mode->crtc_clock,
>>> @@ -1759,20 +1958,6 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>>>   			return -EINVAL;
>>>   		}
>>>   
>>> -		/*
>>> -		 * compute pipe bpp is set to false for DP MST DSC case
>>> -		 * and compressed_bpp is calculated same time once
>>> -		 * vpci timeslots are allocated, because overall bpp
>>> -		 * calculation procedure is bit different for MST case.
>>> -		 */
>>> -		if (compute_pipe_bpp) {
>>> -			u16 output_bpp = intel_dp_output_bpp(pipe_config->output_format,
>>> -							     pipe_config->pipe_bpp);
>>> -
>>> -			pipe_config->dsc.compressed_bpp = min_t(u16,
>>> -								dsc_max_compressed_bpp,
>>> -								output_bpp);
>>> -		}
>>>   		pipe_config->dsc.slice_count = dsc_dp_slice_count;
>>>   	}
>>>   	/*
>>> -- 
>>> 2.25.1
>>>

Stanislav Lisovskiy May 23, 2023, 9:01 a.m. UTC | #4

On Tue, May 16, 2023 at 02:40:27PM +0300, Ville Syrjälä wrote:
> On Tue, May 16, 2023 at 01:43:44PM +0300, Lisovskiy, Stanislav wrote:
> > On Fri, May 12, 2023 at 11:54:16AM +0530, Ankit Nautiyal wrote:
> > > Currently, we take the max lane, rate and pipe bpp, to get the maximum
> > > compressed bpp possible. We then set the output bpp to this value.
> > > This patch provides support to have max bpp, min rate and min lanes,
> > > that can support the min compressed bpp.
> > > 
> > > v2:
> > > -Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
> > > -Fix the checks for limits->max/min_bpp while iterating over list of
> > >  valid DSC bpcs. (Stan)
> > > 
> > > v3:
> > > -Refactor the code to have pipe bpp/compressed bpp computation and slice
> > > count calculation separately for different cases.
> > > 
> > > v4:
> > > -Separate the pipe_bpp calculation for eDP and DP.
> > > 
> > > Signed-off-by: Ankit Nautiyal <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
> > >  1 file changed, 245 insertions(+), 60 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > index 39e2bf3d738d..578320220c9a 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > @@ -1642,6 +1642,209 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
> > >  	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
> > >  }
> > >  
> > > +static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
> > > +				 const struct drm_display_mode *adjusted_mode)
> > > +{
> > > +	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
> > > +	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
> > > +
> > > +	return mode_rate <= link_avail;
> > > +}
> > > +
> > > +static int dsc_compute_link_config(struct intel_dp *intel_dp,
> > > +				   struct intel_crtc_state *pipe_config,
> > > +				   struct link_config_limits *limits,
> > > +				   int pipe_bpp,
> > > +				   u16 compressed_bpp,
> > > +				   int timeslots)
> > > +{
> > > +	const struct drm_display_mode *adjusted_mode =
> > > +		&pipe_config->hw.adjusted_mode;
> > > +	int link_rate, lane_count;
> > > +	int dsc_max_bpp;
> > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > +	int i;
> > > +
> > > +	for (i = 0; i < intel_dp->num_common_rates; i++) {
> > > +		link_rate = intel_dp_common_rate(intel_dp, i);
> > > +		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
> > > +			continue;
> > > +
> > > +		for (lane_count = limits->min_lane_count;
> > > +		     lane_count <= limits->max_lane_count;
> > > +		     lane_count <<= 1) {
> > > +			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> > > +									  link_rate,
> > > +									  lane_count,
> > > +									  adjusted_mode->crtc_clock,
> > > +									  adjusted_mode->crtc_hdisplay,
> > > +									  pipe_config->bigjoiner_pipes,
> > > +									  pipe_config->output_format,
> > > +									  pipe_bpp, timeslots);
> > > +			/*
> > > +			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> > > +			 * supported PPS value can be 63.9375 and with the further
> > > +			 * mention that bpp should be programmed double the target bpp
> > > +			 * restricting our target bpp to be 31.9375 at max
> > > +			 */
> > > +			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > > +				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
> > > +
> > > +			if (compressed_bpp > dsc_max_bpp)
> > > +				continue;
> > > +
> > > +			if (!is_dsc_bw_sufficient(link_rate, lane_count,
> > > +						  compressed_bpp, adjusted_mode))
> > > +				continue;
> > > +
> > > +			pipe_config->lane_count = lane_count;
> > > +			pipe_config->port_clock = link_rate;
> > > +
> > > +			return 0;
> > > +		}
> > > +	}
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static
> > > +u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
> > > +					    struct intel_crtc_state *pipe_config,
> > > +					    int bpc)
> > > +{
> > > +	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
> > > +
> > > +	if (max_bppx16)
> > > +		return max_bppx16;
> > > +	/*
> > > +	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
> > > +	 * values as given in spec Table 2-157 DP v2.0
> > > +	 */
> > > +	switch (pipe_config->output_format) {
> > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > +		return (3 * bpc) << 4;
> > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > +		return (3 * (bpc / 2)) << 4;
> > > +	default:
> > > +		MISSING_CASE(pipe_config->output_format);
> > > +		break;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
> > > +{
> > > +	switch (pipe_config->output_format) {
> > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > +		return 8 << 4;
> > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > +		return 6 << 4;
> > > +	default:
> > > +		MISSING_CASE(pipe_config->output_format);
> > > +		break;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
> > > +				      struct intel_crtc_state *pipe_config,
> > > +				      struct link_config_limits *limits,
> > > +				      int pipe_bpp,
> > > +				      int timeslots)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > +	u16 compressed_bpp;
> > > +	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
> > > +	int ret;
> > > +
> > > +	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
> > > +	if (DISPLAY_VER(dev_priv) <= 12)
> > > +		dsc_src_max_bpp = 23;
> > > +	else
> > > +		dsc_src_max_bpp = 27;
> > 
> > I would may be added some comment about what are those "23/27" numbers or
> > may be even created some self-explanatory #define constants for those.
> 
> I dislike defines like that. They are single use so don't actually
> do anything in terms of avoiding typoes and other accidental
> mismatches, and people always seem put them in some random place
> (eg. top of file) so then it takes extra work to find them.

Ah come on, even my primitive mcedit with ctags plugin can track it :))
However my point is that anything is better than just hard-coded magic
numbers, which is proven antipattern.
Also you never know if it is a single or multiple use, I think it should be
either defined as a constant or as a define, which is self explanatory.

> 
> The best approach IMO is to just use functions with good names.
> Eg. in this case we could just have a full set of clear functions:
> dsc_{sink,source}_{min,max}_bpp() or something along those line.

..which still doesn't explain, why it is 23 there, why it is 27, who sets those
numbers, which spec and so on.

Stan

> 
> > Otherwise in a couple of years, even we ourselves might not be able to recall
> > immediately where those numbers are taken from.
> > 
> > Otherwise looks good to me,
> > 
> > Reviewed-by: Stanislav Lisovskiy <[e-mail]>
> > 
> > > +	dsc_sink_max_bpp =
> > > +		intel_dp_dsc_max_sink_compressed_bppx16(intel_dp,
> > > +							pipe_config, pipe_bpp / 3) >> 4;
> > > +
> > > +	dsc_max_bpp = dsc_sink_max_bpp ? min(dsc_sink_max_bpp, dsc_src_max_bpp) : dsc_src_max_bpp;
> > > +
> > > +	/* Compressed BPP should be less than the Input DSC bpp */
> > > +	dsc_max_bpp = min(dsc_max_bpp, pipe_bpp - 1);
> > > +
> > > +	for (compressed_bpp = dsc_max_bpp;
> > > +	     compressed_bpp >= dsc_min_bpp;
> > > +	     compressed_bpp--) {
> > > +		ret = dsc_compute_link_config(intel_dp,
> > > +					      pipe_config,
> > > +					      limits,
> > > +					      pipe_bpp,
> > > +					      compressed_bpp,
> > > +					      timeslots);
> > > +		if (ret == 0) {
> > > +			pipe_config->dsc.compressed_bpp = compressed_bpp;
> > > +			return 0;
> > > +		}
> > > +	}
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static int intel_dp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
> > > +					 struct intel_crtc_state *pipe_config,
> > > +					 struct drm_connector_state *conn_state,
> > > +					 struct link_config_limits *limits,
> > > +					 int timeslots)
> > > +{
> > > +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > +	u8 dsc_bpc[3] = {0};
> > > +	u8 dsc_max_bpc, dsc_max_bpp;
> > > +	u8 dsc_min_bpc, dsc_min_bpp;
> > > +	u8 max_req_bpc = conn_state->max_requested_bpc;
> > > +	int i, bpp, ret;
> > > +	int num_bpc = drm_dp_dsc_sink_supported_input_bpcs(intel_dp->dsc_dpcd,
> > > +							   dsc_bpc);
> > > +
> > > +	/* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */
> > > +	if (DISPLAY_VER(i915) >= 12)
> > > +		dsc_max_bpc = min_t(u8, 12, max_req_bpc);
> > > +	else
> > > +		dsc_max_bpc = min_t(u8, 10, max_req_bpc);
> > > +
> > > +	dsc_max_bpp = min(dsc_max_bpc * 3, limits->max_bpp);
> > > +
> > > +	/* Min DSC Input BPC for ICL+ is 8 */
> > > +	dsc_min_bpc = 8;
> > > +	dsc_min_bpp = max(dsc_min_bpc * 3, limits->min_bpp);
> > > +
> > > +	/*
> > > +	 * Get the maximum DSC bpc that will be supported by any valid
> > > +	 * link configuration and compressed bpp.
> > > +	 */
> > > +	for (i = 0; i < num_bpc; i++) {
> > > +		bpp = dsc_bpc[i] * 3;
> > > +
> > > +		if (bpp < dsc_min_bpp)
> > > +			break;
> > > +
> > > +		if (bpp > dsc_max_bpp)
> > > +			continue;
> > > +
> > > +		ret = dsc_compute_compressed_bpp(intel_dp, pipe_config,
> > > +						 limits, bpp, timeslots);
> > > +		if (ret == 0) {
> > > +			pipe_config->pipe_bpp = bpp;
> > > +
> > > +			return 0;
> > > +		}
> > > +	}
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +
> > >  static
> > >  bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
> > >  {
> > > @@ -1649,6 +1852,31 @@ bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
> > >  	return (pipe_bpp < 8 * 3);
> > >  }
> > >  
> > > +static int intel_edp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
> > > +					  struct intel_crtc_state *pipe_config,
> > > +					  struct drm_connector_state *conn_state,
> > > +					  struct link_config_limits *limits)
> > > +{
> > > +	/* For eDP use max bpp that can be supported with DSC. */
> > > +	int pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
> > > +						    conn_state->max_requested_bpc);
> > > +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > > +
> > > +	if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
> > > +		drm_dbg_kms(&i915->drm,
> > > +			    "No DSC support for less than 8bpc\n");
> > > +		return -EINVAL;
> > > +	}
> > > +	pipe_config->pipe_bpp = pipe_bpp;
> > > +	pipe_config->port_clock = limits->max_rate;
> > > +	pipe_config->lane_count = limits->max_lane_count;
> > > +	pipe_config->dsc.compressed_bpp =
> > > +		min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
> > > +		      pipe_config->pipe_bpp);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > >  				struct intel_crtc_state *pipe_config,
> > >  				struct drm_connector_state *conn_state,
> > > @@ -1671,6 +1899,12 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > >  	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
> > >  		return -EINVAL;
> > >  
> > > +	/*
> > > +	 * compute pipe bpp is set to false for DP MST DSC case
> > > +	 * and compressed_bpp is calculated same time once
> > > +	 * vpci timeslots are allocated, because overall bpp
> > > +	 * calculation procedure is bit different for MST case.
> > > +	 */
> > >  	if (compute_pipe_bpp) {
> > >  		int pipe_bpp;
> > >  		int forced_bpp = intel_dp->force_dsc_bpc * 3;
> > > @@ -1683,31 +1917,25 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > >  				 forced_bpp && !is_dsc_pipe_bpp_sufficient(forced_bpp),
> > >  				 "Cannot force dsc bpc:%d, due to dsc bpc limits\n",
> > >  				 intel_dp->force_dsc_bpc);
> > > -
> > > -			pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
> > > -								conn_state->max_requested_bpc);
> > > -
> > > -			if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
> > > -				drm_dbg_kms(&dev_priv->drm, "No DSC support for less than 8bpc\n");
> > > -				return -EINVAL;
> > > +			if (intel_dp_is_edp(intel_dp))
> > > +				ret = intel_edp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
> > > +								     conn_state, limits);
> > > +			else
> > > +				ret = intel_dp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
> > > +								    conn_state, limits, timeslots);
> > > +			if (ret) {
> > > +				drm_dbg_kms(&dev_priv->drm,
> > > +					    "No Valid pipe bpp for given mode ret = %d\n", ret);
> > > +				return ret;
> > >  			}
> > >  		}
> > > -
> > > -		pipe_config->pipe_bpp = pipe_bpp;
> > >  	}
> > >  
> > > -	/*
> > > -	 * For now enable DSC for max link rate, max lane count.
> > > -	 * Optimize this later for the minimum possible link rate/lane count
> > > -	 * with DSC enabled for the requested mode.
> > > -	 */
> > >  	pipe_config->port_clock = limits->max_rate;
> > >  	pipe_config->lane_count = limits->max_lane_count;
> > >  
> > > +	/* Calculate Slice count */
> > >  	if (intel_dp_is_edp(intel_dp)) {
> > > -		pipe_config->dsc.compressed_bpp =
> > > -			min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
> > > -			      pipe_config->pipe_bpp);
> > >  		pipe_config->dsc.slice_count =
> > >  			drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
> > >  							true);
> > > @@ -1717,37 +1945,8 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > >  			return -EINVAL;
> > >  		}
> > >  	} else {
> > > -		u16 dsc_max_compressed_bpp = 0;
> > >  		u8 dsc_dp_slice_count;
> > >  
> > > -		if (compute_pipe_bpp) {
> > > -			dsc_max_compressed_bpp =
> > > -				intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> > > -								    pipe_config->port_clock,
> > > -								    pipe_config->lane_count,
> > > -								    adjusted_mode->crtc_clock,
> > > -								    adjusted_mode->crtc_hdisplay,
> > > -								    pipe_config->bigjoiner_pipes,
> > > -								    pipe_config->output_format,
> > > -								    pipe_config->pipe_bpp,
> > > -								    timeslots);
> > > -			/*
> > > -			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> > > -			 * supported PPS value can be 63.9375 and with the further
> > > -			 * mention that bpp should be programmed double the target bpp
> > > -			 * restricting our target bpp to be 31.9375 at max
> > > -			 */
> > > -			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > > -				dsc_max_compressed_bpp = min_t(u16,
> > > -							       dsc_max_compressed_bpp,
> > > -							       31);
> > > -
> > > -			if (!dsc_max_compressed_bpp) {
> > > -				drm_dbg_kms(&dev_priv->drm,
> > > -					    "Compressed BPP not supported\n");
> > > -				return -EINVAL;
> > > -			}
> > > -		}
> > >  		dsc_dp_slice_count =
> > >  			intel_dp_dsc_get_slice_count(intel_dp,
> > >  						     adjusted_mode->crtc_clock,
> > > @@ -1759,20 +1958,6 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > >  			return -EINVAL;
> > >  		}
> > >  
> > > -		/*
> > > -		 * compute pipe bpp is set to false for DP MST DSC case
> > > -		 * and compressed_bpp is calculated same time once
> > > -		 * vpci timeslots are allocated, because overall bpp
> > > -		 * calculation procedure is bit different for MST case.
> > > -		 */
> > > -		if (compute_pipe_bpp) {
> > > -			u16 output_bpp = intel_dp_output_bpp(pipe_config->output_format,
> > > -							     pipe_config->pipe_bpp);
> > > -
> > > -			pipe_config->dsc.compressed_bpp = min_t(u16,
> > > -								dsc_max_compressed_bpp,
> > > -								output_bpp);
> > > -		}
> > >  		pipe_config->dsc.slice_count = dsc_dp_slice_count;
> > >  	}
> > >  	/*
> > > -- 
> > > 2.25.1
> > > 
> 
> -- 
> Ville Syrjälä
> Intel

Ville Syrjälä May 24, 2023, 12:38 p.m. UTC | #5

On Tue, May 23, 2023 at 12:01:34PM +0300, Lisovskiy, Stanislav wrote:
> On Tue, May 16, 2023 at 02:40:27PM +0300, Ville Syrjälä wrote:
> > On Tue, May 16, 2023 at 01:43:44PM +0300, Lisovskiy, Stanislav wrote:
> > > On Fri, May 12, 2023 at 11:54:16AM +0530, Ankit Nautiyal wrote:
> > > > Currently, we take the max lane, rate and pipe bpp, to get the maximum
> > > > compressed bpp possible. We then set the output bpp to this value.
> > > > This patch provides support to have max bpp, min rate and min lanes,
> > > > that can support the min compressed bpp.
> > > > 
> > > > v2:
> > > > -Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
> > > > -Fix the checks for limits->max/min_bpp while iterating over list of
> > > >  valid DSC bpcs. (Stan)
> > > > 
> > > > v3:
> > > > -Refactor the code to have pipe bpp/compressed bpp computation and slice
> > > > count calculation separately for different cases.
> > > > 
> > > > v4:
> > > > -Separate the pipe_bpp calculation for eDP and DP.
> > > > 
> > > > Signed-off-by: Ankit Nautiyal <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
> > > >  1 file changed, 245 insertions(+), 60 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > index 39e2bf3d738d..578320220c9a 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > @@ -1642,6 +1642,209 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
> > > >  	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
> > > >  }
> > > >  
> > > > +static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
> > > > +				 const struct drm_display_mode *adjusted_mode)
> > > > +{
> > > > +	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
> > > > +	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
> > > > +
> > > > +	return mode_rate <= link_avail;
> > > > +}
> > > > +
> > > > +static int dsc_compute_link_config(struct intel_dp *intel_dp,
> > > > +				   struct intel_crtc_state *pipe_config,
> > > > +				   struct link_config_limits *limits,
> > > > +				   int pipe_bpp,
> > > > +				   u16 compressed_bpp,
> > > > +				   int timeslots)
> > > > +{
> > > > +	const struct drm_display_mode *adjusted_mode =
> > > > +		&pipe_config->hw.adjusted_mode;
> > > > +	int link_rate, lane_count;
> > > > +	int dsc_max_bpp;
> > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > +	int i;
> > > > +
> > > > +	for (i = 0; i < intel_dp->num_common_rates; i++) {
> > > > +		link_rate = intel_dp_common_rate(intel_dp, i);
> > > > +		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
> > > > +			continue;
> > > > +
> > > > +		for (lane_count = limits->min_lane_count;
> > > > +		     lane_count <= limits->max_lane_count;
> > > > +		     lane_count <<= 1) {
> > > > +			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> > > > +									  link_rate,
> > > > +									  lane_count,
> > > > +									  adjusted_mode->crtc_clock,
> > > > +									  adjusted_mode->crtc_hdisplay,
> > > > +									  pipe_config->bigjoiner_pipes,
> > > > +									  pipe_config->output_format,
> > > > +									  pipe_bpp, timeslots);
> > > > +			/*
> > > > +			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> > > > +			 * supported PPS value can be 63.9375 and with the further
> > > > +			 * mention that bpp should be programmed double the target bpp
> > > > +			 * restricting our target bpp to be 31.9375 at max
> > > > +			 */
> > > > +			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > > > +				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
> > > > +
> > > > +			if (compressed_bpp > dsc_max_bpp)
> > > > +				continue;
> > > > +
> > > > +			if (!is_dsc_bw_sufficient(link_rate, lane_count,
> > > > +						  compressed_bpp, adjusted_mode))
> > > > +				continue;
> > > > +
> > > > +			pipe_config->lane_count = lane_count;
> > > > +			pipe_config->port_clock = link_rate;
> > > > +
> > > > +			return 0;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return -EINVAL;
> > > > +}
> > > > +
> > > > +static
> > > > +u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
> > > > +					    struct intel_crtc_state *pipe_config,
> > > > +					    int bpc)
> > > > +{
> > > > +	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
> > > > +
> > > > +	if (max_bppx16)
> > > > +		return max_bppx16;
> > > > +	/*
> > > > +	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
> > > > +	 * values as given in spec Table 2-157 DP v2.0
> > > > +	 */
> > > > +	switch (pipe_config->output_format) {
> > > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > > +		return (3 * bpc) << 4;
> > > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > > +		return (3 * (bpc / 2)) << 4;
> > > > +	default:
> > > > +		MISSING_CASE(pipe_config->output_format);
> > > > +		break;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
> > > > +{
> > > > +	switch (pipe_config->output_format) {
> > > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > > +		return 8 << 4;
> > > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > > +		return 6 << 4;
> > > > +	default:
> > > > +		MISSING_CASE(pipe_config->output_format);
> > > > +		break;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
> > > > +				      struct intel_crtc_state *pipe_config,
> > > > +				      struct link_config_limits *limits,
> > > > +				      int pipe_bpp,
> > > > +				      int timeslots)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > +	u16 compressed_bpp;
> > > > +	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
> > > > +	int ret;
> > > > +
> > > > +	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
> > > > +	if (DISPLAY_VER(dev_priv) <= 12)
> > > > +		dsc_src_max_bpp = 23;
> > > > +	else
> > > > +		dsc_src_max_bpp = 27;
> > > 
> > > I would may be added some comment about what are those "23/27" numbers or
> > > may be even created some self-explanatory #define constants for those.
> > 
> > I dislike defines like that. They are single use so don't actually
> > do anything in terms of avoiding typoes and other accidental
> > mismatches, and people always seem put them in some random place
> > (eg. top of file) so then it takes extra work to find them.
> 
> Ah come on, even my primitive mcedit with ctags plugin can track it :))
> However my point is that anything is better than just hard-coded magic
> numbers, which is proven antipattern.

It's still a magic number whether you hide it behind a define or not.

> Also you never know if it is a single or multiple use,

If you use it multiple times then you aren't using the function
correctly.

> I think it should be
> either defined as a constant or as a define, which is self explanatory.

No more self explanatory than a function. Once you have the
function the define is entirely redundant.

> 
> > 
> > The best approach IMO is to just use functions with good names.
> > Eg. in this case we could just have a full set of clear functions:
> > dsc_{sink,source}_{min,max}_bpp() or something along those line.
> 
> ..which still doesn't explain, why it is 23 there, why it is 27, who sets those
> numbers, which spec and so on.

Neither does a define. All a define will do is say that for platform
X return define Y which is defined as Z. Returning Z directly is less
convoluted and just as helpful in figuring out where the numbers came
from. If it's hard to figure out where the number came from then you
can add a comment to indicate where it is specified.

Stanislav Lisovskiy May 24, 2023, 12:59 p.m. UTC | #6

On Wed, May 24, 2023 at 03:38:42PM +0300, Ville Syrjälä wrote:
> On Tue, May 23, 2023 at 12:01:34PM +0300, Lisovskiy, Stanislav wrote:
> > On Tue, May 16, 2023 at 02:40:27PM +0300, Ville Syrjälä wrote:
> > > On Tue, May 16, 2023 at 01:43:44PM +0300, Lisovskiy, Stanislav wrote:
> > > > On Fri, May 12, 2023 at 11:54:16AM +0530, Ankit Nautiyal wrote:
> > > > > Currently, we take the max lane, rate and pipe bpp, to get the maximum
> > > > > compressed bpp possible. We then set the output bpp to this value.
> > > > > This patch provides support to have max bpp, min rate and min lanes,
> > > > > that can support the min compressed bpp.
> > > > > 
> > > > > v2:
> > > > > -Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
> > > > > -Fix the checks for limits->max/min_bpp while iterating over list of
> > > > >  valid DSC bpcs. (Stan)
> > > > > 
> > > > > v3:
> > > > > -Refactor the code to have pipe bpp/compressed bpp computation and slice
> > > > > count calculation separately for different cases.
> > > > > 
> > > > > v4:
> > > > > -Separate the pipe_bpp calculation for eDP and DP.
> > > > > 
> > > > > Signed-off-by: Ankit Nautiyal <[e-mail]>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
> > > > >  1 file changed, 245 insertions(+), 60 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > index 39e2bf3d738d..578320220c9a 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > @@ -1642,6 +1642,209 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
> > > > >  	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
> > > > >  }
> > > > >  
> > > > > +static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
> > > > > +				 const struct drm_display_mode *adjusted_mode)
> > > > > +{
> > > > > +	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
> > > > > +	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
> > > > > +
> > > > > +	return mode_rate <= link_avail;
> > > > > +}
> > > > > +
> > > > > +static int dsc_compute_link_config(struct intel_dp *intel_dp,
> > > > > +				   struct intel_crtc_state *pipe_config,
> > > > > +				   struct link_config_limits *limits,
> > > > > +				   int pipe_bpp,
> > > > > +				   u16 compressed_bpp,
> > > > > +				   int timeslots)
> > > > > +{
> > > > > +	const struct drm_display_mode *adjusted_mode =
> > > > > +		&pipe_config->hw.adjusted_mode;
> > > > > +	int link_rate, lane_count;
> > > > > +	int dsc_max_bpp;
> > > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > > +	int i;
> > > > > +
> > > > > +	for (i = 0; i < intel_dp->num_common_rates; i++) {
> > > > > +		link_rate = intel_dp_common_rate(intel_dp, i);
> > > > > +		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
> > > > > +			continue;
> > > > > +
> > > > > +		for (lane_count = limits->min_lane_count;
> > > > > +		     lane_count <= limits->max_lane_count;
> > > > > +		     lane_count <<= 1) {
> > > > > +			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> > > > > +									  link_rate,
> > > > > +									  lane_count,
> > > > > +									  adjusted_mode->crtc_clock,
> > > > > +									  adjusted_mode->crtc_hdisplay,
> > > > > +									  pipe_config->bigjoiner_pipes,
> > > > > +									  pipe_config->output_format,
> > > > > +									  pipe_bpp, timeslots);
> > > > > +			/*
> > > > > +			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> > > > > +			 * supported PPS value can be 63.9375 and with the further
> > > > > +			 * mention that bpp should be programmed double the target bpp
> > > > > +			 * restricting our target bpp to be 31.9375 at max
> > > > > +			 */
> > > > > +			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > > > > +				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
> > > > > +
> > > > > +			if (compressed_bpp > dsc_max_bpp)
> > > > > +				continue;
> > > > > +
> > > > > +			if (!is_dsc_bw_sufficient(link_rate, lane_count,
> > > > > +						  compressed_bpp, adjusted_mode))
> > > > > +				continue;
> > > > > +
> > > > > +			pipe_config->lane_count = lane_count;
> > > > > +			pipe_config->port_clock = link_rate;
> > > > > +
> > > > > +			return 0;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	return -EINVAL;
> > > > > +}
> > > > > +
> > > > > +static
> > > > > +u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
> > > > > +					    struct intel_crtc_state *pipe_config,
> > > > > +					    int bpc)
> > > > > +{
> > > > > +	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
> > > > > +
> > > > > +	if (max_bppx16)
> > > > > +		return max_bppx16;
> > > > > +	/*
> > > > > +	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
> > > > > +	 * values as given in spec Table 2-157 DP v2.0
> > > > > +	 */
> > > > > +	switch (pipe_config->output_format) {
> > > > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > > > +		return (3 * bpc) << 4;
> > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > > > +		return (3 * (bpc / 2)) << 4;
> > > > > +	default:
> > > > > +		MISSING_CASE(pipe_config->output_format);
> > > > > +		break;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
> > > > > +{
> > > > > +	switch (pipe_config->output_format) {
> > > > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > > > +		return 8 << 4;
> > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > > > +		return 6 << 4;
> > > > > +	default:
> > > > > +		MISSING_CASE(pipe_config->output_format);
> > > > > +		break;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
> > > > > +				      struct intel_crtc_state *pipe_config,
> > > > > +				      struct link_config_limits *limits,
> > > > > +				      int pipe_bpp,
> > > > > +				      int timeslots)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > > +	u16 compressed_bpp;
> > > > > +	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
> > > > > +	int ret;
> > > > > +
> > > > > +	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
> > > > > +	if (DISPLAY_VER(dev_priv) <= 12)
> > > > > +		dsc_src_max_bpp = 23;
> > > > > +	else
> > > > > +		dsc_src_max_bpp = 27;
> > > > 
> > > > I would may be added some comment about what are those "23/27" numbers or
> > > > may be even created some self-explanatory #define constants for those.
> > > 
> > > I dislike defines like that. They are single use so don't actually
> > > do anything in terms of avoiding typoes and other accidental
> > > mismatches, and people always seem put them in some random place
> > > (eg. top of file) so then it takes extra work to find them.
> > 
> > Ah come on, even my primitive mcedit with ctags plugin can track it :))
> > However my point is that anything is better than just hard-coded magic
> > numbers, which is proven antipattern.
> 
> It's still a magic number whether you hide it behind a define or not.

define will gixe a clue at least

> 
> > Also you never know if it is a single or multiple use,
> 
> If you use it multiple times then you aren't using the function
> correctly.

hmm.. I didn't understand we are using defines in many places, like
register names, things like PIPE_A(which is enum but still), it is just 
more explanatory and elegant rather than use 0 instead of PIPE_A, right?

> 
> > I think it should be
> > either defined as a constant or as a define, which is self explanatory.
> 
> No more self explanatory than a function. Once you have the
> function the define is entirely redundant.

The function explains what it does, but I'm afraid you can't put explanation
for all the constants it used in a single function name..

> 
> > 
> > > 
> > > The best approach IMO is to just use functions with good names.
> > > Eg. in this case we could just have a full set of clear functions:
> > > dsc_{sink,source}_{min,max}_bpp() or something along those line.
> > 
> > ..which still doesn't explain, why it is 23 there, why it is 27, who sets those
> > numbers, which spec and so on.
> 
> Neither does a define. All a define will do is say that for platform
> X return define Y which is defined as Z. Returning Z directly is less
> convoluted and just as helpful in figuring out where the numbers came
> from. If it's hard to figure out where the number came from then you
> can add a comment to indicate where it is specified.

Well, I agree about the comment. Comment might do, but hardcoded numbers
are wrong. With this logic we could also not use register names like PLANE_CTL
but just write 0x40*** and then rely that function name will explain what happens
here or use pipe numbers like 0,1,2, instead of PIPE_A/PIPE_B...
Of course we could, but it doesn't look nice then, because numbers do not carry any 
info..

I mean, a concept that magic/hardcoded numbers are wrong isn't even my opinion, it is just 
world-wide known antipattern.
If you don't like defines there are constants also and whatever, my point is that just
hardcoded numbers are almost always bad.

Anyways I'm fine with a comment, as well, just because I think endless arguing is even worse than
magic numbers..

Stan

> 
> -- 
> Ville Syrjälä
> Intel

Ville Syrjälä May 24, 2023, 1:16 p.m. UTC | #7

On Wed, May 24, 2023 at 03:59:47PM +0300, Lisovskiy, Stanislav wrote:
> On Wed, May 24, 2023 at 03:38:42PM +0300, Ville Syrjälä wrote:
> > On Tue, May 23, 2023 at 12:01:34PM +0300, Lisovskiy, Stanislav wrote:
> > > On Tue, May 16, 2023 at 02:40:27PM +0300, Ville Syrjälä wrote:
> > > > On Tue, May 16, 2023 at 01:43:44PM +0300, Lisovskiy, Stanislav wrote:
> > > > > On Fri, May 12, 2023 at 11:54:16AM +0530, Ankit Nautiyal wrote:
> > > > > > Currently, we take the max lane, rate and pipe bpp, to get the maximum
> > > > > > compressed bpp possible. We then set the output bpp to this value.
> > > > > > This patch provides support to have max bpp, min rate and min lanes,
> > > > > > that can support the min compressed bpp.
> > > > > > 
> > > > > > v2:
> > > > > > -Avoid ending up with compressed bpp, same as pipe bpp. (Stan)
> > > > > > -Fix the checks for limits->max/min_bpp while iterating over list of
> > > > > >  valid DSC bpcs. (Stan)
> > > > > > 
> > > > > > v3:
> > > > > > -Refactor the code to have pipe bpp/compressed bpp computation and slice
> > > > > > count calculation separately for different cases.
> > > > > > 
> > > > > > v4:
> > > > > > -Separate the pipe_bpp calculation for eDP and DP.
> > > > > > 
> > > > > > Signed-off-by: Ankit Nautiyal <[e-mail]>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/display/intel_dp.c | 305 +++++++++++++++++++-----
> > > > > >  1 file changed, 245 insertions(+), 60 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > index 39e2bf3d738d..578320220c9a 100644
> > > > > > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > > > > > @@ -1642,6 +1642,209 @@ static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
> > > > > >  	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
> > > > > >  }
> > > > > >  
> > > > > > +static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
> > > > > > +				 const struct drm_display_mode *adjusted_mode)
> > > > > > +{
> > > > > > +	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
> > > > > > +	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
> > > > > > +
> > > > > > +	return mode_rate <= link_avail;
> > > > > > +}
> > > > > > +
> > > > > > +static int dsc_compute_link_config(struct intel_dp *intel_dp,
> > > > > > +				   struct intel_crtc_state *pipe_config,
> > > > > > +				   struct link_config_limits *limits,
> > > > > > +				   int pipe_bpp,
> > > > > > +				   u16 compressed_bpp,
> > > > > > +				   int timeslots)
> > > > > > +{
> > > > > > +	const struct drm_display_mode *adjusted_mode =
> > > > > > +		&pipe_config->hw.adjusted_mode;
> > > > > > +	int link_rate, lane_count;
> > > > > > +	int dsc_max_bpp;
> > > > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > > > +	int i;
> > > > > > +
> > > > > > +	for (i = 0; i < intel_dp->num_common_rates; i++) {
> > > > > > +		link_rate = intel_dp_common_rate(intel_dp, i);
> > > > > > +		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
> > > > > > +			continue;
> > > > > > +
> > > > > > +		for (lane_count = limits->min_lane_count;
> > > > > > +		     lane_count <= limits->max_lane_count;
> > > > > > +		     lane_count <<= 1) {
> > > > > > +			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
> > > > > > +									  link_rate,
> > > > > > +									  lane_count,
> > > > > > +									  adjusted_mode->crtc_clock,
> > > > > > +									  adjusted_mode->crtc_hdisplay,
> > > > > > +									  pipe_config->bigjoiner_pipes,
> > > > > > +									  pipe_config->output_format,
> > > > > > +									  pipe_bpp, timeslots);
> > > > > > +			/*
> > > > > > +			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
> > > > > > +			 * supported PPS value can be 63.9375 and with the further
> > > > > > +			 * mention that bpp should be programmed double the target bpp
> > > > > > +			 * restricting our target bpp to be 31.9375 at max
> > > > > > +			 */
> > > > > > +			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> > > > > > +				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
> > > > > > +
> > > > > > +			if (compressed_bpp > dsc_max_bpp)
> > > > > > +				continue;
> > > > > > +
> > > > > > +			if (!is_dsc_bw_sufficient(link_rate, lane_count,
> > > > > > +						  compressed_bpp, adjusted_mode))
> > > > > > +				continue;
> > > > > > +
> > > > > > +			pipe_config->lane_count = lane_count;
> > > > > > +			pipe_config->port_clock = link_rate;
> > > > > > +
> > > > > > +			return 0;
> > > > > > +		}
> > > > > > +	}
> > > > > > +
> > > > > > +	return -EINVAL;
> > > > > > +}
> > > > > > +
> > > > > > +static
> > > > > > +u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
> > > > > > +					    struct intel_crtc_state *pipe_config,
> > > > > > +					    int bpc)
> > > > > > +{
> > > > > > +	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
> > > > > > +
> > > > > > +	if (max_bppx16)
> > > > > > +		return max_bppx16;
> > > > > > +	/*
> > > > > > +	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
> > > > > > +	 * values as given in spec Table 2-157 DP v2.0
> > > > > > +	 */
> > > > > > +	switch (pipe_config->output_format) {
> > > > > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > > > > +		return (3 * bpc) << 4;
> > > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > > > > +		return (3 * (bpc / 2)) << 4;
> > > > > > +	default:
> > > > > > +		MISSING_CASE(pipe_config->output_format);
> > > > > > +		break;
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
> > > > > > +{
> > > > > > +	switch (pipe_config->output_format) {
> > > > > > +	case INTEL_OUTPUT_FORMAT_RGB:
> > > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR444:
> > > > > > +		return 8 << 4;
> > > > > > +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> > > > > > +		return 6 << 4;
> > > > > > +	default:
> > > > > > +		MISSING_CASE(pipe_config->output_format);
> > > > > > +		break;
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
> > > > > > +				      struct intel_crtc_state *pipe_config,
> > > > > > +				      struct link_config_limits *limits,
> > > > > > +				      int pipe_bpp,
> > > > > > +				      int timeslots)
> > > > > > +{
> > > > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > > > +	u16 compressed_bpp;
> > > > > > +	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
> > > > > > +	if (DISPLAY_VER(dev_priv) <= 12)
> > > > > > +		dsc_src_max_bpp = 23;
> > > > > > +	else
> > > > > > +		dsc_src_max_bpp = 27;
> > > > > 
> > > > > I would may be added some comment about what are those "23/27" numbers or
> > > > > may be even created some self-explanatory #define constants for those.
> > > > 
> > > > I dislike defines like that. They are single use so don't actually
> > > > do anything in terms of avoiding typoes and other accidental
> > > > mismatches, and people always seem put them in some random place
> > > > (eg. top of file) so then it takes extra work to find them.
> > > 
> > > Ah come on, even my primitive mcedit with ctags plugin can track it :))
> > > However my point is that anything is better than just hard-coded magic
> > > numbers, which is proven antipattern.
> > 
> > It's still a magic number whether you hide it behind a define or not.
> 
> define will gixe a clue at least
> 
> > 
> > > Also you never know if it is a single or multiple use,
> > 
> > If you use it multiple times then you aren't using the function
> > correctly.
> 
> hmm.. I didn't understand we are using defines in many places, like
> register names, things like PIPE_A(which is enum but still), it is just 
> more explanatory and elegant rather than use 0 instead of PIPE_A, right?
> 
> > 
> > > I think it should be
> > > either defined as a constant or as a define, which is self explanatory.
> > 
> > No more self explanatory than a function. Once you have the
> > function the define is entirely redundant.
> 
> The function explains what it does, but I'm afraid you can't put explanation
> for all the constants it used in a single function name..

#define FOO_MAX_BPP 23
#define BAR_MAX_BPP 27

...
int max_bpp()
{
	if (IS_FOO)
		return FOO_MAX_BPP;
	else
		return BAR_MAX_BPP;
}

vs.

int max_bpp()
{
	if (IS_FOO)
		return 23;
	else
		return 27;
}

The defines don't add any extra infrmation here.

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 39e2bf3d738d..578320220c9a 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1642,6 +1642,209 @@  static bool intel_dp_dsc_supports_format(struct intel_dp *intel_dp,
 	return drm_dp_dsc_sink_supports_format(intel_dp->dsc_dpcd, sink_dsc_format);
 }
 
+static bool is_dsc_bw_sufficient(int link_rate, int lane_count, int compressed_bpp,
+				 const struct drm_display_mode *adjusted_mode)
+{
+	int mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock, compressed_bpp);
+	int link_avail = intel_dp_max_data_rate(link_rate, lane_count);
+
+	return mode_rate <= link_avail;
+}
+
+static int dsc_compute_link_config(struct intel_dp *intel_dp,
+				   struct intel_crtc_state *pipe_config,
+				   struct link_config_limits *limits,
+				   int pipe_bpp,
+				   u16 compressed_bpp,
+				   int timeslots)
+{
+	const struct drm_display_mode *adjusted_mode =
+		&pipe_config->hw.adjusted_mode;
+	int link_rate, lane_count;
+	int dsc_max_bpp;
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	int i;
+
+	for (i = 0; i < intel_dp->num_common_rates; i++) {
+		link_rate = intel_dp_common_rate(intel_dp, i);
+		if (link_rate < limits->min_rate || link_rate > limits->max_rate)
+			continue;
+
+		for (lane_count = limits->min_lane_count;
+		     lane_count <= limits->max_lane_count;
+		     lane_count <<= 1) {
+			dsc_max_bpp = intel_dp_dsc_get_max_compressed_bpp(dev_priv,
+									  link_rate,
+									  lane_count,
+									  adjusted_mode->crtc_clock,
+									  adjusted_mode->crtc_hdisplay,
+									  pipe_config->bigjoiner_pipes,
+									  pipe_config->output_format,
+									  pipe_bpp, timeslots);
+			/*
+			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
+			 * supported PPS value can be 63.9375 and with the further
+			 * mention that bpp should be programmed double the target bpp
+			 * restricting our target bpp to be 31.9375 at max
+			 */
+			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
+				dsc_max_bpp = min_t(u16, dsc_max_bpp, 31);
+
+			if (compressed_bpp > dsc_max_bpp)
+				continue;
+
+			if (!is_dsc_bw_sufficient(link_rate, lane_count,
+						  compressed_bpp, adjusted_mode))
+				continue;
+
+			pipe_config->lane_count = lane_count;
+			pipe_config->port_clock = link_rate;
+
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static
+u16 intel_dp_dsc_max_sink_compressed_bppx16(struct intel_dp *intel_dp,
+					    struct intel_crtc_state *pipe_config,
+					    int bpc)
+{
+	u16 max_bppx16 = drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd);
+
+	if (max_bppx16)
+		return max_bppx16;
+	/*
+	 * If support not given in DPCD 67h, 68h use the Maximum Allowed bit rate
+	 * values as given in spec Table 2-157 DP v2.0
+	 */
+	switch (pipe_config->output_format) {
+	case INTEL_OUTPUT_FORMAT_RGB:
+	case INTEL_OUTPUT_FORMAT_YCBCR444:
+		return (3 * bpc) << 4;
+	case INTEL_OUTPUT_FORMAT_YCBCR420:
+		return (3 * (bpc / 2)) << 4;
+	default:
+		MISSING_CASE(pipe_config->output_format);
+		break;
+	}
+
+	return 0;
+}
+
+static u16 intel_dp_dsc_min_compressed_bppx16(struct intel_crtc_state *pipe_config)
+{
+	switch (pipe_config->output_format) {
+	case INTEL_OUTPUT_FORMAT_RGB:
+	case INTEL_OUTPUT_FORMAT_YCBCR444:
+		return 8 << 4;
+	case INTEL_OUTPUT_FORMAT_YCBCR420:
+		return 6 << 4;
+	default:
+		MISSING_CASE(pipe_config->output_format);
+		break;
+	}
+
+	return 0;
+}
+
+static int dsc_compute_compressed_bpp(struct intel_dp *intel_dp,
+				      struct intel_crtc_state *pipe_config,
+				      struct link_config_limits *limits,
+				      int pipe_bpp,
+				      int timeslots)
+{
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	u16 compressed_bpp;
+	int dsc_min_bpp, dsc_src_max_bpp, dsc_sink_max_bpp, dsc_max_bpp;
+	int ret;
+
+	dsc_min_bpp = max(intel_dp_dsc_min_compressed_bppx16(pipe_config) >> 4, 8);
+	if (DISPLAY_VER(dev_priv) <= 12)
+		dsc_src_max_bpp = 23;
+	else
+		dsc_src_max_bpp = 27;
+	dsc_sink_max_bpp =
+		intel_dp_dsc_max_sink_compressed_bppx16(intel_dp,
+							pipe_config, pipe_bpp / 3) >> 4;
+
+	dsc_max_bpp = dsc_sink_max_bpp ? min(dsc_sink_max_bpp, dsc_src_max_bpp) : dsc_src_max_bpp;
+
+	/* Compressed BPP should be less than the Input DSC bpp */
+	dsc_max_bpp = min(dsc_max_bpp, pipe_bpp - 1);
+
+	for (compressed_bpp = dsc_max_bpp;
+	     compressed_bpp >= dsc_min_bpp;
+	     compressed_bpp--) {
+		ret = dsc_compute_link_config(intel_dp,
+					      pipe_config,
+					      limits,
+					      pipe_bpp,
+					      compressed_bpp,
+					      timeslots);
+		if (ret == 0) {
+			pipe_config->dsc.compressed_bpp = compressed_bpp;
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int intel_dp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
+					 struct intel_crtc_state *pipe_config,
+					 struct drm_connector_state *conn_state,
+					 struct link_config_limits *limits,
+					 int timeslots)
+{
+	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+	u8 dsc_bpc[3] = {0};
+	u8 dsc_max_bpc, dsc_max_bpp;
+	u8 dsc_min_bpc, dsc_min_bpp;
+	u8 max_req_bpc = conn_state->max_requested_bpc;
+	int i, bpp, ret;
+	int num_bpc = drm_dp_dsc_sink_supported_input_bpcs(intel_dp->dsc_dpcd,
+							   dsc_bpc);
+
+	/* Max DSC Input BPC for ICL is 10 and for TGL+ is 12 */
+	if (DISPLAY_VER(i915) >= 12)
+		dsc_max_bpc = min_t(u8, 12, max_req_bpc);
+	else
+		dsc_max_bpc = min_t(u8, 10, max_req_bpc);
+
+	dsc_max_bpp = min(dsc_max_bpc * 3, limits->max_bpp);
+
+	/* Min DSC Input BPC for ICL+ is 8 */
+	dsc_min_bpc = 8;
+	dsc_min_bpp = max(dsc_min_bpc * 3, limits->min_bpp);
+
+	/*
+	 * Get the maximum DSC bpc that will be supported by any valid
+	 * link configuration and compressed bpp.
+	 */
+	for (i = 0; i < num_bpc; i++) {
+		bpp = dsc_bpc[i] * 3;
+
+		if (bpp < dsc_min_bpp)
+			break;
+
+		if (bpp > dsc_max_bpp)
+			continue;
+
+		ret = dsc_compute_compressed_bpp(intel_dp, pipe_config,
+						 limits, bpp, timeslots);
+		if (ret == 0) {
+			pipe_config->pipe_bpp = bpp;
+
+			return 0;
+		}
+	}
+
+	return -EINVAL;
+}
+
 static
 bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
 {
@@ -1649,6 +1852,31 @@  bool is_dsc_pipe_bpp_sufficient(int pipe_bpp)
 	return (pipe_bpp < 8 * 3);
 }
 
+static int intel_edp_dsc_compute_pipe_bpp(struct intel_dp *intel_dp,
+					  struct intel_crtc_state *pipe_config,
+					  struct drm_connector_state *conn_state,
+					  struct link_config_limits *limits)
+{
+	/* For eDP use max bpp that can be supported with DSC. */
+	int pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
+						    conn_state->max_requested_bpc);
+	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+
+	if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
+		drm_dbg_kms(&i915->drm,
+			    "No DSC support for less than 8bpc\n");
+		return -EINVAL;
+	}
+	pipe_config->pipe_bpp = pipe_bpp;
+	pipe_config->port_clock = limits->max_rate;
+	pipe_config->lane_count = limits->max_lane_count;
+	pipe_config->dsc.compressed_bpp =
+		min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
+		      pipe_config->pipe_bpp);
+
+	return 0;
+}
+
 int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 				struct intel_crtc_state *pipe_config,
 				struct drm_connector_state *conn_state,
@@ -1671,6 +1899,12 @@  int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 	if (!intel_dp_dsc_supports_format(intel_dp, pipe_config->output_format))
 		return -EINVAL;
 
+	/*
+	 * compute pipe bpp is set to false for DP MST DSC case
+	 * and compressed_bpp is calculated same time once
+	 * vpci timeslots are allocated, because overall bpp
+	 * calculation procedure is bit different for MST case.
+	 */
 	if (compute_pipe_bpp) {
 		int pipe_bpp;
 		int forced_bpp = intel_dp->force_dsc_bpc * 3;
@@ -1683,31 +1917,25 @@  int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 				 forced_bpp && !is_dsc_pipe_bpp_sufficient(forced_bpp),
 				 "Cannot force dsc bpc:%d, due to dsc bpc limits\n",
 				 intel_dp->force_dsc_bpc);
-
-			pipe_bpp = intel_dp_dsc_compute_max_bpp(intel_dp,
-								conn_state->max_requested_bpc);
-
-			if (!is_dsc_pipe_bpp_sufficient(pipe_bpp)) {
-				drm_dbg_kms(&dev_priv->drm, "No DSC support for less than 8bpc\n");
-				return -EINVAL;
+			if (intel_dp_is_edp(intel_dp))
+				ret = intel_edp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
+								     conn_state, limits);
+			else
+				ret = intel_dp_dsc_compute_pipe_bpp(intel_dp, pipe_config,
+								    conn_state, limits, timeslots);
+			if (ret) {
+				drm_dbg_kms(&dev_priv->drm,
+					    "No Valid pipe bpp for given mode ret = %d\n", ret);
+				return ret;
 			}
 		}
-
-		pipe_config->pipe_bpp = pipe_bpp;
 	}
 
-	/*
-	 * For now enable DSC for max link rate, max lane count.
-	 * Optimize this later for the minimum possible link rate/lane count
-	 * with DSC enabled for the requested mode.
-	 */
 	pipe_config->port_clock = limits->max_rate;
 	pipe_config->lane_count = limits->max_lane_count;
 
+	/* Calculate Slice count */
 	if (intel_dp_is_edp(intel_dp)) {
-		pipe_config->dsc.compressed_bpp =
-			min_t(u16, drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4,
-			      pipe_config->pipe_bpp);
 		pipe_config->dsc.slice_count =
 			drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
 							true);
@@ -1717,37 +1945,8 @@  int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 			return -EINVAL;
 		}
 	} else {
-		u16 dsc_max_compressed_bpp = 0;
 		u8 dsc_dp_slice_count;
 
-		if (compute_pipe_bpp) {
-			dsc_max_compressed_bpp =
-				intel_dp_dsc_get_max_compressed_bpp(dev_priv,
-								    pipe_config->port_clock,
-								    pipe_config->lane_count,
-								    adjusted_mode->crtc_clock,
-								    adjusted_mode->crtc_hdisplay,
-								    pipe_config->bigjoiner_pipes,
-								    pipe_config->output_format,
-								    pipe_config->pipe_bpp,
-								    timeslots);
-			/*
-			 * According to DSC 1.2a Section 4.1.1 Table 4.1 the maximum
-			 * supported PPS value can be 63.9375 and with the further
-			 * mention that bpp should be programmed double the target bpp
-			 * restricting our target bpp to be 31.9375 at max
-			 */
-			if (pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
-				dsc_max_compressed_bpp = min_t(u16,
-							       dsc_max_compressed_bpp,
-							       31);
-
-			if (!dsc_max_compressed_bpp) {
-				drm_dbg_kms(&dev_priv->drm,
-					    "Compressed BPP not supported\n");
-				return -EINVAL;
-			}
-		}
 		dsc_dp_slice_count =
 			intel_dp_dsc_get_slice_count(intel_dp,
 						     adjusted_mode->crtc_clock,
@@ -1759,20 +1958,6 @@  int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 			return -EINVAL;
 		}
 
-		/*
-		 * compute pipe bpp is set to false for DP MST DSC case
-		 * and compressed_bpp is calculated same time once
-		 * vpci timeslots are allocated, because overall bpp
-		 * calculation procedure is bit different for MST case.
-		 */
-		if (compute_pipe_bpp) {
-			u16 output_bpp = intel_dp_output_bpp(pipe_config->output_format,
-							     pipe_config->pipe_bpp);
-
-			pipe_config->dsc.compressed_bpp = min_t(u16,
-								dsc_max_compressed_bpp,
-								output_bpp);
-		}
 		pipe_config->dsc.slice_count = dsc_dp_slice_count;
 	}
 	/*