diff mbox series

[RFC,4/6] drm/i915/dp: Program VSC Header and DB for Pixel Encoding/Colorimetry Format

Message ID 20190131211046.18603-5-gwan-gyeong.mun@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/dp: Preliminary support for DP YCbCr4:2:0 outputs | expand

Commit Message

Gwan-gyeong Mun Jan. 31, 2019, 9:10 p.m. UTC 
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.

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  | 72 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  2 +
 3 files changed, 75 insertions(+)

Comments

Maarten Lankhorst Feb. 8, 2019, 3:31 p.m. UTC | #1 
Op 31-01-2019 om 22:10 schreef Gwan-gyeong Mun:
> 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.
>
> 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  | 72 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  2 +
>  3 files changed, 75 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ca705546a0ab..8969f03393b8 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3400,6 +3400,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);
>  	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 a61aff23c8b2..3a9a5a3c33a9 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4402,6 +4402,78 @@ 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;
> +
Newline missing?
> +	/* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 */
> +	memset(&vsc_sdp, 0, sizeof(vsc_sdp));
> +	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)
> +	 */
> +	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
> +	 */
> +
> +	/* https://patchwork.freedesktop.org/patch/166830/ i915 implementations
> +	 * uses BT.709 color space
> +	 */
I think we should refer to commit ids instead of patchwork. :)
> +	vsc_sdp.DB16 = 0x3 << 4; /* 0x3 << 4 , YCbCr 420*/
> +	vsc_sdp.DB16 |= 0x1; /* 0x1, ITU-R BT.709 */

But speaking of patchwork, might be nice to setup a VSC for not yuv420 as well.

This could be used for https://patchwork.freedesktop.org/series/47132/

:)

> +	/* 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;
> +
> +	/*
> +	 * 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;
> +
> +	intel_dig_port->write_infoframe(&intel_dig_port->base,
> +			crtc_state, DP_SDP_VSC, &vsc_sdp, sizeof(vsc_sdp));
> +}
> +

This should probably be less hardcoded in the future, but looks ok for now.

~Maarten
Gwan-gyeong Mun Feb. 21, 2019, 7:14 p.m. UTC | #2 
On Fri, 2019-02-08 at 16:31 +0100, Maarten Lankhorst wrote:
> Op 31-01-2019 om 22:10 schreef Gwan-gyeong Mun:
> > 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.
> > 
> > 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  | 72
> > ++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h |  2 +
> >  3 files changed, 75 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index ca705546a0ab..8969f03393b8 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -3400,6 +3400,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);
> >  	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 a61aff23c8b2..3a9a5a3c33a9 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4402,6 +4402,78 @@ 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;
> > +
> Newline missing?
I'll add a missed Newline.
> > +	/* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
> > */
> > +	memset(&vsc_sdp, 0, sizeof(vsc_sdp));
> > +	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)
> > +	 */
> > +	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
> > +	 */
> > +
> > +	/* https://patchwork.freedesktop.org/patch/166830/ i915
> > implementations
> > +	 * uses BT.709 color space
> > +	 */
> I think we should refer to commit ids instead of patchwork. :)
I'll refer to commit ids instead of patchwork. 
> > +	vsc_sdp.DB16 = 0x3 << 4; /* 0x3 << 4 , YCbCr 420*/
> > +	vsc_sdp.DB16 |= 0x1; /* 0x1, ITU-R BT.709 */
> 
> But speaking of patchwork, might be nice to setup a VSC for not
> yuv420 as well.
> 
Including this patch, there are two setup of vsc functions.
these are intel_psr_setup_vsc() and intel_pixel_encoding_setup_vsc().
In my opinion we can refactor setting of vsc function after these
series.
If I have to make a common setting of vsc fuction prior to this patch,
please let me know.
> This could be used for 
> https://patchwork.freedesktop.org/series/47132/
> 
> :)
> 
Followed to https://patchwork.freedesktop.org/series/47132/, latest
rev17 dropped a patch ("drm: Add DP colorspace property").
After adding this patch we can have a usecase to DP colorspace
property.
therefore after landing this patch, we can add and use dropped patch
("drm: Add DP colorspace property") for setting VSC.
> > +	/* 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;
> > +
> > +	/*
> > +	 * 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;
> > +
> > +	intel_dig_port->write_infoframe(&intel_dig_port->base,
> > +			crtc_state, DP_SDP_VSC, &vsc_sdp,
> > sizeof(vsc_sdp));
> > +}
> > +
> 
> This should probably be less hardcoded in the future, but looks ok
> for now.
Certainly, I'll reduce hard coded lines in the future.
> 
> ~Maarten
> 
Br,
Gwan-gyeong.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ca705546a0ab..8969f03393b8 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3400,6 +3400,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);
 	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 a61aff23c8b2..3a9a5a3c33a9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4402,6 +4402,78 @@  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));
+	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)
+	 */
+	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
+	 */
+
+	/* https://patchwork.freedesktop.org/patch/166830/ i915 implementations
+	 * uses BT.709 color space
+	 */
+	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;
+
+	/*
+	 * 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;
+
+	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 90ba5436370e..bc01a69e8a2a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1868,6 +1868,8 @@  u16 intel_dp_dsc_get_output_bpp(int link_clock, u8 lane_count,
 				int mode_clock, int mode_hdisplay);
 u8 intel_dp_dsc_get_slice_count(struct intel_dp *intel_dp, int mode_clock,
 				int mode_hdisplay);
+void intel_dp_ycbcr_420_enable(struct intel_dp *intel_dp,
+			       const struct intel_crtc_state *crtc_state);
 
 /* intel_vdsc.c */
 int intel_dp_compute_dsc_params(struct intel_dp *intel_dp,