Patch queue for -fixes

Patch queue for -fixes

[Chris Wilson]

A couple of fixes for bugs that have finally been caught by QA that came
in through -next (a subtle change in the timing/sequence of pipe/plane
enabling upset PNV, and a broken implementation is_pot()). And a bug
fix for 2.6.38 tiling on gen2/3 (when going from GPU fenced to CPU
fenced).

Comments?
-Chris

[PATCH 1/8] drm/i915: Remove surplus POSTING_READs before wait_for_vblank

[Chris Wilson]

... as wait_for_vblank (and friends) will do a flush of the MMIO writes
anyway.

References: https://bugs.freedesktop.org/show_bug.cgi?id=34601
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3106c0d..3bc6ab5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1518,7 +1518,6 @@ static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
 	val = I915_READ(reg);
 	val |= PIPECONF_ENABLE;
 	I915_WRITE(reg, val);
-	POSTING_READ(reg);
 	intel_wait_for_vblank(dev_priv->dev, pipe);
 }
 
@@ -1554,7 +1553,6 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv,
 	val = I915_READ(reg);
 	val &= ~PIPECONF_ENABLE;
 	I915_WRITE(reg, val);
-	POSTING_READ(reg);
 	intel_wait_for_pipe_off(dev_priv->dev, pipe);
 }
 
@@ -1579,7 +1577,6 @@ static void intel_enable_plane(struct drm_i915_private *dev_priv,
 	val = I915_READ(reg);
 	val |= DISPLAY_PLANE_ENABLE;
 	I915_WRITE(reg, val);
-	POSTING_READ(reg);
 	intel_wait_for_vblank(dev_priv->dev, pipe);
 }
 
@@ -1612,7 +1609,6 @@ static void intel_disable_plane(struct drm_i915_private *dev_priv,
 	val = I915_READ(reg);
 	val &= ~DISPLAY_PLANE_ENABLE;
 	I915_WRITE(reg, val);
-	POSTING_READ(reg);
 	intel_flush_display_plane(dev_priv, plane);
 	intel_wait_for_vblank(dev_priv->dev, pipe);
 }
@@ -1769,7 +1765,6 @@ static void g4x_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 			return;
 
 		I915_WRITE(DPFC_CONTROL, dpfc_ctl & ~DPFC_CTL_EN);
-		POSTING_READ(DPFC_CONTROL);
 		intel_wait_for_vblank(dev, intel_crtc->pipe);
 	}
 
@@ -1861,7 +1856,6 @@ static void ironlake_enable_fbc(struct drm_crtc *crtc, unsigned long interval)
 			return;
 
 		I915_WRITE(ILK_DPFC_CONTROL, dpfc_ctl & ~DPFC_CTL_EN);
-		POSTING_READ(ILK_DPFC_CONTROL);
 		intel_wait_for_vblank(dev, intel_crtc->pipe);
 	}
 
@@ -5777,7 +5771,6 @@ static void intel_increase_pllclock(struct drm_crtc *crtc)
 
 		dpll &= ~DISPLAY_RATE_SELECT_FPA1;
 		I915_WRITE(dpll_reg, dpll);
-		POSTING_READ(dpll_reg);
 		intel_wait_for_vblank(dev, pipe);
 
 		dpll = I915_READ(dpll_reg);
@@ -5821,7 +5814,6 @@ static void intel_decrease_pllclock(struct drm_crtc *crtc)
 
 		dpll |= DISPLAY_RATE_SELECT_FPA1;
 		I915_WRITE(dpll_reg, dpll);
-		dpll = I915_READ(dpll_reg);
 		intel_wait_for_vblank(dev, pipe);
 		dpll = I915_READ(dpll_reg);
 		if (!(dpll & DISPLAY_RATE_SELECT_FPA1))
-- 
1.7.2.3

Re: [PATCH 1/8] drm/i915: Remove surplus POSTING_READs before wait_for_vblank

[Keith Packard]

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

On Fri, 18 Mar 2011 08:02:03 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> ... as wait_for_vblank (and friends) will do a flush of the MMIO writes
> anyway.

This one seems reasonable.

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
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 1/8] drm/i915: Remove surplus POSTING_READs before wait_for_vblank

[Jesse Barnes]

On Fri, 18 Mar 2011 08:02:03 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> ... as wait_for_vblank (and friends) will do a flush of the MMIO writes
> anyway.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=34601
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---

Looks good.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 2/8] drm/i915: skip redundant operations whilst enabling pipes and planes

[Chris Wilson]

If the pipe or plane is already enabled, then we do not need to enable
it again and can skip the delay.

References: https://bugs.freedesktop.org/show_bug.cgi?id=34601
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3bc6ab5..841f039 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1516,8 +1516,10 @@ static void intel_enable_pipe(struct drm_i915_private *dev_priv, enum pipe pipe,
 
 	reg = PIPECONF(pipe);
 	val = I915_READ(reg);
-	val |= PIPECONF_ENABLE;
-	I915_WRITE(reg, val);
+	if (val & PIPECONF_ENABLE)
+		return;
+
+	I915_WRITE(reg, val | PIPECONF_ENABLE);
 	intel_wait_for_vblank(dev_priv->dev, pipe);
 }
 
@@ -1551,8 +1553,10 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv,
 
 	reg = PIPECONF(pipe);
 	val = I915_READ(reg);
-	val &= ~PIPECONF_ENABLE;
-	I915_WRITE(reg, val);
+	if ((val & PIPECONF_ENABLE) == 0)
+		return;
+
+	I915_WRITE(reg, val & ~PIPECONF_ENABLE);
 	intel_wait_for_pipe_off(dev_priv->dev, pipe);
 }
 
@@ -1575,8 +1579,10 @@ static void intel_enable_plane(struct drm_i915_private *dev_priv,
 
 	reg = DSPCNTR(plane);
 	val = I915_READ(reg);
-	val |= DISPLAY_PLANE_ENABLE;
-	I915_WRITE(reg, val);
+	if (val & DISPLAY_PLANE_ENABLE)
+		return;
+
+	I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
 	intel_wait_for_vblank(dev_priv->dev, pipe);
 }
 
@@ -1607,8 +1613,10 @@ static void intel_disable_plane(struct drm_i915_private *dev_priv,
 
 	reg = DSPCNTR(plane);
 	val = I915_READ(reg);
-	val &= ~DISPLAY_PLANE_ENABLE;
-	I915_WRITE(reg, val);
+	if ((val & DISPLAY_PLANE_ENABLE) == 0)
+		return;
+
+	I915_WRITE(reg, val & ~DISPLAY_PLANE_ENABLE);
 	intel_flush_display_plane(dev_priv, plane);
 	intel_wait_for_vblank(dev_priv->dev, pipe);
 }
-- 
1.7.2.3

Re: [PATCH 2/8] drm/i915: skip redundant operations whilst enabling pipes and planes

[Keith Packard]

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

On Fri, 18 Mar 2011 08:02:04 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> If the pipe or plane is already enabled, then we do not need to enable
> it again and can skip the delay.

This should mention that you're also skipping the operation on disable.
Otherwise,

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
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/8] drm/i915: skip redundant operations whilst enabling pipes and planes

[Jesse Barnes]

On Fri, 18 Mar 2011 08:02:04 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> If the pipe or plane is already enabled, then we do not need to enable
> it again and can skip the delay.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=34601
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---

Does this one actually fix the issue?  Unless we're trying to get
suspend, resume, or mode setting down under 100ms it's probably not
worth it to try to avoid the vblank waits (this is me being paranoid
about our mode setting code).

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 2/8] drm/i915: skip redundant operations whilst enabling pipes and planes

[Chris Wilson]

On Fri, 18 Mar 2011 09:25:22 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Fri, 18 Mar 2011 08:02:04 +0000
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > If the pipe or plane is already enabled, then we do not need to enable
> > it again and can skip the delay.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=34601
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> 
> Does this one actually fix the issue?  Unless we're trying to get
> suspend, resume, or mode setting down under 100ms it's probably not
> worth it to try to avoid the vblank waits (this is me being paranoid
> about our mode setting code).

This is the one QA identified as fixing the regression from b24e717988,
which worries me.

I admit to not knowing what the root cause is, but it was a change
introduced by b24e71798 and had unfortunate side-effects.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/8] drm/i915: skip redundant operations whilst enabling pipes and planes

[Jesse Barnes]

On Fri, 18 Mar 2011 17:37:06 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, 18 Mar 2011 09:25:22 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Fri, 18 Mar 2011 08:02:04 +0000
> > Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > 
> > > If the pipe or plane is already enabled, then we do not need to enable
> > > it again and can skip the delay.
> > > 
> > > References: https://bugs.freedesktop.org/show_bug.cgi?id=34601
> > > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > 
> > Does this one actually fix the issue?  Unless we're trying to get
> > suspend, resume, or mode setting down under 100ms it's probably not
> > worth it to try to avoid the vblank waits (this is me being paranoid
> > about our mode setting code).
> 
> This is the one QA identified as fixing the regression from b24e717988,
> which worries me.
> 
> I admit to not knowing what the root cause is, but it was a change
> introduced by b24e71798 and had unfortunate side-effects.

Yeah that message confused me; they also identified the other two fixes
working (both the vblank wait on gen2 and the display plane flush).
I've asked for clarification.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 3/8] drm/i915: Flush the plane enable using the DSPCNTR latch

[Chris Wilson]

References: https://bugs.freedesktop.org/show_bug.cgi?id=34601
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 841f039..2f63999 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1560,6 +1560,17 @@ static void intel_disable_pipe(struct drm_i915_private *dev_priv,
 	intel_wait_for_pipe_off(dev_priv->dev, pipe);
 }
 
+/*
+ * Plane regs are double buffered, going from enabled<->disabled needs a
+ * trigger in order to latch.  The display address reg provides this.
+ */
+static void intel_flush_display_plane(struct drm_i915_private *dev_priv,
+				      enum plane plane)
+{
+	u32 reg = DSPADDR(plane);
+	I915_WRITE(reg, I915_READ(reg));
+}
+
 /**
  * intel_enable_plane - enable a display plane on a given pipe
  * @dev_priv: i915 private structure
@@ -1583,18 +1594,7 @@ static void intel_enable_plane(struct drm_i915_private *dev_priv,
 		return;
 
 	I915_WRITE(reg, val | DISPLAY_PLANE_ENABLE);
-	intel_wait_for_vblank(dev_priv->dev, pipe);
-}
-
-/*
- * Plane regs are double buffered, going from enabled->disabled needs a
- * trigger in order to latch.  The display address reg provides this.
- */
-static void intel_flush_display_plane(struct drm_i915_private *dev_priv,
-				      enum plane plane)
-{
-	u32 reg = DSPADDR(plane);
-	I915_WRITE(reg, I915_READ(reg));
+	intel_flush_display_plane(dev_priv, plane);
 }
 
 /**
-- 
1.7.2.3

Re: [PATCH 3/8] drm/i915: Flush the plane enable using the DSPCNTR latch

[Keith Packard]

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


This comment is misleading -- the patch replaces a call to
intel_wait_for_vblank with a call to intel_flush_display_plane.

From my reading of the docs, enable requires two actions:

 1) DSPACNTR with enable going from 0->1
 2) wait for vblank

At disable, three actions are required:

 1) DSPACNTR with enable going from 1->0
 2) DSPASURF(965+)/DSPAADDR(otherwise) write to trigger the register updates
 3) wait for vblank

-- 
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 3/8] drm/i915: Flush the plane enable using the DSPCNTR latch

[Jesse Barnes]

On Fri, 18 Mar 2011 08:02:05 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> References: https://bugs.freedesktop.org/show_bug.cgi?id=34601
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++------------
>  1 files changed, 12 insertions(+), 12 deletions(-)

Yep, good catch.  Did you see any other places where we ought to flush?

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 3/8] drm/i915: Flush the plane enable using the DSPCNTR latch

[Chris Wilson]

On Fri, 18 Mar 2011 09:26:04 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Fri, 18 Mar 2011 08:02:05 +0000
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=34601
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++------------
> >  1 files changed, 12 insertions(+), 12 deletions(-)
> 
> Yep, good catch.  Did you see any other places where we ought to flush?

This was just an observed change in b24e717988. Do you want to take a
closer look at intel_enable_plane/intel_disable_plane?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 4/8] drm/i915: Only wait for vblank after pipe enabling on gen2

[Chris Wilson]

Later gen can proceed immediately with the modesetting as it is
automatically double-buffered.

References: https://bugs.freedesktop.org/show_bug.cgi?id=34601
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2f63999..95f21d1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1618,7 +1618,9 @@ static void intel_disable_plane(struct drm_i915_private *dev_priv,
 
 	I915_WRITE(reg, val & ~DISPLAY_PLANE_ENABLE);
 	intel_flush_display_plane(dev_priv, plane);
-	intel_wait_for_vblank(dev_priv->dev, pipe);
+
+	if (dev_priv->info->gen == 2)
+		intel_wait_for_vblank(dev_priv->dev, pipe);
 }
 
 static void disable_pch_dp(struct drm_i915_private *dev_priv,
-- 
1.7.2.3

Re: [PATCH 4/8] drm/i915: Only wait for vblank after pipe enabling on gen2

[Keith Packard]

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

On Fri, 18 Mar 2011 08:02:06 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Later gen can proceed immediately with the modesetting as it is
> automatically double-buffered.

This comment is misleading -- the change is in intel_disable_plane, not
intel_enable_pipe.

-- 
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 4/8] drm/i915: Only wait for vblank after pipe enabling on gen2

[Jesse Barnes]

On Fri, 18 Mar 2011 08:02:06 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Later gen can proceed immediately with the modesetting as it is
> automatically double-buffered.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=34601
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---

This is another one that triggers paranoia.  Since this is the disable
path it might be ok though.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 5/8] drm/i915: Prevent racy removal of request from client list

[Chris Wilson]

From: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>

When i915_gem_retire_requests_ring calls i915_gem_request_remove_from_client,
the client_list for that request may already be removed in i915_gem_release.
So we may call twice list_del(&request->client_list), resulting in an
oops like this report:

[126167.230394] BUG: unable to handle kernel paging request at 00100104
[126167.230699] IP: [<f8c2ce44>] i915_gem_retire_requests_ring+0xd4/0x240 [i915]
[126167.231042] *pdpt = 00000000314c1001 *pde = 0000000000000000
[126167.231314] Oops: 0002 [#1] SMP
[126167.231471] last sysfs file: /sys/devices/LNXSYSTM:00/device:00/PNP0C0A:00/power_supply/BAT1/current_now
[126167.231901] Modules linked in: snd_seq_dummy nls_utf8 isofs btrfs zlib_deflate libcrc32c ufs qnx4 hfsplus hfs minix ntfs vfat msdos fat jfs xfs exportfs reiserfs cryptd aes_i586 aes_generic binfmt_misc vboxnetadp vboxnetflt vboxdrv parport_pc ppdev snd_hda_codec_hdmi snd_hda_codec_conexant snd_hda_intel snd_hda_codec snd_hwdep arc4 snd_pcm snd_seq_midi snd_rawmidi snd_seq_midi_event snd_seq uvcvideo videodev snd_timer snd_seq_device joydev iwlagn iwlcore mac80211 snd cfg80211 soundcore i915 drm_kms_helper snd_page_alloc psmouse drm serio_raw i2c_algo_bit video lp parport usbhid hid sky2 sdhci_pci ahci sdhci libahci
[126167.232018]
[126167.232018] Pid: 1101, comm: Xorg Not tainted 2.6.38-6-generic-pae #34-Ubuntu Gateway                          MC7833U /
[126167.232018] EIP: 0060:[<f8c2ce44>] EFLAGS: 00213246 CPU: 0
[126167.232018] EIP is at i915_gem_retire_requests_ring+0xd4/0x240 [i915]
[126167.232018] EAX: 00200200 EBX: f1ac25b0 ECX: 00000040 EDX: 00100100
[126167.232018] ESI: f1a2801c EDI: e87fc060 EBP: ef4d7dd8 ESP: ef4d7db0
[126167.232018]  DS: 007b ES: 007b FS: 00d8 GS: 00e0 SS: 0068
[126167.232018] Process Xorg (pid: 1101, ti=ef4d6000 task=f1ba6500 task.ti=ef4d6000)
[126167.232018] Stack:
[126167.232018]  f1a28000 f1a2809c f1a28094 0058bd97 f1aa2400 f1a2801c 0058bd7b 0058bd85
[126167.232018]  f1a2801c f1a28000 ef4d7e38 f8c2e995 ef4d7e30 ef4d7e60 c14d1ebc f6b3a040
[126167.232018]  f1522cc0 000000db 00000000 f1ba6500 ffffffa1 00000000 00000001 f1a29214
[126167.232018] Call Trace:

Unfortunately the call trace reported was cut, but looking at debug
symbols the crash is at __list_del, when probably list_del is called
twice on the same request->client_list, as the dereferenced value is
LIST_POISON1 + 4, and by looking more at the debug symbols before
list_del call it should have being called by
i915_gem_request_remove_from_client

And as I can see in the code, it seems we indeed have the possibility
to remove a request->client_list twice, which would cause the above,
because we do list_del(&request->client_list) on both
i915_gem_request_remove_from_client and i915_gem_release

As Chris Wilson pointed out, it's indeed the case:
"(...) I had thought that the actual insertion/deletion was serialised
under the struct mutex and the intention of the spinlock was to protect
the unlocked list traversal during throttling. However, I missed that
i915_gem_release() is also called without struct mutex and so we do need
the double check for i915_gem_request_remove_from_client()."

This change does the required check to avoid the duplicate remove of
request->client_list.

Bugzilla: http://bugs.launchpad.net/bugs/733780
Cc: stable@kernel.org # 2.6.38
Signed-off-by: Herton Ronaldo Krzesinski <herton.krzesinski@canonical.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c4c2855..d4bf061 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1796,8 +1796,10 @@ i915_gem_request_remove_from_client(struct drm_i915_gem_request *request)
 		return;
 
 	spin_lock(&file_priv->mm.lock);
-	list_del(&request->client_list);
-	request->file_priv = NULL;
+	if (request->file_priv) {
+		list_del(&request->client_list);
+		request->file_priv = NULL;
+	}
 	spin_unlock(&file_priv->mm.lock);
 }
 
-- 
1.7.2.3

[PATCH 6/8] drm: Fix use-after-free in drm_gem_vm_close()

[Chris Wilson]

As we may release the last reference, we need to store the device in a
local variable in order to unlock afterwards.

[   60.140768] BUG: unable to handle kernel paging request at 6b6b6b9f
[   60.140973] IP: [<c1536d11>] __mutex_unlock_slowpath+0x5a/0x111
[   60.141014] *pdpt = 0000000024a54001 *pde = 0000000000000000
[   60.141014] Oops: 0002 [#1] PREEMPT SMP
[   60.141014] last sysfs file: /sys/devices/LNXSYSTM:00/device:00/PNP0A08:00/PNP0C0A:00/power_supply/BAT0/voltage_now
[   60.141014] Modules linked in: uvcvideo ath9k pegasus ath9k_common ath9k_hw hid_egalax ath3k joydev asus_laptop sparse_keymap battery input_polldev
[   60.141014]
[   60.141014] Pid: 771, comm: meego-ux-daemon Not tainted 2.6.37.2-7.1 #1 EXOPC EXOPG06411/EXOPG06411
[   60.141014] EIP: 0060:[<c1536d11>] EFLAGS: 00010046 CPU: 0
[   60.141014] EIP is at __mutex_unlock_slowpath+0x5a/0x111
[   60.141014] EAX: 00000100 EBX: 6b6b6b9b ECX: e9b4a1b0 EDX: e4a4e580
[   60.141014] ESI: db162558 EDI: 00000246 EBP: e480be50 ESP: e480be44
[   60.141014]  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
[   60.141014] Process meego-ux-daemon (pid: 771, ti=e480a000 task=e9b4a1b0 task.ti=e480a000)
[   60.141014] Stack:
[   60.141014]  e4a4e580 db162558 f5a2f838 e480be58 c1536dd0 e480be68 c125ab1b db162558
[   60.141014]  db1624e0 e480be78 c10ba071 db162558 f760241c e480be94 c10bb0bc 000155fe
[   60.141014]  f760241c f5a2f838 f5a2f8c8 00000000 e480bea4 c1037c24 00000000 f5a2f838
[   60.141014] Call Trace:
[   60.141014]  [<c1536dd0>] ? mutex_unlock+0x8/0xa
[   60.141014]  [<c125ab1b>] ? drm_gem_vm_close+0x39/0x3d
[   60.141014]  [<c10ba071>] ? remove_vma+0x2d/0x58
[   60.141014]  [<c10bb0bc>] ? exit_mmap+0x126/0x13f
[   60.141014]  [<c1037c24>] ? mmput+0x37/0x9a
[   60.141014]  [<c10d450d>] ? exec_mmap+0x178/0x19c
[   60.141014]  [<c1537f85>] ? _raw_spin_unlock+0x1d/0x36
[   60.141014]  [<c10d4eb0>] ? flush_old_exec+0x42/0x75
[   60.141014]  [<c1104442>] ? load_elf_binary+0x32a/0x922
[   60.141014]  [<c10d3f76>] ? search_binary_handler+0x200/0x2ea
[   60.141014]  [<c10d3ecf>] ? search_binary_handler+0x159/0x2ea
[   60.141014]  [<c1104118>] ? load_elf_binary+0x0/0x922
[   60.141014]  [<c10d56b2>] ? do_execve+0x1ff/0x2e6
[   60.141014]  [<c100970e>] ? sys_execve+0x2d/0x55
[   60.141014]  [<c1002a5a>] ? ptregs_execve+0x12/0x18
[   60.141014]  [<c10029dc>] ? sysenter_do_call+0x12/0x3c
[   60.141014]  [<c1530000>] ? init_centaur+0x9c/0x1ba
[   60.141014] Code: c1 00 75 0f ba 38 01 00 00 b8 8c 3a 6c c1 e8 cc 2e b0 ff 9c 58 8d 74 26 00 89 c7 fa 90 8d 74 26 00 e8 d2 b4 b2 ff b8 00 01 00 00 <f0> 66 0f c1 43 04 38 e0 74 07 f3 90 8a 43 04 eb f5 83 3d 64 ef
[   60.141014] EIP: [<c1536d11>] __mutex_unlock_slowpath+0x5a/0x111 SS:ESP 0068:e480be44
[   60.141014] CR2: 000000006b6b6b9f

Reported-by: Rusty Lynch <rusty.lynch@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@kernel.org
---
 drivers/gpu/drm/drm_gem.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 57ce27c..74e4ff5 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -499,11 +499,12 @@ EXPORT_SYMBOL(drm_gem_vm_open);
 void drm_gem_vm_close(struct vm_area_struct *vma)
 {
 	struct drm_gem_object *obj = vma->vm_private_data;
+	struct drm_device *dev = obj->dev;
 
-	mutex_lock(&obj->dev->struct_mutex);
+	mutex_lock(&dev->struct_mutex);
 	drm_vm_close_locked(vma);
 	drm_gem_object_unreference(obj);
-	mutex_unlock(&obj->dev->struct_mutex);
+	mutex_unlock(&dev->struct_mutex);
 }
 EXPORT_SYMBOL(drm_gem_vm_close);
 
-- 
1.7.2.3

Re: [Intel-gfx] [PATCH 6/8] drm: Fix use-after-free in drm_gem_vm_close()

[Keith Packard]

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

On Fri, 18 Mar 2011 08:02:08 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> As we may release the last reference, we need to store the device in a
> local variable in order to unlock afterwards.

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
keith.packard@intel.com

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

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

_______________________________________________
stable mailing list
stable@linux.kernel.org
http://linux.kernel.org/mailman/listinfo/stable

[PATCH 7/8] drm/i915: Re-enable self-refresh

[Chris Wilson]

A broken implementation of is_pot() prevented the detection of when a
singular pipe was enabled.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35402
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 95f21d1..fa925ce 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3889,7 +3889,7 @@ static bool g4x_compute_srwm(struct drm_device *dev,
 
 static inline bool single_plane_enabled(unsigned int mask)
 {
-	return mask && (mask & -mask) == 0;
+	return mask && (mask & -mask) == mask;
 }
 
 static void g4x_update_wm(struct drm_device *dev)
-- 
1.7.2.3

Re: [PATCH 7/8] drm/i915: Re-enable self-refresh

[Keith Packard]

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

On Fri, 18 Mar 2011 08:02:09 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> A broken implementation of is_pot() prevented the detection of when a
> singular pipe was enabled.

> -	return mask && (mask & -mask) == 0;
> +	return mask && (mask & -mask) == mask;

I think the form

        mask && (mask & (mask - 1)) == 0

is easier to reason about.

But, your fix is also correct as it finds the lowest bit set in 'mask'
and checks to see if that is the same as mask.

Reviewed-by: Keith Packard <keithp@keithp.com>

-- 
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 7/8] drm/i915: Re-enable self-refresh

[Chris Wilson]

On Fri, 18 Mar 2011 08:22:16 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Fri, 18 Mar 2011 08:02:09 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > A broken implementation of is_pot() prevented the detection of when a
> > singular pipe was enabled.
> 
> > -	return mask && (mask & -mask) == 0;
> > +	return mask && (mask & -mask) == mask;
> 
> I think the form
> 
>         mask && (mask & (mask - 1)) == 0
> 
> is easier to reason about.

I've reassigned authorship to Yuanhan and gone with your preferred form.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 7/8] drm/i915: Re-enable self-refresh

[Eric Anholt]

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

On Fri, 18 Mar 2011 08:22:16 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Fri, 18 Mar 2011 08:02:09 +0000, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > A broken implementation of is_pot() prevented the detection of when a
> > singular pipe was enabled.
> 
> > -	return mask && (mask & -mask) == 0;
> > +	return mask && (mask & -mask) == mask;
> 
> I think the form
> 
>         mask && (mask & (mask - 1)) == 0
> 
> is easier to reason about.
> 
> But, your fix is also correct as it finds the lowest bit set in 'mask'
> and checks to see if that is the same as mask.

At the risk of bikeshed, is_power_of_2() in log2.h is prettier. (note:
treats 0 as not power of two).

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 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 7/8] drm/i915: Re-enable self-refresh

[Chris Wilson]

On Fri, 18 Mar 2011 11:12:07 -0700, Eric Anholt <eric@anholt.net> wrote:
> At the risk of bikeshed, is_power_of_2() in log2.h is prettier. (note:
> treats 0 as not power of two).

I like the colour of your bikeshed, sold!.

Thanks, I missed that earlier when looking for an implementation of
is_pot().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 8/8] drm/i915: Fix tiling corruption from pipelined fencing

[Chris Wilson]

... even though it was disabled. A mistake in the handling of fence reuse
caused us to skip the vital delay of waiting for the object to finish
rendering before changing the register.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34584
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
[Note for 2.6.38-stable, we need to reintroduce the interruptible passing]
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |   44 +++++++++++++++-----------------------
 1 files changed, 17 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index d4bf061..c5dfb59 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2581,8 +2581,23 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
 		reg = &dev_priv->fence_regs[obj->fence_reg];
 		list_move_tail(&reg->lru_list, &dev_priv->mm.fence_list);
 
-		if (!obj->fenced_gpu_access && !obj->last_fenced_seqno)
-			pipelined = NULL;
+		if (obj->tiling_changed) {
+			ret = i915_gem_object_flush_fence(obj, pipelined);
+			if (ret)
+				return ret;
+
+			if (!obj->fenced_gpu_access && !obj->last_fenced_seqno)
+				pipelined = NULL;
+
+			if (pipelined) {
+				reg->setup_seqno =
+					i915_gem_next_request_seqno(pipelined);
+				obj->last_fenced_seqno = reg->setup_seqno;
+				obj->last_fenced_ring = pipelined;
+			}
+
+			goto update;
+		}
 
 		if (!pipelined) {
 			if (reg->setup_seqno) {
@@ -2601,31 +2616,6 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
 			ret = i915_gem_object_flush_fence(obj, pipelined);
 			if (ret)
 				return ret;
-		} else if (obj->tiling_changed) {
-			if (obj->fenced_gpu_access) {
-				if (obj->base.write_domain & I915_GEM_GPU_DOMAINS) {
-					ret = i915_gem_flush_ring(obj->ring,
-								  0, obj->base.write_domain);
-					if (ret)
-						return ret;
-				}
-
-				obj->fenced_gpu_access = false;
-			}
-		}
-
-		if (!obj->fenced_gpu_access && !obj->last_fenced_seqno)
-			pipelined = NULL;
-		BUG_ON(!pipelined && reg->setup_seqno);
-
-		if (obj->tiling_changed) {
-			if (pipelined) {
-				reg->setup_seqno =
-					i915_gem_next_request_seqno(pipelined);
-				obj->last_fenced_seqno = reg->setup_seqno;
-				obj->last_fenced_ring = pipelined;
-			}
-			goto update;
 		}
 
 		return 0;
-- 
1.7.2.3

Re: [PATCH 8/8] drm/i915: Fix tiling corruption from pipelined fencing

[Daniel Vetter]

On Fri, Mar 18, 2011 at 08:02:10AM +0000, Chris Wilson wrote:
> ... even though it was disabled. A mistake in the handling of fence reuse
> caused us to skip the vital delay of waiting for the object to finish
> rendering before changing the register.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Nice catch and good simplification of the code-flow. One nitpick about a
possible further cleanup below.

> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index d4bf061..c5dfb59 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2581,8 +2581,23 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
>  		reg = &dev_priv->fence_regs[obj->fence_reg];
>  		list_move_tail(&reg->lru_list, &dev_priv->mm.fence_list);
>  
> -		if (!obj->fenced_gpu_access && !obj->last_fenced_seqno)
> -			pipelined = NULL;
> +		if (obj->tiling_changed) {
> +			ret = i915_gem_object_flush_fence(obj, pipelined);
> +			if (ret)
> +				return ret;
> +
> +			if (!obj->fenced_gpu_access && !obj->last_fenced_seqno)
> +				pipelined = NULL;
> +
> +			if (pipelined) {
> +				reg->setup_seqno =
> +					i915_gem_next_request_seqno(pipelined);
> +				obj->last_fenced_seqno = reg->setup_seqno;
> +				obj->last_fenced_ring = pipelined;
> +			}
> +
> +			goto update;

I think we could move the update label 3 lines up, which would make the
above if(pipelined) clause unnecessary. Maybe even drop the goto and
extract the tail of get_fence as a function of its own.

> +		}
>  
>  		if (!pipelined) {
>  			if (reg->setup_seqno) {
> @@ -2601,31 +2616,6 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
>  			ret = i915_gem_object_flush_fence(obj, pipelined);
>  			if (ret)
>  				return ret;
> -		} else if (obj->tiling_changed) {
> -			if (obj->fenced_gpu_access) {
> -				if (obj->base.write_domain & I915_GEM_GPU_DOMAINS) {
> -					ret = i915_gem_flush_ring(obj->ring,
> -								  0, obj->base.write_domain);
> -					if (ret)
> -						return ret;
> -				}
> -
> -				obj->fenced_gpu_access = false;
> -			}
> -		}
> -
> -		if (!obj->fenced_gpu_access && !obj->last_fenced_seqno)
> -			pipelined = NULL;
> -		BUG_ON(!pipelined && reg->setup_seqno);
> -
> -		if (obj->tiling_changed) {
> -			if (pipelined) {
> -				reg->setup_seqno =
> -					i915_gem_next_request_seqno(pipelined);
> -				obj->last_fenced_seqno = reg->setup_seqno;
> -				obj->last_fenced_ring = pipelined;
> -			}
> -			goto update;
>  		}
>  
>  		return 0;
> -- 
> 1.7.2.3
> 

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

Re: [PATCH 8/8] drm/i915: Fix tiling corruption from pipelined fencing

[Chris Wilson]

On Sat, 19 Mar 2011 23:17:56 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Mar 18, 2011 at 08:02:10AM +0000, Chris Wilson wrote:
> > ... even though it was disabled. A mistake in the handling of fence reuse
> > caused us to skip the vital delay of waiting for the object to finish
> > rendering before changing the register.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Nice catch and good simplification of the code-flow. One nitpick about a
> possible further cleanup below.

Hints for further cleanups are always appreciated. :)

> 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index d4bf061..c5dfb59 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -2581,8 +2581,23 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
> >  		reg = &dev_priv->fence_regs[obj->fence_reg];
> >  		list_move_tail(&reg->lru_list, &dev_priv->mm.fence_list);
> >  
> > -		if (!obj->fenced_gpu_access && !obj->last_fenced_seqno)
> > -			pipelined = NULL;
> > +		if (obj->tiling_changed) {
> > +			ret = i915_gem_object_flush_fence(obj, pipelined);
> > +			if (ret)
> > +				return ret;
> > +
> > +			if (!obj->fenced_gpu_access && !obj->last_fenced_seqno)
> > +				pipelined = NULL;
> > +
> > +			if (pipelined) {
> > +				reg->setup_seqno =
> > +					i915_gem_next_request_seqno(pipelined);
> > +				obj->last_fenced_seqno = reg->setup_seqno;
> > +				obj->last_fenced_ring = pipelined;
> > +			}
> > +
> > +			goto update;
> 
> I think we could move the update label 3 lines up, which would make the
> above if(pipelined) clause unnecessary. Maybe even drop the goto and
> extract the tail of get_fence as a function of its own.

Right, I did this in a later patch. I want to keep this as a simple
rearrangement of the code since that is the minimal fix and improve upon
that with further patches. Sound good?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 8/8] drm/i915: Fix tiling corruption from pipelined fencing

[Daniel Vetter]

On Sat, Mar 19, 2011 at 10:25:36PM +0000, Chris Wilson wrote:
> Right, I did this in a later patch. I want to keep this as a simple
> rearrangement of the code since that is the minimal fix and improve upon
> that with further patches. Sound good?

Of course!

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