[PATCH 01/12] drm/i915: enable dip before writing data on gen4

[PATCH 01/12] drm/i915: enable dip before writing data on gen4

[Paulo Zanoni]

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

While testing with the intel_infoframes tool on gen4, I see that when
video DIP is disabled, what we write to the DATA memory is not exactly
what we read back later. This should fix some problems that can be
bisected to "drm/i915: fix ILK+ infoframe support". That commit was
setting VIDEO_DIP_CTL to 0 when initializing, which caused the problem.
Fixes fd.o bug 43947.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=43947
Cc: stable@kernel.org

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 7de2d3b..1eef50d 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -136,7 +136,7 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
 
 	val &= ~VIDEO_DIP_SELECT_MASK;
 
-	I915_WRITE(VIDEO_DIP_CTL, val | port | flags);
+	I915_WRITE(VIDEO_DIP_CTL, VIDEO_DIP_ENABLE | val | port | flags);
 
 	for (i = 0; i < len; i += 4) {
 		I915_WRITE(VIDEO_DIP_DATA, *data);
-- 
1.7.10

[PATCH 02/12] drm/i915: DSL_LINEMASK is 12 bits only on gen2

[Paulo Zanoni]

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

Gen3+ is 13 bits (12:0).

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7bc407a..8da0b40 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2475,7 +2475,8 @@
 
 /* Pipe A */
 #define _PIPEADSL		0x70000
-#define   DSL_LINEMASK		0x00000fff
+#define   DSL_LINEMASK_GEN2	0x00000fff
+#define   DSL_LINEMASK_GEN3	0x00001fff
 #define _PIPEACONF		0x70008
 #define   PIPECONF_ENABLE	(1<<31)
 #define   PIPECONF_DISABLE	0
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e1716be..613f871 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -871,15 +871,20 @@ void intel_wait_for_pipe_off(struct drm_device *dev, int pipe)
 			     100))
 			DRM_DEBUG_KMS("pipe_off wait timed out\n");
 	} else {
-		u32 last_line;
+		u32 last_line, line_mask;
 		int reg = PIPEDSL(pipe);
 		unsigned long timeout = jiffies + msecs_to_jiffies(100);
 
+		if (IS_GEN2(dev))
+			line_mask = DSL_LINEMASK_GEN2;
+		else
+			line_mask = DSL_LINEMASK_GEN3;
+
 		/* Wait for the display line to settle */
 		do {
-			last_line = I915_READ(reg) & DSL_LINEMASK;
+			last_line = I915_READ(reg) & line_mask;
 			mdelay(5);
-		} while (((I915_READ(reg) & DSL_LINEMASK) != last_line) &&
+		} while (((I915_READ(reg) & line_mask) != last_line) &&
 			 time_after(timeout, jiffies));
 		if (time_after(jiffies, timeout))
 			DRM_DEBUG_KMS("pipe_off wait timed out\n");
-- 
1.7.10

Re: [PATCH 02/12] drm/i915: DSL_LINEMASK is 12 bits only on gen2

[Chris Wilson]

On Wed,  2 May 2012 22:55:44 -0300, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Gen3+ is 13 bits (12:0).

Gen3+ is 13 bits (12:0), and on gen2 only (11:0). For both the high bits
are marked reserved, read-only so continue to mask them.

> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 03/12] drm/i915: implement ironlake_wait_for_vblank

[Paulo Zanoni]

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

intel_wait_for_vblank uses PIPESTAT, which does not exist on Ironlake
and newer.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 613f871..a2617b2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -806,6 +806,32 @@ intel_find_pll_g4x_dp(const intel_limit_t *limit, struct drm_crtc *crtc,
 	return true;
 }
 
+static void ironlake_wait_for_vblank(struct drm_device *dev, int pipe)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 vblank_start, line;
+	u32 dsl_reg = PIPEDSL(pipe);
+	u32 pipeconf = I915_READ(PIPECONF(pipe));
+
+	if (!((pipeconf & PIPECONF_ENABLE) &&
+	      (pipeconf & I965_PIPECONF_ACTIVE)))
+		return;
+
+	vblank_start = I915_READ(VBLANK(pipe)) & 0x1FFF;
+
+	if (pipeconf & PIPECONF_INTERLACE_MASK)
+		vblank_start >>= 1;
+
+	line = I915_READ(dsl_reg) & DSL_LINEMASK_GEN3;
+
+	if (line >= vblank_start)
+		return;
+
+	if (wait_for_atomic_us((I915_READ_NOTRACE(dsl_reg) &
+			       DSL_LINEMASK_GEN3) >= vblank_start, 50000))
+		DRM_DEBUG_KMS("vblank wait timed out\n");
+}
+
 /**
  * intel_wait_for_vblank - wait for vblank on a given pipe
  * @dev: drm device
@@ -819,6 +845,11 @@ void intel_wait_for_vblank(struct drm_device *dev, int pipe)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int pipestat_reg = PIPESTAT(pipe);
 
+	if (INTEL_INFO(dev)->gen >= 5) {
+		ironlake_wait_for_vblank(dev, pipe);
+		return;
+	}
+
 	/* Clear existing vblank status. Note this will clear any other
 	 * sticky status fields as well.
 	 *
-- 
1.7.10

Re: [PATCH 03/12] drm/i915: implement ironlake_wait_for_vblank

[Chris Wilson]

On Wed,  2 May 2012 22:55:45 -0300, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> intel_wait_for_vblank uses PIPESTAT, which does not exist on Ironlake
> and newer.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 613f871..a2617b2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -806,6 +806,32 @@ intel_find_pll_g4x_dp(const intel_limit_t *limit, struct drm_crtc *crtc,
>  	return true;
>  }
>  
> +static void ironlake_wait_for_vblank(struct drm_device *dev, int pipe)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 vblank_start, line;
> +	u32 dsl_reg = PIPEDSL(pipe);
> +	u32 pipeconf = I915_READ(PIPECONF(pipe));
> +
> +	if (!((pipeconf & PIPECONF_ENABLE) &&
> +	      (pipeconf & I965_PIPECONF_ACTIVE)))
> +		return;
> +
> +	vblank_start = I915_READ(VBLANK(pipe)) & 0x1FFF;
> +
> +	if (pipeconf & PIPECONF_INTERLACE_MASK)
> +		vblank_start >>= 1;
> +
> +	line = I915_READ(dsl_reg) & DSL_LINEMASK_GEN3;
> +
> +	if (line >= vblank_start)
> +		return;

The caller expects for at least 1 frame to have passed, the
documentation tends to refer to the registers being latched until the
next vblank. As we don't know precisely when those registers are copied,
we need to wait one complete cycle to be sure that we don't return too
early.

Or maybe I'm being too paranoid. Actually, with modesetting one can
never be too paranoid.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 04/12] drm/i915: touch the DIP control register after enabling the HDMI port

[Paulo Zanoni]

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

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

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 1eef50d..8646a50 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -113,6 +113,20 @@ static u32 intel_infoframe_flags(struct dip_infoframe *frame)
 	return flags;
 }
 
+static u32 intel_get_dip_ctl_reg(struct drm_encoder *encoder)
+{
+	struct drm_device *dev = encoder->dev;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
+
+	if (!HAS_PCH_SPLIT(dev)) {
+		return VIDEO_DIP_CTL;
+	} else if (IS_VALLEYVIEW(dev)) {
+		return VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
+	} else {
+		return TVIDEO_DIP_CTL(intel_crtc->pipe);
+	}
+}
+
 static void i9xx_write_infoframe(struct drm_encoder *encoder,
 				 struct dip_infoframe *frame)
 {
@@ -331,6 +345,14 @@ static void intel_hdmi_dpms(struct drm_encoder *encoder, int mode)
 		I915_WRITE(intel_hdmi->sdvox_reg, temp);
 		POSTING_READ(intel_hdmi->sdvox_reg);
 	}
+
+	if ((mode == DRM_MODE_DPMS_ON) && encoder->crtc) {
+		/* We've just enabled the HDMI port. Now we need to touch the
+		 * DIP control register to make sure the infoframes are sent.
+		 */
+		u32 dip_reg = intel_get_dip_ctl_reg(encoder);
+		I915_WRITE(dip_reg, I915_READ(dip_reg));
+	}
 }
 
 static int intel_hdmi_mode_valid(struct drm_connector *connector,
-- 
1.7.10

[PATCH 05/12] drm/i915: change coding style of the write_infoframe functions

[Paulo Zanoni]

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

Don't use intermediate variables, change the value of 'val' as we go
through the function. The new style looks more similar to the rest of
our code. IMHO, it's also easier to read and change.

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

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 8646a50..612d9ed 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -134,32 +134,33 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
-	u32 port, flags, val = I915_READ(VIDEO_DIP_CTL);
+	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? */
 	if (intel_hdmi->sdvox_reg == SDVOB)
-		port = VIDEO_DIP_PORT_B;
+		val |= VIDEO_DIP_PORT_B;
 	else if (intel_hdmi->sdvox_reg == SDVOC)
-		port = VIDEO_DIP_PORT_C;
+		val |= VIDEO_DIP_PORT_C;
 	else
 		return;
 
-	flags = intel_infoframe_index(frame);
-
 	val &= ~VIDEO_DIP_SELECT_MASK;
+	val |= intel_infoframe_index(frame);
+
+	val |= VIDEO_DIP_ENABLE;
 
-	I915_WRITE(VIDEO_DIP_CTL, VIDEO_DIP_ENABLE | val | port | flags);
+	I915_WRITE(VIDEO_DIP_CTL, val);
 
 	for (i = 0; i < len; i += 4) {
 		I915_WRITE(VIDEO_DIP_DATA, *data);
 		data++;
 	}
 
-	flags |= intel_infoframe_flags(frame);
+	val |= intel_infoframe_flags(frame);
 
-	I915_WRITE(VIDEO_DIP_CTL, VIDEO_DIP_ENABLE | val | port | flags);
+	I915_WRITE(VIDEO_DIP_CTL, val);
 }
 
 static void ironlake_write_infoframe(struct drm_encoder *encoder,
@@ -172,24 +173,25 @@ static void ironlake_write_infoframe(struct drm_encoder *encoder,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
 	unsigned i, len = DIP_HEADER_SIZE + frame->len;
-	u32 flags, val = I915_READ(reg);
+	u32 val = I915_READ(reg);
 
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
-	flags = intel_infoframe_index(frame);
-
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
+	val |= intel_infoframe_index(frame);
 
-	I915_WRITE(reg, VIDEO_DIP_ENABLE | val | flags);
+	val |= VIDEO_DIP_ENABLE;
+
+	I915_WRITE(reg, val);
 
 	for (i = 0; i < len; i += 4) {
 		I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
 		data++;
 	}
 
-	flags |= intel_infoframe_flags(frame);
+	val |= intel_infoframe_flags(frame);
 
-	I915_WRITE(reg, VIDEO_DIP_ENABLE | val | flags);
+	I915_WRITE(reg, val);
 }
 
 static void vlv_write_infoframe(struct drm_encoder *encoder,
@@ -202,24 +204,25 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
 	unsigned i, len = DIP_HEADER_SIZE + frame->len;
-	u32 flags, val = I915_READ(reg);
+	u32 val = I915_READ(reg);
 
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
-	flags = intel_infoframe_index(frame);
-
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
+	val |= intel_infoframe_index(frame);
+
+	val |= VIDEO_DIP_ENABLE;
 
-	I915_WRITE(reg, VIDEO_DIP_ENABLE | val | flags);
+	I915_WRITE(reg, val);
 
 	for (i = 0; i < len; i += 4) {
 		I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
 		data++;
 	}
 
-	flags |= intel_infoframe_flags(frame);
+	val |= intel_infoframe_flags(frame);
 
-	I915_WRITE(reg, VIDEO_DIP_ENABLE | val | flags);
+	I915_WRITE(reg, val);
 }
 
 static void intel_set_infoframe(struct drm_encoder *encoder,
-- 
1.7.10

[PATCH 06/12] drm/i915: start writing infoframes at address 0 on gen 4

[Paulo Zanoni]

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

Make sure we're doing the right thing, just like we do on gen5+.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 612d9ed..397299b 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -146,7 +146,7 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
 	else
 		return;
 
-	val &= ~VIDEO_DIP_SELECT_MASK;
+	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= intel_infoframe_index(frame);
 
 	val |= VIDEO_DIP_ENABLE;
-- 
1.7.10

[PATCH 07/12] drm/i915: mask the video DIP port select

[Paulo Zanoni]

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

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8da0b40..cc0b90c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1699,6 +1699,7 @@
 #define   VIDEO_DIP_ENABLE		(1 << 31)
 #define   VIDEO_DIP_PORT_B		(1 << 29)
 #define   VIDEO_DIP_PORT_C		(2 << 29)
+#define   VIDEO_DIP_PORT_MASK		(3 << 29)
 #define   VIDEO_DIP_ENABLE_AVI		(1 << 21)
 #define   VIDEO_DIP_ENABLE_VENDOR	(2 << 21)
 #define   VIDEO_DIP_ENABLE_SPD		(8 << 21)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 397299b..4ce5f1f 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -139,6 +139,7 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
 
 
 	/* 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;
 	else if (intel_hdmi->sdvox_reg == SDVOC)
-- 
1.7.10

Re: [PATCH 07/12] drm/i915: mask the video DIP port select

[Eugeni Dodonov]

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

On Wed, May 2, 2012 at 10:55 PM, Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>

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

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 770 bytes --]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/12] drm/i915: break intel_infoframe_flags into _enable and _index

[Paulo Zanoni]

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

This will allow us to disable an infoframe without changing its
frequency.

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

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 4ce5f1f..57ab62f 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -94,16 +94,33 @@ static u32 intel_infoframe_index(struct dip_infoframe *frame)
 	return flags;
 }
 
-static u32 intel_infoframe_flags(struct dip_infoframe *frame)
+static u32 intel_infoframe_enable(struct dip_infoframe *frame)
 {
 	u32 flags = 0;
 
 	switch (frame->type) {
 	case DIP_TYPE_AVI:
-		flags |= VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_FREQ_VSYNC;
+		flags |= VIDEO_DIP_ENABLE_AVI;
 		break;
 	case DIP_TYPE_SPD:
-		flags |= VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_FREQ_VSYNC;
+		flags |= VIDEO_DIP_ENABLE_SPD;
+		break;
+	default:
+		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
+		break;
+	}
+
+	return flags;
+}
+
+static u32 intel_infoframe_frequency(struct dip_infoframe *frame)
+{
+	u32 flags = 0;
+
+	switch (frame->type) {
+	case DIP_TYPE_AVI:
+	case DIP_TYPE_SPD:
+		flags |= VIDEO_DIP_FREQ_VSYNC;
 		break;
 	default:
 		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
@@ -159,7 +176,8 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
 		data++;
 	}
 
-	val |= intel_infoframe_flags(frame);
+	val |= intel_infoframe_enable(frame);
+	val |= intel_infoframe_frequency(frame);
 
 	I915_WRITE(VIDEO_DIP_CTL, val);
 }
@@ -190,7 +208,8 @@ static void ironlake_write_infoframe(struct drm_encoder *encoder,
 		data++;
 	}
 
-	val |= intel_infoframe_flags(frame);
+	val |= intel_infoframe_enable(frame);
+	val |= intel_infoframe_frequency(frame);
 
 	I915_WRITE(reg, val);
 }
@@ -221,7 +240,8 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
 		data++;
 	}
 
-	val |= intel_infoframe_flags(frame);
+	val |= intel_infoframe_enable(frame);
+	val |= intel_infoframe_frequency(frame);
 
 	I915_WRITE(reg, val);
 }
-- 
1.7.10

Re: [PATCH 08/12] drm/i915: break intel_infoframe_flags into _enable and _index

[Eugeni Dodonov]

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

On Wed, May 2, 2012 at 10:55 PM, Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> This will allow us to disable an infoframe without changing its
> frequency.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>

I think the subject should be 'Break intel_infoframe_flags into _enable and
_frequency', no?

And by looking at what this does, perhaps the 2nd function could be named
intel_infoframe_freq_vsync or intel_infoframe_vsync instead?

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

-- 
Eugeni Dodonov
 <http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 1240 bytes --]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/12] drm/i915: break intel_infoframe_flags into _enable and _index

[Paulo Zanoni]

> I think the subject should be 'Break intel_infoframe_flags into _enable and
> _frequency', no?

Good catch. I can swear I fixed this before...

> And by looking at what this does, perhaps the 2nd function could be named
> intel_infoframe_freq_vsync or intel_infoframe_vsync instead?


Well, bit 18 is called "Video DIP frequency". And the frequency can be
"Once", "Every Vsync" or "At least every other VSync", so I guess
there's no need in putting the "vsync" to the name...

Thank you,
Paulo

-- 
Paulo Zanoni

[PATCH 09/12] drm/i915: disable the infoframe before changing it

[Paulo Zanoni]

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

That's what the VIDEO_DIP_CTL documentation says we need to do. Except
when it's the AVI InfoFrame and we're ironlake_write_infoframe.

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

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 57ab62f..0fc8fab 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -167,6 +167,7 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= intel_infoframe_index(frame);
 
+	val &= ~intel_infoframe_enable(frame);
 	val |= VIDEO_DIP_ENABLE;
 
 	I915_WRITE(VIDEO_DIP_CTL, val);
@@ -199,6 +200,11 @@ static void ironlake_write_infoframe(struct drm_encoder *encoder,
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= intel_infoframe_index(frame);
 
+	if (frame->type == DIP_TYPE_AVI)
+		val |= VIDEO_DIP_ENABLE_AVI;
+	else
+		val &= ~intel_infoframe_enable(frame);
+
 	val |= VIDEO_DIP_ENABLE;
 
 	I915_WRITE(reg, val);
@@ -231,6 +237,7 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= intel_infoframe_index(frame);
 
+	val &= ~intel_infoframe_enable(frame);
 	val |= VIDEO_DIP_ENABLE;
 
 	I915_WRITE(reg, val);
-- 
1.7.10

Re: [PATCH 09/12] drm/i915: disable the infoframe before changing it

[Eugeni Dodonov]

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

On Wed, May 2, 2012 at 10:55 PM, Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> That's what the VIDEO_DIP_CTL documentation says we need to do. Except
> when it's the AVI InfoFrame and we're ironlake_write_infoframe.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>

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


>        val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
>        val |= intel_infoframe_index(frame);
>
> +       if (frame->type == DIP_TYPE_AVI)
> +               val |= VIDEO_DIP_ENABLE_AVI;
> +       else
> +               val &= ~intel_infoframe_enable(frame);
> +
>

Maybe adding a comment here saying why we have this on Ironlake as well?

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 1547 bytes --]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/12] drm/i915: mask the video DIP frequency when changing it

[Paulo Zanoni]

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

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cc0b90c..6e03732 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1710,6 +1710,7 @@
 #define   VIDEO_DIP_FREQ_ONCE		(0 << 16)
 #define   VIDEO_DIP_FREQ_VSYNC		(1 << 16)
 #define   VIDEO_DIP_FREQ_2VSYNC		(2 << 16)
+#define   VIDEO_DIP_FREQ_MASK		(3 << 16)
 
 /* 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 0fc8fab..c64f283 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -178,6 +178,7 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
 	}
 
 	val |= intel_infoframe_enable(frame);
+	val &= ~VIDEO_DIP_FREQ_MASK;
 	val |= intel_infoframe_frequency(frame);
 
 	I915_WRITE(VIDEO_DIP_CTL, val);
@@ -215,6 +216,7 @@ static void ironlake_write_infoframe(struct drm_encoder *encoder,
 	}
 
 	val |= intel_infoframe_enable(frame);
+	val &= ~VIDEO_DIP_FREQ_MASK;
 	val |= intel_infoframe_frequency(frame);
 
 	I915_WRITE(reg, val);
@@ -248,6 +250,7 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
 	}
 
 	val |= intel_infoframe_enable(frame);
+	val &= ~VIDEO_DIP_FREQ_MASK;
 	val |= intel_infoframe_frequency(frame);
 
 	I915_WRITE(reg, val);
-- 
1.7.10

Re: [PATCH 10/12] drm/i915: mask the video DIP frequency when changing it

[Eugeni Dodonov]

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

On Wed, May 2, 2012 at 10:55 PM, Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>

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

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 781 bytes --]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/12] drm/i915: split ironlake_write_infoframe into ibx_ and cpt_

[Paulo Zanoni]

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

1 - The registers are on the PCH, so don't use the Gen number
2 - IBX has a port select (like Gen 4, but ports are different)
3 - CPT needs a workaround when enabling the AVI Infoframe

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6e03732..f715672 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1699,6 +1699,7 @@
 #define   VIDEO_DIP_ENABLE		(1 << 31)
 #define   VIDEO_DIP_PORT_B		(1 << 29)
 #define   VIDEO_DIP_PORT_C		(2 << 29)
+#define   VIDEO_DIP_PORT_D		(3 << 29)
 #define   VIDEO_DIP_PORT_MASK		(3 << 29)
 #define   VIDEO_DIP_ENABLE_AVI		(1 << 21)
 #define   VIDEO_DIP_ENABLE_VENDOR	(2 << 21)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index c64f283..8d397cb 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -184,8 +184,59 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
 	I915_WRITE(VIDEO_DIP_CTL, val);
 }
 
-static void ironlake_write_infoframe(struct drm_encoder *encoder,
-				     struct dip_infoframe *frame)
+static void ibx_write_infoframe(struct drm_encoder *encoder,
+				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_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
+	int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
+	unsigned i, len = DIP_HEADER_SIZE + frame->len;
+	u32 val = I915_READ(reg);
+
+	val &= ~VIDEO_DIP_PORT_MASK;
+	switch (intel_hdmi->sdvox_reg) {
+	case HDMIB:
+		val |= VIDEO_DIP_PORT_B;
+		break;
+	case HDMIC:
+		val |= VIDEO_DIP_PORT_C;
+		break;
+	case HDMID:
+		val |= VIDEO_DIP_PORT_D;
+		break;
+	default:
+		return;
+	}
+
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
+
+	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
+	val |= intel_infoframe_index(frame);
+
+	val &= ~intel_infoframe_enable(frame);
+
+	val |= VIDEO_DIP_ENABLE;
+
+	I915_WRITE(reg, val);
+
+	for (i = 0; i < len; i += 4) {
+		I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
+		data++;
+	}
+
+	val |= intel_infoframe_enable(frame);
+	val &= ~VIDEO_DIP_FREQ_MASK;
+	val |= intel_infoframe_frequency(frame);
+
+	I915_WRITE(reg, val);
+}
+
+static void cpt_write_infoframe(struct drm_encoder *encoder,
+				struct dip_infoframe *frame)
 {
 	uint32_t *data = (uint32_t *)frame;
 	struct drm_device *dev = encoder->dev;
@@ -643,8 +694,12 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg)
 		intel_hdmi->write_infoframe = vlv_write_infoframe;
 		for_each_pipe(i)
 			I915_WRITE(VLV_TVIDEO_DIP_CTL(i), 0);
-	}  else {
-		intel_hdmi->write_infoframe = ironlake_write_infoframe;
+	} else if (HAS_PCH_IBX(dev)) {
+		intel_hdmi->write_infoframe = ibx_write_infoframe;
+		for_each_pipe(i)
+			I915_WRITE(TVIDEO_DIP_CTL(i), 0);
+	} else {
+		intel_hdmi->write_infoframe = cpt_write_infoframe;
 		for_each_pipe(i)
 			I915_WRITE(TVIDEO_DIP_CTL(i), 0);
 	}
-- 
1.7.10

[PATCH 12/12] drm/i915: simplify intel_encoder_commit

[Paulo Zanoni]

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

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a2617b2..f807d36 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3197,8 +3197,7 @@ void intel_encoder_commit(struct drm_encoder *encoder)
 {
 	struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
 	struct drm_device *dev = encoder->dev;
-	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
-	struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
 
 	/* lvds has its own version of commit see intel_lvds_commit */
 	encoder_funcs->dpms(encoder, DRM_MODE_DPMS_ON);
-- 
1.7.10

Re: [PATCH 01/12] drm/i915: enable dip before writing data on gen4

[Daniel Vetter]

On Wed, May 02, 2012 at 10:55:43PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> While testing with the intel_infoframes tool on gen4, I see that when
> video DIP is disabled, what we write to the DATA memory is not exactly
> what we read back later. This should fix some problems that can be
> bisected to "drm/i915: fix ILK+ infoframe support". That commit was
> setting VIDEO_DIP_CTL to 0 when initializing, which caused the problem.
> Fixes fd.o bug 43947.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=43947
> Cc: stable@kernel.org
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Picked up for -fixes, thanks for the patch. I've changed the commit
message a bit to make it clearer that this fixes a regression and to make
the offending commit stand out more.

For consistency when citing a commit that introduced a regression, please
paste the output of git show up to the headline of the commit.

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

Re: [PATCH 01/12] drm/i915: enable dip before writing data on gen4

[Eugeni Dodonov]

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

On Wed, May 2, 2012 at 10:55 PM, Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> While testing with the intel_infoframes tool on gen4, I see that when
> video DIP is disabled, what we write to the DATA memory is not exactly
> what we read back later. This should fix some problems that can be
> bisected to "drm/i915: fix ILK+ infoframe support". That commit was
> setting VIDEO_DIP_CTL to 0 when initializing, which caused the problem.
> Fixes fd.o bug 43947.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=43947
> Cc: stable@kernel.org
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>

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

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 1405 bytes --]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/14] drm/i915: DSL_LINEMASK is 12 bits only on gen2

[Paulo Zanoni]

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

Gen3+ is 13 bits (12:0), and on gen2 only 12 (11:0). For both the high
bits are marked reserved, read-only so continue to mask them. Bit 31
is not reserved and has a meaning.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_reg.h      |    3 ++-
 drivers/gpu/drm/i915/intel_display.c |   11 ++++++++---
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7bc407a..8da0b40 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2475,7 +2475,8 @@
 
 /* Pipe A */
 #define _PIPEADSL		0x70000
-#define   DSL_LINEMASK		0x00000fff
+#define   DSL_LINEMASK_GEN2	0x00000fff
+#define   DSL_LINEMASK_GEN3	0x00001fff
 #define _PIPEACONF		0x70008
 #define   PIPECONF_ENABLE	(1<<31)
 #define   PIPECONF_DISABLE	0
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e1716be..613f871 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -871,15 +871,20 @@ void intel_wait_for_pipe_off(struct drm_device *dev, int pipe)
 			     100))
 			DRM_DEBUG_KMS("pipe_off wait timed out\n");
 	} else {
-		u32 last_line;
+		u32 last_line, line_mask;
 		int reg = PIPEDSL(pipe);
 		unsigned long timeout = jiffies + msecs_to_jiffies(100);
 
+		if (IS_GEN2(dev))
+			line_mask = DSL_LINEMASK_GEN2;
+		else
+			line_mask = DSL_LINEMASK_GEN3;
+
 		/* Wait for the display line to settle */
 		do {
-			last_line = I915_READ(reg) & DSL_LINEMASK;
+			last_line = I915_READ(reg) & line_mask;
 			mdelay(5);
-		} while (((I915_READ(reg) & DSL_LINEMASK) != last_line) &&
+		} while (((I915_READ(reg) & line_mask) != last_line) &&
 			 time_after(timeout, jiffies));
 		if (time_after(jiffies, timeout))
 			DRM_DEBUG_KMS("pipe_off wait timed out\n");
-- 
1.7.10

[PATCH 03/14] drm/i915: implement ironlake_wait_for_vblank

[Paulo Zanoni]

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

intel_wait_for_vblank uses PIPESTAT, which does not exist on Ironlake
and newer, so now we use PIPEFRAME.

There's also a check to see if the pipe is stopped.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 613f871..5036efe 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -806,6 +806,22 @@ intel_find_pll_g4x_dp(const intel_limit_t *limit, struct drm_crtc *crtc,
 	return true;
 }
 
+static void ironlake_wait_for_vblank(struct drm_device *dev, int pipe)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 frame, frame_reg = PIPEFRAME(pipe);
+	u32 pipeconf = I915_READ(PIPECONF(pipe));
+
+	if (!((pipeconf & PIPECONF_ENABLE) &&
+	      (pipeconf & I965_PIPECONF_ACTIVE)))
+		return;
+
+	frame = I915_READ(frame_reg);
+
+	if (wait_for(I915_READ_NOTRACE(frame_reg) != frame, 50))
+		DRM_DEBUG_KMS("vblank wait timed out\n");
+}
+
 /**
  * intel_wait_for_vblank - wait for vblank on a given pipe
  * @dev: drm device
@@ -819,6 +835,11 @@ void intel_wait_for_vblank(struct drm_device *dev, int pipe)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int pipestat_reg = PIPESTAT(pipe);
 
+	if (INTEL_INFO(dev)->gen >= 5) {
+		ironlake_wait_for_vblank(dev, pipe);
+		return;
+	}
+
 	/* Clear existing vblank status. Note this will clear any other
 	 * sticky status fields as well.
 	 *
-- 
1.7.10

Re: [PATCH 03/14] drm/i915: implement ironlake_wait_for_vblank

[Daniel Vetter]

On Fri, May 04, 2012 at 05:18:15PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> intel_wait_for_vblank uses PIPESTAT, which does not exist on Ironlake
> and newer, so now we use PIPEFRAME.
> 
> There's also a check to see if the pipe is stopped.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 613f871..5036efe 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -806,6 +806,22 @@ intel_find_pll_g4x_dp(const intel_limit_t *limit, struct drm_crtc *crtc,
>  	return true;
>  }
>  
> +static void ironlake_wait_for_vblank(struct drm_device *dev, int pipe)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	u32 frame, frame_reg = PIPEFRAME(pipe);
> +	u32 pipeconf = I915_READ(PIPECONF(pipe));
> +
> +	if (!((pipeconf & PIPECONF_ENABLE) &&
> +	      (pipeconf & I965_PIPECONF_ACTIVE)))
> +		return;

Hm, why do we have this check here? The other wait_for_vblank
implementation doesn't bother with this. And if we call wait_for_vblank on
a disabled pipe, that's a bug imo.
-Daniel

> +
> +	frame = I915_READ(frame_reg);
> +
> +	if (wait_for(I915_READ_NOTRACE(frame_reg) != frame, 50))
> +		DRM_DEBUG_KMS("vblank wait timed out\n");
> +}
> +
>  /**
>   * intel_wait_for_vblank - wait for vblank on a given pipe
>   * @dev: drm device
> @@ -819,6 +835,11 @@ void intel_wait_for_vblank(struct drm_device *dev, int pipe)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int pipestat_reg = PIPESTAT(pipe);
>  
> +	if (INTEL_INFO(dev)->gen >= 5) {
> +		ironlake_wait_for_vblank(dev, pipe);
> +		return;
> +	}
> +
>  	/* Clear existing vblank status. Note this will clear any other
>  	 * sticky status fields as well.
>  	 *
> -- 
> 1.7.10
> 
> _______________________________________________
> 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

Re: [PATCH 03/14] drm/i915: implement ironlake_wait_for_vblank

[Paulo Zanoni]

2012/5/8 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, May 04, 2012 at 05:18:15PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> intel_wait_for_vblank uses PIPESTAT, which does not exist on Ironlake
>> and newer, so now we use PIPEFRAME.
>>
>> There's also a check to see if the pipe is stopped.
>
> Hm, why do we have this check here? The other wait_for_vblank
> implementation doesn't bother with this. And if we call wait_for_vblank on
> a disabled pipe, that's a bug imo.
> -Daniel

Because not checking for a stopped pipe adds a useless 50ms delay
(it's the wait_for() timeout). We're currently calling
wait_for_vblank() on disabled pipes.
- Maybe I should add a DRM_DEBUG_KMS message there and then start
fixing these cases?
- Maybe I should move the check to intel_wait_for_vblank (including
the debug message)?

-- 
Paulo Zanoni

Re: [PATCH 03/14] drm/i915: implement ironlake_wait_for_vblank

[Daniel Vetter]

On Tue, May 08, 2012 at 09:34:18AM -0300, Paulo Zanoni wrote:
> 2012/5/8 Daniel Vetter <daniel@ffwll.ch>:
> > On Fri, May 04, 2012 at 05:18:15PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> intel_wait_for_vblank uses PIPESTAT, which does not exist on Ironlake
> >> and newer, so now we use PIPEFRAME.
> >>
> >> There's also a check to see if the pipe is stopped.
> >
> > Hm, why do we have this check here? The other wait_for_vblank
> > implementation doesn't bother with this. And if we call wait_for_vblank on
> > a disabled pipe, that's a bug imo.
> > -Daniel
> 
> Because not checking for a stopped pipe adds a useless 50ms delay
> (it's the wait_for() timeout). We're currently calling
> wait_for_vblank() on disabled pipes.
> - Maybe I should add a DRM_DEBUG_KMS message there and then start
> fixing these cases?
> - Maybe I should move the check to intel_wait_for_vblank (including
> the debug message)?

Either is fine, the thing that irked was just that now the semantics of
wait_for_vblank changed depending upon which chipset it runs on:
- for ilk, it will immediately return if the pipe is off
- for pre-ilk, it will time out for 50 ms, complain with a debug message
  and then return.

I'm leaning towards fixing up the callers to not call wait_for_vblank when
the pipe is off (and ditching the check from the ilk wait_for_vblank).
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 03/14] drm/i915: implement ironlake_wait_for_vblank

[Daniel Vetter]

On Tue, May 08, 2012 at 02:44:24PM +0200, Daniel Vetter wrote:
> On Tue, May 08, 2012 at 09:34:18AM -0300, Paulo Zanoni wrote:
> > 2012/5/8 Daniel Vetter <daniel@ffwll.ch>:
> > > On Fri, May 04, 2012 at 05:18:15PM -0300, Paulo Zanoni wrote:
> > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >>
> > >> intel_wait_for_vblank uses PIPESTAT, which does not exist on Ironlake
> > >> and newer, so now we use PIPEFRAME.
> > >>
> > >> There's also a check to see if the pipe is stopped.
> > >
> > > Hm, why do we have this check here? The other wait_for_vblank
> > > implementation doesn't bother with this. And if we call wait_for_vblank on
> > > a disabled pipe, that's a bug imo.
> > > -Daniel
> > 
> > Because not checking for a stopped pipe adds a useless 50ms delay
> > (it's the wait_for() timeout). We're currently calling
> > wait_for_vblank() on disabled pipes.
> > - Maybe I should add a DRM_DEBUG_KMS message there and then start
> > fixing these cases?
> > - Maybe I should move the check to intel_wait_for_vblank (including
> > the debug message)?
> 
> Either is fine, the thing that irked was just that now the semantics of
> wait_for_vblank changed depending upon which chipset it runs on:
> - for ilk, it will immediately return if the pipe is off
> - for pre-ilk, it will time out for 50 ms, complain with a debug message
>   and then return.
> 
> I'm leaning towards fixing up the callers to not call wait_for_vblank when
> the pipe is off (and ditching the check from the ilk wait_for_vblank).

Paulo acked this on irc, so I've queued the patch for -next with the pipe
check removed, thanks.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 04/14] drm/i915: touch the DIP control register after enabling the HDMI port

[Paulo Zanoni]

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

This is not documented anywhere, but it seems necessary to make the
InfoFrames work, especially when all you have is an HDMI monitor.

Some bugs get fixed just by running "./intel_infoframes -d". This
patch fixes this problem on my machine.

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

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 1eef50d..8646a50 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -113,6 +113,20 @@ static u32 intel_infoframe_flags(struct dip_infoframe *frame)
 	return flags;
 }
 
+static u32 intel_get_dip_ctl_reg(struct drm_encoder *encoder)
+{
+	struct drm_device *dev = encoder->dev;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
+
+	if (!HAS_PCH_SPLIT(dev)) {
+		return VIDEO_DIP_CTL;
+	} else if (IS_VALLEYVIEW(dev)) {
+		return VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
+	} else {
+		return TVIDEO_DIP_CTL(intel_crtc->pipe);
+	}
+}
+
 static void i9xx_write_infoframe(struct drm_encoder *encoder,
 				 struct dip_infoframe *frame)
 {
@@ -331,6 +345,14 @@ static void intel_hdmi_dpms(struct drm_encoder *encoder, int mode)
 		I915_WRITE(intel_hdmi->sdvox_reg, temp);
 		POSTING_READ(intel_hdmi->sdvox_reg);
 	}
+
+	if ((mode == DRM_MODE_DPMS_ON) && encoder->crtc) {
+		/* We've just enabled the HDMI port. Now we need to touch the
+		 * DIP control register to make sure the infoframes are sent.
+		 */
+		u32 dip_reg = intel_get_dip_ctl_reg(encoder);
+		I915_WRITE(dip_reg, I915_READ(dip_reg));
+	}
 }
 
 static int intel_hdmi_mode_valid(struct drm_connector *connector,
-- 
1.7.10

Re: [PATCH 04/14] drm/i915: touch the DIP control register after enabling the HDMI port

[Daniel Vetter]

On Fri, May 04, 2012 at 05:18:16PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This is not documented anywhere, but it seems necessary to make the
> InfoFrames work, especially when all you have is an HDMI monitor.
> 
> Some bugs get fixed just by running "./intel_infoframes -d". This
> patch fixes this problem on my machine.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Can you elaborate a bit on which machines really need this? I fear that
we're adding a hack here which is only required on a few chips and then
keep it around forever ...
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |   22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 1eef50d..8646a50 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -113,6 +113,20 @@ static u32 intel_infoframe_flags(struct dip_infoframe *frame)
>  	return flags;
>  }
>  
> +static u32 intel_get_dip_ctl_reg(struct drm_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->dev;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> +
> +	if (!HAS_PCH_SPLIT(dev)) {
> +		return VIDEO_DIP_CTL;
> +	} else if (IS_VALLEYVIEW(dev)) {
> +		return VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
> +	} else {
> +		return TVIDEO_DIP_CTL(intel_crtc->pipe);
> +	}
> +}
> +
>  static void i9xx_write_infoframe(struct drm_encoder *encoder,
>  				 struct dip_infoframe *frame)
>  {
> @@ -331,6 +345,14 @@ static void intel_hdmi_dpms(struct drm_encoder *encoder, int mode)
>  		I915_WRITE(intel_hdmi->sdvox_reg, temp);
>  		POSTING_READ(intel_hdmi->sdvox_reg);
>  	}
> +
> +	if ((mode == DRM_MODE_DPMS_ON) && encoder->crtc) {
> +		/* We've just enabled the HDMI port. Now we need to touch the
> +		 * DIP control register to make sure the infoframes are sent.
> +		 */
> +		u32 dip_reg = intel_get_dip_ctl_reg(encoder);
> +		I915_WRITE(dip_reg, I915_READ(dip_reg));
> +	}
>  }
>  
>  static int intel_hdmi_mode_valid(struct drm_connector *connector,
> -- 
> 1.7.10
> 
> _______________________________________________
> 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

Re: [PATCH 04/14] drm/i915: touch the DIP control register after enabling the HDMI port

[Daniel Vetter]

On Tue, May 08, 2012 at 01:59:50PM +0200, Daniel Vetter wrote:
> On Fri, May 04, 2012 at 05:18:16PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > This is not documented anywhere, but it seems necessary to make the
> > InfoFrames work, especially when all you have is an HDMI monitor.
> > 
> > Some bugs get fixed just by running "./intel_infoframes -d". This
> > patch fixes this problem on my machine.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Can you elaborate a bit on which machines really need this? I fear that
> we're adding a hack here which is only required on a few chips and then
> keep it around forever ...

While reviewing the ilk dip port select change (and looking at gm45 bspec,
too) I've noticed that bspec says we can't change the port while the DIP
stuff is enabled. Could it be that we need to properly disable DIP on dpms
off to ensure that the hw doesn't get confused? We do rewrite all
infoframes in mode_set anyway ...
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 05/14] drm/i915: change coding style of the write_infoframe functions

