[PATCH 0/3] drm/i915: "Fix" g4x infoframes w/ multiple HDMI/DVI monitors

[PATCH 0/3] drm/i915: "Fix" g4x infoframes w/ multiple HDMI/DVI monitors

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

I was readin the infoframe code a bit today, and noticed that g4x can't
transmit infoframes to more than one port, even though it can have two
HDMI/DVI ports.

So I decided to "fix" it by making it work on a first come, first served
basis. This at least makes it reasonable to have a HDMI + DVI monitors
hooked up to the same machine w/o the DVI clobbering the HDMI infoframes.

Tested quickly on an ELK w/ HDMI and DVI monitors attached.

If we allowed HDMI cloning, we'd have a similar problem on PCH platforms
since the DIP stuff lives on the transcoder. But we don't allow that currently
so I left the code alone. Also a quick gander at IBX spec says that
feeding multiple HDMI ports from the same transcoder isn't supported. But
I suspect that might be exactly because of this infoframe issue. I've not
asked for clarification on that, nor have I tried HDMI cloning on PCH yet.
Maybe I'll do that at some point if I get bored :)

Ville Syrjälä (3):
  drm/i915: Add encoder .off() hook
  drm/i915: Convert DIP port switch cases to a simple macro
  drm/i915: Make infoframe trnsmission more reliable on g4x

 drivers/gpu/drm/i915/i915_reg.h      |  4 +--
 drivers/gpu/drm/i915/intel_display.c |  5 +++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_hdmi.c    | 63 +++++++++++++++++++-----------------
 4 files changed, 40 insertions(+), 33 deletions(-)

-- 
1.8.3.2

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

[PATCH 1/3] drm/i915: Add encoder .off() hook

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Add an encoder specific hook to be called alongside the crtc .off()
hook.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +++++
 drivers/gpu/drm/i915/intel_drv.h     | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ec96002..e5b336e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4343,6 +4343,7 @@ void intel_crtc_update_dpms(struct drm_crtc *crtc)
 static void intel_crtc_disable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
+	struct intel_encoder *intel_encoder;
 	struct drm_connector *connector;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -4359,6 +4360,10 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	assert_cursor_disabled(dev_priv, to_intel_crtc(crtc)->pipe);
 	assert_pipe_disabled(dev->dev_private, to_intel_crtc(crtc)->pipe);
 
+	for_each_encoder_on_crtc(dev, crtc, intel_encoder)
+		if (intel_encoder->off)
+			intel_encoder->off(intel_encoder);
+
 	if (crtc->fb) {
 		mutex_lock(&dev->struct_mutex);
 		intel_unpin_fb_obj(to_intel_framebuffer(crtc->fb)->obj);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7b3c209..48ad05c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -139,6 +139,7 @@ struct intel_encoder {
 	void (*mode_set)(struct intel_encoder *intel_encoder);
 	void (*disable)(struct intel_encoder *);
 	void (*post_disable)(struct intel_encoder *);
+	void (*off)(struct intel_encoder *);
 	/* Read out the current hw state of this connector, returning true if
 	 * the encoder is active. If the encoder is enabled it also set the pipe
 	 * it is connected to in the pipe parameter. */
-- 
1.8.3.2

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

Re: [PATCH 1/3] drm/i915: Add encoder .off() hook

[Damien Lespiau]

On Thu, Jan 23, 2014 at 11:15:33PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Add an encoder specific hook to be called alongside the crtc .off()
> hook.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

[PATCH 2/3] drm/i915: Convert DIP port switch cases to a simple macro

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We have a couple of switch cases to compute the port value for the
VIDEO_DIP_CTL register. Replace them with a simple macro.

We do lose a few BUG() calls, but many people may consider that
an improvement.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h   |  4 +---
 drivers/gpu/drm/i915/intel_hdmi.c | 31 ++-----------------------------
 2 files changed, 3 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 5d06ad6..bf2eeb2 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2333,9 +2333,7 @@
 #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)
-#define   VIDEO_DIP_PORT_D		(3 << 29)
+#define   VIDEO_DIP_PORT(port)		((port) << 29)
 #define   VIDEO_DIP_PORT_MASK		(3 << 29)
 #define   VIDEO_DIP_ENABLE_GCP		(1 << 25)
 #define   VIDEO_DIP_ENABLE_AVI		(1 << 21)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 6db0d9d..9a8c7f8 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -423,7 +423,7 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 	struct intel_hdmi *intel_hdmi = &intel_dig_port->hdmi;
 	u32 reg = VIDEO_DIP_CTL;
 	u32 val = I915_READ(reg);
-	u32 port;
+	u32 port = VIDEO_DIP_PORT(intel_dig_port->port);
 
 	assert_hdmi_port_disabled(intel_hdmi);
 
@@ -447,18 +447,6 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 		return;
 	}
 
-	switch (intel_dig_port->port) {
-	case PORT_B:
-		port = VIDEO_DIP_PORT_B;
-		break;
-	case PORT_C:
-		port = VIDEO_DIP_PORT_C;
-		break;
-	default:
-		BUG();
-		return;
-	}
-
 	if (port != (val & VIDEO_DIP_PORT_MASK)) {
 		if (val & VIDEO_DIP_ENABLE) {
 			val &= ~VIDEO_DIP_ENABLE;
@@ -489,7 +477,7 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 	struct intel_hdmi *intel_hdmi = &intel_dig_port->hdmi;
 	u32 reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
 	u32 val = I915_READ(reg);
-	u32 port;
+	u32 port = VIDEO_DIP_PORT(intel_dig_port->port);
 
 	assert_hdmi_port_disabled(intel_hdmi);
 
@@ -505,21 +493,6 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
 		return;
 	}
 
-	switch (intel_dig_port->port) {
-	case PORT_B:
-		port = VIDEO_DIP_PORT_B;
-		break;
-	case PORT_C:
-		port = VIDEO_DIP_PORT_C;
-		break;
-	case PORT_D:
-		port = VIDEO_DIP_PORT_D;
-		break;
-	default:
-		BUG();
-		return;
-	}
-
 	if (port != (val & VIDEO_DIP_PORT_MASK)) {
 		if (val & VIDEO_DIP_ENABLE) {
 			val &= ~VIDEO_DIP_ENABLE;
-- 
1.8.3.2

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

Re: [PATCH 2/3] drm/i915: Convert DIP port switch cases to a simple macro

[Damien Lespiau]

On Thu, Jan 23, 2014 at 11:15:34PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We have a couple of switch cases to compute the port value for the
> VIDEO_DIP_CTL register. Replace them with a simple macro.
> 
> We do lose a few BUG() calls, but many people may consider that
> an improvement.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 2/3] drm/i915: Convert DIP port switch cases to a simple macro

[Daniel Vetter]

On Mon, Feb 10, 2014 at 12:11:33PM +0000, Damien Lespiau wrote:
> On Thu, Jan 23, 2014 at 11:15:34PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > We have a couple of switch cases to compute the port value for the
> > VIDEO_DIP_CTL register. Replace them with a simple macro.
> > 
> > We do lose a few BUG() calls, but many people may consider that
> > an improvement.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 3/3] drm/i915: Make infoframe trnsmission more reliable on g4x

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

On g4x there's just one video DIP, but there can be two HDMI/DVI
ports. Currently even a DVI monitor on another port can clobber
the infoframes meant for a real HDMI monitor on the other port.

Make sure we only ever send DIPs to one port. The first port with a
HDMI sink to get there gets to own the DIP until such time that the
port is completely turned off (ie. not just DPMS off). If there are
two HDMI sinks attached, the one to arrive second doesn't get any
infoframes. And if there's a DVI sink on the other port, it will
no longer disable DIP transmission for the other port.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 9a8c7f8..673e371 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -427,6 +427,15 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 
 	assert_hdmi_port_disabled(intel_hdmi);
 
+	/* Is DIP already being transmitted on another port? */
+	if ((val & VIDEO_DIP_ENABLE) != 0 &&
+	    (val & VIDEO_DIP_PORT_MASK) != port) {
+		if (intel_hdmi->has_hdmi_sink)
+			DRM_DEBUG_KMS("Video DIP busy. Can't transmit on port %c\n",
+				      port_name(intel_dig_port->port));
+		return;
+	}
+
 	/* If the registers were not initialized yet, they might be zeroes,
 	 * which means we're selecting the AVI DIP and we're setting its
 	 * frequency to once. This seems to really confuse the HW and make
@@ -439,7 +448,8 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
 	val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
 
 	if (!intel_hdmi->has_hdmi_sink) {
-		if (!(val & VIDEO_DIP_ENABLE))
+		if ((val & VIDEO_DIP_ENABLE) == 0 ||
+		    (val & VIDEO_DIP_PORT_MASK) != port)
 			return;
 		val &= ~VIDEO_DIP_ENABLE;
 		I915_WRITE(reg, val);
@@ -1143,6 +1153,23 @@ static void vlv_hdmi_post_disable(struct intel_encoder *encoder)
 	mutex_unlock(&dev_priv->dpio_lock);
 }
 
+static void g4x_hdmi_off(struct intel_encoder *encoder)
+{
+	struct drm_i915_private *dev_priv = encoder->base.dev->dev_private;
+	struct intel_digital_port *intel_dig_port = enc_to_dig_port(&encoder->base);
+	u32 port = VIDEO_DIP_PORT(intel_dig_port->port);
+	u32 reg = VIDEO_DIP_CTL;
+	u32 val = I915_READ(reg);
+
+	if ((val & VIDEO_DIP_ENABLE) == 0 ||
+	    (val & VIDEO_DIP_PORT_MASK) != port)
+		return;
+
+	val &= ~VIDEO_DIP_ENABLE;
+	I915_WRITE(reg, val);
+	POSTING_READ(reg);
+}
+
 static void intel_hdmi_destroy(struct drm_connector *connector)
 {
 	drm_connector_cleanup(connector);
@@ -1285,6 +1312,9 @@ void intel_hdmi_init(struct drm_device *dev, int hdmi_reg, enum port port)
 		intel_encoder->enable = intel_enable_hdmi;
 	}
 
+	if (IS_G4X(dev))
+		intel_encoder->off = g4x_hdmi_off;
+
 	intel_encoder->type = INTEL_OUTPUT_HDMI;
 	intel_encoder->crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
 	intel_encoder->cloneable = false;
-- 
1.8.3.2

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

Re: [PATCH 3/3] drm/i915: Make infoframe trnsmission more reliable on g4x

[Damien Lespiau]

On Thu, Jan 23, 2014 at 11:15:35PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On g4x there's just one video DIP, but there can be two HDMI/DVI
> ports. Currently even a DVI monitor on another port can clobber
> the infoframes meant for a real HDMI monitor on the other port.
> 
> Make sure we only ever send DIPs to one port. The first port with a
> HDMI sink to get there gets to own the DIP until such time that the
> port is completely turned off (ie. not just DPMS off). If there are
> two HDMI sinks attached, the one to arrive second doesn't get any
> infoframes. And if there's a DVI sink on the other port, it will
> no longer disable DIP transmission for the other port.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

I guess follow-up patches could turn off Video DIPs on other platforms,
if I read the code correctly we're always leaving Video DIPs on when
disabling the pipe.

-- 
Damien

Re: [PATCH 3/3] drm/i915: Make infoframe trnsmission more reliable on g4x

[Ville Syrjälä]

On Mon, Feb 10, 2014 at 12:16:14PM +0000, Damien Lespiau wrote:
> On Thu, Jan 23, 2014 at 11:15:35PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > On g4x there's just one video DIP, but there can be two HDMI/DVI
> > ports. Currently even a DVI monitor on another port can clobber
> > the infoframes meant for a real HDMI monitor on the other port.
> > 
> > Make sure we only ever send DIPs to one port. The first port with a
> > HDMI sink to get there gets to own the DIP until such time that the
> > port is completely turned off (ie. not just DPMS off). If there are
> > two HDMI sinks attached, the one to arrive second doesn't get any
> > infoframes. And if there's a DVI sink on the other port, it will
> > no longer disable DIP transmission for the other port.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> 
> I guess follow-up patches could turn off Video DIPs on other platforms,
> if I read the code correctly we're always leaving Video DIPs on when
> disabling the pipe.

Yeah. Actually I realized afterwards that that's also a problem if we
switch from say HDMI->DP on the same transcoder. We might end up
sending infoframes to the DP sink that we constructed for the HDMI sink.
Or if the video DIP has port select bits (not all platforms do), then
I think we might even end up sending infoframes from some transcoder to
a port that isn't connected to said transcoder. I have no idea if the
hardware can even do that, but it would seem safer not to program it
that way in the first place.

But the .off hook isn't sufficient to fix that since we could switch the
output without calling the .off hook. I think the real solution would to
always disable infoframes in hdmi disable or post_disable, and always
re-enabling them in enable or pre_enable.

That would require changing the new g4x logic you just reviewed since
we couldn't track the owner of the video DIP by the register contents
anymore. So I think we'd need to add a piece of software state for that
purpose instead.

If anyone's looking for a relatively straightforward small project,
this would seem to fit the bill.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/3] drm/i915: Make infoframe trnsmission more reliable on g4x

[Daniel Vetter]

On Mon, Feb 10, 2014 at 02:40:07PM +0200, Ville Syrjälä wrote:
> On Mon, Feb 10, 2014 at 12:16:14PM +0000, Damien Lespiau wrote:
> > On Thu, Jan 23, 2014 at 11:15:35PM +0200, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > On g4x there's just one video DIP, but there can be two HDMI/DVI
> > > ports. Currently even a DVI monitor on another port can clobber
> > > the infoframes meant for a real HDMI monitor on the other port.
> > > 
> > > Make sure we only ever send DIPs to one port. The first port with a
> > > HDMI sink to get there gets to own the DIP until such time that the
> > > port is completely turned off (ie. not just DPMS off). If there are
> > > two HDMI sinks attached, the one to arrive second doesn't get any
> > > infoframes. And if there's a DVI sink on the other port, it will
> > > no longer disable DIP transmission for the other port.
> > > 
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> > 
> > I guess follow-up patches could turn off Video DIPs on other platforms,
> > if I read the code correctly we're always leaving Video DIPs on when
> > disabling the pipe.
> 
> Yeah. Actually I realized afterwards that that's also a problem if we
> switch from say HDMI->DP on the same transcoder. We might end up
> sending infoframes to the DP sink that we constructed for the HDMI sink.
> Or if the video DIP has port select bits (not all platforms do), then
> I think we might even end up sending infoframes from some transcoder to
> a port that isn't connected to said transcoder. I have no idea if the
> hardware can even do that, but it would seem safer not to program it
> that way in the first place.
> 
> But the .off hook isn't sufficient to fix that since we could switch the
> output without calling the .off hook. I think the real solution would to
> always disable infoframes in hdmi disable or post_disable, and always
> re-enabling them in enable or pre_enable.
> 
> That would require changing the new g4x logic you just reviewed since
> we couldn't track the owner of the video DIP by the register contents
> anymore. So I think we'd need to add a piece of software state for that
> purpose instead.
> 
> If anyone's looking for a relatively straightforward small project,
> this would seem to fit the bill.

Or plan B:
1. Add infoframe tracking to the pipe_config.
2. Use the new ->new_config pointers to arbitrate on g4x/vlv and clear the
pipe_config->has_infoframes bit if more than one pipe needs. Yeah that
means if you disable the 1st pipe the 2nd won't automatically get
infoframes, but a) meh b) we can fix that once we have atomic modeset and
recompute the config always. So a FIXME comment should be good enough.
3. Lock it down with infoframes hw readout support. Make that also check
that the infoframes are correct (i.e. our own), which gives us
infoframe-correct fastboot support.

Now the ->off hook in this series moves into another direction entirely,
so I'm not sure whether I should merge it. Also we don't seem to have bug
reports about this really, probably due to the lack of state checking ;-)
Suggestions highly welcome.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch