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)
> +{
> + if (ctx < 0 || ctx > 0x1f)
> + return "Invalid";
> +
> + switch (ctx) {
> + case HDMI_AUDIO_CODING_TYPE_EXT_STREAM:
> + return "Stream";
CEA-861-E describes this as: "Refer to Audio Coding Type (CT) field in
Data Byte 1". Maybe "Refer to CT"?
I wonder if we should also update the name of the symbolic constant to
reflect that (HDMI_AUDIO_CODING_TYPE_EXT_CT?).
> +static void hdmi_audio_infoframe_log(const char *level,
> + struct device *dev,
> + struct hdmi_audio_infoframe *frame)
> +{
> + hdmi_infoframe_log_header(level, dev, frame);
> +
> + if (frame->channels)
> + hdmi_log(" channels: %d ch\n", frame->channels - 1);
I'd leave out the "ch" at the end, also perhaps "%d" -> "%u".
> + else
> + hdmi_log(" channels: Refer to stream header\n");
> + hdmi_log(" coding type: %s\n",
> + hdmi_audio_coding_type_get_name(frame->coding_type));
> + hdmi_log(" sample size: %s\n",
> + hdmi_audio_sample_size_get_name(frame->sample_size));
> + hdmi_log(" sample frequency: %s\n",
> +
> hdmi_audio_sample_frequency_get_name(frame->sample_frequency));
> + hdmi_log(" coding type ext: %s\n",
> +
> hdmi_audio_coding_type_ext_get_name(frame->coding_type_ext));
> + hdmi_log(" channel allocation: %d\n",
> + frame->channel_allocation);
The table for this is rather huge, so it's probably not a good idea to
return a string representation, but perhaps printing in hex would make
it easier to relate to the specification?
> + hdmi_log(" level shift value: %d db\n",
> + frame->level_shift_value);
Could be "%u" again. Also "db" -> "dB".
> +hdmi_vendor_any_infoframe_log(const char *level,
> + struct device *dev,
> + union hdmi_vendor_any_infoframe *frame)
> +{
[...]
> + if (hvf->vic)
> + hdmi_log(" HDMI VIC: %d\n", hvf->vic);
%u
> + if (hvf->s3d_struct != HDMI_3D_STRUCTURE_INVALID) {
> + hdmi_log(" 3D structure: %s\n",
> + hdmi_3d_structure_get_name(hvf->s3d_struct));
> + if (hvf->s3d_struct >= HDMI_3D_STRUCTURE_SIDE_BY_SIDE_HALF)
> + hdmi_log(" 3D extension data: %d\n",
> + hvf->s3d_ext_data);
%u
> +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?
> +static int hdmi_spd_infoframe_unpack(struct hdmi_spd_infoframe *frame,
> + void *buffer)
> +{
> + u8 *ptr = buffer;
> + int ret;
> +
> + if (ptr[0] != HDMI_INFOFRAME_TYPE_SPD ||
> + ptr[1] != 1 ||
> + ptr[2] != HDMI_SPD_INFOFRAME_SIZE) {
> + return -EINVAL;
> + }
> +
> + if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(SPD)) != 0)
> + return -EINVAL;
Same here.
> +static int hdmi_audio_infoframe_unpack(struct hdmi_audio_infoframe *frame,
> + void *buffer)
> +{
> + u8 *ptr = buffer;
> + int ret;
> +
> + if (ptr[0] != HDMI_INFOFRAME_TYPE_AUDIO ||
> + ptr[1] != 1 ||
> + ptr[2] != HDMI_AUDIO_INFOFRAME_SIZE) {
> + return -EINVAL;
> + }
> +
> + if (hdmi_infoframe_checksum(buffer, HDMI_INFOFRAME_SIZE(AUDIO)) != 0)
> + return -EINVAL;
And here.
> +static int
> +hdmi_vendor_any_infoframe_unpack(union hdmi_vendor_any_infoframe *frame,
> + void *buffer)
> +{
[...]
> + /* HDMI OUI */
> + if ((ptr[0] != 0x03) ||
> + (ptr[1] != 0x0c) ||
> + (ptr[2] != 0x00))
> + return -EINVAL;
It'd be nice if this would actually use the HDMI_IEEE_OUI constant. The
_pack() function hardcodes this too, so I guess it's fine and something
that could be cleaned up later on if somebody cares enough.
Thierry
pgpn6NpdMPWZd.pgp
Description: PGP signature
