[PATCH 1/2] drm/i915: vblank status not valid while training display port

[PATCH 1/2] drm/i915: vblank status not valid while training display port

[Keith Packard]

While the display port is in training mode, vblank interrupts don't
occur. Because we have to wait for the display port output to turn on
before starting the training sequence, enable the output in 'normal'
mode so that we can tell when a vblank has occurred, then start the
training sequence.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1a51ee0..9ab8708 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1138,18 +1138,14 @@ static bool
 intel_dp_set_link_train(struct intel_dp *intel_dp,
 			uint32_t dp_reg_value,
 			uint8_t dp_train_pat,
-			uint8_t train_set[4],
-			bool first)
+			uint8_t train_set[4])
 {
 	struct drm_device *dev = intel_dp->base.enc.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(intel_dp->base.enc.crtc);
 	int ret;
 
 	I915_WRITE(intel_dp->output_reg, dp_reg_value);
 	POSTING_READ(intel_dp->output_reg);
-	if (first)
-		intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 	intel_dp_aux_native_write_1(intel_dp,
 				    DP_TRAINING_PATTERN_SET,
@@ -1174,10 +1170,15 @@ intel_dp_link_train(struct intel_dp *intel_dp)
 	uint8_t voltage;
 	bool clock_recovery = false;
 	bool channel_eq = false;
-	bool first = true;
 	int tries;
 	u32 reg;
 	uint32_t DP = intel_dp->DP;
+	struct intel_crtc *intel_crtc = to_intel_crtc(intel_dp->base.enc.crtc);
+
+	/* Enable output, wait for it to become active */
+	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
+	POSTING_READ(intel_dp->output_reg);
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 	/* Write the link configuration data */
 	intel_dp_aux_native_write(intel_dp, DP_LINK_BW_SET,
@@ -1210,9 +1211,8 @@ intel_dp_link_train(struct intel_dp *intel_dp)
 			reg = DP | DP_LINK_TRAIN_PAT_1;
 
 		if (!intel_dp_set_link_train(intel_dp, reg,
-					     DP_TRAINING_PATTERN_1, train_set, first))
+					     DP_TRAINING_PATTERN_1, train_set))
 			break;
-		first = false;
 		/* Set training pattern 1 */
 
 		udelay(100);
@@ -1266,8 +1266,7 @@ intel_dp_link_train(struct intel_dp *intel_dp)
 
 		/* channel eq pattern */
 		if (!intel_dp_set_link_train(intel_dp, reg,
-					     DP_TRAINING_PATTERN_2, train_set,
-					     false))
+					     DP_TRAINING_PATTERN_2, train_set))
 			break;
 
 		udelay(400);
-- 
1.7.1

[PATCH 2/2] drm/i915: Use pipe state to tell when pipe is off

[Keith Packard]

Instead of waiting for the display line value to settle, we can simply
wait for the pipe configuration register 'state' bit to turn off.

Contrarywise, disabling the plane will not cause the display line
value to stop changing, so instead we wait for the vblank interrupt
bit to get set. And, we only do this when we're not about to wait for
the pipe to turn off.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_display.c |   62 +++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h     |    1 -
 2 files changed, 38 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5bf51a..9792285 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1013,8 +1013,8 @@ void intel_wait_for_vblank(struct drm_device *dev, int pipe)
 		DRM_DEBUG_KMS("vblank wait timed out\n");
 }
 
-/**
- * intel_wait_for_vblank_off - wait for vblank after disabling a pipe
+/*
+ * intel_wait_for_pipe_off - wait for pipe to turn off
  * @dev: drm device
  * @pipe: pipe to wait for
  *
@@ -1022,25 +1022,39 @@ void intel_wait_for_vblank(struct drm_device *dev, int pipe)
  * spinning on the vblank interrupt status bit, since we won't actually
  * see an interrupt when the pipe is disabled.
  *
- * So this function waits for the display line value to settle (it
- * usually ends up stopping at the start of the next frame).
+ * On Gen4 and above:
+ *   wait for the pipe register state bit to turn off
+ *
+ * Otherwise:
+ *   wait for the display line value to settle (it usually
+ *   ends up stopping at the start of the next frame).
+ *  
  */
-void intel_wait_for_vblank_off(struct drm_device *dev, int pipe)
+static void intel_wait_for_pipe_off(struct drm_device *dev, int pipe)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	int pipedsl_reg = (pipe == 0 ? PIPEADSL : PIPEBDSL);
-	unsigned long timeout = jiffies + msecs_to_jiffies(100);
-	u32 last_line;
-
-	/* Wait for the display line to settle */
-	do {
-		last_line = I915_READ(pipedsl_reg) & DSL_LINEMASK;
-		mdelay(5);
-	} while (((I915_READ(pipedsl_reg) & DSL_LINEMASK) != last_line) &&
-		 time_after(timeout, jiffies));
-
-	if (time_after(jiffies, timeout))
-		DRM_DEBUG_KMS("vblank wait timed out\n");
+
+	if (INTEL_INFO(dev)->gen >= 4) {
+		int pipeconf_reg = (pipe == 0 ? PIPEACONF : PIPEBCONF);
+
+		/* Wait for the Pipe State to go off */
+		if (wait_for((I915_READ(pipeconf_reg) & I965_PIPECONF_ACTIVE) == 0,
+			     100, 0))
+			DRM_DEBUG_KMS("pipe_off wait timed out\n");
+	} else {
+		u32 last_line;
+		int pipedsl_reg = (pipe == 0 ? PIPEADSL : PIPEBDSL);
+		unsigned long timeout = jiffies + msecs_to_jiffies(100);
+
+		/* Wait for the display line to settle */
+		do {
+			last_line = I915_READ(pipedsl_reg) & DSL_LINEMASK;
+			mdelay(5);
+		} while (((I915_READ(pipedsl_reg) & DSL_LINEMASK) != last_line) &&
+			 time_after(timeout, jiffies));
+		if (time_after(jiffies, timeout))
+			DRM_DEBUG_KMS("pipe_off wait timed out\n");
+	}
 }
 
 /* Parameters have changed, update FBC info */
@@ -2328,13 +2342,13 @@ static void i9xx_crtc_dpms(struct drm_crtc *crtc, int mode)
 			I915_READ(dspbase_reg);
 		}
 
-		/* Wait for vblank for the disable to take effect */
-		intel_wait_for_vblank_off(dev, pipe);
-
 		/* Don't disable pipe A or pipe A PLLs if needed */
 		if (pipeconf_reg == PIPEACONF &&
-		    (dev_priv->quirks & QUIRK_PIPEA_FORCE))
+		    (dev_priv->quirks & QUIRK_PIPEA_FORCE)) {
+			/* Wait for vblank for the disable to take effect */
+			intel_wait_for_vblank(dev, pipe);
 			goto skip_pipe_off;
+		}
 
 		/* Next, disable display pipes */
 		temp = I915_READ(pipeconf_reg);
@@ -2343,8 +2357,8 @@ static void i9xx_crtc_dpms(struct drm_crtc *crtc, int mode)
 			I915_READ(pipeconf_reg);
 		}
 
-		/* Wait for vblank for the disable to take effect. */
-		intel_wait_for_vblank_off(dev, pipe);
+		/* Wait for the pipe to turn off */
+		intel_wait_for_pipe_off(dev, pipe);
 
 		temp = I915_READ(dpll_reg);
 		if ((temp & DPLL_VCO_ENABLE) != 0) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index ad312ca..8828b3a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -229,7 +229,6 @@ extern struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev,
 						    struct drm_crtc *crtc);
 int intel_get_pipe_from_crtc_id(struct drm_device *dev, void *data,
 				struct drm_file *file_priv);
-extern void intel_wait_for_vblank_off(struct drm_device *dev, int pipe);
 extern void intel_wait_for_vblank(struct drm_device *dev, int pipe);
 extern struct drm_crtc *intel_get_crtc_from_pipe(struct drm_device *dev, int pipe);
 extern struct drm_crtc *intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
-- 
1.7.1

Re: [PATCH 2/2] drm/i915: Use pipe state to tell when pipe is off

[Chris Wilson]

On Sun,  3 Oct 2010 00:33:06 -0700, Keith Packard <keithp@keithp.com> wrote:
> Instead of waiting for the display line value to settle, we can simply
> wait for the pipe configuration register 'state' bit to turn off.

Thanks Keith, series applied to -fixes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Use pipe state to tell when pipe is off

[Jesse Barnes]

On Sun,  3 Oct 2010 00:33:06 -0700
Keith Packard <keithp@keithp.com> wrote:

> Instead of waiting for the display line value to settle, we can simply
> wait for the pipe configuration register 'state' bit to turn off.
> 
> Contrarywise, disabling the plane will not cause the display line
> value to stop changing, so instead we wait for the vblank interrupt
> bit to get set. And, we only do this when we're not about to wait for
> the pipe to turn off.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---

Do these fixes help with the DP issues you've been seeing, Keith?
Seems like the first one shouldn't change behavior since we ought to
time out on waiting on vblank in that case, and the timeout is the same
as the msleep we used to use.

The second one looks like a good change, but really the pipe off change
is separate from the plane disable bug fix.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/2] drm/i915: Use pipe state to tell when pipe is off

[Keith Packard]

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

On Sun, 3 Oct 2010 08:10:48 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Do these fixes help with the DP issues you've been seeing, Keith?
> Seems like the first one shouldn't change behavior since we ought to
> time out on waiting on vblank in that case, and the timeout is the same
> as the msleep we used to use.

The first one changes when the monitor sees the training message --
before the change, the training message would get sent before waiting
for the vblank, and could potentially mess up the monitor
synchronization with signals.

I tested this by turning an external DP monitor on/off repeatedly
without X running. Before the patch, the monitor would fail to sync once
in a while. With the patch, I haven't seen it fail. That's not to say
that it has actually fixed anything, just that it seems better.

The best feature of the patch is that it shortens the time it takes to
light up a DP pipe -- the code was always hitting the timeout instead of
seeing a vblank signal, so we'd get a 50ms delay instead of a couple of ms.

> The second one looks like a good change, but really the pipe off change
> is separate from the plane disable bug fix.

Yeah, yeah, I know. I should have split the patch into two pieces...

With these two patches in place, I'm not getting any timeouts while
waiting for vblank, which seems like a useful result, and should make
mode setting a tiny bit faster as well.

I've got a couple more changes to work on today:

 1) re-train the monitor when it gets unplugged and then plugged back
    in. Right now, if you kick the cable out, you're stuck fumbling
    around in the dark trying to run 'xrandr' again.
   
 2) send hotplug notification through the X server, at least for the
    'reliable' hotplug signals. Right now, when you run 'xrandr'
    after changing connections, gnome sees the connection status change
    event and 'does stuff', which frequently collides with the 'xrandr'
    command you're running. This is very confusing to users.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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 2/2] drm/i915: Use pipe state to tell when pipe is off

[Jesse Barnes]

"Keith Packard" <keithp@keithp.com> wrote:

>On Sun, 3 Oct 2010 08:10:48 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>
>> Do these fixes help with the DP issues you've been seeing, Keith?
>> Seems like the first one shouldn't change behavior since we ought to
>> time out on waiting on vblank in that case, and the timeout is the same
>> as the msleep we used to use.
>
>The first one changes when the monitor sees the training message --
>before the change, the training message would get sent before waiting
>for the vblank, and could potentially mess up the monitor
>synchronization with signals.
>
>I tested this by turning an external DP monitor on/off repeatedly
>without X running. Before the patch, the monitor would fail to sync once
>in a while. With the patch, I haven't seen it fail. That's not to say
>that it has actually fixed anything, just that it seems better.
>
>The best feature of the patch is that it shortens the time it takes to
>light up a DP pipe -- the code was always hitting the timeout instead of
>seeing a vblank signal, so we'd get a 50ms delay instead of a couple of ms.

Yeah, definitely an improvement.  I'll test myself when I get home (tested on my PCH eDP already, unfortunately didn't help).

>
>> The second one looks like a good change, but really the pipe off change
>> is separate from the plane disable bug fix.
>
>Yeah, yeah, I know. I should have split the patch into two pieces...
>
>With these two patches in place, I'm not getting any timeouts while
>waiting for vblank, which seems like a useful result, and should make
>mode setting a tiny bit faster as well.
>
>I've got a couple more changes to work on today:
>
> 1) re-train the monitor when it gets unplugged and then plugged back
>    in. Right now, if you kick the cable out, you're stuck fumbling
>    around in the dark trying to run 'xrandr' again.

Cool.  Making use of our hotplug interrupts and never polling should be a goal; it looked like there were some other aux commands we could add as well.  I'm meeting with the SV guys on Wed. To go over our eDP stuff, hopefully we can run the above through their equipment too and make sure we have all the timings etc correct. 

>   
> 2) send hotplug notification through the X server, at least for the
>    'reliable' hotplug signals. Right now, when you run 'xrandr'
>    after changing connections, gnome sees the connection status change
>    event and 'does stuff', which frequently collides with the 'xrandr'
>    command you're running. This is very confusing to users.

Great, yeah we shouldn't be sending events in response to our normal status ioctls, that sounds broken.  On 9xx and above we should be able to do reliable hotplug for everything (though at a power cost for TV and maybe VGA).  
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/2] drm/i915: Use pipe state to tell when pipe is off

[Dave Airlie]

> mode setting a tiny bit faster as well.
>
> I've got a couple more changes to work on today:
>
>  1) re-train the monitor when it gets unplugged and then plugged back
>    in. Right now, if you kick the cable out, you're stuck fumbling
>    around in the dark trying to run 'xrandr' again.

don't you do this already? both radeon/nouveau handle DP replug fine,
I thought Intel would have been where I stole the code from.

Dave.

Re: [PATCH 2/2] drm/i915: Use pipe state to tell when pipe is off

[Jesse Barnes]

"Dave Airlie" <airlied@gmail.com> wrote:

>> mode setting a tiny bit faster as well.
>>
>> I've got a couple more changes to work on today:
>>
>>  1) re-train the monitor when it gets unplugged and then plugged back
>>    in. Right now, if you kick the cable out, you're stuck fumbling
>>    around in the dark trying to run 'xrandr' again.
>
>don't you do this already? both radeon/nouveau handle DP replug fine,
>I thought Intel would have been where I stole the code from.

There is some code to handle the interrupts, but I'm not sure if it's wired up.  I think we rely on polling right now.
-- 
Jesse Barnes, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915: Use pipe state to tell when pipe is off

[Keith Packard]

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

On Mon, 4 Oct 2010 06:33:01 +1000, Dave Airlie <airlied@gmail.com> wrote:

> don't you do this already? both radeon/nouveau handle DP replug fine,
> I thought Intel would have been where I stole the code from.

There was a one-line bug. Patch already posted.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel