[PATCH 1/2] drm/i915: implement hsw_write_infoframe

[PATCH 1/2] drm/i915: implement hsw_write_infoframe

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Both the control and data registers are completely different now.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h   |    4 +++
 drivers/gpu/drm/i915/intel_hdmi.c |   68 +++++++++++++++++++++++++++++++++----
 2 files changed, 66 insertions(+), 6 deletions(-)

Things are simpler and easier now.

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index fb76b19..2d49b95 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1697,6 +1697,7 @@
 /* Video Data Island Packet control */
 #define VIDEO_DIP_DATA		0x61178
 #define VIDEO_DIP_CTL		0x61170
+/* Pre HSW: */
 #define   VIDEO_DIP_ENABLE		(1 << 31)
 #define   VIDEO_DIP_PORT_B		(1 << 29)
 #define   VIDEO_DIP_PORT_C		(2 << 29)
@@ -1713,6 +1714,9 @@
 #define   VIDEO_DIP_FREQ_VSYNC		(1 << 16)
 #define   VIDEO_DIP_FREQ_2VSYNC		(2 << 16)
 #define   VIDEO_DIP_FREQ_MASK		(3 << 16)
+/* HSW and later: */
+#define   VIDEO_DIP_ENABLE_AVI_HSW	(1 << 12)
+#define   VIDEO_DIP_ENABLE_SPD_HSW	(1 << 0)
 
 /* Panel power sequencing */
 #define PP_STATUS	0x61200
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 03b3524..ed91db0 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -101,6 +101,44 @@ static u32 g4x_infoframe_enable(struct dip_infoframe *frame)
 	return flags;
 }
 
+static u32 hsw_infoframe_enable(struct dip_infoframe *frame)
+{
+	u32 flags = 0;
+
+	switch (frame->type) {
+	case DIP_TYPE_AVI:
+		flags |= VIDEO_DIP_ENABLE_AVI_HSW;
+		break;
+	case DIP_TYPE_SPD:
+		flags |= VIDEO_DIP_ENABLE_SPD_HSW;
+		break;
+	default:
+		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
+		break;
+	}
+
+	return flags;
+}
+
+static u32 hsw_infoframe_data_reg(struct dip_infoframe *frame, enum pipe pipe)
+{
+	u32 reg = 0;
+
+	switch (frame->type) {
+	case DIP_TYPE_AVI:
+		reg = HSW_TVIDEO_DIP_AVI_DATA(pipe);
+		break;
+	case DIP_TYPE_SPD:
+		reg = HSW_TVIDEO_DIP_SPD_DATA(pipe);
+		break;
+	default:
+		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
+		break;
+	}
+
+	return reg;
+}
+
 static void g4x_write_infoframe(struct drm_encoder *encoder,
 				struct dip_infoframe *frame)
 {
@@ -266,15 +304,33 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
 }
 
 static void hsw_write_infoframe(struct drm_encoder *encoder,
-				     struct dip_infoframe *frame)
+				struct dip_infoframe *frame)
 {
-	/* Not implemented yet, so avoid doing anything at all.
-	 * This is the placeholder for Paulo Zanoni's infoframe writing patch
-	 */
-	DRM_DEBUG_DRIVER("Attempting to write infoframe on Haswell, this is not implemented yet.\n");
+	uint32_t *data = (uint32_t *)frame;
+	struct drm_device *dev = encoder->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc = encoder->crtc;
+	struct intel_crtc *intel_crtc = to_intel_crtc(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 val = I915_READ(ctl_reg);
 
-	return;
+	if (data_reg == 0)
+		return;
+
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
+
+	val &= ~hsw_infoframe_enable(frame);
+	I915_WRITE(ctl_reg, val);
+
+	for (i = 0; i < len; i += 4) {
+		I915_WRITE(data_reg + i, *data);
+		data++;
+	}
 
+	val |= hsw_infoframe_enable(frame);
+	I915_WRITE(ctl_reg, val);
 }
 
 static void intel_set_infoframe(struct drm_encoder *encoder,
-- 
1.7.10

[PATCH 2/2] drm/i915: small hdmi coding style cleanups

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

- Changed the coding style of auxiliary infoframe functions to make
  them smaller
- Fixed the column alignment of some function definitions
- Remove definition of "struct drm_crtc" in some places as they're
  used only to retrieve "struct intel_crtc"

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   71 +++++++++++--------------------------
 1 file changed, 20 insertions(+), 51 deletions(-)

Non-important auxiliary functions don't deserve to fill the whole screen.

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index ed91db0..e996994 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -65,78 +65,54 @@ void intel_dip_infoframe_csum(struct dip_infoframe *frame)
 
 static u32 g4x_infoframe_index(struct dip_infoframe *frame)
 {
-	u32 flags = 0;
-
 	switch (frame->type) {
 	case DIP_TYPE_AVI:
-		flags |= VIDEO_DIP_SELECT_AVI;
-		break;
+		return VIDEO_DIP_SELECT_AVI;
 	case DIP_TYPE_SPD:
-		flags |= VIDEO_DIP_SELECT_SPD;
-		break;
+		return VIDEO_DIP_SELECT_SPD;
 	default:
 		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
-		break;
+		return 0;
 	}
-
-	return flags;
 }
 
 static u32 g4x_infoframe_enable(struct dip_infoframe *frame)
 {
-	u32 flags = 0;
-
 	switch (frame->type) {
 	case DIP_TYPE_AVI:
-		flags |= VIDEO_DIP_ENABLE_AVI;
-		break;
+		return VIDEO_DIP_ENABLE_AVI;
 	case DIP_TYPE_SPD:
-		flags |= VIDEO_DIP_ENABLE_SPD;
-		break;
+		return VIDEO_DIP_ENABLE_SPD;
 	default:
 		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
-		break;
+		return 0;
 	}
-
-	return flags;
 }
 
 static u32 hsw_infoframe_enable(struct dip_infoframe *frame)
 {
-	u32 flags = 0;
-
 	switch (frame->type) {
 	case DIP_TYPE_AVI:
-		flags |= VIDEO_DIP_ENABLE_AVI_HSW;
-		break;
+		return VIDEO_DIP_ENABLE_AVI_HSW;
 	case DIP_TYPE_SPD:
-		flags |= VIDEO_DIP_ENABLE_SPD_HSW;
-		break;
+		return VIDEO_DIP_ENABLE_SPD_HSW;
 	default:
 		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
-		break;
+		return 0;
 	}
-
-	return flags;
 }
 
 static u32 hsw_infoframe_data_reg(struct dip_infoframe *frame, enum pipe pipe)
 {
-	u32 reg = 0;
-
 	switch (frame->type) {
 	case DIP_TYPE_AVI:
-		reg = HSW_TVIDEO_DIP_AVI_DATA(pipe);
-		break;
+		return HSW_TVIDEO_DIP_AVI_DATA(pipe);
 	case DIP_TYPE_SPD:
-		reg = HSW_TVIDEO_DIP_SPD_DATA(pipe);
-		break;
+		return HSW_TVIDEO_DIP_SPD_DATA(pipe);
 	default:
 		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
-		break;
+		return 0;
 	}
-
-	return reg;
 }
 
 static void g4x_write_infoframe(struct drm_encoder *encoder,
@@ -149,8 +125,6 @@ static void g4x_write_infoframe(struct drm_encoder *encoder,
 	u32 val = I915_READ(VIDEO_DIP_CTL);
 	unsigned i, len = DIP_HEADER_SIZE + frame->len;
 
-
-	/* XXX first guess at handling video port, is this corrent? */
 	val &= ~VIDEO_DIP_PORT_MASK;
 	if (intel_hdmi->sdvox_reg == SDVOB)
 		val |= VIDEO_DIP_PORT_B;
@@ -185,8 +159,7 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
 	uint32_t *data = (uint32_t *)frame;
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc = encoder->crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 	int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
 	unsigned i, len = DIP_HEADER_SIZE + frame->len;
@@ -235,8 +208,7 @@ static void cpt_write_infoframe(struct drm_encoder *encoder,
 	uint32_t *data = (uint32_t *)frame;
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc = encoder->crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	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);
@@ -270,13 +242,12 @@ static void cpt_write_infoframe(struct drm_encoder *encoder,
 }
 
 static void vlv_write_infoframe(struct drm_encoder *encoder,
-				     struct dip_infoframe *frame)
+				struct dip_infoframe *frame)
 {
 	uint32_t *data = (uint32_t *)frame;
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc = encoder->crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	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);
@@ -309,8 +280,7 @@ static void hsw_write_infoframe(struct drm_encoder *encoder,
 	uint32_t *data = (uint32_t *)frame;
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc = encoder->crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	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;
@@ -381,8 +351,7 @@ static void intel_hdmi_mode_set(struct drm_encoder *encoder,
 {
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_crtc *crtc = encoder->crtc;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 	u32 sdvox;
 
@@ -556,8 +525,8 @@ intel_hdmi_detect_audio(struct drm_connector *connector)
 
 static int
 intel_hdmi_set_property(struct drm_connector *connector,
-		      struct drm_property *property,
-		      uint64_t val)
+			struct drm_property *property,
+			uint64_t val)
 {
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 	struct drm_i915_private *dev_priv = connector->dev->dev_private;
-- 
1.7.10

Re: [PATCH 2/2] drm/i915: small hdmi coding style cleanups

[Eugeni Dodonov]

On 05/11/2012 04:48 PM, Paulo Zanoni wrote:
> From: Paulo Zanoni<paulo.r.zanoni@intel.com>
>
> - Changed the coding style of auxiliary infoframe functions to make
>    them smaller
> - Fixed the column alignment of some function definitions
> - Remove definition of "struct drm_crtc" in some places as they're
>    used only to retrieve "struct intel_crtc"
>
> Signed-off-by: Paulo Zanoni<paulo.r.zanoni@intel.com>

Except for the comments I had on the other patch:
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

Re: [PATCH 1/2] drm/i915: implement hsw_write_infoframe

[Eugeni Dodonov]

On 05/11/2012 04:48 PM, Paulo Zanoni wrote:
> From: Paulo Zanoni<paulo.r.zanoni@intel.com>
>
> Both the control and data registers are completely different now.
>
> Signed-off-by: Paulo Zanoni<paulo.r.zanoni@intel.com>

Haswell for the win! :)

Just some comments below, but nothing too critical.

> +static u32 hsw_infoframe_enable(struct dip_infoframe *frame)
> +{
> +	u32 flags = 0;
> +
> +	switch (frame->type) {
> +	case DIP_TYPE_AVI:
> +		flags |= VIDEO_DIP_ENABLE_AVI_HSW;
> +		break;
> +	case DIP_TYPE_SPD:
> +		flags |= VIDEO_DIP_ENABLE_SPD_HSW;
> +		break;
> +	default:
> +		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
> +		break;
> +	}
> +
> +	return flags;

I think it makes sense to inverse order of patches, and simplify the 
_infoframe stuff you do in your 2nd patch first; and then add this part 
with new style directly.

And I think it would be worth adding a comment with TODO saying that we 
still need to support other frames (GCP, VSC and so on). We don't have 
any use for them right now, but in the future we might. And we have all 
the registers defined already anyway.

> +}
> +
> +static u32 hsw_infoframe_data_reg(struct dip_infoframe *frame, enum pipe pipe)
> +{
> +	u32 reg = 0;
> +
> +	switch (frame->type) {
> +	case DIP_TYPE_AVI:
> +		reg = HSW_TVIDEO_DIP_AVI_DATA(pipe);
> +		break;
> +	case DIP_TYPE_SPD:
> +		reg = HSW_TVIDEO_DIP_SPD_DATA(pipe);
> +		break;
> +	default:
> +		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
> +		break;
> +	}
> +
> +	return reg;
> +}

I think you could simplify it here as well, similarly to what you do in 
your 2nd patch, and return the register directly.

>   static void hsw_write_infoframe(struct drm_encoder *encoder,
> -				     struct dip_infoframe *frame)
> +				struct dip_infoframe *frame)

I think this should go into the other patch (which simplifies tabbing 
and such), no?

But other than that, very nice!

Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

Re: [PATCH 1/2] drm/i915: implement hsw_write_infoframe

[Daniel Vetter]

On Fri, May 11, 2012 at 07:53:01PM -0300, Eugeni Dodonov wrote:
> On 05/11/2012 04:48 PM, Paulo Zanoni wrote:
> >From: Paulo Zanoni<paulo.r.zanoni@intel.com>
> >
> >Both the control and data registers are completely different now.
> >
> >Signed-off-by: Paulo Zanoni<paulo.r.zanoni@intel.com>
> 
> Haswell for the win! :)
> 
> Just some comments below, but nothing too critical.

I kinda agree with Eugeni's bikesheds - fixing up style issues in the new
code right away sounds better.
-Daniel

> 
> >+static u32 hsw_infoframe_enable(struct dip_infoframe *frame)
> >+{
> >+	u32 flags = 0;
> >+
> >+	switch (frame->type) {
> >+	case DIP_TYPE_AVI:
> >+		flags |= VIDEO_DIP_ENABLE_AVI_HSW;
> >+		break;
> >+	case DIP_TYPE_SPD:
> >+		flags |= VIDEO_DIP_ENABLE_SPD_HSW;
> >+		break;
> >+	default:
> >+		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
> >+		break;
> >+	}
> >+
> >+	return flags;
> 
> I think it makes sense to inverse order of patches, and simplify the
> _infoframe stuff you do in your 2nd patch first; and then add this
> part with new style directly.
> 
> And I think it would be worth adding a comment with TODO saying that
> we still need to support other frames (GCP, VSC and so on). We don't
> have any use for them right now, but in the future we might. And we
> have all the registers defined already anyway.
> 
> >+}
> >+
> >+static u32 hsw_infoframe_data_reg(struct dip_infoframe *frame, enum pipe pipe)
> >+{
> >+	u32 reg = 0;
> >+
> >+	switch (frame->type) {
> >+	case DIP_TYPE_AVI:
> >+		reg = HSW_TVIDEO_DIP_AVI_DATA(pipe);
> >+		break;
> >+	case DIP_TYPE_SPD:
> >+		reg = HSW_TVIDEO_DIP_SPD_DATA(pipe);
> >+		break;
> >+	default:
> >+		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
> >+		break;
> >+	}
> >+
> >+	return reg;
> >+}
> 
> I think you could simplify it here as well, similarly to what you do
> in your 2nd patch, and return the register directly.
> 
> >  static void hsw_write_infoframe(struct drm_encoder *encoder,
> >-				     struct dip_infoframe *frame)
> >+				struct dip_infoframe *frame)
> 
> I think this should go into the other patch (which simplifies
> tabbing and such), no?
> 
> But other than that, very nice!
> 
> Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48