On Fri, 2020-03-20 at 13:57 +0200, Laurent Pinchart wrote:

Hi Jani,

On Fri, Mar 20, 2020 at 01:32:17PM +0200, Jani Nikula wrote:

quoted

On Fri, 20 Mar 2020, Jani Nikula [off-list ref]
wrote:

quoted

On Tue, 11 Feb 2020, Gwan-gyeong Mun [off-list ref]
wrote:

quoted

It adds an unpack only function for DRM infoframe for dynamic
range and
mastering infoframe readout.
It unpacks the information data block contained in the binary
buffer into
a structured frame of the HDMI Dynamic Range and Mastering
(DRM)
information frame.

In contrast to hdmi_drm_infoframe_unpack() function, it does
not verify
a checksum.

It can be used for unpacking a DP HDR Metadata Infoframe SDP
case.
DP HDR Metadata Infoframe SDP uses the same Dynamic Range and
Mastering
(DRM) information (CTA-861-G spec.) such as HDMI DRM infoframe.
But DP SDP header and payload structure are different from HDMI
DRM
Infoframe. Therefore unpacking DRM infoframe for DP requires
skipping of
a verifying checksum.

Signed-off-by: Gwan-gyeong Mun <redacted>
Reviewed-by: Uma Shankar <redacted>

Bartlomiej, can I have your ack for merging this via drm-intel
along
with the rest of the series, please?

Or Hans or Laurent, from v4l/video point of view.

I'm no expert on InfoFrame, I'll only comment on the API below.

quoted

quoted

quoted

---
 drivers/video/hdmi.c | 58 +++++++++++++++++++++++++++++++-----
--------
 include/linux/hdmi.h |  2 ++
 2 files changed, 43 insertions(+), 17 deletions(-)

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 9c82e2a0a411..9818836d82b7 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c

@@ -1775,20 +1775,18 @@ hdmi_vendor_any_infoframe_unpack(union

hdmi_vendor_any_infoframe *frame,
 }
 
 /**
- * hdmi_drm_infoframe_unpack() - unpack binary buffer to a
HDMI DRM infoframe
+ * hdmi_drm_infoframe_unpack_only() - unpack binary buffer to
a HDMI DRM infoframe
  * @frame: HDMI DRM infoframe
  * @buffer: source buffer
  * @size: size of buffer
  *
- * Unpacks the information contained in binary @buffer into a
structured
+ * Unpacks the information data block contained in binary
@buffer into a structured

Line wrap please.

This needs to be clarified to explain exactly what the buffer points
to.

Okay I'll update clear comments next version.

Also, as this is applicable to DP too, shouldn't we drop the hdmi_
prefix ? Is there another prefix that could be used for functions
that
are application to infoframe handling shared by different display
interfaces ? A bit of refactoring would help making all this clear.

Both DP and HDMI use CTA-861-G spec for DRM infoframe. I'll update
prefix with cta_ instead of hdmi_.

quoted

quoted

quoted

  * @frame of the HDMI Dynamic Range and Mastering (DRM)
information frame.
- * Also verifies the checksum as required by section 5.3.5 of
the HDMI 1.4
- * specification.
  *
  * Returns 0 on success or a negative error code on failure.
  */
-static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe
*frame,
-				     const void *buffer, size_t
size)
+int hdmi_drm_infoframe_unpack_only(struct hdmi_drm_infoframe
*frame,
+				   const void *buffer, size_t
size)
 {
 	const u8 *ptr = buffer;
 	const u8 *temp;

@@ -1797,23 +1795,13 @@ static int

hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame,
 	int ret;
 	int i;
 
-	if (size < HDMI_INFOFRAME_SIZE(DRM))
-		return -EINVAL;
-
-	if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM ||
-	    ptr[1] != 1 ||
-	    ptr[2] != HDMI_DRM_INFOFRAME_SIZE)
-		return -EINVAL;
-
-	if (hdmi_infoframe_checksum(buffer,
HDMI_INFOFRAME_SIZE(DRM)) != 0)
+	if (size < HDMI_DRM_INFOFRAME_SIZE)
 		return -EINVAL;
 
 	ret = hdmi_drm_infoframe_init(frame);
 	if (ret)
 		return ret;
 
-	ptr += HDMI_INFOFRAME_HEADER_SIZE;
-
 	frame->eotf = ptr[0] & 0x7;
 	frame->metadata_type = ptr[1] & 0x7;
 

@@ -1837,6 +1825,42 @@ static int

hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe *frame,
 
 	return 0;
 }
+EXPORT_SYMBOL(hdmi_drm_infoframe_unpack_only);
+
+/**
+ * hdmi_drm_infoframe_unpack() - unpack binary buffer to a
HDMI DRM infoframe
+ * @frame: HDMI DRM infoframe
+ * @buffer: source buffer
+ * @size: size of buffer
+ *
+ * Unpacks the information contained in binary @buffer into a
structured

Same here. The difference between the two functions is "information
data
block" vs. "information", it's very unclear to the reader without
looking at either the commit message or the implementation.

I'll update clear comments next version.

Thank you for giving me review comments.

G.G.

quoted

quoted

quoted

+ * @frame of the HDMI Dynamic Range and Mastering (DRM)
information frame.
+ * Also verifies the checksum as required by section 5.3.5 of
the HDMI 1.4
+ * specification.
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+static int hdmi_drm_infoframe_unpack(struct hdmi_drm_infoframe
*frame,
+				     const void *buffer, size_t
size)
+{
+	const u8 *ptr = buffer;
+	int ret;
+
+	if (size < HDMI_INFOFRAME_SIZE(DRM))
+		return -EINVAL;
+
+	if (ptr[0] != HDMI_INFOFRAME_TYPE_DRM ||
+	    ptr[1] != 1 ||
+	    ptr[2] != HDMI_DRM_INFOFRAME_SIZE)
+		return -EINVAL;
+
+	if (hdmi_infoframe_checksum(buffer,
HDMI_INFOFRAME_SIZE(DRM)) != 0)
+		return -EINVAL;
+
+	ret = hdmi_drm_infoframe_unpack_only(frame, ptr +
HDMI_INFOFRAME_HEADER_SIZE,
+					     size -
HDMI_INFOFRAME_HEADER_SIZE);
+	return ret;
+}
 
 /**
  * hdmi_infoframe_unpack() - unpack binary buffer to a HDMI
infoframe

diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index 9918a6c910c5..afb43efc03e0 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h

@@ -219,6 +219,8 @@ ssize_t hdmi_drm_infoframe_pack(struct

hdmi_drm_infoframe *frame, void *buffer,
 ssize_t hdmi_drm_infoframe_pack_only(const struct
hdmi_drm_infoframe *frame,
 				     void *buffer, size_t
size);
 int hdmi_drm_infoframe_check(struct hdmi_drm_infoframe
*frame);
+int hdmi_drm_infoframe_unpack_only(struct hdmi_drm_infoframe
*frame,
+				   const void *buffer, size_t
size);
 
 enum hdmi_spd_sdi {
 	HDMI_SPD_SDI_UNKNOWN,

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel