Port the i915 HDMI infoframe code to the common infrastructure

Port the i915 HDMI infoframe code to the common infrastructure

[Damien Lespiau]

It's time to revive Thierry and Paulo effort to port the i915 HDMI infoframe
code to the common helpers.

Last attempt was:
  http://lists.freedesktop.org/archives/dri-devel/2013-February/035202.html

I've split the series a bit to address Daniel's concerns about reviewability
and pimped up video/hdmi by adding a new generic union hdmi_infoframe.

Basic testing with both a TV and a monitor seems to indicate we're still
writing the same infoframe data in the DIP registers as before, yey!

-- 
Damien

[PATCH 1/8] video/hdmi: Replace the payload length by their defines

[Damien Lespiau]

Cc: Thierry Reding <thierry.reding@avionic-design.de>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/video/hdmi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index 4017833..dbd882f 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -52,7 +52,7 @@ int hdmi_avi_infoframe_init(struct hdmi_avi_infoframe *frame)
 
 	frame->type = HDMI_INFOFRAME_TYPE_AVI;
 	frame->version = 2;
-	frame->length = 13;
+	frame->length = HDMI_AVI_INFOFRAME_SIZE;
 
 	return 0;
 }
@@ -151,7 +151,7 @@ int hdmi_spd_infoframe_init(struct hdmi_spd_infoframe *frame,
 
 	frame->type = HDMI_INFOFRAME_TYPE_SPD;
 	frame->version = 1;
-	frame->length = 25;
+	frame->length = HDMI_SPD_INFOFRAME_SIZE;
 
 	strncpy(frame->vendor, vendor, sizeof(frame->vendor));
 	strncpy(frame->product, product, sizeof(frame->product));
@@ -218,7 +218,7 @@ int hdmi_audio_infoframe_init(struct hdmi_audio_infoframe *frame)
 
 	frame->type = HDMI_INFOFRAME_TYPE_AUDIO;
 	frame->version = 1;
-	frame->length = 10;
+	frame->length = HDMI_AUDIO_INFOFRAME_SIZE;
 
 	return 0;
 }
-- 
1.8.3.1

Re: [PATCH 1/8] video/hdmi: Replace the payload length by their defines

[Ville Syrjälä]

On Fri, Aug 02, 2013 at 06:22:53PM +0100, Damien Lespiau wrote:
> Cc: Thierry Reding <thierry.reding@avionic-design.de>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/video/hdmi.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index 4017833..dbd882f 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -52,7 +52,7 @@ int hdmi_avi_infoframe_init(struct hdmi_avi_infoframe *frame)
>  
>  	frame->type = HDMI_INFOFRAME_TYPE_AVI;
>  	frame->version = 2;
> -	frame->length = 13;
> +	frame->length = HDMI_AVI_INFOFRAME_SIZE;
>  
>  	return 0;
>  }
> @@ -151,7 +151,7 @@ int hdmi_spd_infoframe_init(struct hdmi_spd_infoframe *frame,
>  
>  	frame->type = HDMI_INFOFRAME_TYPE_SPD;
>  	frame->version = 1;
> -	frame->length = 25;
> +	frame->length = HDMI_SPD_INFOFRAME_SIZE;
>  
>  	strncpy(frame->vendor, vendor, sizeof(frame->vendor));
>  	strncpy(frame->product, product, sizeof(frame->product));
> @@ -218,7 +218,7 @@ int hdmi_audio_infoframe_init(struct hdmi_audio_infoframe *frame)
>  
>  	frame->type = HDMI_INFOFRAME_TYPE_AUDIO;
>  	frame->version = 1;
> -	frame->length = 10;
> +	frame->length = HDMI_AUDIO_INFOFRAME_SIZE;
>  
>  	return 0;
>  }
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[PATCH 2/8] video/hdmi: Introduce a generic hdmi_infoframe union

[Damien Lespiau]

And a way to pack hdmi_infoframe generically.

Cc: Thierry Reding <thierry.reding@avionic-design.de>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/video/hdmi.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/hdmi.h | 17 +++++++++++++++++
 2 files changed, 60 insertions(+)

diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
index dbd882f..f7a85e5 100644
--- a/drivers/video/hdmi.c
+++ b/drivers/video/hdmi.c
@@ -22,6 +22,7 @@
  */
 
 #include <linux/bitops.h>
+#include <linux/bug.h>
 #include <linux/errno.h>
 #include <linux/export.h>
 #include <linux/hdmi.h>
@@ -321,3 +322,45 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
 	return length;
 }
 EXPORT_SYMBOL(hdmi_vendor_infoframe_pack);
+
+/**
+ * hdmi_infoframe_pack() - write a HDMI infoframe to binary buffer
+ * @frame: HDMI 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_infoframe_pack(union hdmi_infoframe *frame, void *buffer, size_t size)
+{
+	ssize_t length;
+
+	switch (frame->any.type) {
+	case HDMI_INFOFRAME_TYPE_AVI:
+		length = hdmi_avi_infoframe_pack(&frame->avi, buffer, size);
+		break;
+	case HDMI_INFOFRAME_TYPE_SPD:
+		length = hdmi_spd_infoframe_pack(&frame->spd, buffer, size);
+		break;
+	case HDMI_INFOFRAME_TYPE_AUDIO:
+		length = hdmi_audio_infoframe_pack(&frame->audio, buffer, size);
+		break;
+	case HDMI_INFOFRAME_TYPE_VENDOR:
+		length = hdmi_vendor_infoframe_pack(&frame->vendor,
+						    buffer, size);
+		break;
+	default:
+		WARN(1, "Bad infoframe type %d\n", frame->any.type);
+		length = -EINVAL;
+	}
+
+	return length;
+}
+EXPORT_SYMBOL(hdmi_infoframe_pack);
diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index 3b58944..0f3f82e 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -23,6 +23,12 @@ enum hdmi_infoframe_type {
 #define HDMI_SPD_INFOFRAME_SIZE    25
 #define HDMI_AUDIO_INFOFRAME_SIZE  10
 
+struct hdmi_any_infoframe {
+	enum hdmi_infoframe_type type;
+	unsigned char version;
+	unsigned char length;
+};
+
 enum hdmi_colorspace {
 	HDMI_COLORSPACE_RGB,
 	HDMI_COLORSPACE_YUV422,
@@ -228,4 +234,15 @@ struct hdmi_vendor_infoframe {
 ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
 				   void *buffer, size_t size);
 
+union hdmi_infoframe {
+	struct hdmi_any_infoframe any;
+	struct hdmi_avi_infoframe avi;
+	struct hdmi_spd_infoframe spd;
+	struct hdmi_vendor_infoframe vendor;
+	struct hdmi_audio_infoframe audio;
+};
+
+ssize_t
+hdmi_infoframe_pack(union hdmi_infoframe *frame, void *buffer, size_t size);
+
 #endif /* _DRM_HDMI_H */
-- 
1.8.3.1

Re: [PATCH 2/8] video/hdmi: Introduce a generic hdmi_infoframe union

[Ville Syrjälä]

On Fri, Aug 02, 2013 at 06:22:54PM +0100, Damien Lespiau wrote:
> And a way to pack hdmi_infoframe generically.
> 
> Cc: Thierry Reding <thierry.reding@avionic-design.de>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/video/hdmi.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/hdmi.h | 17 +++++++++++++++++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index dbd882f..f7a85e5 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -22,6 +22,7 @@
>   */
>  
>  #include <linux/bitops.h>
> +#include <linux/bug.h>
>  #include <linux/errno.h>
>  #include <linux/export.h>
>  #include <linux/hdmi.h>
> @@ -321,3 +322,45 @@ ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
>  	return length;
>  }
>  EXPORT_SYMBOL(hdmi_vendor_infoframe_pack);
> +
> +/**
> + * hdmi_infoframe_pack() - write a HDMI infoframe to binary buffer
> + * @frame: HDMI 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_infoframe_pack(union hdmi_infoframe *frame, void *buffer, size_t size)
> +{
> +	ssize_t length;

I was thinking that we might move the buffer length check here. But
then we'd need to make this the only public entry point, and that means
going through all current users. So maybe it's not worth it.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
> +	switch (frame->any.type) {
> +	case HDMI_INFOFRAME_TYPE_AVI:
> +		length = hdmi_avi_infoframe_pack(&frame->avi, buffer, size);
> +		break;
> +	case HDMI_INFOFRAME_TYPE_SPD:
> +		length = hdmi_spd_infoframe_pack(&frame->spd, buffer, size);
> +		break;
> +	case HDMI_INFOFRAME_TYPE_AUDIO:
> +		length = hdmi_audio_infoframe_pack(&frame->audio, buffer, size);
> +		break;
> +	case HDMI_INFOFRAME_TYPE_VENDOR:
> +		length = hdmi_vendor_infoframe_pack(&frame->vendor,
> +						    buffer, size);
> +		break;
> +	default:
> +		WARN(1, "Bad infoframe type %d\n", frame->any.type);
> +		length = -EINVAL;
> +	}
> +
> +	return length;
> +}
> +EXPORT_SYMBOL(hdmi_infoframe_pack);
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index 3b58944..0f3f82e 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -23,6 +23,12 @@ enum hdmi_infoframe_type {
>  #define HDMI_SPD_INFOFRAME_SIZE    25
>  #define HDMI_AUDIO_INFOFRAME_SIZE  10
>  
> +struct hdmi_any_infoframe {
> +	enum hdmi_infoframe_type type;
> +	unsigned char version;
> +	unsigned char length;
> +};
> +
>  enum hdmi_colorspace {
>  	HDMI_COLORSPACE_RGB,
>  	HDMI_COLORSPACE_YUV422,
> @@ -228,4 +234,15 @@ struct hdmi_vendor_infoframe {
>  ssize_t hdmi_vendor_infoframe_pack(struct hdmi_vendor_infoframe *frame,
>  				   void *buffer, size_t size);
>  
> +union hdmi_infoframe {
> +	struct hdmi_any_infoframe any;
> +	struct hdmi_avi_infoframe avi;
> +	struct hdmi_spd_infoframe spd;
> +	struct hdmi_vendor_infoframe vendor;
> +	struct hdmi_audio_infoframe audio;
> +};
> +
> +ssize_t
> +hdmi_infoframe_pack(union hdmi_infoframe *frame, void *buffer, size_t size);
> +
>  #endif /* _DRM_HDMI_H */
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[PATCH 3/8] video/hdmi: Add a macro to return the size of a full infoframe

[Damien Lespiau]