[Paulo Zanoni]

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

Don't use intermediate variables, change the value of 'val' as we go
through the function. The new style looks more similar to the rest of
our code. IMHO, it's also easier to read and change.

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

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 8646a50..612d9ed 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -134,32 +134,33 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
 	struct drm_device *dev = encoder->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
-	u32 port, flags, val = I915_READ(VIDEO_DIP_CTL);
+	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? */
 	if (intel_hdmi->sdvox_reg == SDVOB)
-		port = VIDEO_DIP_PORT_B;
+		val |= VIDEO_DIP_PORT_B;
 	else if (intel_hdmi->sdvox_reg == SDVOC)
-		port = VIDEO_DIP_PORT_C;
+		val |= VIDEO_DIP_PORT_C;
 	else
 		return;
 
-	flags = intel_infoframe_index(frame);
-
 	val &= ~VIDEO_DIP_SELECT_MASK;
+	val |= intel_infoframe_index(frame);
+
+	val |= VIDEO_DIP_ENABLE;
 
-	I915_WRITE(VIDEO_DIP_CTL, VIDEO_DIP_ENABLE | val | port | flags);
+	I915_WRITE(VIDEO_DIP_CTL, val);
 
 	for (i = 0; i < len; i += 4) {
 		I915_WRITE(VIDEO_DIP_DATA, *data);
 		data++;
 	}
 
-	flags |= intel_infoframe_flags(frame);
+	val |= intel_infoframe_flags(frame);
 
-	I915_WRITE(VIDEO_DIP_CTL, VIDEO_DIP_ENABLE | val | port | flags);
+	I915_WRITE(VIDEO_DIP_CTL, val);
 }
 
 static void ironlake_write_infoframe(struct drm_encoder *encoder,
@@ -172,24 +173,25 @@ static void ironlake_write_infoframe(struct drm_encoder *encoder,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
 	unsigned i, len = DIP_HEADER_SIZE + frame->len;
-	u32 flags, val = I915_READ(reg);
+	u32 val = I915_READ(reg);
 
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
-	flags = intel_infoframe_index(frame);
-
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
+	val |= intel_infoframe_index(frame);
 
-	I915_WRITE(reg, VIDEO_DIP_ENABLE | val | flags);
+	val |= VIDEO_DIP_ENABLE;
+
+	I915_WRITE(reg, val);
 
 	for (i = 0; i < len; i += 4) {
 		I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
 		data++;
 	}
 
-	flags |= intel_infoframe_flags(frame);
+	val |= intel_infoframe_flags(frame);
 
-	I915_WRITE(reg, VIDEO_DIP_ENABLE | val | flags);
+	I915_WRITE(reg, val);
 }
 
 static void vlv_write_infoframe(struct drm_encoder *encoder,
@@ -202,24 +204,25 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
 	unsigned i, len = DIP_HEADER_SIZE + frame->len;
-	u32 flags, val = I915_READ(reg);
+	u32 val = I915_READ(reg);
 
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
-	flags = intel_infoframe_index(frame);
-
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
+	val |= intel_infoframe_index(frame);
+
+	val |= VIDEO_DIP_ENABLE;
 
-	I915_WRITE(reg, VIDEO_DIP_ENABLE | val | flags);
+	I915_WRITE(reg, val);
 
 	for (i = 0; i < len; i += 4) {
 		I915_WRITE(VLV_TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
 		data++;
 	}
 
-	flags |= intel_infoframe_flags(frame);
+	val |= intel_infoframe_flags(frame);
 
-	I915_WRITE(reg, VIDEO_DIP_ENABLE | val | flags);
+	I915_WRITE(reg, val);
 }
 
 static void intel_set_infoframe(struct drm_encoder *encoder,
-- 
1.7.10

[PATCH 06/14] drm/i915: start writing infoframes at address 0 on gen 4

[Paulo Zanoni]

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

Make sure we're doing the right thing, just like we do on gen5+.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 612d9ed..397299b 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -146,7 +146,7 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
 	else
 		return;
 
-	val &= ~VIDEO_DIP_SELECT_MASK;
+	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= intel_infoframe_index(frame);
 
 	val |= VIDEO_DIP_ENABLE;
-- 
1.7.10

[PATCH 07/14] drm/i915: mask the video DIP port select

[Paulo Zanoni]

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

Should prevent bugs when changing the port.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h   |    1 +
 drivers/gpu/drm/i915/intel_hdmi.c |    1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8da0b40..cc0b90c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1699,6 +1699,7 @@
 #define   VIDEO_DIP_ENABLE		(1 << 31)
 #define   VIDEO_DIP_PORT_B		(1 << 29)
 #define   VIDEO_DIP_PORT_C		(2 << 29)
+#define   VIDEO_DIP_PORT_MASK		(3 << 29)
 #define   VIDEO_DIP_ENABLE_AVI		(1 << 21)
 #define   VIDEO_DIP_ENABLE_VENDOR	(2 << 21)
 #define   VIDEO_DIP_ENABLE_SPD		(8 << 21)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 397299b..4ce5f1f 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -139,6 +139,7 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
 
 
 	/* 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;
 	else if (intel_hdmi->sdvox_reg == SDVOC)
-- 
1.7.10

Re: [PATCH 07/14] drm/i915: mask the video DIP port select

[Daniel Vetter]

On Fri, May 04, 2012 at 05:18:19PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Should prevent bugs when changing the port.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
Patchs 5-7 are queued for -next, thanks.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH 08/14] drm/i915: break intel_infoframe_flags into _enable and _frequency

[Paulo Zanoni]

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

This will allow us to disable an infoframe without changing its
frequency.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 4ce5f1f..57ab62f 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -94,16 +94,33 @@ static u32 intel_infoframe_index(struct dip_infoframe *frame)
 	return flags;
 }
 
-static u32 intel_infoframe_flags(struct dip_infoframe *frame)
+static u32 intel_infoframe_enable(struct dip_infoframe *frame)
 {
 	u32 flags = 0;
 
 	switch (frame->type) {
 	case DIP_TYPE_AVI:
-		flags |= VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_FREQ_VSYNC;
+		flags |= VIDEO_DIP_ENABLE_AVI;
 		break;
 	case DIP_TYPE_SPD:
-		flags |= VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_FREQ_VSYNC;
+		flags |= VIDEO_DIP_ENABLE_SPD;
+		break;
+	default:
+		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
+		break;
+	}
+
+	return flags;
+}
+
+static u32 intel_infoframe_frequency(struct dip_infoframe *frame)
+{
+	u32 flags = 0;
+
+	switch (frame->type) {
+	case DIP_TYPE_AVI:
+	case DIP_TYPE_SPD:
+		flags |= VIDEO_DIP_FREQ_VSYNC;
 		break;
 	default:
 		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
@@ -159,7 +176,8 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
 		data++;
 	}
 
-	val |= intel_infoframe_flags(frame);
+	val |= intel_infoframe_enable(frame);
+	val |= intel_infoframe_frequency(frame);
 
 	I915_WRITE(VIDEO_DIP_CTL, val);
 }
@@ -190,7 +208,8 @@ static void ironlake_write_infoframe(struct drm_encoder *encoder,
 		data++;
 	}
 
-	val |= intel_infoframe_flags(frame);
+	val |= intel_infoframe_enable(frame);
+	val |= intel_infoframe_frequency(frame);
 
 	I915_WRITE(reg, val);
 }
@@ -221,7 +240,8 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
 		data++;
 	}
 
-	val |= intel_infoframe_flags(frame);
+	val |= intel_infoframe_enable(frame);
+	val |= intel_infoframe_frequency(frame);
 
 	I915_WRITE(reg, val);
 }
-- 
1.7.10

Re: [PATCH 08/14] drm/i915: break intel_infoframe_flags into _enable and _frequency

[Daniel Vetter]

On Fri, May 04, 2012 at 05:18:20PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This will allow us to disable an infoframe without changing its
> frequency.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

I'm a bit unhappy about intel_infoframe_frequency, a 15 line function to
essentially always return VIDEO_DIP_FREQ_VSYNC (at least for all the
cases). Furthermore the intel_ prefix for both these functions is a bit a
misdenomer because hsw will change all this. Are you ok if I smash a
bikeshed on top of your patches that replaces intel_infoframe_freq with
VIDEO_DIP_FREQ_VSYNC?

-Daniel
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |   32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 4ce5f1f..57ab62f 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -94,16 +94,33 @@ static u32 intel_infoframe_index(struct dip_infoframe *frame)
>  	return flags;
>  }
>  
> -static u32 intel_infoframe_flags(struct dip_infoframe *frame)
> +static u32 intel_infoframe_enable(struct dip_infoframe *frame)
>  {
>  	u32 flags = 0;
>  
>  	switch (frame->type) {
>  	case DIP_TYPE_AVI:
> -		flags |= VIDEO_DIP_ENABLE_AVI | VIDEO_DIP_FREQ_VSYNC;
> +		flags |= VIDEO_DIP_ENABLE_AVI;
>  		break;
>  	case DIP_TYPE_SPD:
> -		flags |= VIDEO_DIP_ENABLE_SPD | VIDEO_DIP_FREQ_VSYNC;
> +		flags |= VIDEO_DIP_ENABLE_SPD;
> +		break;
> +	default:
> +		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
> +		break;
> +	}
> +
> +	return flags;
> +}
> +
> +static u32 intel_infoframe_frequency(struct dip_infoframe *frame)
> +{
> +	u32 flags = 0;
> +
> +	switch (frame->type) {
> +	case DIP_TYPE_AVI:
> +	case DIP_TYPE_SPD:
> +		flags |= VIDEO_DIP_FREQ_VSYNC;
>  		break;
>  	default:
>  		DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type);
> @@ -159,7 +176,8 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
>  		data++;
>  	}
>  
> -	val |= intel_infoframe_flags(frame);
> +	val |= intel_infoframe_enable(frame);
> +	val |= intel_infoframe_frequency(frame);
>  
>  	I915_WRITE(VIDEO_DIP_CTL, val);
>  }
> @@ -190,7 +208,8 @@ static void ironlake_write_infoframe(struct drm_encoder *encoder,
>  		data++;
>  	}
>  
> -	val |= intel_infoframe_flags(frame);
> +	val |= intel_infoframe_enable(frame);
> +	val |= intel_infoframe_frequency(frame);
>  
>  	I915_WRITE(reg, val);
>  }
> @@ -221,7 +240,8 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
>  		data++;
>  	}
>  
> -	val |= intel_infoframe_flags(frame);
> +	val |= intel_infoframe_enable(frame);
> +	val |= intel_infoframe_frequency(frame);
>  
>  	I915_WRITE(reg, val);
>  }
> -- 
> 1.7.10
> 
> _______________________________________________
> 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

[PATCH 09/14] drm/i915: disable the infoframe before changing it

[Paulo Zanoni]

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

That's what the VIDEO_DIP_CTL documentation says we need to do. Except
when it's the AVI InfoFrame and we're ironlake_write_infoframe.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 57ab62f..6e1086d 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -167,6 +167,7 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= intel_infoframe_index(frame);
 
+	val &= ~intel_infoframe_enable(frame);
 	val |= VIDEO_DIP_ENABLE;
 
 	I915_WRITE(VIDEO_DIP_CTL, val);
@@ -199,6 +200,13 @@ static void ironlake_write_infoframe(struct drm_encoder *encoder,
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= intel_infoframe_index(frame);
 
+	/* 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 |= VIDEO_DIP_ENABLE_AVI;
+	else
+		val &= ~intel_infoframe_enable(frame);
+
 	val |= VIDEO_DIP_ENABLE;
 
 	I915_WRITE(reg, val);
@@ -231,6 +239,7 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= intel_infoframe_index(frame);
 
+	val &= ~intel_infoframe_enable(frame);
 	val |= VIDEO_DIP_ENABLE;
 
 	I915_WRITE(reg, val);
-- 
1.7.10

[PATCH 10/14] drm/i915: mask the video DIP frequency when changing it

[Paulo Zanoni]

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

Better safe than sorry. Currently we never change the frequency and
use the same for every infoframe type, so the only way to reproduce a
bug would be with the BIOS doing something.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h   |    1 +
 drivers/gpu/drm/i915/intel_hdmi.c |    3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index cc0b90c..6e03732 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1710,6 +1710,7 @@
 #define   VIDEO_DIP_FREQ_ONCE		(0 << 16)
 #define   VIDEO_DIP_FREQ_VSYNC		(1 << 16)
 #define   VIDEO_DIP_FREQ_2VSYNC		(2 << 16)
+#define   VIDEO_DIP_FREQ_MASK		(3 << 16)
 
 /* 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 6e1086d..979c0ca 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -178,6 +178,7 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
 	}
 
 	val |= intel_infoframe_enable(frame);
+	val &= ~VIDEO_DIP_FREQ_MASK;
 	val |= intel_infoframe_frequency(frame);
 
 	I915_WRITE(VIDEO_DIP_CTL, val);
@@ -217,6 +218,7 @@ static void ironlake_write_infoframe(struct drm_encoder *encoder,
 	}
 
 	val |= intel_infoframe_enable(frame);
+	val &= ~VIDEO_DIP_FREQ_MASK;
 	val |= intel_infoframe_frequency(frame);
 
 	I915_WRITE(reg, val);
@@ -250,6 +252,7 @@ static void vlv_write_infoframe(struct drm_encoder *encoder,
 	}
 
 	val |= intel_infoframe_enable(frame);
+	val &= ~VIDEO_DIP_FREQ_MASK;
 	val |= intel_infoframe_frequency(frame);
 
 	I915_WRITE(reg, val);
-- 
1.7.10

[PATCH 11/14] drm/i915: simplify intel_encoder_commit

[Paulo Zanoni]

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

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5036efe..5115b8a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3187,8 +3187,7 @@ void intel_encoder_commit(struct drm_encoder *encoder)
 {
 	struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private;
 	struct drm_device *dev = encoder->dev;
-	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
-	struct intel_crtc *intel_crtc = to_intel_crtc(intel_encoder->base.crtc);
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
 
 	/* lvds has its own version of commit see intel_lvds_commit */
 	encoder_funcs->dpms(encoder, DRM_MODE_DPMS_ON);
-- 
1.7.10

[PATCH 12/14] drm/i915: split ironlake_write_infoframe into ibx_ and cpt_

[Paulo Zanoni]

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

The registers are on the PCH, so use the PCH name instead of the CPU
name. Also, the way this function is implemented is really only for
CPT and PPT. For now, both functions have the same implementations:
the next patch will fix ibx_write_infoframe.

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

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 979c0ca..6c9c901 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -184,8 +184,48 @@ static void i9xx_write_infoframe(struct drm_encoder *encoder,
 	I915_WRITE(VIDEO_DIP_CTL, val);
 }
 
-static void ironlake_write_infoframe(struct drm_encoder *encoder,
-				     struct dip_infoframe *frame)
+static void ibx_write_infoframe(struct drm_encoder *encoder,
+				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);
+	int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
+	unsigned i, len = DIP_HEADER_SIZE + frame->len;
+	u32 val = I915_READ(reg);
+
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
+
+	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
+	val |= intel_infoframe_index(frame);
+
+	/* 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 |= VIDEO_DIP_ENABLE_AVI;
+	else
+		val &= ~intel_infoframe_enable(frame);
+
+	val |= VIDEO_DIP_ENABLE;
+
+	I915_WRITE(reg, val);
+
+	for (i = 0; i < len; i += 4) {
+		I915_WRITE(TVIDEO_DIP_DATA(intel_crtc->pipe), *data);
+		data++;
+	}
+
+	val |= intel_infoframe_enable(frame);
+	val &= ~VIDEO_DIP_FREQ_MASK;
+	val |= intel_infoframe_frequency(frame);
+
+	I915_WRITE(reg, val);
+}
+
+static void cpt_write_infoframe(struct drm_encoder *encoder,
+				struct dip_infoframe *frame)
 {
 	uint32_t *data = (uint32_t *)frame;
 	struct drm_device *dev = encoder->dev;
@@ -645,8 +685,12 @@ void intel_hdmi_init(struct drm_device *dev, int sdvox_reg)
 		intel_hdmi->write_infoframe = vlv_write_infoframe;
 		for_each_pipe(i)
 			I915_WRITE(VLV_TVIDEO_DIP_CTL(i), 0);
-	}  else {
-		intel_hdmi->write_infoframe = ironlake_write_infoframe;
+	} else if (HAS_PCH_IBX(dev)) {
+		intel_hdmi->write_infoframe = ibx_write_infoframe;
+		for_each_pipe(i)
+			I915_WRITE(TVIDEO_DIP_CTL(i), 0);
+	} else {
+		intel_hdmi->write_infoframe = cpt_write_infoframe;
 		for_each_pipe(i)
 			I915_WRITE(TVIDEO_DIP_CTL(i), 0);
 	}
-- 
1.7.10

[PATCH 13/14] drm/i915: ibx_write_infoframe can disable AVI

[Paulo Zanoni]

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

IBX does not need the workaround used in cpt_write_infoframe that
requires the AVI frame to be enabled while being updated.

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

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 6c9c901..e49cd22 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -201,13 +201,7 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
 	val |= intel_infoframe_index(frame);
 
-	/* 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 |= VIDEO_DIP_ENABLE_AVI;
-	else
-		val &= ~intel_infoframe_enable(frame);
-
+	val &= ~intel_infoframe_enable(frame);
 	val |= VIDEO_DIP_ENABLE;
 
 	I915_WRITE(reg, val);
-- 
1.7.10

Re: [PATCH 13/14] drm/i915: ibx_write_infoframe can disable AVI

[Daniel Vetter]

On Fri, May 04, 2012 at 05:18:25PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> IBX does not need the workaround used in cpt_write_infoframe that
> requires the AVI frame to be enabled while being updated.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Minor bikeshed about the way you structure the patch series: Intead of
adding this avi-infoframe special case to the ironlake function, then
split out the ibx variant and remove it again for it, it's clearer when
you split up the functions first and only then add the cpt/ppt special
case to that function.

But it's just this hunk, so imo a resend is overkill. But for future patch
series which involve more fixes (like e.g. the irq_handler split-ups from
Chris Wilson I've merged recently) doing things this way is really much
better.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |    8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 6c9c901..e49cd22 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -201,13 +201,7 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
>  	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
>  	val |= intel_infoframe_index(frame);
>  
> -	/* 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 |= VIDEO_DIP_ENABLE_AVI;
> -	else
> -		val &= ~intel_infoframe_enable(frame);
> -
> +	val &= ~intel_infoframe_enable(frame);
>  	val |= VIDEO_DIP_ENABLE;
>  
>  	I915_WRITE(reg, val);
> -- 
> 1.7.10
> 
> _______________________________________________
> 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

[PATCH 14/14] drm/i915: set the DIP port on ibx_write_infoframe

[Paulo Zanoni]

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

Just like Gen 4, IBX has a "Port Select" field on the DIP register,
but the ports are different.

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

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6e03732..f715672 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1699,6 +1699,7 @@
 #define   VIDEO_DIP_ENABLE		(1 << 31)
 #define   VIDEO_DIP_PORT_B		(1 << 29)
 #define   VIDEO_DIP_PORT_C		(2 << 29)
+#define   VIDEO_DIP_PORT_D		(3 << 29)
 #define   VIDEO_DIP_PORT_MASK		(3 << 29)
 #define   VIDEO_DIP_ENABLE_AVI		(1 << 21)
 #define   VIDEO_DIP_ENABLE_VENDOR	(2 << 21)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index e49cd22..a00e32a 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -192,10 +192,26 @@ static void ibx_write_infoframe(struct drm_encoder *encoder,
 	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_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
 	int reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
 	unsigned i, len = DIP_HEADER_SIZE + frame->len;
 	u32 val = I915_READ(reg);
 
+	val &= ~VIDEO_DIP_PORT_MASK;
+	switch (intel_hdmi->sdvox_reg) {
+	case HDMIB:
+		val |= VIDEO_DIP_PORT_B;
+		break;
+	case HDMIC:
+		val |= VIDEO_DIP_PORT_C;
+		break;
+	case HDMID:
+		val |= VIDEO_DIP_PORT_D;
+		break;
+	default:
+		return;
+	}
+
 	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 	val &= ~(VIDEO_DIP_SELECT_MASK | 0xf); /* clear DIP data offset */
-- 
1.7.10

Re: [PATCH 14/14] drm/i915: set the DIP port on ibx_write_infoframe

[Daniel Vetter]

On Fri, May 04, 2012 at 05:18:26PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Just like Gen 4, IBX has a "Port Select" field on the DIP register,
> but the ports are different.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Patches 8-14 are merged to dinq, too. Thanks.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 02/14] drm/i915: DSL_LINEMASK is 12 bits only on gen2

[Daniel Vetter]

On Fri, May 04, 2012 at 05:18:14PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Gen3+ is 13 bits (12:0), and on gen2 only 12 (11:0). For both the high
> bits are marked reserved, read-only so continue to mask them. Bit 31
> is not reserved and has a meaning.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Queued for -next, thanks for the patch. While reading through the code
I've noticed that there are other places where we get this wrong. In the
crt load detect code we don't even bother with properly masking, and in
the precise vblank timestamp code we always use the gen3+ mask. That code
in addition doesn't properly handle the lack of the PIPEDSL register on
ilk+. Can I volunteer you to look into that?

Thanks, Daniel
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |    3 ++-
>  drivers/gpu/drm/i915/intel_display.c |   11 ++++++++---
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7bc407a..8da0b40 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2475,7 +2475,8 @@
>  
>  /* Pipe A */
>  #define _PIPEADSL		0x70000
> -#define   DSL_LINEMASK		0x00000fff
> +#define   DSL_LINEMASK_GEN2	0x00000fff
> +#define   DSL_LINEMASK_GEN3	0x00001fff
>  #define _PIPEACONF		0x70008
>  #define   PIPECONF_ENABLE	(1<<31)
>  #define   PIPECONF_DISABLE	0
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e1716be..613f871 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -871,15 +871,20 @@ void intel_wait_for_pipe_off(struct drm_device *dev, int pipe)
>  			     100))
>  			DRM_DEBUG_KMS("pipe_off wait timed out\n");
>  	} else {
> -		u32 last_line;
> +		u32 last_line, line_mask;
>  		int reg = PIPEDSL(pipe);
>  		unsigned long timeout = jiffies + msecs_to_jiffies(100);
>  
> +		if (IS_GEN2(dev))
> +			line_mask = DSL_LINEMASK_GEN2;
> +		else
> +			line_mask = DSL_LINEMASK_GEN3;
> +
>  		/* Wait for the display line to settle */
>  		do {
> -			last_line = I915_READ(reg) & DSL_LINEMASK;
> +			last_line = I915_READ(reg) & line_mask;
>  			mdelay(5);
> -		} while (((I915_READ(reg) & DSL_LINEMASK) != last_line) &&
> +		} while (((I915_READ(reg) & line_mask) != last_line) &&
>  			 time_after(timeout, jiffies));
>  		if (time_after(jiffies, timeout))
>  			DRM_DEBUG_KMS("pipe_off wait timed out\n");
> -- 
> 1.7.10
> 
> _______________________________________________
> 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

Re: [PATCH 02/14] drm/i915: DSL_LINEMASK is 12 bits only on gen2

[Daniel Vetter]

On Tue, May 08, 2012 at 01:49:36PM +0200, Daniel Vetter wrote:
> On Fri, May 04, 2012 at 05:18:14PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Gen3+ is 13 bits (12:0), and on gen2 only 12 (11:0). For both the high
> > bits are marked reserved, read-only so continue to mask them. Bit 31
> > is not reserved and has a meaning.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Queued for -next, thanks for the patch. While reading through the code
> I've noticed that there are other places where we get this wrong. In the
> crt load detect code we don't even bother with properly masking, and in
> the precise vblank timestamp code we always use the gen3+ mask. That code
> in addition doesn't properly handle the lack of the PIPEDSL register on
> ilk+. Can I volunteer you to look into that?

Scrap the last remark, if mixed up PIPEDSL with PIPESTAT.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48