Re: [PATCH 5/5] drm/i915: Add YCBCR 4:2:0 support for LSPCON

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

On Tue, Nov 14, 2017 at 08:47:26PM +0530, Shashank Sharma wrote:
> LSPCON chips are capable of generating YCBCR 4:2:0 outputs, if asked
> nicely :). In order to generate YCBCR 4:2:0 outputs, a source must:
> - send YCBCR 4:4:4 signals to LSPCON
> - program color space as 4:2:0 in AVI infoframes
> 
> This will indicate LSPCON FW to start scaling down from YCBCR 4:4:4
> and generate YCBCR 4:2:0 output. Unlike HDMI 2.0 native case, as the
> scaling is done by LSPCON device, we need not to reserve a scaler for
> 4:2:0 outputs.
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  2 ++
>  drivers/gpu/drm/i915/intel_ddi.c     |  8 ++++++++
>  drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++----
>  drivers/gpu/drm/i915/intel_dp.c      |  7 ++++++-
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  drivers/gpu/drm/i915/intel_lspcon.c  | 22 ++++++++++++++++++++++
>  6 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f0f8f60..ea6ef5e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -8495,6 +8495,8 @@ enum skl_power_gate {
>  #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC)
>  
>  #define  TRANS_MSA_SYNC_CLK		(1<<0)
> +#define  TRANS_MSA_SAMPLING_444        (2<<1)
> +#define  TRANS_MSA_CLRSP_YCBCR		(2<<3)
>  #define  TRANS_MSA_6_BPC		(0<<5)
>  #define  TRANS_MSA_8_BPC		(1<<5)
>  #define  TRANS_MSA_10_BPC		(2<<5)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 9e2ab02..3fd839d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1499,6 +1499,14 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>  		break;
>  	}
>  
> +	/*
> +	 * To get YCBCR 420 output from LSPCON, we should send YCBCR 444
> +	 * signals. And as per DP 1.2 spec section 2.3.4.3 while sending
> +	 * YCBCR 444 signals we should program MSA MISC1/0 fields with
> +	 * colorspace information.
> +	 */
> +	if (crtc_state->lspcon_active && crtc_state->ycbcr420)

I think it would be much cleaner if we just set
crtc_state->ycbcr444=true here instead of crtc_state->ycbcr420=true,
and then have a separate flag to control the 444->420 downsampling in
the DP device/LSPCON. Not quite sure what we should call that, maybe just
somehting like crtc_state->ycbcr444_to_ycbcr420. That way you'll pretty
much have implemented ycbcr444 support for us apart from the uapi to
flip it on.

Looks like state readout support is also missing from this patch.

> +		temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
>  	I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 737de25..787119b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4638,7 +4638,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  	 */
>  	need_scaling = src_w != dst_w || src_h != dst_h;
>  
> -	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
> +	if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX &&
> +		!crtc_state->lspcon_active)
>  		need_scaling = true;
>  
>  	/*
> @@ -8133,9 +8134,15 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>  			val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>  
>  		if (config->ycbcr420) {
> -			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
> -				PIPEMISC_YUV420_ENABLE |
> -				PIPEMISC_YUV420_MODE_FULL_BLEND;
> +			val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
> +			/*
> +			 * LSPCON doesn't need scaling/blending to be done in
> +			 * pipe. It just needs YCBCR444 input and proper AVI
> +			 * infoframes for 4:2:0 output enabling.
> +			 */
> +			if (!config->lspcon_active)
> +				val |= PIPEMISC_YUV420_ENABLE |
> +				       PIPEMISC_YUV420_MODE_FULL_BLEND;
>  		}
>  
>  		I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index fa9e5e6..1d4b669 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1657,8 +1657,13 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  						    intel_dp->max_link_rate);
>  
>  	/* LSPCON needs special handling to drive YCBCR420 outputs */
> -	if (lspcon->active)
> +	if (lspcon->active) {
> +		struct drm_connector *connector = &intel_connector->base;
> +
>  		pipe_config->lspcon_active = true;
> +		pipe_config->ycbcr420 = lspcon_ycbcr420_config(connector,
> +							       pipe_config);
> +	}
>  
>  	/* No common link rates between source and sink */
>  	WARN_ON(common_len <= 0);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a468dd6..f271967 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>  			   const struct drm_connector_state *conn_state);
>  bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
>  			      const struct intel_crtc_state *pipe_config);
> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
> +			     struct intel_crtc_state *config);
>  
>  /* intel_pipe_crc.c */
>  int intel_pipe_crc_create(struct drm_minor *minor);
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index 77f0687..ca77f50 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -180,6 +180,26 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
>  	return true;
>  }
>  
> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
> +			     struct intel_crtc_state *config)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +	struct drm_display_mode *mode = &config->base.adjusted_mode;
> +
> +	if (drm_mode_is_420_only(info, mode)) {
> +
> +		if (!connector->ycbcr_420_allowed) {
> +			DRM_ERROR("Platform doesn't support YCBCR420 output\n");
> +			return false;
> +		}
> +
> +		config->port_clock /= 2;
> +		return true;
> +	}
> +
> +	return false;
> +}
> +
>  static bool lspcon_probe(struct intel_lspcon *lspcon)
>  {
>  	int retry;
> @@ -516,6 +536,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>  	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
>  	struct drm_device *dev = intel_dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> +	struct drm_connector *connector = &dp->attached_connector->base;
>  
>  	if (!HAS_LSPCON(dev_priv)) {
>  		DRM_ERROR("LSPCON is not supported on this platform\n");
> @@ -540,6 +561,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>  		return false;
>  	}
>  
> +	connector->ycbcr_420_allowed = true;
>  	lspcon->active = true;
>  	DRM_DEBUG_KMS("Success: LSPCON init\n");
>  	return true;
> -- 
> 2.7.4

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