Re: [Intel-gfx] [PATCH v7 3/6] drm/i915/dp: Program VSC Header and DB for Pixel Encoding/Colorimetry Format
On Fri, 2019-05-17 at 15:36 +0200, Maarten Lankhorst wrote:
> Op 10-05-2019 om 03:53 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.
> >
> > v2:
> > Minor style fix. [Maarten]
> > Refer to commit ids instead of patchwork. [Maarten]
> >
> > v6: Rebase
> >
> > v7:
> > Rebase and addressed review comments from Ville.
> > Use a structure initializer instead of memset().
> > Fix non-standard comment format.
> > Remove a referring to specific commit.
> > Add a setting of dynamic range bit to vsc_sdp.DB17.
> > Add a setting of bpc which is based on pipe_bpp.
> > Remove duplicated checking of connector's ycbcr_420_allowed from
> > intel_pixel_encoding_setup_vsc(). It is already checked from
> > intel_dp_ycbcr420_config().
> > Remove comments for VSC_SDP_EXTENSION_FOR_COLORIMETRY_SUPPORTED.
> > It is
> > already implemented on intel_dp_get_colorimetry_status().
> >
> > Cc: Maarten Lankhorst
> > Cc: Ville Syrjälä
> > Signed-off-by: Gwan-gyeong Mun
> > ---
> > drivers/gpu/drm/i915/intel_ddi.c | 1 +
> > drivers/gpu/drm/i915/intel_dp.c | 90
> >
> > drivers/gpu/drm/i915/intel_drv.h | 2 +
> > 3 files changed, 93 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);
> > 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 9f219f8f0c71..a3fd279a5c52 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4407,6 +4407,96 @@ 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_sdp vsc_sdp = {};
> > +
> > + /* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119
> > */
> > + vsc_sdp.sdp_header.HB0 = 0;
> > + vsc_sdp.sdp_header.HB1 = 0x7;
> > +
> > + /*
> > +* VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> > +* Colorimetry Format indication.
> > +*/
> > + 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
> > +*/
> > + vsc_sdp.DB[16] = 0x3 << 4; /* 0x3 << 4 , YCbCr 420*/
> > + /* RGB->YCBCR color conversion uses the BT.709 color space. */
> > + vsc_sdp.DB[16] |= 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.
> > +*/
> > + switch (crtc_state->pipe_bpp) {
> > + case 24: /* 8bpc */
> > + vsc_sdp.DB[17] = 0x1;
> > + break;
> > + case 30: /* 10bpc */
> > + vsc_sdp.DB[17] = 0x2;
> > + break;
> > + case 36: /* 12bpc */
> > + vsc_sdp.DB[17] = 0x3;
> > + break;
> > + case 48: /* 16bpc */
> > + vsc_sdp.DB[17] = 0x4;
> > + break;
> > + default:
> > + DRM_DEBUG_KMS("Invalid bpp value '%d'\n", crtc_state-
> > >pipe_bpp);
>
> I would use MISSING_CASE(crtc_state->pipe_bpp); here, as it's pretty
> fatal to VSC setup. :)
>
> Otherwise series looks good from an atomic point of view. I would try
> to get an ACK from the analogix bridge maintainer for
> inclusion in the drm-intel-next-queued tree, since it seems harmless
> enough to include from there.
>
Hi,
Thank you for reviewing the series.
And I'll replace DRM_DEBUG_KMS() to MISSING_CASE() on default switch-
case.
> So for whole series:
> Reviewed-by: Maarten Lankhorst
>
> > + break;
> > + }
> > +
> > +
Re: [Intel-gfx] [PATCH v7 3/6] drm/i915/dp: Program VSC Header and DB for Pixel Encoding/Colorimetry Format
Op 10-05-2019 om 03:53 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.
>
> v2:
> Minor style fix. [Maarten]
> Refer to commit ids instead of patchwork. [Maarten]
>
> v6: Rebase
>
> v7:
> Rebase and addressed review comments from Ville.
> Use a structure initializer instead of memset().
> Fix non-standard comment format.
> Remove a referring to specific commit.
> Add a setting of dynamic range bit to vsc_sdp.DB17.
> Add a setting of bpc which is based on pipe_bpp.
> Remove duplicated checking of connector's ycbcr_420_allowed from
> intel_pixel_encoding_setup_vsc(). It is already checked from
> intel_dp_ycbcr420_config().
> Remove comments for VSC_SDP_EXTENSION_FOR_COLORIMETRY_SUPPORTED. It is
> already implemented on intel_dp_get_colorimetry_status().
>
> Cc: Maarten Lankhorst
> Cc: Ville Syrjälä
> Signed-off-by: Gwan-gyeong Mun
> ---
> drivers/gpu/drm/i915/intel_ddi.c | 1 +
> drivers/gpu/drm/i915/intel_dp.c | 90
> drivers/gpu/drm/i915/intel_drv.h | 2 +
> 3 files changed, 93 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);
> 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 9f219f8f0c71..a3fd279a5c52 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4407,6 +4407,96 @@ 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_sdp vsc_sdp = {};
> +
> + /* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 */
> + vsc_sdp.sdp_header.HB0 = 0;
> + vsc_sdp.sdp_header.HB1 = 0x7;
> +
> + /*
> + * VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
> + * Colorimetry Format indication.
> + */
> + 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
> + */
> + vsc_sdp.DB[16] = 0x3 << 4; /* 0x3 << 4 , YCbCr 420*/
> + /* RGB->YCBCR color conversion uses the BT.709 color space. */
> + vsc_sdp.DB[16] |= 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.
> + */
> + switch (crtc_state->pipe_bpp) {
> + case 24: /* 8bpc */
> + vsc_sdp.DB[17] = 0x1;
> + break;
> + case 30: /* 10bpc */
> + vsc_sdp.DB[17] = 0x2;
> + break;
> + case 36: /* 12bpc */
> + vsc_sdp.DB[17] = 0x3;
> + break;
> + case 48: /* 16bpc */
> + vsc_sdp.DB[17] = 0x4;
> + break;
> + default:
> + DRM_DEBUG_KMS("Invalid bpp value '%d'\n", crtc_state->pipe_bpp);
I would use MISSING_CASE(crtc_state->pipe_bpp); here, as it's pretty fatal to
VSC setup. :)
Otherwise series looks good from an atomic point of view. I would try to get an
ACK from the analogix bridge maintainer for
inclusion in the drm-intel-next-queued tree, since it seems harmless enough to
include from there.
So for whole series:
Reviewed-by: Maarten Lankhorst
> + break;
> + }
> +
> + /*
> + * Dynamic Range (Bit 7)
> + * 0 = VESA range, 1 = CTA range.
> + * all YCbCr are always limited range
> + */
> + vsc_sdp.DB[17] |= 0x80;
> +
> + /*
> + * Content Type (Bits 2:0)
> + * 000b = Not defined.
> + * 001b = Graphics.
> + * 010b = Photo.
> + * 011b = Video.
> + * 100b = Game
> + * All other
[Intel-gfx] [PATCH v7 3/6] drm/i915/dp: Program VSC Header and DB for Pixel Encoding/Colorimetry Format
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
v7:
Rebase and addressed review comments from Ville.
Use a structure initializer instead of memset().
Fix non-standard comment format.
Remove a referring to specific commit.
Add a setting of dynamic range bit to vsc_sdp.DB17.
Add a setting of bpc which is based on pipe_bpp.
Remove duplicated checking of connector's ycbcr_420_allowed from
intel_pixel_encoding_setup_vsc(). It is already checked from
intel_dp_ycbcr420_config().
Remove comments for VSC_SDP_EXTENSION_FOR_COLORIMETRY_SUPPORTED. It is
already implemented on intel_dp_get_colorimetry_status().
Cc: Maarten Lankhorst
Cc: Ville Syrjälä
Signed-off-by: Gwan-gyeong Mun
---
drivers/gpu/drm/i915/intel_ddi.c | 1 +
drivers/gpu/drm/i915/intel_dp.c | 90
drivers/gpu/drm/i915/intel_drv.h | 2 +
3 files changed, 93 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);
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 9f219f8f0c71..a3fd279a5c52 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4407,6 +4407,96 @@ 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_sdp vsc_sdp = {};
+
+ /* Prepare VSC Header for SU as per DP 1.4a spec, Table 2-119 */
+ vsc_sdp.sdp_header.HB0 = 0;
+ vsc_sdp.sdp_header.HB1 = 0x7;
+
+ /*
+* VSC SDP supporting 3D stereo, PSR2, and Pixel Encoding/
+* Colorimetry Format indication.
+*/
+ 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
+*/
+ vsc_sdp.DB[16] = 0x3 << 4; /* 0x3 << 4 , YCbCr 420*/
+ /* RGB->YCBCR color conversion uses the BT.709 color space. */
+ vsc_sdp.DB[16] |= 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.
+*/
+ switch (crtc_state->pipe_bpp) {
+ case 24: /* 8bpc */
+ vsc_sdp.DB[17] = 0x1;
+ break;
+ case 30: /* 10bpc */
+ vsc_sdp.DB[17] = 0x2;
+ break;
+ case 36: /* 12bpc */
+ vsc_sdp.DB[17] = 0x3;
+ break;
+ case 48: /* 16bpc */
+ vsc_sdp.DB[17] = 0x4;
+ break;
+ default:
+ DRM_DEBUG_KMS("Invalid bpp value '%d'\n", crtc_state->pipe_bpp);
+ break;
+ }
+
+ /*
+* Dynamic Range (Bit 7)
+* 0 = VESA range, 1 = CTA range.
+* all YCbCr are always limited range
+*/
+ vsc_sdp.DB[17] |= 0x80;
+
+ /*
+* 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.DB[18] = 0;
+
+ intel_dig_port->write_infoframe(_dig_port->base,
+ crtc_state, DP_SDP_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)
+