Cc: Thierry Reding <thierry.reding@avionic-design.de>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 include/linux/hdmi.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
index 0f3f82e..bc6743e 100644
--- a/include/linux/hdmi.h
+++ b/include/linux/hdmi.h
@@ -23,6 +23,9 @@ enum hdmi_infoframe_type {
 #define HDMI_SPD_INFOFRAME_SIZE    25
 #define HDMI_AUDIO_INFOFRAME_SIZE  10
 
+#define HDMI_INFOFRAME_SIZE(type)	\
+	(HDMI_INFOFRAME_HEADER_SIZE + HDMI_ ## type ## _INFOFRAME_SIZE)
+
 struct hdmi_any_infoframe {
 	enum hdmi_infoframe_type type;
 	unsigned char version;
-- 
1.8.3.1

Re: [PATCH 3/8] video/hdmi: Add a macro to return the size of a full infoframe

[Ville Syrjälä]

On Fri, Aug 02, 2013 at 06:22:55PM +0100, Damien Lespiau wrote:
> Cc: Thierry Reding <thierry.reding@avionic-design.de>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  include/linux/hdmi.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/hdmi.h b/include/linux/hdmi.h
> index 0f3f82e..bc6743e 100644
> --- a/include/linux/hdmi.h
> +++ b/include/linux/hdmi.h
> @@ -23,6 +23,9 @@ enum hdmi_infoframe_type {
>  #define HDMI_SPD_INFOFRAME_SIZE    25
>  #define HDMI_AUDIO_INFOFRAME_SIZE  10
>  
> +#define HDMI_INFOFRAME_SIZE(type)	\
> +	(HDMI_INFOFRAME_HEADER_SIZE + HDMI_ ## type ## _INFOFRAME_SIZE)
> +
>  struct hdmi_any_infoframe {
>  	enum hdmi_infoframe_type type;
>  	unsigned char version;
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[PATCH 4/8] drm/i915/hdmi: Change the write_infoframe vfunc to take a buffer and a type

[Damien Lespiau]

First step in the move to the shared infoframe infrastructure, let's
move the different infoframe helpers and the write_infoframe() vfunc to
a type (enum hdmi_infoframe_type) and a buffer + len instead of using
our struct dip_infoframe.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
Signed-off-by: Thierry Reding <thierry.reding at avionic-design.de>
---
 drivers/gpu/drm/i915/intel_drv.h  |   4 +-
 drivers/gpu/drm/i915/intel_hdmi.c | 103 +++++++++++++++++++++-----------------
 2 files changed, 59 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 474797b..f8c21ac 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>
@@ -463,7 +464,8 @@ struct intel_hdmi {
 	enum hdmi_force_audio force_audio;
 	bool rgb_quant_range_selectable;
 	void (*write_infoframe)(struct drm_encoder *encoder,
-				struct dip_infoframe *frame);
+				enum hdmi_infoframe_type type,
+				uint8_t *frame, ssize_t len);
 	void (*set_infoframes)(struct drm_encoder *encoder,
 			       struct drm_display_mode *adjusted_mode);
 };
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index e82cd81..d3225df 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -29,6 +29,7 @@
 #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>
@@ -81,74 +82,75 @@ void intel_dip_infoframe_csum(struct dip_infoframe *frame)
 	frame->checksum = 0x100 - sum;
 }
 
-static u32 g4x_infoframe_index(struct dip_infoframe *frame)
+static u32 g4x_infoframe_index(enum hdmi_infoframe_type type)
 {
-	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,
+static u32 hsw_infoframe_data_reg(enum hdmi_infoframe_type type,
 				  enum transcoder cpu_transcoder)
 {
-	switch (frame->type) {
-	case DIP_TYPE_AVI:
+	switch (type) {
+	case HDMI_INFOFRAME_TYPE_AVI:
 		return HSW_TVIDEO_DIP_AVI_DATA(cpu_transcoder);
-	case DIP_TYPE_SPD:
+	case HDMI_INFOFRAME_TYPE_SPD:
 		return HSW_TVIDEO_DIP_SPD_DATA(cpu_transcoder);
 	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)
+				enum hdmi_infoframe_type type,
+				uint8_t *frame, ssize_t len)
 {
 	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 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);
 
@@ -162,7 +164,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;
 
@@ -171,22 +173,23 @@ static void g4x_write_infoframe(struct drm_encoder *encoder,
 }
 
 static void ibx_write_infoframe(struct drm_encoder *encoder,
-				struct dip_infoframe *frame)
+				enum hdmi_infoframe_type type,
+				uint8_t *frame, ssize_t len)
 {
 	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 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);
 
@@ -200,7 +203,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;
 
@@ -209,25 +212,26 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
 }
 
 static void cpt_write_infoframe(struct drm_encoder *encoder,
-				struct dip_infoframe *frame)
+				enum hdmi_infoframe_type type,
+				uint8_t *frame, ssize_t len)
 {
 	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 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);
 
@@ -241,7 +245,7 @@ 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;
 
@@ -250,22 +254,23 @@ static void cpt_write_infoframe(struct drm_encoder *encoder,
 }
 
 static void vlv_write_infoframe(struct drm_encoder *encoder,
-				     struct dip_infoframe *frame)
+				enum hdmi_infoframe_type type,
+				uint8_t *frame, ssize_t len)
 {
 	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 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);
 
@@ -279,7 +284,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;
 
@@ -288,21 +293,24 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
 }
 
 static void hsw_write_infoframe(struct drm_encoder *encoder,
-				struct dip_infoframe *frame)
+				enum hdmi_infoframe_type type,
+				uint8_t *frame, ssize_t len)
 {
 	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->config.cpu_transcoder);
-	u32 data_reg = hsw_infoframe_data_reg(frame, intel_crtc->config.cpu_transcoder);
-	unsigned int i, len = DIP_HEADER_SIZE + frame->len;
+	u32 data_reg;
+	unsigned int i;
 	u32 val = I915_READ(ctl_reg);
 
+	data_reg = hsw_infoframe_data_reg(type,
+					  intel_crtc->config.cpu_transcoder);
 	if (data_reg == 0)
 		return;
 
-	val &= ~hsw_infoframe_enable(frame);
+	val &= ~hsw_infoframe_enable(type);
 	I915_WRITE(ctl_reg, val);
 
 	mmiowb();
@@ -315,7 +323,7 @@ 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);
 }
@@ -326,7 +334,8 @@ static void intel_set_infoframe(struct drm_encoder *encoder,
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 
 	intel_dip_infoframe_csum(frame);
-	intel_hdmi->write_infoframe(encoder, frame);
+	intel_hdmi->write_infoframe(encoder, frame->type, (uint8_t *)frame,
+				    DIP_HEADER_SIZE + frame->len);
 }
 
 static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
-- 
1.8.3.1

Re: [PATCH 4/8] drm/i915/hdmi: Change the write_infoframe vfunc to take a buffer and a type

[Ville Syrjälä]

On Fri, Aug 02, 2013 at 06:22:56PM +0100, Damien Lespiau wrote:
> First step in the move to the shared infoframe infrastructure, let's
> move the different infoframe helpers and the write_infoframe() vfunc to
> a type (enum hdmi_infoframe_type) and a buffer + len instead of using
> our struct dip_infoframe.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Signed-off-by: Thierry Reding <thierry.reding at avionic-design.de>
> ---
>  drivers/gpu/drm/i915/intel_drv.h  |   4 +-
>  drivers/gpu/drm/i915/intel_hdmi.c | 103 +++++++++++++++++++++-----------------
>  2 files changed, 59 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 474797b..f8c21ac 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>
> @@ -463,7 +464,8 @@ struct intel_hdmi {
>  	enum hdmi_force_audio force_audio;
>  	bool rgb_quant_range_selectable;
>  	void (*write_infoframe)(struct drm_encoder *encoder,
> -				struct dip_infoframe *frame);
> +				enum hdmi_infoframe_type type,
> +				uint8_t *frame, ssize_t len);

While at it, could we sprinkle a bit of constness around the infoframe
code and make this 'const uint8_t *frame'?

>  	void (*set_infoframes)(struct drm_encoder *encoder,
>  			       struct drm_display_mode *adjusted_mode);
>  };
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index e82cd81..d3225df 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -29,6 +29,7 @@
>  #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>
> @@ -81,74 +82,75 @@ void intel_dip_infoframe_csum(struct dip_infoframe *frame)
>  	frame->checksum = 0x100 - sum;
>  }
>  
> -static u32 g4x_infoframe_index(struct dip_infoframe *frame)
> +static u32 g4x_infoframe_index(enum hdmi_infoframe_type type)
>  {
> -	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,
> +static u32 hsw_infoframe_data_reg(enum hdmi_infoframe_type type,
>  				  enum transcoder cpu_transcoder)
>  {
> -	switch (frame->type) {
> -	case DIP_TYPE_AVI:
> +	switch (type) {
> +	case HDMI_INFOFRAME_TYPE_AVI:
>  		return HSW_TVIDEO_DIP_AVI_DATA(cpu_transcoder);
> -	case DIP_TYPE_SPD:
> +	case HDMI_INFOFRAME_TYPE_SPD:
>  		return HSW_TVIDEO_DIP_SPD_DATA(cpu_transcoder);
>  	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)
> +				enum hdmi_infoframe_type type,
> +				uint8_t *frame, ssize_t len)
>  {
>  	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 i;

'len' is now signed, but before it was unsigned, and 'i' is still
unsigned. A bit of consistency with the types might be nice.

>  
>  	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);
>  
> @@ -162,7 +164,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;
>  
> @@ -171,22 +173,23 @@ static void g4x_write_infoframe(struct drm_encoder *encoder,
>  }
>  
>  static void ibx_write_infoframe(struct drm_encoder *encoder,
> -				struct dip_infoframe *frame)
> +				enum hdmi_infoframe_type type,
> +				uint8_t *frame, ssize_t len)
>  {
>  	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 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);
>  
> @@ -200,7 +203,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;
>  
> @@ -209,25 +212,26 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
>  }
>  
>  static void cpt_write_infoframe(struct drm_encoder *encoder,
> -				struct dip_infoframe *frame)
> +				enum hdmi_infoframe_type type,
> +				uint8_t *frame, ssize_t len)
>  {
>  	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 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);
>  
> @@ -241,7 +245,7 @@ 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;
>  
> @@ -250,22 +254,23 @@ static void cpt_write_infoframe(struct drm_encoder *encoder,
>  }
>  
>  static void vlv_write_infoframe(struct drm_encoder *encoder,
> -				     struct dip_infoframe *frame)
> +				enum hdmi_infoframe_type type,
> +				uint8_t *frame, ssize_t len)
>  {
>  	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 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);
>  
> @@ -279,7 +284,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;
>  
> @@ -288,21 +293,24 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
>  }
>  
>  static void hsw_write_infoframe(struct drm_encoder *encoder,
> -				struct dip_infoframe *frame)
> +				enum hdmi_infoframe_type type,
> +				uint8_t *frame, ssize_t len)
>  {
>  	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->config.cpu_transcoder);
> -	u32 data_reg = hsw_infoframe_data_reg(frame, intel_crtc->config.cpu_transcoder);
> -	unsigned int i, len = DIP_HEADER_SIZE + frame->len;
> +	u32 data_reg;
> +	unsigned int i;
>  	u32 val = I915_READ(ctl_reg);
>  
> +	data_reg = hsw_infoframe_data_reg(type,
> +					  intel_crtc->config.cpu_transcoder);
>  	if (data_reg == 0)
>  		return;
>  
> -	val &= ~hsw_infoframe_enable(frame);
> +	val &= ~hsw_infoframe_enable(type);
>  	I915_WRITE(ctl_reg, val);
>  
>  	mmiowb();
> @@ -315,7 +323,7 @@ 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);
>  }
> @@ -326,7 +334,8 @@ static void intel_set_infoframe(struct drm_encoder *encoder,
>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>  
>  	intel_dip_infoframe_csum(frame);
> -	intel_hdmi->write_infoframe(encoder, frame);
> +	intel_hdmi->write_infoframe(encoder, frame->type, (uint8_t *)frame,
> +				    DIP_HEADER_SIZE + frame->len);
>  }
>  
>  static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[PATCH 5/8] drm/i915/hdmi: Port the infoframe code to the common hdmi helpers

[Damien Lespiau]

Let's use the drivers/video/hmdi.c and drm infoframe helpers to build
our infoframes.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 94 +++++++++++++++++++++++++++++----------
 1 file changed, 70 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index d3225df..57dd413 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -328,14 +328,55 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
 	POSTING_READ(ctl_reg);
 }
 
+/*
+ * The data we write to the DIP data buffer registers is 1 byte bigger than the
+ * HDMI infoframe size because of an ECC/reserved byte at position 3 (starting
+ * at 0). It's also a byte used by DisplayPort so the same DIP registers can be
+ * used for both technologies.
+ *
+ * DW0: Reserved/ECC/DP | HB2 | HB1 | HB0
+ * DW1:       DB3       | DB2 | DB1 | DB0
+ * DW2:       DB7       | DB6 | DB5 | DB4
+ * DW3: ...
+ *
+ * (HB is Header Byte, DB is Data Byte)
+ *
+ * The hdmi pack() functions don't know about that hardware specific hole so we
+ * trick them by giving an offset into the buffer and moving back the header
+ * bytes by one.
+ *
+ * The other constraint is that we write data to a 32 bits register, so we
+ * need to round the buffer up to the next multiple of 4, if not already one.
+ */
+#define INFOFRAME_BUFFER_SIZE(type)	\
+	(ALIGN(HDMI_INFOFRAME_SIZE(type) + 1, 4))
+
+/* now the packing buffer needs to be able to hold the largest infoframe */
+#define DIP_BUFFER_SIZE	\
+	(max(INFOFRAME_BUFFER_SIZE(AVI), INFOFRAME_BUFFER_SIZE(SPD)))
+
 static void intel_set_infoframe(struct drm_encoder *encoder,
-				struct dip_infoframe *frame)
+				union hdmi_infoframe *frame)
 {
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+	uint8_t buffer[DIP_BUFFER_SIZE];
+	ssize_t len;
+
+	memset(buffer, 0, sizeof(buffer));
+
+	/* see comment above for the reason for this offset */
+	len = hdmi_infoframe_pack(frame, buffer + 1, sizeof(buffer) - 1);
+	if (len < 0)
+		return;
 
-	intel_dip_infoframe_csum(frame);
-	intel_hdmi->write_infoframe(encoder, frame->type, (uint8_t *)frame,
-				    DIP_HEADER_SIZE + frame->len);
+	/* Insert the 'hole' (see big comment above) at position 3 */
+	buffer[0] = buffer[1];
+	buffer[1] = buffer[2];
+	buffer[2] = buffer[3];
+	buffer[3] = 0;
+	len++;
+
+	intel_hdmi->write_infoframe(encoder, frame->any.type, buffer, len);
 }
 
 static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
@@ -343,40 +384,45 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
 {
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
-	struct dip_infoframe avi_if = {
-		.type = DIP_TYPE_AVI,
-		.ver = DIP_VERSION_AVI,
-		.len = DIP_LEN_AVI,
-	};
+	union hdmi_infoframe frame;
+	int ret;
+
+	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
+						       adjusted_mode);
+	if (ret < 0) {
+		DRM_ERROR("couldn't fill AVI infoframe\n");
+		return;
+	}
 
 	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
-		avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
+		frame.avi.pixel_repeat = 1;
 
 	if (intel_hdmi->rgb_quant_range_selectable) {
 		if (intel_crtc->config.limited_color_range)
-			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_LIMITED;
+			frame.avi.quantization_range =
+				HDMI_QUANTIZATION_RANGE_LIMITED;
 		else
-			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_FULL;
+			frame.avi.quantization_range =
+				HDMI_QUANTIZATION_RANGE_FULL;
 	}
 
-	avi_if.body.avi.VIC = drm_match_cea_mode(adjusted_mode);
-
-	intel_set_infoframe(encoder, &avi_if);
+	intel_set_infoframe(encoder, &frame);
 }
 
 static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
 {
-	struct dip_infoframe spd_if;
+	union hdmi_infoframe frame;
+	int ret;
+
+	ret = hdmi_spd_infoframe_init(&frame.spd, "Intel", "Integrated gfx");
+	if (ret < 0) {
+		DRM_ERROR("couldn't fill SPD infoframe\n");
+		return;
+	}
 
-	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;
+	frame.spd.sdi = HDMI_SPD_SDI_PC;
 
-	intel_set_infoframe(encoder, &spd_if);
+	intel_set_infoframe(encoder, &frame);
 }
 
 static void g4x_set_infoframes(struct drm_encoder *encoder,
-- 
1.8.3.1

Re: [PATCH 5/8] drm/i915/hdmi: Port the infoframe code to the common hdmi helpers

[Ville Syrjälä]

On Fri, Aug 02, 2013 at 06:22:57PM +0100, Damien Lespiau wrote:
> Let's use the drivers/video/hmdi.c and drm infoframe helpers to build
> our infoframes.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 94 +++++++++++++++++++++++++++++----------
>  1 file changed, 70 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index d3225df..57dd413 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -328,14 +328,55 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
>  	POSTING_READ(ctl_reg);
>  }
>  
> +/*
> + * The data we write to the DIP data buffer registers is 1 byte bigger than the
> + * HDMI infoframe size because of an ECC/reserved byte at position 3 (starting
> + * at 0). It's also a byte used by DisplayPort so the same DIP registers can be
> + * used for both technologies.
> + *
> + * DW0: Reserved/ECC/DP | HB2 | HB1 | HB0
> + * DW1:       DB3       | DB2 | DB1 | DB0
> + * DW2:       DB7       | DB6 | DB5 | DB4
> + * DW3: ...
> + *
> + * (HB is Header Byte, DB is Data Byte)
> + *
> + * The hdmi pack() functions don't know about that hardware specific hole so we
> + * trick them by giving an offset into the buffer and moving back the header
> + * bytes by one.
> + *
> + * The other constraint is that we write data to a 32 bits register, so we
> + * need to round the buffer up to the next multiple of 4, if not already one.
> + */
> +#define INFOFRAME_BUFFER_SIZE(type)	\
> +	(ALIGN(HDMI_INFOFRAME_SIZE(type) + 1, 4))
> +
> +/* now the packing buffer needs to be able to hold the largest infoframe */
> +#define DIP_BUFFER_SIZE	\
> +	(max(INFOFRAME_BUFFER_SIZE(AVI), INFOFRAME_BUFFER_SIZE(SPD)))
> +
>  static void intel_set_infoframe(struct drm_encoder *encoder,
> -				struct dip_infoframe *frame)
> +				union hdmi_infoframe *frame)
>  {
>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> +	uint8_t buffer[DIP_BUFFER_SIZE];
> +	ssize_t len;
> +
> +	memset(buffer, 0, sizeof(buffer));

Could just zero initialize 'buffer' when declaring it. However
hdmi_*_infoframe_pack() already contain a memset(), so we shouldn't even
need to zero it here.

> +
> +	/* see comment above for the reason for this offset */
> +	len = hdmi_infoframe_pack(frame, buffer + 1, sizeof(buffer) - 1);
> +	if (len < 0)
> +		return;
>  
> -	intel_dip_infoframe_csum(frame);
> -	intel_hdmi->write_infoframe(encoder, frame->type, (uint8_t *)frame,
> -				    DIP_HEADER_SIZE + frame->len);
> +	/* Insert the 'hole' (see big comment above) at position 3 */
> +	buffer[0] = buffer[1];
> +	buffer[1] = buffer[2];
> +	buffer[2] = buffer[3];
> +	buffer[3] = 0;
> +	len++;
> +
> +	intel_hdmi->write_infoframe(encoder, frame->any.type, buffer, len);
>  }
>  
>  static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
> @@ -343,40 +384,45 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>  {
>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> -	struct dip_infoframe avi_if = {
> -		.type = DIP_TYPE_AVI,
> -		.ver = DIP_VERSION_AVI,
> -		.len = DIP_LEN_AVI,
> -	};
> +	union hdmi_infoframe frame;
> +	int ret;
> +
> +	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> +						       adjusted_mode);
> +	if (ret < 0) {
> +		DRM_ERROR("couldn't fill AVI infoframe\n");
> +		return;
> +	}
>  
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> -		avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
> +		frame.avi.pixel_repeat = 1;

Perhaps that should be part of drm_hdmi_avi_infoframe_from_display_mode()?

>  
>  	if (intel_hdmi->rgb_quant_range_selectable) {
>  		if (intel_crtc->config.limited_color_range)
> -			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_LIMITED;
> +			frame.avi.quantization_range =
> +				HDMI_QUANTIZATION_RANGE_LIMITED;
>  		else
> -			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_FULL;
> +			frame.avi.quantization_range =
> +				HDMI_QUANTIZATION_RANGE_FULL;
>  	}
>  
> -	avi_if.body.avi.VIC = drm_match_cea_mode(adjusted_mode);
> -
> -	intel_set_infoframe(encoder, &avi_if);
> +	intel_set_infoframe(encoder, &frame);
>  }
>  
>  static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
>  {
> -	struct dip_infoframe spd_if;
> +	union hdmi_infoframe frame;
> +	int ret;
> +
> +	ret = hdmi_spd_infoframe_init(&frame.spd, "Intel", "Integrated gfx");
> +	if (ret < 0) {
> +		DRM_ERROR("couldn't fill SPD infoframe\n");
> +		return;
> +	}
>  
> -	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;
> +	frame.spd.sdi = HDMI_SPD_SDI_PC;
>  
> -	intel_set_infoframe(encoder, &spd_if);
> +	intel_set_infoframe(encoder, &frame);
>  }
>  
>  static void g4x_set_infoframes(struct drm_encoder *encoder,
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 5/8] drm/i915/hdmi: Port the infoframe code to the common hdmi helpers

[Damien Lespiau]

On Mon, Aug 05, 2013 at 04:12:11PM +0300, Ville Syrjälä wrote:
> >  static void intel_set_infoframe(struct drm_encoder *encoder,
> > -				struct dip_infoframe *frame)
> > +				union hdmi_infoframe *frame)
> >  {
> >  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> > +	uint8_t buffer[DIP_BUFFER_SIZE];
> > +	ssize_t len;
> > +
> > +	memset(buffer, 0, sizeof(buffer));
> 
> Could just zero initialize 'buffer' when declaring it. However
> hdmi_*_infoframe_pack() already contain a memset(), so we shouldn't even
> need to zero it here.

There was a good, admittedly well hidden, reason for this one. _pack()
only zeroes up to the size of the infoframe and _write_infoframe() makes
sure sure to write 0s dwords after the infoframe (rounded up to a
multiple of 4) for the checksum. Without it we're left with
4 - (infoframe_size % 4) bytes of garbage in the infoframe data.

The new series makes the hdmi _pack helpers zero the whole incoming
buffer instead, I think it makes sense.

-- 
Damien

> 
> > +
> > +	/* see comment above for the reason for this offset */
> > +	len = hdmi_infoframe_pack(frame, buffer + 1, sizeof(buffer) - 1);
> > +	if (len < 0)
> > +		return;
> >  
> > -	intel_dip_infoframe_csum(frame);
> > -	intel_hdmi->write_infoframe(encoder, frame->type, (uint8_t *)frame,
> > -				    DIP_HEADER_SIZE + frame->len);
> > +	/* Insert the 'hole' (see big comment above) at position 3 */
> > +	buffer[0] = buffer[1];
> > +	buffer[1] = buffer[2];
> > +	buffer[2] = buffer[3];
> > +	buffer[3] = 0;
> > +	len++;
> > +
> > +	intel_hdmi->write_infoframe(encoder, frame->any.type, buffer, len);
> >  }
> >  
> >  static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
> > @@ -343,40 +384,45 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
> >  {
> >  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> > -	struct dip_infoframe avi_if = {
> > -		.type = DIP_TYPE_AVI,
> > -		.ver = DIP_VERSION_AVI,
> > -		.len = DIP_LEN_AVI,
> > -	};
> > +	union hdmi_infoframe frame;
> > +	int ret;
> > +
> > +	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> > +						       adjusted_mode);
> > +	if (ret < 0) {
> > +		DRM_ERROR("couldn't fill AVI infoframe\n");
> > +		return;
> > +	}
> >  
> >  	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> > -		avi_if.body.avi.YQ_CN_PR |= DIP_AVI_PR_2;
> > +		frame.avi.pixel_repeat = 1;
> 
> Perhaps that should be part of drm_hdmi_avi_infoframe_from_display_mode()?
> 
> >  
> >  	if (intel_hdmi->rgb_quant_range_selectable) {
> >  		if (intel_crtc->config.limited_color_range)
> > -			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_LIMITED;
> > +			frame.avi.quantization_range =
> > +				HDMI_QUANTIZATION_RANGE_LIMITED;
> >  		else
> > -			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_FULL;
> > +			frame.avi.quantization_range =
> > +				HDMI_QUANTIZATION_RANGE_FULL;
> >  	}
> >  
> > -	avi_if.body.avi.VIC = drm_match_cea_mode(adjusted_mode);
> > -
> > -	intel_set_infoframe(encoder, &avi_if);
> > +	intel_set_infoframe(encoder, &frame);
> >  }
> >  
> >  static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
> >  {
> > -	struct dip_infoframe spd_if;
> > +	union hdmi_infoframe frame;
> > +	int ret;
> > +
> > +	ret = hdmi_spd_infoframe_init(&frame.spd, "Intel", "Integrated gfx");
> > +	if (ret < 0) {
> > +		DRM_ERROR("couldn't fill SPD infoframe\n");
> > +		return;
> > +	}
> >  
> > -	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;
> > +	frame.spd.sdi = HDMI_SPD_SDI_PC;
> >  
> > -	intel_set_infoframe(encoder, &spd_if);
> > +	intel_set_infoframe(encoder, &frame);
> >  }
> >  
> >  static void g4x_set_infoframes(struct drm_encoder *encoder,
> > -- 
> > 1.8.3.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

[PATCH 6/8] drm/i915/hmdi: Rename set_infoframe() to write_infoframe()

[Damien Lespiau]

set_frame() wraps the write_frame() vfunc. Be consistent and name the
wrapping function like the vfunc being called.

It's doubly confusing as we also have a set_infoframes() vfunc and
set_infoframe() doesn't wrap it.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 57dd413..8424882 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -355,8 +355,8 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
 #define DIP_BUFFER_SIZE	\
 	(max(INFOFRAME_BUFFER_SIZE(AVI), INFOFRAME_BUFFER_SIZE(SPD)))
 
-static void intel_set_infoframe(struct drm_encoder *encoder,
-				union hdmi_infoframe *frame)
+static void intel_write_infoframe(struct drm_encoder *encoder,
+				  union hdmi_infoframe *frame)
 {
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 	uint8_t buffer[DIP_BUFFER_SIZE];
@@ -406,7 +406,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
 				HDMI_QUANTIZATION_RANGE_FULL;
 	}
 
-	intel_set_infoframe(encoder, &frame);
+	intel_write_infoframe(encoder, &frame);
 }
 
 static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
@@ -422,7 +422,7 @@ static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
 
 	frame.spd.sdi = HDMI_SPD_SDI_PC;
 
-	intel_set_infoframe(encoder, &frame);
+	intel_write_infoframe(encoder, &frame);
 }
 
 static void g4x_set_infoframes(struct drm_encoder *encoder,
-- 
1.8.3.1

Re: [PATCH 6/8] drm/i915/hmdi: Rename set_infoframe() to write_infoframe()

[Ville Syrjälä]

On Fri, Aug 02, 2013 at 06:22:58PM +0100, Damien Lespiau wrote:
> set_frame() wraps the write_frame() vfunc. Be consistent and name the
> wrapping function like the vfunc being called.
> 
> It's doubly confusing as we also have a set_infoframes() vfunc and
> set_infoframe() doesn't wrap it.

I guess the logic was 
 set_infoframes -> set_<type>_infoframe -> set_infoframe -> write_infoframe

But I don't really have an opinion on which is better, so:

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 57dd413..8424882 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -355,8 +355,8 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
>  #define DIP_BUFFER_SIZE	\
>  	(max(INFOFRAME_BUFFER_SIZE(AVI), INFOFRAME_BUFFER_SIZE(SPD)))
>  
> -static void intel_set_infoframe(struct drm_encoder *encoder,
> -				union hdmi_infoframe *frame)
> +static void intel_write_infoframe(struct drm_encoder *encoder,
> +				  union hdmi_infoframe *frame)
>  {
>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>  	uint8_t buffer[DIP_BUFFER_SIZE];
> @@ -406,7 +406,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
>  				HDMI_QUANTIZATION_RANGE_FULL;
>  	}
>  
> -	intel_set_infoframe(encoder, &frame);
> +	intel_write_infoframe(encoder, &frame);
>  }
>  
>  static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
> @@ -422,7 +422,7 @@ static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
>  
>  	frame.spd.sdi = HDMI_SPD_SDI_PC;
>  
> -	intel_set_infoframe(encoder, &frame);
> +	intel_write_infoframe(encoder, &frame);
>  }
>  
>  static void g4x_set_infoframes(struct drm_encoder *encoder,
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 6/8] drm/i915/hmdi: Rename set_infoframe() to write_infoframe()

[Damien Lespiau]

On Mon, Aug 05, 2013 at 08:50:45PM +0300, Ville Syrjälä wrote:
> On Fri, Aug 02, 2013 at 06:22:58PM +0100, Damien Lespiau wrote:
> > set_frame() wraps the write_frame() vfunc. Be consistent and name the
> > wrapping function like the vfunc being called.
> > 
> > It's doubly confusing as we also have a set_infoframes() vfunc and
> > set_infoframe() doesn't wrap it.
> 
> I guess the logic was 
>  set_infoframes -> set_<type>_infoframe -> set_infoframe -> write_infoframe
> 
> But I don't really have an opinion on which is better, so:

Meh, doesn't really matter :) I'll just drop this patch.

> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_hdmi.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 57dd413..8424882 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -355,8 +355,8 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
> >  #define DIP_BUFFER_SIZE	\
> >  	(max(INFOFRAME_BUFFER_SIZE(AVI), INFOFRAME_BUFFER_SIZE(SPD)))
> >  
> > -static void intel_set_infoframe(struct drm_encoder *encoder,
> > -				union hdmi_infoframe *frame)
> > +static void intel_write_infoframe(struct drm_encoder *encoder,
> > +				  union hdmi_infoframe *frame)
> >  {
> >  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
> >  	uint8_t buffer[DIP_BUFFER_SIZE];
> > @@ -406,7 +406,7 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder,
> >  				HDMI_QUANTIZATION_RANGE_FULL;
> >  	}
> >  
> > -	intel_set_infoframe(encoder, &frame);
> > +	intel_write_infoframe(encoder, &frame);
> >  }
> >  
> >  static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
> > @@ -422,7 +422,7 @@ static void intel_hdmi_set_spd_infoframe(struct drm_encoder *encoder)
> >  
> >  	frame.spd.sdi = HDMI_SPD_SDI_PC;
> >  
> > -	intel_set_infoframe(encoder, &frame);
> > +	intel_write_infoframe(encoder, &frame);
> >  }
> >  
> >  static void g4x_set_infoframes(struct drm_encoder *encoder,
> > -- 
> > 1.8.3.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

Re: [PATCH 6/8] drm/i915/hmdi: Rename set_infoframe() to write_infoframe()

[Daniel Vetter]

On Tue, Aug 6, 2013 at 7:49 PM, Damien Lespiau <damien.lespiau@intel.com> wrote:
> On Mon, Aug 05, 2013 at 08:50:45PM +0300, Ville Syrjälä wrote:
>> On Fri, Aug 02, 2013 at 06:22:58PM +0100, Damien Lespiau wrote:
>> > set_frame() wraps the write_frame() vfunc. Be consistent and name the
>> > wrapping function like the vfunc being called.
>> >
>> > It's doubly confusing as we also have a set_infoframes() vfunc and
>> > set_infoframe() doesn't wrap it.
>>
>> I guess the logic was
>>  set_infoframes -> set_<type>_infoframe -> set_infoframe -> write_infoframe
>>
>> But I don't really have an opinion on which is better, so:
>
> Meh, doesn't really matter :) I'll just drop this patch.

I think it makes sense and more clearly distinguishes set_infoframes
from from writing them. So would have merged ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/8] drm/i915/hmdi: Rename set_infoframe() to write_infoframe()

[Damien Lespiau]

On Tue, Aug 06, 2013 at 08:01:34PM +0200, Daniel Vetter wrote:
> On Tue, Aug 6, 2013 at 7:49 PM, Damien Lespiau <damien.lespiau@intel.com> wrote:
> > On Mon, Aug 05, 2013 at 08:50:45PM +0300, Ville Syrjälä wrote:
> >> On Fri, Aug 02, 2013 at 06:22:58PM +0100, Damien Lespiau wrote:
> >> > set_frame() wraps the write_frame() vfunc. Be consistent and name the
> >> > wrapping function like the vfunc being called.
> >> >
> >> > It's doubly confusing as we also have a set_infoframes() vfunc and
> >> > set_infoframe() doesn't wrap it.
> >>
> >> I guess the logic was
> >>  set_infoframes -> set_<type>_infoframe -> set_infoframe -> write_infoframe
> >>
> >> But I don't really have an opinion on which is better, so:
> >
> > Meh, doesn't really matter :) I'll just drop this patch.
> 
> I think it makes sense and more clearly distinguishes set_infoframes
> from from writing them. So would have merged ...

Will resend with the new series then :)

-- 
Damien

[PATCH 7/8] drm/i915/sdvo: Port the infoframe code to the shared infrastructure

[Damien Lespiau]

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
Signed-off-by: Thierry Reding <thierry.reding at avionic-design.de>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 47423f3..02f220b 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -963,30 +963,32 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo,
 static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo,
 					 const struct drm_display_mode *adjusted_mode)
 {
-	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)];
-	struct intel_crtc *intel_crtc = to_intel_crtc(intel_sdvo->base.base.crtc);
+	uint8_t sdvo_data[HDMI_INFOFRAME_SIZE(AVI)];
+	struct drm_crtc *crtc = intel_sdvo->base.base.crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	union hdmi_infoframe frame;
+	int ret;
+	ssize_t len;
+
+	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
+						       adjusted_mode);
+	if (ret < 0) {
+		DRM_ERROR("couldn't fill AVI infoframe\n");
+		return false;
+	}
 
 	if (intel_sdvo->rgb_quant_range_selectable) {
 		if (intel_crtc->config.limited_color_range)
-			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_LIMITED;
+			frame.avi.quantization_range =
+				HDMI_QUANTIZATION_RANGE_LIMITED;
 		else
-			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_FULL;
+			frame.avi.quantization_range =
+				HDMI_QUANTIZATION_RANGE_FULL;
 	}
 
-	avi_if.body.avi.VIC = drm_match_cea_mode(adjusted_mode);
-
-	intel_dip_infoframe_csum(&avi_if);
-
-	/* 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_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.3.1

Re: [PATCH 7/8] drm/i915/sdvo: Port the infoframe code to the shared infrastructure

[Ville Syrjälä]

On Fri, Aug 02, 2013 at 06:22:59PM +0100, Damien Lespiau wrote:
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Signed-off-by: Thierry Reding <thierry.reding at avionic-design.de>

Looks all right.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_sdvo.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
> index 47423f3..02f220b 100644
> --- a/drivers/gpu/drm/i915/intel_sdvo.c
> +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> @@ -963,30 +963,32 @@ static bool intel_sdvo_write_infoframe(struct intel_sdvo *intel_sdvo,
>  static bool intel_sdvo_set_avi_infoframe(struct intel_sdvo *intel_sdvo,
>  					 const struct drm_display_mode *adjusted_mode)
>  {
> -	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)];
> -	struct intel_crtc *intel_crtc = to_intel_crtc(intel_sdvo->base.base.crtc);
> +	uint8_t sdvo_data[HDMI_INFOFRAME_SIZE(AVI)];
> +	struct drm_crtc *crtc = intel_sdvo->base.base.crtc;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	union hdmi_infoframe frame;
> +	int ret;
> +	ssize_t len;
> +
> +	ret = drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> +						       adjusted_mode);
> +	if (ret < 0) {
> +		DRM_ERROR("couldn't fill AVI infoframe\n");
> +		return false;
> +	}
>  
>  	if (intel_sdvo->rgb_quant_range_selectable) {
>  		if (intel_crtc->config.limited_color_range)
> -			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_LIMITED;
> +			frame.avi.quantization_range =
> +				HDMI_QUANTIZATION_RANGE_LIMITED;
>  		else
> -			avi_if.body.avi.ITC_EC_Q_SC |= DIP_AVI_RGB_QUANT_RANGE_FULL;
> +			frame.avi.quantization_range =
> +				HDMI_QUANTIZATION_RANGE_FULL;
>  	}
>  
> -	avi_if.body.avi.VIC = drm_match_cea_mode(adjusted_mode);
> -
> -	intel_dip_infoframe_csum(&avi_if);
> -
> -	/* 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_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.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[PATCH 8/8] drm/i915: Remove the now obsolete infoframe definitions

[Damien Lespiau]

All the HDMI infoframe code has been ported to use video/hdmi.c, so it's
time to say bye bye to this code.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h  | 61 ---------------------------------------
 drivers/gpu/drm/i915/intel_hdmi.c | 15 ----------
 2 files changed, 76 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f8c21ac..98ff8b7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -394,66 +394,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_AVI_RGB_QUANT_RANGE_DEFAULT	(0 << 2)
-#define DIP_AVI_RGB_QUANT_RANGE_LIMITED	(1 << 2)
-#define DIP_AVI_RGB_QUANT_RANGE_FULL	(2 << 2)
-
-#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 hdmi_reg;
 	int ddc_bus;
@@ -567,7 +507,6 @@ extern void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 extern struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
 extern bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 				      struct intel_crtc_config *pipe_config);
-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 8424882..85dff3f 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -67,21 +67,6 @@ 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)
-{
-	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(enum hdmi_infoframe_type type)
 {
 	switch (type) {
-- 
1.8.3.1

Re: [PATCH 8/8] drm/i915: Remove the now obsolete infoframe definitions

[Ville Syrjälä]

On Fri, Aug 02, 2013 at 06:23:00PM +0100, Damien Lespiau wrote:
> All the HDMI infoframe code has been ported to use video/hdmi.c, so it's
> time to say bye bye to this code.
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>

I'm no compiler, but fwiw:

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_drv.h  | 61 ---------------------------------------
>  drivers/gpu/drm/i915/intel_hdmi.c | 15 ----------
>  2 files changed, 76 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f8c21ac..98ff8b7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -394,66 +394,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_AVI_RGB_QUANT_RANGE_DEFAULT	(0 << 2)
> -#define DIP_AVI_RGB_QUANT_RANGE_LIMITED	(1 << 2)
> -#define DIP_AVI_RGB_QUANT_RANGE_FULL	(2 << 2)
> -
> -#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 hdmi_reg;
>  	int ddc_bus;
> @@ -567,7 +507,6 @@ extern void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  extern struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
>  extern bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  				      struct intel_crtc_config *pipe_config);
> -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 8424882..85dff3f 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -67,21 +67,6 @@ 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)
> -{
> -	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(enum hdmi_infoframe_type type)
>  {
>  	switch (type) {
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC