Re: [PATCH 02/10] drm: i915: Get DSC capability from DP sink

[Manasi Navare <manasi.d.navare@intel.com>]

On Fri, Feb 23, 2018 at 09:25:45PM +0530, Gaurav K Singh wrote:
> Get decompression capabilities from DP sink by doing
> DPCD reads of different offsets as per eDP/DP specs.
> 
> Signed-off-by: Gaurav K Singh <gaurav.k.singh@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 167 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 167 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1868f73f730c..f494a851ff89 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5883,6 +5883,149 @@ void intel_edp_drrs_flush(struct drm_i915_private *dev_priv,
>  	return downclock_mode;
>  }
>  
> +static void intel_dp_sink_get_dsc_capability(struct intel_dp *intel_dp,
> +					struct dp_sink_dsc_caps *dp_dsc_caps)
> +{
> +	u8 rcbuffer_blocksize;
> +	u8 fec_dpcd;
> +	unsigned long line_buffer_bit_depth, sink_support_max_bpp_msb;
> +

Clear the previously cached dsc_dpcd before caching them again since it might still
have those from previous edp > 1.4 but we might not need them if current edp < 1.4

> +	/* VDSC is supported only for eDp v1.4 or higher, DPCD 0x00700 offset */
> +	if (intel_dp->edp_dpcd[0] < 0x03)

Use the #define DP_EDP_14 for the eDP version check of 1.4

> +		return;
> +
> +	/* Read DPCD 0x060 to 0x06a */
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_DSC_SUPPORT, intel_dp->dsc_dpcd,
> +			     sizeof(intel_dp->dsc_dpcd)) < 0)
> +		return;
> +
> +	dp_dsc_caps->is_dsc_supported = intel_dp->dsc_dpcd[0] &
> +					DP_DSC_DECOMPRESSION_IS_SUPPORTED;

Like I mentioned on patch 1/10 from the reviews I had got on my DSC patches,
no need to have is_dsc_supported field or dp_dsc_caps since the entire set of
DPCD registers is cached, this can be computed on the fly when needed.

> +
> +	if (!dp_dsc_caps->is_dsc_supported)
> +		return;
> +
> +	drm_dp_dpcd_readb(&intel_dp->aux, 0x090, &fec_dpcd);
> +	intel_dp->fec_dpcd = fec_dpcd;
> +
> +	/* For DP DSC, FEC support is must */

FEC support is only needed for DP DSC not eDP, but looks like currently this
function is only getting called on eDP init connector and not on all DP
hotplug processing. If its for DP then, even DP 1.4 version check needed.
Since here its only eDP, FEC is not mandatory infact no FEC support on eDP 1.4.

> +	if (!(intel_dp->fec_dpcd & 0x1))
> +		return;
> +
> +	/* No VDSC support for less than 8 BPC */
> +	if (intel_dp->dsc_dpcd[0xa] < DP_DSC_8_BPC)

Use some #define instead of 0xa ( I had used intel_dp->dsc_dpcd[DP_DSC_BPC - DP_DSC_SUPPORT])

> +		return;

What happens to the cached values when you return from this function in case of
no VDSC support. Shouldnt they be reset now that it cannot be implemented.

> +
> +	if (intel_dp->dsc_dpcd[0xa] & DP_DSC_8_BPC)
> +		DRM_INFO("8 Bits per color support\n");
> +	if (intel_dp->dsc_dpcd[0xa] & DP_DSC_10_BPC)
> +		DRM_INFO("10 Bits per color support\n");
> +	if (intel_dp->dsc_dpcd[0xa] & DP_DSC_12_BPC)
> +		DRM_INFO("12 Bits per color support\n");
> +
> +	dp_dsc_caps->dsc_major_ver = intel_dp->dsc_dpcd[1] & DP_DSC_MAJOR_MASK;
> +	dp_dsc_caps->dsc_minor_ver = (intel_dp->dsc_dpcd[1] &
> +				DP_DSC_MINOR_MASK) >> DP_DSC_MINOR_SHIFT;
> +
> +	rcbuffer_blocksize = intel_dp->dsc_dpcd[2] & 0x3;
> +
> +	switch (rcbuffer_blocksize) {
> +	case 0:
> +		dp_dsc_caps->rcbuffer_blocksize = 1;
> +		break;
> +	case 1:
> +		dp_dsc_caps->rcbuffer_blocksize = 4;
> +		break;
> +	case 2:
> +		dp_dsc_caps->rcbuffer_blocksize = 16;
> +		break;
> +	case 3:
> +		dp_dsc_caps->rcbuffer_blocksize = 64;
> +		break;
> +	default:
> +		break;
> +
> +	}
> +	dp_dsc_caps->rcbuffer_size_in_blocks = intel_dp->dsc_dpcd[3] + 1;
> +
> +	dp_dsc_caps->rcbuffer_size =
> +			dp_dsc_caps->rcbuffer_size_in_blocks *
> +			dp_dsc_caps->rcbuffer_blocksize * 1024 * 8;
> +

Move this rc buffer computation to a helper. (refer to my patch)

> +	dp_dsc_caps->slice_caps = intel_dp->dsc_dpcd[4];

get this again in a  helper. Refer to my patch.

> +	line_buffer_bit_depth = intel_dp->dsc_dpcd[5];
> +
> +	if (line_buffer_bit_depth == 8)
> +		dp_dsc_caps->line_buffer_bit_depth = intel_dp->dsc_dpcd[5];
> +	else
> +		dp_dsc_caps->line_buffer_bit_depth = intel_dp->dsc_dpcd[5] + 9;

Move these above to helpers and use #defines for these numerical values making it
more readable.

> +
> +	dp_dsc_caps->is_block_pred_supported = intel_dp->dsc_dpcd[6] &
> +					DP_DSC_BLK_PREDICTION_IS_SUPPORTED;
> +
> +	dp_dsc_caps->sink_support_max_bpp = intel_dp->dsc_dpcd[7];
> +	sink_support_max_bpp_msb = (intel_dp->dsc_dpcd[8] & 0x3) << 8;
> +	dp_dsc_caps->sink_support_max_bpp |= sink_support_max_bpp_msb;
> +
> +	dp_dsc_caps->color_format_caps = intel_dp->dsc_dpcd[9];
> +	dp_dsc_caps->color_depth_caps = intel_dp->dsc_dpcd[0xa];
> +}
> +
> +static void intel_dp_get_compression_data(struct intel_dp *intel_dp,
> +					struct dp_sink_dsc_caps dp_dsc_caps)
> +{
> +	if (!dp_dsc_caps.is_dsc_supported)
> +		return;

Directly compute from dsc_dpcd[0] & compression_supported

> +
> +	intel_dp->compr_params.compression_support =
> +						dp_dsc_caps.is_dsc_supported;
> +	intel_dp->compr_params.dsc_cfg.dsc_version_major =
> +						dp_dsc_caps.dsc_major_ver;
> +	intel_dp->compr_params.dsc_cfg.dsc_version_minor =
> +						dp_dsc_caps.dsc_minor_ver;
> +
> +	/* By default set bpc to 8 */
> +	intel_dp->compr_params.dsc_cfg.bits_per_component = 8;
> +
> +	/* Take the max for Bits per component */
> +	if (intel_dp->dsc_dpcd[0xa] & DP_DSC_8_BPC)
> +		intel_dp->compr_params.dsc_cfg.bits_per_component = 8;
> +	if (intel_dp->dsc_dpcd[0xa] & DP_DSC_10_BPC)
> +		intel_dp->compr_params.dsc_cfg.bits_per_component = 10;
> +	if (intel_dp->dsc_dpcd[0xa] & DP_DSC_12_BPC)
> +		intel_dp->compr_params.dsc_cfg.bits_per_component = 12;
> +

Move this to a separate helper computing max bpc from dsc_dpcd

> +	intel_dp->compr_params.compression_bpp =
> +					dp_dsc_caps.sink_support_max_bpp >> 4;

Compute in a helper (refer to my patch)

> +	intel_dp->compr_params.dsc_cfg.bits_per_pixel =
> +					dp_dsc_caps.sink_support_max_bpp;
> +	intel_dp->compr_params.dsc_cfg.convert_rgb = dp_dsc_caps.RGB_support;
> +	intel_dp->compr_params.dsc_cfg.enable422 = dp_dsc_caps.YCbCr422_support;
> +	intel_dp->compr_params.dsc_cfg.block_pred_enable =
> +					dp_dsc_caps.is_block_pred_supported;
> +
> +	/* Always try to enable 2 DSC instances, by default */
> +	intel_dp->compr_params.dsc_cfg.num_vdsc_instances = 2;
> +
> +	if (dp_dsc_caps.four_slice_per_line_support)
> +		intel_dp->compr_params.dsc_cfg.slice_count = 4;
> +	else if (dp_dsc_caps.two_slice_per_line_support)
> +		intel_dp->compr_params.dsc_cfg.slice_count = 2;
> +	else if (dp_dsc_caps.one_slice_per_line_support) {
> +		/*
> +		 * Cannot use 2 DSC engines simultaneously when
> +		 * slice per line support is only 1
> +		 */
> +		intel_dp->compr_params.dsc_cfg.slice_count = 1;
> +		intel_dp->compr_params.dsc_cfg.num_vdsc_instances = 1;
> +	} else
> +		DRM_INFO("Slice count not supported:%d\n",
> +							dp_dsc_caps.slice_caps);

Move the slice count computation to a helper (refer to my patch)

> +
> +	intel_dp->compr_params.dsc_cfg.line_buf_depth =
> +					dp_dsc_caps.line_buffer_bit_depth;
> +}
> +
>  static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  				     struct intel_connector *intel_connector)
>  {
> @@ -5892,6 +6035,7 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	struct drm_display_mode *fixed_mode = NULL;
>  	struct drm_display_mode *alt_fixed_mode = NULL;
>  	struct drm_display_mode *downclock_mode = NULL;
> +	struct dp_sink_dsc_caps sink_dp_dsc_caps = {0};
>  	bool has_dpcd;
>  	struct drm_display_mode *scan;
>  	struct edid *edid;
> @@ -5930,6 +6074,12 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  		goto out_vdd_off;
>  	}
>  
> +	/* Get DSC capability of DP sink */
> +	if (INTEL_GEN(dev_priv) >= 9) {
> +		intel_dp_sink_get_dsc_capability(intel_dp, &sink_dp_dsc_caps);
> +		intel_dp_get_compression_data(intel_dp, sink_dp_dsc_caps);
> +	}
> +

This can be moved to intel_edp_init_dpcd

>  	mutex_lock(&dev->mode_config.mutex);
>  	edid = drm_get_edid(connector, &intel_dp->aux.ddc);
>  	if (edid) {
> @@ -5968,6 +6118,23 @@ static bool intel_edp_init_connector(struct intel_dp *intel_dp,
>  	}
>  	mutex_unlock(&dev->mode_config.mutex);
>  
> +	if (intel_dp->compr_params.compression_support) {
> +		intel_dp->compr_params.dsc_cfg.pic_width = fixed_mode->hdisplay;
> +		intel_dp->compr_params.dsc_cfg.pic_height =
> +							fixed_mode->vdisplay;
> +		intel_dp->compr_params.dsc_cfg.slice_width = DIV_ROUND_UP(
> +				intel_dp->compr_params.dsc_cfg.pic_width,
> +				intel_dp->compr_params.dsc_cfg.slice_count);
> +
> +		/* slice height data is not available from dpcd */
> +		if (intel_dp->compr_params.dsc_cfg.pic_height % 8 == 0)
> +			intel_dp->compr_params.dsc_cfg.slice_height = 8;
> +		if (intel_dp->compr_params.dsc_cfg.pic_height % 4 == 0)
> +			intel_dp->compr_params.dsc_cfg.slice_height = 4;
> +		if (intel_dp->compr_params.dsc_cfg.pic_height % 2 == 0)
> +			intel_dp->compr_params.dsc_cfg.slice_height = 2;

This configuration should happen at compute_config time where we configure the pipe for
a specific mode, also configure dsc_config.
This dsc_config should be a intel_crtc_state field. Please refer to my patch following on M-L soon.

Manasi

> +	}
> +
>  	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>  		intel_dp->edp_notifier.notifier_call = edp_notify_handler;
>  		register_reboot_notifier(&intel_dp->edp_notifier);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx