Re: [PATCHv2 2/3] hdmi: added unpack and logging functions for InfoFrames
Hi Thierry,
On 12/18/2014 09:19 AM, Thierry Reding wrote:
+static int hdmi_avi_infoframe_unpack(struct hdmi_avi_infoframe *frame,
+ void *buffer)
+{
+u8 *ptr = buffer;
+int ret;
+
+if (ptr[0] != HDMI_INFOFRAME_TYPE_AVI ||
+ptr[1] != 2 ||
+ptr[2] != HDMI_AVI_INFOFRAME_SIZE)
+return -EINVAL;
+
+if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(AVI)) != 0)
You use the parameterized HDMI_INFOFRAME_SIZE() here, but the plain
macro above. Perhaps make those consistent?
I'm not sure what you mean here since HDMI_AVI_INFOFRAME_SIZE !=
HDMI_INFOFRAME_SIZE(AVI).
The latter includes the infoframe header size.
I'm assuming you missed that. If not, then please clarify.
Regards,
Hans
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv2 2/3] hdmi: added unpack and logging functions for InfoFrames
On Tue, Dec 02, 2014 at 01:08:45PM +0100, Hans Verkuil wrote:
From: Martin Bugge marbu...@cisco.com
When receiving video it is very useful to be able to unpack the InfoFrames.
Logging is useful as well, both for transmitters and receivers.
Especially when implementing the VIDIOC_LOG_STATUS ioctl (supported by many
V4L2 drivers) for a receiver it is important to be able to easily log what
the InfoFrame contains. This greatly simplifies debugging.
Signed-off-by: Martin Bugge marbu...@cisco.com
Signed-off-by: Hans Verkuil hans.verk...@cisco.com
---
drivers/video/hdmi.c | 819
++-
include/linux/hdmi.h | 4 +
2 files changed, 816 insertions(+), 7 deletions(-)
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 9e758a8..5f7ab47 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -27,10 +27,12 @@
#include linux/export.h
#include linux/hdmi.h
#include linux/string.h
+#include linux/device.h
-static void hdmi_infoframe_checksum(void *buffer, size_t size)
+#define hdmi_log(fmt, ...) dev_printk(level, dev, fmt, ##__VA_ARGS__)
I personally dislike macros like these that make assumptions about the
environment. While somewhat longer, directly using dev_printk() would in
my opinion be clearer.
But I realize this is somewhat bikesheddy, so don't consider it a hard
objection.
+
+static u8 hdmi_infoframe_checksum(u8 *ptr, size_t size)
{
- u8 *ptr = buffer;
For consistency with the other functions I'd prefer this to take void *
instead of u8 *. That'd also clean up the diff in this part a little.
u8 csum = 0;
size_t i;
@@ -38,7 +40,14 @@ static void hdmi_infoframe_checksum(void *buffer, size_t
size)
for (i = 0; i size; i++)
csum += ptr[i];
- ptr[3] = 256 - csum;
+ return 256 - csum;
+}
+
+static void hdmi_infoframe_set_checksum(void *buffer, size_t size)
+{
+ u8 *ptr = buffer;
+ /* update checksum */
I think checkpatch warns these days about missing blank lines after the
declaration block. But perhaps it is tricked by the comment immediately
following.
Nit: I don't think the comment adds any value.
+static void hdmi_infoframe_log_header(const char *level,
+ struct device *dev, void *f)
Perhaps rather than pass a void *, make this take a hdmi_any_infoframe *
and require callers to explicitly cast.
This is an internal API and therefore less likely to be abused, so again
rather bikesheddy.
+static const char *hdmi_nups_get_name(enum hdmi_nups nups)
+{
+ switch (nups) {
+ case HDMI_NUPS_UNKNOWN:
+ return No Known Non-uniform Scaling;
s/No Known/Unknown/?
+static void hdmi_avi_infoframe_log(const char *level,
+struct device *dev,
+struct hdmi_avi_infoframe *frame)
+{
+ hdmi_infoframe_log_header(level, dev, frame);
+
+ hdmi_log(colorspace: %s\n,
+ hdmi_colorspace_get_name(frame-colorspace));
+ hdmi_log(scan mode: %s\n,
+ hdmi_scan_mode_get_name(frame-scan_mode));
+ hdmi_log(colorimetry: %s\n,
+ hdmi_colorimetry_get_name(frame-colorimetry));
+ hdmi_log(picture aspect: %s\n,
+ hdmi_picture_aspect_get_name(frame-picture_aspect));
+ hdmi_log(active aspect: %s\n,
+ hdmi_active_aspect_get_name(frame-active_aspect));
+ hdmi_log(itc: %s\n, frame-itc ? IT Content : No Data);
+ hdmi_log(extended colorimetry: %s\n,
+
hdmi_extended_colorimetry_get_name(frame-extended_colorimetry));
+ hdmi_log(quantization range: %s\n,
+
hdmi_quantization_range_get_name(frame-quantization_range));
+ hdmi_log(nups: %s\n, hdmi_nups_get_name(frame-nups));
+ hdmi_log(video code: %d\n, frame-video_code);
This could be %u.
+ hdmi_log(ycc quantization range: %s\n,
+
hdmi_ycc_quantization_range_get_name(frame-ycc_quantization_range));
+ hdmi_log(hdmi content type: %s\n,
+ hdmi_content_type_get_name(frame-content_type));
+ hdmi_log(pixel repeat: %d\n, frame-pixel_repeat);
+ hdmi_log(bar top %d, bottom %d, left %d, right %d\n,
+ frame-top_bar, frame-bottom_bar,
+ frame-left_bar, frame-right_bar);
Same here.
+static const char *
+hdmi_audio_coding_type_get_name(enum hdmi_audio_coding_type coding_type)
+{
+ switch (coding_type) {
+ case HDMI_AUDIO_CODING_TYPE_STREAM:
+ return Refer to Stream Header;
[...]
+ case HDMI_AUDIO_CODING_TYPE_CXT:
+ return Refer to CXT;
These aren't really names, but I can't come up with anything better.
+static const char *
+hdmi_audio_coding_type_ext_get_name(enum hdmi_audio_coding_type_ext ctx)
+{
+
[PATCHv2 2/3] hdmi: added unpack and logging functions for InfoFrames
From: Martin Bugge marbu...@cisco.com
When receiving video it is very useful to be able to unpack the InfoFrames.
Logging is useful as well, both for transmitters and receivers.
Especially when implementing the VIDIOC_LOG_STATUS ioctl (supported by many
V4L2 drivers) for a receiver it is important to be able to easily log what
the InfoFrame contains. This greatly simplifies debugging.
Signed-off-by: Martin Bugge marbu...@cisco.com
Signed-off-by: Hans Verkuil hans.verk...@cisco.com
---
drivers/video/hdmi.c | 819 ++-
include/linux/hdmi.h | 4 +
2 files changed, 816 insertions(+), 7 deletions(-)
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 9e758a8..5f7ab47 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -27,10 +27,12 @@
#include linux/export.h
#include linux/hdmi.h
#include linux/string.h
+#include linux/device.h
-static void hdmi_infoframe_checksum(void *buffer, size_t size)
+#define hdmi_log(fmt, ...) dev_printk(level, dev, fmt, ##__VA_ARGS__)
+
+static u8 hdmi_infoframe_checksum(u8 *ptr, size_t size)
{
- u8 *ptr = buffer;
u8 csum = 0;
size_t i;
@@ -38,7 +40,14 @@ static void hdmi_infoframe_checksum(void *buffer, size_t
size)
for (i = 0; i size; i++)
csum += ptr[i];
- ptr[3] = 256 - csum;
+ return 256 - csum;
+}
+
+static void hdmi_infoframe_set_checksum(void *buffer, size_t size)
+{
+ u8 *ptr = buffer;
+ /* update checksum */
+ ptr[3] = hdmi_infoframe_checksum(buffer, size);
}
/**
@@ -136,7 +145,7 @@ ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe
*frame, void *buffer,
ptr[11] = frame-right_bar 0xff;
ptr[12] = (frame-right_bar 8) 0xff;
- hdmi_infoframe_checksum(buffer, length);
+ hdmi_infoframe_set_checksum(buffer, length);
return length;
}
@@ -206,7 +215,7 @@ ssize_t hdmi_spd_infoframe_pack(struct hdmi_spd_infoframe
*frame, void *buffer,
ptr[24] = frame-sdi;
- hdmi_infoframe_checksum(buffer, length);
+ hdmi_infoframe_set_checksum(buffer, length);
return length;
}
@@ -281,7 +290,7 @@ ssize_t hdmi_audio_infoframe_pack(struct
hdmi_audio_infoframe *frame,
if (frame-downmix_inhibit)
ptr[4] |= BIT(7);
- hdmi_infoframe_checksum(buffer, length);
+ hdmi_infoframe_set_checksum(buffer, length);
return length;
}
@@ -373,7 +382,7 @@ ssize_t hdmi_vendor_infoframe_pack(struct
hdmi_vendor_infoframe *frame,
ptr[9] = (frame-s3d_ext_data 0xf) 4;
}
- hdmi_infoframe_checksum(buffer, length);
+ hdmi_infoframe_set_checksum(buffer, length);
return length;
}
@@ -434,3 +443,799 @@ hdmi_infoframe_pack(union hdmi_infoframe *frame, void
*buffer, size_t size)
return length;
}
EXPORT_SYMBOL(hdmi_infoframe_pack);
+
+static const char *hdmi_infoframe_type_get_name(enum hdmi_infoframe_type type)
+{
+ if (type 0x80 || type 0x9f)
+ return Invalid;
+ switch (type) {
+ case HDMI_INFOFRAME_TYPE_VENDOR:
+ return Vendor;
+ case HDMI_INFOFRAME_TYPE_AVI:
+ return Auxiliary Video Information (AVI);
+ case HDMI_INFOFRAME_TYPE_SPD:
+ return Source Product Description (SPD);
+ case HDMI_INFOFRAME_TYPE_AUDIO:
+ return Audio;
+ }
+ return Reserved;
+}
+
+static void hdmi_infoframe_log_header(const char *level,
+ struct device *dev, void *f)
+{
+ struct hdmi_any_infoframe *frame = f;
+
+ hdmi_log(HDMI infoframe: %s, version %d, length %d\n,
+ hdmi_infoframe_type_get_name(frame-type),
+ frame-version, frame-length);
+}
+
+static const char *hdmi_colorspace_get_name(enum hdmi_colorspace colorspace)
+{
+ switch (colorspace) {
+ case HDMI_COLORSPACE_RGB:
+ return RGB;
+ case HDMI_COLORSPACE_YUV422:
+ return YCbCr 4:2:2;
+ case HDMI_COLORSPACE_YUV444:
+ return YCbCr 4:4:4;
+ case HDMI_COLORSPACE_YUV420:
+ return YCbCr 4:2:0;
+ case HDMI_COLORSPACE_RESERVED4:
+ return Reserved (4);
+ case HDMI_COLORSPACE_RESERVED5:
+ return Reserved (5);
+ case HDMI_COLORSPACE_RESERVED6:
+ return Reserved (6);
+ case HDMI_COLORSPACE_IDO_DEFINED:
+ return IDO Defined;
+ }
+ return Invalid;
+}
+
+static const char *hdmi_scan_mode_get_name(enum hdmi_scan_mode scan_mode)
+{
+ switch (scan_mode) {
+ case HDMI_SCAN_MODE_NONE:
+ return No Data;
+ case HDMI_SCAN_MODE_OVERSCAN:
+ return Overscan;
+ case HDMI_SCAN_MODE_UNDERSCAN:
+ return Underscan;
+ case HDMI_SCAN_MODE_RESERVED:
+ return Reserved;
+ }
+ return Invalid;
+}
+
+static
