Re: [PATCH v6 3/6] drm/i915/dp: Program VSC Header and DB for Pixel Encoding/Colorimetry Format

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

On Wed, May 08, 2019 at 11:17:54AM +0300, Gwan-gyeong Mun wrote:
> Function intel_pixel_encoding_setup_vsc handles vsc header and data block
> setup for pixel encoding / colorimetry format.
> 
> Setup VSC header and data block in function intel_pixel_encoding_setup_vsc
> for pixel encoding / colorimetry format as per dp 1.4a spec,
> section 2.2.5.7.1, table 2-119: VSC SDP Header Bytes, section 2.2.5.7.5,
> table 2-120:VSC SDP Payload for DB16 through DB18.
> 
> v2:
>   Minor style fix. [Maarten]
>   Refer to commit ids instead of patchwork. [Maarten]
> 
> v6: Rebase
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Gwan-gyeong Mun <gwan-gyeong.mun@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c |  1 +
>  drivers/gpu/drm/i915/intel_dp.c  | 73 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  2 +
>  3 files changed, 76 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index cd5277d98b03..2f1688ea5a2c 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3391,6 +3391,7 @@ static void intel_enable_ddi_dp(struct intel_encoder *encoder,
>  
>  	intel_edp_backlight_on(crtc_state, conn_state);
>  	intel_psr_enable(intel_dp, crtc_state);
> +	intel_dp_ycbcr_420_enable(intel_dp, crtc_state);

I wonder if this is a bit too late. But we do it for PSR here, so I
guess we should think about this for both cases. We should actually
add full readout + state checker for the VSC SDP for both cases as
well. But that can be done later.

>  	intel_edp_drrs_enable(intel_dp, crtc_state);
>  
>  	if (crtc_state->has_audio)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 06a3417a88d1..74aad8830a80 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4394,6 +4394,79 @@ u8 intel_dp_dsc_get_slice_count(struct intel_dp *intel_dp,
>  	return 0;
>  }
>  
> +static void
> +intel_pixel_encoding_setup_vsc(struct intel_dp *intel_dp,
> +			       const struct intel_crtc_state *crtc_state)
> +{
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct dp_vsc_sdp vsc_sdp;
> +
> +	if (!intel_dp->attached_connector->base.ycbcr_420_allowed)
> +		return;
> +
> +	/* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 */
> +	memset(&vsc_sdp, 0, sizeof(vsc_sdp));

Can be replaced with '= {}' in the declaration.

> +	vsc_sdp.sdp_header.HB0 = 0;
> +	vsc_sdp.sdp_header.HB1 = 0x7;
> +
> +	/* VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> +	 * Colorimetry Format indication. A DP Source device is allowed
> +	 * to indicate the pixel encoding/colorimetry format to the DP Sink
> +	 * device with VSC SDP only when the DP Sink device supports it
> +	 * (i.e., VSC_SDP_EXTENSION_FOR_COLORIMETRY_SUPPORTED bit in the register
> +	 * DPRX_FEATURE_ENUMERATION_LIST (DPCD Address 02210h, bit 3) is set to 1)
> +	 */

Are we checking that bit somewhere? I suppose the sink might a bit nuts
if it declares 420_only modes without that set, but maybe it should be
checked anyway.

Also non-standard comment format all over. Should be
/*
 * blah
 */

> +	vsc_sdp.sdp_header.HB2 = 0x5;
> +
> +	/* VSC SDP supporting 3D stereo, + PSR2, + Pixel Encoding/
> +	 * Colorimetry Format indication (HB2 = 05h).
> +	 */
> +	vsc_sdp.sdp_header.HB3 = 0x13;
> +	/* YCbCr 420 = 3h DB16[7:4] ITU-R BT.601 = 0h, ITU-R BT.709 = 1h
> +	 * DB16[3:0] DP 1.4a spec, Table 2-120
> +	 */
> +
> +	/* Commit id (25edf91501b8 "drm/i915: prepare csc unit for YCBCR420 output")
> +	 * uses the BT.709 color space to perform RGB->YCBCR conversion.
> +	 */

I don't think referring to specific commit here is particularly helpful.
The situation will change anyway at some point.

> +	vsc_sdp.DB16 = 0x3 << 4; /* 0x3 << 4 , YCbCr 420*/
> +	vsc_sdp.DB16 |= 0x1; /* 0x1, ITU-R BT.709 */
> +
> +	/* For pixel encoding formats YCbCr444, YCbCr422, YCbCr420, and Y Only,
> +	 * the following Component Bit Depth values are defined:
> +	 * 001b = 8bpc.
> +	 * 010b = 10bpc.
> +	 * 011b = 12bpc.
> +	 * 100b = 16bpc.
> +	 */
> +	vsc_sdp.DB17 = 0x1;

Don't we need to base this on pipe_bpp? 

And I'm thinking we want the CEA bit set here as well.

> +
> +	/*
> +	 * Content Type (Bits 2:0)
> +	 * 000b = Not defined.
> +	 * 001b = Graphics.
> +	 * 010b = Photo.
> +	 * 011b = Video.
> +	 * 100b = Game
> +	 * All other values are RESERVED.
> +	 * Note: See CTA-861-G for the definition and expected
> +	 * processing by a stream sink for the above contect types.
> +	 */
> +	vsc_sdp.DB18 = 0;

Hmm. I guess we could add the content type prop for DP too.
But that's something for the future.

All the magic numbers should be defined somewhere in the core dp header.
But that could be done as a followup. The PSR code probably needs
changing too in that case.

> +
> +	intel_dig_port->write_infoframe(&intel_dig_port->base,
> +			crtc_state, DP_SDP_VSC, &vsc_sdp, sizeof(vsc_sdp));
> +}
> +
> +void intel_dp_ycbcr_420_enable(struct intel_dp *intel_dp,
> +			       const struct intel_crtc_state *crtc_state)
> +{
> +	if (crtc_state->output_format != INTEL_OUTPUT_FORMAT_YCBCR420)
> +		return;
> +
> +	intel_pixel_encoding_setup_vsc(intel_dp, crtc_state);
> +}
> +
>  static u8 intel_dp_autotest_link_training(struct intel_dp *intel_dp)
>  {
>  	int status = 0;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 247893ed1543..5d1845526cf8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1576,6 +1576,8 @@ void intel_dp_get_m_n(struct intel_crtc *crtc,
>  		      struct intel_crtc_state *pipe_config);
>  void intel_dp_set_m_n(const struct intel_crtc_state *crtc_state,
>  		      enum link_m_n_set m_n);
> +void intel_dp_ycbcr_420_enable(struct intel_dp *intel_dp,
> +			       const struct intel_crtc_state *crtc_state);
>  int intel_dotclock_calculate(int link_freq, const struct intel_link_m_n *m_n);
>  bool bxt_find_best_dpll(struct intel_crtc_state *crtc_state,
>  			struct dpll *best_clock);
> -- 
> 2.21.0

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