Re: [ 08/32] drm/i915: correctly order the ring init sequence
On Fri, Aug 24, 2012 at 08:03:14PM -0300, Herton Ronaldo Krzesinski wrote: > On Sun, Aug 19, 2012 at 08:57:04PM -0700, Greg Kroah-Hartman wrote: > > From: Greg KH > > > > 3.4-stable review patch. If anyone has any objections, please let me know. > > > > -- > > > > From: Daniel Vetter > > > > commit 0d8957c8a90bbb5d34fab9a304459448a5131e06 upstream. > > > > We may only start to set up the new register values after having > > confirmed that the ring is truely off. Otherwise the hw might lose the > > newly written register values. This is caught later on in the init > > sequence, when we check whether the register writes have stuck. > > > > Reviewed-by: Jani Nikula > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50522 > > Tested-by: Yang Guang > > Signed-off-by: Daniel Vetter > > Signed-off-by: Greg Kroah-Hartman > > I think with this commit also the following commits should be picked for > 3.4 right? (as suggested for 3.0): > > f01db988ef6f6c70a6cc36ee71e4a98a68901229 > b7884eb45ec98c0d34c7f49005ae9d4b4b4e38f6 > > Just reporting that I tested this 3.4.10 proposed update with the two > commits above cherry-picked/backported applied, and worked ok. 3.4.10 as-is works fine for me, so I'm a bit leary of wanting to add more patches to it (or 3.0.42), until I get some kind of confirmation that these two patches are really needed. Anyone else having problems here? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 08/32] drm/i915: correctly order the ring init sequence
On Fri, Aug 24, 2012 at 08:03:14PM -0300, Herton Ronaldo Krzesinski wrote: On Sun, Aug 19, 2012 at 08:57:04PM -0700, Greg Kroah-Hartman wrote: From: Greg KH gre...@linuxfoundation.org 3.4-stable review patch. If anyone has any objections, please let me know. -- From: Daniel Vetter daniel.vet...@ffwll.ch commit 0d8957c8a90bbb5d34fab9a304459448a5131e06 upstream. We may only start to set up the new register values after having confirmed that the ring is truely off. Otherwise the hw might lose the newly written register values. This is caught later on in the init sequence, when we check whether the register writes have stuck. Reviewed-by: Jani Nikula jani.nik...@intel.com Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50522 Tested-by: Yang Guang guang.a.y...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org I think with this commit also the following commits should be picked for 3.4 right? (as suggested for 3.0): f01db988ef6f6c70a6cc36ee71e4a98a68901229 b7884eb45ec98c0d34c7f49005ae9d4b4b4e38f6 Just reporting that I tested this 3.4.10 proposed update with the two commits above cherry-picked/backported applied, and worked ok. 3.4.10 as-is works fine for me, so I'm a bit leary of wanting to add more patches to it (or 3.0.42), until I get some kind of confirmation that these two patches are really needed. Anyone else having problems here? thanks, greg k-h -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ 08/32] drm/i915: correctly order the ring init sequence
On Sun, Aug 19, 2012 at 08:57:04PM -0700, Greg Kroah-Hartman wrote:
> From: Greg KH
>
> 3.4-stable review patch. If anyone has any objections, please let me know.
>
> --
>
> From: Daniel Vetter
>
> commit 0d8957c8a90bbb5d34fab9a304459448a5131e06 upstream.
>
> We may only start to set up the new register values after having
> confirmed that the ring is truely off. Otherwise the hw might lose the
> newly written register values. This is caught later on in the init
> sequence, when we check whether the register writes have stuck.
>
> Reviewed-by: Jani Nikula
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50522
> Tested-by: Yang Guang
> Signed-off-by: Daniel Vetter
> Signed-off-by: Greg Kroah-Hartman
I think with this commit also the following commits should be picked for
3.4 right? (as suggested for 3.0):
f01db988ef6f6c70a6cc36ee71e4a98a68901229
b7884eb45ec98c0d34c7f49005ae9d4b4b4e38f6
Just reporting that I tested this 3.4.10 proposed update with the two
commits above cherry-picked/backported applied, and worked ok.
The first cherry-picked cleanly, while
b7884eb45ec98c0d34c7f49005ae9d4b4b4e38f6 needed backporting for 3.4,
like happened with 3.0, this is a proposed backport which I
applied/tested, is similar to 3.0 and 3.2 versions:
>From a2712ae26afde5be2bc62080755d1324164f53d3 Mon Sep 17 00:00:00 2001
From: Daniel Vetter
Date: Mon, 4 Jun 2012 11:18:15 +0200
Subject: [PATCH] drm/i915: hold forcewake around ring hw init
Empirical evidence suggests that we need to: On at least one ivb
machine when running the hangman i-g-t test, the rings don't properly
initialize properly - the RING_START registers seems to be stuck at
all zeros.
Holding forcewake around this register init sequences makes chip reset
reliable again. Note that this is not the first such issue:
commit f01db988ef6f6c70a6cc36ee71e4a98a68901229
Author: Sean Paul
Date: Fri Mar 16 12:43:22 2012 -0400
drm/i915: Add wait_for in init_ring_common
added delay loops to make RING_START and RING_CTL initialization
reliable on the blt ring at boot-up. So I guess it won't hurt if we do
this unconditionally for all force_wake needing gpus.
To avoid copy of the HAS_FORCE_WAKE check I've added a new
intel_info bit for that.
v2: Fixup missing commas in static struct and properly handling the
error case in init_ring_common, both noticed by Jani Nikula.
Cc: sta...@vger.kernel.org
Reported-and-tested-by: Yang Guang
Reviewed-by: Eugeni Dodonov
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50522
Signed-Off-by: Daniel Vetter
[herton: backport to 3.4:
- adjust for different struct intel_device_info layouts
- drop changes to Haswell/Valleyview, not present in 3.4
- NEEDS_FORCE_WAKE is on i915_drv.h, and doesn't have IS_VALLEYVIEW ]
Signed-off-by: Herton Ronaldo Krzesinski
---
drivers/gpu/drm/i915/i915_drv.c |4
drivers/gpu/drm/i915/i915_drv.h |7 +--
drivers/gpu/drm/i915/intel_ringbuffer.c | 16 +---
3 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ae8a64f..c654557 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -224,6 +224,7 @@ static const struct intel_device_info
intel_sandybridge_d_info = {
.has_bsd_ring = 1,
.has_blt_ring = 1,
.has_llc = 1,
+ .has_force_wake = 1,
};
static const struct intel_device_info intel_sandybridge_m_info = {
@@ -233,6 +234,7 @@ static const struct intel_device_info
intel_sandybridge_m_info = {
.has_bsd_ring = 1,
.has_blt_ring = 1,
.has_llc = 1,
+ .has_force_wake = 1,
};
static const struct intel_device_info intel_ivybridge_d_info = {
@@ -241,6 +243,7 @@ static const struct intel_device_info
intel_ivybridge_d_info = {
.has_bsd_ring = 1,
.has_blt_ring = 1,
.has_llc = 1,
+ .has_force_wake = 1,
};
static const struct intel_device_info intel_ivybridge_m_info = {
@@ -250,6 +253,7 @@ static const struct intel_device_info
intel_ivybridge_m_info = {
.has_bsd_ring = 1,
.has_blt_ring = 1,
.has_llc = 1,
+ .has_force_wake = 1,
};
static const struct pci_device_id pciidlist[] = { /* aka */
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5fabc6c..a2117b2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -255,6 +255,7 @@ struct intel_device_info {
u8 is_broadwater:1;
u8 is_crestline:1;
u8 is_ivybridge:1;
+ u8 has_force_wake:1;
u8 has_fbc:1;
u8 has_pipe_cxsr:1;
u8 has_hotplug:1;
@@ -1051,6 +1052,8 @@ struct drm_i915_file_private {
#define HAS_PCH_CPT(dev) (INTEL_PCH_TYPE(dev) == PCH_CPT)
#define HAS_PCH_IBX(dev) (INTEL_PCH_TYPE(dev) == PCH_IBX)
+#define HAS_FORCE_WAKE(dev) (INTEL_INFO(dev)->has_force_wake)
+
#include "i915_trace.h"
Re: [ 08/32] drm/i915: correctly order the ring init sequence
On Sun, Aug 19, 2012 at 08:57:04PM -0700, Greg Kroah-Hartman wrote:
From: Greg KH gre...@linuxfoundation.org
3.4-stable review patch. If anyone has any objections, please let me know.
--
From: Daniel Vetter daniel.vet...@ffwll.ch
commit 0d8957c8a90bbb5d34fab9a304459448a5131e06 upstream.
We may only start to set up the new register values after having
confirmed that the ring is truely off. Otherwise the hw might lose the
newly written register values. This is caught later on in the init
sequence, when we check whether the register writes have stuck.
Reviewed-by: Jani Nikula jani.nik...@intel.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50522
Tested-by: Yang Guang guang.a.y...@intel.com
Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch
Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org
I think with this commit also the following commits should be picked for
3.4 right? (as suggested for 3.0):
f01db988ef6f6c70a6cc36ee71e4a98a68901229
b7884eb45ec98c0d34c7f49005ae9d4b4b4e38f6
Just reporting that I tested this 3.4.10 proposed update with the two
commits above cherry-picked/backported applied, and worked ok.
The first cherry-picked cleanly, while
b7884eb45ec98c0d34c7f49005ae9d4b4b4e38f6 needed backporting for 3.4,
like happened with 3.0, this is a proposed backport which I
applied/tested, is similar to 3.0 and 3.2 versions:
From a2712ae26afde5be2bc62080755d1324164f53d3 Mon Sep 17 00:00:00 2001
From: Daniel Vetter daniel.vet...@ffwll.ch
Date: Mon, 4 Jun 2012 11:18:15 +0200
Subject: [PATCH] drm/i915: hold forcewake around ring hw init
Empirical evidence suggests that we need to: On at least one ivb
machine when running the hangman i-g-t test, the rings don't properly
initialize properly - the RING_START registers seems to be stuck at
all zeros.
Holding forcewake around this register init sequences makes chip reset
reliable again. Note that this is not the first such issue:
commit f01db988ef6f6c70a6cc36ee71e4a98a68901229
Author: Sean Paul seanp...@chromium.org
Date: Fri Mar 16 12:43:22 2012 -0400
drm/i915: Add wait_for in init_ring_common
added delay loops to make RING_START and RING_CTL initialization
reliable on the blt ring at boot-up. So I guess it won't hurt if we do
this unconditionally for all force_wake needing gpus.
To avoid copypasting of the HAS_FORCE_WAKE check I've added a new
intel_info bit for that.
v2: Fixup missing commas in static struct and properly handling the
error case in init_ring_common, both noticed by Jani Nikula.
Cc: sta...@vger.kernel.org
Reported-and-tested-by: Yang Guang guang.a.y...@intel.com
Reviewed-by: Eugeni Dodonov eugeni.dodo...@intel.com
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50522
Signed-Off-by: Daniel Vetter daniel.vet...@ffwll.ch
[herton: backport to 3.4:
- adjust for different struct intel_device_info layouts
- drop changes to Haswell/Valleyview, not present in 3.4
- NEEDS_FORCE_WAKE is on i915_drv.h, and doesn't have IS_VALLEYVIEW ]
Signed-off-by: Herton Ronaldo Krzesinski herton.krzesin...@canonical.com
---
drivers/gpu/drm/i915/i915_drv.c |4
drivers/gpu/drm/i915/i915_drv.h |7 +--
drivers/gpu/drm/i915/intel_ringbuffer.c | 16 +---
3 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ae8a64f..c654557 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -224,6 +224,7 @@ static const struct intel_device_info
intel_sandybridge_d_info = {
.has_bsd_ring = 1,
.has_blt_ring = 1,
.has_llc = 1,
+ .has_force_wake = 1,
};
static const struct intel_device_info intel_sandybridge_m_info = {
@@ -233,6 +234,7 @@ static const struct intel_device_info
intel_sandybridge_m_info = {
.has_bsd_ring = 1,
.has_blt_ring = 1,
.has_llc = 1,
+ .has_force_wake = 1,
};
static const struct intel_device_info intel_ivybridge_d_info = {
@@ -241,6 +243,7 @@ static const struct intel_device_info
intel_ivybridge_d_info = {
.has_bsd_ring = 1,
.has_blt_ring = 1,
.has_llc = 1,
+ .has_force_wake = 1,
};
static const struct intel_device_info intel_ivybridge_m_info = {
@@ -250,6 +253,7 @@ static const struct intel_device_info
intel_ivybridge_m_info = {
.has_bsd_ring = 1,
.has_blt_ring = 1,
.has_llc = 1,
+ .has_force_wake = 1,
};
static const struct pci_device_id pciidlist[] = { /* aka */
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5fabc6c..a2117b2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -255,6 +255,7 @@ struct intel_device_info {
u8 is_broadwater:1;
u8 is_crestline:1;
u8 is_ivybridge:1;
+ u8 has_force_wake:1;
u8 has_fbc:1;
u8 has_pipe_cxsr:1;
u8 has_hotplug:1;
[ 08/32] drm/i915: correctly order the ring init sequence
From: Greg KH 3.4-stable review patch. If anyone has any objections, please let me know. -- From: Daniel Vetter commit 0d8957c8a90bbb5d34fab9a304459448a5131e06 upstream. We may only start to set up the new register values after having confirmed that the ring is truely off. Otherwise the hw might lose the newly written register values. This is caught later on in the init sequence, when we check whether the register writes have stuck. Reviewed-by: Jani Nikula Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50522 Tested-by: Yang Guang Signed-off-by: Daniel Vetter Signed-off-by: Greg Kroah-Hartman --- drivers/gpu/drm/i915/intel_ringbuffer.c |7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -258,8 +258,6 @@ static int init_ring_common(struct intel I915_WRITE_HEAD(ring, 0); ring->write_tail(ring, 0); - /* Initialize the ring. */ - I915_WRITE_START(ring, obj->gtt_offset); head = I915_READ_HEAD(ring) & HEAD_ADDR; /* G45 ring initialization fails to reset head to zero */ @@ -285,6 +283,11 @@ static int init_ring_common(struct intel } } + /* Initialize the ring. This must happen _after_ we've cleared the ring +* registers with the above sequence (the readback of the HEAD registers +* also enforces ordering), otherwise the hw might lose the new ring +* register values. */ + I915_WRITE_START(ring, obj->gtt_offset); I915_WRITE_CTL(ring, ((ring->size - PAGE_SIZE) & RING_NR_PAGES) | RING_VALID); -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[ 08/32] drm/i915: correctly order the ring init sequence
From: Greg KH gre...@linuxfoundation.org 3.4-stable review patch. If anyone has any objections, please let me know. -- From: Daniel Vetter daniel.vet...@ffwll.ch commit 0d8957c8a90bbb5d34fab9a304459448a5131e06 upstream. We may only start to set up the new register values after having confirmed that the ring is truely off. Otherwise the hw might lose the newly written register values. This is caught later on in the init sequence, when we check whether the register writes have stuck. Reviewed-by: Jani Nikula jani.nik...@intel.com Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50522 Tested-by: Yang Guang guang.a.y...@intel.com Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch Signed-off-by: Greg Kroah-Hartman gre...@linuxfoundation.org --- drivers/gpu/drm/i915/intel_ringbuffer.c |7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -258,8 +258,6 @@ static int init_ring_common(struct intel I915_WRITE_HEAD(ring, 0); ring-write_tail(ring, 0); - /* Initialize the ring. */ - I915_WRITE_START(ring, obj-gtt_offset); head = I915_READ_HEAD(ring) HEAD_ADDR; /* G45 ring initialization fails to reset head to zero */ @@ -285,6 +283,11 @@ static int init_ring_common(struct intel } } + /* Initialize the ring. This must happen _after_ we've cleared the ring +* registers with the above sequence (the readback of the HEAD registers +* also enforces ordering), otherwise the hw might lose the new ring +* register values. */ + I915_WRITE_START(ring, obj-gtt_offset); I915_WRITE_CTL(ring, ((ring-size - PAGE_SIZE) RING_NR_PAGES) | RING_VALID); -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
