* [RFC v2 1/5] drm: Add some missing forward declarations
2012-12-05 16:45 [RFC v2 0/5] Add HDMI infoframe helpers Thierry Reding
@ 2012-12-05 16:45 ` Thierry Reding
2012-12-05 16:45 ` [RFC v2 2/5] video: Add generic HDMI infoframe helpers Thierry Reding
` (3 subsequent siblings)
4 siblings, 0 replies; 30+ messages in thread
From: Thierry Reding @ 2012-12-05 16:45 UTC (permalink / raw)
To: dri-devel
The drm_file and drm_clip_rect structures are used throughout the file
but they are never declared nor pulled in through an include. Add
forward declarations to make them available.
Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
include/drm/drm_crtc.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index c0635b7..71a5c74 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -38,7 +38,8 @@ struct drm_device;
struct drm_mode_set;
struct drm_framebuffer;
struct drm_object_properties;
-
+struct drm_file;
+struct drm_clip_rect;
#define DRM_MODE_OBJECT_CRTC 0xcccccccc
#define DRM_MODE_OBJECT_CONNECTOR 0xc0c0c0c0
--
1.8.0.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [RFC v2 2/5] video: Add generic HDMI infoframe helpers
2012-12-05 16:45 [RFC v2 0/5] Add HDMI infoframe helpers Thierry Reding
2012-12-05 16:45 ` [RFC v2 1/5] drm: Add some missing forward declarations Thierry Reding
@ 2012-12-05 16:45 ` Thierry Reding
2012-12-07 14:32 ` Paulo Zanoni
2012-12-05 16:45 ` [RFC v2 3/5] drm: Add " Thierry Reding
` (2 subsequent siblings)
4 siblings, 1 reply; 30+ messages in thread
From: Thierry Reding @ 2012-12-05 16:45 UTC (permalink / raw)
To: dri-devel
Add generic helpers to pack HDMI infoframes into binary buffers.
Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
Changes in v2:
- add support for audio, vendor-specific and SPD infoframes
- add various validity checks on infoframes
- factor out checksum computation
drivers/video/Kconfig | 3 +
drivers/video/Makefile | 1 +
drivers/video/hdmi.c | 322 +++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/hdmi.h | 220 +++++++++++++++++++++++++++++++++
4 files changed, 546 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 807c7fa..baa3f90 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -54,6 +54,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 f592f3b..0b50082 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..6b95270
--- /dev/null
+++ b/drivers/video/hdmi.c
@@ -0,0 +1,322 @@
+/*
+ * 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>
+#include <linux/string.h>
+
+static void hdmi_infoframe_checksum(void *buffer, size_t size)
+{
+ u8 *ptr = buffer;
+ u8 csum = 0;
+ size_t i;
+
+ /* compute checksum */
+ for (i = 0; i < size; i++)
+ csum += ptr[i];
+
+ ptr[3] = 256 - csum;
+}
+
+/**
+ * hdmi_avi_infoframe_init() - initialize an HDMI AVI infoframe
+ * @frame: HDMI AVI infoframe
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int hdmi_avi_infoframe_init(struct hdmi_avi_infoframe *frame)
+{
+ if (!frame)
+ return -EINVAL;
+
+ memset(frame, 0, sizeof(*frame));
+
+ frame->type = HDMI_INFOFRAME_TYPE_AVI;
+ frame->version = 2;
+ frame->length = 13;
+
+ return 0;
+}
+EXPORT_SYMBOL(hdmi_avi_infoframe_init);
+
+/**
+ * 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.
+ *
+ * Returns the number of bytes packed into the binary buffer or a negative
+ * error code on failure.
+ */
+ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe *frame, void *buffer,
+ size_t size)
+{
+ u8 *ptr = buffer;
+ size_t length;
+
+ if (!frame || !buffer)
+ return -EINVAL;
+
+ length = 4 + frame->length;
+
+ if (size < length)
+ return -ENOSPC;
+
+ memset(buffer, 0, length);
+
+ 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;
+
+ hdmi_infoframe_checksum(buffer, length);
+
+ return length;
+}
+EXPORT_SYMBOL(hdmi_avi_infoframe_pack);
+
+/**
+ * hdmi_spd_infoframe_init() - initialize an HDMI SPD infoframe
+ * @frame: HDMI SPD infoframe
+ * @vendor: vendor string
+ * @product: product string
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int hdmi_spd_infoframe_init(struct hdmi_spd_infoframe *frame,
+ const char *vendor, const char *product)
+{
+ if (!frame)
+ return -EINVAL;
+
+ memset(frame, 0, sizeof(*frame));
+
+ frame->type = HDMI_INFOFRAME_TYPE_SPD;
+ frame->version = 1;
+ frame->length = 25;
+
+ strncpy(frame->vendor, vendor, sizeof(frame->vendor));
+ strncpy(frame->product, product, sizeof(frame->product));
+
+ return 0;
+}
+EXPORT_SYMBOL(hdmi_spd_infoframe_init);
+
+/**
+ * hdmi_spd_infoframe_pack() - write HDMI SPD infoframe to binary buffer
+ * @frame: HDMI SPD 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.
+ *
+ * Returns the number of bytes packed into the binary buffer or a negative
+ * error code on failure.
+ */
+ssize_t hdmi_spd_infoframe_pack(struct hdmi_spd_infoframe *frame, void *buffer,
+ size_t size)
+{
+ u8 *ptr = buffer;
+ size_t length, i;
+
+ if (!frame || !buffer)
+ return -EINVAL;
+
+ length = 4 + frame->length;
+
+ if (size < length)
+ return -ENOSPC;
+
+ memset(buffer, 0, length);
+
+ ptr[0] = frame->type;
+ ptr[1] = frame->version;
+ ptr[2] = frame->length;
+ ptr[3] = 0; /* checksum */
+
+ for (i = 0; i < sizeof(frame->vendor); i++)
+ ptr[4 + i] = frame->vendor[i];
+
+ for (i = 0; i < sizeof(frame->product); i++)
+ ptr[12 + i] = frame->product[i];
+
+ ptr[26] = frame->sdi;
+
+ hdmi_infoframe_checksum(buffer, length);
+
+ return length;
+}
+EXPORT_SYMBOL(hdmi_spd_infoframe_pack);
+
+/**
+ * hdmi_audio_infoframe_init() - initialize an HDMI audio infoframe
+ * @frame: HDMI audio infoframe
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int hdmi_audio_infoframe_init(struct hdmi_audio_infoframe *frame)
+{
+ if (!frame)
+ return -EINVAL;
+
+ memset(frame, 0, sizeof(*frame));
+
+ frame->type = HDMI_INFOFRAME_TYPE_AUDIO;
+ frame->version = 1;
+ frame->length = 10;
+
+ return 0;
+}
+EXPORT_SYMBOL(hdmi_audio_infoframe_init);
+
+/**
+ * hdmi_audio_infoframe_pack() - write HDMI audio infoframe to binary buffer
+ * @frame: HDMI audio 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.
+ *
+ * Returns the number of bytes packed into the binary buffer or a negative
+ * error code on failure.
+ */
+ssize_t hdmi_audio_infoframe_pack(struct hdmi_audio_infoframe *frame,
+ void *buffer, size_t size)
+{
+ unsigned char channels;
+ u8 *ptr = buffer;
+ size_t length;
+
+ if (!frame || !buffer)
+ return -EINVAL;
+
+ length = 4 + frame->length;
+
+ if (size < length)
+ return -ENOSPC;
+
+ memset(buffer, 0, length);
+
+ if (frame->channels >= 2)
+ channels = frame->channels - 1;
+ else
+ channels = 0;
+
+ ptr[0] = frame->type;
+ ptr[1] = frame->version;
+ ptr[2] = frame->length;
+ ptr[3] = 0; /* checksum */
+
+ ptr[4] = ((frame->coding_type & 0xf) << 4) | (channels & 0x7);
+ ptr[5] = ((frame->sample_frequency & 0x7) << 2) |
+ (frame->sample_size & 0x3);
+ ptr[6] = 0;
+ ptr[7] = frame->channel_allocation;
+ ptr[8] = (frame->level_shift_value & 0xf) << 3;
+
+ if (frame->downmix_inhibit)
+ ptr[8] |= BIT(7);
+
+ hdmi_infoframe_checksum(buffer, length);
+
+ return length;
+}
+EXPORT_SYMBOL(hdmi_audio_infoframe_pack);
+
+/**
+ * hdmi_vendor_infoframe_pack() - write a HDMI vendor infoframe to binary
+ * buffer
+ * @frame: HDMI vendor 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.
+ *
+ * Returns the number of bytes packed into the binary buffer or a negative
+ * error code on failure.
+ */
+ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
+ void *buffer, size_t size)
+{
+ u8 *ptr = buffer;
+ size_t length;
+
+ if (!frame || !buffer)
+ return -EINVAL;
+
+ length = 4 + frame->length;
+
+ if (size < length)
+ return -ENOSPC;
+
+ memset(buffer, 0, length);
+
+ ptr[0] = frame->type;
+ ptr[1] = frame->version;
+ ptr[2] = frame->length;
+ ptr[3] = 0; /* checksum */
+
+ memcpy(&ptr[4], frame->data, frame->length);
+
+ hdmi_infoframe_checksum(buffer, length);
+
+ return length;
+}
+EXPORT_SYMBOL(hdmi_vendor_infoframe_pack);
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
new file mode 100644
index 0000000..18f74c0
--- /dev/null
+++ b/include/linux/hdmi.h
@@ -0,0 +1,220 @@
+/*
+ * 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_VENDOR = 0x81,
+ HDMI_INFOFRAME_TYPE_AVI = 0x82,
+ HDMI_INFOFRAME_TYPE_SPD = 0x83,
+ HDMI_INFOFRAME_TYPE_AUDIO = 0x84,
+};
+
+#define HDMI_AVI_INFOFRAME_SIZE 13
+#define HDMI_SPD_INFOFRAME_SIZE 25
+#define HDMI_AUDIO_INFOFRAME_SIZE 10
+
+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;
+};
+
+int hdmi_avi_infoframe_init(struct hdmi_avi_infoframe *frame);
+ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe *frame, void *buffer,
+ size_t size);
+
+enum hdmi_spd_sdi {
+ HDMI_SPD_SDI_UNKNOWN,
+ HDMI_SPD_SDI_DSTB,
+ HDMI_SPD_SDI_DVDP,
+ HDMI_SPD_SDI_DVHS,
+ HDMI_SPD_SDI_HDDVR,
+ HDMI_SPD_SDI_DVC,
+ HDMI_SPD_SDI_DSC,
+ HDMI_SPD_SDI_VCD,
+ HDMI_SPD_SDI_GAME,
+ HDMI_SPD_SDI_PC,
+ HDMI_SPD_SDI_BD,
+ HDMI_SPD_SDI_SCD,
+};
+
+struct hdmi_spd_infoframe {
+ enum hdmi_infoframe_type type;
+ unsigned char version;
+ unsigned char length;
+ char vendor[8];
+ char product[16];
+ enum hdmi_spd_sdi sdi;
+};
+
+int hdmi_spd_infoframe_init(struct hdmi_spd_infoframe *frame,
+ const char *vendor, const char *product);
+ssize_t hdmi_spd_infoframe_pack(struct hdmi_spd_infoframe *frame, void *buffer,
+ size_t size);
+
+enum hdmi_audio_coding_type {
+ HDMI_AUDIO_CODING_TYPE_STREAM,
+ HDMI_AUDIO_CODING_TYPE_IEC_60958,
+ HDMI_AUDIO_CODING_TYPE_AC3,
+ HDMI_AUDIO_CODING_TYPE_MPEG1,
+ HDMI_AUDIO_CODING_TYPE_MP3,
+ HDMI_AUDIO_CODING_TYPE_MPEG2,
+ HDMI_AUDIO_CODING_TYPE_AAC,
+ HDMI_AUDIO_CODING_TYPE_DTS,
+ HDMI_AUDIO_CODING_TYPE_ATRAC,
+ HDMI_AUDIO_CODING_TYPE_ONE_BIT_AUDIO,
+ HDMI_AUDIO_CODING_TYPE_DOLBY_DIGITAL_PLUS,
+ HDMI_AUDIO_CODING_TYPE_DTS_HD,
+ HDMI_AUDIO_CODING_TYPE_MAT_MLP,
+ HDMI_AUDIO_CODING_TYPE_DST,
+ HDMI_AUDIO_CODING_TYPE_WMPRO,
+};
+
+enum hdmi_audio_sample_size {
+ HDMI_AUDIO_SAMPLE_SIZE_STREAM,
+ HDMI_AUDIO_SAMPLE_SIZE_16,
+ HDMI_AUDIO_SAMPLE_SIZE_20,
+ HDMI_AUDIO_SAMPLE_SIZE_24,
+};
+
+enum hdmi_audio_sample_frequency {
+ HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM,
+ HDMI_AUDIO_SAMPLE_FREQUENCY_32000,
+ HDMI_AUDIO_SAMPLE_FREQUENCY_44100,
+ HDMI_AUDIO_SAMPLE_FREQUENCY_48000,
+ HDMI_AUDIO_SAMPLE_FREQUENCY_88200,
+ HDMI_AUDIO_SAMPLE_FREQUENCY_96000,
+ HDMI_AUDIO_SAMPLE_FREQUENCY_176400,
+ HDMI_AUDIO_SAMPLE_FREQUENCY_192000,
+};
+
+struct hdmi_audio_infoframe {
+ enum hdmi_infoframe_type type;
+ unsigned char version;
+ unsigned char length;
+ unsigned char channels;
+ enum hdmi_audio_coding_type coding_type;
+ enum hdmi_audio_sample_size sample_size;
+ enum hdmi_audio_sample_frequency sample_frequency;
+ unsigned char channel_allocation;
+ unsigned char level_shift_value;
+ bool downmix_inhibit;
+
+};
+
+int hdmi_audio_infoframe_init(struct hdmi_audio_infoframe *frame);
+ssize_t hdmi_audio_infoframe_pack(struct hdmi_audio_infoframe *frame,
+ void *buffer, size_t size);
+
+struct hdmi_vendor_infoframe {
+ enum hdmi_infoframe_type type;
+ unsigned char version;
+ unsigned char length;
+ u8 data[27];
+};
+
+ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
+ void *buffer, size_t size);
+
+#endif /* _DRM_HDMI_H */
--
1.8.0.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [RFC v2 2/5] video: Add generic HDMI infoframe helpers
2012-12-05 16:45 ` [RFC v2 2/5] video: Add generic HDMI infoframe helpers Thierry Reding
@ 2012-12-07 14:32 ` Paulo Zanoni
2012-12-11 7:58 ` Thierry Reding
0 siblings, 1 reply; 30+ messages in thread
From: Paulo Zanoni @ 2012-12-07 14:32 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel
2012/12/5 Thierry Reding <thierry.reding@avionic-design.de>:
> Add generic helpers to pack HDMI infoframes into binary buffers.
>
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> ---
> Changes in v2:
> - add support for audio, vendor-specific and SPD infoframes
> - add various validity checks on infoframes
> - factor out checksum computation
>
> drivers/video/Kconfig | 3 +
> drivers/video/Makefile | 1 +
> drivers/video/hdmi.c | 322 +++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/hdmi.h | 220 +++++++++++++++++++++++++++++++++
> 4 files changed, 546 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 807c7fa..baa3f90 100644
> --- a/drivers/video/Kconfig
> +++ b/drivers/video/Kconfig
> @@ -54,6 +54,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 f592f3b..0b50082 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..6b95270
> --- /dev/null
> +++ b/drivers/video/hdmi.c
> @@ -0,0 +1,322 @@
> +/*
> + * 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>
> +#include <linux/string.h>
> +
> +static void hdmi_infoframe_checksum(void *buffer, size_t size)
> +{
> + u8 *ptr = buffer;
> + u8 csum = 0;
> + size_t i;
> +
> + /* compute checksum */
> + for (i = 0; i < size; i++)
> + csum += ptr[i];
> +
> + ptr[3] = 256 - csum;
> +}
> +
> +/**
> + * hdmi_avi_infoframe_init() - initialize an HDMI AVI infoframe
> + * @frame: HDMI AVI infoframe
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int hdmi_avi_infoframe_init(struct hdmi_avi_infoframe *frame)
> +{
> + if (!frame)
> + return -EINVAL;
> +
> + memset(frame, 0, sizeof(*frame));
> +
> + frame->type = HDMI_INFOFRAME_TYPE_AVI;
> + frame->version = 2;
> + frame->length = 13;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(hdmi_avi_infoframe_init);
> +
> +/**
> + * 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.
> + *
> + * Returns the number of bytes packed into the binary buffer or a negative
> + * error code on failure.
> + */
> +ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe *frame, void *buffer,
> + size_t size)
> +{
> + u8 *ptr = buffer;
> + size_t length;
> +
> + if (!frame || !buffer)
> + return -EINVAL;
> +
> + length = 4 + frame->length;
> +
> + if (size < length)
> + return -ENOSPC;
> +
> + memset(buffer, 0, length);
> +
> + 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;
> +
> + hdmi_infoframe_checksum(buffer, length);
> +
> + return length;
> +}
> +EXPORT_SYMBOL(hdmi_avi_infoframe_pack);
> +
> +/**
> + * hdmi_spd_infoframe_init() - initialize an HDMI SPD infoframe
> + * @frame: HDMI SPD infoframe
> + * @vendor: vendor string
> + * @product: product string
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int hdmi_spd_infoframe_init(struct hdmi_spd_infoframe *frame,
> + const char *vendor, const char *product)
> +{
> + if (!frame)
> + return -EINVAL;
> +
> + memset(frame, 0, sizeof(*frame));
> +
> + frame->type = HDMI_INFOFRAME_TYPE_SPD;
> + frame->version = 1;
> + frame->length = 25;
> +
> + strncpy(frame->vendor, vendor, sizeof(frame->vendor));
> + strncpy(frame->product, product, sizeof(frame->product));
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(hdmi_spd_infoframe_init);
> +
> +/**
> + * hdmi_spd_infoframe_pack() - write HDMI SPD infoframe to binary buffer
> + * @frame: HDMI SPD 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.
> + *
> + * Returns the number of bytes packed into the binary buffer or a negative
> + * error code on failure.
> + */
> +ssize_t hdmi_spd_infoframe_pack(struct hdmi_spd_infoframe *frame, void *buffer,
> + size_t size)
> +{
> + u8 *ptr = buffer;
> + size_t length, i;
> +
> + if (!frame || !buffer)
> + return -EINVAL;
> +
> + length = 4 + frame->length;
> +
> + if (size < length)
> + return -ENOSPC;
> +
> + memset(buffer, 0, length);
> +
> + ptr[0] = frame->type;
> + ptr[1] = frame->version;
> + ptr[2] = frame->length;
> + ptr[3] = 0; /* checksum */
> +
> + for (i = 0; i < sizeof(frame->vendor); i++)
> + ptr[4 + i] = frame->vendor[i];
> +
> + for (i = 0; i < sizeof(frame->product); i++)
> + ptr[12 + i] = frame->product[i];
> +
> + ptr[26] = frame->sdi;
Shouldn't this be ptr[28] ?
> +
> + hdmi_infoframe_checksum(buffer, length);
> +
> + return length;
> +}
> +EXPORT_SYMBOL(hdmi_spd_infoframe_pack);
> +
> +/**
> + * hdmi_audio_infoframe_init() - initialize an HDMI audio infoframe
> + * @frame: HDMI audio infoframe
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int hdmi_audio_infoframe_init(struct hdmi_audio_infoframe *frame)
> +{
> + if (!frame)
> + return -EINVAL;
> +
> + memset(frame, 0, sizeof(*frame));
> +
> + frame->type = HDMI_INFOFRAME_TYPE_AUDIO;
> + frame->version = 1;
> + frame->length = 10;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(hdmi_audio_infoframe_init);
> +
> +/**
> + * hdmi_audio_infoframe_pack() - write HDMI audio infoframe to binary buffer
> + * @frame: HDMI audio 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.
> + *
> + * Returns the number of bytes packed into the binary buffer or a negative
> + * error code on failure.
> + */
> +ssize_t hdmi_audio_infoframe_pack(struct hdmi_audio_infoframe *frame,
> + void *buffer, size_t size)
> +{
> + unsigned char channels;
> + u8 *ptr = buffer;
> + size_t length;
> +
> + if (!frame || !buffer)
> + return -EINVAL;
> +
> + length = 4 + frame->length;
> +
> + if (size < length)
> + return -ENOSPC;
> +
> + memset(buffer, 0, length);
> +
> + if (frame->channels >= 2)
> + channels = frame->channels - 1;
> + else
> + channels = 0;
> +
> + ptr[0] = frame->type;
> + ptr[1] = frame->version;
> + ptr[2] = frame->length;
> + ptr[3] = 0; /* checksum */
> +
> + ptr[4] = ((frame->coding_type & 0xf) << 4) | (channels & 0x7);
> + ptr[5] = ((frame->sample_frequency & 0x7) << 2) |
> + (frame->sample_size & 0x3);
> + ptr[6] = 0;
> + ptr[7] = frame->channel_allocation;
> + ptr[8] = (frame->level_shift_value & 0xf) << 3;
> +
> + if (frame->downmix_inhibit)
> + ptr[8] |= BIT(7);
> +
> + hdmi_infoframe_checksum(buffer, length);
> +
> + return length;
> +}
> +EXPORT_SYMBOL(hdmi_audio_infoframe_pack);
> +
> +/**
> + * hdmi_vendor_infoframe_pack() - write a HDMI vendor infoframe to binary
> + * buffer
> + * @frame: HDMI vendor 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.
> + *
> + * Returns the number of bytes packed into the binary buffer or a negative
> + * error code on failure.
> + */
> +ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
> + void *buffer, size_t size)
> +{
> + u8 *ptr = buffer;
> + size_t length;
> +
> + if (!frame || !buffer)
> + return -EINVAL;
> +
> + length = 4 + frame->length;
> +
> + if (size < length)
> + return -ENOSPC;
> +
> + memset(buffer, 0, length);
> +
> + ptr[0] = frame->type;
> + ptr[1] = frame->version;
> + ptr[2] = frame->length;
> + ptr[3] = 0; /* checksum */
> +
> + memcpy(&ptr[4], frame->data, frame->length);
> +
> + hdmi_infoframe_checksum(buffer, length);
> +
> + return length;
> +}
> +EXPORT_SYMBOL(hdmi_vendor_infoframe_pack);
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> new file mode 100644
> index 0000000..18f74c0
> --- /dev/null
> +++ b/include/linux/hdmi.h
> @@ -0,0 +1,220 @@
> +/*
> + * 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_VENDOR = 0x81,
> + HDMI_INFOFRAME_TYPE_AVI = 0x82,
> + HDMI_INFOFRAME_TYPE_SPD = 0x83,
> + HDMI_INFOFRAME_TYPE_AUDIO = 0x84,
> +};
> +
> +#define HDMI_AVI_INFOFRAME_SIZE 13
> +#define HDMI_SPD_INFOFRAME_SIZE 25
> +#define HDMI_AUDIO_INFOFRAME_SIZE 10
> +
> +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;
> +};
> +
> +int hdmi_avi_infoframe_init(struct hdmi_avi_infoframe *frame);
> +ssize_t hdmi_avi_infoframe_pack(struct hdmi_avi_infoframe *frame, void *buffer,
> + size_t size);
> +
> +enum hdmi_spd_sdi {
> + HDMI_SPD_SDI_UNKNOWN,
> + HDMI_SPD_SDI_DSTB,
> + HDMI_SPD_SDI_DVDP,
> + HDMI_SPD_SDI_DVHS,
> + HDMI_SPD_SDI_HDDVR,
> + HDMI_SPD_SDI_DVC,
> + HDMI_SPD_SDI_DSC,
> + HDMI_SPD_SDI_VCD,
> + HDMI_SPD_SDI_GAME,
> + HDMI_SPD_SDI_PC,
> + HDMI_SPD_SDI_BD,
> + HDMI_SPD_SDI_SCD,
> +};
> +
> +struct hdmi_spd_infoframe {
> + enum hdmi_infoframe_type type;
> + unsigned char version;
> + unsigned char length;
> + char vendor[8];
> + char product[16];
> + enum hdmi_spd_sdi sdi;
> +};
> +
> +int hdmi_spd_infoframe_init(struct hdmi_spd_infoframe *frame,
> + const char *vendor, const char *product);
> +ssize_t hdmi_spd_infoframe_pack(struct hdmi_spd_infoframe *frame, void *buffer,
> + size_t size);
> +
> +enum hdmi_audio_coding_type {
> + HDMI_AUDIO_CODING_TYPE_STREAM,
> + HDMI_AUDIO_CODING_TYPE_IEC_60958,
> + HDMI_AUDIO_CODING_TYPE_AC3,
> + HDMI_AUDIO_CODING_TYPE_MPEG1,
> + HDMI_AUDIO_CODING_TYPE_MP3,
> + HDMI_AUDIO_CODING_TYPE_MPEG2,
> + HDMI_AUDIO_CODING_TYPE_AAC,
> + HDMI_AUDIO_CODING_TYPE_DTS,
> + HDMI_AUDIO_CODING_TYPE_ATRAC,
> + HDMI_AUDIO_CODING_TYPE_ONE_BIT_AUDIO,
> + HDMI_AUDIO_CODING_TYPE_DOLBY_DIGITAL_PLUS,
> + HDMI_AUDIO_CODING_TYPE_DTS_HD,
> + HDMI_AUDIO_CODING_TYPE_MAT_MLP,
> + HDMI_AUDIO_CODING_TYPE_DST,
> + HDMI_AUDIO_CODING_TYPE_WMPRO,
> +};
> +
> +enum hdmi_audio_sample_size {
> + HDMI_AUDIO_SAMPLE_SIZE_STREAM,
> + HDMI_AUDIO_SAMPLE_SIZE_16,
> + HDMI_AUDIO_SAMPLE_SIZE_20,
> + HDMI_AUDIO_SAMPLE_SIZE_24,
> +};
> +
> +enum hdmi_audio_sample_frequency {
> + HDMI_AUDIO_SAMPLE_FREQUENCY_STREAM,
> + HDMI_AUDIO_SAMPLE_FREQUENCY_32000,
> + HDMI_AUDIO_SAMPLE_FREQUENCY_44100,
> + HDMI_AUDIO_SAMPLE_FREQUENCY_48000,
> + HDMI_AUDIO_SAMPLE_FREQUENCY_88200,
> + HDMI_AUDIO_SAMPLE_FREQUENCY_96000,
> + HDMI_AUDIO_SAMPLE_FREQUENCY_176400,
> + HDMI_AUDIO_SAMPLE_FREQUENCY_192000,
> +};
> +
> +struct hdmi_audio_infoframe {
> + enum hdmi_infoframe_type type;
> + unsigned char version;
> + unsigned char length;
> + unsigned char channels;
> + enum hdmi_audio_coding_type coding_type;
> + enum hdmi_audio_sample_size sample_size;
> + enum hdmi_audio_sample_frequency sample_frequency;
> + unsigned char channel_allocation;
> + unsigned char level_shift_value;
> + bool downmix_inhibit;
> +
> +};
> +
> +int hdmi_audio_infoframe_init(struct hdmi_audio_infoframe *frame);
> +ssize_t hdmi_audio_infoframe_pack(struct hdmi_audio_infoframe *frame,
> + void *buffer, size_t size);
> +
> +struct hdmi_vendor_infoframe {
> + enum hdmi_infoframe_type type;
> + unsigned char version;
> + unsigned char length;
> + u8 data[27];
> +};
> +
> +ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
> + void *buffer, size_t size);
> +
> +#endif /* _DRM_HDMI_H */
> --
> 1.8.0.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC v2 2/5] video: Add generic HDMI infoframe helpers
2012-12-07 14:32 ` Paulo Zanoni
@ 2012-12-11 7:58 ` Thierry Reding
0 siblings, 0 replies; 30+ messages in thread
From: Thierry Reding @ 2012-12-11 7:58 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 1289 bytes --]
On Fri, Dec 07, 2012 at 12:32:42PM -0200, Paulo Zanoni wrote:
> 2012/12/5 Thierry Reding <thierry.reding@avionic-design.de>:
[...]
> > +ssize_t hdmi_spd_infoframe_pack(struct hdmi_spd_infoframe *frame, void *buffer,
> > + size_t size)
> > +{
> > + u8 *ptr = buffer;
> > + size_t length, i;
> > +
> > + if (!frame || !buffer)
> > + return -EINVAL;
> > +
> > + length = 4 + frame->length;
> > +
> > + if (size < length)
> > + return -ENOSPC;
> > +
> > + memset(buffer, 0, length);
> > +
> > + ptr[0] = frame->type;
> > + ptr[1] = frame->version;
> > + ptr[2] = frame->length;
> > + ptr[3] = 0; /* checksum */
> > +
> > + for (i = 0; i < sizeof(frame->vendor); i++)
> > + ptr[4 + i] = frame->vendor[i];
> > +
> > + for (i = 0; i < sizeof(frame->product); i++)
> > + ptr[12 + i] = frame->product[i];
> > +
> > + ptr[26] = frame->sdi;
>
> Shouldn't this be ptr[28] ?
Yes, you're absolutely right.
By the way, I had to scroll through your reply several times before I
found this single line. If you remove unneeded context when replying
your comments are much easier to locate.
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [RFC v2 3/5] drm: Add HDMI infoframe helpers
2012-12-05 16:45 [RFC v2 0/5] Add HDMI infoframe helpers Thierry Reding
2012-12-05 16:45 ` [RFC v2 1/5] drm: Add some missing forward declarations Thierry Reding
2012-12-05 16:45 ` [RFC v2 2/5] video: Add generic HDMI infoframe helpers Thierry Reding
@ 2012-12-05 16:45 ` Thierry Reding
2012-12-05 17:51 ` Lars-Peter Clausen
` (2 more replies)
2012-12-05 16:45 ` [RFC v2 4/5] drm: tegra: Use generic " Thierry Reding
2012-12-05 16:45 ` [RFC v2 5/5] drm/i915: " Thierry Reding
4 siblings, 3 replies; 30+ messages in thread
From: Thierry Reding @ 2012-12-05 16:45 UTC (permalink / raw)
To: dri-devel
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>
---
Changes in v2:
- reuse CEA modes defined in drm_edid_modes.h
drivers/gpu/drm/Kconfig | 7 +++++
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_hdmi.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++
include/drm/drm_hdmi.h | 19 +++++++++++++
4 files changed, 98 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 6f58c81..4a0b781 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-$(CONFIG_PCI) += ati_pcigart.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..821ca56
--- /dev/null
+++ b/drivers/gpu/drm/drm_hdmi.c
@@ -0,0 +1,71 @@
+/*
+ * 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>
+
+#include "drm_edid_modes.h"
+
+static inline unsigned int
+drm_mode_cea_vic(const struct drm_display_mode *mode)
+{
+ unsigned int i;
+
+ for (i = 0; i < drm_num_cea_modes; i++)
+ if (drm_mode_equal(mode, &edid_cea_modes[i]))
+ return i + 1;
+
+ return 0;
+}
+
+static inline enum hdmi_picture_aspect
+drm_display_mode_get_aspect(const struct drm_display_mode *mode)
+{
+ enum hdmi_picture_aspect aspect = HDMI_PICTURE_ASPECT_NONE;
+
+ if ((mode->hdisplay * 9) / 16 == mode->vdisplay)
+ aspect = HDMI_PICTURE_ASPECT_16_9;
+ else if ((mode->hdisplay * 3) / 4 == mode->vdisplay)
+ aspect = HDMI_PICTURE_ASPECT_4_3;
+
+ return aspect;
+}
+
+/**
+ * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe with
+ * data from a DRM display mode
+ * @frame: HDMI AVI infoframe
+ * @mode: DRM display mode
+ *
+ * Returns 0 on success or a negative error code on failure.
+ */
+int
+drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
+ const struct drm_display_mode *mode)
+{
+ int err;
+
+ if (!frame || !mode)
+ return -EINVAL;
+
+ err = hdmi_avi_infoframe_init(frame);
+ if (err < 0)
+ return err;
+
+ frame->video_code = drm_mode_cea_vic(mode);
+ if (!frame->video_code)
+ return 0;
+
+ frame->picture_aspect = drm_display_mode_get_aspect(mode);
+ frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
+
+ return 0;
+}
+EXPORT_SYMBOL(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..e20462d
--- /dev/null
+++ b/include/drm/drm_hdmi.h
@@ -0,0 +1,19 @@
+/*
+ * 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,
+ const struct drm_display_mode *mode);
+
+#endif
--
1.8.0.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [RFC v2 3/5] drm: Add HDMI infoframe helpers
2012-12-05 16:45 ` [RFC v2 3/5] drm: Add " Thierry Reding
@ 2012-12-05 17:51 ` Lars-Peter Clausen
2012-12-06 7:28 ` Thierry Reding
2012-12-06 14:09 ` Daniel Vetter
2012-12-07 19:00 ` Daniel Vetter
2 siblings, 1 reply; 30+ messages in thread
From: Lars-Peter Clausen @ 2012-12-05 17:51 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel
On 12/05/2012 05:45 PM, Thierry Reding wrote:
> Add a generic helper to fill in an HDMI AVI infoframe with data
> extracted from a DRM display mode.
That's a very nice patch series, comes in pretty handy. Thanks :)
I've just one comment.
> [...]
> +static inline enum hdmi_picture_aspect
> +drm_display_mode_get_aspect(const struct drm_display_mode *mode)
> +{
> + enum hdmi_picture_aspect aspect = HDMI_PICTURE_ASPECT_NONE;
> +
> + if ((mode->hdisplay * 9) / 16 == mode->vdisplay)
> + aspect = HDMI_PICTURE_ASPECT_16_9;
> + else if ((mode->hdisplay * 3) / 4 == mode->vdisplay)
> + aspect = HDMI_PICTURE_ASPECT_4_3;
> +
> + return aspect;
> +}
> +
> +/**
> + * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe with
> + * data from a DRM display mode
> + * @frame: HDMI AVI infoframe
> + * @mode: DRM display mode
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int
> +drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
> + const struct drm_display_mode *mode)
> +{
[...]
> +
> + frame->picture_aspect = drm_display_mode_get_aspect(mode);
As far as I know the picture aspect ratio is supposed to be the aspect ratio
of the displayed content not the pixel aspect ratio. The receiver already
knows the pixel aspect ratio since it knows both the pixel width and the
height. The picture aspect ratio is something that could be set by a
userspace application running in fullscreen, e.g. a video player.
> + frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
> +
> + return 0;
> +}
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC v2 3/5] drm: Add HDMI infoframe helpers
2012-12-05 17:51 ` Lars-Peter Clausen
@ 2012-12-06 7:28 ` Thierry Reding
2012-12-06 8:28 ` Lars-Peter Clausen
0 siblings, 1 reply; 30+ messages in thread
From: Thierry Reding @ 2012-12-06 7:28 UTC (permalink / raw)
To: Lars-Peter Clausen; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 2451 bytes --]
On Wed, Dec 05, 2012 at 06:51:20PM +0100, Lars-Peter Clausen wrote:
> On 12/05/2012 05:45 PM, Thierry Reding wrote:
> > Add a generic helper to fill in an HDMI AVI infoframe with data
> > extracted from a DRM display mode.
>
> That's a very nice patch series, comes in pretty handy. Thanks :)
>
> I've just one comment.
>
> > [...]
> > +static inline enum hdmi_picture_aspect
> > +drm_display_mode_get_aspect(const struct drm_display_mode *mode)
> > +{
> > + enum hdmi_picture_aspect aspect = HDMI_PICTURE_ASPECT_NONE;
> > +
> > + if ((mode->hdisplay * 9) / 16 == mode->vdisplay)
> > + aspect = HDMI_PICTURE_ASPECT_16_9;
> > + else if ((mode->hdisplay * 3) / 4 == mode->vdisplay)
> > + aspect = HDMI_PICTURE_ASPECT_4_3;
> > +
> > + return aspect;
> > +}
> > +
> > +/**
> > + * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe with
> > + * data from a DRM display mode
> > + * @frame: HDMI AVI infoframe
> > + * @mode: DRM display mode
> > + *
> > + * Returns 0 on success or a negative error code on failure.
> > + */
> > +int
> > +drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
> > + const struct drm_display_mode *mode)
> > +{
> [...]
> > +
> > + frame->picture_aspect = drm_display_mode_get_aspect(mode);
>
> As far as I know the picture aspect ratio is supposed to be the aspect ratio
> of the displayed content not the pixel aspect ratio. The receiver already
> knows the pixel aspect ratio since it knows both the pixel width and the
> height. The picture aspect ratio is something that could be set by a
> userspace application running in fullscreen, e.g. a video player.
Isn't that what the active aspect ratio is supposed to do? Unfortunately
neither the HDMI specification nor CEA-861-D seem to be very specific
about this. What I did in the above was basically just refactoring from
what the Tegra driver does. None of the other drivers are filling in any
useful values here. Most equipment seems to work just fine if you pass a
zeroed out AVI infoframe. =)
Given that we don't have a way to pass any kind of information to the
kernel about this, the only options we have are to either use this or go
with HDMI_PICTURE_ASPECT_NONE. I suppose that the latter will work fine
too.
Does anybody else have more information about what the correct value for
this is?
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC v2 3/5] drm: Add HDMI infoframe helpers
2012-12-06 7:28 ` Thierry Reding
@ 2012-12-06 8:28 ` Lars-Peter Clausen
0 siblings, 0 replies; 30+ messages in thread
From: Lars-Peter Clausen @ 2012-12-06 8:28 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel
On 12/06/2012 08:28 AM, Thierry Reding wrote:
> On Wed, Dec 05, 2012 at 06:51:20PM +0100, Lars-Peter Clausen wrote:
>> On 12/05/2012 05:45 PM, Thierry Reding wrote:
>>> Add a generic helper to fill in an HDMI AVI infoframe with data
>>> extracted from a DRM display mode.
>>
>> That's a very nice patch series, comes in pretty handy. Thanks :)
>>
>> I've just one comment.
>>
>>> [...]
>>> +static inline enum hdmi_picture_aspect
>>> +drm_display_mode_get_aspect(const struct drm_display_mode *mode)
>>> +{
>>> + enum hdmi_picture_aspect aspect = HDMI_PICTURE_ASPECT_NONE;
>>> +
>>> + if ((mode->hdisplay * 9) / 16 == mode->vdisplay)
>>> + aspect = HDMI_PICTURE_ASPECT_16_9;
>>> + else if ((mode->hdisplay * 3) / 4 == mode->vdisplay)
>>> + aspect = HDMI_PICTURE_ASPECT_4_3;
>>> +
>>> + return aspect;
>>> +}
>>> +
>>> +/**
>>> + * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe with
>>> + * data from a DRM display mode
>>> + * @frame: HDMI AVI infoframe
>>> + * @mode: DRM display mode
>>> + *
>>> + * Returns 0 on success or a negative error code on failure.
>>> + */
>>> +int
>>> +drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
>>> + const struct drm_display_mode *mode)
>>> +{
>> [...]
>>> +
>>> + frame->picture_aspect = drm_display_mode_get_aspect(mode);
>>
>> As far as I know the picture aspect ratio is supposed to be the aspect ratio
>> of the displayed content not the pixel aspect ratio. The receiver already
>> knows the pixel aspect ratio since it knows both the pixel width and the
>> height. The picture aspect ratio is something that could be set by a
>> userspace application running in fullscreen, e.g. a video player.
>
> Isn't that what the active aspect ratio is supposed to do? Unfortunately
> neither the HDMI specification nor CEA-861-D seem to be very specific
> about this. What I did in the above was basically just refactoring from
> what the Tegra driver does. None of the other drivers are filling in any
> useful values here. Most equipment seems to work just fine if you pass a
> zeroed out AVI infoframe. =)
>
The active aspect ratio is for when you have black borders on the sides of your
images. The picture aspect field is for when you for example have non square
pixels, which probably does not happen so often with PC like device.
Hm, btw. I think some HDMI VICs are defined to have non-square pixels like for
example 576p I wonder if we should skip these when we select the VIC. Otherwise
the video data may look stretched when displayed on the HDMI sink.
> Given that we don't have a way to pass any kind of information to the
> kernel about this, the only options we have are to either use this or go
> with HDMI_PICTURE_ASPECT_NONE. I suppose that the latter will work fine
> too.
>
Agreed. HDMI_PICTURE_ASPECT_NONE should hopefully work.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC v2 3/5] drm: Add HDMI infoframe helpers
2012-12-05 16:45 ` [RFC v2 3/5] drm: Add " Thierry Reding
2012-12-05 17:51 ` Lars-Peter Clausen
@ 2012-12-06 14:09 ` Daniel Vetter
2012-12-06 14:28 ` Thierry Reding
2012-12-07 19:00 ` Daniel Vetter
2 siblings, 1 reply; 30+ messages in thread
From: Daniel Vetter @ 2012-12-06 14:09 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel
On Wed, Dec 05, 2012 at 05:45:42PM +0100, Thierry Reding wrote:
> 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>
A few quick comments below.
> ---
> Changes in v2:
> - reuse CEA modes defined in drm_edid_modes.h
>
> drivers/gpu/drm/Kconfig | 7 +++++
> drivers/gpu/drm/Makefile | 1 +
> drivers/gpu/drm/drm_hdmi.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++
> include/drm/drm_hdmi.h | 19 +++++++++++++
> 4 files changed, 98 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 6f58c81..4a0b781 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-$(CONFIG_PCI) += ati_pcigart.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..821ca56
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_hdmi.c
> @@ -0,0 +1,71 @@
> +/*
> + * 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>
> +
> +#include "drm_edid_modes.h"
This creates a 2nd copy of that massive table. Imo we could just shovel
the infoframe helpers into the drm_edid.c file - allowing different
helpers to be disabled isn't that useful, since most drivers will want
them pretty much all anyway. Or at least most of them.
> +
> +static inline unsigned int
> +drm_mode_cea_vic(const struct drm_display_mode *mode)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < drm_num_cea_modes; i++)
> + if (drm_mode_equal(mode, &edid_cea_modes[i]))
> + return i + 1;
> +
> + return 0;
> +}
Same function in drm_edid will land through drm-intel tree in drm-next
rsn. I'll send that pull request somewhen next week probably.
http://cgit.freedesktop.org/~danvet/drm-intel/commit/?id=374a868a726eb8a1cb28ba88805e51ce34222f8d
> +
> +static inline enum hdmi_picture_aspect
> +drm_display_mode_get_aspect(const struct drm_display_mode *mode)
> +{
> + enum hdmi_picture_aspect aspect = HDMI_PICTURE_ASPECT_NONE;
> +
> + if ((mode->hdisplay * 9) / 16 == mode->vdisplay)
> + aspect = HDMI_PICTURE_ASPECT_16_9;
> + else if ((mode->hdisplay * 3) / 4 == mode->vdisplay)
> + aspect = HDMI_PICTURE_ASPECT_4_3;
> +
> + return aspect;
> +}
> +
> +/**
> + * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe with
> + * data from a DRM display mode
> + * @frame: HDMI AVI infoframe
> + * @mode: DRM display mode
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
> +int
> +drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
> + const struct drm_display_mode *mode)
> +{
> + int err;
> +
> + if (!frame || !mode)
> + return -EINVAL;
> +
> + err = hdmi_avi_infoframe_init(frame);
> + if (err < 0)
> + return err;
> +
> + frame->video_code = drm_mode_cea_vic(mode);
> + if (!frame->video_code)
> + return 0;
> +
> + frame->picture_aspect = drm_display_mode_get_aspect(mode);
> + frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
Note that the intel avi infoframe in intel_hdmi_set_avi_infoframe also
sets the pixel repeat for double clocked modes with:
if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
Cheers, Daniel
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(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..e20462d
> --- /dev/null
> +++ b/include/drm/drm_hdmi.h
> @@ -0,0 +1,19 @@
> +/*
> + * 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,
> + const struct drm_display_mode *mode);
> +
> +#endif
> --
> 1.8.0.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC v2 3/5] drm: Add HDMI infoframe helpers
2012-12-06 14:09 ` Daniel Vetter
@ 2012-12-06 14:28 ` Thierry Reding
2012-12-06 15:44 ` Daniel Vetter
0 siblings, 1 reply; 30+ messages in thread
From: Thierry Reding @ 2012-12-06 14:28 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 3207 bytes --]
On Thu, Dec 06, 2012 at 03:09:00PM +0100, Daniel Vetter wrote:
> On Wed, Dec 05, 2012 at 05:45:42PM +0100, Thierry Reding wrote:
[...]
> > diff --git a/drivers/gpu/drm/drm_hdmi.c b/drivers/gpu/drm/drm_hdmi.c
> > new file mode 100644
> > index 0000000..821ca56
> > --- /dev/null
> > +++ b/drivers/gpu/drm/drm_hdmi.c
> > @@ -0,0 +1,71 @@
> > +/*
> > + * 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>
> > +
> > +#include "drm_edid_modes.h"
>
> This creates a 2nd copy of that massive table. Imo we could just shovel
> the infoframe helpers into the drm_edid.c file - allowing different
> helpers to be disabled isn't that useful, since most drivers will want
> them pretty much all anyway. Or at least most of them.
I can move this helper to drm_edid.c. But maybe it might be worth moving
the table from the header into a source file and just export it from
there.
> > +
> > +static inline unsigned int
> > +drm_mode_cea_vic(const struct drm_display_mode *mode)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < drm_num_cea_modes; i++)
> > + if (drm_mode_equal(mode, &edid_cea_modes[i]))
> > + return i + 1;
> > +
> > + return 0;
> > +}
>
> Same function in drm_edid will land through drm-intel tree in drm-next
> rsn. I'll send that pull request somewhen next week probably.
>
> http://cgit.freedesktop.org/~danvet/drm-intel/commit/?id=374a868a726eb8a1cb28ba88805e51ce34222f8d
Yes, I'm aware of that patch (I reviewed it =) and I was planning on
dropping the implementation in this patch once the above patch makes
it into linux-next.
> > +/**
> > + * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe with
> > + * data from a DRM display mode
> > + * @frame: HDMI AVI infoframe
> > + * @mode: DRM display mode
> > + *
> > + * Returns 0 on success or a negative error code on failure.
> > + */
> > +int
> > +drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
> > + const struct drm_display_mode *mode)
> > +{
> > + int err;
> > +
> > + if (!frame || !mode)
> > + return -EINVAL;
> > +
> > + err = hdmi_avi_infoframe_init(frame);
> > + if (err < 0)
> > + return err;
> > +
> > + frame->video_code = drm_mode_cea_vic(mode);
> > + if (!frame->video_code)
> > + return 0;
> > +
> > + frame->picture_aspect = drm_display_mode_get_aspect(mode);
> > + frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
>
> Note that the intel avi infoframe in intel_hdmi_set_avi_infoframe also
> sets the pixel repeat for double clocked modes with:
>
> if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
I left that in place in the Intel driver, but I suppose we can just as
well move it to the infoframe helpers since it should be a generic
option.
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC v2 3/5] drm: Add HDMI infoframe helpers
2012-12-06 14:28 ` Thierry Reding
@ 2012-12-06 15:44 ` Daniel Vetter
2012-12-06 16:02 ` Thierry Reding
0 siblings, 1 reply; 30+ messages in thread
From: Daniel Vetter @ 2012-12-06 15:44 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel
On Thu, Dec 6, 2012 at 3:28 PM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
>> Note that the intel avi infoframe in intel_hdmi_set_avi_infoframe also
>> sets the pixel repeat for double clocked modes with:
>>
>> if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
>> avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
>
> I left that in place in the Intel driver, but I suppose we can just as
> well move it to the infoframe helpers since it should be a generic
> option.
Yeah, noticed that while reading through the drm/i915 patch ;-) If you
move I think that would be good in a 2nd patch, just in case it blows
up somewhere it's easier for other drivers it'd be easier to
root-cause.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC v2 3/5] drm: Add HDMI infoframe helpers
2012-12-06 15:44 ` Daniel Vetter
@ 2012-12-06 16:02 ` Thierry Reding
0 siblings, 0 replies; 30+ messages in thread
From: Thierry Reding @ 2012-12-06 16:02 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 849 bytes --]
On Thu, Dec 06, 2012 at 04:44:38PM +0100, Daniel Vetter wrote:
> On Thu, Dec 6, 2012 at 3:28 PM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> >> Note that the intel avi infoframe in intel_hdmi_set_avi_infoframe also
> >> sets the pixel repeat for double clocked modes with:
> >>
> >> if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> >> avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
> >
> > I left that in place in the Intel driver, but I suppose we can just as
> > well move it to the infoframe helpers since it should be a generic
> > option.
>
> Yeah, noticed that while reading through the drm/i915 patch ;-) If you
> move I think that would be good in a 2nd patch, just in case it blows
> up somewhere it's easier for other drivers it'd be easier to
> root-cause.
Okay, will do.
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC v2 3/5] drm: Add HDMI infoframe helpers
2012-12-05 16:45 ` [RFC v2 3/5] drm: Add " Thierry Reding
2012-12-05 17:51 ` Lars-Peter Clausen
2012-12-06 14:09 ` Daniel Vetter
@ 2012-12-07 19:00 ` Daniel Vetter
2 siblings, 0 replies; 30+ messages in thread
From: Daniel Vetter @ 2012-12-07 19:00 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel
On Wed, Dec 05, 2012 at 05:45:42PM +0100, Thierry Reding wrote:
> 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>
> +/**
> + * drm_hdmi_avi_infoframe_from_display_mode() - fill an HDMI AVI infoframe with
> + * data from a DRM display mode
> + * @frame: HDMI AVI infoframe
> + * @mode: DRM display mode
> + *
> + * Returns 0 on success or a negative error code on failure.
> + */
Another request: Can you please integrate the kerneldoc reference into the
drm documentation? That way their a bit more accessible, and the kerneldoc
generation also checks whether the function declaration still matches up
with the kerneldoc - some drm core stuff has gotten ridiculously out of
sync with reality ...
-Daniel
> +int
> +drm_hdmi_avi_infoframe_from_display_mode(struct hdmi_avi_infoframe *frame,
> + const struct drm_display_mode *mode)
> +{
> + int err;
> +
> + if (!frame || !mode)
> + return -EINVAL;
> +
> + err = hdmi_avi_infoframe_init(frame);
> + if (err < 0)
> + return err;
> +
> + frame->video_code = drm_mode_cea_vic(mode);
> + if (!frame->video_code)
> + return 0;
> +
> + frame->picture_aspect = drm_display_mode_get_aspect(mode);
> + frame->active_aspect = HDMI_ACTIVE_ASPECT_PICTURE;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(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..e20462d
> --- /dev/null
> +++ b/include/drm/drm_hdmi.h
> @@ -0,0 +1,19 @@
> +/*
> + * 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,
> + const struct drm_display_mode *mode);
> +
> +#endif
> --
> 1.8.0.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 30+ messages in thread
* [RFC v2 4/5] drm: tegra: Use generic HDMI infoframe helpers
2012-12-05 16:45 [RFC v2 0/5] Add HDMI infoframe helpers Thierry Reding
` (2 preceding siblings ...)
2012-12-05 16:45 ` [RFC v2 3/5] drm: Add " Thierry Reding
@ 2012-12-05 16:45 ` Thierry Reding
2012-12-05 16:45 ` [RFC v2 5/5] drm/i915: " Thierry Reding
4 siblings, 0 replies; 30+ messages in thread
From: Thierry Reding @ 2012-12-05 16:45 UTC (permalink / raw)
To: dri-devel
Use the generic HDMI infoframe helpers to get rid of the NVIDIA Tegra
reimplementation.
Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
drivers/gpu/drm/tegra/Kconfig | 1 +
drivers/gpu/drm/tegra/hdmi.c | 226 ++++++++++++++++++++----------------------
drivers/gpu/drm/tegra/hdmi.h | 189 -----------------------------------
3 files changed, 110 insertions(+), 306 deletions(-)
diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
index affd741..ae445d6 100644
--- a/drivers/gpu/drm/tegra/Kconfig
+++ b/drivers/gpu/drm/tegra/Kconfig
@@ -4,6 +4,7 @@ config DRM_TEGRA
select DRM_KMS_HELPER
select DRM_GEM_CMA_HELPER
select DRM_KMS_CMA_HELPER
+ select DRM_HDMI
select FB_CFB_FILLRECT
select FB_CFB_COPYAREA
select FB_CFB_IMAGEBLIT
diff --git a/drivers/gpu/drm/tegra/hdmi.c b/drivers/gpu/drm/tegra/hdmi.c
index ab40164..10e5f24 100644
--- a/drivers/gpu/drm/tegra/hdmi.c
+++ b/drivers/gpu/drm/tegra/hdmi.c
@@ -10,6 +10,7 @@
#include <linux/clk.h>
#include <linux/debugfs.h>
#include <linux/gpio.h>
+#include <linux/hdmi.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/platform_device.h>
@@ -17,6 +18,8 @@
#include <mach/clk.h>
+#include <drm/drm_hdmi.h>
+
#include "hdmi.h"
#include "drm.h"
#include "dc.h"
@@ -414,54 +417,65 @@ static int tegra_hdmi_setup_audio(struct tegra_hdmi *hdmi, unsigned int pclk)
return 0;
}
-static void tegra_hdmi_write_infopack(struct tegra_hdmi *hdmi,
- unsigned int offset, u8 type,
- u8 version, void *data, size_t size)
+static inline unsigned long tegra_hdmi_subpack(const u8 *ptr, size_t size)
{
- unsigned long value;
- u8 *ptr = data;
- u32 subpack[2];
+ unsigned long value = 0;
size_t i;
- u8 csum;
- /* first byte of data is the checksum */
- csum = type + version + size - 1;
+ for (i = size; i > 0; i--)
+ value = (value << 8) | ptr[i - 1];
- for (i = 1; i < size; i++)
- csum += ptr[i];
+ return value;
+}
- ptr[0] = 0x100 - csum;
+static void tegra_hdmi_write_infopack(struct tegra_hdmi *hdmi, const void *data,
+ size_t size)
+{
+ const u8 *ptr = data;
+ unsigned long offset;
+ unsigned long value;
+ size_t i, j;
- value = INFOFRAME_HEADER_TYPE(type) |
- INFOFRAME_HEADER_VERSION(version) |
- INFOFRAME_HEADER_LEN(size - 1);
- tegra_hdmi_writel(hdmi, value, offset);
+ switch (ptr[0]) {
+ case HDMI_INFOFRAME_TYPE_AVI:
+ offset = HDMI_NV_PDISP_HDMI_AVI_INFOFRAME_HEADER;
+ break;
- /* The audio inforame only has one set of subpack registers. The hdmi
- * block pads the rest of the data as per the spec so we have to fixup
- * the length before filling in the subpacks.
- */
- if (offset == HDMI_NV_PDISP_HDMI_AUDIO_INFOFRAME_HEADER)
- size = 6;
+ case HDMI_INFOFRAME_TYPE_AUDIO:
+ offset = HDMI_NV_PDISP_HDMI_AUDIO_INFOFRAME_HEADER;
+ break;
- /* each subpack 7 bytes devided into:
- * subpack_low - bytes 0 - 3
- * subpack_high - bytes 4 - 6 (with byte 7 padded to 0x00)
- */
- for (i = 0; i < size; i++) {
- size_t index = i % 7;
+ case HDMI_INFOFRAME_TYPE_VENDOR:
+ offset = HDMI_NV_PDISP_HDMI_GENERIC_HEADER;
+ break;
+
+ default:
+ dev_err(hdmi->dev, "unsupported infoframe type: %02x\n",
+ ptr[0]);
+ return;
+ }
+
+ value = INFOFRAME_HEADER_TYPE(ptr[0]) |
+ INFOFRAME_HEADER_VERSION(ptr[1]) |
+ INFOFRAME_HEADER_LEN(ptr[2]);
+ tegra_hdmi_writel(hdmi, value, offset);
+ offset++;
- if (index == 0)
- memset(subpack, 0x0, sizeof(subpack));
+ /*
+ * Each subpack contains 7 bytes, divided into:
+ * - subpack_low: bytes 0 - 3
+ * - subpack_high: bytes 4 - 6 (with byte 7 padded to 0x00)
+ */
+ for (i = 3, j = 0; i < size; i += 7, j += 8) {
+ size_t rem = size - i, num = min_t(size_t, rem, 4);
- ((u8 *)subpack)[index] = ptr[i];
+ value = tegra_hdmi_subpack(&ptr[i], num);
+ tegra_hdmi_writel(hdmi, value, offset++);
- if (index == 6 || (i + 1 == size)) {
- unsigned int reg = offset + 1 + (i / 7) * 2;
+ num = min_t(size_t, rem - num, 3);
- tegra_hdmi_writel(hdmi, subpack[0], reg);
- tegra_hdmi_writel(hdmi, subpack[1], reg + 1);
- }
+ value = tegra_hdmi_subpack(&ptr[i + 4], num);
+ tegra_hdmi_writel(hdmi, value, offset++);
}
}
@@ -469,9 +483,8 @@ static void tegra_hdmi_setup_avi_infoframe(struct tegra_hdmi *hdmi,
struct drm_display_mode *mode)
{
struct hdmi_avi_infoframe frame;
- unsigned int h_front_porch;
- unsigned int hsize = 16;
- unsigned int vsize = 9;
+ u8 buffer[17];
+ ssize_t err;
if (hdmi->dvi) {
tegra_hdmi_writel(hdmi, 0,
@@ -479,69 +492,19 @@ static void tegra_hdmi_setup_avi_infoframe(struct tegra_hdmi *hdmi,
return;
}
- h_front_porch = mode->htotal - mode->hsync_end;
- memset(&frame, 0, sizeof(frame));
- frame.r = HDMI_AVI_R_SAME;
-
- switch (mode->vdisplay) {
- case 480:
- if (mode->hdisplay == 640) {
- frame.m = HDMI_AVI_M_4_3;
- frame.vic = 1;
- } else {
- frame.m = HDMI_AVI_M_16_9;
- frame.vic = 3;
- }
- break;
-
- case 576:
- if (((hsize * 10) / vsize) > 14) {
- frame.m = HDMI_AVI_M_16_9;
- frame.vic = 18;
- } else {
- frame.m = HDMI_AVI_M_4_3;
- frame.vic = 17;
- }
- break;
-
- case 720:
- case 1470: /* stereo mode */
- frame.m = HDMI_AVI_M_16_9;
-
- if (h_front_porch == 110)
- frame.vic = 4;
- else
- frame.vic = 19;
- break;
-
- case 1080:
- case 2205: /* stereo mode */
- frame.m = HDMI_AVI_M_16_9;
-
- switch (h_front_porch) {
- case 88:
- frame.vic = 16;
- break;
-
- case 528:
- frame.vic = 31;
- break;
-
- default:
- frame.vic = 32;
- break;
- }
- break;
+ err = drm_hdmi_avi_infoframe_from_display_mode(&frame, mode);
+ if (err < 0) {
+ dev_err(hdmi->dev, "failed to setup AVI infoframe: %zd\n", err);
+ return;
+ }
- default:
- frame.m = HDMI_AVI_M_16_9;
- frame.vic = 0;
- break;
+ err = hdmi_avi_infoframe_pack(&frame, buffer, sizeof(buffer));
+ if (err < 0) {
+ dev_err(hdmi->dev, "failed to pack AVI infoframe: %zd\n", err);
+ return;
}
- tegra_hdmi_write_infopack(hdmi, HDMI_NV_PDISP_HDMI_AVI_INFOFRAME_HEADER,
- HDMI_INFOFRAME_TYPE_AVI, HDMI_AVI_VERSION,
- &frame, sizeof(frame));
+ tegra_hdmi_write_infopack(hdmi, buffer, err);
tegra_hdmi_writel(hdmi, INFOFRAME_CTRL_ENABLE,
HDMI_NV_PDISP_HDMI_AVI_INFOFRAME_CTRL);
@@ -550,6 +513,8 @@ static void tegra_hdmi_setup_avi_infoframe(struct tegra_hdmi *hdmi,
static void tegra_hdmi_setup_audio_infoframe(struct tegra_hdmi *hdmi)
{
struct hdmi_audio_infoframe frame;
+ u8 buffer[14];
+ ssize_t err;
if (hdmi->dvi) {
tegra_hdmi_writel(hdmi, 0,
@@ -557,14 +522,29 @@ static void tegra_hdmi_setup_audio_infoframe(struct tegra_hdmi *hdmi)
return;
}
- memset(&frame, 0, sizeof(frame));
- frame.cc = HDMI_AUDIO_CC_2;
+ err = hdmi_audio_infoframe_init(&frame);
+ if (err < 0) {
+ dev_err(hdmi->dev, "failed to initialize audio infoframe: %d\n",
+ err);
+ return;
+ }
+
+ frame.channels = 2;
+
+ err = hdmi_audio_infoframe_pack(&frame, buffer, sizeof(buffer));
+ if (err < 0) {
+ dev_err(hdmi->dev, "failed to pack audio infoframe: %zd\n",
+ err);
+ return;
+ }
- tegra_hdmi_write_infopack(hdmi,
- HDMI_NV_PDISP_HDMI_AUDIO_INFOFRAME_HEADER,
- HDMI_INFOFRAME_TYPE_AUDIO,
- HDMI_AUDIO_VERSION,
- &frame, sizeof(frame));
+ /*
+ * The audio infoframe has only one set of subpack registers, so the
+ * infoframe needs to be truncated. One set of subpack registers can
+ * contain 7 bytes. Including the 3 byte header only the first 10
+ * bytes can be programmed.
+ */
+ tegra_hdmi_write_infopack(hdmi, buffer, min(10, err));
tegra_hdmi_writel(hdmi, INFOFRAME_CTRL_ENABLE,
HDMI_NV_PDISP_HDMI_AUDIO_INFOFRAME_CTRL);
@@ -572,8 +552,10 @@ static void tegra_hdmi_setup_audio_infoframe(struct tegra_hdmi *hdmi)
static void tegra_hdmi_setup_stereo_infoframe(struct tegra_hdmi *hdmi)
{
- struct hdmi_stereo_infoframe frame;
+ struct hdmi_vendor_infoframe frame;
unsigned long value;
+ u8 buffer[10];
+ ssize_t err;
if (!hdmi->stereo) {
value = tegra_hdmi_readl(hdmi, HDMI_NV_PDISP_HDMI_GENERIC_CTRL);
@@ -583,22 +565,32 @@ static void tegra_hdmi_setup_stereo_infoframe(struct tegra_hdmi *hdmi)
}
memset(&frame, 0, sizeof(frame));
- frame.regid0 = 0x03;
- frame.regid1 = 0x0c;
- frame.regid2 = 0x00;
- frame.hdmi_video_format = 2;
+
+ frame.type = HDMI_INFOFRAME_TYPE_VENDOR;
+ frame.version = 0x01;
+ frame.length = 6;
+
+ frame.data[0] = 0x03; /* regid0 */
+ frame.data[1] = 0x0c; /* regid1 */
+ frame.data[2] = 0x00; /* regid2 */
+ frame.data[3] = 0x02 << 5; /* video format */
/* TODO: 74 MHz limit? */
if (1) {
- frame._3d_structure = 0;
+ frame.data[4] = 0x00 << 4; /* 3D structure */
} else {
- frame._3d_structure = 8;
- frame._3d_ext_data = 0;
+ frame.data[4] = 0x08 << 4; /* 3D structure */
+ frame.data[5] = 0x00 << 4; /* 3D ext. data */
+ }
+
+ err = hdmi_vendor_infoframe_pack(&frame, buffer, sizeof(buffer));
+ if (err < 0) {
+ dev_err(hdmi->dev, "failed to pack vendor infoframe: %zd\n",
+ err);
+ return;
}
- tegra_hdmi_write_infopack(hdmi, HDMI_NV_PDISP_HDMI_GENERIC_HEADER,
- HDMI_INFOFRAME_TYPE_VENDOR,
- HDMI_VENDOR_VERSION, &frame, 6);
+ tegra_hdmi_write_infopack(hdmi, buffer, err);
value = tegra_hdmi_readl(hdmi, HDMI_NV_PDISP_HDMI_GENERIC_CTRL);
value |= GENERIC_CTRL_ENABLE;
diff --git a/drivers/gpu/drm/tegra/hdmi.h b/drivers/gpu/drm/tegra/hdmi.h
index 1477f36..52ac36e 100644
--- a/drivers/gpu/drm/tegra/hdmi.h
+++ b/drivers/gpu/drm/tegra/hdmi.h
@@ -10,195 +10,6 @@
#ifndef TEGRA_HDMI_H
#define TEGRA_HDMI_H 1
-#define HDMI_INFOFRAME_TYPE_VENDOR 0x81
-#define HDMI_INFOFRAME_TYPE_AVI 0x82
-#define HDMI_INFOFRAME_TYPE_SPD 0x83
-#define HDMI_INFOFRAME_TYPE_AUDIO 0x84
-#define HDMI_INFOFRAME_TYPE_MPEG_SRC 0x85
-#define HDMI_INFOFRAME_TYPE_NTSC_VBI 0x86
-
-/* all fields little endian */
-struct hdmi_avi_infoframe {
- /* PB0 */
- u8 csum;
-
- /* PB1 */
- unsigned s:2; /* scan information */
- unsigned b:2; /* bar info data valid */
- unsigned a:1; /* active info present */
- unsigned y:2; /* RGB or YCbCr */
- unsigned res1:1;
-
- /* PB2 */
- unsigned r:4; /* active format aspect ratio */
- unsigned m:2; /* picture aspect ratio */
- unsigned c:2; /* colorimetry */
-
- /* PB3 */
- unsigned sc:2; /* scan information */
- unsigned q:2; /* quantization range */
- unsigned ec:3; /* extended colorimetry */
- unsigned itc:1; /* it content */
-
- /* PB4 */
- unsigned vic:7; /* video format id code */
- unsigned res4:1;
-
- /* PB5 */
- unsigned pr:4; /* pixel repetition factor */
- unsigned cn:2; /* it content type*/
- unsigned yq:2; /* ycc quantization range */
-
- /* PB6-7 */
- u16 top_bar_end_line;
-
- /* PB8-9 */
- u16 bot_bar_start_line;
-
- /* PB10-11 */
- u16 left_bar_end_pixel;
-
- /* PB12-13 */
- u16 right_bar_start_pixel;
-} __packed;
-
-#define HDMI_AVI_VERSION 0x02
-
-#define HDMI_AVI_Y_RGB 0x0
-#define HDMI_AVI_Y_YCBCR_422 0x1
-#define HDMI_AVI_Y_YCBCR_444 0x2
-
-#define HDMI_AVI_B_VERT 0x1
-#define HDMI_AVI_B_HORIZ 0x2
-
-#define HDMI_AVI_S_NONE 0x0
-#define HDMI_AVI_S_OVERSCAN 0x1
-#define HDMI_AVI_S_UNDERSCAN 0x2
-
-#define HDMI_AVI_C_NONE 0x0
-#define HDMI_AVI_C_SMPTE 0x1
-#define HDMI_AVI_C_ITU_R 0x2
-#define HDMI_AVI_C_EXTENDED 0x4
-
-#define HDMI_AVI_M_4_3 0x1
-#define HDMI_AVI_M_16_9 0x2
-
-#define HDMI_AVI_R_SAME 0x8
-#define HDMI_AVI_R_4_3_CENTER 0x9
-#define HDMI_AVI_R_16_9_CENTER 0xa
-#define HDMI_AVI_R_14_9_CENTER 0xb
-
-/* all fields little endian */
-struct hdmi_audio_infoframe {
- /* PB0 */
- u8 csum;
-
- /* PB1 */
- unsigned cc:3; /* channel count */
- unsigned res1:1;
- unsigned ct:4; /* coding type */
-
- /* PB2 */
- unsigned ss:2; /* sample size */
- unsigned sf:3; /* sample frequency */
- unsigned res2:3;
-
- /* PB3 */
- unsigned cxt:5; /* coding extention type */
- unsigned res3:3;
-
- /* PB4 */
- u8 ca; /* channel/speaker allocation */
-
- /* PB5 */
- unsigned res5:3;
- unsigned lsv:4; /* level shift value */
- unsigned dm_inh:1; /* downmix inhibit */
-
- /* PB6-10 reserved */
- u8 res6;
- u8 res7;
- u8 res8;
- u8 res9;
- u8 res10;
-} __packed;
-
-#define HDMI_AUDIO_VERSION 0x01
-
-#define HDMI_AUDIO_CC_STREAM 0x0 /* specified by audio stream */
-#define HDMI_AUDIO_CC_2 0x1
-#define HDMI_AUDIO_CC_3 0x2
-#define HDMI_AUDIO_CC_4 0x3
-#define HDMI_AUDIO_CC_5 0x4
-#define HDMI_AUDIO_CC_6 0x5
-#define HDMI_AUDIO_CC_7 0x6
-#define HDMI_AUDIO_CC_8 0x7
-
-#define HDMI_AUDIO_CT_STREAM 0x0 /* specified by audio stream */
-#define HDMI_AUDIO_CT_PCM 0x1
-#define HDMI_AUDIO_CT_AC3 0x2
-#define HDMI_AUDIO_CT_MPEG1 0x3
-#define HDMI_AUDIO_CT_MP3 0x4
-#define HDMI_AUDIO_CT_MPEG2 0x5
-#define HDMI_AUDIO_CT_AAC_LC 0x6
-#define HDMI_AUDIO_CT_DTS 0x7
-#define HDMI_AUDIO_CT_ATRAC 0x8
-#define HDMI_AUDIO_CT_DSD 0x9
-#define HDMI_AUDIO_CT_E_AC3 0xa
-#define HDMI_AUDIO_CT_DTS_HD 0xb
-#define HDMI_AUDIO_CT_MLP 0xc
-#define HDMI_AUDIO_CT_DST 0xd
-#define HDMI_AUDIO_CT_WMA_PRO 0xe
-#define HDMI_AUDIO_CT_CXT 0xf
-
-#define HDMI_AUDIO_SF_STREAM 0x0 /* specified by audio stream */
-#define HDMI_AUIDO_SF_32K 0x1
-#define HDMI_AUDIO_SF_44_1K 0x2
-#define HDMI_AUDIO_SF_48K 0x3
-#define HDMI_AUDIO_SF_88_2K 0x4
-#define HDMI_AUDIO_SF_96K 0x5
-#define HDMI_AUDIO_SF_176_4K 0x6
-#define HDMI_AUDIO_SF_192K 0x7
-
-#define HDMI_AUDIO_SS_STREAM 0x0 /* specified by audio stream */
-#define HDMI_AUDIO_SS_16BIT 0x1
-#define HDMI_AUDIO_SS_20BIT 0x2
-#define HDMI_AUDIO_SS_24BIT 0x3
-
-#define HDMI_AUDIO_CXT_CT 0x0 /* refer to coding in CT */
-#define HDMI_AUDIO_CXT_HE_AAC 0x1
-#define HDMI_AUDIO_CXT_HE_AAC_V2 0x2
-#define HDMI_AUDIO_CXT_MPEG_SURROUND 0x3
-
-/* all fields little endian */
-struct hdmi_stereo_infoframe {
- /* PB0 */
- u8 csum;
-
- /* PB1 */
- u8 regid0;
-
- /* PB2 */
- u8 regid1;
-
- /* PB3 */
- u8 regid2;
-
- /* PB4 */
- unsigned res1:5;
- unsigned hdmi_video_format:3;
-
- /* PB5 */
- unsigned res2:4;
- unsigned _3d_structure:4;
-
- /* PB6*/
- unsigned res3:4;
- unsigned _3d_ext_data:4;
-} __packed;
-
-#define HDMI_VENDOR_VERSION 0x01
-
/* register definitions */
#define HDMI_CTXSW 0x00
--
1.8.0.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* [RFC v2 5/5] drm/i915: Use generic HDMI infoframe helpers
2012-12-05 16:45 [RFC v2 0/5] Add HDMI infoframe helpers Thierry Reding
` (3 preceding siblings ...)
2012-12-05 16:45 ` [RFC v2 4/5] drm: tegra: Use generic " Thierry Reding
@ 2012-12-05 16:45 ` Thierry Reding
2012-12-06 14:16 ` Daniel Vetter
` (2 more replies)
4 siblings, 3 replies; 30+ messages in thread
From: Thierry Reding @ 2012-12-05 16:45 UTC (permalink / raw)
To: dri-devel
Use the generic HDMI infoframe helpers to get rid of the duplicate
implementation in the i915 driver.
Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
drivers/gpu/drm/Kconfig | 2 +
drivers/gpu/drm/i915/intel_drv.h | 62 +--------
drivers/gpu/drm/i915/intel_hdmi.c | 268 +++++++++++++++++++++-----------------
drivers/gpu/drm/i915/intel_sdvo.c | 22 ++--
4 files changed, 159 insertions(+), 195 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 94a4623..5225012 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -144,6 +144,8 @@ config DRM_I915
select INPUT if ACPI
select ACPI_VIDEO if ACPI
select ACPI_BUTTON if ACPI
+ select DRM_HDMI
+ select HDMI
help
Choose this option if you have a system that has "Intel Graphics
Media Accelerator" or "HD Graphics" integrated graphics,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 522061c..70a7440 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -276,63 +276,6 @@ struct cxsr_latency {
#define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
#define to_intel_plane(x) container_of(x, struct intel_plane, base)
-#define DIP_HEADER_SIZE 5
-
-#define DIP_TYPE_AVI 0x82
-#define DIP_VERSION_AVI 0x2
-#define DIP_LEN_AVI 13
-#define DIP_AVI_PR_1 0
-#define DIP_AVI_PR_2 1
-
-#define DIP_TYPE_SPD 0x83
-#define DIP_VERSION_SPD 0x1
-#define DIP_LEN_SPD 25
-#define DIP_SPD_UNKNOWN 0
-#define DIP_SPD_DSTB 0x1
-#define DIP_SPD_DVDP 0x2
-#define DIP_SPD_DVHS 0x3
-#define DIP_SPD_HDDVR 0x4
-#define DIP_SPD_DVC 0x5
-#define DIP_SPD_DSC 0x6
-#define DIP_SPD_VCD 0x7
-#define DIP_SPD_GAME 0x8
-#define DIP_SPD_PC 0x9
-#define DIP_SPD_BD 0xa
-#define DIP_SPD_SCD 0xb
-
-struct dip_infoframe {
- uint8_t type; /* HB0 */
- uint8_t ver; /* HB1 */
- uint8_t len; /* HB2 - body len, not including checksum */
- uint8_t ecc; /* Header ECC */
- uint8_t checksum; /* PB0 */
- union {
- struct {
- /* PB1 - Y 6:5, A 4:4, B 3:2, S 1:0 */
- uint8_t Y_A_B_S;
- /* PB2 - C 7:6, M 5:4, R 3:0 */
- uint8_t C_M_R;
- /* PB3 - ITC 7:7, EC 6:4, Q 3:2, SC 1:0 */
- uint8_t ITC_EC_Q_SC;
- /* PB4 - VIC 6:0 */
- uint8_t VIC;
- /* PB5 - YQ 7:6, CN 5:4, PR 3:0 */
- uint8_t YQ_CN_PR;
- /* PB6 to PB13 */
- uint16_t top_bar_end;
- uint16_t bottom_bar_start;
- uint16_t left_bar_end;
- uint16_t right_bar_start;
- } __attribute__ ((packed)) avi;
- struct {
- uint8_t vn[8];
- uint8_t pd[16];
- uint8_t sdi;
- } __attribute__ ((packed)) spd;
- uint8_t payload[27];
- } __attribute__ ((packed)) body;
-} __attribute__((packed));
-
struct intel_hdmi {
u32 sdvox_reg;
int ddc_bus;
@@ -340,8 +283,8 @@ struct intel_hdmi {
bool has_hdmi_sink;
bool has_audio;
enum hdmi_force_audio force_audio;
- void (*write_infoframe)(struct drm_encoder *encoder,
- struct dip_infoframe *frame);
+ void (*write_infoframe)(struct drm_encoder *encoder, const void *frame,
+ size_t len);
void (*set_infoframes)(struct drm_encoder *encoder,
struct drm_display_mode *adjusted_mode);
};
@@ -430,7 +373,6 @@ extern struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
extern bool intel_hdmi_mode_fixup(struct drm_encoder *encoder,
const struct drm_display_mode *mode,
struct drm_display_mode *adjusted_mode);
-extern void intel_dip_infoframe_csum(struct dip_infoframe *avi_if);
extern bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg,
bool is_sdvob);
extern void intel_dvo_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 5c279b4..a5492b1 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -29,9 +29,11 @@
#include <linux/i2c.h>
#include <linux/slab.h>
#include <linux/delay.h>
+#include <linux/hdmi.h>
#include <drm/drmP.h>
#include <drm/drm_crtc.h>
#include <drm/drm_edid.h>
+#include <drm/drm_hdmi.h>
#include "intel_drv.h"
#include <drm/i915_drm.h>
#include "i915_drv.h"
@@ -66,102 +68,112 @@ static struct intel_hdmi *intel_attached_hdmi(struct drm_connector *connector)
return enc_to_intel_hdmi(&intel_attached_encoder(connector)->base);
}
-void intel_dip_infoframe_csum(struct dip_infoframe *frame)
+static u32 g4x_infoframe_index(enum hdmi_infoframe_type type)
{
- uint8_t *data = (uint8_t *)frame;
- uint8_t sum = 0;
- unsigned i;
-
- frame->checksum = 0;
- frame->ecc = 0;
-
- for (i = 0; i < frame->len + DIP_HEADER_SIZE; i++)
- sum += data[i];
-
- frame->checksum = 0x100 - sum;
-}
-
-static u32 g4x_infoframe_index(struct dip_infoframe *frame)
-{
- switch (frame->type) {
- case DIP_TYPE_AVI:
+ switch (type) {
+ case HDMI_INFOFRAME_TYPE_AVI:
return VIDEO_DIP_SELECT_AVI;
- case DIP_TYPE_SPD:
+ case HDMI_INFOFRAME_TYPE_SPD:
return VIDEO_DIP_SELECT_SPD;
default:
- DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
+ DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
return 0;
}
}
-static u32 g4x_infoframe_enable(struct dip_infoframe *frame)
+static u32 g4x_infoframe_enable(enum hdmi_infoframe_type type)
{
- switch (frame->type) {
- case DIP_TYPE_AVI:
+ switch (type) {
+ case HDMI_INFOFRAME_TYPE_AVI:
return VIDEO_DIP_ENABLE_AVI;
- case DIP_TYPE_SPD:
+ case HDMI_INFOFRAME_TYPE_SPD:
return VIDEO_DIP_ENABLE_SPD;
default:
- DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
+ DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
return 0;
}
}
-static u32 hsw_infoframe_enable(struct dip_infoframe *frame)
+static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type)
{
- switch (frame->type) {
- case DIP_TYPE_AVI:
+ switch (type) {
+ case HDMI_INFOFRAME_TYPE_AVI:
return VIDEO_DIP_ENABLE_AVI_HSW;
- case DIP_TYPE_SPD:
+ case HDMI_INFOFRAME_TYPE_SPD:
return VIDEO_DIP_ENABLE_SPD_HSW;
default:
- DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
+ DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
return 0;
}
}
-static u32 hsw_infoframe_data_reg(struct dip_infoframe *frame, enum pipe pipe)
+static u32 hsw_infoframe_data_reg(enum hdmi_infoframe_type type, enum pipe pipe)
{
- switch (frame->type) {
- case DIP_TYPE_AVI:
+ switch (type) {
+ case HDMI_INFOFRAME_TYPE_AVI:
return HSW_TVIDEO_DIP_AVI_DATA(pipe);
- case DIP_TYPE_SPD:
+ case HDMI_INFOFRAME_TYPE_SPD:
return HSW_TVIDEO_DIP_SPD_DATA(pipe);
default:
- DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
+ DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
return 0;
}
}
-static void g4x_write_infoframe(struct drm_encoder *encoder,
- struct dip_infoframe *frame)
+static u32 intel_dip_pack(const u8 *data, size_t size)
{
- uint32_t *data = (uint32_t *)frame;
+ u32 value = 0;
+ size_t i;
+
+ for (i = size; i > 0; i--)
+ value = (value << 8) | data[i - 1];
+
+ return value;
+}
+
+static void g4x_dip_write(struct drm_i915_private *dev_priv,
+ unsigned long offset, const void *dip, size_t len)
+{
+ u32 data;
+ size_t i;
+
+ /* Write infoframe header and zero out ECC at byte 4 */
+ data = intel_dip_pack(dip, 3);
+ I915_WRITE(offset, data);
+
+ for (i = 3; i < len; i += 4) {
+ size_t rem = min_t(size_t, len - i, 4);
+ data = intel_dip_pack(dip + i, rem);
+ I915_WRITE(offset, data);
+ }
+
+ /* Write every possible data byte to force correct ECC calculation. */
+ for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
+ I915_WRITE(offset, 0);
+}
+
+static void g4x_write_infoframe(struct drm_encoder *encoder, const void *frame,
+ size_t len)
+{
+ enum hdmi_infoframe_type type = ((const u8 *)frame)[0];
struct drm_device *dev = encoder->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
u32 val = I915_READ(VIDEO_DIP_CTL);
- unsigned i, len = DIP_HEADER_SIZE + frame->len;
WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
- val |= g4x_infoframe_index(frame);
+ val |= g4x_infoframe_index(type);
- val &= ~g4x_infoframe_enable(frame);
+ val &= ~g4x_infoframe_enable(type);
I915_WRITE(VIDEO_DIP_CTL, val);
mmiowb();
- for (i = 0; i < len; i += 4) {
- I915_WRITE(VIDEO_DIP_DATA, *data);
- data++;
- }
- /* Write every possible data byte to force correct ECC calculation. */
- for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
- I915_WRITE(VIDEO_DIP_DATA, 0);
+ g4x_dip_write(dev_priv, VIDEO_DIP_DATA, frame, len);
mmiowb();
- val |= g4x_infoframe_enable(frame);
+ val |= g4x_infoframe_enable(type);
val &= ~VIDEO_DIP_FREQ_MASK;
val |= VIDEO_DIP_FREQ_VSYNC;
@@ -169,37 +181,31 @@ static void g4x_write_infoframe(struct drm_encoder *encoder,
POSTING_READ(VIDEO_DIP_CTL);
}
-static void ibx_write_infoframe(struct drm_encoder *encoder,
- struct dip_infoframe *frame)
+static void ibx_write_infoframe(struct drm_encoder *encoder, const void *frame,
+ size_t len)
{
- uint32_t *data = (uint32_t *)frame;
+ enum hdmi_infoframe_type type = ((const u8 *)frame)[0];
struct drm_device *dev = encoder->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
- unsigned i, len = DIP_HEADER_SIZE + frame->len;
u32 val = I915_READ(reg);
WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
- val |= g4x_infoframe_index(frame);
+ val |= g4x_infoframe_index(type);
- val &= ~g4x_infoframe_enable(frame);
+ val &= ~g4x_infoframe_enable(type);
I915_WRITE(reg, val);
mmiowb();
- for (i = 0; i < len; i += 4) {
- I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
- data++;
- }
- /* Write every possible data byte to force correct ECC calculation. */
- for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
- I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0);
+ g4x_dip_write(dev_priv, TVIDEO_DIP_DATA(intel_crtc->pipe), frame,
+ len);
mmiowb();
- val |= g4x_infoframe_enable(frame);
+ val |= g4x_infoframe_enable(type);
val &= ~VIDEO_DIP_FREQ_MASK;
val |= VIDEO_DIP_FREQ_VSYNC;
@@ -207,40 +213,33 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
POSTING_READ(reg);
}
-static void cpt_write_infoframe(struct drm_encoder *encoder,
- struct dip_infoframe *frame)
+static void cpt_write_infoframe(struct drm_encoder *encoder, const void *frame,
+ size_t len)
{
- uint32_t *data = (uint32_t *)frame;
+ enum hdmi_infoframe_type type = ((const u8 *)frame)[0];
struct drm_device *dev = encoder->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
- unsigned i, len = DIP_HEADER_SIZE + frame->len;
u32 val = I915_READ(reg);
WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
- val |= g4x_infoframe_index(frame);
+ val |= g4x_infoframe_index(type);
/* The DIP control register spec says that we need to update the AVI
* infoframe without clearing its enable bit */
- if (frame->type != DIP_TYPE_AVI)
- val &= ~g4x_infoframe_enable(frame);
+ if (type != HDMI_INFOFRAME_TYPE_AVI)
+ val &= ~g4x_infoframe_enable(type);
I915_WRITE(reg, val);
mmiowb();
- for (i = 0; i < len; i += 4) {
- I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
- data++;
- }
- /* Write every possible data byte to force correct ECC calculation. */
- for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
- I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0);
+ g4x_dip_write(dev_priv, TVIDEO_DIP_DATA(intel_crtc->pipe), frame, len);
mmiowb();
- val |= g4x_infoframe_enable(frame);
+ val |= g4x_infoframe_enable(type);
val &= ~VIDEO_DIP_FREQ_MASK;
val |= VIDEO_DIP_FREQ_VSYNC;
@@ -248,37 +247,31 @@ static void cpt_write_infoframe(struct drm_encoder *encoder,
POSTING_READ(reg);
}
-static void vlv_write_infoframe(struct drm_encoder *encoder,
- struct dip_infoframe *frame)
+static void vlv_write_infoframe(struct drm_encoder *encoder, const void *frame,
+ size_t len)
{
- uint32_t *data = (uint32_t *)frame;
+ enum hdmi_infoframe_type type = ((const u8 *)frame)[0];
struct drm_device *dev = encoder->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
int reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
- unsigned i, len = DIP_HEADER_SIZE + frame->len;
u32 val = I915_READ(reg);
WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
- val |= g4x_infoframe_index(frame);
+ val |= g4x_infoframe_index(type);
- val &= ~g4x_infoframe_enable(frame);
+ val &= ~g4x_infoframe_enable(type);
I915_WRITE(reg, val);
mmiowb();
- for (i = 0; i < len; i += 4) {
- I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
- data++;
- }
- /* Write every possible data byte to force correct ECC calculation. */
- for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
- I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), 0);
+ g4x_dip_write(dev_priv, VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), frame,
+ len);
mmiowb();
- val |= g4x_infoframe_enable(frame);
+ val |= g4x_infoframe_enable(type);
val &= ~VIDEO_DIP_FREQ_MASK;
val |= VIDEO_DIP_FREQ_VSYNC;
@@ -286,76 +279,105 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
POSTING_READ(reg);
}
-static void hsw_write_infoframe(struct drm_encoder *encoder,
- struct dip_infoframe *frame)
+static void hsw_write_infoframe(struct drm_encoder *encoder, const void *frame,
+ size_t len)
{
- uint32_t *data = (uint32_t *)frame;
+ enum hdmi_infoframe_type type = ((const u8 *)frame)[0];
struct drm_device *dev = encoder->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
u32 ctl_reg = HSW_TVIDEO_DIP_CTL(intel_crtc->pipe);
- u32 data_reg = hsw_infoframe_data_reg(frame, intel_crtc->pipe);
- unsigned int i, len = DIP_HEADER_SIZE + frame->len;
+ u32 data_reg = hsw_infoframe_data_reg(type, intel_crtc->pipe);
u32 val = I915_READ(ctl_reg);
+ unsigned long offset;
+ u32 data;
+ size_t i;
if (data_reg == 0)
return;
- val &= ~hsw_infoframe_enable(frame);
+ val &= ~hsw_infoframe_enable(type);
I915_WRITE(ctl_reg, val);
mmiowb();
- for (i = 0; i < len; i += 4) {
- I915_WRITE(data_reg + i, *data);
- data++;
+
+ /* Write infoframe header and zero out ECC at byte 4 */
+ data = intel_dip_pack(frame, 3);
+ I915_WRITE(data_reg, data);
+
+ for (i = 3, offset = 4; i < len; i += 4, offset += 4) {
+ size_t rem = min_t(size_t, len - i, 4);
+ data = intel_dip_pack(frame + i, rem);
+ I915_WRITE(data_reg + offset, data);
}
+
/* Write every possible data byte to force correct ECC calculation. */
- for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
- I915_WRITE(data_reg + i, 0);
+ for (; i < VIDEO_DIP_DATA_SIZE; i += 4, offset += 4)
+ I915_WRITE(data_reg + offset, 0);
+
mmiowb();
- val |= hsw_infoframe_enable(frame);
+ val |= hsw_infoframe_enable(type);
I915_WRITE(ctl_reg, val);
POSTING_READ(ctl_reg);
}
+/*
static void intel_set_infoframe(struct drm_encoder *encoder,
- struct dip_infoframe *frame)
+ const struct hdmi_infoframe *frame)
{
struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+ u8 buffer[32];
+ ssize_t len;
- intel_dip_infoframe_csum(frame);
- intel_hdmi->write_infoframe(encoder, frame);
+ len = hdmi_infoframe_pack(frame, buffer, sizeof(buffer));
+ if (len < 0)
+ return;
+
+ intel_hdmi->write_infoframe(encoder, buffer, len);
}
+*/
static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
struct drm_display_mode *adjusted_mode)
{
- struct dip_infoframe avi_if = {
- .type = DIP_TYPE_AVI,
- .ver = DIP_VERSION_AVI,
- .len = DIP_LEN_AVI,
- };
+ struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+ u8 buffer[HDMI_AVI_INFOFRAME_SIZE];
+ struct hdmi_avi_infoframe frame;
+ ssize_t len;
+
+ len = drm_hdmi_avi_infoframe_from_display_mode(&frame, adjusted_mode);
+ if (len < 0)
+ return;
if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
- avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
+ frame.pixel_repeat = 2;
+
+ len = hdmi_avi_infoframe_pack(&frame, buffer, sizeof(buffer));
+ if (len < 0)
+ return;
- intel_set_infoframe(encoder, &avi_if);
+ intel_hdmi->write_infoframe(encoder, buffer, len);
}
static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
{
- struct dip_infoframe spd_if;
+ struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+ u8 buffer[HDMI_SPD_INFOFRAME_SIZE];
+ struct hdmi_spd_infoframe frame;
+ ssize_t len;
+
+ len = hdmi_spd_infoframe_init(&frame, "Intel", "Integrated gfx");
+ if (len < 0)
+ return;
+
+ frame.sdi = HDMI_SPD_SDI_PC;
- memset(&spd_if, 0, sizeof(spd_if));
- spd_if.type = DIP_TYPE_SPD;
- spd_if.ver = DIP_VERSION_SPD;
- spd_if.len = DIP_LEN_SPD;
- strcpy(spd_if.body.spd.vn, "Intel");
- strcpy(spd_if.body.spd.pd, "Integrated gfx");
- spd_if.body.spd.sdi = DIP_SPD_PC;
+ len = hdmi_spd_infoframe_pack(&frame, buffer, sizeof(buffer));
+ if (len < 0)
+ return;
- intel_set_infoframe(encoder, &spd_if);
+ intel_hdmi->write_infoframe(encoder, buffer, len);
}
static void g4x_set_infoframes(struct drm_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 4b07401..192d791 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -29,6 +29,7 @@
#include <linux/slab.h>
#include <linux/delay.h>
#include <linux/export.h>
+#include <linux/hdmi.h>
#include <drm/drmP.h>
#include <drm/drm_crtc.h>
#include <drm/drm_edid.h>
@@ -935,20 +936,17 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo,
static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo)
{
- struct dip_infoframe avi_if = {
- .type = DIP_TYPE_AVI,
- .ver = DIP_VERSION_AVI,
- .len = DIP_LEN_AVI,
- };
- uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)];
+ uint8_t sdvo_data[HDMI_AVI_INFOFRAME_SIZE];
+ struct hdmi_avi_infoframe frame;
+ ssize_t len;
- intel_dip_infoframe_csum(&avi_if);
+ len = hdmi_avi_infoframe_init(&frame);
+ if (len < 0)
+ return false;
- /* sdvo spec says that the ecc is handled by the hw, and it looks like
- * we must not send the ecc field, either. */
- memcpy(sdvo_data, &avi_if, 3);
- sdvo_data[3] = avi_if.checksum;
- memcpy(&sdvo_data[4], &avi_if.body, sizeof(avi_if.body.avi));
+ len = hdmi_avi_infoframe_pack(&frame, sdvo_data, sizeof(sdvo_data));
+ if (len < 0)
+ return false;
return intel_sdvo_write_infoframe(intel_sdvo, SDVO_HBUF_INDEX_AVI_IF,
SDVO_HBUF_TX_VSYNC,
--
1.8.0.1
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [RFC v2 5/5] drm/i915: Use generic HDMI infoframe helpers
2012-12-05 16:45 ` [RFC v2 5/5] drm/i915: " Thierry Reding
@ 2012-12-06 14:16 ` Daniel Vetter
2012-12-06 14:23 ` Thierry Reding
2012-12-06 16:11 ` Paulo Zanoni
2012-12-07 15:11 ` [RFC] " Paulo Zanoni
2 siblings, 1 reply; 30+ messages in thread
From: Daniel Vetter @ 2012-12-06 14:16 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel
On Wed, Dec 05, 2012 at 05:45:44PM +0100, Thierry Reding wrote:
> Use the generic HDMI infoframe helpers to get rid of the duplicate
> implementation in the i915 driver.
>
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 4b07401..192d791 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -29,6 +29,7 @@
> #include <linux/slab.h>
> #include <linux/delay.h>
> #include <linux/export.h>
> +#include <linux/hdmi.h>
> #include <drm/drmP.h>
> #include <drm/drm_crtc.h>
> #include <drm/drm_edid.h>
> @@ -935,20 +936,17 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo,
>
> static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo)
> {
> - struct dip_infoframe avi_if = {
> - .type = DIP_TYPE_AVI,
> - .ver = DIP_VERSION_AVI,
> - .len = DIP_LEN_AVI,
> - };
> - uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)];
> + uint8_t sdvo_data[HDMI_AVI_INFOFRAME_SIZE];
> + struct hdmi_avi_infoframe frame;
> + ssize_t len;
>
> - intel_dip_infoframe_csum(&avi_if);
> + len = hdmi_avi_infoframe_init(&frame);
> + if (len < 0)
> + return false;
>
> - /* sdvo spec says that the ecc is handled by the hw, and it looks like
> - * we must not send the ecc field, either. */
> - memcpy(sdvo_data, &avi_if, 3);
> - sdvo_data[3] = avi_if.checksum;
This difference between how we need to send the infoframe to the hw
between sdvo (without ecc field present) and native hdmi (ecc field
present, but still computed by the hw) seems to have been lost. On a quick
read from your infoframe packer it looks like the ecc field isn't part of
the packed infoframe, so this patch would break native hdmi ports (in the
intel_hdmi.c file).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC v2 5/5] drm/i915: Use generic HDMI infoframe helpers
2012-12-06 14:16 ` Daniel Vetter
@ 2012-12-06 14:23 ` Thierry Reding
2012-12-06 15:57 ` Daniel Vetter
0 siblings, 1 reply; 30+ messages in thread
From: Thierry Reding @ 2012-12-06 14:23 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 2583 bytes --]
On Thu, Dec 06, 2012 at 03:16:31PM +0100, Daniel Vetter wrote:
> On Wed, Dec 05, 2012 at 05:45:44PM +0100, Thierry Reding wrote:
> > Use the generic HDMI infoframe helpers to get rid of the duplicate
> > implementation in the i915 driver.
> >
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
>
> > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> > index 4b07401..192d791 100644
> > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > @@ -29,6 +29,7 @@
> > #include <linux/slab.h>
> > #include <linux/delay.h>
> > #include <linux/export.h>
> > +#include <linux/hdmi.h>
> > #include <drm/drmP.h>
> > #include <drm/drm_crtc.h>
> > #include <drm/drm_edid.h>
> > @@ -935,20 +936,17 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo,
> >
> > static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo)
> > {
> > - struct dip_infoframe avi_if = {
> > - .type = DIP_TYPE_AVI,
> > - .ver = DIP_VERSION_AVI,
> > - .len = DIP_LEN_AVI,
> > - };
> > - uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)];
> > + uint8_t sdvo_data[HDMI_AVI_INFOFRAME_SIZE];
> > + struct hdmi_avi_infoframe frame;
> > + ssize_t len;
> >
> > - intel_dip_infoframe_csum(&avi_if);
> > + len = hdmi_avi_infoframe_init(&frame);
> > + if (len < 0)
> > + return false;
> >
> > - /* sdvo spec says that the ecc is handled by the hw, and it looks like
> > - * we must not send the ecc field, either. */
> > - memcpy(sdvo_data, &avi_if, 3);
> > - sdvo_data[3] = avi_if.checksum;
>
> This difference between how we need to send the infoframe to the hw
> between sdvo (without ecc field present) and native hdmi (ecc field
> present, but still computed by the hw) seems to have been lost. On a quick
> read from your infoframe packer it looks like the ecc field isn't part of
> the packed infoframe, so this patch would break native hdmi ports (in the
> intel_hdmi.c file).
Actually this ECC field seems to be something specific to Intel
hardware. It doesn't appear in the HDMI specification nor in CEA-861-D.
So it the binary infoframe representation doesn't contain it, therefore
it should be handled by the respective drivers. So in fact the code
should be working fine for HDMI and SDVO, since for the HDMI ports I
explicitly left the fourth byte of the header empty, while for SDVO, the
checksum is the one as computed by hdmi_avi_infoframe_pack(), which is
at byte 3 and therefore matches the previous code.
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
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC v2 5/5] drm/i915: Use generic HDMI infoframe helpers
2012-12-06 14:23 ` Thierry Reding
@ 2012-12-06 15:57 ` Daniel Vetter
2012-12-06 16:02 ` Thierry Reding
0 siblings, 1 reply; 30+ messages in thread
From: Daniel Vetter @ 2012-12-06 15:57 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel
On Thu, Dec 6, 2012 at 3:23 PM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
>> > - /* sdvo spec says that the ecc is handled by the hw, and it looks like
>> > - * we must not send the ecc field, either. */
>> > - memcpy(sdvo_data, &avi_if, 3);
>> > - sdvo_data[3] = avi_if.checksum;
>>
>> This difference between how we need to send the infoframe to the hw
>> between sdvo (without ecc field present) and native hdmi (ecc field
>> present, but still computed by the hw) seems to have been lost. On a quick
>> read from your infoframe packer it looks like the ecc field isn't part of
>> the packed infoframe, so this patch would break native hdmi ports (in the
>> intel_hdmi.c file).
>
> Actually this ECC field seems to be something specific to Intel
> hardware. It doesn't appear in the HDMI specification nor in CEA-861-D.
> So it the binary infoframe representation doesn't contain it, therefore
> it should be handled by the respective drivers. So in fact the code
> should be working fine for HDMI and SDVO, since for the HDMI ports I
> explicitly left the fourth byte of the header empty, while for SDVO, the
> checksum is the one as computed by hdmi_avi_infoframe_pack(), which is
> at byte 3 and therefore matches the previous code.
Oh dear, I've failed to read the patch correctly and totally missed
the new intel_dip_pack function and assorted logic changes. Sorry for
the confusion.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC v2 5/5] drm/i915: Use generic HDMI infoframe helpers
2012-12-06 15:57 ` Daniel Vetter
@ 2012-12-06 16:02 ` Thierry Reding
0 siblings, 0 replies; 30+ messages in thread
From: Thierry Reding @ 2012-12-06 16:02 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 1907 bytes --]
On Thu, Dec 06, 2012 at 04:57:27PM +0100, Daniel Vetter wrote:
> On Thu, Dec 6, 2012 at 3:23 PM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> >> > - /* sdvo spec says that the ecc is handled by the hw, and it looks like
> >> > - * we must not send the ecc field, either. */
> >> > - memcpy(sdvo_data, &avi_if, 3);
> >> > - sdvo_data[3] = avi_if.checksum;
> >>
> >> This difference between how we need to send the infoframe to the hw
> >> between sdvo (without ecc field present) and native hdmi (ecc field
> >> present, but still computed by the hw) seems to have been lost. On a quick
> >> read from your infoframe packer it looks like the ecc field isn't part of
> >> the packed infoframe, so this patch would break native hdmi ports (in the
> >> intel_hdmi.c file).
> >
> > Actually this ECC field seems to be something specific to Intel
> > hardware. It doesn't appear in the HDMI specification nor in CEA-861-D.
> > So it the binary infoframe representation doesn't contain it, therefore
> > it should be handled by the respective drivers. So in fact the code
> > should be working fine for HDMI and SDVO, since for the HDMI ports I
> > explicitly left the fourth byte of the header empty, while for SDVO, the
> > checksum is the one as computed by hdmi_avi_infoframe_pack(), which is
> > at byte 3 and therefore matches the previous code.
>
> Oh dear, I've failed to read the patch correctly and totally missed
> the new intel_dip_pack function and assorted logic changes. Sorry for
> the confusion.
At first I thought to split that out in a separate patch but then
decided it wasn't worth the trouble. But there's always the possibility
that I've broken things horribly here since the patch is rather messy,
so it's good that you look closely and some testing will certainly be
good to validate that things still work as before.
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC v2 5/5] drm/i915: Use generic HDMI infoframe helpers
2012-12-05 16:45 ` [RFC v2 5/5] drm/i915: " Thierry Reding
2012-12-06 14:16 ` Daniel Vetter
@ 2012-12-06 16:11 ` Paulo Zanoni
2012-12-06 16:55 ` Paulo Zanoni
2012-12-07 7:22 ` Thierry Reding
2012-12-07 15:11 ` [RFC] " Paulo Zanoni
2 siblings, 2 replies; 30+ messages in thread
From: Paulo Zanoni @ 2012-12-06 16:11 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel
Hi
2012/12/5 Thierry Reding <thierry.reding@avionic-design.de>:
> Use the generic HDMI infoframe helpers to get rid of the duplicate
> implementation in the i915 driver.
A few comments:
- I've compiled your patches and now "intel_infoframes -d" tells my my
infoframes are full of zeroes. So there's a bug somewhere... I have to
say that the i915 infoframe registers are quite complicated and it
took me a long time to fix a lot of its problems, so please don't
break it and read below for a suggestion :)
- Why do we need that kconfig stuff? Why not just put all the code
inside drivers/gpu/drm?
- I really like having "common code between drivers merged", but I
really don't see how the i915 driver is benefiting from this change.
We're just basically complicating intel_hdmi.c to use a new struct
that doesn't fit our needs due to the lack of the ECC byte. My idea:
Instead of calling hdmi_avi_infoframe_pack, why don't we just
implement intel_hdmi_avi_infoframe_pack that converts a "struct
hdmi_avi_infoframe" into our own good-old "struct dip_infoframe" that
has the cool ECC value in the right place? This would probably
drastically reduce your chances of introducing bugs in our code :)
- Instead of "converting our own structs into structs that don't
exactly fit our usage", what I would really like to see is generic
_optional_ drm functions to help me fill the infoframe data, like the
things I did in the past. Examples:
(i) I want a function that reads the EDID and tells me whether the
monitor is going to respect my "underscan" settings (AVI infoframe
field S) or not.
(ii) I want unified overscan/underscan properties between all the
drivers: we need properties to allow requesting specific
overscan/underscan properties and also properties to allow the driver
to force underscan in case the monitor doesn't want to cooperate (as
far as I remember, radeon has these properties, but we need to have
the behavior explicitly documented so all the drivers can try to
implement the exact same thing, and then we also need to ask
user-space guys to add support for these things in their GUIs).
(iii) I want a function that helps me properly setting/changing the
correct pixel and picture aspect ratios. it seems the only difference
between some modes (with different VICs) is the pixel/aspect ratio,
how do we expose this to the user-space and let it select the one it
wants? How does the driver know exactly which one is the selected one?
(iv) I want code that enables user-space to use those modes with
variable pixel repetition rates and select exactly the one it wants to
use with the correct pixel repetition.
(v) I'd like a quirk list to recognize monitors/devices that don't
work correctly when they receive infoframes, or when the VIC is 0,
etc.
(vi) I want magical functions that help me setting all those
color-related stuff I did not study yet.
(vii) Ideally, I'd like to accomplish all the above by looking at
"struct drm_display_mode" and maybe passing it to helper functions
that I can _optionally_ use if I want.
(viii) I don't want the infoframe code to be a "mid layer" that gets
in the way, I want it to be a set of optional Lego bricks I can use if
I need.
(ix) There's certainly more things I'm missing here, but I believe you
got the idea :)
Of course, implementing all I described will take a few months :)
Thanks,
Paulo
>
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> ---
> drivers/gpu/drm/Kconfig | 2 +
> drivers/gpu/drm/i915/intel_drv.h | 62 +--------
> drivers/gpu/drm/i915/intel_hdmi.c | 268 +++++++++++++++++++++-----------------
> drivers/gpu/drm/i915/intel_sdvo.c | 22 ++--
> 4 files changed, 159 insertions(+), 195 deletions(-)
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 94a4623..5225012 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -144,6 +144,8 @@ config DRM_I915
> select INPUT if ACPI
> select ACPI_VIDEO if ACPI
> select ACPI_BUTTON if ACPI
> + select DRM_HDMI
> + select HDMI
> help
> Choose this option if you have a system that has "Intel Graphics
> Media Accelerator" or "HD Graphics" integrated graphics,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 522061c..70a7440 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -276,63 +276,6 @@ struct cxsr_latency {
> #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
> #define to_intel_plane(x) container_of(x, struct intel_plane, base)
>
> -#define DIP_HEADER_SIZE 5
> -
> -#define DIP_TYPE_AVI 0x82
> -#define DIP_VERSION_AVI 0x2
> -#define DIP_LEN_AVI 13
> -#define DIP_AVI_PR_1 0
> -#define DIP_AVI_PR_2 1
> -
> -#define DIP_TYPE_SPD 0x83
> -#define DIP_VERSION_SPD 0x1
> -#define DIP_LEN_SPD 25
> -#define DIP_SPD_UNKNOWN 0
> -#define DIP_SPD_DSTB 0x1
> -#define DIP_SPD_DVDP 0x2
> -#define DIP_SPD_DVHS 0x3
> -#define DIP_SPD_HDDVR 0x4
> -#define DIP_SPD_DVC 0x5
> -#define DIP_SPD_DSC 0x6
> -#define DIP_SPD_VCD 0x7
> -#define DIP_SPD_GAME 0x8
> -#define DIP_SPD_PC 0x9
> -#define DIP_SPD_BD 0xa
> -#define DIP_SPD_SCD 0xb
> -
> -struct dip_infoframe {
> - uint8_t type; /* HB0 */
> - uint8_t ver; /* HB1 */
> - uint8_t len; /* HB2 - body len, not including checksum */
> - uint8_t ecc; /* Header ECC */
> - uint8_t checksum; /* PB0 */
> - union {
> - struct {
> - /* PB1 - Y 6:5, A 4:4, B 3:2, S 1:0 */
> - uint8_t Y_A_B_S;
> - /* PB2 - C 7:6, M 5:4, R 3:0 */
> - uint8_t C_M_R;
> - /* PB3 - ITC 7:7, EC 6:4, Q 3:2, SC 1:0 */
> - uint8_t ITC_EC_Q_SC;
> - /* PB4 - VIC 6:0 */
> - uint8_t VIC;
> - /* PB5 - YQ 7:6, CN 5:4, PR 3:0 */
> - uint8_t YQ_CN_PR;
> - /* PB6 to PB13 */
> - uint16_t top_bar_end;
> - uint16_t bottom_bar_start;
> - uint16_t left_bar_end;
> - uint16_t right_bar_start;
> - } __attribute__ ((packed)) avi;
> - struct {
> - uint8_t vn[8];
> - uint8_t pd[16];
> - uint8_t sdi;
> - } __attribute__ ((packed)) spd;
> - uint8_t payload[27];
> - } __attribute__ ((packed)) body;
> -} __attribute__((packed));
> -
> struct intel_hdmi {
> u32 sdvox_reg;
> int ddc_bus;
> @@ -340,8 +283,8 @@ struct intel_hdmi {
> bool has_hdmi_sink;
> bool has_audio;
> enum hdmi_force_audio force_audio;
> - void (*write_infoframe)(struct drm_encoder *encoder,
> - struct dip_infoframe *frame);
> + void (*write_infoframe)(struct drm_encoder *encoder, const void *frame,
> + size_t len);
> void (*set_infoframes)(struct drm_encoder *encoder,
> struct drm_display_mode *adjusted_mode);
> };
> @@ -430,7 +373,6 @@ extern struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
> extern bool intel_hdmi_mode_fixup(struct drm_encoder *encoder,
> const struct drm_display_mode *mode,
> struct drm_display_mode *adjusted_mode);
> -extern void intel_dip_infoframe_csum(struct dip_infoframe *avi_if);
> extern bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg,
> bool is_sdvob);
> extern void intel_dvo_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 5c279b4..a5492b1 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -29,9 +29,11 @@
> #include <linux/i2c.h>
> #include <linux/slab.h>
> #include <linux/delay.h>
> +#include <linux/hdmi.h>
> #include <drm/drmP.h>
> #include <drm/drm_crtc.h>
> #include <drm/drm_edid.h>
> +#include <drm/drm_hdmi.h>
> #include "intel_drv.h"
> #include <drm/i915_drm.h>
> #include "i915_drv.h"
> @@ -66,102 +68,112 @@ static struct intel_hdmi *intel_attached_hdmi(struct drm_connector *connector)
> return enc_to_intel_hdmi(&intel_attached_encoder(connector)->base);
> }
>
> -void intel_dip_infoframe_csum(struct dip_infoframe *frame)
> +static u32 g4x_infoframe_index(enum hdmi_infoframe_type type)
> {
> - uint8_t *data = (uint8_t *)frame;
> - uint8_t sum = 0;
> - unsigned i;
> -
> - frame->checksum = 0;
> - frame->ecc = 0;
> -
> - for (i = 0; i < frame->len + DIP_HEADER_SIZE; i++)
> - sum += data[i];
> -
> - frame->checksum = 0x100 - sum;
> -}
> -
> -static u32 g4x_infoframe_index(struct dip_infoframe *frame)
> -{
> - switch (frame->type) {
> - case DIP_TYPE_AVI:
> + switch (type) {
> + case HDMI_INFOFRAME_TYPE_AVI:
> return VIDEO_DIP_SELECT_AVI;
> - case DIP_TYPE_SPD:
> + case HDMI_INFOFRAME_TYPE_SPD:
> return VIDEO_DIP_SELECT_SPD;
> default:
> - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
> + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> return 0;
> }
> }
>
> -static u32 g4x_infoframe_enable(struct dip_infoframe *frame)
> +static u32 g4x_infoframe_enable(enum hdmi_infoframe_type type)
> {
> - switch (frame->type) {
> - case DIP_TYPE_AVI:
> + switch (type) {
> + case HDMI_INFOFRAME_TYPE_AVI:
> return VIDEO_DIP_ENABLE_AVI;
> - case DIP_TYPE_SPD:
> + case HDMI_INFOFRAME_TYPE_SPD:
> return VIDEO_DIP_ENABLE_SPD;
> default:
> - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
> + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> return 0;
> }
> }
>
> -static u32 hsw_infoframe_enable(struct dip_infoframe *frame)
> +static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type)
> {
> - switch (frame->type) {
> - case DIP_TYPE_AVI:
> + switch (type) {
> + case HDMI_INFOFRAME_TYPE_AVI:
> return VIDEO_DIP_ENABLE_AVI_HSW;
> - case DIP_TYPE_SPD:
> + case HDMI_INFOFRAME_TYPE_SPD:
> return VIDEO_DIP_ENABLE_SPD_HSW;
> default:
> - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
> + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> return 0;
> }
> }
>
> -static u32 hsw_infoframe_data_reg(struct dip_infoframe *frame, enum pipe pipe)
> +static u32 hsw_infoframe_data_reg(enum hdmi_infoframe_type type, enum pipe pipe)
> {
> - switch (frame->type) {
> - case DIP_TYPE_AVI:
> + switch (type) {
> + case HDMI_INFOFRAME_TYPE_AVI:
> return HSW_TVIDEO_DIP_AVI_DATA(pipe);
> - case DIP_TYPE_SPD:
> + case HDMI_INFOFRAME_TYPE_SPD:
> return HSW_TVIDEO_DIP_SPD_DATA(pipe);
> default:
> - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
> + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
> return 0;
> }
> }
>
> -static void g4x_write_infoframe(struct drm_encoder *encoder,
> - struct dip_infoframe *frame)
> +static u32 intel_dip_pack(const u8 *data, size_t size)
> {
> - uint32_t *data = (uint32_t *)frame;
> + u32 value = 0;
> + size_t i;
> +
> + for (i = size; i > 0; i--)
> + value = (value << 8) | data[i - 1];
> +
> + return value;
> +}
> +
> +static void g4x_dip_write(struct drm_i915_private *dev_priv,
> + unsigned long offset, const void *dip, size_t len)
> +{
> + u32 data;
> + size_t i;
> +
> + /* Write infoframe header and zero out ECC at byte 4 */
> + data = intel_dip_pack(dip, 3);
> + I915_WRITE(offset, data);
> +
> + for (i = 3; i < len; i += 4) {
> + size_t rem = min_t(size_t, len - i, 4);
> + data = intel_dip_pack(dip + i, rem);
> + I915_WRITE(offset, data);
> + }
> +
> + /* Write every possible data byte to force correct ECC calculation. */
> + for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
> + I915_WRITE(offset, 0);
> +}
> +
> +static void g4x_write_infoframe(struct drm_encoder *encoder, const void *frame,
> + size_t len)
> +{
> + enum hdmi_infoframe_type type = ((const u8 *)frame)[0];
> struct drm_device *dev = encoder->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> u32 val = I915_READ(VIDEO_DIP_CTL);
> - unsigned i, len = DIP_HEADER_SIZE + frame->len;
>
> WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
>
> val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
> - val |= g4x_infoframe_index(frame);
> + val |= g4x_infoframe_index(type);
>
> - val &= ~g4x_infoframe_enable(frame);
> + val &= ~g4x_infoframe_enable(type);
>
> I915_WRITE(VIDEO_DIP_CTL, val);
>
> mmiowb();
> - for (i = 0; i < len; i += 4) {
> - I915_WRITE(VIDEO_DIP_DATA, *data);
> - data++;
> - }
> - /* Write every possible data byte to force correct ECC calculation. */
> - for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
> - I915_WRITE(VIDEO_DIP_DATA, 0);
> + g4x_dip_write(dev_priv, VIDEO_DIP_DATA, frame, len);
> mmiowb();
>
> - val |= g4x_infoframe_enable(frame);
> + val |= g4x_infoframe_enable(type);
> val &= ~VIDEO_DIP_FREQ_MASK;
> val |= VIDEO_DIP_FREQ_VSYNC;
>
> @@ -169,37 +181,31 @@ static void g4x_write_infoframe(struct drm_encoder *encoder,
> POSTING_READ(VIDEO_DIP_CTL);
> }
>
> -static void ibx_write_infoframe(struct drm_encoder *encoder,
> - struct dip_infoframe *frame)
> +static void ibx_write_infoframe(struct drm_encoder *encoder, const void *frame,
> + size_t len)
> {
> - uint32_t *data = (uint32_t *)frame;
> + enum hdmi_infoframe_type type = ((const u8 *)frame)[0];
> struct drm_device *dev = encoder->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
> - unsigned i, len = DIP_HEADER_SIZE + frame->len;
> u32 val = I915_READ(reg);
>
> WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
>
> val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
> - val |= g4x_infoframe_index(frame);
> + val |= g4x_infoframe_index(type);
>
> - val &= ~g4x_infoframe_enable(frame);
> + val &= ~g4x_infoframe_enable(type);
>
> I915_WRITE(reg, val);
>
> mmiowb();
> - for (i = 0; i < len; i += 4) {
> - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
> - data++;
> - }
> - /* Write every possible data byte to force correct ECC calculation. */
> - for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
> - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0);
> + g4x_dip_write(dev_priv, TVIDEO_DIP_DATA(intel_crtc->pipe), frame,
> + len);
> mmiowb();
>
> - val |= g4x_infoframe_enable(frame);
> + val |= g4x_infoframe_enable(type);
> val &= ~VIDEO_DIP_FREQ_MASK;
> val |= VIDEO_DIP_FREQ_VSYNC;
>
> @@ -207,40 +213,33 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
> POSTING_READ(reg);
> }
>
> -static void cpt_write_infoframe(struct drm_encoder *encoder,
> - struct dip_infoframe *frame)
> +static void cpt_write_infoframe(struct drm_encoder *encoder, const void *frame,
> + size_t len)
> {
> - uint32_t *data = (uint32_t *)frame;
> + enum hdmi_infoframe_type type = ((const u8 *)frame)[0];
> struct drm_device *dev = encoder->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
> - unsigned i, len = DIP_HEADER_SIZE + frame->len;
> u32 val = I915_READ(reg);
>
> WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
>
> val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
> - val |= g4x_infoframe_index(frame);
> + val |= g4x_infoframe_index(type);
>
> /* The DIP control register spec says that we need to update the AVI
> * infoframe without clearing its enable bit */
> - if (frame->type != DIP_TYPE_AVI)
> - val &= ~g4x_infoframe_enable(frame);
> + if (type != HDMI_INFOFRAME_TYPE_AVI)
> + val &= ~g4x_infoframe_enable(type);
>
> I915_WRITE(reg, val);
>
> mmiowb();
> - for (i = 0; i < len; i += 4) {
> - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
> - data++;
> - }
> - /* Write every possible data byte to force correct ECC calculation. */
> - for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
> - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0);
> + g4x_dip_write(dev_priv, TVIDEO_DIP_DATA(intel_crtc->pipe), frame, len);
> mmiowb();
>
> - val |= g4x_infoframe_enable(frame);
> + val |= g4x_infoframe_enable(type);
> val &= ~VIDEO_DIP_FREQ_MASK;
> val |= VIDEO_DIP_FREQ_VSYNC;
>
> @@ -248,37 +247,31 @@ static void cpt_write_infoframe(struct drm_encoder *encoder,
> POSTING_READ(reg);
> }
>
> -static void vlv_write_infoframe(struct drm_encoder *encoder,
> - struct dip_infoframe *frame)
> +static void vlv_write_infoframe(struct drm_encoder *encoder, const void *frame,
> + size_t len)
> {
> - uint32_t *data = (uint32_t *)frame;
> + enum hdmi_infoframe_type type = ((const u8 *)frame)[0];
> struct drm_device *dev = encoder->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> int reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
> - unsigned i, len = DIP_HEADER_SIZE + frame->len;
> u32 val = I915_READ(reg);
>
> WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
>
> val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
> - val |= g4x_infoframe_index(frame);
> + val |= g4x_infoframe_index(type);
>
> - val &= ~g4x_infoframe_enable(frame);
> + val &= ~g4x_infoframe_enable(type);
>
> I915_WRITE(reg, val);
>
> mmiowb();
> - for (i = 0; i < len; i += 4) {
> - I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
> - data++;
> - }
> - /* Write every possible data byte to force correct ECC calculation. */
> - for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
> - I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), 0);
> + g4x_dip_write(dev_priv, VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), frame,
> + len);
> mmiowb();
>
> - val |= g4x_infoframe_enable(frame);
> + val |= g4x_infoframe_enable(type);
> val &= ~VIDEO_DIP_FREQ_MASK;
> val |= VIDEO_DIP_FREQ_VSYNC;
>
> @@ -286,76 +279,105 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
> POSTING_READ(reg);
> }
>
> -static void hsw_write_infoframe(struct drm_encoder *encoder,
> - struct dip_infoframe *frame)
> +static void hsw_write_infoframe(struct drm_encoder *encoder, const void *frame,
> + size_t len)
> {
> - uint32_t *data = (uint32_t *)frame;
> + enum hdmi_infoframe_type type = ((const u8 *)frame)[0];
> struct drm_device *dev = encoder->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> u32 ctl_reg = HSW_TVIDEO_DIP_CTL(intel_crtc->pipe);
> - u32 data_reg = hsw_infoframe_data_reg(frame, intel_crtc->pipe);
> - unsigned int i, len = DIP_HEADER_SIZE + frame->len;
> + u32 data_reg = hsw_infoframe_data_reg(type, intel_crtc->pipe);
> u32 val = I915_READ(ctl_reg);
> + unsigned long offset;
> + u32 data;
> + size_t i;
>
> if (data_reg == 0)
> return;
>
> - val &= ~hsw_infoframe_enable(frame);
> + val &= ~hsw_infoframe_enable(type);
> I915_WRITE(ctl_reg, val);
>
> mmiowb();
> - for (i = 0; i < len; i += 4) {
> - I915_WRITE(data_reg + i, *data);
> - data++;
> +
> + /* Write infoframe header and zero out ECC at byte 4 */
> + data = intel_dip_pack(frame, 3);
> + I915_WRITE(data_reg, data);
> +
> + for (i = 3, offset = 4; i < len; i += 4, offset += 4) {
> + size_t rem = min_t(size_t, len - i, 4);
> + data = intel_dip_pack(frame + i, rem);
> + I915_WRITE(data_reg + offset, data);
> }
> +
> /* Write every possible data byte to force correct ECC calculation. */
> - for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
> - I915_WRITE(data_reg + i, 0);
> + for (; i < VIDEO_DIP_DATA_SIZE; i += 4, offset += 4)
> + I915_WRITE(data_reg + offset, 0);
> +
> mmiowb();
>
> - val |= hsw_infoframe_enable(frame);
> + val |= hsw_infoframe_enable(type);
> I915_WRITE(ctl_reg, val);
> POSTING_READ(ctl_reg);
> }
>
> +/*
> static void intel_set_infoframe(struct drm_encoder *encoder,
> - struct dip_infoframe *frame)
> + const struct hdmi_infoframe *frame)
> {
> struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> + u8 buffer[32];
> + ssize_t len;
>
> - intel_dip_infoframe_csum(frame);
> - intel_hdmi->write_infoframe(encoder, frame);
> + len = hdmi_infoframe_pack(frame, buffer, sizeof(buffer));
> + if (len < 0)
> + return;
> +
> + intel_hdmi->write_infoframe(encoder, buffer, len);
> }
> +*/
>
> static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
> struct drm_display_mode *adjusted_mode)
> {
> - struct dip_infoframe avi_if = {
> - .type = DIP_TYPE_AVI,
> - .ver = DIP_VERSION_AVI,
> - .len = DIP_LEN_AVI,
> - };
> + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> + u8 buffer[HDMI_AVI_INFOFRAME_SIZE];
> + struct hdmi_avi_infoframe frame;
> + ssize_t len;
> +
> + len = drm_hdmi_avi_infoframe_from_display_mode(&frame, adjusted_mode);
> + if (len < 0)
> + return;
>
> if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> - avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
> + frame.pixel_repeat = 2;
> +
> + len = hdmi_avi_infoframe_pack(&frame, buffer, sizeof(buffer));
> + if (len < 0)
> + return;
>
> - intel_set_infoframe(encoder, &avi_if);
> + intel_hdmi->write_infoframe(encoder, buffer, len);
> }
>
> static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
> {
> - struct dip_infoframe spd_if;
> + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> + u8 buffer[HDMI_SPD_INFOFRAME_SIZE];
> + struct hdmi_spd_infoframe frame;
> + ssize_t len;
> +
> + len = hdmi_spd_infoframe_init(&frame, "Intel", "Integrated gfx");
> + if (len < 0)
> + return;
> +
> + frame.sdi = HDMI_SPD_SDI_PC;
>
> - memset(&spd_if, 0, sizeof(spd_if));
> - spd_if.type = DIP_TYPE_SPD;
> - spd_if.ver = DIP_VERSION_SPD;
> - spd_if.len = DIP_LEN_SPD;
> - strcpy(spd_if.body.spd.vn, "Intel");
> - strcpy(spd_if.body.spd.pd, "Integrated gfx");
> - spd_if.body.spd.sdi = DIP_SPD_PC;
> + len = hdmi_spd_infoframe_pack(&frame, buffer, sizeof(buffer));
> + if (len < 0)
> + return;
>
> - intel_set_infoframe(encoder, &spd_if);
> + intel_hdmi->write_infoframe(encoder, buffer, len);
> }
>
> static void g4x_set_infoframes(struct drm_encoder *encoder,
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 4b07401..192d791 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -29,6 +29,7 @@
> #include <linux/slab.h>
> #include <linux/delay.h>
> #include <linux/export.h>
> +#include <linux/hdmi.h>
> #include <drm/drmP.h>
> #include <drm/drm_crtc.h>
> #include <drm/drm_edid.h>
> @@ -935,20 +936,17 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo,
>
> static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo)
> {
> - struct dip_infoframe avi_if = {
> - .type = DIP_TYPE_AVI,
> - .ver = DIP_VERSION_AVI,
> - .len = DIP_LEN_AVI,
> - };
> - uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)];
> + uint8_t sdvo_data[HDMI_AVI_INFOFRAME_SIZE];
> + struct hdmi_avi_infoframe frame;
> + ssize_t len;
>
> - intel_dip_infoframe_csum(&avi_if);
> + len = hdmi_avi_infoframe_init(&frame);
> + if (len < 0)
> + return false;
>
> - /* sdvo spec says that the ecc is handled by the hw, and it looks like
> - * we must not send the ecc field, either. */
> - memcpy(sdvo_data, &avi_if, 3);
> - sdvo_data[3] = avi_if.checksum;
> - memcpy(&sdvo_data[4], &avi_if.body, sizeof(avi_if.body.avi));
> + len = hdmi_avi_infoframe_pack(&frame, sdvo_data, sizeof(sdvo_data));
> + if (len < 0)
> + return false;
>
> return intel_sdvo_write_infoframe(intel_sdvo, SDVO_HBUF_INDEX_AVI_IF,
> SDVO_HBUF_TX_VSYNC,
> --
> 1.8.0.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC v2 5/5] drm/i915: Use generic HDMI infoframe helpers
2012-12-06 16:11 ` Paulo Zanoni
@ 2012-12-06 16:55 ` Paulo Zanoni
2012-12-07 7:28 ` Thierry Reding
2012-12-07 7:22 ` Thierry Reding
1 sibling, 1 reply; 30+ messages in thread
From: Paulo Zanoni @ 2012-12-06 16:55 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel
Hi
2012/12/6 Paulo Zanoni <przanoni@gmail.com>:
> Hi
>
> 2012/12/5 Thierry Reding <thierry.reding@avionic-design.de>:
>> Use the generic HDMI infoframe helpers to get rid of the duplicate
>> implementation in the i915 driver.
>
> A few comments:
>
> - I've compiled your patches and now "intel_infoframes -d" tells my my
> infoframes are full of zeroes. So there's a bug somewhere... I have to
> say that the i915 infoframe registers are quite complicated and it
> took me a long time to fix a lot of its problems, so please don't
> break it and read below for a suggestion :)
>
> - Why do we need that kconfig stuff? Why not just put all the code
> inside drivers/gpu/drm?
>
> - I really like having "common code between drivers merged", but I
> really don't see how the i915 driver is benefiting from this change.
> We're just basically complicating intel_hdmi.c to use a new struct
> that doesn't fit our needs due to the lack of the ECC byte. My idea:
> Instead of calling hdmi_avi_infoframe_pack, why don't we just
> implement intel_hdmi_avi_infoframe_pack that converts a "struct
> hdmi_avi_infoframe" into our own good-old "struct dip_infoframe" that
> has the cool ECC value in the right place? This would probably
> drastically reduce your chances of introducing bugs in our code :)
Just to be a little bit more clear about this paragraph since Daniel
asked me a few questions:
For the changes inside intel_hdmi.c, I'd really like your patch to not
touch any of the "write_infoframe" or "set_infoframes" callbacks. I
think you can do everything by just patching intel_set_infoframe,
intel_hdmi_set_avi_infoframe and intel_hdmi_set_spd_infoframe. In one
of those functions you could call something like
"intel_hdmi_infoframe_pack(struct hdmi_avi_infoframe, struct
dip_infoframe)" and then proceed. This way you don 't need to touch
the code that actually writes stuff to the hardware.
Also, it seems intel_sdvo.c can perfectly use the generic struct from
your patch (the one that doesn't contain the ECC value), so maybe we
could even try to move "struct dip_infoframe" to inside intel_hdmi.c,
making sure intel_sdvo.c can't even use it.
>
> - Instead of "converting our own structs into structs that don't
> exactly fit our usage", what I would really like to see is generic
> _optional_ drm functions to help me fill the infoframe data, like the
> things I did in the past. Examples:
> (i) I want a function that reads the EDID and tells me whether the
> monitor is going to respect my "underscan" settings (AVI infoframe
> field S) or not.
> (ii) I want unified overscan/underscan properties between all the
> drivers: we need properties to allow requesting specific
> overscan/underscan properties and also properties to allow the driver
> to force underscan in case the monitor doesn't want to cooperate (as
> far as I remember, radeon has these properties, but we need to have
> the behavior explicitly documented so all the drivers can try to
> implement the exact same thing, and then we also need to ask
> user-space guys to add support for these things in their GUIs).
> (iii) I want a function that helps me properly setting/changing the
> correct pixel and picture aspect ratios. it seems the only difference
> between some modes (with different VICs) is the pixel/aspect ratio,
> how do we expose this to the user-space and let it select the one it
> wants? How does the driver know exactly which one is the selected one?
> (iv) I want code that enables user-space to use those modes with
> variable pixel repetition rates and select exactly the one it wants to
> use with the correct pixel repetition.
> (v) I'd like a quirk list to recognize monitors/devices that don't
> work correctly when they receive infoframes, or when the VIC is 0,
> etc.
> (vi) I want magical functions that help me setting all those
> color-related stuff I did not study yet.
> (vii) Ideally, I'd like to accomplish all the above by looking at
> "struct drm_display_mode" and maybe passing it to helper functions
> that I can _optionally_ use if I want.
> (viii) I don't want the infoframe code to be a "mid layer" that gets
> in the way, I want it to be a set of optional Lego bricks I can use if
> I need.
> (ix) There's certainly more things I'm missing here, but I believe you
> got the idea :)
>
> Of course, implementing all I described will take a few months :)
And to make it clear: the list above doesn't contain any "requirements
to get your code merged", they're just suggestions, since you're
already touching this code and you need to take these problems into
consideration while designing things. We can merge the patch first and
then later worry about fixing the items on the list :)
>
> Thanks,
> Paulo
>
>>
>> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
>> ---
>> drivers/gpu/drm/Kconfig | 2 +
>> drivers/gpu/drm/i915/intel_drv.h | 62 +--------
>> drivers/gpu/drm/i915/intel_hdmi.c | 268 +++++++++++++++++++++-----------------
>> drivers/gpu/drm/i915/intel_sdvo.c | 22 ++--
>> 4 files changed, 159 insertions(+), 195 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 94a4623..5225012 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -144,6 +144,8 @@ config DRM_I915
>> select INPUT if ACPI
>> select ACPI_VIDEO if ACPI
>> select ACPI_BUTTON if ACPI
>> + select DRM_HDMI
>> + select HDMI
>> help
>> Choose this option if you have a system that has "Intel Graphics
>> Media Accelerator" or "HD Graphics" integrated graphics,
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 522061c..70a7440 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -276,63 +276,6 @@ struct cxsr_latency {
>> #define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
>> #define to_intel_plane(x) container_of(x, struct intel_plane, base)
>>
>> -#define DIP_HEADER_SIZE 5
>> -
>> -#define DIP_TYPE_AVI 0x82
>> -#define DIP_VERSION_AVI 0x2
>> -#define DIP_LEN_AVI 13
>> -#define DIP_AVI_PR_1 0
>> -#define DIP_AVI_PR_2 1
>> -
>> -#define DIP_TYPE_SPD 0x83
>> -#define DIP_VERSION_SPD 0x1
>> -#define DIP_LEN_SPD 25
>> -#define DIP_SPD_UNKNOWN 0
>> -#define DIP_SPD_DSTB 0x1
>> -#define DIP_SPD_DVDP 0x2
>> -#define DIP_SPD_DVHS 0x3
>> -#define DIP_SPD_HDDVR 0x4
>> -#define DIP_SPD_DVC 0x5
>> -#define DIP_SPD_DSC 0x6
>> -#define DIP_SPD_VCD 0x7
>> -#define DIP_SPD_GAME 0x8
>> -#define DIP_SPD_PC 0x9
>> -#define DIP_SPD_BD 0xa
>> -#define DIP_SPD_SCD 0xb
>> -
>> -struct dip_infoframe {
>> - uint8_t type; /* HB0 */
>> - uint8_t ver; /* HB1 */
>> - uint8_t len; /* HB2 - body len, not including checksum */
>> - uint8_t ecc; /* Header ECC */
>> - uint8_t checksum; /* PB0 */
>> - union {
>> - struct {
>> - /* PB1 - Y 6:5, A 4:4, B 3:2, S 1:0 */
>> - uint8_t Y_A_B_S;
>> - /* PB2 - C 7:6, M 5:4, R 3:0 */
>> - uint8_t C_M_R;
>> - /* PB3 - ITC 7:7, EC 6:4, Q 3:2, SC 1:0 */
>> - uint8_t ITC_EC_Q_SC;
>> - /* PB4 - VIC 6:0 */
>> - uint8_t VIC;
>> - /* PB5 - YQ 7:6, CN 5:4, PR 3:0 */
>> - uint8_t YQ_CN_PR;
>> - /* PB6 to PB13 */
>> - uint16_t top_bar_end;
>> - uint16_t bottom_bar_start;
>> - uint16_t left_bar_end;
>> - uint16_t right_bar_start;
>> - } __attribute__ ((packed)) avi;
>> - struct {
>> - uint8_t vn[8];
>> - uint8_t pd[16];
>> - uint8_t sdi;
>> - } __attribute__ ((packed)) spd;
>> - uint8_t payload[27];
>> - } __attribute__ ((packed)) body;
>> -} __attribute__((packed));
>> -
>> struct intel_hdmi {
>> u32 sdvox_reg;
>> int ddc_bus;
>> @@ -340,8 +283,8 @@ struct intel_hdmi {
>> bool has_hdmi_sink;
>> bool has_audio;
>> enum hdmi_force_audio force_audio;
>> - void (*write_infoframe)(struct drm_encoder *encoder,
>> - struct dip_infoframe *frame);
>> + void (*write_infoframe)(struct drm_encoder *encoder, const void *frame,
>> + size_t len);
>> void (*set_infoframes)(struct drm_encoder *encoder,
>> struct drm_display_mode *adjusted_mode);
>> };
>> @@ -430,7 +373,6 @@ extern struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
>> extern bool intel_hdmi_mode_fixup(struct drm_encoder *encoder,
>> const struct drm_display_mode *mode,
>> struct drm_display_mode *adjusted_mode);
>> -extern void intel_dip_infoframe_csum(struct dip_infoframe *avi_if);
>> extern bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg,
>> bool is_sdvob);
>> extern void intel_dvo_init(struct drm_device *dev);
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index 5c279b4..a5492b1 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -29,9 +29,11 @@
>> #include <linux/i2c.h>
>> #include <linux/slab.h>
>> #include <linux/delay.h>
>> +#include <linux/hdmi.h>
>> #include <drm/drmP.h>
>> #include <drm/drm_crtc.h>
>> #include <drm/drm_edid.h>
>> +#include <drm/drm_hdmi.h>
>> #include "intel_drv.h"
>> #include <drm/i915_drm.h>
>> #include "i915_drv.h"
>> @@ -66,102 +68,112 @@ static struct intel_hdmi *intel_attached_hdmi(struct drm_connector *connector)
>> return enc_to_intel_hdmi(&intel_attached_encoder(connector)->base);
>> }
>>
>> -void intel_dip_infoframe_csum(struct dip_infoframe *frame)
>> +static u32 g4x_infoframe_index(enum hdmi_infoframe_type type)
>> {
>> - uint8_t *data = (uint8_t *)frame;
>> - uint8_t sum = 0;
>> - unsigned i;
>> -
>> - frame->checksum = 0;
>> - frame->ecc = 0;
>> -
>> - for (i = 0; i < frame->len + DIP_HEADER_SIZE; i++)
>> - sum += data[i];
>> -
>> - frame->checksum = 0x100 - sum;
>> -}
>> -
>> -static u32 g4x_infoframe_index(struct dip_infoframe *frame)
>> -{
>> - switch (frame->type) {
>> - case DIP_TYPE_AVI:
>> + switch (type) {
>> + case HDMI_INFOFRAME_TYPE_AVI:
>> return VIDEO_DIP_SELECT_AVI;
>> - case DIP_TYPE_SPD:
>> + case HDMI_INFOFRAME_TYPE_SPD:
>> return VIDEO_DIP_SELECT_SPD;
>> default:
>> - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
>> + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
>> return 0;
>> }
>> }
>>
>> -static u32 g4x_infoframe_enable(struct dip_infoframe *frame)
>> +static u32 g4x_infoframe_enable(enum hdmi_infoframe_type type)
>> {
>> - switch (frame->type) {
>> - case DIP_TYPE_AVI:
>> + switch (type) {
>> + case HDMI_INFOFRAME_TYPE_AVI:
>> return VIDEO_DIP_ENABLE_AVI;
>> - case DIP_TYPE_SPD:
>> + case HDMI_INFOFRAME_TYPE_SPD:
>> return VIDEO_DIP_ENABLE_SPD;
>> default:
>> - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
>> + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
>> return 0;
>> }
>> }
>>
>> -static u32 hsw_infoframe_enable(struct dip_infoframe *frame)
>> +static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type)
>> {
>> - switch (frame->type) {
>> - case DIP_TYPE_AVI:
>> + switch (type) {
>> + case HDMI_INFOFRAME_TYPE_AVI:
>> return VIDEO_DIP_ENABLE_AVI_HSW;
>> - case DIP_TYPE_SPD:
>> + case HDMI_INFOFRAME_TYPE_SPD:
>> return VIDEO_DIP_ENABLE_SPD_HSW;
>> default:
>> - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
>> + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
>> return 0;
>> }
>> }
>>
>> -static u32 hsw_infoframe_data_reg(struct dip_infoframe *frame, enum pipe pipe)
>> +static u32 hsw_infoframe_data_reg(enum hdmi_infoframe_type type, enum pipe pipe)
>> {
>> - switch (frame->type) {
>> - case DIP_TYPE_AVI:
>> + switch (type) {
>> + case HDMI_INFOFRAME_TYPE_AVI:
>> return HSW_TVIDEO_DIP_AVI_DATA(pipe);
>> - case DIP_TYPE_SPD:
>> + case HDMI_INFOFRAME_TYPE_SPD:
>> return HSW_TVIDEO_DIP_SPD_DATA(pipe);
>> default:
>> - DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
>> + DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
>> return 0;
>> }
>> }
>>
>> -static void g4x_write_infoframe(struct drm_encoder *encoder,
>> - struct dip_infoframe *frame)
>> +static u32 intel_dip_pack(const u8 *data, size_t size)
>> {
>> - uint32_t *data = (uint32_t *)frame;
>> + u32 value = 0;
>> + size_t i;
>> +
>> + for (i = size; i > 0; i--)
>> + value = (value << 8) | data[i - 1];
>> +
>> + return value;
>> +}
>> +
>> +static void g4x_dip_write(struct drm_i915_private *dev_priv,
>> + unsigned long offset, const void *dip, size_t len)
>> +{
>> + u32 data;
>> + size_t i;
>> +
>> + /* Write infoframe header and zero out ECC at byte 4 */
>> + data = intel_dip_pack(dip, 3);
>> + I915_WRITE(offset, data);
>> +
>> + for (i = 3; i < len; i += 4) {
>> + size_t rem = min_t(size_t, len - i, 4);
>> + data = intel_dip_pack(dip + i, rem);
>> + I915_WRITE(offset, data);
>> + }
>> +
>> + /* Write every possible data byte to force correct ECC calculation. */
>> + for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
>> + I915_WRITE(offset, 0);
>> +}
>> +
>> +static void g4x_write_infoframe(struct drm_encoder *encoder, const void *frame,
>> + size_t len)
>> +{
>> + enum hdmi_infoframe_type type = ((const u8 *)frame)[0];
>> struct drm_device *dev = encoder->dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> u32 val = I915_READ(VIDEO_DIP_CTL);
>> - unsigned i, len = DIP_HEADER_SIZE + frame->len;
>>
>> WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
>>
>> val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
>> - val |= g4x_infoframe_index(frame);
>> + val |= g4x_infoframe_index(type);
>>
>> - val &= ~g4x_infoframe_enable(frame);
>> + val &= ~g4x_infoframe_enable(type);
>>
>> I915_WRITE(VIDEO_DIP_CTL, val);
>>
>> mmiowb();
>> - for (i = 0; i < len; i += 4) {
>> - I915_WRITE(VIDEO_DIP_DATA, *data);
>> - data++;
>> - }
>> - /* Write every possible data byte to force correct ECC calculation. */
>> - for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
>> - I915_WRITE(VIDEO_DIP_DATA, 0);
>> + g4x_dip_write(dev_priv, VIDEO_DIP_DATA, frame, len);
>> mmiowb();
>>
>> - val |= g4x_infoframe_enable(frame);
>> + val |= g4x_infoframe_enable(type);
>> val &= ~VIDEO_DIP_FREQ_MASK;
>> val |= VIDEO_DIP_FREQ_VSYNC;
>>
>> @@ -169,37 +181,31 @@ static void g4x_write_infoframe(struct drm_encoder *encoder,
>> POSTING_READ(VIDEO_DIP_CTL);
>> }
>>
>> -static void ibx_write_infoframe(struct drm_encoder *encoder,
>> - struct dip_infoframe *frame)
>> +static void ibx_write_infoframe(struct drm_encoder *encoder, const void *frame,
>> + size_t len)
>> {
>> - uint32_t *data = (uint32_t *)frame;
>> + enum hdmi_infoframe_type type = ((const u8 *)frame)[0];
>> struct drm_device *dev = encoder->dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
>> int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
>> - unsigned i, len = DIP_HEADER_SIZE + frame->len;
>> u32 val = I915_READ(reg);
>>
>> WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
>>
>> val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
>> - val |= g4x_infoframe_index(frame);
>> + val |= g4x_infoframe_index(type);
>>
>> - val &= ~g4x_infoframe_enable(frame);
>> + val &= ~g4x_infoframe_enable(type);
>>
>> I915_WRITE(reg, val);
>>
>> mmiowb();
>> - for (i = 0; i < len; i += 4) {
>> - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
>> - data++;
>> - }
>> - /* Write every possible data byte to force correct ECC calculation. */
>> - for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
>> - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0);
>> + g4x_dip_write(dev_priv, TVIDEO_DIP_DATA(intel_crtc->pipe), frame,
>> + len);
>> mmiowb();
>>
>> - val |= g4x_infoframe_enable(frame);
>> + val |= g4x_infoframe_enable(type);
>> val &= ~VIDEO_DIP_FREQ_MASK;
>> val |= VIDEO_DIP_FREQ_VSYNC;
>>
>> @@ -207,40 +213,33 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
>> POSTING_READ(reg);
>> }
>>
>> -static void cpt_write_infoframe(struct drm_encoder *encoder,
>> - struct dip_infoframe *frame)
>> +static void cpt_write_infoframe(struct drm_encoder *encoder, const void *frame,
>> + size_t len)
>> {
>> - uint32_t *data = (uint32_t *)frame;
>> + enum hdmi_infoframe_type type = ((const u8 *)frame)[0];
>> struct drm_device *dev = encoder->dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
>> int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
>> - unsigned i, len = DIP_HEADER_SIZE + frame->len;
>> u32 val = I915_READ(reg);
>>
>> WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
>>
>> val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
>> - val |= g4x_infoframe_index(frame);
>> + val |= g4x_infoframe_index(type);
>>
>> /* The DIP control register spec says that we need to update the AVI
>> * infoframe without clearing its enable bit */
>> - if (frame->type != DIP_TYPE_AVI)
>> - val &= ~g4x_infoframe_enable(frame);
>> + if (type != HDMI_INFOFRAME_TYPE_AVI)
>> + val &= ~g4x_infoframe_enable(type);
>>
>> I915_WRITE(reg, val);
>>
>> mmiowb();
>> - for (i = 0; i < len; i += 4) {
>> - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
>> - data++;
>> - }
>> - /* Write every possible data byte to force correct ECC calculation. */
>> - for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
>> - I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0);
>> + g4x_dip_write(dev_priv, TVIDEO_DIP_DATA(intel_crtc->pipe), frame, len);
>> mmiowb();
>>
>> - val |= g4x_infoframe_enable(frame);
>> + val |= g4x_infoframe_enable(type);
>> val &= ~VIDEO_DIP_FREQ_MASK;
>> val |= VIDEO_DIP_FREQ_VSYNC;
>>
>> @@ -248,37 +247,31 @@ static void cpt_write_infoframe(struct drm_encoder *encoder,
>> POSTING_READ(reg);
>> }
>>
>> -static void vlv_write_infoframe(struct drm_encoder *encoder,
>> - struct dip_infoframe *frame)
>> +static void vlv_write_infoframe(struct drm_encoder *encoder, const void *frame,
>> + size_t len)
>> {
>> - uint32_t *data = (uint32_t *)frame;
>> + enum hdmi_infoframe_type type = ((const u8 *)frame)[0];
>> struct drm_device *dev = encoder->dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
>> int reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
>> - unsigned i, len = DIP_HEADER_SIZE + frame->len;
>> u32 val = I915_READ(reg);
>>
>> WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
>>
>> val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
>> - val |= g4x_infoframe_index(frame);
>> + val |= g4x_infoframe_index(type);
>>
>> - val &= ~g4x_infoframe_enable(frame);
>> + val &= ~g4x_infoframe_enable(type);
>>
>> I915_WRITE(reg, val);
>>
>> mmiowb();
>> - for (i = 0; i < len; i += 4) {
>> - I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
>> - data++;
>> - }
>> - /* Write every possible data byte to force correct ECC calculation. */
>> - for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
>> - I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), 0);
>> + g4x_dip_write(dev_priv, VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), frame,
>> + len);
>> mmiowb();
>>
>> - val |= g4x_infoframe_enable(frame);
>> + val |= g4x_infoframe_enable(type);
>> val &= ~VIDEO_DIP_FREQ_MASK;
>> val |= VIDEO_DIP_FREQ_VSYNC;
>>
>> @@ -286,76 +279,105 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
>> POSTING_READ(reg);
>> }
>>
>> -static void hsw_write_infoframe(struct drm_encoder *encoder,
>> - struct dip_infoframe *frame)
>> +static void hsw_write_infoframe(struct drm_encoder *encoder, const void *frame,
>> + size_t len)
>> {
>> - uint32_t *data = (uint32_t *)frame;
>> + enum hdmi_infoframe_type type = ((const u8 *)frame)[0];
>> struct drm_device *dev = encoder->dev;
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
>> u32 ctl_reg = HSW_TVIDEO_DIP_CTL(intel_crtc->pipe);
>> - u32 data_reg = hsw_infoframe_data_reg(frame, intel_crtc->pipe);
>> - unsigned int i, len = DIP_HEADER_SIZE + frame->len;
>> + u32 data_reg = hsw_infoframe_data_reg(type, intel_crtc->pipe);
>> u32 val = I915_READ(ctl_reg);
>> + unsigned long offset;
>> + u32 data;
>> + size_t i;
>>
>> if (data_reg == 0)
>> return;
>>
>> - val &= ~hsw_infoframe_enable(frame);
>> + val &= ~hsw_infoframe_enable(type);
>> I915_WRITE(ctl_reg, val);
>>
>> mmiowb();
>> - for (i = 0; i < len; i += 4) {
>> - I915_WRITE(data_reg + i, *data);
>> - data++;
>> +
>> + /* Write infoframe header and zero out ECC at byte 4 */
>> + data = intel_dip_pack(frame, 3);
>> + I915_WRITE(data_reg, data);
>> +
>> + for (i = 3, offset = 4; i < len; i += 4, offset += 4) {
>> + size_t rem = min_t(size_t, len - i, 4);
>> + data = intel_dip_pack(frame + i, rem);
>> + I915_WRITE(data_reg + offset, data);
>> }
>> +
>> /* Write every possible data byte to force correct ECC calculation. */
>> - for (; i < VIDEO_DIP_DATA_SIZE; i += 4)
>> - I915_WRITE(data_reg + i, 0);
>> + for (; i < VIDEO_DIP_DATA_SIZE; i += 4, offset += 4)
>> + I915_WRITE(data_reg + offset, 0);
>> +
>> mmiowb();
>>
>> - val |= hsw_infoframe_enable(frame);
>> + val |= hsw_infoframe_enable(type);
>> I915_WRITE(ctl_reg, val);
>> POSTING_READ(ctl_reg);
>> }
>>
>> +/*
>> static void intel_set_infoframe(struct drm_encoder *encoder,
>> - struct dip_infoframe *frame)
>> + const struct hdmi_infoframe *frame)
>> {
>> struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>> + u8 buffer[32];
>> + ssize_t len;
>>
>> - intel_dip_infoframe_csum(frame);
>> - intel_hdmi->write_infoframe(encoder, frame);
>> + len = hdmi_infoframe_pack(frame, buffer, sizeof(buffer));
>> + if (len < 0)
>> + return;
>> +
>> + intel_hdmi->write_infoframe(encoder, buffer, len);
>> }
>> +*/
>>
>> static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>> struct drm_display_mode *adjusted_mode)
>> {
>> - struct dip_infoframe avi_if = {
>> - .type = DIP_TYPE_AVI,
>> - .ver = DIP_VERSION_AVI,
>> - .len = DIP_LEN_AVI,
>> - };
>> + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>> + u8 buffer[HDMI_AVI_INFOFRAME_SIZE];
>> + struct hdmi_avi_infoframe frame;
>> + ssize_t len;
>> +
>> + len = drm_hdmi_avi_infoframe_from_display_mode(&frame, adjusted_mode);
>> + if (len < 0)
>> + return;
>>
>> if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
>> - avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
>> + frame.pixel_repeat = 2;
>> +
>> + len = hdmi_avi_infoframe_pack(&frame, buffer, sizeof(buffer));
>> + if (len < 0)
>> + return;
>>
>> - intel_set_infoframe(encoder, &avi_if);
>> + intel_hdmi->write_infoframe(encoder, buffer, len);
>> }
>>
>> static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
>> {
>> - struct dip_infoframe spd_if;
>> + struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>> + u8 buffer[HDMI_SPD_INFOFRAME_SIZE];
>> + struct hdmi_spd_infoframe frame;
>> + ssize_t len;
>> +
>> + len = hdmi_spd_infoframe_init(&frame, "Intel", "Integrated gfx");
>> + if (len < 0)
>> + return;
>> +
>> + frame.sdi = HDMI_SPD_SDI_PC;
>>
>> - memset(&spd_if, 0, sizeof(spd_if));
>> - spd_if.type = DIP_TYPE_SPD;
>> - spd_if.ver = DIP_VERSION_SPD;
>> - spd_if.len = DIP_LEN_SPD;
>> - strcpy(spd_if.body.spd.vn, "Intel");
>> - strcpy(spd_if.body.spd.pd, "Integrated gfx");
>> - spd_if.body.spd.sdi = DIP_SPD_PC;
>> + len = hdmi_spd_infoframe_pack(&frame, buffer, sizeof(buffer));
>> + if (len < 0)
>> + return;
>>
>> - intel_set_infoframe(encoder, &spd_if);
>> + intel_hdmi->write_infoframe(encoder, buffer, len);
>> }
>>
>> static void g4x_set_infoframes(struct drm_encoder *encoder,
>> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
>> index 4b07401..192d791 100644
>> --- a/drivers/gpu/drm/i915/intel_sdvo.c
>> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
>> @@ -29,6 +29,7 @@
>> #include <linux/slab.h>
>> #include <linux/delay.h>
>> #include <linux/export.h>
>> +#include <linux/hdmi.h>
>> #include <drm/drmP.h>
>> #include <drm/drm_crtc.h>
>> #include <drm/drm_edid.h>
>> @@ -935,20 +936,17 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo,
>>
>> static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo)
>> {
>> - struct dip_infoframe avi_if = {
>> - .type = DIP_TYPE_AVI,
>> - .ver = DIP_VERSION_AVI,
>> - .len = DIP_LEN_AVI,
>> - };
>> - uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)];
>> + uint8_t sdvo_data[HDMI_AVI_INFOFRAME_SIZE];
>> + struct hdmi_avi_infoframe frame;
>> + ssize_t len;
>>
>> - intel_dip_infoframe_csum(&avi_if);
>> + len = hdmi_avi_infoframe_init(&frame);
>> + if (len < 0)
>> + return false;
>>
>> - /* sdvo spec says that the ecc is handled by the hw, and it looks like
>> - * we must not send the ecc field, either. */
>> - memcpy(sdvo_data, &avi_if, 3);
>> - sdvo_data[3] = avi_if.checksum;
>> - memcpy(&sdvo_data[4], &avi_if.body, sizeof(avi_if.body.avi));
>> + len = hdmi_avi_infoframe_pack(&frame, sdvo_data, sizeof(sdvo_data));
>> + if (len < 0)
>> + return false;
>>
>> return intel_sdvo_write_infoframe(intel_sdvo, SDVO_HBUF_INDEX_AVI_IF,
>> SDVO_HBUF_TX_VSYNC,
>> --
>> 1.8.0.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Paulo Zanoni
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [RFC v2 5/5] drm/i915: Use generic HDMI infoframe helpers
2012-12-06 16:55 ` Paulo Zanoni
@ 2012-12-07 7:28 ` Thierry Reding
2012-12-07 8:30 ` Daniel Vetter
0 siblings, 1 reply; 30+ messages in thread
From: Thierry Reding @ 2012-12-07 7:28 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 2370 bytes --]
On Thu, Dec 06, 2012 at 02:55:23PM -0200, Paulo Zanoni wrote:
> Hi
>
> 2012/12/6 Paulo Zanoni <przanoni@gmail.com>:
> > Hi
> >
> > 2012/12/5 Thierry Reding <thierry.reding@avionic-design.de>:
> >> Use the generic HDMI infoframe helpers to get rid of the duplicate
> >> implementation in the i915 driver.
> >
> > A few comments:
> >
> > - I've compiled your patches and now "intel_infoframes -d" tells my my
> > infoframes are full of zeroes. So there's a bug somewhere... I have to
> > say that the i915 infoframe registers are quite complicated and it
> > took me a long time to fix a lot of its problems, so please don't
> > break it and read below for a suggestion :)
> >
> > - Why do we need that kconfig stuff? Why not just put all the code
> > inside drivers/gpu/drm?
> >
> > - I really like having "common code between drivers merged", but I
> > really don't see how the i915 driver is benefiting from this change.
> > We're just basically complicating intel_hdmi.c to use a new struct
> > that doesn't fit our needs due to the lack of the ECC byte. My idea:
> > Instead of calling hdmi_avi_infoframe_pack, why don't we just
> > implement intel_hdmi_avi_infoframe_pack that converts a "struct
> > hdmi_avi_infoframe" into our own good-old "struct dip_infoframe" that
> > has the cool ECC value in the right place? This would probably
> > drastically reduce your chances of introducing bugs in our code :)
>
> Just to be a little bit more clear about this paragraph since Daniel
> asked me a few questions:
>
> For the changes inside intel_hdmi.c, I'd really like your patch to not
> touch any of the "write_infoframe" or "set_infoframes" callbacks. I
> think you can do everything by just patching intel_set_infoframe,
> intel_hdmi_set_avi_infoframe and intel_hdmi_set_spd_infoframe. In one
> of those functions you could call something like
> "intel_hdmi_infoframe_pack(struct hdmi_avi_infoframe, struct
> dip_infoframe)" and then proceed. This way you don 't need to touch
> the code that actually writes stuff to the hardware.
I think I already explained in my previous mail how IMO this completely
defeats the purpose of this patch series. Furthermore the functions that
write the infoframes to the hardware themselves could use some
refactoring of their own. There is a lot of duplication there.
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC v2 5/5] drm/i915: Use generic HDMI infoframe helpers
2012-12-07 7:28 ` Thierry Reding
@ 2012-12-07 8:30 ` Daniel Vetter
2012-12-07 8:49 ` Thierry Reding
0 siblings, 1 reply; 30+ messages in thread
From: Daniel Vetter @ 2012-12-07 8:30 UTC (permalink / raw)
To: Thierry Reding; +Cc: dri-devel
On Fri, Dec 7, 2012 at 8:28 AM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> On Thu, Dec 06, 2012 at 02:55:23PM -0200, Paulo Zanoni wrote:
>> 2012/12/6 Paulo Zanoni <przanoni@gmail.com>:
>> For the changes inside intel_hdmi.c, I'd really like your patch to not
>> touch any of the "write_infoframe" or "set_infoframes" callbacks. I
>> think you can do everything by just patching intel_set_infoframe,
>> intel_hdmi_set_avi_infoframe and intel_hdmi_set_spd_infoframe. In one
>> of those functions you could call something like
>> "intel_hdmi_infoframe_pack(struct hdmi_avi_infoframe, struct
>> dip_infoframe)" and then proceed. This way you don 't need to touch
>> the code that actually writes stuff to the hardware.
>
> I think I already explained in my previous mail how IMO this completely
> defeats the purpose of this patch series. Furthermore the functions that
> write the infoframes to the hardware themselves could use some
> refactoring of their own. There is a lot of duplication there.
I agree with Paulo that we shouldn't touch the hw interfacing
functions - we've had too many bugs and regressions in there, so
burned child et al applies. Now I also disagreed with Paulo's idea to
keep around the infoframe stuff completed for intel_hdmi.c - common
frameworks for sink handling are useful, if only that if forces people
to yell at each another and discovered that way how utterly broken
real world hw is ;-)
I think the idea Paulo proposed in the first reply of keeping our own
infoframe packing code, but using the new data structures to construct
it is worse than the current code: Other people will extend the
helpers, but totally forget about the special intel-packing function.
The idea I've discussed a bit with Paulo is to have our own wrapper
function which intel_hdmi_set_spd_infoframe and
intel_hdmi_set_avi_infoframe can call. The wrapper allocates a new
temporary buffer in the common layout without the ECC byte, calls the
common packing function with that temporary bufffer and then copies
things over into the intel layout. That way we can use the common
infoframe construction&packing stuff, without running the risk of
blowing up the dip write functions right away (now that they seem to
actually work).
At least that was my takeaway from the discussion yesterday with
Paulo, and now reading his second mail I see that he doesn't detail
how the intel infoframe packing function could also work. So it reads
like the exact same idea as in the first mail unfortunately. Or Paulo
and I still have a big disagreement, in which case I need to discuss
things a bit more with him about how we could tackle this. Because to
reiterate my point from the beginning, I think common sink handling
helpers are a Good Thing.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC v2 5/5] drm/i915: Use generic HDMI infoframe helpers
2012-12-07 8:30 ` Daniel Vetter
@ 2012-12-07 8:49 ` Thierry Reding
0 siblings, 0 replies; 30+ messages in thread
From: Thierry Reding @ 2012-12-07 8:49 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 3126 bytes --]
On Fri, Dec 07, 2012 at 09:30:34AM +0100, Daniel Vetter wrote:
> On Fri, Dec 7, 2012 at 8:28 AM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > On Thu, Dec 06, 2012 at 02:55:23PM -0200, Paulo Zanoni wrote:
> >> 2012/12/6 Paulo Zanoni <przanoni@gmail.com>:
> >> For the changes inside intel_hdmi.c, I'd really like your patch to not
> >> touch any of the "write_infoframe" or "set_infoframes" callbacks. I
> >> think you can do everything by just patching intel_set_infoframe,
> >> intel_hdmi_set_avi_infoframe and intel_hdmi_set_spd_infoframe. In one
> >> of those functions you could call something like
> >> "intel_hdmi_infoframe_pack(struct hdmi_avi_infoframe, struct
> >> dip_infoframe)" and then proceed. This way you don 't need to touch
> >> the code that actually writes stuff to the hardware.
> >
> > I think I already explained in my previous mail how IMO this completely
> > defeats the purpose of this patch series. Furthermore the functions that
> > write the infoframes to the hardware themselves could use some
> > refactoring of their own. There is a lot of duplication there.
>
> I agree with Paulo that we shouldn't touch the hw interfacing
> functions - we've had too many bugs and regressions in there, so
> burned child et al applies. Now I also disagreed with Paulo's idea to
> keep around the infoframe stuff completed for intel_hdmi.c - common
> frameworks for sink handling are useful, if only that if forces people
> to yell at each another and discovered that way how utterly broken
> real world hw is ;-)
>
> I think the idea Paulo proposed in the first reply of keeping our own
> infoframe packing code, but using the new data structures to construct
> it is worse than the current code: Other people will extend the
> helpers, but totally forget about the special intel-packing function.
> The idea I've discussed a bit with Paulo is to have our own wrapper
> function which intel_hdmi_set_spd_infoframe and
> intel_hdmi_set_avi_infoframe can call. The wrapper allocates a new
> temporary buffer in the common layout without the ECC byte, calls the
> common packing function with that temporary bufffer and then copies
> things over into the intel layout. That way we can use the common
> infoframe construction&packing stuff, without running the risk of
> blowing up the dip write functions right away (now that they seem to
> actually work).
So looking at the differences between the standard HDMI infoframe and
the dip infoframe structures, this would mean copying 3 bytes of header,
add a 0 byte, then copying the remaining bytes. I think all of this can
be done much more easily when writing to the hardware.
Maybe it would help to do something similar to what I did on Tegra to
validate that the same infoframe ends up in the registers as for the old
code. I used a #ifdef HDMI_GENERIC to easily switch between both code
paths and added some debug output to the register writes so that I could
compare both at the register level. If we do the same for Intel hardware
we should be able to fix this properly.
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC v2 5/5] drm/i915: Use generic HDMI infoframe helpers
2012-12-06 16:11 ` Paulo Zanoni
2012-12-06 16:55 ` Paulo Zanoni
@ 2012-12-07 7:22 ` Thierry Reding
1 sibling, 0 replies; 30+ messages in thread
From: Thierry Reding @ 2012-12-07 7:22 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 5870 bytes --]
On Thu, Dec 06, 2012 at 02:11:00PM -0200, Paulo Zanoni wrote:
> Hi
>
> 2012/12/5 Thierry Reding <thierry.reding@avionic-design.de>:
> > Use the generic HDMI infoframe helpers to get rid of the duplicate
> > implementation in the i915 driver.
>
> A few comments:
>
> - I've compiled your patches and now "intel_infoframes -d" tells my my
> infoframes are full of zeroes. So there's a bug somewhere... I have to
> say that the i915 infoframe registers are quite complicated and it
> took me a long time to fix a lot of its problems, so please don't
> break it and read below for a suggestion :)
I don't know this intel_infoframes utility, where can I get it from?
> - Why do we need that kconfig stuff? Why not just put all the code
> inside drivers/gpu/drm?
The reason for this is that it was suggested to make these helpers
generic and not DRM specific in order to allow them to be shared with
other subsystems such as V4L.
Daniel already suggested to move the DRM helper into drm_edid.c to get
rid of the additional Kconfig option.
> - I really like having "common code between drivers merged", but I
> really don't see how the i915 driver is benefiting from this change.
> We're just basically complicating intel_hdmi.c to use a new struct
> that doesn't fit our needs due to the lack of the ECC byte. My idea:
> Instead of calling hdmi_avi_infoframe_pack, why don't we just
> implement intel_hdmi_avi_infoframe_pack that converts a "struct
> hdmi_avi_infoframe" into our own good-old "struct dip_infoframe" that
> has the cool ECC value in the right place? This would probably
> drastically reduce your chances of introducing bugs in our code :)
That would completely defeat the purpose of this patch series. The goal
was to unify all implementations and only keep the hardware specifics
in the drivers. Clearly the ECC byte is specific to the i915 hardware
and not part of the HDMI specification.
I don't see how any of the i915 code is complicated by this change. The
only complicated thing about it is handling the ECC byte properly. How
about we try to fix things so that i915 works properly with this series
instead of doing some partial conversion?
> - Instead of "converting our own structs into structs that don't
> exactly fit our usage", what I would really like to see is generic
> _optional_ drm functions to help me fill the infoframe data, like the
> things I did in the past. Examples:
But that's precisely what the DRM helper function is supposed to do. It
is very natural to do this with a generic structure as well. The point
being that this is defined by the HDMI specification, so compliant
hardware needs to implement this in some way or another. If Intel chips
require an extra ECC byte to be transmitted as part of the infoframe
header, then that's something the Intel driver should take care of. The
rest of the code can be shared between all implementations.
> (i) I want a function that reads the EDID and tells me whether the
> monitor is going to respect my "underscan" settings (AVI infoframe
> field S) or not.
> (ii) I want unified overscan/underscan properties between all the
> drivers: we need properties to allow requesting specific
> overscan/underscan properties and also properties to allow the driver
> to force underscan in case the monitor doesn't want to cooperate (as
> far as I remember, radeon has these properties, but we need to have
> the behavior explicitly documented so all the drivers can try to
> implement the exact same thing, and then we also need to ask
> user-space guys to add support for these things in their GUIs).
> (iii) I want a function that helps me properly setting/changing the
> correct pixel and picture aspect ratios. it seems the only difference
> between some modes (with different VICs) is the pixel/aspect ratio,
> how do we expose this to the user-space and let it select the one it
> wants? How does the driver know exactly which one is the selected one?
> (iv) I want code that enables user-space to use those modes with
> variable pixel repetition rates and select exactly the one it wants to
> use with the correct pixel repetition.
> (v) I'd like a quirk list to recognize monitors/devices that don't
> work correctly when they receive infoframes, or when the VIC is 0,
> etc.
> (vi) I want magical functions that help me setting all those
> color-related stuff I did not study yet.
Most of the above require interfaces that we don't have.
> (vii) Ideally, I'd like to accomplish all the above by looking at
> "struct drm_display_mode" and maybe passing it to helper functions
> that I can _optionally_ use if I want.
That's what the DRM helper is used for. I don't see how breaking this up
into smaller pieces would be useful. This is data that is transmitted to
HDMI equipment, so ideally the data would be the same independent of the
HDMI source. Having each driver repeat the same sequence to fill in the
same fields in different structures is exactly what this patchset tries
to fix.
> (viii) I don't want the infoframe code to be a "mid layer" that gets
> in the way, I want it to be a set of optional Lego bricks I can use if
> I need.
That's precisely the reason why there's a split between filling the
infoframe structures and packing them. Filling in the data can be done
either using the DRM helper, or drivers can choose to fill it themselves
even though I don't see the point as I already explained above.
Packing on the other hand is a very generic operation and none of the
drivers in the kernel need to pack the data in any different way. Some
hardware may required the packed data to be written to the registers in
some slighly different ways, but as I also already explained I think
that kind of code belongs in the drivers.
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
^ permalink raw reply [flat|nested] 30+ messages in thread
* [RFC] drm/i915: Use generic HDMI infoframe helpers
2012-12-05 16:45 ` [RFC v2 5/5] drm/i915: " Thierry Reding
2012-12-06 14:16 ` Daniel Vetter
2012-12-06 16:11 ` Paulo Zanoni
@ 2012-12-07 15:11 ` Paulo Zanoni
2012-12-07 15:32 ` Daniel Vetter
2012-12-11 8:35 ` Thierry Reding
2 siblings, 2 replies; 30+ messages in thread
From: Paulo Zanoni @ 2012-12-07 15:11 UTC (permalink / raw)
To: dri-devel; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Use the generic HDMI infoframe helpers to get rid of the duplicate
implementation in the i915 driver.
This patch is based on the initial patch by Thierry Reding, but with a
different approach.
TODO:
- The SDVO part is totally untested. I am not sure if the buffer
size on the SDVO code must be a multiple of 4.
- The HDMI part was tested only on SNB/CPT.
- The patch is forcing pixel_repeat to 1 so I can properly test the
patch. Remove this before the final version.
- The correctnes of this patch depends on a fix on patch "video: add
Generic HDMI infoframe helpers"
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/Kconfig | 2 +
drivers/gpu/drm/i915/intel_drv.h | 62 +-----------
drivers/gpu/drm/i915/intel_hdmi.c | 189 ++++++++++++++++++++-----------------
drivers/gpu/drm/i915/intel_sdvo.c | 21 ++---
4 files changed, 117 insertions(+), 157 deletions(-)
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 94a4623..5225012 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -144,6 +144,8 @@ config DRM_I915
select INPUT if ACPI
select ACPI_VIDEO if ACPI
select ACPI_BUTTON if ACPI
+ select DRM_HDMI
+ select HDMI
help
Choose this option if you have a system that has "Intel Graphics
Media Accelerator" or "HD Graphics" integrated graphics,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 522061c..2712396 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -26,6 +26,7 @@
#define __INTEL_DRV_H__
#include <linux/i2c.h>
+#include <linux/hdmi.h>
#include <drm/i915_drm.h>
#include "i915_drv.h"
#include <drm/drm_crtc.h>
@@ -276,63 +277,6 @@ struct cxsr_latency {
#define to_intel_framebuffer(x) container_of(x, struct intel_framebuffer, base)
#define to_intel_plane(x) container_of(x, struct intel_plane, base)
-#define DIP_HEADER_SIZE 5
-
-#define DIP_TYPE_AVI 0x82
-#define DIP_VERSION_AVI 0x2
-#define DIP_LEN_AVI 13
-#define DIP_AVI_PR_1 0
-#define DIP_AVI_PR_2 1
-
-#define DIP_TYPE_SPD 0x83
-#define DIP_VERSION_SPD 0x1
-#define DIP_LEN_SPD 25
-#define DIP_SPD_UNKNOWN 0
-#define DIP_SPD_DSTB 0x1
-#define DIP_SPD_DVDP 0x2
-#define DIP_SPD_DVHS 0x3
-#define DIP_SPD_HDDVR 0x4
-#define DIP_SPD_DVC 0x5
-#define DIP_SPD_DSC 0x6
-#define DIP_SPD_VCD 0x7
-#define DIP_SPD_GAME 0x8
-#define DIP_SPD_PC 0x9
-#define DIP_SPD_BD 0xa
-#define DIP_SPD_SCD 0xb
-
-struct dip_infoframe {
- uint8_t type; /* HB0 */
- uint8_t ver; /* HB1 */
- uint8_t len; /* HB2 - body len, not including checksum */
- uint8_t ecc; /* Header ECC */
- uint8_t checksum; /* PB0 */
- union {
- struct {
- /* PB1 - Y 6:5, A 4:4, B 3:2, S 1:0 */
- uint8_t Y_A_B_S;
- /* PB2 - C 7:6, M 5:4, R 3:0 */
- uint8_t C_M_R;
- /* PB3 - ITC 7:7, EC 6:4, Q 3:2, SC 1:0 */
- uint8_t ITC_EC_Q_SC;
- /* PB4 - VIC 6:0 */
- uint8_t VIC;
- /* PB5 - YQ 7:6, CN 5:4, PR 3:0 */
- uint8_t YQ_CN_PR;
- /* PB6 to PB13 */
- uint16_t top_bar_end;
- uint16_t bottom_bar_start;
- uint16_t left_bar_end;
- uint16_t right_bar_start;
- } __attribute__ ((packed)) avi;
- struct {
- uint8_t vn[8];
- uint8_t pd[16];
- uint8_t sdi;
- } __attribute__ ((packed)) spd;
- uint8_t payload[27];
- } __attribute__ ((packed)) body;
-} __attribute__((packed));
-
struct intel_hdmi {
u32 sdvox_reg;
int ddc_bus;
@@ -341,7 +285,8 @@ struct intel_hdmi {
bool has_audio;
enum hdmi_force_audio force_audio;
void (*write_infoframe)(struct drm_encoder *encoder,
- struct dip_infoframe *frame);
+ uint8_t *frame, ssize_t len,
+ enum hdmi_infoframe_type type);
void (*set_infoframes)(struct drm_encoder *encoder,
struct drm_display_mode *adjusted_mode);
};
@@ -430,7 +375,6 @@ extern struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
extern bool intel_hdmi_mode_fixup(struct drm_encoder *encoder,
const struct drm_display_mode *mode,
struct drm_display_mode *adjusted_mode);
-extern void intel_dip_infoframe_csum(struct dip_infoframe *avi_if);
extern bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg,
bool is_sdvob);
extern void intel_dvo_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 5c279b4..c4cf8f8 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -29,9 +29,11 @@
#include <linux/i2c.h>
#include <linux/slab.h>
#include <linux/delay.h>
+#include <linux/hdmi.h>
#include <drm/drmP.h>
#include <drm/drm_crtc.h>
#include <drm/drm_edid.h>
+#include <drm/drm_hdmi.h>
#include "intel_drv.h"
#include <drm/i915_drm.h>
#include "i915_drv.h"
@@ -66,88 +68,75 @@ static struct intel_hdmi *intel_attached_hdmi(struct drm_connector *connector)
return enc_to_intel_hdmi(&intel_attached_encoder(connector)->base);
}
-void intel_dip_infoframe_csum(struct dip_infoframe *frame)
+static u32 g4x_infoframe_index(enum hdmi_infoframe_type type)
{
- uint8_t *data = (uint8_t *)frame;
- uint8_t sum = 0;
- unsigned i;
-
- frame->checksum = 0;
- frame->ecc = 0;
-
- for (i = 0; i < frame->len + DIP_HEADER_SIZE; i++)
- sum += data[i];
-
- frame->checksum = 0x100 - sum;
-}
-
-static u32 g4x_infoframe_index(struct dip_infoframe *frame)
-{
- switch (frame->type) {
- case DIP_TYPE_AVI:
+ switch (type) {
+ case HDMI_INFOFRAME_TYPE_AVI:
return VIDEO_DIP_SELECT_AVI;
- case DIP_TYPE_SPD:
+ case HDMI_INFOFRAME_TYPE_SPD:
return VIDEO_DIP_SELECT_SPD;
default:
- DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
+ DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
return 0;
}
}
-static u32 g4x_infoframe_enable(struct dip_infoframe *frame)
+static u32 g4x_infoframe_enable(enum hdmi_infoframe_type type)
{
- switch (frame->type) {
- case DIP_TYPE_AVI:
+ switch (type) {
+ case HDMI_INFOFRAME_TYPE_AVI:
return VIDEO_DIP_ENABLE_AVI;
- case DIP_TYPE_SPD:
+ case HDMI_INFOFRAME_TYPE_SPD:
return VIDEO_DIP_ENABLE_SPD;
default:
- DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
+ DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
return 0;
}
}
-static u32 hsw_infoframe_enable(struct dip_infoframe *frame)
+static u32 hsw_infoframe_enable(enum hdmi_infoframe_type type)
{
- switch (frame->type) {
- case DIP_TYPE_AVI:
+ switch (type) {
+ case HDMI_INFOFRAME_TYPE_AVI:
return VIDEO_DIP_ENABLE_AVI_HSW;
- case DIP_TYPE_SPD:
+ case HDMI_INFOFRAME_TYPE_SPD:
return VIDEO_DIP_ENABLE_SPD_HSW;
default:
- DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
+ DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
return 0;
}
}
-static u32 hsw_infoframe_data_reg(struct dip_infoframe *frame, enum pipe pipe)
+static u32 hsw_infoframe_data_reg(enum hdmi_infoframe_type type, enum pipe pipe)
{
- switch (frame->type) {
- case DIP_TYPE_AVI:
+ switch (type) {
+ case HDMI_INFOFRAME_TYPE_AVI:
return HSW_TVIDEO_DIP_AVI_DATA(pipe);
- case DIP_TYPE_SPD:
+ case HDMI_INFOFRAME_TYPE_SPD:
return HSW_TVIDEO_DIP_SPD_DATA(pipe);
default:
- DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
+ DRM_DEBUG_DRIVER("unknown info frame type %d\n", type);
return 0;
}
}
static void g4x_write_infoframe(struct drm_encoder *encoder,
- struct dip_infoframe *frame)
+ uint8_t *frame,
+ ssize_t len,
+ enum hdmi_infoframe_type type)
{
uint32_t *data = (uint32_t *)frame;
struct drm_device *dev = encoder->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
u32 val = I915_READ(VIDEO_DIP_CTL);
- unsigned i, len = DIP_HEADER_SIZE + frame->len;
+ unsigned int i;
WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
- val |= g4x_infoframe_index(frame);
+ val |= g4x_infoframe_index(type);
- val &= ~g4x_infoframe_enable(frame);
+ val &= ~g4x_infoframe_enable(type);
I915_WRITE(VIDEO_DIP_CTL, val);
@@ -161,7 +150,7 @@ static void g4x_write_infoframe(struct drm_encoder *encoder,
I915_WRITE(VIDEO_DIP_DATA, 0);
mmiowb();
- val |= g4x_infoframe_enable(frame);
+ val |= g4x_infoframe_enable(type);
val &= ~VIDEO_DIP_FREQ_MASK;
val |= VIDEO_DIP_FREQ_VSYNC;
@@ -170,22 +159,24 @@ static void g4x_write_infoframe(struct drm_encoder *encoder,
}
static void ibx_write_infoframe(struct drm_encoder *encoder,
- struct dip_infoframe *frame)
+ uint8_t *frame,
+ ssize_t len,
+ enum hdmi_infoframe_type type)
{
uint32_t *data = (uint32_t *)frame;
struct drm_device *dev = encoder->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
- unsigned i, len = DIP_HEADER_SIZE + frame->len;
+ unsigned int i;
u32 val = I915_READ(reg);
WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
- val |= g4x_infoframe_index(frame);
+ val |= g4x_infoframe_index(type);
- val &= ~g4x_infoframe_enable(frame);
+ val &= ~g4x_infoframe_enable(type);
I915_WRITE(reg, val);
@@ -199,7 +190,7 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0);
mmiowb();
- val |= g4x_infoframe_enable(frame);
+ val |= g4x_infoframe_enable(type);
val &= ~VIDEO_DIP_FREQ_MASK;
val |= VIDEO_DIP_FREQ_VSYNC;
@@ -208,25 +199,27 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
}
static void cpt_write_infoframe(struct drm_encoder *encoder,
- struct dip_infoframe *frame)
+ uint8_t *frame,
+ ssize_t len,
+ enum hdmi_infoframe_type type)
{
uint32_t *data = (uint32_t *)frame;
struct drm_device *dev = encoder->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
- unsigned i, len = DIP_HEADER_SIZE + frame->len;
+ unsigned int i;
u32 val = I915_READ(reg);
WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
- val |= g4x_infoframe_index(frame);
+ val |= g4x_infoframe_index(type);
/* The DIP control register spec says that we need to update the AVI
* infoframe without clearing its enable bit */
- if (frame->type != DIP_TYPE_AVI)
- val &= ~g4x_infoframe_enable(frame);
+ if (type != HDMI_INFOFRAME_TYPE_AVI)
+ val &= ~g4x_infoframe_enable(type);
I915_WRITE(reg, val);
@@ -240,31 +233,34 @@ static void cpt_write_infoframe(struct drm_encoder *encoder,
I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), 0);
mmiowb();
- val |= g4x_infoframe_enable(frame);
+ val |= g4x_infoframe_enable(type);
val &= ~VIDEO_DIP_FREQ_MASK;
val |= VIDEO_DIP_FREQ_VSYNC;
I915_WRITE(reg, val);
POSTING_READ(reg);
+
}
static void vlv_write_infoframe(struct drm_encoder *encoder,
- struct dip_infoframe *frame)
+ uint8_t *frame,
+ ssize_t len,
+ enum hdmi_infoframe_type type)
{
uint32_t *data = (uint32_t *)frame;
struct drm_device *dev = encoder->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
int reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
- unsigned i, len = DIP_HEADER_SIZE + frame->len;
+ unsigned int i;
u32 val = I915_READ(reg);
WARN(!(val & VIDEO_DIP_ENABLE), "Writing DIP with CTL reg disabled\n");
val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
- val |= g4x_infoframe_index(frame);
+ val |= g4x_infoframe_index(type);
- val &= ~g4x_infoframe_enable(frame);
+ val &= ~g4x_infoframe_enable(type);
I915_WRITE(reg, val);
@@ -278,7 +274,7 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), 0);
mmiowb();
- val |= g4x_infoframe_enable(frame);
+ val |= g4x_infoframe_enable(type);
val &= ~VIDEO_DIP_FREQ_MASK;
val |= VIDEO_DIP_FREQ_VSYNC;
@@ -287,21 +283,23 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
}
static void hsw_write_infoframe(struct drm_encoder *encoder,
- struct dip_infoframe *frame)
+ uint8_t *frame,
+ ssize_t len,
+ enum hdmi_infoframe_type type)
{
uint32_t *data = (uint32_t *)frame;
struct drm_device *dev = encoder->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
u32 ctl_reg = HSW_TVIDEO_DIP_CTL(intel_crtc->pipe);
- u32 data_reg = hsw_infoframe_data_reg(frame, intel_crtc->pipe);
- unsigned int i, len = DIP_HEADER_SIZE + frame->len;
+ u32 data_reg = hsw_infoframe_data_reg(type, intel_crtc->pipe);
+ unsigned int i;
u32 val = I915_READ(ctl_reg);
if (data_reg == 0)
return;
- val &= ~hsw_infoframe_enable(frame);
+ val &= ~hsw_infoframe_enable(type);
I915_WRITE(ctl_reg, val);
mmiowb();
@@ -314,48 +312,67 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
I915_WRITE(data_reg + i, 0);
mmiowb();
- val |= hsw_infoframe_enable(frame);
+ val |= hsw_infoframe_enable(type);
I915_WRITE(ctl_reg, val);
POSTING_READ(ctl_reg);
}
-static void intel_set_infoframe(struct drm_encoder *encoder,
- struct dip_infoframe *frame)
+static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
+ struct drm_display_mode *adjusted_mode)
{
struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+ struct hdmi_avi_infoframe frame;
+ uint8_t buffer[20]; /* will be read as uin32_t, so 20 not 18 */
+ ssize_t len;
- intel_dip_infoframe_csum(frame);
- intel_hdmi->write_infoframe(encoder, frame);
-}
+ memset(buffer, 0, sizeof(buffer));
-static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
- struct drm_display_mode *adjusted_mode)
-{
- struct dip_infoframe avi_if = {
- .type = DIP_TYPE_AVI,
- .ver = DIP_VERSION_AVI,
- .len = DIP_LEN_AVI,
- };
+ len = drm_hdmi_avi_infoframe_from_display_mode(&frame, adjusted_mode);
+ WARN(len < 0, "drm_hdmi_avi_infoframe_from_display_mode failed\n");
+
+ /* if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK) helps me test */
+ frame.pixel_repeat = 1;
+
+ len = hdmi_avi_infoframe_pack(&frame, buffer + 1, sizeof(buffer) - 1);
+ WARN(len < 0, "hdmi_avi_infoframe_pack failed\n");
- if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
- avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
+ /* Insert the ECC */
+ buffer[0] = buffer[1];
+ buffer[1] = buffer[2];
+ buffer[2] = buffer[3];
+ buffer[3] = 0;
+ len++;
- intel_set_infoframe(encoder, &avi_if);
+ intel_hdmi->write_infoframe(encoder, buffer, len,
+ HDMI_INFOFRAME_TYPE_AVI);
}
static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
{
- struct dip_infoframe spd_if;
+ struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+ struct hdmi_spd_infoframe frame;
+ uint8_t buffer[32]; /* will be read as uint32_t, so 32, not 30 */
+ ssize_t len;
+
+ memset(buffer, 0, sizeof(buffer));
+
+ len = hdmi_spd_infoframe_init(&frame, "Intel", "Integrated gfx");
+ WARN(len < 0, "hdmi_spd_infoframe_init failed\n");
+
+ frame.sdi = HDMI_SPD_SDI_PC;
+
+ len = hdmi_spd_infoframe_pack(&frame, buffer + 1, sizeof(buffer) - 1);
+ WARN(len < 0, "hdmi_spd_infoframe_pack failed\n");
- memset(&spd_if, 0, sizeof(spd_if));
- spd_if.type = DIP_TYPE_SPD;
- spd_if.ver = DIP_VERSION_SPD;
- spd_if.len = DIP_LEN_SPD;
- strcpy(spd_if.body.spd.vn, "Intel");
- strcpy(spd_if.body.spd.pd, "Integrated gfx");
- spd_if.body.spd.sdi = DIP_SPD_PC;
+ /* Insert the ECC */
+ buffer[0] = buffer[1];
+ buffer[1] = buffer[2];
+ buffer[2] = buffer[3];
+ buffer[3] = 0;
+ len++;
- intel_set_infoframe(encoder, &spd_if);
+ intel_hdmi->write_infoframe(encoder, buffer, len,
+ HDMI_INFOFRAME_TYPE_SPD);
}
static void g4x_set_infoframes(struct drm_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index a4bee83..867eb65 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -29,6 +29,7 @@
#include <linux/slab.h>
#include <linux/delay.h>
#include <linux/export.h>
+#include <linux/hdmi.h>
#include <drm/drmP.h>
#include <drm/drm_crtc.h>
#include <drm/drm_edid.h>
@@ -935,20 +936,16 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo,
static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo)
{
- struct dip_infoframe avi_if = {
- .type = DIP_TYPE_AVI,
- .ver = DIP_VERSION_AVI,
- .len = DIP_LEN_AVI,
- };
- uint8_t sdvo_data[4 + sizeof(avi_if.body.avi)];
+ uint8_t sdvo_data[HDMI_AVI_INFOFRAME_SIZE +
+ HDMI_INFOFRAME_HEADER_SIZE];
+ struct hdmi_avi_infoframe frame;
+ ssize_t len;
- intel_dip_infoframe_csum(&avi_if);
+ len = hdmi_avi_infoframe_init(&frame);
+ WARN(len < 0, "hdmi_avi_infoframe_init failed!\n");
- /* sdvo spec says that the ecc is handled by the hw, and it looks like
- * we must not send the ecc field, either. */
- memcpy(sdvo_data, &avi_if, 3);
- sdvo_data[3] = avi_if.checksum;
- memcpy(&sdvo_data[4], &avi_if.body, sizeof(avi_if.body.avi));
+ len = hdmi_avi_infoframe_pack(&frame, sdvo_data, sizeof(sdvo_data));
+ WARN(len < 0, "hdmi_avi_infoframe_pack failed!\n");
return intel_sdvo_write_infoframe(intel_sdvo, SDVO_HBUF_INDEX_AVI_IF,
SDVO_HBUF_TX_VSYNC,
--
1.7.10.4
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [RFC] drm/i915: Use generic HDMI infoframe helpers
2012-12-07 15:11 ` [RFC] " Paulo Zanoni
@ 2012-12-07 15:32 ` Daniel Vetter
2012-12-11 10:39 ` Daniel Vetter
2012-12-11 8:35 ` Thierry Reding
1 sibling, 1 reply; 30+ messages in thread
From: Daniel Vetter @ 2012-12-07 15:32 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Paulo Zanoni, dri-devel
Hi Paulo,
On Fri, Dec 7, 2012 at 4:11 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Use the generic HDMI infoframe helpers to get rid of the duplicate
> implementation in the i915 driver.
>
> This patch is based on the initial patch by Thierry Reding, but with a
> different approach.
>
> TODO:
> - The SDVO part is totally untested. I am not sure if the buffer
> size on the SDVO code must be a multiple of 4.
> - The HDMI part was tested only on SNB/CPT.
> - The patch is forcing pixel_repeat to 1 so I can properly test the
> patch. Remove this before the final version.
> - The correctnes of this patch depends on a fix on patch "video: add
> Generic HDMI infoframe helpers"
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
I like what this looks like, so I think we have a rough plan for
integrating the proposed hdmi infoframe helpers into i915. Some minor
bikeshedding left, but we can look at this after your big vacation in
2013. For reference my bikeshed:
- Maybe extract the type argument sprinkling into a prep patch, that
way it's even more clear what's changing (and that very little
changes).
+ uint8_t buffer[32]; /* will be read as uint32_t, so 32, not 30 */
- ROUND_UP(30, sizeof(uint32_t)) and maybe use #defines for 30.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC] drm/i915: Use generic HDMI infoframe helpers
2012-12-07 15:32 ` Daniel Vetter
@ 2012-12-11 10:39 ` Daniel Vetter
0 siblings, 0 replies; 30+ messages in thread
From: Daniel Vetter @ 2012-12-11 10:39 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Paulo Zanoni, dri-devel
On Fri, Dec 7, 2012 at 4:32 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> TODO:
>> - The SDVO part is totally untested. I am not sure if the buffer
>> size on the SDVO code must be a multiple of 4.
Forgotten to mention: SDVO actually needs alignment to blocks of 8,
but intel_sdvo_write_infoframe already takes care of all that so no
rounding of the buffer size is needed.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [RFC] drm/i915: Use generic HDMI infoframe helpers
2012-12-07 15:11 ` [RFC] " Paulo Zanoni
2012-12-07 15:32 ` Daniel Vetter
@ 2012-12-11 8:35 ` Thierry Reding
1 sibling, 0 replies; 30+ messages in thread
From: Thierry Reding @ 2012-12-11 8:35 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Paulo Zanoni, dri-devel
[-- Attachment #1.1: Type: text/plain, Size: 1063 bytes --]
On Fri, Dec 07, 2012 at 01:11:29PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Use the generic HDMI infoframe helpers to get rid of the duplicate
> implementation in the i915 driver.
>
> This patch is based on the initial patch by Thierry Reding, but with a
> different approach.
>
> TODO:
> - The SDVO part is totally untested. I am not sure if the buffer
> size on the SDVO code must be a multiple of 4.
> - The HDMI part was tested only on SNB/CPT.
> - The patch is forcing pixel_repeat to 1 so I can properly test the
> patch. Remove this before the final version.
> - The correctnes of this patch depends on a fix on patch "video: add
> Generic HDMI infoframe helpers"
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Hi Paulo,
This looks good to me in general. I still think that writing the
infoframe to the registers could be refactored some more, but I suppose
it could just as well be done in a separate patch. Or not. It's your
code after all.
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
^ permalink raw reply [flat|nested] 30+ messages in thread