RE: [PATCH v8 07/19] drm/i915/dp: Compute DSC pipe config in atomic check

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx]
>Sent: Tuesday, November 6, 2018 6:43 AM
>To: Navare, Manasi D <manasi.d.navare@xxxxxxxxx>
>Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx; Jani Nikula
><jani.nikula@xxxxxxxxxxxxxxx>; Srivatsa, Anusha <anusha.srivatsa@xxxxxxxxx>;
>Singh, Gaurav K <gaurav.k.singh@xxxxxxxxx>
>Subject: Re: [PATCH v8 07/19] drm/i915/dp: Compute DSC pipe config in atomic
>check
>
>On Fri, Nov 02, 2018 at 07:09:03PM -0700, Manasi Navare wrote:
>> On Fri, Nov 02, 2018 at 02:31:26PM -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.
>> >
>> > v11:
>> > * Const crtc_state, reject DSC on DP without FEC (Ville)
>> > * Dont set dsc_split to false (Ville)
>> > v10:
>> > * Add a helper for dp_dsc support (Ville)
>> > * Set pipe_config to max bpp, link params for DSC for now (Ville)
>> > * Compute bpp - use dp dsc support helper (Ville)
>> > 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 <jani.nikula@xxxxxxxxxxxxxxx>
>> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
>> > Cc: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
>> > Cc: Gaurav K Singh <gaurav.k.singh@xxxxxxxxx>
>> > Signed-off-by: Manasi Navare <manasi.d.navare@xxxxxxxxx>
>> > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
>> > Acked-by: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c |  20 +++-
>> >  drivers/gpu/drm/i915/intel_display.h |   3 +-
>> >  drivers/gpu/drm/i915/intel_dp.c      | 167 ++++++++++++++++++++++++---
>> >  drivers/gpu/drm/i915/intel_dp_mst.c  |   2 +-
>> >  4 files changed, 166 insertions(+), 26 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c
>> > b/drivers/gpu/drm/i915/intel_display.c
>> > index b219d5858160..477e53c37353 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -6442,7 +6442,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); @@ -6679,17 +6679,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 b39b4bda8e40..58326fc9d935
>> > 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
>> > @@ -1840,6 +1842,29 @@ struct link_config_limits {
>> >  	int min_bpp, max_bpp;
>> >  };
>> >
>> > +static bool intel_dp_source_supports_dsc(struct intel_dp *intel_dp,
>> > +					 const struct intel_crtc_state
>*pipe_config) {
>> > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>> > +
>> > +	/* FIXME: FEC needed for external DP until then reject DSC on DP */

If we want to check for FEC support in intel_dp_supports_dsc(), this FIXME should be moved from here.

Anusha 
>> > +	if (!intel_dp_is_edp(intel_dp))
>> > +		return false;
>> > +
>> > +	return INTEL_GEN(dev_priv) >= 10 &&
>> > +		pipe_config->cpu_transcoder != TRANSCODER_A; }
>> > +
>> > +static bool intel_dp_supports_dsc(struct intel_dp *intel_dp,
>> > +				  const struct intel_crtc_state *pipe_config) {
>> > +	if (!intel_dp_source_supports_dsc(intel_dp, pipe_config) ||
>> > +	    !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))
>> > +		return false;
>> > +
>> > +	return true;
>> > +}
>> > +
>> >  static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>> >  				struct intel_crtc_state *pipe_config)  { @@ -
>1863,6 +1888,15 @@
>> > static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>> >  		}
>> >  	}
>> >
>> > +	/* If DSC is supported, use the max value reported by panel */
>> > +	if (intel_dp_supports_dsc(intel_dp, pipe_config)) {
>> > +		bpc = min_t(u8,
>> > +			    drm_dp_dsc_sink_max_color_depth(intel_dp-
>>dsc_dpcd),
>> > +			    DP_DSC_MAX_SUPPORTED_BPC);
>> > +		if (bpc)
>> > +			bpp = min(bpp, 3*bpc);
>>
>> Ville, the problem with chosing the min between existing bpp and 3 *
>> bpc (computed from DSC dpcd) is that EDID based bpc might be lower and
>> in case DSC is supported, the bpc value should be based on the dsc
>> color depth and that should override the value obtained from EDID.
>> This was confirmed from the VDSC programming documents obtained from
>> HW architecture folks
>>
>> So i think it should still be if DSC supported use the bpc directly
>> from min_t(u8,
>> drm_dp_dsc_sink_max_color_depth(intel_dp-
>>dsc_dpcd),DP_DSC_MAX_SUPPORT
>> ED_BPC);
>
>Maybe
>
>bpc_min = something;
>bpc_max = something_else;
>bpp = clamp(bpp, bpc_min*3, bpc_max*3);
>
>Either that or we just return an error of the requested bpp is below the dsc
>minimum.
>
>>
>> Manasi
>>
>> > +	}
>> > +
>> >  	return bpp;
>> >  }
>> >
>> > @@ -1974,6 +2008,80 @@ 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,
>> > +					const 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;
>> > +	u16 dsc_max_output_bpp = 0;
>> > +	u8 dsc_dp_slice_count = 0;
>> > +
>> > +	if (!intel_dp_supports_dsc(intel_dp, pipe_config))
>> > +		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;
>> > +	}
>> > +
>> > +	/*
>> > +	 * For now enable DSC for max bpp, max link rate, max lane count.
>> > +	 * Optimize this later for the minimum possible link rate/lane count
>> > +	 * with DSC enabled for the requested mode.
>> > +	 */
>> > +	pipe_config->pipe_bpp = limits->max_bpp;
>> > +	pipe_config->port_clock = intel_dp->common_rates[limits->max_clock];
>> > +	pipe_config->lane_count = limits->max_lane_count;
>> > +
>> > +	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.
>> > +	 */
>> > +	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) @@ -1982,6
>+2090,7 @@
>> > intel_dp_compute_link_config(struct intel_encoder *encoder,
>> >  	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);
>@@ -1995,8 +2104,12 @@
>> > 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.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
>> > +	if (intel_dp_supports_dsc(intel_dp, pipe_config) &&
>> > +	    limits.max_bpp >= DP_DSC_MIN_SUPPORTED_BPC*3)
>> > +		limits.min_bpp = DP_DSC_MIN_SUPPORTED_BPC * 3;
>> > +	else
>> > +		limits.min_bpp = 6 * 3;
>> >
>> >  	if (intel_dp_is_edp(intel_dp) && intel_dp->edp_dpcd[0] < DP_EDP_14) {
>> >  		/*
>> > @@ -2020,7 +2133,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
>@@
>> > -2030,26 +2143,42 @@ 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) {
>> > +		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;
>> >  }
>> >
>> > @@ -2133,7 +2262,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,
>> > @@ -2142,7 +2273,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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux