[RFC 0/2] Add HDMI helpers

[RFC 0/2] Add HDMI helpers

[Thierry Reding]

This small series is very much work in progress, but I still wanted to
get feedback in this early stage to gather requirements from the folks
working on the display drivers that these helpers target.

Patch 1 in the series adds a generic helper to pack a structure that
describes an HDMI AVI infoframe into the binary format as specified in
the HDMI specification. The resulting binary buffer should be easily
programmable into the HDMI controller.

Patch 2 provides a helper to fill an HDMI AVI infoframe with data from
a struct drm_display_mode.

This is all pretty rough right now, but I think some feedback would be
good at this point, to see if the design is at all sensible. I should
also mention that I haven't actually tested this on real hardware yet.
Furthermore I have plans to add something similar for the other types
of infoframes specified by HDMI once the direction becomes clearer.

Thierry

Thierry Reding (2):
  video: Add generic HDMI helpers
  drm: Add HDMI helpers

 drivers/gpu/drm/Kconfig    |   7 +++
 drivers/gpu/drm/Makefile   |   1 +
 drivers/gpu/drm/drm_hdmi.c | 107 ++++++++++++++++++++++++++++++++++++++++
 drivers/video/Kconfig      |   3 ++
 drivers/video/Makefile     |   1 +
 drivers/video/hdmi.c       |  84 ++++++++++++++++++++++++++++++++
 include/drm/drm_hdmi.h     |  18 +++++++
 include/linux/hdmi.h       | 119 +++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 340 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_hdmi.c
 create mode 100644 drivers/video/hdmi.c
 create mode 100644 include/drm/drm_hdmi.h
 create mode 100644 include/linux/hdmi.h

-- 
1.8.0

[RFC 1/2] video: Add generic HDMI helpers

[Thierry Reding]

Add generic helpers to pack HDMI infoframes into a binary buffers.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/video/Kconfig  |   3 ++
 drivers/video/Makefile |   1 +
 drivers/video/hdmi.c   |  84 ++++++++++++++++++++++++++++++++++
 include/linux/hdmi.h   | 119 +++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 207 insertions(+)
 create mode 100644 drivers/video/hdmi.c
 create mode 100644 include/linux/hdmi.h

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index fb9a14e..17ac897 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -52,6 +52,9 @@ config OF_VIDEOMODE
 	help
 	  helper to get videomodes from the devicetree
 
+config HDMI
+	bool
+
 menuconfig FB
 	tristate "Support for frame buffer devices"
 	---help---
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index b936b00..d1d38ea 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -5,6 +5,7 @@
 # Each configuration option enables a list of files.
 
 obj-$(CONFIG_VGASTATE)            += vgastate.o
+obj-$(CONFIG_HDMI)                += hdmi.o
 obj-y                             += fb_notify.o
 obj-$(CONFIG_FB)                  += fb.o
 fb-y                              := fbmem.o fbmon.o fbcmap.o fbsysfs.o \
diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
new file mode 100644
index 0000000..3d6ee5e
--- /dev/null
+++ b/drivers/video/hdmi.c
@@ -0,0 +1,84 @@
+/*
+ * Copyright (C) 2012 Avionic Design GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/bitops.h>
+#include <linux/errno.h>
+#include <linux/export.h>
+#include <linux/hdmi.h>
+
+/**
+ * hdmi_avi_infoframe_pack() - write HDMI AVI infoframe to binary buffer
+ * @frame: HDMI AVI infoframe
+ * @buffer: destination buffer
+ * @size: size of buffer
+ *
+ * Packs the information contained in the @frame structure into a binary
+ * representation that can be written into the corresponding controller
+ * registers. Also computes the checksum as required by section 5.3.5 of the
+ * HDMI 1.4 specification.
+ */
+ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe *frame, void *buffer,
+				size_t size)
+{
+	u8 *ptr = buffer;
+	unsigned int i;
+	u8 csum = 0;
+
+	if (frame->length > 0x1f)
+		return -EINVAL;
+
+	ptr[0] = frame->type;
+	ptr[1] = frame->version;
+	ptr[2] = frame->length;
+	ptr[3] = 0; /* checksum */
+	ptr[4] = ((frame->colorspace & 0x3) << 5) | (frame->scan_mode & 0x3);
+
+	if (frame->active_info_valid)
+		ptr[4] |= BIT(4);
+
+	if (frame->horizontal_bar_valid)
+		ptr[4] |= BIT(3);
+
+	if (frame->vertical_bar_valid)
+		ptr[4] |= BIT(2);
+
+	ptr[5] = ((frame->colorimetry & 0x3) << 6) |
+		 ((frame->picture_aspect & 0x3) << 4) |
+		 (frame->active_aspect & 0xf);
+
+	ptr[6] = ((frame->extended_colorimetry & 0x7) << 4) |
+		 ((frame->quantization_range & 0x3) << 2) |
+		 (frame->nups & 0x3);
+
+	if (frame->itc)
+		ptr[6] |= BIT(7);
+
+	ptr[7] = frame->video_code & 0x7f;
+
+	ptr[8] = ((frame->ycc_quantization_range & 0x3) << 6) |
+		 ((frame->content_type & 0x3) << 4) |
+		 (frame->pixel_repeat & 0xf);
+
+	ptr[9] = frame->top_bar & 0xff;
+	ptr[10] = (frame->top_bar >> 8) & 0xff;
+	ptr[11] = frame->bottom_bar & 0xff;
+	ptr[12] = (frame->bottom_bar >> 8) & 0xff;
+	ptr[13] = frame->left_bar & 0xff;
+	ptr[14] = (frame->left_bar >> 8) & 0xff;
+	ptr[15] = frame->right_bar & 0xff;
+	ptr[16] = (frame->right_bar >> 8) & 0xff;
+
+	/* compute checksum */
+	for (i = 0; i < 4 + frame->length; i++)
+		csum += ptr[i];
+
+	ptr[3] = 256 - csum;
+
+	return 4 + frame->length;
+}
+EXPORT_SYMBOL_GPL(hdmi_avi_infoframe_pack);
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
new file mode 100644
index 0000000..132556f
--- /dev/null
+++ b/include/linux/hdmi.h
@@ -0,0 +1,119 @@
+/*
+ * Copyright (C) 2012 Avionic Design GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __LINUX_HDMI_H_
+#define __LINUX_HDMI_H_
+
+#include <linux/types.h>
+
+enum hdmi_infoframe_type {
+	HDMI_INFOFRAME_TYPE_AVI,
+};
+
+enum hdmi_colorspace {
+	HDMI_COLORSPACE_RGB,
+	HDMI_COLORSPACE_YUV422,
+	HDMI_COLORSPACE_YUV444,
+};
+
+enum hdmi_scan_mode {
+	HDMI_SCAN_MODE_NONE,
+	HDMI_SCAN_MODE_OVERSCAN,
+	HDMI_SCAN_MODE_UNDERSCAN,
+};
+
+enum hdmi_colorimetry {
+	HDMI_COLORIMETRY_NONE,
+	HDMI_COLORIMETRY_ITU_601,
+	HDMI_COLORIMETRY_ITU_709,
+	HDMI_COLORIMETRY_EXTENDED,
+};
+
+enum hdmi_picture_aspect {
+	HDMI_PICTURE_ASPECT_NONE,
+	HDMI_PICTURE_ASPECT_4_3,
+	HDMI_PICTURE_ASPECT_16_9,
+};
+
+enum hdmi_active_aspect {
+	HDMI_ACTIVE_ASPECT_16_9_TOP = 2,
+	HDMI_ACTIVE_ASPECT_14_9_TOP = 3,
+	HDMI_ACTIVE_ASPECT_16_9_CENTER = 4,
+	HDMI_ACTIVE_ASPECT_PICTURE = 8,
+	HDMI_ACTIVE_ASPECT_4_3 = 9,
+	HDMI_ACTIVE_ASPECT_16_9 = 10,
+	HDMI_ACTIVE_ASPECT_14_9 = 11,
+	HDMI_ACTIVE_ASPECT_4_3_SP_14_9 = 13,
+	HDMI_ACTIVE_ASPECT_16_9_SP_14_9 = 14,
+	HDMI_ACTIVE_ASPECT_16_9_SP_4_3 = 15,
+};
+
+enum hdmi_extended_colorimetry {
+	HDMI_EXTENDED_COLORIMETRY_XV_YCC_601,
+	HDMI_EXTENDED_COLORIMETRY_XV_YCC_709,
+	HDMI_EXTENDED_COLORIMETRY_S_YCC_601,
+	HDMI_EXTENDED_COLORIMETRY_ADOBE_YCC_601,
+	HDMI_EXTENDED_COLORIMETRY_ADOBE_RGB,
+};
+
+enum hdmi_quantization_range {
+	HDMI_QUANTIZATION_RANGE_DEFAULT,
+	HDMI_QUANTIZATION_RANGE_LIMITED,
+	HDMI_QUANTIZATION_RANGE_FULL,
+};
+
+/* non-uniform picture scaling */
+enum hdmi_nups {
+	HDMI_NUPS_UNKNOWN,
+	HDMI_NUPS_HORIZONTAL,
+	HDMI_NUPS_VERTICAL,
+	HDMI_NUPS_BOTH,
+};
+
+enum hdmi_ycc_quantization_range {
+	HDMI_YCC_QUANTIZATION_RANGE_LIMITED,
+	HDMI_YCC_QUANTIZATION_RANGE_FULL,
+};
+
+enum hdmi_content_type {
+	HDMI_CONTENT_TYPE_NONE,
+	HDMI_CONTENT_TYPE_PHOTO,
+	HDMI_CONTENT_TYPE_CINEMA,
+	HDMI_CONTENT_TYPE_GAME,
+};
+
+struct hdmi_avi_infoframe {
+	enum hdmi_infoframe_type type;
+	unsigned char version;
+	unsigned char length;
+	enum hdmi_colorspace colorspace;
+	bool active_info_valid;
+	bool horizontal_bar_valid;
+	bool vertical_bar_valid;
+	enum hdmi_scan_mode scan_mode;
+	enum hdmi_colorimetry colorimetry;
+	enum hdmi_picture_aspect picture_aspect;
+	enum hdmi_active_aspect active_aspect;
+	bool itc;
+	enum hdmi_extended_colorimetry extended_colorimetry;
+	enum hdmi_quantization_range quantization_range;
+	enum hdmi_nups nups;
+	unsigned char video_code;
+	enum hdmi_ycc_quantization_range ycc_quantization_range;
+	enum hdmi_content_type content_type;
+	unsigned char pixel_repeat;
+	unsigned short top_bar;
+	unsigned short bottom_bar;
+	unsigned short left_bar;
+	unsigned short right_bar;
+};
+
+ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe *frame, void *buffer,
+				size_t size);
+
+#endif /* _DRM_HDMI_H */
-- 
1.8.0

[RFC 2/2] drm: Add HDMI helpers

[Thierry Reding]

Add a generic helper to fill in an HDMI AVI infoframe with data
extracted from a DRM display mode.

Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
 drivers/gpu/drm/Kconfig    |   7 +++
 drivers/gpu/drm/Makefile   |   1 +
 drivers/gpu/drm/drm_hdmi.c | 107 +++++++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_hdmi.h     |  18 ++++++++
 4 files changed, 133 insertions(+)
 create mode 100644 drivers/gpu/drm/drm_hdmi.c
 create mode 100644 include/drm/drm_hdmi.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 983201b..94a4623 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -69,6 +69,13 @@ config DRM_KMS_CMA_HELPER
 	help
 	  Choose this if you need the KMS CMA helper functions
 
+config DRM_HDMI
+	bool
+	depends on DRM
+	select HDMI
+	help
+	  Choose this if you need the HDMI helper functions
+
 config DRM_TDFX
 	tristate "3dfx Banshee/Voodoo3+"
 	depends on DRM && PCI
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 0bfda06..330451b 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -16,6 +16,7 @@ drm-y       :=	drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \
 
 drm-$(CONFIG_COMPAT) += drm_ioc32.o
 drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
+drm-$(CONFIG_DRM_HDMI) += drm_hdmi.o
 
 drm-usb-y   := drm_usb.o
 
diff --git a/drivers/gpu/drm/drm_hdmi.c b/drivers/gpu/drm/drm_hdmi.c
new file mode 100644
index 0000000..1a8d914
--- /dev/null
+++ b/drivers/gpu/drm/drm_hdmi.c
@@ -0,0 +1,107 @@
+/*
+ * Copyright (C) 2012 Avionic Design GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/hdmi.h>
+
+#include <drm/drm_crtc.h>
+#include <drm/drm_hdmi.h>
+
+struct hdmi_cea_mode {
+	unsigned char vic;
+	unsigned int width;
+	unsigned int height;
+	unsigned int refresh;
+	enum hdmi_picture_aspect aspect;
+	bool interlaced;
+	unsigned int repeat_min;
+	unsigned int repeat_max;
+};
+
+static const struct hdmi_cea_mode hdmi_cea_modes[] = {
+	{  1,  640,  480,  60, HDMI_PICTURE_ASPECT_4_3,  false, 0, 0 },
+	{  2,  720,  480,  60, HDMI_PICTURE_ASPECT_NONE, false, 0, 0 },
+	{  4, 1280,  720,  60, HDMI_PICTURE_ASPECT_16_9, false, 0, 0 },
+	{  5, 1920, 1080,  60, HDMI_PICTURE_ASPECT_16_9, true,  0, 0 },
+	{  6,  720,  480,  60, HDMI_PICTURE_ASPECT_NONE, true,  1, 1 },
+	{  8,  720,  240,  60, HDMI_PICTURE_ASPECT_NONE, false, 1, 1 },
+	{ 10, 2880,  480,  60, HDMI_PICTURE_ASPECT_NONE, true,  0, 9 },
+	{ 12, 2880,  240,  60, HDMI_PICTURE_ASPECT_NONE, false, 0, 9 },
+	{ 14, 1440,  480,  60, HDMI_PICTURE_ASPECT_NONE, false, 0, 1 },
+	{ 16, 1920, 1080,  60, HDMI_PICTURE_ASPECT_16_9, false, 0, 0 },
+	{ 17,  720,  576,  50, HDMI_PICTURE_ASPECT_NONE, false, 0, 0 },
+	{ 19, 1280,  720,  50, HDMI_PICTURE_ASPECT_16_9, false, 0, 0 },
+	{ 20, 1920, 1080,  50, HDMI_PICTURE_ASPECT_16_9, true,  0, 0 },
+	{ 21,  720,  576,  50, HDMI_PICTURE_ASPECT_NONE, true,  1, 1 },
+	{ 23,  720,  288,  50, HDMI_PICTURE_ASPECT_NONE, false, 1, 1 },
+	{ 25, 2880,  576,  50, HDMI_PICTURE_ASPECT_NONE, true,  0, 9 },
+	{ 27, 2880,  288,  50, HDMI_PICTURE_ASPECT_NONE, false, 0, 9 },
+	{ 29, 1440,  576,  50, HDMI_PICTURE_ASPECT_NONE, false, 0, 1 },
+	{ 31, 1920, 1080,  50, HDMI_PICTURE_ASPECT_16_9, false, 0, 0 },
+	{ 32, 1920, 1080,  24, HDMI_PICTURE_ASPECT_16_9, false, 0, 0 },
+	{ 33, 1920, 1080,  25, HDMI_PICTURE_ASPECT_16_9, false, 0, 0 },
+	{ 34, 1920, 1080,  30, HDMI_PICTURE_ASPECT_16_9, false, 0, 0 },
+	{ 35, 2880,  480,  60, HDMI_PICTURE_ASPECT_NONE, false, 0, 3 },
+	{ 37, 2880,  576,  50, HDMI_PICTURE_ASPECT_NONE, false, 0, 3 },
+	{ 39, 1920, 1080,  50, HDMI_PICTURE_ASPECT_16_9, true,  0, 0 },
+	{ 40, 1920, 1080, 100, HDMI_PICTURE_ASPECT_16_9, true,  0, 0 },
+	{ 41, 1280,  720, 100, HDMI_PICTURE_ASPECT_16_9, false, 0, 0 },
+	{ 42,  720,  576, 100, HDMI_PICTURE_ASPECT_NONE, false, 0, 0 },
+	{ 43,  720,  576, 100, HDMI_PICTURE_ASPECT_NONE, false, 0, 0 },
+	{ 44,  720,  576, 100, HDMI_PICTURE_ASPECT_NONE, true,  1, 1 },
+	{ 45,  720,  576, 100, HDMI_PICTURE_ASPECT_NONE, true,  1, 1 },
+	{ 46, 1920, 1080, 120, HDMI_PICTURE_ASPECT_16_9, true,  0, 0 },
+	{ 47, 1280,  720, 120, HDMI_PICTURE_ASPECT_16_9, false, 0, 0 },
+	{ 48,  720,  480, 120, HDMI_PICTURE_ASPECT_NONE, false, 0, 0 },
+	{ 49,  720,  480, 120, HDMI_PICTURE_ASPECT_NONE, false, 0, 0 },
+	{ 50,  720,  480, 120, HDMI_PICTURE_ASPECT_NONE, true,  1, 1 },
+	{ 51,  720,  480, 120, HDMI_PICTURE_ASPECT_NONE, true,  1, 1 },
+	{ 52,  720,  576, 200, HDMI_PICTURE_ASPECT_NONE, false, 0, 0 },
+	{ 53,  720,  576, 200, HDMI_PICTURE_ASPECT_NONE, false, 0, 0 },
+	{ 54,  720,  576, 200, HDMI_PICTURE_ASPECT_NONE, true,  1, 1 },
+	{ 55,  720,  576, 200, HDMI_PICTURE_ASPECT_NONE, true,  1, 1 },
+	{ 56,  720,  480, 240, HDMI_PICTURE_ASPECT_NONE, false, 0, 0 },
+	{ 57,  720,  480, 240, HDMI_PICTURE_ASPECT_NONE, false, 0, 0 },
+	{ 58,  720,  480, 240, HDMI_PICTURE_ASPECT_NONE, true,  1, 1 },
+	{ 59,  720,  480, 240, HDMI_PICTURE_ASPECT_NONE, true,  1, 1 },
+	{ 60, 1280,  720,  24, HDMI_PICTURE_ASPECT_16_9, false, 0, 0 },
+	{ 61, 1280,  720,  25, HDMI_PICTURE_ASPECT_16_9, false, 0, 0 },
+	{ 62, 1280,  720,  30, HDMI_PICTURE_ASPECT_16_9, false, 0, 0 },
+	{ 63, 1920, 1080, 120, HDMI_PICTURE_ASPECT_16_9, false, 0, 0 },
+	{ 64, 1920, 1080, 100, HDMI_PICTURE_ASPECT_16_9, false, 0, 0 },
+};
+
+int drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
+					     struct drm_display_mode *mode)
+{
+	unsigned int i;
+
+	memset(frame, 0, sizeof(*frame));
+
+	frame->type = HDMI_INFOFRAME_TYPE_AVI;
+	frame->version = 2;
+	frame->length = 13;
+
+	for (i = 0; i < ARRAY_SIZE(hdmi_cea_modes); i++) {
+		const struct hdmi_cea_mode *cea = &hdmi_cea_modes[i];
+
+		if (mode->hdisplay != cea->width ||
+		    mode->vdisplay != cea->height)
+			continue;
+
+		if ((mode->flags & DRM_MODE_FLAG_INTERLACE) && !cea->interlaced)
+			continue;
+
+		frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
+		frame->picture_aspect = cea->aspect;
+		frame->video_code = cea->vic;
+		break;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(drm_hdmi_avi_infoframe_from_display_mode);
diff --git a/include/drm/drm_hdmi.h b/include/drm/drm_hdmi.h
new file mode 100644
index 0000000..07ce049
--- /dev/null
+++ b/include/drm/drm_hdmi.h
@@ -0,0 +1,18 @@
+/*
+ * Copyright (C) 2012 Avionic Design GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _DRM_HDMI_H_
+#define _DRM_HDMI_H_
+
+struct hdmi_avi_infoframe;
+struct drm_display_mode;
+
+int drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
+					     struct drm_display_mode *mode);
+
+#endif
-- 
1.8.0

Re: [RFC 0/2] Add HDMI helpers

[Christian König]

Hi Thierry,

On 21.11.2012 16:01, Thierry Reding wrote:
> This small series is very much work in progress, but I still wanted to
> get feedback in this early stage to gather requirements from the folks
> working on the display drivers that these helpers target.
>
> Patch 1 in the series adds a generic helper to pack a structure that
> describes an HDMI AVI infoframe into the binary format as specified in
> the HDMI specification. The resulting binary buffer should be easily
> programmable into the HDMI controller.
>
> Patch 2 provides a helper to fill an HDMI AVI infoframe with data from
> a struct drm_display_mode.
>
> This is all pretty rough right now, but I think some feedback would be
> good at this point, to see if the design is at all sensible. I should
> also mention that I haven't actually tested this on real hardware yet.
> Furthermore I have plans to add something similar for the other types
> of infoframes specified by HDMI once the direction becomes clearer.
In general I like the idea of storing the informations in a C struct and 
only packing it into the binary form when needed.

I would rather like to see a complete implementation of all the 
interesting HDMI packets, including the necessary calculations/tables 
for audio timing recovery etc before it gets committed upstream.

Not sure about the separate configuration option. I'm not so much into 
the config/build system of linux (I know that it is rather complicated), 
but in general I would like to see that activated automatically as soon 
as any driver starts using it (or at least the driver depending on that 
option to be active).

Also two notes about the code in itself:
1. hdmi_avi_infoframe_pack: gets the size of the target buffer, but 
unfortunately doesn't checks it.
2. Separate the CRC calculation, we probably need that more than once.

Thanks for looking into that, keep up with the work.

Cheers,
Christian.

Re: [RFC 0/2] Add HDMI helpers

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2923 bytes --]

On Fri, Nov 23, 2012 at 10:24:22AM +0100, Christian König wrote:
> Hi Thierry,
> 
> On 21.11.2012 16:01, Thierry Reding wrote:
> >This small series is very much work in progress, but I still wanted to
> >get feedback in this early stage to gather requirements from the folks
> >working on the display drivers that these helpers target.
> >
> >Patch 1 in the series adds a generic helper to pack a structure that
> >describes an HDMI AVI infoframe into the binary format as specified in
> >the HDMI specification. The resulting binary buffer should be easily
> >programmable into the HDMI controller.
> >
> >Patch 2 provides a helper to fill an HDMI AVI infoframe with data from
> >a struct drm_display_mode.
> >
> >This is all pretty rough right now, but I think some feedback would be
> >good at this point, to see if the design is at all sensible. I should
> >also mention that I haven't actually tested this on real hardware yet.
> >Furthermore I have plans to add something similar for the other types
> >of infoframes specified by HDMI once the direction becomes clearer.
> In general I like the idea of storing the informations in a C struct
> and only packing it into the binary form when needed.
> 
> I would rather like to see a complete implementation of all the
> interesting HDMI packets, including the necessary
> calculations/tables for audio timing recovery etc before it gets
> committed upstream.

I'm afraid I'm not very familiar with those packets. Also I haven't seen
any actual users of audio timing recovery packets so far, so I'm not
sure that adding support for them would be very useful.

What I have added so far is support for audio and vendor-specific
infoframes, since those have actual users. I also noticed that i915
implements SPD infoframes as well, so I can extract that code into the
generic helpers as well.

> Not sure about the separate configuration option. I'm not so much
> into the config/build system of linux (I know that it is rather
> complicated), but in general I would like to see that activated
> automatically as soon as any driver starts using it (or at least the
> driver depending on that option to be active).

The configuration option isn't supposed to be user visible. Drivers that
make use of this are supposed to select HDMI or DRM_HDMI to cause the
generic code to be pulled in.

> Also two notes about the code in itself:
> 1. hdmi_avi_infoframe_pack: gets the size of the target buffer, but
> unfortunately doesn't checks it.

That should already be fixed in the latest code in my local working
branch.

> 2. Separate the CRC calculation, we probably need that more than once.

I've also pulled that out into a separate function. I haven't made it a
public function yet, but I don't think that'll be necessary either since
it is used internally by the various infoframe packing functions.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 0/2] Add HDMI helpers

[Rafał Miłecki]

2012/11/23 Christian König <deathsimple@vodafone.de>:
> Hi Thierry,
>
>
> On 21.11.2012 16:01, Thierry Reding wrote:
>>
>> This small series is very much work in progress, but I still wanted to
>> get feedback in this early stage to gather requirements from the folks
>> working on the display drivers that these helpers target.
>>
>> Patch 1 in the series adds a generic helper to pack a structure that
>> describes an HDMI AVI infoframe into the binary format as specified in
>> the HDMI specification. The resulting binary buffer should be easily
>> programmable into the HDMI controller.
>>
>> Patch 2 provides a helper to fill an HDMI AVI infoframe with data from
>> a struct drm_display_mode.
>>
>> This is all pretty rough right now, but I think some feedback would be
>> good at this point, to see if the design is at all sensible. I should
>> also mention that I haven't actually tested this on real hardware yet.
>> Furthermore I have plans to add something similar for the other types
>> of infoframes specified by HDMI once the direction becomes clearer.
>
> In general I like the idea of storing the informations in a C struct and
> only packing it into the binary form when needed.
>
> I would rather like to see a complete implementation of all the interesting
> HDMI packets, including the necessary calculations/tables for audio timing
> recovery etc before it gets committed upstream.
>
> Not sure about the separate configuration option. I'm not so much into the
> config/build system of linux (I know that it is rather complicated), but in
> general I would like to see that activated automatically as soon as any
> driver starts using it (or at least the driver depending on that option to
> be active).

It's rather trivial ;) You just use "select FOO_BAR".

-- 
Rafał
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC 0/2] Add HDMI helpers

[Christian König]

On 23.11.2012 10:38, Thierry Reding wrote:
> On Fri, Nov 23, 2012 at 10:24:22AM +0100, Christian König wrote:
>> Hi Thierry,
>>
>> On 21.11.2012 16:01, Thierry Reding wrote:
>>> This small series is very much work in progress, but I still wanted to
>>> get feedback in this early stage to gather requirements from the folks
>>> working on the display drivers that these helpers target.
>>>
>>> Patch 1 in the series adds a generic helper to pack a structure that
>>> describes an HDMI AVI infoframe into the binary format as specified in
>>> the HDMI specification. The resulting binary buffer should be easily
>>> programmable into the HDMI controller.
>>>
>>> Patch 2 provides a helper to fill an HDMI AVI infoframe with data from
>>> a struct drm_display_mode.
>>>
>>> This is all pretty rough right now, but I think some feedback would be
>>> good at this point, to see if the design is at all sensible. I should
>>> also mention that I haven't actually tested this on real hardware yet.
>>> Furthermore I have plans to add something similar for the other types
>>> of infoframes specified by HDMI once the direction becomes clearer.
>> In general I like the idea of storing the informations in a C struct
>> and only packing it into the binary form when needed.
>>
>> I would rather like to see a complete implementation of all the
>> interesting HDMI packets, including the necessary
>> calculations/tables for audio timing recovery etc before it gets
>> committed upstream.
> I'm afraid I'm not very familiar with those packets. Also I haven't seen
> any actual users of audio timing recovery packets so far, so I'm not
> sure that adding support for them would be very useful.

Take a look at: drivers/gpu/drm/radeon/r600_hdmi.c There we have 
r600_hdmi_predefined_acr, r600_hdmi_calc_cts and r600_hdmi_acr doing the 
audio clock recovery calculation/table manually. Those values are only 
used as fallback, since the hardware can do the calculation on their own.

I'm not sure if anybody else is doing this calculation in any driver 
code, but it definitely isn't driver specific and part of the HDMI 
specification, so I would say that it belongs into the general purpose 
file as well.

> What I have added so far is support for audio and vendor-specific
> infoframes, since those have actual users. I also noticed that i915
> implements SPD infoframes as well, so I can extract that code into the
> generic helpers as well.
>
>> Not sure about the separate configuration option. I'm not so much
>> into the config/build system of linux (I know that it is rather
>> complicated), but in general I would like to see that activated
>> automatically as soon as any driver starts using it (or at least the
>> driver depending on that option to be active).
> The configuration option isn't supposed to be user visible. Drivers that
> make use of this are supposed to select HDMI or DRM_HDMI to cause the
> generic code to be pulled in.
>
>> Also two notes about the code in itself:
>> 1. hdmi_avi_infoframe_pack: gets the size of the target buffer, but
>> unfortunately doesn't checks it.
> That should already be fixed in the latest code in my local working
> branch.
>
>> 2. Separate the CRC calculation, we probably need that more than once.
> I've also pulled that out into a separate function. I haven't made it a
> public function yet, but I don't think that'll be necessary either since
> it is used internally by the various infoframe packing functions.
>

Sounds good, let me know when you need a review of the code.

Christian.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel