Re: [RFC v2 5/6] drm/i915/dp: Update pipe_bpp for DP YCbCr4:2:0 outputs

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

On Thu, Feb 21, 2019 at 09:27:30PM +0200, Gwan-gyeong Mun wrote:
> pipe_bpp value was assumed RGB therefore it was multiplied with 3.
> But YCbCr 4:2:0 requires multiplier value to 1.5 therefore it divides
> pipe_bpp to 2.
>  - RGB bpp = bpc x 3
>  - YCbCr 4:2:0 bpp = bpc x 1.5
> 
> v2: Minor style fix.
> 
> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c |  7 +++++-
>  drivers/gpu/drm/i915/intel_dp.c  | 41 ++++++++++++++++++++++++++------
>  2 files changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 30825ae67903..801fc9463306 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1696,6 +1696,7 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>  	u32 temp;
> +	int bpp;
>  
>  	if (!intel_crtc_has_dp_encoder(crtc_state))
>  		return;
> @@ -1707,7 +1708,11 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>  	if (crtc_state->limited_color_range)
>  		temp |= TRANS_MSA_CEA_RANGE;
>  
> -	switch (crtc_state->pipe_bpp) {
> +	bpp = crtc_state->pipe_bpp;
> +	if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR420)
> +		bpp *= 2;
> +
> +	switch (bpp) {
>  	case 18:
>  		temp |= TRANS_MSA_6_BPC;
>  		break;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a2f70b7a81de..50a270a5f4bd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1767,12 +1767,13 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>  	struct intel_connector *intel_connector = intel_dp->attached_connector;
>  	int bpp, bpc;
> +	int bpp_divider = pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ? 2 : 1;
>  
>  	bpp = pipe_config->pipe_bpp;
>  	bpc = drm_dp_downstream_max_bpc(intel_dp->dpcd, intel_dp->downstream_ports);
>  
>  	if (bpc > 0)
> -		bpp = min(bpp, 3*bpc);
> +		bpp = min(bpp, 3 * bpc / bpp_divider);
>  
>  	if (intel_dp_is_edp(intel_dp)) {
>  		/* Get bpp from vbt only for panels that dont have bpp in edid */
> @@ -1793,12 +1794,14 @@ intel_dp_adjust_compliance_config(struct intel_dp *intel_dp,
>  				  struct intel_crtc_state *pipe_config,
>  				  struct link_config_limits *limits)
>  {
> +	int bpp_divider = pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ? 2 : 1;
> +
>  	/* For DP Compliance we override the computed bpp for the pipe */
>  	if (intel_dp->compliance.test_data.bpc != 0) {
> -		int bpp = 3 * intel_dp->compliance.test_data.bpc;
> +		int bpp = 3 * intel_dp->compliance.test_data.bpc / bpp_divider;
>  
>  		limits->min_bpp = limits->max_bpp = bpp;
> -		pipe_config->dither_force_disable = bpp == 6 * 3;
> +		pipe_config->dither_force_disable = bpp == 6 * 3 / bpp_divider;
>  
>  		DRM_DEBUG_KMS("Setting pipe_bpp to %d\n", bpp);
>  	}
> @@ -1832,8 +1835,9 @@ intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>  	int bpp, clock, lane_count;
>  	int mode_rate, link_clock, link_avail;
> +	int bpp_divider = pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ? 2 : 1;
>  
> -	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
> +	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3 / bpp_divider) {
>  		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
>  						   bpp);
>  
> @@ -1868,8 +1872,9 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>  	int bpp, clock, lane_count;
>  	int mode_rate, link_clock, link_avail;
> +	int bpp_divider = pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ? 2 : 1;
>  
> -	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
> +	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3 / bpp_divider) {
>  		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
>  						   bpp);
>  
> @@ -2015,6 +2020,7 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  	struct link_config_limits limits;
>  	int common_len;
>  	int ret;
> +	int bpp_divider = pipe_config->output_format == INTEL_OUTPUT_FORMAT_YCBCR420 ? 2 : 1;
>  
>  	common_len = intel_dp_common_len_rate_limit(intel_dp,
>  						    intel_dp->max_link_rate);
> @@ -2028,7 +2034,7 @@ 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 = 6 * 3 / bpp_divider;
>  	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) {
> @@ -2116,6 +2122,11 @@ intel_dp_ycbcr420_config(struct drm_connector *connector,
>  	}
>  
>  	config->output_format = INTEL_OUTPUT_FORMAT_YCBCR420;
> +	/* pipe_bpp value was assumed RGB therefore it was multiplied
> +	 * with 3. But YCbCr 4:2:0 requires multiplier value to 1.5
> +	 * therefore it divides pipe_bpp to 2.
> +	 */
> +	config->pipe_bpp /= 2;

This seems wrong to me. The pipe is still running at the full bpp
(well pipe_bpp in general is a bit of a incorrect concept, but we'll
ignbore that issue for now). More importantly this is not how we
did it for HDMI.

Is there anywhere else besides the link bw computation where you
actually need to know the actual bpc going over the link? If that's
the only place then we should leave pipe_bpp alone and just adjust
there.

>  
>  	/* YCBCR 420 output conversion needs a scaler */
>  	if (skl_update_scaler_crtc(config)) {
> @@ -4453,7 +4464,23 @@ intel_pixel_encoding_setup_vsc(struct intel_dp *intel_dp,
>  	 * 011b = 12bpc.
>  	 * 100b = 16bpc.
>  	 */
> -	vsc_sdp.DB17 = 0x1;
> +	switch (crtc_state->pipe_bpp) {
> +	case 12: /* 8bpc */
> +		vsc_sdp.DB17 = 0x1;
> +		break;
> +	case 15: /* 10bpc */
> +		vsc_sdp.DB17 = 0x2;
> +		break;
> +	case 18: /* 12bpc */
> +		vsc_sdp.DB17 = 0x3;
> +		break;
> +	case 24: /* 16bpc */
> +		vsc_sdp.DB17 = 0x4;
> +		break;
> +	default:
> +		DRM_DEBUG_KMS("Invalid bpp value '%d'\n", crtc_state->pipe_bpp);
> +		break;
> +	}
>  
>  	/*
>  	 * Content Type (Bits 2:0)
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx