[v6,13/28] drm/i915/dp: Compute DSC pipe config in atomic check

Commit Message

Navare, Manasi Oct. 24, 2018, 10:28 p.m. UTC

DSC params like the enable, compressed bpp, slice count and
dsc_split are added to the intel_crtc_state. These parameters
are set based on the requested mode and available link parameters
during the pipe configuration in atomic check phase.
These values are then later used to populate the remaining DSC
and RC parameters before enbaling DSC in atomic commit.

v9:
* Rebase on top of drm-tip that now uses fast_narrow config
for edp (Manasi)
v8:
* Check for DSC bpc not 0 (manasi)

v7:
* Fix indentation in compute_m_n (Manasi)

v6 (From Gaurav):
* Remove function call of intel_dp_compute_dsc_params() and
invoke intel_dp_compute_dsc_params() in the patch where
it is defined to fix compilation warning (Gaurav)

v5:
Add drm_dsc_cfg in intel_crtc_state (Manasi)

v4:
* Rebase on refactoring of intel_dp_compute_config on tip (Manasi)
* Add a comment why we need to check PSR while enabling DSC (Gaurav)

v3:
* Check PPR > max_cdclock to use 2 VDSC instances (Ville)

v2:
* Add if-else for eDP/DP (Gaurav)

Cc: Jani Nikula <[e-mail]>
Cc: Ville Syrjala <[e-mail]>
Cc: Anusha Srivatsa <[e-mail]>
Cc: Gaurav K Singh <[e-mail]>
Signed-off-by: Manasi Navare <[e-mail]>
Reviewed-by: Anusha Srivatsa <[e-mail]>
Acked-by: Jani Nikula <[e-mail]>
---
 drivers/gpu/drm/i915/intel_display.c |  20 +++-
 drivers/gpu/drm/i915/intel_display.h |   3 +-
 drivers/gpu/drm/i915/intel_dp.c      | 170 ++++++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_dp_mst.c  |   2 +-
 4 files changed, 155 insertions(+), 40 deletions(-)

Comments

Ville Syrjälä Oct. 29, 2018, 8:30 p.m. UTC | #1

On Wed, Oct 24, 2018 at 03:28:25PM -0700, Manasi Navare wrote:
> DSC params like the enable, compressed bpp, slice count and
> dsc_split are added to the intel_crtc_state. These parameters
> are set based on the requested mode and available link parameters
> during the pipe configuration in atomic check phase.
> These values are then later used to populate the remaining DSC
> and RC parameters before enbaling DSC in atomic commit.
> 
> v9:
> * Rebase on top of drm-tip that now uses fast_narrow config
> for edp (Manasi)
> v8:
> * Check for DSC bpc not 0 (manasi)
> 
> v7:
> * Fix indentation in compute_m_n (Manasi)
> 
> v6 (From Gaurav):
> * Remove function call of intel_dp_compute_dsc_params() and
> invoke intel_dp_compute_dsc_params() in the patch where
> it is defined to fix compilation warning (Gaurav)
> 
> v5:
> Add drm_dsc_cfg in intel_crtc_state (Manasi)
> 
> v4:
> * Rebase on refactoring of intel_dp_compute_config on tip (Manasi)
> * Add a comment why we need to check PSR while enabling DSC (Gaurav)
> 
> v3:
> * Check PPR > max_cdclock to use 2 VDSC instances (Ville)
> 
> v2:
> * Add if-else for eDP/DP (Gaurav)
> 
> Cc: Jani Nikula <[e-mail]>
> Cc: Ville Syrjala <[e-mail]>
> Cc: Anusha Srivatsa <[e-mail]>
> Cc: Gaurav K Singh <[e-mail]>
> Signed-off-by: Manasi Navare <[e-mail]>
> Reviewed-by: Anusha Srivatsa <[e-mail]>
> Acked-by: Jani Nikula <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  20 +++-
>  drivers/gpu/drm/i915/intel_display.h |   3 +-
>  drivers/gpu/drm/i915/intel_dp.c      | 170 ++++++++++++++++++++++-----
>  drivers/gpu/drm/i915/intel_dp_mst.c  |   2 +-
>  4 files changed, 155 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fe045abb6472..18737bd82b68 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6434,7 +6434,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
>  
>  	pipe_config->fdi_lanes = lane;
>  
> -	intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
> +	intel_link_compute_m_n(pipe_config->pipe_bpp, 0, lane, fdi_dotclock,
>  			       link_bw, &pipe_config->fdi_m_n, false);
>  
>  	ret = ironlake_check_fdi_lanes(dev, intel_crtc->pipe, pipe_config);
> @@ -6671,17 +6671,25 @@ static void compute_m_n(unsigned int m, unsigned int n,
>  }
>  
>  void
> -intel_link_compute_m_n(int bits_per_pixel, int nlanes,
> +intel_link_compute_m_n(int bits_per_pixel, uint16_t compressed_bpp,
> +		       int nlanes,
>  		       int pixel_clock, int link_clock,
>  		       struct intel_link_m_n *m_n,
>  		       bool constant_n)
>  {
>  	m_n->tu = 64;
>  
> -	compute_m_n(bits_per_pixel * pixel_clock,
> -		    link_clock * nlanes * 8,
> -		    &m_n->gmch_m, &m_n->gmch_n,
> -		    constant_n);
> +	/* For DSC, Data M/N calculation uses compressed BPP */
> +	if (compressed_bpp)
> +		compute_m_n(compressed_bpp * pixel_clock,
> +			    link_clock * nlanes * 8,
> +			    &m_n->gmch_m, &m_n->gmch_n,
> +			    constant_n);
> +	else
> +		compute_m_n(bits_per_pixel * pixel_clock,
> +			    link_clock * nlanes * 8,
> +			    &m_n->gmch_m, &m_n->gmch_n,
> +			    constant_n);
>  
>  	compute_m_n(pixel_clock, link_clock,
>  		    &m_n->link_m, &m_n->link_n,
> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> index 5d50decbcbb5..b0b23e1e9392 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -407,7 +407,8 @@ struct intel_link_m_n {
>  	     (__i)++) \
>  		for_each_if(plane)
>  
> -void intel_link_compute_m_n(int bpp, int nlanes,
> +void intel_link_compute_m_n(int bpp, uint16_t compressed_bpp,
> +			    int nlanes,
>  			    int pixel_clock, int link_clock,
>  			    struct intel_link_m_n *m_n,
>  			    bool constant_n);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 6f66a38ba0b2..a88f9371dd32 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -47,6 +47,8 @@
>  
>  /* DP DSC small joiner has 2 FIFOs each of 640 x 6 bytes */
>  #define DP_DSC_MAX_SMALL_JOINER_RAM_BUFFER	61440
> +#define DP_DSC_MIN_SUPPORTED_BPC		8
> +#define DP_DSC_MAX_SUPPORTED_BPC		10
>  
>  /* DP DSC throughput values used for slice count calculations KPixels/s */
>  #define DP_DSC_PEAK_PIXEL_RATE			2720000
> @@ -1924,6 +1926,16 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  		}
>  	}
>  
> +	/* If DSC is supported, use the max value reported by panel */
> +	if (INTEL_GEN(dev_priv) >= 10 &&
> +	    drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) {
> +		bpc = min_t(u8,
> +			    drm_dp_dsc_sink_max_color_depth(intel_dp->dsc_dpcd),
> +			    DP_DSC_MAX_SUPPORTED_BPC);
> +		if (bpc)
> +			bpp = 3 * bpc;

This seems like it should be 'bpp = min(bpp, 3*bpc)'. 
Otherwise we may bump the bpp above a limit we already established earlier.

> +	}
> +
>  	return bpp;
>  }
>  
> @@ -1984,14 +1996,11 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
>  				link_clock = intel_dp->common_rates[clock];
>  				link_avail = intel_dp_max_data_rate(link_clock,
>  								    lane_count);
> -
> -				if (mode_rate <= link_avail) {
> -					pipe_config->lane_count = lane_count;
> -					pipe_config->pipe_bpp = bpp;
> -					pipe_config->port_clock = link_clock;
> -
> +				pipe_config->lane_count = lane_count;
> +				pipe_config->pipe_bpp = bpp;
> +				pipe_config->port_clock = link_clock;
> +				if (mode_rate <= link_avail)
>  					return true;

Why do we need to assign these if we don't accept the configuration?

> -				}
>  			}
>  		}
>  	}
> @@ -2020,14 +2029,11 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
>  				link_clock = intel_dp->common_rates[clock];
>  				link_avail = intel_dp_max_data_rate(link_clock,
>  								    lane_count);
> -
> -				if (mode_rate <= link_avail) {
> -					pipe_config->lane_count = lane_count;
> -					pipe_config->pipe_bpp = bpp;
> -					pipe_config->port_clock = link_clock;
> -
> +				pipe_config->lane_count = lane_count;
> +				pipe_config->pipe_bpp = bpp;
> +				pipe_config->port_clock = link_clock;
> +				if (mode_rate <= link_avail)
>  					return true;
> -				}
>  			}
>  		}
>  	}
> @@ -2035,14 +2041,88 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
>  	return false;
>  }
>  
> +static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> +					struct intel_crtc_state *pipe_config,
> +					struct link_config_limits *limits)
> +{
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> +	enum pipe pipe = to_intel_crtc(pipe_config->base.crtc)->pipe;
> +	u16 dsc_max_output_bpp = 0;
> +	u8 dsc_dp_slice_count = 0;
> +
> +	if (INTEL_GEN(dev_priv) < 10 ||
> +	    !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))
> +		return false;
> +
> +	/* DP DSC only supported on Pipe B and C */
> +	if (pipe == PIPE_A && !intel_dp_is_edp(intel_dp))
> +		return false;

Do we mean 'transcoder == EDP || transcoder == B || transcoder == C' ?
Or maybe 'transcoder != A' for short. I guess this will need to adjusted
for the next platform anyway so making the assumption that transcoder A
is the only one that can't do DSC is fine.

This whole thing seems like a good helper function.
intel_dp_source_supports_dsc() or something like that. And then we
could have intel_dp_supports_dsc() which just calls that +
drm_dp_sink_supports_dsc().

intel_dp_compute_bpp() should use this at least, and probably a few
other places as well.

> +
> +	/* DSC not supported for DSC sink BPC < 8 */
> +	if (limits->max_bpp < 3 * DP_DSC_MIN_SUPPORTED_BPC) {
> +		DRM_DEBUG_KMS("No DSC support for less than 8bpc\n");
> +		return false;
> +	}
> +
> +	if (intel_dp_is_edp(intel_dp)) {
> +		pipe_config->dsc_params.compressed_bpp =
> +			drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4;
> +		pipe_config->dsc_params.slice_count =
> +			drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
> +							true);
> +	} else {
> +		dsc_max_output_bpp =
> +			intel_dp_dsc_get_output_bpp(pipe_config->port_clock,
> +						    pipe_config->lane_count,
> +						    adjusted_mode->crtc_clock,
> +						    adjusted_mode->crtc_hdisplay);
> +		dsc_dp_slice_count =
> +			intel_dp_dsc_get_slice_count(intel_dp,
> +						     adjusted_mode->crtc_clock,
> +						     adjusted_mode->crtc_hdisplay);
> +		if (!(dsc_max_output_bpp && dsc_dp_slice_count)) {
> +			DRM_DEBUG_KMS("Compressed BPP/Slice Count not supported\n");
> +			return false;
> +		}
> +		pipe_config->dsc_params.compressed_bpp = dsc_max_output_bpp >> 4;
> +		pipe_config->dsc_params.slice_count = dsc_dp_slice_count;
> +	}
> +	/*
> +	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
> +	 * is greater than the maximum Cdclock and if slice count is even
> +	 * then we need to use 2 VDSC instances.
> +	 */
> +	pipe_config->dsc_params.dsc_split = false;
> +	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq) {
> +		if (pipe_config->dsc_params.slice_count > 1) {
> +			pipe_config->dsc_params.dsc_split = true;
> +		} else {
> +			DRM_DEBUG_KMS("Cannot split stream to use 2 VDSC instances\n");
> +			return false;
> +		}
> +	}
> +	pipe_config->dsc_params.compression_enable = true;
> +	DRM_DEBUG_KMS("DP DSC computed with Input Bpp = %d "
> +		      "Compressed Bpp = %d Slice Count = %d\n",
> +		      pipe_config->pipe_bpp,
> +		      pipe_config->dsc_params.compressed_bpp,
> +		      pipe_config->dsc_params.slice_count);
> +
> +	return true;
> +}
> +
>  static bool
>  intel_dp_compute_link_config(struct intel_encoder *encoder,
>  			     struct intel_crtc_state *pipe_config)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>  	struct link_config_limits limits;
>  	int common_len;
> +	bool ret = false;
>  
>  	common_len = intel_dp_common_len_rate_limit(intel_dp,
>  						    intel_dp->max_link_rate);
> @@ -2056,7 +2136,9 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  	limits.min_lane_count = 1;
>  	limits.max_lane_count = intel_dp_max_lane_count(intel_dp);
>  
> -	limits.min_bpp = 6 * 3;
> +	limits.min_bpp = (INTEL_GEN(dev_priv) >= 10 &&
> +			  drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) ?
> +		DP_DSC_MIN_SUPPORTED_BPC * 3 : 6 * 3;
>  	limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);

Hmm. This now assumes that we will in fact use DSC. I guess we can only
make that assumption when we've determined that DSC is supported and the
max_bpp also allows DSC.

So something like:

max_bpp = intel_dp_compute_bpp();
if (supports_dsc() && max_bpp >= DP_DSC_MIN_SUPPORTED_BPC*3)
	min_bpp = DP_DSC_MIN_SUPPORTED_BPC * 3;
else
	min_bpp = 6 * 3

>  
>  	if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) {
> @@ -2081,7 +2163,7 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  		      intel_dp->common_rates[limits.max_clock],
>  		      limits.max_bpp, adjusted_mode->crtc_clock);
>  
> -	if (intel_dp_is_edp(intel_dp)) {
> +	if (intel_dp_is_edp(intel_dp))
>  		/*
>  		 * Optimize for fast and narrow. eDP 1.3 section 3.3 and eDP 1.4
>  		 * section A.1: "It is recommended that the minimum number of
> @@ -2091,26 +2173,48 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  		 * Note that we use the max clock and lane count for eDP 1.3 and
>  		 * earlier, and fast vs. wide is irrelevant.
>  		 */
> -		if (!intel_dp_compute_link_config_fast(intel_dp, pipe_config,
> -						       &limits))
> -			return false;
> -	} else {
> +		ret = intel_dp_compute_link_config_fast(intel_dp, pipe_config,
> +							&limits);
> +	else
>  		/* Optimize for slow and wide. */
> -		if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config,
> -						       &limits))
> +		ret = intel_dp_compute_link_config_wide(intel_dp, pipe_config,
> +							&limits);
> +
> +	/* enable compression if the mode doesn't fit available BW */
> +	if (!ret) {
> +		DRM_DEBUG_KMS("DP required Link rate %i does not fit available %i\n",
> +			      intel_dp_link_required(adjusted_mode->crtc_clock,
> +						     pipe_config->pipe_bpp),
> +			      intel_dp_max_data_rate(pipe_config->port_clock,
> +						     pipe_config->lane_count));
> +
> +		if (!intel_dp_dsc_compute_config(intel_dp, pipe_config,
> +						 &limits))
>  			return false;
>  	}
>  
> -	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> -		      pipe_config->lane_count, pipe_config->port_clock,
> -		      pipe_config->pipe_bpp);
> +	if (pipe_config->dsc_params.compression_enable) {
> +		DRM_DEBUG_KMS("DP lane count %d clock %d Input bpp %d Compressed bpp %d\n",
> +			      pipe_config->lane_count, pipe_config->port_clock,
> +			      pipe_config->pipe_bpp,
> +			      pipe_config->dsc_params.compressed_bpp);
>  
> -	DRM_DEBUG_KMS("DP link rate required %i available %i\n",
> -		      intel_dp_link_required(adjusted_mode->crtc_clock,
> -					     pipe_config->pipe_bpp),
> -		      intel_dp_max_data_rate(pipe_config->port_clock,
> -					     pipe_config->lane_count));
> +		DRM_DEBUG_KMS("DP link rate required %i available %i\n",
> +			      intel_dp_link_required(adjusted_mode->crtc_clock,
> +						     pipe_config->dsc_params.compressed_bpp),
> +			      intel_dp_max_data_rate(pipe_config->port_clock,
> +						     pipe_config->lane_count));
> +	} else {
> +		DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> +			      pipe_config->lane_count, pipe_config->port_clock,
> +			      pipe_config->pipe_bpp);
>  
> +		DRM_DEBUG_KMS("DP link rate required %i available %i\n",
> +			      intel_dp_link_required(adjusted_mode->crtc_clock,
> +						     pipe_config->pipe_bpp),
> +			      intel_dp_max_data_rate(pipe_config->port_clock,
> +						     pipe_config->lane_count));
> +	}
>  	return true;
>  }
>  
> @@ -2194,7 +2298,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  			intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_LIMITED;
>  	}
>  
> -	intel_link_compute_m_n(pipe_config->pipe_bpp, pipe_config->lane_count,
> +	intel_link_compute_m_n(pipe_config->pipe_bpp,
> +			       pipe_config->dsc_params.compressed_bpp,
> +			       pipe_config->lane_count,
>  			       adjusted_mode->crtc_clock,
>  			       pipe_config->port_clock,
>  			       &pipe_config->dp_m_n,
> @@ -2203,7 +2309,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	if (intel_connector->panel.downclock_mode != NULL &&
>  		dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) {
>  			pipe_config->has_drrs = true;
> -			intel_link_compute_m_n(pipe_config->pipe_bpp,
> +			intel_link_compute_m_n(pipe_config->pipe_bpp, 0,
>  					       pipe_config->lane_count,
>  					       intel_connector->panel.downclock_mode->clock,
>  					       pipe_config->port_clock,
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 8b71d64ebd9d..e02caedd443c 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -90,7 +90,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
>  		}
>  	}
>  
> -	intel_link_compute_m_n(bpp, lane_count,
> +	intel_link_compute_m_n(bpp, 0, lane_count,
>  			       adjusted_mode->crtc_clock,
>  			       pipe_config->port_clock,
>  			       &pipe_config->dp_m_n,
> -- 
> 2.18.0

Ville Syrjälä Oct. 29, 2018, 8:34 p.m. UTC | #2

On Mon, Oct 29, 2018 at 10:30:39PM +0200, Ville Syrjälä wrote:
> On Wed, Oct 24, 2018 at 03:28:25PM -0700, Manasi Navare wrote:
> > DSC params like the enable, compressed bpp, slice count and
> > dsc_split are added to the intel_crtc_state. These parameters
> > are set based on the requested mode and available link parameters
> > during the pipe configuration in atomic check phase.
> > These values are then later used to populate the remaining DSC
> > and RC parameters before enbaling DSC in atomic commit.
> > 
> > v9:
> > * Rebase on top of drm-tip that now uses fast_narrow config
> > for edp (Manasi)
> > v8:
> > * Check for DSC bpc not 0 (manasi)
> > 
> > v7:
> > * Fix indentation in compute_m_n (Manasi)
> > 
> > v6 (From Gaurav):
> > * Remove function call of intel_dp_compute_dsc_params() and
> > invoke intel_dp_compute_dsc_params() in the patch where
> > it is defined to fix compilation warning (Gaurav)
> > 
> > v5:
> > Add drm_dsc_cfg in intel_crtc_state (Manasi)
> > 
> > v4:
> > * Rebase on refactoring of intel_dp_compute_config on tip (Manasi)
> > * Add a comment why we need to check PSR while enabling DSC (Gaurav)
> > 
> > v3:
> > * Check PPR > max_cdclock to use 2 VDSC instances (Ville)
> > 
> > v2:
> > * Add if-else for eDP/DP (Gaurav)
> > 
> > Cc: Jani Nikula <[e-mail]>
> > Cc: Ville Syrjala <[e-mail]>
> > Cc: Anusha Srivatsa <[e-mail]>
> > Cc: Gaurav K Singh <[e-mail]>
> > Signed-off-by: Manasi Navare <[e-mail]>
> > Reviewed-by: Anusha Srivatsa <[e-mail]>
> > Acked-by: Jani Nikula <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |  20 +++-
> >  drivers/gpu/drm/i915/intel_display.h |   3 +-
> >  drivers/gpu/drm/i915/intel_dp.c      | 170 ++++++++++++++++++++++-----
> >  drivers/gpu/drm/i915/intel_dp_mst.c  |   2 +-
> >  4 files changed, 155 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index fe045abb6472..18737bd82b68 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6434,7 +6434,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> >  
> >  	pipe_config->fdi_lanes = lane;
> >  
> > -	intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
> > +	intel_link_compute_m_n(pipe_config->pipe_bpp, 0, lane, fdi_dotclock,
> >  			       link_bw, &pipe_config->fdi_m_n, false);
> >  
> >  	ret = ironlake_check_fdi_lanes(dev, intel_crtc->pipe, pipe_config);
> > @@ -6671,17 +6671,25 @@ static void compute_m_n(unsigned int m, unsigned int n,
> >  }
> >  
> >  void
> > -intel_link_compute_m_n(int bits_per_pixel, int nlanes,
> > +intel_link_compute_m_n(int bits_per_pixel, uint16_t compressed_bpp,
> > +		       int nlanes,
> >  		       int pixel_clock, int link_clock,
> >  		       struct intel_link_m_n *m_n,
> >  		       bool constant_n)
> >  {
> >  	m_n->tu = 64;
> >  
> > -	compute_m_n(bits_per_pixel * pixel_clock,
> > -		    link_clock * nlanes * 8,
> > -		    &m_n->gmch_m, &m_n->gmch_n,
> > -		    constant_n);
> > +	/* For DSC, Data M/N calculation uses compressed BPP */
> > +	if (compressed_bpp)
> > +		compute_m_n(compressed_bpp * pixel_clock,
> > +			    link_clock * nlanes * 8,
> > +			    &m_n->gmch_m, &m_n->gmch_n,
> > +			    constant_n);
> > +	else
> > +		compute_m_n(bits_per_pixel * pixel_clock,
> > +			    link_clock * nlanes * 8,
> > +			    &m_n->gmch_m, &m_n->gmch_n,
> > +			    constant_n);
> >  
> >  	compute_m_n(pixel_clock, link_clock,
> >  		    &m_n->link_m, &m_n->link_n,
> > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > index 5d50decbcbb5..b0b23e1e9392 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -407,7 +407,8 @@ struct intel_link_m_n {
> >  	     (__i)++) \
> >  		for_each_if(plane)
> >  
> > -void intel_link_compute_m_n(int bpp, int nlanes,
> > +void intel_link_compute_m_n(int bpp, uint16_t compressed_bpp,
> > +			    int nlanes,
> >  			    int pixel_clock, int link_clock,
> >  			    struct intel_link_m_n *m_n,
> >  			    bool constant_n);
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 6f66a38ba0b2..a88f9371dd32 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -47,6 +47,8 @@
> >  
> >  /* DP DSC small joiner has 2 FIFOs each of 640 x 6 bytes */
> >  #define DP_DSC_MAX_SMALL_JOINER_RAM_BUFFER	61440
> > +#define DP_DSC_MIN_SUPPORTED_BPC		8
> > +#define DP_DSC_MAX_SUPPORTED_BPC		10
> >  
> >  /* DP DSC throughput values used for slice count calculations KPixels/s */
> >  #define DP_DSC_PEAK_PIXEL_RATE			2720000
> > @@ -1924,6 +1926,16 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
> >  		}
> >  	}
> >  
> > +	/* If DSC is supported, use the max value reported by panel */
> > +	if (INTEL_GEN(dev_priv) >= 10 &&
> > +	    drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) {
> > +		bpc = min_t(u8,
> > +			    drm_dp_dsc_sink_max_color_depth(intel_dp->dsc_dpcd),
> > +			    DP_DSC_MAX_SUPPORTED_BPC);
> > +		if (bpc)
> > +			bpp = 3 * bpc;
> 
> This seems like it should be 'bpp = min(bpp, 3*bpc)'. 
> Otherwise we may bump the bpp above a limit we already established earlier.
> 
> > +	}
> > +
> >  	return bpp;
> >  }
> >  
> > @@ -1984,14 +1996,11 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
> >  				link_clock = intel_dp->common_rates[clock];
> >  				link_avail = intel_dp_max_data_rate(link_clock,
> >  								    lane_count);
> > -
> > -				if (mode_rate <= link_avail) {
> > -					pipe_config->lane_count = lane_count;
> > -					pipe_config->pipe_bpp = bpp;
> > -					pipe_config->port_clock = link_clock;
> > -
> > +				pipe_config->lane_count = lane_count;
> > +				pipe_config->pipe_bpp = bpp;
> > +				pipe_config->port_clock = link_clock;
> > +				if (mode_rate <= link_avail)
> >  					return true;
> 
> Why do we need to assign these if we don't accept the configuration?
> 
> > -				}
> >  			}
> >  		}
> >  	}
> > @@ -2020,14 +2029,11 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
> >  				link_clock = intel_dp->common_rates[clock];
> >  				link_avail = intel_dp_max_data_rate(link_clock,
> >  								    lane_count);
> > -
> > -				if (mode_rate <= link_avail) {
> > -					pipe_config->lane_count = lane_count;
> > -					pipe_config->pipe_bpp = bpp;
> > -					pipe_config->port_clock = link_clock;
> > -
> > +				pipe_config->lane_count = lane_count;
> > +				pipe_config->pipe_bpp = bpp;
> > +				pipe_config->port_clock = link_clock;
> > +				if (mode_rate <= link_avail)
> >  					return true;
> > -				}
> >  			}
> >  		}
> >  	}
> > @@ -2035,14 +2041,88 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
> >  	return false;
> >  }
> >  
> > +static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > +					struct intel_crtc_state *pipe_config,
> > +					struct link_config_limits *limits)
> > +{
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> > +	enum pipe pipe = to_intel_crtc(pipe_config->base.crtc)->pipe;
> > +	u16 dsc_max_output_bpp = 0;
> > +	u8 dsc_dp_slice_count = 0;
> > +
> > +	if (INTEL_GEN(dev_priv) < 10 ||
> > +	    !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))
> > +		return false;
> > +
> > +	/* DP DSC only supported on Pipe B and C */
> > +	if (pipe == PIPE_A && !intel_dp_is_edp(intel_dp))
> > +		return false;
> 
> Do we mean 'transcoder == EDP || transcoder == B || transcoder == C' ?
> Or maybe 'transcoder != A' for short. I guess this will need to adjusted
> for the next platform anyway so making the assumption that transcoder A
> is the only one that can't do DSC is fine.
> 
> This whole thing seems like a good helper function.
> intel_dp_source_supports_dsc() or something like that. And then we
> could have intel_dp_supports_dsc() which just calls that +
> drm_dp_sink_supports_dsc().

Another confusion about these checks is glk. Some other places seem
to indicate that glk has DSC, but then code like this here doesn't 
accept glk. What's up with that?

Navare, Manasi Oct. 29, 2018, 9:42 p.m. UTC | #3

On Mon, Oct 29, 2018 at 10:30:39PM +0200, Ville Syrjälä wrote:
> On Wed, Oct 24, 2018 at 03:28:25PM -0700, Manasi Navare wrote:
> > DSC params like the enable, compressed bpp, slice count and
> > dsc_split are added to the intel_crtc_state. These parameters
> > are set based on the requested mode and available link parameters
> > during the pipe configuration in atomic check phase.
> > These values are then later used to populate the remaining DSC
> > and RC parameters before enbaling DSC in atomic commit.
> > 
> > v9:
> > * Rebase on top of drm-tip that now uses fast_narrow config
> > for edp (Manasi)
> > v8:
> > * Check for DSC bpc not 0 (manasi)
> > 
> > v7:
> > * Fix indentation in compute_m_n (Manasi)
> > 
> > v6 (From Gaurav):
> > * Remove function call of intel_dp_compute_dsc_params() and
> > invoke intel_dp_compute_dsc_params() in the patch where
> > it is defined to fix compilation warning (Gaurav)
> > 
> > v5:
> > Add drm_dsc_cfg in intel_crtc_state (Manasi)
> > 
> > v4:
> > * Rebase on refactoring of intel_dp_compute_config on tip (Manasi)
> > * Add a comment why we need to check PSR while enabling DSC (Gaurav)
> > 
> > v3:
> > * Check PPR > max_cdclock to use 2 VDSC instances (Ville)
> > 
> > v2:
> > * Add if-else for eDP/DP (Gaurav)
> > 
> > Cc: Jani Nikula <[e-mail]>
> > Cc: Ville Syrjala <[e-mail]>
> > Cc: Anusha Srivatsa <[e-mail]>
> > Cc: Gaurav K Singh <[e-mail]>
> > Signed-off-by: Manasi Navare <[e-mail]>
> > Reviewed-by: Anusha Srivatsa <[e-mail]>
> > Acked-by: Jani Nikula <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |  20 +++-
> >  drivers/gpu/drm/i915/intel_display.h |   3 +-
> >  drivers/gpu/drm/i915/intel_dp.c      | 170 ++++++++++++++++++++++-----
> >  drivers/gpu/drm/i915/intel_dp_mst.c  |   2 +-
> >  4 files changed, 155 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index fe045abb6472..18737bd82b68 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6434,7 +6434,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> >  
> >  	pipe_config->fdi_lanes = lane;
> >  
> > -	intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
> > +	intel_link_compute_m_n(pipe_config->pipe_bpp, 0, lane, fdi_dotclock,
> >  			       link_bw, &pipe_config->fdi_m_n, false);
> >  
> >  	ret = ironlake_check_fdi_lanes(dev, intel_crtc->pipe, pipe_config);
> > @@ -6671,17 +6671,25 @@ static void compute_m_n(unsigned int m, unsigned int n,
> >  }
> >  
> >  void
> > -intel_link_compute_m_n(int bits_per_pixel, int nlanes,
> > +intel_link_compute_m_n(int bits_per_pixel, uint16_t compressed_bpp,
> > +		       int nlanes,
> >  		       int pixel_clock, int link_clock,
> >  		       struct intel_link_m_n *m_n,
> >  		       bool constant_n)
> >  {
> >  	m_n->tu = 64;
> >  
> > -	compute_m_n(bits_per_pixel * pixel_clock,
> > -		    link_clock * nlanes * 8,
> > -		    &m_n->gmch_m, &m_n->gmch_n,
> > -		    constant_n);
> > +	/* For DSC, Data M/N calculation uses compressed BPP */
> > +	if (compressed_bpp)
> > +		compute_m_n(compressed_bpp * pixel_clock,
> > +			    link_clock * nlanes * 8,
> > +			    &m_n->gmch_m, &m_n->gmch_n,
> > +			    constant_n);
> > +	else
> > +		compute_m_n(bits_per_pixel * pixel_clock,
> > +			    link_clock * nlanes * 8,
> > +			    &m_n->gmch_m, &m_n->gmch_n,
> > +			    constant_n);
> >  
> >  	compute_m_n(pixel_clock, link_clock,
> >  		    &m_n->link_m, &m_n->link_n,
> > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > index 5d50decbcbb5..b0b23e1e9392 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -407,7 +407,8 @@ struct intel_link_m_n {
> >  	     (__i)++) \
> >  		for_each_if(plane)
> >  
> > -void intel_link_compute_m_n(int bpp, int nlanes,
> > +void intel_link_compute_m_n(int bpp, uint16_t compressed_bpp,
> > +			    int nlanes,
> >  			    int pixel_clock, int link_clock,
> >  			    struct intel_link_m_n *m_n,
> >  			    bool constant_n);
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 6f66a38ba0b2..a88f9371dd32 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -47,6 +47,8 @@
> >  
> >  /* DP DSC small joiner has 2 FIFOs each of 640 x 6 bytes */
> >  #define DP_DSC_MAX_SMALL_JOINER_RAM_BUFFER	61440
> > +#define DP_DSC_MIN_SUPPORTED_BPC		8
> > +#define DP_DSC_MAX_SUPPORTED_BPC		10
> >  
> >  /* DP DSC throughput values used for slice count calculations KPixels/s */
> >  #define DP_DSC_PEAK_PIXEL_RATE			2720000
> > @@ -1924,6 +1926,16 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
> >  		}
> >  	}
> >  
> > +	/* If DSC is supported, use the max value reported by panel */
> > +	if (INTEL_GEN(dev_priv) >= 10 &&
> > +	    drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) {
> > +		bpc = min_t(u8,
> > +			    drm_dp_dsc_sink_max_color_depth(intel_dp->dsc_dpcd),
> > +			    DP_DSC_MAX_SUPPORTED_BPC);
> > +		if (bpc)
> > +			bpp = 3 * bpc;
> 
> This seems like it should be 'bpp = min(bpp, 3*bpc)'. 
> Otherwise we may bump the bpp above a limit we already established earlier.

Yes makes sense. Will change this.

Manasi

> 
> > +	}
> > +
> >  	return bpp;
> >  }
> >  
> > @@ -1984,14 +1996,11 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
> >  				link_clock = intel_dp->common_rates[clock];
> >  				link_avail = intel_dp_max_data_rate(link_clock,
> >  								    lane_count);
> > -
> > -				if (mode_rate <= link_avail) {
> > -					pipe_config->lane_count = lane_count;
> > -					pipe_config->pipe_bpp = bpp;
> > -					pipe_config->port_clock = link_clock;
> > -
> > +				pipe_config->lane_count = lane_count;
> > +				pipe_config->pipe_bpp = bpp;
> > +				pipe_config->port_clock = link_clock;
> > +				if (mode_rate <= link_avail)
> >  					return true;
> 
> Why do we need to assign these if we don't accept the configuration?
> 
> > -				}
> >  			}
> >  		}
> >  	}
> > @@ -2020,14 +2029,11 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
> >  				link_clock = intel_dp->common_rates[clock];
> >  				link_avail = intel_dp_max_data_rate(link_clock,
> >  								    lane_count);
> > -
> > -				if (mode_rate <= link_avail) {
> > -					pipe_config->lane_count = lane_count;
> > -					pipe_config->pipe_bpp = bpp;
> > -					pipe_config->port_clock = link_clock;
> > -
> > +				pipe_config->lane_count = lane_count;
> > +				pipe_config->pipe_bpp = bpp;
> > +				pipe_config->port_clock = link_clock;
> > +				if (mode_rate <= link_avail)
> >  					return true;
> > -				}
> >  			}
> >  		}
> >  	}
> > @@ -2035,14 +2041,88 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
> >  	return false;
> >  }
> >  
> > +static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > +					struct intel_crtc_state *pipe_config,
> > +					struct link_config_limits *limits)
> > +{
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> > +	enum pipe pipe = to_intel_crtc(pipe_config->base.crtc)->pipe;
> > +	u16 dsc_max_output_bpp = 0;
> > +	u8 dsc_dp_slice_count = 0;
> > +
> > +	if (INTEL_GEN(dev_priv) < 10 ||
> > +	    !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))
> > +		return false;
> > +
> > +	/* DP DSC only supported on Pipe B and C */
> > +	if (pipe == PIPE_A && !intel_dp_is_edp(intel_dp))
> > +		return false;
> 
> Do we mean 'transcoder == EDP || transcoder == B || transcoder == C' ?
> Or maybe 'transcoder != A' for short. I guess this will need to adjusted
> for the next platform anyway so making the assumption that transcoder A
> is the only one that can't do DSC is fine.
> 
> This whole thing seems like a good helper function.
> intel_dp_source_supports_dsc() or something like that. And then we
> could have intel_dp_supports_dsc() which just calls that +
> drm_dp_sink_supports_dsc().
> 
> intel_dp_compute_bpp() should use this at least, and probably a few
> other places as well.
> 
> > +
> > +	/* DSC not supported for DSC sink BPC < 8 */
> > +	if (limits->max_bpp < 3 * DP_DSC_MIN_SUPPORTED_BPC) {
> > +		DRM_DEBUG_KMS("No DSC support for less than 8bpc\n");
> > +		return false;
> > +	}
> > +
> > +	if (intel_dp_is_edp(intel_dp)) {
> > +		pipe_config->dsc_params.compressed_bpp =
> > +			drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4;
> > +		pipe_config->dsc_params.slice_count =
> > +			drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
> > +							true);
> > +	} else {
> > +		dsc_max_output_bpp =
> > +			intel_dp_dsc_get_output_bpp(pipe_config->port_clock,
> > +						    pipe_config->lane_count,
> > +						    adjusted_mode->crtc_clock,
> > +						    adjusted_mode->crtc_hdisplay);
> > +		dsc_dp_slice_count =
> > +			intel_dp_dsc_get_slice_count(intel_dp,
> > +						     adjusted_mode->crtc_clock,
> > +						     adjusted_mode->crtc_hdisplay);
> > +		if (!(dsc_max_output_bpp && dsc_dp_slice_count)) {
> > +			DRM_DEBUG_KMS("Compressed BPP/Slice Count not supported\n");
> > +			return false;
> > +		}
> > +		pipe_config->dsc_params.compressed_bpp = dsc_max_output_bpp >> 4;
> > +		pipe_config->dsc_params.slice_count = dsc_dp_slice_count;
> > +	}
> > +	/*
> > +	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
> > +	 * is greater than the maximum Cdclock and if slice count is even
> > +	 * then we need to use 2 VDSC instances.
> > +	 */
> > +	pipe_config->dsc_params.dsc_split = false;
> > +	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq) {
> > +		if (pipe_config->dsc_params.slice_count > 1) {
> > +			pipe_config->dsc_params.dsc_split = true;
> > +		} else {
> > +			DRM_DEBUG_KMS("Cannot split stream to use 2 VDSC instances\n");
> > +			return false;
> > +		}
> > +	}
> > +	pipe_config->dsc_params.compression_enable = true;
> > +	DRM_DEBUG_KMS("DP DSC computed with Input Bpp = %d "
> > +		      "Compressed Bpp = %d Slice Count = %d\n",
> > +		      pipe_config->pipe_bpp,
> > +		      pipe_config->dsc_params.compressed_bpp,
> > +		      pipe_config->dsc_params.slice_count);
> > +
> > +	return true;
> > +}
> > +
> >  static bool
> >  intel_dp_compute_link_config(struct intel_encoder *encoder,
> >  			     struct intel_crtc_state *pipe_config)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> >  	struct link_config_limits limits;
> >  	int common_len;
> > +	bool ret = false;
> >  
> >  	common_len = intel_dp_common_len_rate_limit(intel_dp,
> >  						    intel_dp->max_link_rate);
> > @@ -2056,7 +2136,9 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >  	limits.min_lane_count = 1;
> >  	limits.max_lane_count = intel_dp_max_lane_count(intel_dp);
> >  
> > -	limits.min_bpp = 6 * 3;
> > +	limits.min_bpp = (INTEL_GEN(dev_priv) >= 10 &&
> > +			  drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) ?
> > +		DP_DSC_MIN_SUPPORTED_BPC * 3 : 6 * 3;
> >  	limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
> 
> Hmm. This now assumes that we will in fact use DSC. I guess we can only
> make that assumption when we've determined that DSC is supported and the
> max_bpp also allows DSC.
> 
> So something like:
> 
> max_bpp = intel_dp_compute_bpp();
> if (supports_dsc() && max_bpp >= DP_DSC_MIN_SUPPORTED_BPC*3)
> 	min_bpp = DP_DSC_MIN_SUPPORTED_BPC * 3;
> else
> 	min_bpp = 6 * 3
> 
> >  
> >  	if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) {
> > @@ -2081,7 +2163,7 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >  		      intel_dp->common_rates[limits.max_clock],
> >  		      limits.max_bpp, adjusted_mode->crtc_clock);
> >  
> > -	if (intel_dp_is_edp(intel_dp)) {
> > +	if (intel_dp_is_edp(intel_dp))
> >  		/*
> >  		 * Optimize for fast and narrow. eDP 1.3 section 3.3 and eDP 1.4
> >  		 * section A.1: "It is recommended that the minimum number of
> > @@ -2091,26 +2173,48 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >  		 * Note that we use the max clock and lane count for eDP 1.3 and
> >  		 * earlier, and fast vs. wide is irrelevant.
> >  		 */
> > -		if (!intel_dp_compute_link_config_fast(intel_dp, pipe_config,
> > -						       &limits))
> > -			return false;
> > -	} else {
> > +		ret = intel_dp_compute_link_config_fast(intel_dp, pipe_config,
> > +							&limits);
> > +	else
> >  		/* Optimize for slow and wide. */
> > -		if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config,
> > -						       &limits))
> > +		ret = intel_dp_compute_link_config_wide(intel_dp, pipe_config,
> > +							&limits);
> > +
> > +	/* enable compression if the mode doesn't fit available BW */
> > +	if (!ret) {
> > +		DRM_DEBUG_KMS("DP required Link rate %i does not fit available %i\n",
> > +			      intel_dp_link_required(adjusted_mode->crtc_clock,
> > +						     pipe_config->pipe_bpp),
> > +			      intel_dp_max_data_rate(pipe_config->port_clock,
> > +						     pipe_config->lane_count));
> > +
> > +		if (!intel_dp_dsc_compute_config(intel_dp, pipe_config,
> > +						 &limits))
> >  			return false;
> >  	}
> >  
> > -	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> > -		      pipe_config->lane_count, pipe_config->port_clock,
> > -		      pipe_config->pipe_bpp);
> > +	if (pipe_config->dsc_params.compression_enable) {
> > +		DRM_DEBUG_KMS("DP lane count %d clock %d Input bpp %d Compressed bpp %d\n",
> > +			      pipe_config->lane_count, pipe_config->port_clock,
> > +			      pipe_config->pipe_bpp,
> > +			      pipe_config->dsc_params.compressed_bpp);
> >  
> > -	DRM_DEBUG_KMS("DP link rate required %i available %i\n",
> > -		      intel_dp_link_required(adjusted_mode->crtc_clock,
> > -					     pipe_config->pipe_bpp),
> > -		      intel_dp_max_data_rate(pipe_config->port_clock,
> > -					     pipe_config->lane_count));
> > +		DRM_DEBUG_KMS("DP link rate required %i available %i\n",
> > +			      intel_dp_link_required(adjusted_mode->crtc_clock,
> > +						     pipe_config->dsc_params.compressed_bpp),
> > +			      intel_dp_max_data_rate(pipe_config->port_clock,
> > +						     pipe_config->lane_count));
> > +	} else {
> > +		DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> > +			      pipe_config->lane_count, pipe_config->port_clock,
> > +			      pipe_config->pipe_bpp);
> >  
> > +		DRM_DEBUG_KMS("DP link rate required %i available %i\n",
> > +			      intel_dp_link_required(adjusted_mode->crtc_clock,
> > +						     pipe_config->pipe_bpp),
> > +			      intel_dp_max_data_rate(pipe_config->port_clock,
> > +						     pipe_config->lane_count));
> > +	}
> >  	return true;
> >  }
> >  
> > @@ -2194,7 +2298,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >  			intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_LIMITED;
> >  	}
> >  
> > -	intel_link_compute_m_n(pipe_config->pipe_bpp, pipe_config->lane_count,
> > +	intel_link_compute_m_n(pipe_config->pipe_bpp,
> > +			       pipe_config->dsc_params.compressed_bpp,
> > +			       pipe_config->lane_count,
> >  			       adjusted_mode->crtc_clock,
> >  			       pipe_config->port_clock,
> >  			       &pipe_config->dp_m_n,
> > @@ -2203,7 +2309,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >  	if (intel_connector->panel.downclock_mode != NULL &&
> >  		dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) {
> >  			pipe_config->has_drrs = true;
> > -			intel_link_compute_m_n(pipe_config->pipe_bpp,
> > +			intel_link_compute_m_n(pipe_config->pipe_bpp, 0,
> >  					       pipe_config->lane_count,
> >  					       intel_connector->panel.downclock_mode->clock,
> >  					       pipe_config->port_clock,
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 8b71d64ebd9d..e02caedd443c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -90,7 +90,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  		}
> >  	}
> >  
> > -	intel_link_compute_m_n(bpp, lane_count,
> > +	intel_link_compute_m_n(bpp, 0, lane_count,
> >  			       adjusted_mode->crtc_clock,
> >  			       pipe_config->port_clock,
> >  			       &pipe_config->dp_m_n,
> > -- 
> > 2.18.0
> 
> -- 
> Ville Syrjälä
> Intel

Navare, Manasi Oct. 29, 2018, 10:12 p.m. UTC | #4

On Mon, Oct 29, 2018 at 10:30:39PM +0200, Ville Syrjälä wrote:
> On Wed, Oct 24, 2018 at 03:28:25PM -0700, Manasi Navare wrote:
> > DSC params like the enable, compressed bpp, slice count and
> > dsc_split are added to the intel_crtc_state. These parameters
> > are set based on the requested mode and available link parameters
> > during the pipe configuration in atomic check phase.
> > These values are then later used to populate the remaining DSC
> > and RC parameters before enbaling DSC in atomic commit.
> > 
> > v9:
> > * Rebase on top of drm-tip that now uses fast_narrow config
> > for edp (Manasi)
> > v8:
> > * Check for DSC bpc not 0 (manasi)
> > 
> > v7:
> > * Fix indentation in compute_m_n (Manasi)
> > 
> > v6 (From Gaurav):
> > * Remove function call of intel_dp_compute_dsc_params() and
> > invoke intel_dp_compute_dsc_params() in the patch where
> > it is defined to fix compilation warning (Gaurav)
> > 
> > v5:
> > Add drm_dsc_cfg in intel_crtc_state (Manasi)
> > 
> > v4:
> > * Rebase on refactoring of intel_dp_compute_config on tip (Manasi)
> > * Add a comment why we need to check PSR while enabling DSC (Gaurav)
> > 
> > v3:
> > * Check PPR > max_cdclock to use 2 VDSC instances (Ville)
> > 
> > v2:
> > * Add if-else for eDP/DP (Gaurav)
> > 
> > Cc: Jani Nikula <[e-mail]>
> > Cc: Ville Syrjala <[e-mail]>
> > Cc: Anusha Srivatsa <[e-mail]>
> > Cc: Gaurav K Singh <[e-mail]>
> > Signed-off-by: Manasi Navare <[e-mail]>
> > Reviewed-by: Anusha Srivatsa <[e-mail]>
> > Acked-by: Jani Nikula <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |  20 +++-
> >  drivers/gpu/drm/i915/intel_display.h |   3 +-
> >  drivers/gpu/drm/i915/intel_dp.c      | 170 ++++++++++++++++++++++-----
> >  drivers/gpu/drm/i915/intel_dp_mst.c  |   2 +-
> >  4 files changed, 155 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index fe045abb6472..18737bd82b68 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6434,7 +6434,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> >  
> >  	pipe_config->fdi_lanes = lane;
> >  
> > -	intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
> > +	intel_link_compute_m_n(pipe_config->pipe_bpp, 0, lane, fdi_dotclock,
> >  			       link_bw, &pipe_config->fdi_m_n, false);
> >  
> >  	ret = ironlake_check_fdi_lanes(dev, intel_crtc->pipe, pipe_config);
> > @@ -6671,17 +6671,25 @@ static void compute_m_n(unsigned int m, unsigned int n,
> >  }
> >  
> >  void
> > -intel_link_compute_m_n(int bits_per_pixel, int nlanes,
> > +intel_link_compute_m_n(int bits_per_pixel, uint16_t compressed_bpp,
> > +		       int nlanes,
> >  		       int pixel_clock, int link_clock,
> >  		       struct intel_link_m_n *m_n,
> >  		       bool constant_n)
> >  {
> >  	m_n->tu = 64;
> >  
> > -	compute_m_n(bits_per_pixel * pixel_clock,
> > -		    link_clock * nlanes * 8,
> > -		    &m_n->gmch_m, &m_n->gmch_n,
> > -		    constant_n);
> > +	/* For DSC, Data M/N calculation uses compressed BPP */
> > +	if (compressed_bpp)
> > +		compute_m_n(compressed_bpp * pixel_clock,
> > +			    link_clock * nlanes * 8,
> > +			    &m_n->gmch_m, &m_n->gmch_n,
> > +			    constant_n);
> > +	else
> > +		compute_m_n(bits_per_pixel * pixel_clock,
> > +			    link_clock * nlanes * 8,
> > +			    &m_n->gmch_m, &m_n->gmch_n,
> > +			    constant_n);
> >  
> >  	compute_m_n(pixel_clock, link_clock,
> >  		    &m_n->link_m, &m_n->link_n,
> > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > index 5d50decbcbb5..b0b23e1e9392 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -407,7 +407,8 @@ struct intel_link_m_n {
> >  	     (__i)++) \
> >  		for_each_if(plane)
> >  
> > -void intel_link_compute_m_n(int bpp, int nlanes,
> > +void intel_link_compute_m_n(int bpp, uint16_t compressed_bpp,
> > +			    int nlanes,
> >  			    int pixel_clock, int link_clock,
> >  			    struct intel_link_m_n *m_n,
> >  			    bool constant_n);
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 6f66a38ba0b2..a88f9371dd32 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -47,6 +47,8 @@
> >  
> >  /* DP DSC small joiner has 2 FIFOs each of 640 x 6 bytes */
> >  #define DP_DSC_MAX_SMALL_JOINER_RAM_BUFFER	61440
> > +#define DP_DSC_MIN_SUPPORTED_BPC		8
> > +#define DP_DSC_MAX_SUPPORTED_BPC		10
> >  
> >  /* DP DSC throughput values used for slice count calculations KPixels/s */
> >  #define DP_DSC_PEAK_PIXEL_RATE			2720000
> > @@ -1924,6 +1926,16 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
> >  		}
> >  	}
> >  
> > +	/* If DSC is supported, use the max value reported by panel */
> > +	if (INTEL_GEN(dev_priv) >= 10 &&
> > +	    drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) {
> > +		bpc = min_t(u8,
> > +			    drm_dp_dsc_sink_max_color_depth(intel_dp->dsc_dpcd),
> > +			    DP_DSC_MAX_SUPPORTED_BPC);
> > +		if (bpc)
> > +			bpp = 3 * bpc;
> 
> This seems like it should be 'bpp = min(bpp, 3*bpc)'. 
> Otherwise we may bump the bpp above a limit we already established earlier.
>

Yes will make this change.
 
> > +	}
> > +
> >  	return bpp;
> >  }
> >  
> > @@ -1984,14 +1996,11 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
> >  				link_clock = intel_dp->common_rates[clock];
> >  				link_avail = intel_dp_max_data_rate(link_clock,
> >  								    lane_count);
> > -
> > -				if (mode_rate <= link_avail) {
> > -					pipe_config->lane_count = lane_count;
> > -					pipe_config->pipe_bpp = bpp;
> > -					pipe_config->port_clock = link_clock;
> > -
> > +				pipe_config->lane_count = lane_count;
> > +				pipe_config->pipe_bpp = bpp;
> > +				pipe_config->port_clock = link_clock;
> > +				if (mode_rate <= link_avail)
> >  					return true;
> 
> Why do we need to assign these if we don't accept the configuration?

We need to assign them because in case of failure, we use them to then configure DSC parameters
in intel_dp_dsc_compute_config().
Previously we were just returning a failure if this failed and so we need not ssign them. But now
in case this fails, we try the DSC compute config.

> 
> > -				}
> >  			}
> >  		}
> >  	}
> > @@ -2020,14 +2029,11 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
> >  				link_clock = intel_dp->common_rates[clock];
> >  				link_avail = intel_dp_max_data_rate(link_clock,
> >  								    lane_count);
> > -
> > -				if (mode_rate <= link_avail) {
> > -					pipe_config->lane_count = lane_count;
> > -					pipe_config->pipe_bpp = bpp;
> > -					pipe_config->port_clock = link_clock;
> > -
> > +				pipe_config->lane_count = lane_count;
> > +				pipe_config->pipe_bpp = bpp;
> > +				pipe_config->port_clock = link_clock;
> > +				if (mode_rate <= link_avail)
> >  					return true;
> > -				}
> >  			}
> >  		}
> >  	}
> > @@ -2035,14 +2041,88 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
> >  	return false;
> >  }
> >  
> > +static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > +					struct intel_crtc_state *pipe_config,
> > +					struct link_config_limits *limits)
> > +{
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> > +	enum pipe pipe = to_intel_crtc(pipe_config->base.crtc)->pipe;
> > +	u16 dsc_max_output_bpp = 0;
> > +	u8 dsc_dp_slice_count = 0;
> > +
> > +	if (INTEL_GEN(dev_priv) < 10 ||
> > +	    !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))
> > +		return false;
> > +
> > +	/* DP DSC only supported on Pipe B and C */
> > +	if (pipe == PIPE_A && !intel_dp_is_edp(intel_dp))
> > +		return false;
> 
> Do we mean 'transcoder == EDP || transcoder == B || transcoder == C' ?
> Or maybe 'transcoder != A' for short. I guess this will need to adjusted
> for the next platform anyway so making the assumption that transcoder A
> is the only one that can't do DSC is fine.

Yes the mode I am trying to reject is in case of a NUC, where there is no edp, the first
DP connection will use Pipe A and transcoder A which does not support DSC. So reject it.
So yea I guess I could just check for transcoder == A , then reject.

> 
> This whole thing seems like a good helper function.
> intel_dp_source_supports_dsc() or something like that. And then we
> could have intel_dp_supports_dsc() which just calls that +
> drm_dp_sink_supports_dsc().
> 
> intel_dp_compute_bpp() should use this at least, and probably a few
> other places as well.
> 
> > +
> > +	/* DSC not supported for DSC sink BPC < 8 */
> > +	if (limits->max_bpp < 3 * DP_DSC_MIN_SUPPORTED_BPC) {
> > +		DRM_DEBUG_KMS("No DSC support for less than 8bpc\n");
> > +		return false;
> > +	}
> > +
> > +	if (intel_dp_is_edp(intel_dp)) {
> > +		pipe_config->dsc_params.compressed_bpp =
> > +			drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4;
> > +		pipe_config->dsc_params.slice_count =
> > +			drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
> > +							true);
> > +	} else {
> > +		dsc_max_output_bpp =
> > +			intel_dp_dsc_get_output_bpp(pipe_config->port_clock,
> > +						    pipe_config->lane_count,
> > +						    adjusted_mode->crtc_clock,
> > +						    adjusted_mode->crtc_hdisplay);
> > +		dsc_dp_slice_count =
> > +			intel_dp_dsc_get_slice_count(intel_dp,
> > +						     adjusted_mode->crtc_clock,
> > +						     adjusted_mode->crtc_hdisplay);
> > +		if (!(dsc_max_output_bpp && dsc_dp_slice_count)) {
> > +			DRM_DEBUG_KMS("Compressed BPP/Slice Count not supported\n");
> > +			return false;
> > +		}
> > +		pipe_config->dsc_params.compressed_bpp = dsc_max_output_bpp >> 4;
> > +		pipe_config->dsc_params.slice_count = dsc_dp_slice_count;
> > +	}
> > +	/*
> > +	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
> > +	 * is greater than the maximum Cdclock and if slice count is even
> > +	 * then we need to use 2 VDSC instances.
> > +	 */
> > +	pipe_config->dsc_params.dsc_split = false;
> > +	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq) {
> > +		if (pipe_config->dsc_params.slice_count > 1) {
> > +			pipe_config->dsc_params.dsc_split = true;
> > +		} else {
> > +			DRM_DEBUG_KMS("Cannot split stream to use 2 VDSC instances\n");
> > +			return false;
> > +		}
> > +	}
> > +	pipe_config->dsc_params.compression_enable = true;
> > +	DRM_DEBUG_KMS("DP DSC computed with Input Bpp = %d "
> > +		      "Compressed Bpp = %d Slice Count = %d\n",
> > +		      pipe_config->pipe_bpp,
> > +		      pipe_config->dsc_params.compressed_bpp,
> > +		      pipe_config->dsc_params.slice_count);
> > +
> > +	return true;
> > +}
> > +
> >  static bool
> >  intel_dp_compute_link_config(struct intel_encoder *encoder,
> >  			     struct intel_crtc_state *pipe_config)
> >  {
> > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> >  	struct link_config_limits limits;
> >  	int common_len;
> > +	bool ret = false;
> >  
> >  	common_len = intel_dp_common_len_rate_limit(intel_dp,
> >  						    intel_dp->max_link_rate);
> > @@ -2056,7 +2136,9 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >  	limits.min_lane_count = 1;
> >  	limits.max_lane_count = intel_dp_max_lane_count(intel_dp);
> >  
> > -	limits.min_bpp = 6 * 3;
> > +	limits.min_bpp = (INTEL_GEN(dev_priv) >= 10 &&
> > +			  drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) ?
> > +		DP_DSC_MIN_SUPPORTED_BPC * 3 : 6 * 3;
> >  	limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
> 
> Hmm. This now assumes that we will in fact use DSC. I guess we can only
> make that assumption when we've determined that DSC is supported and the
> max_bpp also allows DSC.

But the intel_dp_compute_bpp does check for supports_dsc so that
should be fine. But for min_bpp yes I could use the below logic.
And may be move that to a helper?

Manasi


> 
> So something like:
> 
> max_bpp = intel_dp_compute_bpp();
> if (supports_dsc() && max_bpp >= DP_DSC_MIN_SUPPORTED_BPC*3)
> 	min_bpp = DP_DSC_MIN_SUPPORTED_BPC * 3;
> else
> 	min_bpp = 6 * 3
> 
> >  
> >  	if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) {
> > @@ -2081,7 +2163,7 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >  		      intel_dp->common_rates[limits.max_clock],
> >  		      limits.max_bpp, adjusted_mode->crtc_clock);
> >  
> > -	if (intel_dp_is_edp(intel_dp)) {
> > +	if (intel_dp_is_edp(intel_dp))
> >  		/*
> >  		 * Optimize for fast and narrow. eDP 1.3 section 3.3 and eDP 1.4
> >  		 * section A.1: "It is recommended that the minimum number of
> > @@ -2091,26 +2173,48 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >  		 * Note that we use the max clock and lane count for eDP 1.3 and
> >  		 * earlier, and fast vs. wide is irrelevant.
> >  		 */
> > -		if (!intel_dp_compute_link_config_fast(intel_dp, pipe_config,
> > -						       &limits))
> > -			return false;
> > -	} else {
> > +		ret = intel_dp_compute_link_config_fast(intel_dp, pipe_config,
> > +							&limits);
> > +	else
> >  		/* Optimize for slow and wide. */
> > -		if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config,
> > -						       &limits))
> > +		ret = intel_dp_compute_link_config_wide(intel_dp, pipe_config,
> > +							&limits);
> > +
> > +	/* enable compression if the mode doesn't fit available BW */
> > +	if (!ret) {
> > +		DRM_DEBUG_KMS("DP required Link rate %i does not fit available %i\n",
> > +			      intel_dp_link_required(adjusted_mode->crtc_clock,
> > +						     pipe_config->pipe_bpp),
> > +			      intel_dp_max_data_rate(pipe_config->port_clock,
> > +						     pipe_config->lane_count));
> > +
> > +		if (!intel_dp_dsc_compute_config(intel_dp, pipe_config,
> > +						 &limits))
> >  			return false;
> >  	}
> >  
> > -	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> > -		      pipe_config->lane_count, pipe_config->port_clock,
> > -		      pipe_config->pipe_bpp);
> > +	if (pipe_config->dsc_params.compression_enable) {
> > +		DRM_DEBUG_KMS("DP lane count %d clock %d Input bpp %d Compressed bpp %d\n",
> > +			      pipe_config->lane_count, pipe_config->port_clock,
> > +			      pipe_config->pipe_bpp,
> > +			      pipe_config->dsc_params.compressed_bpp);
> >  
> > -	DRM_DEBUG_KMS("DP link rate required %i available %i\n",
> > -		      intel_dp_link_required(adjusted_mode->crtc_clock,
> > -					     pipe_config->pipe_bpp),
> > -		      intel_dp_max_data_rate(pipe_config->port_clock,
> > -					     pipe_config->lane_count));
> > +		DRM_DEBUG_KMS("DP link rate required %i available %i\n",
> > +			      intel_dp_link_required(adjusted_mode->crtc_clock,
> > +						     pipe_config->dsc_params.compressed_bpp),
> > +			      intel_dp_max_data_rate(pipe_config->port_clock,
> > +						     pipe_config->lane_count));
> > +	} else {
> > +		DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> > +			      pipe_config->lane_count, pipe_config->port_clock,
> > +			      pipe_config->pipe_bpp);
> >  
> > +		DRM_DEBUG_KMS("DP link rate required %i available %i\n",
> > +			      intel_dp_link_required(adjusted_mode->crtc_clock,
> > +						     pipe_config->pipe_bpp),
> > +			      intel_dp_max_data_rate(pipe_config->port_clock,
> > +						     pipe_config->lane_count));
> > +	}
> >  	return true;
> >  }
> >  
> > @@ -2194,7 +2298,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >  			intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_LIMITED;
> >  	}
> >  
> > -	intel_link_compute_m_n(pipe_config->pipe_bpp, pipe_config->lane_count,
> > +	intel_link_compute_m_n(pipe_config->pipe_bpp,
> > +			       pipe_config->dsc_params.compressed_bpp,
> > +			       pipe_config->lane_count,
> >  			       adjusted_mode->crtc_clock,
> >  			       pipe_config->port_clock,
> >  			       &pipe_config->dp_m_n,
> > @@ -2203,7 +2309,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >  	if (intel_connector->panel.downclock_mode != NULL &&
> >  		dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) {
> >  			pipe_config->has_drrs = true;
> > -			intel_link_compute_m_n(pipe_config->pipe_bpp,
> > +			intel_link_compute_m_n(pipe_config->pipe_bpp, 0,
> >  					       pipe_config->lane_count,
> >  					       intel_connector->panel.downclock_mode->clock,
> >  					       pipe_config->port_clock,
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 8b71d64ebd9d..e02caedd443c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -90,7 +90,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  		}
> >  	}
> >  
> > -	intel_link_compute_m_n(bpp, lane_count,
> > +	intel_link_compute_m_n(bpp, 0, lane_count,
> >  			       adjusted_mode->crtc_clock,
> >  			       pipe_config->port_clock,
> >  			       &pipe_config->dp_m_n,
> > -- 
> > 2.18.0
> 
> -- 
> Ville Syrjälä
> Intel

Navare, Manasi Oct. 29, 2018, 11:08 p.m. UTC | #5

On Mon, Oct 29, 2018 at 10:34:58PM +0200, Ville Syrjälä wrote:
> On Mon, Oct 29, 2018 at 10:30:39PM +0200, Ville Syrjälä wrote:
> > On Wed, Oct 24, 2018 at 03:28:25PM -0700, Manasi Navare wrote:
> > > DSC params like the enable, compressed bpp, slice count and
> > > dsc_split are added to the intel_crtc_state. These parameters
> > > are set based on the requested mode and available link parameters
> > > during the pipe configuration in atomic check phase.
> > > These values are then later used to populate the remaining DSC
> > > and RC parameters before enbaling DSC in atomic commit.
> > > 
> > > v9:
> > > * Rebase on top of drm-tip that now uses fast_narrow config
> > > for edp (Manasi)
> > > v8:
> > > * Check for DSC bpc not 0 (manasi)
> > > 
> > > v7:
> > > * Fix indentation in compute_m_n (Manasi)
> > > 
> > > v6 (From Gaurav):
> > > * Remove function call of intel_dp_compute_dsc_params() and
> > > invoke intel_dp_compute_dsc_params() in the patch where
> > > it is defined to fix compilation warning (Gaurav)
> > > 
> > > v5:
> > > Add drm_dsc_cfg in intel_crtc_state (Manasi)
> > > 
> > > v4:
> > > * Rebase on refactoring of intel_dp_compute_config on tip (Manasi)
> > > * Add a comment why we need to check PSR while enabling DSC (Gaurav)
> > > 
> > > v3:
> > > * Check PPR > max_cdclock to use 2 VDSC instances (Ville)
> > > 
> > > v2:
> > > * Add if-else for eDP/DP (Gaurav)
> > > 
> > > Cc: Jani Nikula <[e-mail]>
> > > Cc: Ville Syrjala <[e-mail]>
> > > Cc: Anusha Srivatsa <[e-mail]>
> > > Cc: Gaurav K Singh <[e-mail]>
> > > Signed-off-by: Manasi Navare <[e-mail]>
> > > Reviewed-by: Anusha Srivatsa <[e-mail]>
> > > Acked-by: Jani Nikula <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |  20 +++-
> > >  drivers/gpu/drm/i915/intel_display.h |   3 +-
> > >  drivers/gpu/drm/i915/intel_dp.c      | 170 ++++++++++++++++++++++-----
> > >  drivers/gpu/drm/i915/intel_dp_mst.c  |   2 +-
> > >  4 files changed, 155 insertions(+), 40 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index fe045abb6472..18737bd82b68 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -6434,7 +6434,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> > >  
> > >  	pipe_config->fdi_lanes = lane;
> > >  
> > > -	intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
> > > +	intel_link_compute_m_n(pipe_config->pipe_bpp, 0, lane, fdi_dotclock,
> > >  			       link_bw, &pipe_config->fdi_m_n, false);
> > >  
> > >  	ret = ironlake_check_fdi_lanes(dev, intel_crtc->pipe, pipe_config);
> > > @@ -6671,17 +6671,25 @@ static void compute_m_n(unsigned int m, unsigned int n,
> > >  }
> > >  
> > >  void
> > > -intel_link_compute_m_n(int bits_per_pixel, int nlanes,
> > > +intel_link_compute_m_n(int bits_per_pixel, uint16_t compressed_bpp,
> > > +		       int nlanes,
> > >  		       int pixel_clock, int link_clock,
> > >  		       struct intel_link_m_n *m_n,
> > >  		       bool constant_n)
> > >  {
> > >  	m_n->tu = 64;
> > >  
> > > -	compute_m_n(bits_per_pixel * pixel_clock,
> > > -		    link_clock * nlanes * 8,
> > > -		    &m_n->gmch_m, &m_n->gmch_n,
> > > -		    constant_n);
> > > +	/* For DSC, Data M/N calculation uses compressed BPP */
> > > +	if (compressed_bpp)
> > > +		compute_m_n(compressed_bpp * pixel_clock,
> > > +			    link_clock * nlanes * 8,
> > > +			    &m_n->gmch_m, &m_n->gmch_n,
> > > +			    constant_n);
> > > +	else
> > > +		compute_m_n(bits_per_pixel * pixel_clock,
> > > +			    link_clock * nlanes * 8,
> > > +			    &m_n->gmch_m, &m_n->gmch_n,
> > > +			    constant_n);
> > >  
> > >  	compute_m_n(pixel_clock, link_clock,
> > >  		    &m_n->link_m, &m_n->link_n,
> > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > index 5d50decbcbb5..b0b23e1e9392 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > @@ -407,7 +407,8 @@ struct intel_link_m_n {
> > >  	     (__i)++) \
> > >  		for_each_if(plane)
> > >  
> > > -void intel_link_compute_m_n(int bpp, int nlanes,
> > > +void intel_link_compute_m_n(int bpp, uint16_t compressed_bpp,
> > > +			    int nlanes,
> > >  			    int pixel_clock, int link_clock,
> > >  			    struct intel_link_m_n *m_n,
> > >  			    bool constant_n);
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 6f66a38ba0b2..a88f9371dd32 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -47,6 +47,8 @@
> > >  
> > >  /* DP DSC small joiner has 2 FIFOs each of 640 x 6 bytes */
> > >  #define DP_DSC_MAX_SMALL_JOINER_RAM_BUFFER	61440
> > > +#define DP_DSC_MIN_SUPPORTED_BPC		8
> > > +#define DP_DSC_MAX_SUPPORTED_BPC		10
> > >  
> > >  /* DP DSC throughput values used for slice count calculations KPixels/s */
> > >  #define DP_DSC_PEAK_PIXEL_RATE			2720000
> > > @@ -1924,6 +1926,16 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
> > >  		}
> > >  	}
> > >  
> > > +	/* If DSC is supported, use the max value reported by panel */
> > > +	if (INTEL_GEN(dev_priv) >= 10 &&
> > > +	    drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) {
> > > +		bpc = min_t(u8,
> > > +			    drm_dp_dsc_sink_max_color_depth(intel_dp->dsc_dpcd),
> > > +			    DP_DSC_MAX_SUPPORTED_BPC);
> > > +		if (bpc)
> > > +			bpp = 3 * bpc;
> > 
> > This seems like it should be 'bpp = min(bpp, 3*bpc)'. 
> > Otherwise we may bump the bpp above a limit we already established earlier.
> > 
> > > +	}
> > > +
> > >  	return bpp;
> > >  }
> > >  
> > > @@ -1984,14 +1996,11 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
> > >  				link_clock = intel_dp->common_rates[clock];
> > >  				link_avail = intel_dp_max_data_rate(link_clock,
> > >  								    lane_count);
> > > -
> > > -				if (mode_rate <= link_avail) {
> > > -					pipe_config->lane_count = lane_count;
> > > -					pipe_config->pipe_bpp = bpp;
> > > -					pipe_config->port_clock = link_clock;
> > > -
> > > +				pipe_config->lane_count = lane_count;
> > > +				pipe_config->pipe_bpp = bpp;
> > > +				pipe_config->port_clock = link_clock;
> > > +				if (mode_rate <= link_avail)
> > >  					return true;
> > 
> > Why do we need to assign these if we don't accept the configuration?
> > 
> > > -				}
> > >  			}
> > >  		}
> > >  	}
> > > @@ -2020,14 +2029,11 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
> > >  				link_clock = intel_dp->common_rates[clock];
> > >  				link_avail = intel_dp_max_data_rate(link_clock,
> > >  								    lane_count);
> > > -
> > > -				if (mode_rate <= link_avail) {
> > > -					pipe_config->lane_count = lane_count;
> > > -					pipe_config->pipe_bpp = bpp;
> > > -					pipe_config->port_clock = link_clock;
> > > -
> > > +				pipe_config->lane_count = lane_count;
> > > +				pipe_config->pipe_bpp = bpp;
> > > +				pipe_config->port_clock = link_clock;
> > > +				if (mode_rate <= link_avail)
> > >  					return true;
> > > -				}
> > >  			}
> > >  		}
> > >  	}
> > > @@ -2035,14 +2041,88 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
> > >  	return false;
> > >  }
> > >  
> > > +static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > > +					struct intel_crtc_state *pipe_config,
> > > +					struct link_config_limits *limits)
> > > +{
> > > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > > +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> > > +	enum pipe pipe = to_intel_crtc(pipe_config->base.crtc)->pipe;
> > > +	u16 dsc_max_output_bpp = 0;
> > > +	u8 dsc_dp_slice_count = 0;
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 10 ||
> > > +	    !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))
> > > +		return false;
> > > +
> > > +	/* DP DSC only supported on Pipe B and C */
> > > +	if (pipe == PIPE_A && !intel_dp_is_edp(intel_dp))
> > > +		return false;
> > 
> > Do we mean 'transcoder == EDP || transcoder == B || transcoder == C' ?
> > Or maybe 'transcoder != A' for short. I guess this will need to adjusted
> > for the next platform anyway so making the assumption that transcoder A
> > is the only one that can't do DSC is fine.
> > 
> > This whole thing seems like a good helper function.
> > intel_dp_source_supports_dsc() or something like that. And then we
> > could have intel_dp_supports_dsc() which just calls that +
> > drm_dp_sink_supports_dsc().
>

So intel_dp_source_supports_dsc will have platform checks, but for it to do
transcoder check it would also need crtc_state as input.

And yes intel_dp_supports_dsc() can call this and drm_dp_sink_supports_dsc()
 
> Another confusion about these checks is glk. Some other places seem
> to indicate that glk has DSC, but then code like this here doesn't 
> accept glk. What's up with that?

This is because on GLK, VDSC is only supported on eDP and not on external DP

Manasi

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

Ville Syrjälä Oct. 30, 2018, 11:41 a.m. UTC | #6

On Mon, Oct 29, 2018 at 03:12:51PM -0700, Manasi Navare wrote:
> On Mon, Oct 29, 2018 at 10:30:39PM +0200, Ville Syrjälä wrote:
> > On Wed, Oct 24, 2018 at 03:28:25PM -0700, Manasi Navare wrote:
> > > DSC params like the enable, compressed bpp, slice count and
> > > dsc_split are added to the intel_crtc_state. These parameters
> > > are set based on the requested mode and available link parameters
> > > during the pipe configuration in atomic check phase.
> > > These values are then later used to populate the remaining DSC
> > > and RC parameters before enbaling DSC in atomic commit.
> > > 
> > > v9:
> > > * Rebase on top of drm-tip that now uses fast_narrow config
> > > for edp (Manasi)
> > > v8:
> > > * Check for DSC bpc not 0 (manasi)
> > > 
> > > v7:
> > > * Fix indentation in compute_m_n (Manasi)
> > > 
> > > v6 (From Gaurav):
> > > * Remove function call of intel_dp_compute_dsc_params() and
> > > invoke intel_dp_compute_dsc_params() in the patch where
> > > it is defined to fix compilation warning (Gaurav)
> > > 
> > > v5:
> > > Add drm_dsc_cfg in intel_crtc_state (Manasi)
> > > 
> > > v4:
> > > * Rebase on refactoring of intel_dp_compute_config on tip (Manasi)
> > > * Add a comment why we need to check PSR while enabling DSC (Gaurav)
> > > 
> > > v3:
> > > * Check PPR > max_cdclock to use 2 VDSC instances (Ville)
> > > 
> > > v2:
> > > * Add if-else for eDP/DP (Gaurav)
> > > 
> > > Cc: Jani Nikula <[e-mail]>
> > > Cc: Ville Syrjala <[e-mail]>
> > > Cc: Anusha Srivatsa <[e-mail]>
> > > Cc: Gaurav K Singh <[e-mail]>
> > > Signed-off-by: Manasi Navare <[e-mail]>
> > > Reviewed-by: Anusha Srivatsa <[e-mail]>
> > > Acked-by: Jani Nikula <[e-mail]>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |  20 +++-
> > >  drivers/gpu/drm/i915/intel_display.h |   3 +-
> > >  drivers/gpu/drm/i915/intel_dp.c      | 170 ++++++++++++++++++++++-----
> > >  drivers/gpu/drm/i915/intel_dp_mst.c  |   2 +-
> > >  4 files changed, 155 insertions(+), 40 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index fe045abb6472..18737bd82b68 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -6434,7 +6434,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> > >  
> > >  	pipe_config->fdi_lanes = lane;
> > >  
> > > -	intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
> > > +	intel_link_compute_m_n(pipe_config->pipe_bpp, 0, lane, fdi_dotclock,
> > >  			       link_bw, &pipe_config->fdi_m_n, false);
> > >  
> > >  	ret = ironlake_check_fdi_lanes(dev, intel_crtc->pipe, pipe_config);
> > > @@ -6671,17 +6671,25 @@ static void compute_m_n(unsigned int m, unsigned int n,
> > >  }
> > >  
> > >  void
> > > -intel_link_compute_m_n(int bits_per_pixel, int nlanes,
> > > +intel_link_compute_m_n(int bits_per_pixel, uint16_t compressed_bpp,
> > > +		       int nlanes,
> > >  		       int pixel_clock, int link_clock,
> > >  		       struct intel_link_m_n *m_n,
> > >  		       bool constant_n)
> > >  {
> > >  	m_n->tu = 64;
> > >  
> > > -	compute_m_n(bits_per_pixel * pixel_clock,
> > > -		    link_clock * nlanes * 8,
> > > -		    &m_n->gmch_m, &m_n->gmch_n,
> > > -		    constant_n);
> > > +	/* For DSC, Data M/N calculation uses compressed BPP */
> > > +	if (compressed_bpp)
> > > +		compute_m_n(compressed_bpp * pixel_clock,
> > > +			    link_clock * nlanes * 8,
> > > +			    &m_n->gmch_m, &m_n->gmch_n,
> > > +			    constant_n);
> > > +	else
> > > +		compute_m_n(bits_per_pixel * pixel_clock,
> > > +			    link_clock * nlanes * 8,
> > > +			    &m_n->gmch_m, &m_n->gmch_n,
> > > +			    constant_n);
> > >  
> > >  	compute_m_n(pixel_clock, link_clock,
> > >  		    &m_n->link_m, &m_n->link_n,
> > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > index 5d50decbcbb5..b0b23e1e9392 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > @@ -407,7 +407,8 @@ struct intel_link_m_n {
> > >  	     (__i)++) \
> > >  		for_each_if(plane)
> > >  
> > > -void intel_link_compute_m_n(int bpp, int nlanes,
> > > +void intel_link_compute_m_n(int bpp, uint16_t compressed_bpp,
> > > +			    int nlanes,
> > >  			    int pixel_clock, int link_clock,
> > >  			    struct intel_link_m_n *m_n,
> > >  			    bool constant_n);
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index 6f66a38ba0b2..a88f9371dd32 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -47,6 +47,8 @@
> > >  
> > >  /* DP DSC small joiner has 2 FIFOs each of 640 x 6 bytes */
> > >  #define DP_DSC_MAX_SMALL_JOINER_RAM_BUFFER	61440
> > > +#define DP_DSC_MIN_SUPPORTED_BPC		8
> > > +#define DP_DSC_MAX_SUPPORTED_BPC		10
> > >  
> > >  /* DP DSC throughput values used for slice count calculations KPixels/s */
> > >  #define DP_DSC_PEAK_PIXEL_RATE			2720000
> > > @@ -1924,6 +1926,16 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
> > >  		}
> > >  	}
> > >  
> > > +	/* If DSC is supported, use the max value reported by panel */
> > > +	if (INTEL_GEN(dev_priv) >= 10 &&
> > > +	    drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) {
> > > +		bpc = min_t(u8,
> > > +			    drm_dp_dsc_sink_max_color_depth(intel_dp->dsc_dpcd),
> > > +			    DP_DSC_MAX_SUPPORTED_BPC);
> > > +		if (bpc)
> > > +			bpp = 3 * bpc;
> > 
> > This seems like it should be 'bpp = min(bpp, 3*bpc)'. 
> > Otherwise we may bump the bpp above a limit we already established earlier.
> >
> 
> Yes will make this change.
>  
> > > +	}
> > > +
> > >  	return bpp;
> > >  }
> > >  
> > > @@ -1984,14 +1996,11 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
> > >  				link_clock = intel_dp->common_rates[clock];
> > >  				link_avail = intel_dp_max_data_rate(link_clock,
> > >  								    lane_count);
> > > -
> > > -				if (mode_rate <= link_avail) {
> > > -					pipe_config->lane_count = lane_count;
> > > -					pipe_config->pipe_bpp = bpp;
> > > -					pipe_config->port_clock = link_clock;
> > > -
> > > +				pipe_config->lane_count = lane_count;
> > > +				pipe_config->pipe_bpp = bpp;
> > > +				pipe_config->port_clock = link_clock;
> > > +				if (mode_rate <= link_avail)
> > >  					return true;
> > 
> > Why do we need to assign these if we don't accept the configuration?
> 
> We need to assign them because in case of failure, we use them to then configure DSC parameters
> in intel_dp_dsc_compute_config().

What you are doing is now is effectively:
pipe_config->lane_count = max_lanes;
pipe_config->pipe_bpp = min_bpp;
pipe_config->port_clock = max_clock;

So might as well do that explicitly in dsc_compute_config() then.
Not sure that makes sense though. Maybe we DSC we can make due
with a slower/narrower link? So we might want to iterate the
possible configurations again with dsc.

> Previously we were just returning a failure if this failed and so we need not ssign them. But now
> in case this fails, we try the DSC compute config.
> 
> > 
> > > -				}
> > >  			}
> > >  		}
> > >  	}
> > > @@ -2020,14 +2029,11 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
> > >  				link_clock = intel_dp->common_rates[clock];
> > >  				link_avail = intel_dp_max_data_rate(link_clock,
> > >  								    lane_count);
> > > -
> > > -				if (mode_rate <= link_avail) {
> > > -					pipe_config->lane_count = lane_count;
> > > -					pipe_config->pipe_bpp = bpp;
> > > -					pipe_config->port_clock = link_clock;
> > > -
> > > +				pipe_config->lane_count = lane_count;
> > > +				pipe_config->pipe_bpp = bpp;
> > > +				pipe_config->port_clock = link_clock;
> > > +				if (mode_rate <= link_avail)
> > >  					return true;
> > > -				}
> > >  			}
> > >  		}
> > >  	}
> > > @@ -2035,14 +2041,88 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
> > >  	return false;
> > >  }
> > >  
> > > +static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > > +					struct intel_crtc_state *pipe_config,
> > > +					struct link_config_limits *limits)
> > > +{
> > > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > > +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> > > +	enum pipe pipe = to_intel_crtc(pipe_config->base.crtc)->pipe;
> > > +	u16 dsc_max_output_bpp = 0;
> > > +	u8 dsc_dp_slice_count = 0;
> > > +
> > > +	if (INTEL_GEN(dev_priv) < 10 ||
> > > +	    !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))
> > > +		return false;
> > > +
> > > +	/* DP DSC only supported on Pipe B and C */
> > > +	if (pipe == PIPE_A && !intel_dp_is_edp(intel_dp))
> > > +		return false;
> > 
> > Do we mean 'transcoder == EDP || transcoder == B || transcoder == C' ?
> > Or maybe 'transcoder != A' for short. I guess this will need to adjusted
> > for the next platform anyway so making the assumption that transcoder A
> > is the only one that can't do DSC is fine.
> 
> Yes the mode I am trying to reject is in case of a NUC, where there is no edp, the first
> DP connection will use Pipe A and transcoder A which does not support DSC. So reject it.
> So yea I guess I could just check for transcoder == A , then reject.
> 
> > 
> > This whole thing seems like a good helper function.
> > intel_dp_source_supports_dsc() or something like that. And then we
> > could have intel_dp_supports_dsc() which just calls that +
> > drm_dp_sink_supports_dsc().
> > 
> > intel_dp_compute_bpp() should use this at least, and probably a few
> > other places as well.
> > 
> > > +
> > > +	/* DSC not supported for DSC sink BPC < 8 */
> > > +	if (limits->max_bpp < 3 * DP_DSC_MIN_SUPPORTED_BPC) {
> > > +		DRM_DEBUG_KMS("No DSC support for less than 8bpc\n");
> > > +		return false;
> > > +	}
> > > +
> > > +	if (intel_dp_is_edp(intel_dp)) {
> > > +		pipe_config->dsc_params.compressed_bpp =
> > > +			drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4;
> > > +		pipe_config->dsc_params.slice_count =
> > > +			drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
> > > +							true);
> > > +	} else {
> > > +		dsc_max_output_bpp =
> > > +			intel_dp_dsc_get_output_bpp(pipe_config->port_clock,
> > > +						    pipe_config->lane_count,
> > > +						    adjusted_mode->crtc_clock,
> > > +						    adjusted_mode->crtc_hdisplay);
> > > +		dsc_dp_slice_count =
> > > +			intel_dp_dsc_get_slice_count(intel_dp,
> > > +						     adjusted_mode->crtc_clock,
> > > +						     adjusted_mode->crtc_hdisplay);
> > > +		if (!(dsc_max_output_bpp && dsc_dp_slice_count)) {
> > > +			DRM_DEBUG_KMS("Compressed BPP/Slice Count not supported\n");
> > > +			return false;
> > > +		}
> > > +		pipe_config->dsc_params.compressed_bpp = dsc_max_output_bpp >> 4;
> > > +		pipe_config->dsc_params.slice_count = dsc_dp_slice_count;
> > > +	}
> > > +	/*
> > > +	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
> > > +	 * is greater than the maximum Cdclock and if slice count is even
> > > +	 * then we need to use 2 VDSC instances.
> > > +	 */
> > > +	pipe_config->dsc_params.dsc_split = false;
> > > +	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq) {
> > > +		if (pipe_config->dsc_params.slice_count > 1) {
> > > +			pipe_config->dsc_params.dsc_split = true;
> > > +		} else {
> > > +			DRM_DEBUG_KMS("Cannot split stream to use 2 VDSC instances\n");
> > > +			return false;
> > > +		}
> > > +	}
> > > +	pipe_config->dsc_params.compression_enable = true;
> > > +	DRM_DEBUG_KMS("DP DSC computed with Input Bpp = %d "
> > > +		      "Compressed Bpp = %d Slice Count = %d\n",
> > > +		      pipe_config->pipe_bpp,
> > > +		      pipe_config->dsc_params.compressed_bpp,
> > > +		      pipe_config->dsc_params.slice_count);
> > > +
> > > +	return true;
> > > +}
> > > +
> > >  static bool
> > >  intel_dp_compute_link_config(struct intel_encoder *encoder,
> > >  			     struct intel_crtc_state *pipe_config)
> > >  {
> > > +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > >  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> > >  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> > >  	struct link_config_limits limits;
> > >  	int common_len;
> > > +	bool ret = false;
> > >  
> > >  	common_len = intel_dp_common_len_rate_limit(intel_dp,
> > >  						    intel_dp->max_link_rate);
> > > @@ -2056,7 +2136,9 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> > >  	limits.min_lane_count = 1;
> > >  	limits.max_lane_count = intel_dp_max_lane_count(intel_dp);
> > >  
> > > -	limits.min_bpp = 6 * 3;
> > > +	limits.min_bpp = (INTEL_GEN(dev_priv) >= 10 &&
> > > +			  drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) ?
> > > +		DP_DSC_MIN_SUPPORTED_BPC * 3 : 6 * 3;
> > >  	limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
> > 
> > Hmm. This now assumes that we will in fact use DSC. I guess we can only
> > make that assumption when we've determined that DSC is supported and the
> > max_bpp also allows DSC.
> 
> But the intel_dp_compute_bpp does check for supports_dsc so that
> should be fine. But for min_bpp yes I could use the below logic.

min_bpp is what I'm talking about here. We can't set it assuming DSC
will be used if the max_bpp is already below the DSC minimum.

> And may be move that to a helper?

Is there some other use for it? I'd rather keep this "populate the
limits" thing in one spot.

> 
> Manasi
> 
> 
> > 
> > So something like:
> > 
> > max_bpp = intel_dp_compute_bpp();
> > if (supports_dsc() && max_bpp >= DP_DSC_MIN_SUPPORTED_BPC*3)
> > 	min_bpp = DP_DSC_MIN_SUPPORTED_BPC * 3;
> > else
> > 	min_bpp = 6 * 3
> > 
> > >  
> > >  	if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) {
> > > @@ -2081,7 +2163,7 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> > >  		      intel_dp->common_rates[limits.max_clock],
> > >  		      limits.max_bpp, adjusted_mode->crtc_clock);
> > >  
> > > -	if (intel_dp_is_edp(intel_dp)) {
> > > +	if (intel_dp_is_edp(intel_dp))
> > >  		/*
> > >  		 * Optimize for fast and narrow. eDP 1.3 section 3.3 and eDP 1.4
> > >  		 * section A.1: "It is recommended that the minimum number of
> > > @@ -2091,26 +2173,48 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> > >  		 * Note that we use the max clock and lane count for eDP 1.3 and
> > >  		 * earlier, and fast vs. wide is irrelevant.
> > >  		 */
> > > -		if (!intel_dp_compute_link_config_fast(intel_dp, pipe_config,
> > > -						       &limits))
> > > -			return false;
> > > -	} else {
> > > +		ret = intel_dp_compute_link_config_fast(intel_dp, pipe_config,
> > > +							&limits);
> > > +	else
> > >  		/* Optimize for slow and wide. */
> > > -		if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config,
> > > -						       &limits))
> > > +		ret = intel_dp_compute_link_config_wide(intel_dp, pipe_config,
> > > +							&limits);
> > > +
> > > +	/* enable compression if the mode doesn't fit available BW */
> > > +	if (!ret) {
> > > +		DRM_DEBUG_KMS("DP required Link rate %i does not fit available %i\n",
> > > +			      intel_dp_link_required(adjusted_mode->crtc_clock,
> > > +						     pipe_config->pipe_bpp),
> > > +			      intel_dp_max_data_rate(pipe_config->port_clock,
> > > +						     pipe_config->lane_count));
> > > +
> > > +		if (!intel_dp_dsc_compute_config(intel_dp, pipe_config,
> > > +						 &limits))
> > >  			return false;
> > >  	}
> > >  
> > > -	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> > > -		      pipe_config->lane_count, pipe_config->port_clock,
> > > -		      pipe_config->pipe_bpp);
> > > +	if (pipe_config->dsc_params.compression_enable) {
> > > +		DRM_DEBUG_KMS("DP lane count %d clock %d Input bpp %d Compressed bpp %d\n",
> > > +			      pipe_config->lane_count, pipe_config->port_clock,
> > > +			      pipe_config->pipe_bpp,
> > > +			      pipe_config->dsc_params.compressed_bpp);
> > >  
> > > -	DRM_DEBUG_KMS("DP link rate required %i available %i\n",
> > > -		      intel_dp_link_required(adjusted_mode->crtc_clock,
> > > -					     pipe_config->pipe_bpp),
> > > -		      intel_dp_max_data_rate(pipe_config->port_clock,
> > > -					     pipe_config->lane_count));
> > > +		DRM_DEBUG_KMS("DP link rate required %i available %i\n",
> > > +			      intel_dp_link_required(adjusted_mode->crtc_clock,
> > > +						     pipe_config->dsc_params.compressed_bpp),
> > > +			      intel_dp_max_data_rate(pipe_config->port_clock,
> > > +						     pipe_config->lane_count));
> > > +	} else {
> > > +		DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> > > +			      pipe_config->lane_count, pipe_config->port_clock,
> > > +			      pipe_config->pipe_bpp);
> > >  
> > > +		DRM_DEBUG_KMS("DP link rate required %i available %i\n",
> > > +			      intel_dp_link_required(adjusted_mode->crtc_clock,
> > > +						     pipe_config->pipe_bpp),
> > > +			      intel_dp_max_data_rate(pipe_config->port_clock,
> > > +						     pipe_config->lane_count));
> > > +	}
> > >  	return true;
> > >  }
> > >  
> > > @@ -2194,7 +2298,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> > >  			intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_LIMITED;
> > >  	}
> > >  
> > > -	intel_link_compute_m_n(pipe_config->pipe_bpp, pipe_config->lane_count,
> > > +	intel_link_compute_m_n(pipe_config->pipe_bpp,
> > > +			       pipe_config->dsc_params.compressed_bpp,
> > > +			       pipe_config->lane_count,
> > >  			       adjusted_mode->crtc_clock,
> > >  			       pipe_config->port_clock,
> > >  			       &pipe_config->dp_m_n,
> > > @@ -2203,7 +2309,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> > >  	if (intel_connector->panel.downclock_mode != NULL &&
> > >  		dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) {
> > >  			pipe_config->has_drrs = true;
> > > -			intel_link_compute_m_n(pipe_config->pipe_bpp,
> > > +			intel_link_compute_m_n(pipe_config->pipe_bpp, 0,
> > >  					       pipe_config->lane_count,
> > >  					       intel_connector->panel.downclock_mode->clock,
> > >  					       pipe_config->port_clock,
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > index 8b71d64ebd9d..e02caedd443c 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > > @@ -90,7 +90,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
> > >  		}
> > >  	}
> > >  
> > > -	intel_link_compute_m_n(bpp, lane_count,
> > > +	intel_link_compute_m_n(bpp, 0, lane_count,
> > >  			       adjusted_mode->crtc_clock,
> > >  			       pipe_config->port_clock,
> > >  			       &pipe_config->dp_m_n,
> > > -- 
> > > 2.18.0
> > 
> > -- 
> > Ville Syrjälä
> > Intel

Ville Syrjälä Oct. 30, 2018, 11:46 a.m. UTC | #7

On Mon, Oct 29, 2018 at 04:08:43PM -0700, Manasi Navare wrote:
> On Mon, Oct 29, 2018 at 10:34:58PM +0200, Ville Syrjälä wrote:
> > On Mon, Oct 29, 2018 at 10:30:39PM +0200, Ville Syrjälä wrote:
> > > On Wed, Oct 24, 2018 at 03:28:25PM -0700, Manasi Navare wrote:
> > > > DSC params like the enable, compressed bpp, slice count and
> > > > dsc_split are added to the intel_crtc_state. These parameters
> > > > are set based on the requested mode and available link parameters
> > > > during the pipe configuration in atomic check phase.
> > > > These values are then later used to populate the remaining DSC
> > > > and RC parameters before enbaling DSC in atomic commit.
> > > > 
> > > > v9:
> > > > * Rebase on top of drm-tip that now uses fast_narrow config
> > > > for edp (Manasi)
> > > > v8:
> > > > * Check for DSC bpc not 0 (manasi)
> > > > 
> > > > v7:
> > > > * Fix indentation in compute_m_n (Manasi)
> > > > 
> > > > v6 (From Gaurav):
> > > > * Remove function call of intel_dp_compute_dsc_params() and
> > > > invoke intel_dp_compute_dsc_params() in the patch where
> > > > it is defined to fix compilation warning (Gaurav)
> > > > 
> > > > v5:
> > > > Add drm_dsc_cfg in intel_crtc_state (Manasi)
> > > > 
> > > > v4:
> > > > * Rebase on refactoring of intel_dp_compute_config on tip (Manasi)
> > > > * Add a comment why we need to check PSR while enabling DSC (Gaurav)
> > > > 
> > > > v3:
> > > > * Check PPR > max_cdclock to use 2 VDSC instances (Ville)
> > > > 
> > > > v2:
> > > > * Add if-else for eDP/DP (Gaurav)
> > > > 
> > > > Cc: Jani Nikula <[e-mail]>
> > > > Cc: Ville Syrjala <[e-mail]>
> > > > Cc: Anusha Srivatsa <[e-mail]>
> > > > Cc: Gaurav K Singh <[e-mail]>
> > > > Signed-off-by: Manasi Navare <[e-mail]>
> > > > Reviewed-by: Anusha Srivatsa <[e-mail]>
> > > > Acked-by: Jani Nikula <[e-mail]>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c |  20 +++-
> > > >  drivers/gpu/drm/i915/intel_display.h |   3 +-
> > > >  drivers/gpu/drm/i915/intel_dp.c      | 170 ++++++++++++++++++++++-----
> > > >  drivers/gpu/drm/i915/intel_dp_mst.c  |   2 +-
> > > >  4 files changed, 155 insertions(+), 40 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > > index fe045abb6472..18737bd82b68 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -6434,7 +6434,7 @@ static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
> > > >  
> > > >  	pipe_config->fdi_lanes = lane;
> > > >  
> > > > -	intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
> > > > +	intel_link_compute_m_n(pipe_config->pipe_bpp, 0, lane, fdi_dotclock,
> > > >  			       link_bw, &pipe_config->fdi_m_n, false);
> > > >  
> > > >  	ret = ironlake_check_fdi_lanes(dev, intel_crtc->pipe, pipe_config);
> > > > @@ -6671,17 +6671,25 @@ static void compute_m_n(unsigned int m, unsigned int n,
> > > >  }
> > > >  
> > > >  void
> > > > -intel_link_compute_m_n(int bits_per_pixel, int nlanes,
> > > > +intel_link_compute_m_n(int bits_per_pixel, uint16_t compressed_bpp,
> > > > +		       int nlanes,
> > > >  		       int pixel_clock, int link_clock,
> > > >  		       struct intel_link_m_n *m_n,
> > > >  		       bool constant_n)
> > > >  {
> > > >  	m_n->tu = 64;
> > > >  
> > > > -	compute_m_n(bits_per_pixel * pixel_clock,
> > > > -		    link_clock * nlanes * 8,
> > > > -		    &m_n->gmch_m, &m_n->gmch_n,
> > > > -		    constant_n);
> > > > +	/* For DSC, Data M/N calculation uses compressed BPP */
> > > > +	if (compressed_bpp)
> > > > +		compute_m_n(compressed_bpp * pixel_clock,
> > > > +			    link_clock * nlanes * 8,
> > > > +			    &m_n->gmch_m, &m_n->gmch_n,
> > > > +			    constant_n);
> > > > +	else
> > > > +		compute_m_n(bits_per_pixel * pixel_clock,
> > > > +			    link_clock * nlanes * 8,
> > > > +			    &m_n->gmch_m, &m_n->gmch_n,
> > > > +			    constant_n);
> > > >  
> > > >  	compute_m_n(pixel_clock, link_clock,
> > > >  		    &m_n->link_m, &m_n->link_n,
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > > > index 5d50decbcbb5..b0b23e1e9392 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.h
> > > > +++ b/drivers/gpu/drm/i915/intel_display.h
> > > > @@ -407,7 +407,8 @@ struct intel_link_m_n {
> > > >  	     (__i)++) \
> > > >  		for_each_if(plane)
> > > >  
> > > > -void intel_link_compute_m_n(int bpp, int nlanes,
> > > > +void intel_link_compute_m_n(int bpp, uint16_t compressed_bpp,
> > > > +			    int nlanes,
> > > >  			    int pixel_clock, int link_clock,
> > > >  			    struct intel_link_m_n *m_n,
> > > >  			    bool constant_n);
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 6f66a38ba0b2..a88f9371dd32 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -47,6 +47,8 @@
> > > >  
> > > >  /* DP DSC small joiner has 2 FIFOs each of 640 x 6 bytes */
> > > >  #define DP_DSC_MAX_SMALL_JOINER_RAM_BUFFER	61440
> > > > +#define DP_DSC_MIN_SUPPORTED_BPC		8
> > > > +#define DP_DSC_MAX_SUPPORTED_BPC		10
> > > >  
> > > >  /* DP DSC throughput values used for slice count calculations KPixels/s */
> > > >  #define DP_DSC_PEAK_PIXEL_RATE			2720000
> > > > @@ -1924,6 +1926,16 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
> > > >  		}
> > > >  	}
> > > >  
> > > > +	/* If DSC is supported, use the max value reported by panel */
> > > > +	if (INTEL_GEN(dev_priv) >= 10 &&
> > > > +	    drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) {
> > > > +		bpc = min_t(u8,
> > > > +			    drm_dp_dsc_sink_max_color_depth(intel_dp->dsc_dpcd),
> > > > +			    DP_DSC_MAX_SUPPORTED_BPC);
> > > > +		if (bpc)
> > > > +			bpp = 3 * bpc;
> > > 
> > > This seems like it should be 'bpp = min(bpp, 3*bpc)'. 
> > > Otherwise we may bump the bpp above a limit we already established earlier.
> > > 
> > > > +	}
> > > > +
> > > >  	return bpp;
> > > >  }
> > > >  
> > > > @@ -1984,14 +1996,11 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
> > > >  				link_clock = intel_dp->common_rates[clock];
> > > >  				link_avail = intel_dp_max_data_rate(link_clock,
> > > >  								    lane_count);
> > > > -
> > > > -				if (mode_rate <= link_avail) {
> > > > -					pipe_config->lane_count = lane_count;
> > > > -					pipe_config->pipe_bpp = bpp;
> > > > -					pipe_config->port_clock = link_clock;
> > > > -
> > > > +				pipe_config->lane_count = lane_count;
> > > > +				pipe_config->pipe_bpp = bpp;
> > > > +				pipe_config->port_clock = link_clock;
> > > > +				if (mode_rate <= link_avail)
> > > >  					return true;
> > > 
> > > Why do we need to assign these if we don't accept the configuration?
> > > 
> > > > -				}
> > > >  			}
> > > >  		}
> > > >  	}
> > > > @@ -2020,14 +2029,11 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
> > > >  				link_clock = intel_dp->common_rates[clock];
> > > >  				link_avail = intel_dp_max_data_rate(link_clock,
> > > >  								    lane_count);
> > > > -
> > > > -				if (mode_rate <= link_avail) {
> > > > -					pipe_config->lane_count = lane_count;
> > > > -					pipe_config->pipe_bpp = bpp;
> > > > -					pipe_config->port_clock = link_clock;
> > > > -
> > > > +				pipe_config->lane_count = lane_count;
> > > > +				pipe_config->pipe_bpp = bpp;
> > > > +				pipe_config->port_clock = link_clock;
> > > > +				if (mode_rate <= link_avail)
> > > >  					return true;
> > > > -				}
> > > >  			}
> > > >  		}
> > > >  	}
> > > > @@ -2035,14 +2041,88 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
> > > >  	return false;
> > > >  }
> > > >  
> > > > +static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> > > > +					struct intel_crtc_state *pipe_config,
> > > > +					struct link_config_limits *limits)
> > > > +{
> > > > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > > > +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > > > +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> > > > +	enum pipe pipe = to_intel_crtc(pipe_config->base.crtc)->pipe;
> > > > +	u16 dsc_max_output_bpp = 0;
> > > > +	u8 dsc_dp_slice_count = 0;
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) < 10 ||
> > > > +	    !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))
> > > > +		return false;
> > > > +
> > > > +	/* DP DSC only supported on Pipe B and C */
> > > > +	if (pipe == PIPE_A && !intel_dp_is_edp(intel_dp))
> > > > +		return false;
> > > 
> > > Do we mean 'transcoder == EDP || transcoder == B || transcoder == C' ?
> > > Or maybe 'transcoder != A' for short. I guess this will need to adjusted
> > > for the next platform anyway so making the assumption that transcoder A
> > > is the only one that can't do DSC is fine.
> > > 
> > > This whole thing seems like a good helper function.
> > > intel_dp_source_supports_dsc() or something like that. And then we
> > > could have intel_dp_supports_dsc() which just calls that +
> > > drm_dp_sink_supports_dsc().
> >
> 
> So intel_dp_source_supports_dsc will have platform checks, but for it to do
> transcoder check it would also need crtc_state as input.

Indeed. Do we have that in all the places we need this?

> 
> And yes intel_dp_supports_dsc() can call this and drm_dp_sink_supports_dsc()
>  
> > Another confusion about these checks is glk. Some other places seem
> > to indicate that glk has DSC, but then code like this here doesn't 
> > accept glk. What's up with that?
> 
> This is because on GLK, VDSC is only supported on eDP and not on external DP

But we don't consider DSC when computing any of the link params and
whatnot. If that's intentional then there is something rather
non-obvious about how all this is meant to work. I don't think there
really should be that much of a difference between DP and eDP when
it comes to DSC.

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fe045abb6472..18737bd82b68 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6434,7 +6434,7 @@  static int ironlake_fdi_compute_config(struct intel_crtc *intel_crtc,
 
 	pipe_config->fdi_lanes = lane;
 
-	intel_link_compute_m_n(pipe_config->pipe_bpp, lane, fdi_dotclock,
+	intel_link_compute_m_n(pipe_config->pipe_bpp, 0, lane, fdi_dotclock,
 			       link_bw, &pipe_config->fdi_m_n, false);
 
 	ret = ironlake_check_fdi_lanes(dev, intel_crtc->pipe, pipe_config);
@@ -6671,17 +6671,25 @@  static void compute_m_n(unsigned int m, unsigned int n,
 }
 
 void
-intel_link_compute_m_n(int bits_per_pixel, int nlanes,
+intel_link_compute_m_n(int bits_per_pixel, uint16_t compressed_bpp,
+		       int nlanes,
 		       int pixel_clock, int link_clock,
 		       struct intel_link_m_n *m_n,
 		       bool constant_n)
 {
 	m_n->tu = 64;
 
-	compute_m_n(bits_per_pixel * pixel_clock,
-		    link_clock * nlanes * 8,
-		    &m_n->gmch_m, &m_n->gmch_n,
-		    constant_n);
+	/* For DSC, Data M/N calculation uses compressed BPP */
+	if (compressed_bpp)
+		compute_m_n(compressed_bpp * pixel_clock,
+			    link_clock * nlanes * 8,
+			    &m_n->gmch_m, &m_n->gmch_n,
+			    constant_n);
+	else
+		compute_m_n(bits_per_pixel * pixel_clock,
+			    link_clock * nlanes * 8,
+			    &m_n->gmch_m, &m_n->gmch_n,
+			    constant_n);
 
 	compute_m_n(pixel_clock, link_clock,
 		    &m_n->link_m, &m_n->link_n,
diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
index 5d50decbcbb5..b0b23e1e9392 100644
--- a/drivers/gpu/drm/i915/intel_display.h
+++ b/drivers/gpu/drm/i915/intel_display.h
@@ -407,7 +407,8 @@  struct intel_link_m_n {
 	     (__i)++) \
 		for_each_if(plane)
 
-void intel_link_compute_m_n(int bpp, int nlanes,
+void intel_link_compute_m_n(int bpp, uint16_t compressed_bpp,
+			    int nlanes,
 			    int pixel_clock, int link_clock,
 			    struct intel_link_m_n *m_n,
 			    bool constant_n);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 6f66a38ba0b2..a88f9371dd32 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -47,6 +47,8 @@ 
 
 /* DP DSC small joiner has 2 FIFOs each of 640 x 6 bytes */
 #define DP_DSC_MAX_SMALL_JOINER_RAM_BUFFER	61440
+#define DP_DSC_MIN_SUPPORTED_BPC		8
+#define DP_DSC_MAX_SUPPORTED_BPC		10
 
 /* DP DSC throughput values used for slice count calculations KPixels/s */
 #define DP_DSC_PEAK_PIXEL_RATE			2720000
@@ -1924,6 +1926,16 @@  static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 		}
 	}
 
+	/* If DSC is supported, use the max value reported by panel */
+	if (INTEL_GEN(dev_priv) >= 10 &&
+	    drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) {
+		bpc = min_t(u8,
+			    drm_dp_dsc_sink_max_color_depth(intel_dp->dsc_dpcd),
+			    DP_DSC_MAX_SUPPORTED_BPC);
+		if (bpc)
+			bpp = 3 * bpc;
+	}
+
 	return bpp;
 }
 
@@ -1984,14 +1996,11 @@  intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
 				link_clock = intel_dp->common_rates[clock];
 				link_avail = intel_dp_max_data_rate(link_clock,
 								    lane_count);
-
-				if (mode_rate <= link_avail) {
-					pipe_config->lane_count = lane_count;
-					pipe_config->pipe_bpp = bpp;
-					pipe_config->port_clock = link_clock;
-
+				pipe_config->lane_count = lane_count;
+				pipe_config->pipe_bpp = bpp;
+				pipe_config->port_clock = link_clock;
+				if (mode_rate <= link_avail)
 					return true;
-				}
 			}
 		}
 	}
@@ -2020,14 +2029,11 @@  intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
 				link_clock = intel_dp->common_rates[clock];
 				link_avail = intel_dp_max_data_rate(link_clock,
 								    lane_count);
-
-				if (mode_rate <= link_avail) {
-					pipe_config->lane_count = lane_count;
-					pipe_config->pipe_bpp = bpp;
-					pipe_config->port_clock = link_clock;
-
+				pipe_config->lane_count = lane_count;
+				pipe_config->pipe_bpp = bpp;
+				pipe_config->port_clock = link_clock;
+				if (mode_rate <= link_avail)
 					return true;
-				}
 			}
 		}
 	}
@@ -2035,14 +2041,88 @@  intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
 	return false;
 }
 
+static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
+					struct intel_crtc_state *pipe_config,
+					struct link_config_limits *limits)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
+	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
+	enum pipe pipe = to_intel_crtc(pipe_config->base.crtc)->pipe;
+	u16 dsc_max_output_bpp = 0;
+	u8 dsc_dp_slice_count = 0;
+
+	if (INTEL_GEN(dev_priv) < 10 ||
+	    !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))
+		return false;
+
+	/* DP DSC only supported on Pipe B and C */
+	if (pipe == PIPE_A && !intel_dp_is_edp(intel_dp))
+		return false;
+
+	/* DSC not supported for DSC sink BPC < 8 */
+	if (limits->max_bpp < 3 * DP_DSC_MIN_SUPPORTED_BPC) {
+		DRM_DEBUG_KMS("No DSC support for less than 8bpc\n");
+		return false;
+	}
+
+	if (intel_dp_is_edp(intel_dp)) {
+		pipe_config->dsc_params.compressed_bpp =
+			drm_edp_dsc_sink_output_bpp(intel_dp->dsc_dpcd) >> 4;
+		pipe_config->dsc_params.slice_count =
+			drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
+							true);
+	} else {
+		dsc_max_output_bpp =
+			intel_dp_dsc_get_output_bpp(pipe_config->port_clock,
+						    pipe_config->lane_count,
+						    adjusted_mode->crtc_clock,
+						    adjusted_mode->crtc_hdisplay);
+		dsc_dp_slice_count =
+			intel_dp_dsc_get_slice_count(intel_dp,
+						     adjusted_mode->crtc_clock,
+						     adjusted_mode->crtc_hdisplay);
+		if (!(dsc_max_output_bpp && dsc_dp_slice_count)) {
+			DRM_DEBUG_KMS("Compressed BPP/Slice Count not supported\n");
+			return false;
+		}
+		pipe_config->dsc_params.compressed_bpp = dsc_max_output_bpp >> 4;
+		pipe_config->dsc_params.slice_count = dsc_dp_slice_count;
+	}
+	/*
+	 * VDSC engine operates at 1 Pixel per clock, so if peak pixel rate
+	 * is greater than the maximum Cdclock and if slice count is even
+	 * then we need to use 2 VDSC instances.
+	 */
+	pipe_config->dsc_params.dsc_split = false;
+	if (adjusted_mode->crtc_clock > dev_priv->max_cdclk_freq) {
+		if (pipe_config->dsc_params.slice_count > 1) {
+			pipe_config->dsc_params.dsc_split = true;
+		} else {
+			DRM_DEBUG_KMS("Cannot split stream to use 2 VDSC instances\n");
+			return false;
+		}
+	}
+	pipe_config->dsc_params.compression_enable = true;
+	DRM_DEBUG_KMS("DP DSC computed with Input Bpp = %d "
+		      "Compressed Bpp = %d Slice Count = %d\n",
+		      pipe_config->pipe_bpp,
+		      pipe_config->dsc_params.compressed_bpp,
+		      pipe_config->dsc_params.slice_count);
+
+	return true;
+}
+
 static bool
 intel_dp_compute_link_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config)
 {
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 	struct link_config_limits limits;
 	int common_len;
+	bool ret = false;
 
 	common_len = intel_dp_common_len_rate_limit(intel_dp,
 						    intel_dp->max_link_rate);
@@ -2056,7 +2136,9 @@  intel_dp_compute_link_config(struct intel_encoder *encoder,
 	limits.min_lane_count = 1;
 	limits.max_lane_count = intel_dp_max_lane_count(intel_dp);
 
-	limits.min_bpp = 6 * 3;
+	limits.min_bpp = (INTEL_GEN(dev_priv) >= 10 &&
+			  drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd)) ?
+		DP_DSC_MIN_SUPPORTED_BPC * 3 : 6 * 3;
 	limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
 
 	if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) {
@@ -2081,7 +2163,7 @@  intel_dp_compute_link_config(struct intel_encoder *encoder,
 		      intel_dp->common_rates[limits.max_clock],
 		      limits.max_bpp, adjusted_mode->crtc_clock);
 
-	if (intel_dp_is_edp(intel_dp)) {
+	if (intel_dp_is_edp(intel_dp))
 		/*
 		 * Optimize for fast and narrow. eDP 1.3 section 3.3 and eDP 1.4
 		 * section A.1: "It is recommended that the minimum number of
@@ -2091,26 +2173,48 @@  intel_dp_compute_link_config(struct intel_encoder *encoder,
 		 * Note that we use the max clock and lane count for eDP 1.3 and
 		 * earlier, and fast vs. wide is irrelevant.
 		 */
-		if (!intel_dp_compute_link_config_fast(intel_dp, pipe_config,
-						       &limits))
-			return false;
-	} else {
+		ret = intel_dp_compute_link_config_fast(intel_dp, pipe_config,
+							&limits);
+	else
 		/* Optimize for slow and wide. */
-		if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config,
-						       &limits))
+		ret = intel_dp_compute_link_config_wide(intel_dp, pipe_config,
+							&limits);
+
+	/* enable compression if the mode doesn't fit available BW */
+	if (!ret) {
+		DRM_DEBUG_KMS("DP required Link rate %i does not fit available %i\n",
+			      intel_dp_link_required(adjusted_mode->crtc_clock,
+						     pipe_config->pipe_bpp),
+			      intel_dp_max_data_rate(pipe_config->port_clock,
+						     pipe_config->lane_count));
+
+		if (!intel_dp_dsc_compute_config(intel_dp, pipe_config,
+						 &limits))
 			return false;
 	}
 
-	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
-		      pipe_config->lane_count, pipe_config->port_clock,
-		      pipe_config->pipe_bpp);
+	if (pipe_config->dsc_params.compression_enable) {
+		DRM_DEBUG_KMS("DP lane count %d clock %d Input bpp %d Compressed bpp %d\n",
+			      pipe_config->lane_count, pipe_config->port_clock,
+			      pipe_config->pipe_bpp,
+			      pipe_config->dsc_params.compressed_bpp);
 
-	DRM_DEBUG_KMS("DP link rate required %i available %i\n",
-		      intel_dp_link_required(adjusted_mode->crtc_clock,
-					     pipe_config->pipe_bpp),
-		      intel_dp_max_data_rate(pipe_config->port_clock,
-					     pipe_config->lane_count));
+		DRM_DEBUG_KMS("DP link rate required %i available %i\n",
+			      intel_dp_link_required(adjusted_mode->crtc_clock,
+						     pipe_config->dsc_params.compressed_bpp),
+			      intel_dp_max_data_rate(pipe_config->port_clock,
+						     pipe_config->lane_count));
+	} else {
+		DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
+			      pipe_config->lane_count, pipe_config->port_clock,
+			      pipe_config->pipe_bpp);
 
+		DRM_DEBUG_KMS("DP link rate required %i available %i\n",
+			      intel_dp_link_required(adjusted_mode->crtc_clock,
+						     pipe_config->pipe_bpp),
+			      intel_dp_max_data_rate(pipe_config->port_clock,
+						     pipe_config->lane_count));
+	}
 	return true;
 }
 
@@ -2194,7 +2298,9 @@  intel_dp_compute_config(struct intel_encoder *encoder,
 			intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_LIMITED;
 	}
 
-	intel_link_compute_m_n(pipe_config->pipe_bpp, pipe_config->lane_count,
+	intel_link_compute_m_n(pipe_config->pipe_bpp,
+			       pipe_config->dsc_params.compressed_bpp,
+			       pipe_config->lane_count,
 			       adjusted_mode->crtc_clock,
 			       pipe_config->port_clock,
 			       &pipe_config->dp_m_n,
@@ -2203,7 +2309,7 @@  intel_dp_compute_config(struct intel_encoder *encoder,
 	if (intel_connector->panel.downclock_mode != NULL &&
 		dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) {
 			pipe_config->has_drrs = true;
-			intel_link_compute_m_n(pipe_config->pipe_bpp,
+			intel_link_compute_m_n(pipe_config->pipe_bpp, 0,
 					       pipe_config->lane_count,
 					       intel_connector->panel.downclock_mode->clock,
 					       pipe_config->port_clock,
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 8b71d64ebd9d..e02caedd443c 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -90,7 +90,7 @@  static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 		}
 	}
 
-	intel_link_compute_m_n(bpp, lane_count,
+	intel_link_compute_m_n(bpp, 0, lane_count,
 			       adjusted_mode->crtc_clock,
 			       pipe_config->port_clock,
 			       &pipe_config->dp_m_n,