Re: [PATCH v3 06/17] drm/i915/dp: Read out DP SDPs (Secondary Data Packet)
On Wed, 2020-02-05 at 21:59 +0530, Shankar, Uma wrote:
> > -Original Message-
> > From: dri-devel On Behalf
> > Of Gwan-
> > gyeong Mun
> > Sent: Tuesday, February 4, 2020 4:50 AM
> > To: intel-...@lists.freedesktop.org
> > Cc: linux-fb...@vger.kernel.org; dri-devel@lists.freedesktop.org
> > Subject: [PATCH v3 06/17] drm/i915/dp: Read out DP SDPs (Secondary
> > Data Packet)
>
> Drop the content in bracket.
>
> > It adds code to read the DP SDPs from the video DIP and unpack them
> > into the crtc
> > state.
> >
> > It adds routines that read out DP VSC SDP and DP HDR Metadata
> > Infoframe SDP In
> > order to unpack DP VSC SDP, it adds intel_dp_vsc_sdp_unpack()
> > function.
> > It follows DP 1.4a spec. [Table 2-116: VSC SDP Header Bytes] and
> > [Table 2-117: VSC
> > SDP Payload for DB16 through DB18]
> >
> > In order to unpack DP HDR Metadata Infoframe SDP, it adds
> > intel_dp_hdr_metadata_infoframe_sdp_unpack(). And it follows DP
> > 1.4a spec.
> > ([Table 2-125: INFOFRAME SDP v1.2 Header Bytes] and [Table 2-126:
> > INFOFRAME
> > SDP v1.2 Payload Data Bytes - DB0 through DB31]) and CTA-861-G
> > spec. [Table-42
> > Dynamic Range and Mastering InfoFrame].
> >
> > A nameing rule and style of intel_read_dp_sdp() function references
>
> Typo in naming.
>
> > intel_read_infoframe() function of intel_hdmi.c
> >
> > v2: Minor style fix
> > v3: Replace a structure name to drm_dp_vsc_sdp from
> > intel_dp_vsc_sdp
> >
> > Signed-off-by: Gwan-gyeong Mun
> > ---
> > drivers/gpu/drm/i915/display/intel_dp.c | 170
> >
> > drivers/gpu/drm/i915/display/intel_dp.h | 3 +
> > 2 files changed, 173 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> > b/drivers/gpu/drm/i915/display/intel_dp.c
> > index dd7e5588001e..d4ece0a824c0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -4925,6 +4925,176 @@ void intel_dp_set_infoframes(struct
> > intel_encoder
> > *encoder,
> > intel_write_dp_sdp(encoder, crtc_state,
> > HDMI_PACKET_TYPE_GAMUT_METADATA); }
> >
> > +static int intel_dp_vsc_sdp_unpack(struct drm_dp_vsc_sdp *vsc,
> > + const void *buffer, size_t size) {
> > + const struct dp_sdp *sdp = buffer;
> > +
> > + if (size < sizeof(struct dp_sdp))
> > + return -EINVAL;
> > +
> > + memset(vsc, 0, size);
> > +
> > + if (sdp->sdp_header.HB0 != 0)
> > + return -EINVAL;
> > +
> > + if (sdp->sdp_header.HB1 != DP_SDP_VSC)
> > + return -EINVAL;
> > + vsc->sdp_type = sdp->sdp_header.HB1;
> > +
> > + if (sdp->sdp_header.HB2 == 0x2 && sdp->sdp_header.HB3 == 0x8) {
> > + vsc->revision = sdp->sdp_header.HB2;
> > + vsc->length = sdp->sdp_header.HB3;
> > + } else if (sdp->sdp_header.HB2 == 0x4 && sdp->sdp_header.HB3 ==
> > 0xe) {
> > + vsc->revision = sdp->sdp_header.HB2;
> > + vsc->length = sdp->sdp_header.HB3;
> > + } else if (sdp->sdp_header.HB2 == 0x5 && sdp->sdp_header.HB3 ==
> > 0x13) {
> > + vsc->revision = sdp->sdp_header.HB2;
> > + vsc->length = sdp->sdp_header.HB3;
>
> The above 2 lines can be done unconditionally, may be combine the if
> checks.
>
> > + vsc->colorspace = (sdp->db[16] >> 4) & 0xf;
> > + vsc->colorimetry = sdp->db[16] & 0xf;
> > + vsc->dynamic_range = (sdp->db[17] >> 7) & 0x1;
> > +
> > + switch (sdp->db[17] & 0x7) {
> > + case 0x1:
> > + vsc->bpc = 8;
> > + break;
> > + case 0x2:
> > + vsc->bpc = 10;
> > + break;
> > + case 0x3:
> > + vsc->bpc = 12;
> > + break;
> > + case 0x4:
> > + vsc->bpc = 16;
> > + break;
> > + default:
> > + MISSING_CASE(sdp->db[17] & 0x7);
>
> Handle 6bpc case as well.
>
Yes, I'll update everything that you commented.
> > + return -EINVAL;
> > + }
> > +
> > + vsc->content_type = sdp->d
RE: [PATCH v3 06/17] drm/i915/dp: Read out DP SDPs (Secondary Data Packet)
> -Original Message-
> From: dri-devel On Behalf Of Gwan-
> gyeong Mun
> Sent: Tuesday, February 4, 2020 4:50 AM
> To: intel-...@lists.freedesktop.org
> Cc: linux-fb...@vger.kernel.org; dri-devel@lists.freedesktop.org
> Subject: [PATCH v3 06/17] drm/i915/dp: Read out DP SDPs (Secondary Data
> Packet)
Drop the content in bracket.
> It adds code to read the DP SDPs from the video DIP and unpack them into the
> crtc
> state.
>
> It adds routines that read out DP VSC SDP and DP HDR Metadata Infoframe SDP In
> order to unpack DP VSC SDP, it adds intel_dp_vsc_sdp_unpack() function.
> It follows DP 1.4a spec. [Table 2-116: VSC SDP Header Bytes] and [Table
> 2-117: VSC
> SDP Payload for DB16 through DB18]
>
> In order to unpack DP HDR Metadata Infoframe SDP, it adds
> intel_dp_hdr_metadata_infoframe_sdp_unpack(). And it follows DP 1.4a spec.
> ([Table 2-125: INFOFRAME SDP v1.2 Header Bytes] and [Table 2-126: INFOFRAME
> SDP v1.2 Payload Data Bytes - DB0 through DB31]) and CTA-861-G spec. [Table-42
> Dynamic Range and Mastering InfoFrame].
>
> A nameing rule and style of intel_read_dp_sdp() function references
Typo in naming.
> intel_read_infoframe() function of intel_hdmi.c
>
> v2: Minor style fix
> v3: Replace a structure name to drm_dp_vsc_sdp from intel_dp_vsc_sdp
>
> Signed-off-by: Gwan-gyeong Mun
> ---
> drivers/gpu/drm/i915/display/intel_dp.c | 170
> drivers/gpu/drm/i915/display/intel_dp.h | 3 +
> 2 files changed, 173 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index dd7e5588001e..d4ece0a824c0 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4925,6 +4925,176 @@ void intel_dp_set_infoframes(struct intel_encoder
> *encoder,
> intel_write_dp_sdp(encoder, crtc_state,
> HDMI_PACKET_TYPE_GAMUT_METADATA); }
>
> +static int intel_dp_vsc_sdp_unpack(struct drm_dp_vsc_sdp *vsc,
> +const void *buffer, size_t size) {
> + const struct dp_sdp *sdp = buffer;
> +
> + if (size < sizeof(struct dp_sdp))
> + return -EINVAL;
> +
> + memset(vsc, 0, size);
> +
> + if (sdp->sdp_header.HB0 != 0)
> + return -EINVAL;
> +
> + if (sdp->sdp_header.HB1 != DP_SDP_VSC)
> + return -EINVAL;
> + vsc->sdp_type = sdp->sdp_header.HB1;
> +
> + if (sdp->sdp_header.HB2 == 0x2 && sdp->sdp_header.HB3 == 0x8) {
> + vsc->revision = sdp->sdp_header.HB2;
> + vsc->length = sdp->sdp_header.HB3;
> + } else if (sdp->sdp_header.HB2 == 0x4 && sdp->sdp_header.HB3 == 0xe) {
> + vsc->revision = sdp->sdp_header.HB2;
> + vsc->length = sdp->sdp_header.HB3;
> + } else if (sdp->sdp_header.HB2 == 0x5 && sdp->sdp_header.HB3 == 0x13) {
> + vsc->revision = sdp->sdp_header.HB2;
> + vsc->length = sdp->sdp_header.HB3;
The above 2 lines can be done unconditionally, may be combine the if checks.
> + vsc->colorspace = (sdp->db[16] >> 4) & 0xf;
> + vsc->colorimetry = sdp->db[16] & 0xf;
> + vsc->dynamic_range = (sdp->db[17] >> 7) & 0x1;
> +
> + switch (sdp->db[17] & 0x7) {
> + case 0x1:
> + vsc->bpc = 8;
> + break;
> + case 0x2:
> + vsc->bpc = 10;
> + break;
> + case 0x3:
> + vsc->bpc = 12;
> + break;
> + case 0x4:
> + vsc->bpc = 16;
> + break;
> + default:
> + MISSING_CASE(sdp->db[17] & 0x7);
Handle 6bpc case as well.
> + return -EINVAL;
> + }
> +
> + vsc->content_type = sdp->db[18] & 0x7;
> + } else {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int
> +intel_dp_hdr_metadata_infoframe_sdp_unpack(struct hdmi_drm_infoframe
> *drm_infoframe,
> +const void *buffer, size_t size) {
> + int ret;
> +
> + const struct dp_sdp *sdp = buffer;
> +
> + if (size < sizeof(struct dp_sdp))
> + return -EINVAL;
> +
> + if (sdp->sdp_header.HB0 != 0)
> + return -EINVAL;
> +
> + if (sdp->sdp_header.HB1 != HDMI_INFOFRAME_TYPE_DRM)
> + return -EINVAL;
&
[PATCH v3 06/17] drm/i915/dp: Read out DP SDPs (Secondary Data Packet)
It adds code to read the DP SDPs from the video DIP and unpack them into
the crtc state.
It adds routines that read out DP VSC SDP and DP HDR Metadata Infoframe SDP
In order to unpack DP VSC SDP, it adds intel_dp_vsc_sdp_unpack() function.
It follows DP 1.4a spec. [Table 2-116: VSC SDP Header Bytes] and
[Table 2-117: VSC SDP Payload for DB16 through DB18]
In order to unpack DP HDR Metadata Infoframe SDP, it adds
intel_dp_hdr_metadata_infoframe_sdp_unpack(). And it follows DP 1.4a spec.
([Table 2-125: INFOFRAME SDP v1.2 Header Bytes] and
[Table 2-126: INFOFRAME SDP v1.2 Payload Data Bytes - DB0 through DB31])
and CTA-861-G spec. [Table-42 Dynamic Range and Mastering InfoFrame].
A nameing rule and style of intel_read_dp_sdp() function references
intel_read_infoframe() function of intel_hdmi.c
v2: Minor style fix
v3: Replace a structure name to drm_dp_vsc_sdp from intel_dp_vsc_sdp
Signed-off-by: Gwan-gyeong Mun
---
drivers/gpu/drm/i915/display/intel_dp.c | 170
drivers/gpu/drm/i915/display/intel_dp.h | 3 +
2 files changed, 173 insertions(+)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
b/drivers/gpu/drm/i915/display/intel_dp.c
index dd7e5588001e..d4ece0a824c0 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -4925,6 +4925,176 @@ void intel_dp_set_infoframes(struct intel_encoder
*encoder,
intel_write_dp_sdp(encoder, crtc_state,
HDMI_PACKET_TYPE_GAMUT_METADATA);
}
+static int intel_dp_vsc_sdp_unpack(struct drm_dp_vsc_sdp *vsc,
+ const void *buffer, size_t size)
+{
+ const struct dp_sdp *sdp = buffer;
+
+ if (size < sizeof(struct dp_sdp))
+ return -EINVAL;
+
+ memset(vsc, 0, size);
+
+ if (sdp->sdp_header.HB0 != 0)
+ return -EINVAL;
+
+ if (sdp->sdp_header.HB1 != DP_SDP_VSC)
+ return -EINVAL;
+ vsc->sdp_type = sdp->sdp_header.HB1;
+
+ if (sdp->sdp_header.HB2 == 0x2 && sdp->sdp_header.HB3 == 0x8) {
+ vsc->revision = sdp->sdp_header.HB2;
+ vsc->length = sdp->sdp_header.HB3;
+ } else if (sdp->sdp_header.HB2 == 0x4 && sdp->sdp_header.HB3 == 0xe) {
+ vsc->revision = sdp->sdp_header.HB2;
+ vsc->length = sdp->sdp_header.HB3;
+ } else if (sdp->sdp_header.HB2 == 0x5 && sdp->sdp_header.HB3 == 0x13) {
+ vsc->revision = sdp->sdp_header.HB2;
+ vsc->length = sdp->sdp_header.HB3;
+ vsc->colorspace = (sdp->db[16] >> 4) & 0xf;
+ vsc->colorimetry = sdp->db[16] & 0xf;
+ vsc->dynamic_range = (sdp->db[17] >> 7) & 0x1;
+
+ switch (sdp->db[17] & 0x7) {
+ case 0x1:
+ vsc->bpc = 8;
+ break;
+ case 0x2:
+ vsc->bpc = 10;
+ break;
+ case 0x3:
+ vsc->bpc = 12;
+ break;
+ case 0x4:
+ vsc->bpc = 16;
+ break;
+ default:
+ MISSING_CASE(sdp->db[17] & 0x7);
+ return -EINVAL;
+ }
+
+ vsc->content_type = sdp->db[18] & 0x7;
+ } else {
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int
+intel_dp_hdr_metadata_infoframe_sdp_unpack(struct hdmi_drm_infoframe
*drm_infoframe,
+ const void *buffer, size_t size)
+{
+ int ret;
+
+ const struct dp_sdp *sdp = buffer;
+
+ if (size < sizeof(struct dp_sdp))
+ return -EINVAL;
+
+ if (sdp->sdp_header.HB0 != 0)
+ return -EINVAL;
+
+ if (sdp->sdp_header.HB1 != HDMI_INFOFRAME_TYPE_DRM)
+ return -EINVAL;
+
+ /*
+* Least Significant Eight Bits of (Data Byte Count – 1)
+* 1Dh (i.e., Data Byte Count = 30 bytes).
+*/
+ if (sdp->sdp_header.HB2 != 0x1D)
+ return -EINVAL;
+
+ /* Most Significant Two Bits of (Data Byte Count – 1), Clear to 00b. */
+ if ((sdp->sdp_header.HB3 & 0x3) != 0)
+ return -EINVAL;
+
+ /* INFOFRAME SDP Version Number */
+ if (((sdp->sdp_header.HB3 >> 2) & 0x3f) != 0x13)
+ return -EINVAL;
+
+ /* CTA Header Byte 2 (INFOFRAME Version Number) */
+ if (sdp->db[0] != 1)
+ return -EINVAL;
+
+ /* CTA Header Byte 3 (Length of INFOFRAME): HDMI_DRM_INFOFRAME_SIZE */
+ if (sdp->db[1] != HDMI_DRM_INFOFRAME_SIZE)
+ return -EINVAL;
+
+ ret = hdmi_drm_infoframe_unpack_only(drm_infoframe, &sdp->db[2],
+HDMI_DRM_INFOFRAME_SIZE);
+
+ return ret;
+}
+
+static void intel_read_dp_vsc_sdp(struct intel_encoder *encoder,
+ struct intel_crtc_state *crtc_state,
+
