[PATCH 2/3] hdmi: added unpack and logging functions for InfoFrames

[Thierry Reding]

On Fri, Nov 28, 2014 at 03:50:50PM +0100, Hans Verkuil wrote:
> From: Martin Bugge <marbugge at 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 <marbugge at cisco.com>
> Signed-off-by: Hans Verkuil <hans.verkuil at 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 char *hdmi_quantization_range_txt(enum hdmi_quantization_range quantization_range)
> +{
> +	switch (quantization_range) {
> +	case HDMI_QUANTIZATION_RANGE_DEFAULT: return "Default (depends on video format)";

I think "Default" would do here ("depends on video format" can be
derived from the reading of the specification). Generally I think these
should focus on providing a human-readable version of the infoframes,
not be a replacement for reading the specification.

> +/**
> + * hdmi_avi_infoframe_log() - log info of HDMI AVI infoframe
> + * @dev: device
> + * @frame: HDMI AVI infoframe
> + */
> +static void hdmi_avi_infoframe_log(struct device *dev, struct hdmi_avi_infoframe *frame)

Perhaps allow this to take a log level? I can imagine drivers wanting to
use this with dev_dbg() instead.

> +/**
> + * hdmi_vendor_infoframe_log() - log info of HDMI VENDOR infoframe
> + * @dev: device
> + * @frame: HDMI VENDOR infoframe
> + */
> +static void hdmi_vendor_any_infoframe_log(struct device *dev, union hdmi_vendor_any_infoframe *frame)
> +{
> +	struct hdmi_vendor_infoframe *hvf = &frame->hdmi;
> +
> +	hdmi_infoframe_log_header(dev, frame);
> +
> +	if (frame->any.oui != HDMI_IEEE_OUI) {
> +		dev_info(dev, "    not a HDMI vendor infoframe\n");
> +		return;
> +	}
> +	if (hvf->vic == 0 && hvf->s3d_struct == HDMI_3D_STRUCTURE_INVALID) {
> +		dev_info(dev, "    empty frame\n");
> +		return;
> +	}
> +
> +	if (hvf->vic) {
> +		dev_info(dev, "    Hdmi Vic: %d\n", hvf->vic);

"HDMI VIC"?

> +	}

No need for these braces.

> +/**
> + * 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.

> +/**
> + * hdmi_avi_infoframe_unpack() - unpack binary buffer to a HDMI AVI infoframe
> + * @buffer: source buffer
> + * @frame: HDMI AVI infoframe
> + *
> + * Unpacks the information contained in binary @buffer into a structured
> + * @frame of the HDMI Auxiliary Video (AVI) 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_avi_infoframe_unpack(void *buffer, struct hdmi_avi_infoframe *frame)

I'm on the fence about ordering of arguments here. I think I'd slightly
prefer the infoframe to be the first, to make the API more object-
oriented.

> +{
> +	u8 *ptr = buffer;
> +	int ret;
> +
> +	if (ptr[0] != HDMI_INFOFRAME_TYPE_AVI ||
> +	    ptr[1] != 2 ||
> +	    ptr[2] != HDMI_AVI_INFOFRAME_SIZE) {
> +		return -EINVAL;
> +	}

No need for the braces.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20141201/b49d69f2/attachment.sig>