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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8815 bytes --]

On Tue, Dec 02, 2014 at 01:08:45PM +0100, Hans Verkuil wrote:
> From: Martin Bugge <marbugge@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@cisco.com>
> Signed-off-by: Hans Verkuil <hans.verkuil@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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]