Re: [PATCH 2/3] hdmi: added unpack and logging functions for InfoFrames
On Fri, Nov 28, 2014 at 03:50:50PM +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 | 622
++-
include/linux/hdmi.h | 3 +
2 files changed, 618 insertions(+), 7 deletions(-)
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 9e758a8..9f0f554 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -27,10 +27,10 @@
#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)
+static u8 hdmi_infoframe_calc_checksum(u8 *ptr, size_t size)
I'd personally keep the name here.
@@ -434,3 +441,604 @@ 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_txt(enum hdmi_infoframe_type type)
Perhaps: hdmi_infoframe_type_get_name()?
+{
+ 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;
I'd prefer case ...: and return ...; on separate lines for
readability.
+ }
+ return Invalid/Unknown;
+}
Maybe include the numerical value here? Of course that either means that
callers must pass in a buffer or we sacrifice thread-safety. The buffer
could be optional, somewhat like this:
const char *hdmi_infoframe_get_name(char *buffer, size_t length,
enum hdmi_infoframe_type type)
{
const char *name = NULL;
switch (type) {
case HDMI_INFOFRAME_TYPE_VENDOR:
name = Vendor;
break;
...
}
if (buffer) {
if (!name)
snprintf(buffer, length, unknown (%d), type);
else
snprintf(buffer, length, name);
name = buffer;
}
return name;
}
That way the function would be generally useful and could even be made
publicly available.
+static void hdmi_infoframe_log_header(struct device *dev, void *f)
+{
+ struct hdmi_any_infoframe *frame = f;
+ dev_info(dev, HDMI infoframe: %s, version %d, length %d\n,
+ hdmi_infoframe_type_txt(frame-type), frame-version,
frame-length);
+}
+
+static const char *hdmi_colorspace_txt(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_IDO_DEFINED: return IDO Defined;
+ }
+ return Future;
+}
Similar comments as for the above.
+static const char *hdmi_scan_mode_txt(enum hdmi_scan_mode scan_mode)
+{
+ switch(scan_mode) {
+ case HDMI_SCAN_MODE_NONE: return No Data;
+ case HDMI_SCAN_MODE_OVERSCAN: return Composed for overscanned display;
+ case HDMI_SCAN_MODE_UNDERSCAN: return Composed for underscanned
display;
+ }
+ return Future;
+}
This isn't really a name any more, I think it should either stick to
names like None, Overscan, Underscan or it should return a
description, in which case hdmi_scan_mode_get_description() might be
more accurate for a name.
+static const char *hdmi_colorimetry_txt(enum hdmi_colorimetry colorimetry)
+{
+ switch(colorimetry) {
+ case HDMI_COLORIMETRY_NONE: return No Data;
+ case HDMI_COLORIMETRY_ITU_601: return ITU601;
+ case HDMI_COLORIMETRY_ITU_709: return ITU709;
+ case HDMI_COLORIMETRY_EXTENDED: return Extended;
+ }
+ return Invalid/Unknown;
+}
These are names again, so same comments as for the infoframe type. And
perhaps No Data - None in that case.
+
+static const char *hdmi_picture_aspect_txt(enum hdmi_picture_aspect
picture_aspect)
+{
+ switch (picture_aspect) {
+ case HDMI_PICTURE_ASPECT_NONE: return No Data;
+ case HDMI_PICTURE_ASPECT_4_3: return 4:3;
+ case HDMI_PICTURE_ASPECT_16_9: return 16:9;
+ }
+ return Future;
+}
Same here.
+static const
Re: [PATCH 2/3] hdmi: added unpack and logging functions for InfoFrames
Hi Thierry,
Thanks for the review, see my comments below.
On 12/01/2014 02:15 PM, Thierry Reding wrote:
On Fri, Nov 28, 2014 at 03:50:50PM +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 | 622
++-
include/linux/hdmi.h | 3 +
2 files changed, 618 insertions(+), 7 deletions(-)
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 9e758a8..9f0f554 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -27,10 +27,10 @@
#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)
+static u8 hdmi_infoframe_calc_checksum(u8 *ptr, size_t size)
I'd personally keep the name here.
I'll do that.
@@ -434,3 +441,604 @@ 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_txt(enum hdmi_infoframe_type type)
Perhaps: hdmi_infoframe_type_get_name()?
I think that's better as well, I'll change it.
+{
+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;
I'd prefer case ...: and return ...; on separate lines for
readability.
I actually think that makes it *less* readable. If you really want that, then
I'll
change it, but I would suggest that you try it yourself first to see if it is
really more readable for you. It isn't for me, so I'll keep this for the next
version.
+}
+return Invalid/Unknown;
+}
Maybe include the numerical value here? Of course that either means that
callers must pass in a buffer or we sacrifice thread-safety. The buffer
could be optional, somewhat like this:
const char *hdmi_infoframe_get_name(char *buffer, size_t length,
enum hdmi_infoframe_type type)
{
const char *name = NULL;
switch (type) {
case HDMI_INFOFRAME_TYPE_VENDOR:
name = Vendor;
break;
...
}
if (buffer) {
if (!name)
snprintf(buffer, length, unknown (%d), type);
else
snprintf(buffer, length, name);
name = buffer;
}
return name;
}
That way the function would be generally useful and could even be made
publicly available.
I would do this only where it makes sense. Some of these fields have only one or
two reserved bits left, and in that case is it easier to just say something
like Reserved (3) and do that for each reserved value.
+static void hdmi_infoframe_log_header(struct device *dev, void *f)
+{
+struct hdmi_any_infoframe *frame = f;
+dev_info(dev, HDMI infoframe: %s, version %d, length %d\n,
+hdmi_infoframe_type_txt(frame-type), frame-version,
frame-length);
+}
+
+static const char *hdmi_colorspace_txt(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_IDO_DEFINED: return IDO Defined;
+}
+return Future;
+}
Similar comments as for the above.
+static const char *hdmi_scan_mode_txt(enum hdmi_scan_mode scan_mode)
+{
+switch(scan_mode) {
+case HDMI_SCAN_MODE_NONE: return No Data;
+case HDMI_SCAN_MODE_OVERSCAN: return Composed for overscanned display;
+case HDMI_SCAN_MODE_UNDERSCAN: return Composed for underscanned
display;
+}
+return Future;
+}
This isn't really a name any more, I think it should either stick to
names like None, Overscan, Underscan
I agree with that, I'll change it.
or it should return a
description, in which case hdmi_scan_mode_get_description() might be
more accurate for a name.
+static const char *hdmi_colorimetry_txt(enum hdmi_colorimetry colorimetry)
+{
+switch(colorimetry) {
+case HDMI_COLORIMETRY_NONE: return No Data;
+
Re: [PATCH 2/3] hdmi: added unpack and logging functions for InfoFrames
On Mon, Dec 01, 2014 at 02:48:47PM +0100, Hans Verkuil wrote:
Hi Thierry,
Thanks for the review, see my comments below.
On 12/01/2014 02:15 PM, Thierry Reding wrote:
On Fri, Nov 28, 2014 at 03:50:50PM +0100, Hans Verkuil wrote:
[...]
+{
+ 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;
I'd prefer case ...: and return ...; on separate lines for
readability.
I actually think that makes it *less* readable. If you really want that, then
I'll
change it, but I would suggest that you try it yourself first to see if it is
really more readable for you. It isn't for me, so I'll keep this for the next
version.
I did, and I still think separate lines are more readable, especially if
you throw in a blank line after the return ...;. Anyway, I could keep
my OCD in check if it weren't for the fact that half of these are the
cause for checkpatch to complain. And then if you change the ones that
checkpatch wants you to change, all the others would be inconsistent and
then I'd complain about the inconsistency...
checkpatch flagged a couple other issues, please make sure to address
those as well.
Maybe include the numerical value here? Of course that either means that
callers must pass in a buffer or we sacrifice thread-safety. The buffer
could be optional, somewhat like this:
const char *hdmi_infoframe_get_name(char *buffer, size_t length,
enum hdmi_infoframe_type type)
{
const char *name = NULL;
switch (type) {
case HDMI_INFOFRAME_TYPE_VENDOR:
name = Vendor;
break;
...
}
if (buffer) {
if (!name)
snprintf(buffer, length, unknown (%d), type);
else
snprintf(buffer, length, name);
name = buffer;
}
return name;
}
That way the function would be generally useful and could even be made
publicly available.
I would do this only where it makes sense. Some of these fields have only one
or
two reserved bits left, and in that case is it easier to just say something
like Reserved (3) and do that for each reserved value.
Okay.
+/**
+ * hdmi_infoframe_log() - log info of HDMI infoframe
+ * @dev: device
+ * @frame: HDMI infoframe
+ */
+void hdmi_infoframe_log(struct device *dev, union hdmi_infoframe *frame)
+{
+ switch (frame-any.type) {
+ case HDMI_INFOFRAME_TYPE_AVI:
+ hdmi_avi_infoframe_log(dev, frame-avi);
+ break;
+ case HDMI_INFOFRAME_TYPE_SPD:
+ hdmi_spd_infoframe_log(dev, frame-spd);
+ break;
+ case HDMI_INFOFRAME_TYPE_AUDIO:
+ hdmi_audio_infoframe_log(dev, frame-audio);
+ break;
+ case HDMI_INFOFRAME_TYPE_VENDOR:
+ hdmi_vendor_any_infoframe_log(dev, frame-vendor);
+ break;
+ default:
+ WARN(1, Bad infoframe type %d\n, frame-any.type);
Does it make sense for this to be WARN? It's perfectly legal for future
devices to expose new types of infoframes. Perhaps even expected. But if
we want to keep this here to help get bug reports so that we don't
forget to update this code, then maybe we should do the same wherever we
query the name of enum values above.
I'll drop the WARN from the log function. I think it should also be dropped
from the unpack. The only place it makes sense is for pack() since there the
data comes from the driver, not from an external source.
Sounds good.
Thierry
pgpQTn1nRD50A.pgp
Description: PGP signature
Re: [PATCH 2/3] hdmi: added unpack and logging functions for InfoFrames
On Mon, Dec 01, 2014 at 04:33:31PM +0100, Thierry Reding wrote:
On Mon, Dec 01, 2014 at 02:48:47PM +0100, Hans Verkuil wrote:
Hi Thierry,
Thanks for the review, see my comments below.
On 12/01/2014 02:15 PM, Thierry Reding wrote:
On Fri, Nov 28, 2014 at 03:50:50PM +0100, Hans Verkuil wrote:
[...]
+{
+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;
I'd prefer case ...: and return ...; on separate lines for
readability.
I actually think that makes it *less* readable. If you really want that,
then I'll
change it, but I would suggest that you try it yourself first to see if it
is
really more readable for you. It isn't for me, so I'll keep this for the
next
version.
I did, and I still think separate lines are more readable, especially if
you throw in a blank line after the return ...;. Anyway, I could keep
my OCD in check if it weren't for the fact that half of these are the
cause for checkpatch to complain. And then if you change the ones that
checkpatch wants you to change, all the others would be inconsistent and
then I'd complain about the inconsistency...
Throwing in my own unasked-for bikshed opinion ;-)
I agree with Thierry that it's more readable since this way the key and
the value can be lined up easily. That's also possible with the
single-line case/return like you do with some more tabs, but usually means
all the lines overflow the 80 limit badly. So only really works for small
defines/values. So my rule of thumb is
- go with single-line case/return and align them
- but break the lines if they would be too long.
Now back to doing useful stuff for me.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
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
[PATCH 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 | 622 ++-
include/linux/hdmi.h | 3 +
2 files changed, 618 insertions(+), 7 deletions(-)
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 9e758a8..9f0f554 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -27,10 +27,10 @@
#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)
+static u8 hdmi_infoframe_calc_checksum(u8 *ptr, size_t size)
{
- u8 *ptr = buffer;
u8 csum = 0;
size_t i;
@@ -38,7 +38,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_calc_checksum(buffer, size);
}
/**
@@ -136,7 +143,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 +213,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 +288,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 +380,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 +441,604 @@ 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_txt(enum hdmi_infoframe_type type)
+{
+ 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 Invalid/Unknown;
+}
+
+static void hdmi_infoframe_log_header(struct device *dev, void *f)
+{
+ struct hdmi_any_infoframe *frame = f;
+ dev_info(dev, HDMI infoframe: %s, version %d, length %d\n,
+ hdmi_infoframe_type_txt(frame-type), frame-version,
frame-length);
+}
+
+static const char *hdmi_colorspace_txt(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_IDO_DEFINED: return IDO Defined;
+ }
+ return Future;
+}
+
+static const char *hdmi_scan_mode_txt(enum hdmi_scan_mode scan_mode)
+{
+ switch(scan_mode) {
+ case HDMI_SCAN_MODE_NONE: return No Data;
+ case HDMI_SCAN_MODE_OVERSCAN: return Composed for overscanned display;
+ case HDMI_SCAN_MODE_UNDERSCAN: return Composed for underscanned
display;
+ }
+ return Future;
+}
+
+static const char *hdmi_colorimetry_txt(enum hdmi_colorimetry colorimetry)
+{
+ switch(colorimetry) {
+ case HDMI_COLORIMETRY_NONE: return No Data;
+ case HDMI_COLORIMETRY_ITU_601: return ITU601;
+ case HDMI_COLORIMETRY_ITU_709: return ITU709;
+ case HDMI_COLORIMETRY_EXTENDED: return Extended;
+ }
+ return Invalid/Unknown;
+}
+
+static const char *hdmi_picture_aspect_txt(enum hdmi_picture_aspect
picture_aspect)
+{
+ switch (picture_aspect) {
+ case HDMI_PICTURE_ASPECT_NONE: return No Data;
+ case HDMI_PICTURE_ASPECT_4_3: return 4:3;
+ case HDMI_PICTURE_ASPECT_16_9: return 16:9;
+ }
+
