Re: Linux 3.16-rc2

Re: Linux 3.16-rc2

[Thomas Meyer]

Am Samstag, den 21.06.2014, 19:22 -1000 schrieb Linus Torvalds:
> It's a day early, but tomorrow ends up being inconvenient for me due
> to being on the road most of the day, so here you are. These days most
> people send me their pull requests and patches during the week, so
> it's not like I expect that a Sunday release would have made much of a
> difference. And it's also not like I didn't have enough changes for
> making a rc2 release.
> 
> Anyway, enough excuses. 3.16-rc2 is out, and contains the usual
> assortment of fixes all over the map. The most unusual part at this
> point is how the sparc changes stand out (at almost 40% of the patch
> by bulk), but they are basically all just sparse warning cleanups.
> 
> Similarly, some Nouveau drm changes standing out size-wise, but again
> those are largely due to small firmware fixes resulting in big
> generated changes. The actual real changes are fairly small.
> 
> Ignoring those two unusually large changes (in lines), everything else
> looks fairly normal. There are driver changes, some tooling updates
> (particularly perf), and various other arch updates (arm, s390,
> unicore32, x86..). And just misc random stuff all over the place -
> rtmutex, btrfs, yadda yadda.
> 
> The shortlog is not tiny, but small enough to include here, so you can
> see the details there if you care.
> 
> Please do go test it out,
> 

the i915 driver is still broken in 3.16-rc2. Resume from ram crashes the
X server.

First bad commit is:

# first bad commit: [78f2975eec9faff353a6194e854d3d39907bab68] drm/i915: Move all ring resets before setting the HWS page

commit 78f2975eec9faff353a6194e854d3d39907bab68
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Apr 2 16:36:07 2014 +0100

    drm/i915: Move all ring resets before setting the HWS page
    
    In commit a51435a3137ad8ae75c288c39bd2d8b2696bae8f
    Author: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
    Date:   Wed Mar 12 16:39:40 2014 +0530
    
        drm/i915: disable rings before HW status page setup
    
    we reordered stopping the rings to do so before we set the HWS register.
    However, there is an extra workaround for g45 to reset the rings twice,
    and for consistency we should apply that workaround before setting the
    HWS to be sure that the rings are truly stopped.
    
    Reference: http://lkml.kernel.org/r/20140423202248.GA3621@amd.pavel.ucw.cz
    Tested-by: Pavel Machek <pavel@ucw.cz>
    Cc: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
    Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
    Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Above commit is not revertable anymore on 3.16-rc2 without conflict.

Re: Linux 3.16-rc2

[Jani Nikula]

On Tue, 24 Jun 2014, Thomas Meyer <thomas@m3y3r.de> wrote:
> the i915 driver is still broken in 3.16-rc2. Resume from ram crashes the
> X server.

This is not new to 3.16-rc2; apparently we've had it since v3.15-rc4
[1]. Also related [2].

Chris, any fresh ideas?

Thomas, please consider filing a bug against DRM/Intel at [3]. They will
have a better chance of not being forgotten as the mail thread goes
cold.

Thanks,
Jani.


[1] http://thread.gmane.org/gmane.linux.kernel/1700872
[2] https://bugs.freedesktop.org/show_bug.cgi?id=76554
[3] https://bugs.freedesktop.org/enter_bug.cgi?product=DRI


>
> First bad commit is:
>
> # first bad commit: [78f2975eec9faff353a6194e854d3d39907bab68] drm/i915: Move all ring resets before setting the HWS page
>
> commit 78f2975eec9faff353a6194e854d3d39907bab68
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Wed Apr 2 16:36:07 2014 +0100
>
>     drm/i915: Move all ring resets before setting the HWS page
>     
>     In commit a51435a3137ad8ae75c288c39bd2d8b2696bae8f
>     Author: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>     Date:   Wed Mar 12 16:39:40 2014 +0530
>     
>         drm/i915: disable rings before HW status page setup
>     
>     we reordered stopping the rings to do so before we set the HWS register.
>     However, there is an extra workaround for g45 to reset the rings twice,
>     and for consistency we should apply that workaround before setting the
>     HWS to be sure that the rings are truly stopped.
>     
>     Reference: http://lkml.kernel.org/r/20140423202248.GA3621@amd.pavel.ucw.cz
>     Tested-by: Pavel Machek <pavel@ucw.cz>
>     Cc: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>     Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>     Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>     Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>
> Above commit is not revertable anymore on 3.16-rc2 without conflict.
>
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: Linux 3.16-rc2

[Chris Wilson]

On Tue, Jun 24, 2014 at 02:06:24PM +0300, Jani Nikula wrote:
> On Tue, 24 Jun 2014, Thomas Meyer <thomas@m3y3r.de> wrote:
> > the i915 driver is still broken in 3.16-rc2. Resume from ram crashes the
> > X server.
> 
> This is not new to 3.16-rc2; apparently we've had it since v3.15-rc4
> [1]. Also related [2].
> 
> Chris, any fresh ideas?

Nope. The bug is https://bugs.freedesktop.org/show_bug.cgi?id=76554
everything we know and have tried is there. Which is not much more than
at time of the original incarnation:

commit 50aa253d820ad4577e2231202f2c8fd89f9dc4e6
Author: Keith Packard <keithp@keithp.com>
Date:   Tue Oct 14 17:20:35 2008 -0700

    i915: Fix up ring initialization to cover G45 oddities
    
    G45 appears quite sensitive to ring initialization register writes,
    sometimes leaving the HEAD register with the START register contents. Check
    to make sure HEAD is reset correctly when START is written, and fix it up,
    screaming loudly.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: Linux 3.16-rc2

[Thomas Meyer]

Am Dienstag, den 24.06.2014, 12:57 +0100 schrieb Chris Wilson:
> On Tue, Jun 24, 2014 at 02:06:24PM +0300, Jani Nikula wrote:
> > On Tue, 24 Jun 2014, Thomas Meyer <thomas@m3y3r.de> wrote:
> > > the i915 driver is still broken in 3.16-rc2. Resume from ram crashes the
> > > X server.
> > 
> > This is not new to 3.16-rc2; apparently we've had it since v3.15-rc4
> > [1]. Also related [2].
> > 
> > Chris, any fresh ideas?
> 
> Nope. The bug is https://bugs.freedesktop.org/show_bug.cgi?id=76554
> everything we know and have tried is there. Which is not much more than
> at time of the original incarnation:
> 
> commit 50aa253d820ad4577e2231202f2c8fd89f9dc4e6
> Author: Keith Packard <keithp@keithp.com>
> Date:   Tue Oct 14 17:20:35 2008 -0700
> 
>     i915: Fix up ring initialization to cover G45 oddities
>     
>     G45 appears quite sensitive to ring initialization register writes,
>     sometimes leaving the HEAD register with the START register contents. Check
>     to make sure HEAD is reset correctly when START is written, and fix it up,
>     screaming loudly.
> -Chris
> 

Hi,

so why not revert 78f2975eec9faff353a6194e854d3d39907bab68 (drm/i915:
Move all ring resets before setting the HWS page) ?

Without this commit 3.15 is super stable for me. This seems to be also
true for others according to above bug report.

thomas

Re: Linux 3.16-rc2

[Chris Wilson]

On Tue, Jun 24, 2014 at 02:24:30PM +0200, Thomas Meyer wrote:
> Am Dienstag, den 24.06.2014, 12:57 +0100 schrieb Chris Wilson:
> > On Tue, Jun 24, 2014 at 02:06:24PM +0300, Jani Nikula wrote:
> > > On Tue, 24 Jun 2014, Thomas Meyer <thomas@m3y3r.de> wrote:
> > > > the i915 driver is still broken in 3.16-rc2. Resume from ram crashes the
> > > > X server.
> > > 
> > > This is not new to 3.16-rc2; apparently we've had it since v3.15-rc4
> > > [1]. Also related [2].
> > > 
> > > Chris, any fresh ideas?
> > 
> > Nope. The bug is https://bugs.freedesktop.org/show_bug.cgi?id=76554
> > everything we know and have tried is there. Which is not much more than
> > at time of the original incarnation:
> > 
> > commit 50aa253d820ad4577e2231202f2c8fd89f9dc4e6
> > Author: Keith Packard <keithp@keithp.com>
> > Date:   Tue Oct 14 17:20:35 2008 -0700
> > 
> >     i915: Fix up ring initialization to cover G45 oddities
> >     
> >     G45 appears quite sensitive to ring initialization register writes,
> >     sometimes leaving the HEAD register with the START register contents. Check
> >     to make sure HEAD is reset correctly when START is written, and fix it up,
> >     screaming loudly.
> > -Chris
> > 
> 
> Hi,
> 
> so why not revert 78f2975eec9faff353a6194e854d3d39907bab68 (drm/i915:
> Move all ring resets before setting the HWS page) ?

Because that patch was in response to a boot time regression.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: Linux 3.16-rc2

[Pavel Machek]

On Tue 2014-06-24 13:27:37, Chris Wilson wrote:
> On Tue, Jun 24, 2014 at 02:24:30PM +0200, Thomas Meyer wrote:
> > Am Dienstag, den 24.06.2014, 12:57 +0100 schrieb Chris Wilson:
> > > On Tue, Jun 24, 2014 at 02:06:24PM +0300, Jani Nikula wrote:
> > > > On Tue, 24 Jun 2014, Thomas Meyer <thomas@m3y3r.de> wrote:
> > > > > the i915 driver is still broken in 3.16-rc2. Resume from ram crashes the
> > > > > X server.
> > > > 
> > > > This is not new to 3.16-rc2; apparently we've had it since v3.15-rc4
> > > > [1]. Also related [2].
> > > > 
> > > > Chris, any fresh ideas?
> > > 
> > > Nope. The bug is https://bugs.freedesktop.org/show_bug.cgi?id=76554
> > > everything we know and have tried is there. Which is not much more than
> > > at time of the original incarnation:
> > > 
> > > commit 50aa253d820ad4577e2231202f2c8fd89f9dc4e6
> > > Author: Keith Packard <keithp@keithp.com>
> > > Date:   Tue Oct 14 17:20:35 2008 -0700
> > > 
> > >     i915: Fix up ring initialization to cover G45 oddities
> > >     
> > >     G45 appears quite sensitive to ring initialization register writes,
> > >     sometimes leaving the HEAD register with the START register contents. Check
> > >     to make sure HEAD is reset correctly when START is written, and fix it up,
> > >     screaming loudly.
> > > -Chris
> > > 
> > 
> > Hi,
> > 
> > so why not revert 78f2975eec9faff353a6194e854d3d39907bab68 (drm/i915:
> > Move all ring resets before setting the HWS page) ?
> 
> Because that patch was in response to a boot time regression.

It seems we are in a fairly ugly "fix one board, it breaks another" iterations, right?

(BTW, if you apply patch to fix this bug, could you Cc me? I have strange feeling
that it will break my setup... Actually, it probably makes sense to Cc all the people
who reported problems with ring initialization...

What patch caused the boot time regression you are talking about? We need to get 
list of commits involved in this, and revert the original one...

Jiri Kosina may have the same problem, right?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Linux 3.16-rc2

[Chris Wilson]

On Mon, Jun 30, 2014 at 12:02:20PM +0200, Pavel Machek wrote:
> On Tue 2014-06-24 13:27:37, Chris Wilson wrote:
> > On Tue, Jun 24, 2014 at 02:24:30PM +0200, Thomas Meyer wrote:
> > > Am Dienstag, den 24.06.2014, 12:57 +0100 schrieb Chris Wilson:
> > > > On Tue, Jun 24, 2014 at 02:06:24PM +0300, Jani Nikula wrote:
> > > > > On Tue, 24 Jun 2014, Thomas Meyer <thomas@m3y3r.de> wrote:
> > > > > > the i915 driver is still broken in 3.16-rc2. Resume from ram crashes the
> > > > > > X server.
> > > > > 
> > > > > This is not new to 3.16-rc2; apparently we've had it since v3.15-rc4
> > > > > [1]. Also related [2].
> > > > > 
> > > > > Chris, any fresh ideas?
> > > > 
> > > > Nope. The bug is https://bugs.freedesktop.org/show_bug.cgi?id=76554
> > > > everything we know and have tried is there. Which is not much more than
> > > > at time of the original incarnation:
> > > > 
> > > > commit 50aa253d820ad4577e2231202f2c8fd89f9dc4e6
> > > > Author: Keith Packard <keithp@keithp.com>
> > > > Date:   Tue Oct 14 17:20:35 2008 -0700
> > > > 
> > > >     i915: Fix up ring initialization to cover G45 oddities
> > > >     
> > > >     G45 appears quite sensitive to ring initialization register writes,
> > > >     sometimes leaving the HEAD register with the START register contents. Check
> > > >     to make sure HEAD is reset correctly when START is written, and fix it up,
> > > >     screaming loudly.
> > > > -Chris
> > > > 
> > > 
> > > Hi,
> > > 
> > > so why not revert 78f2975eec9faff353a6194e854d3d39907bab68 (drm/i915:
> > > Move all ring resets before setting the HWS page) ?
> > 
> > Because that patch was in response to a boot time regression.
> 
> It seems we are in a fairly ugly "fix one board, it breaks another" iterations, right?
> 
> (BTW, if you apply patch to fix this bug, could you Cc me? I have strange feeling
> that it will break my setup... Actually, it probably makes sense to Cc all the people
> who reported problems with ring initialization...
> 
> What patch caused the boot time regression you are talking about? We need to get 
> list of commits involved in this, and revert the original one...

commit 9991ae787a0c87fe7c783b4b6f4754c3cdbb6213
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Apr 2 16:36:07 2014 +0100

    drm/i915: Move all ring resets before setting the HWS page
    
    In commit a51435a3137ad8ae75c288c39bd2d8b2696bae8f
    Author: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
    Date:   Wed Mar 12 16:39:40 2014 +0530
    
        drm/i915: disable rings before HW status page setup
    
    we reordered stopping the rings to do so before we set the HWS register.
    However, there is an extra workaround for g45 to reset the rings twice,
    and for consistency we should apply that workaround before setting the
    HWS to be sure that the rings are truly stopped.
    
    Cc: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
    Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
    Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>


commit a51435a3137ad8ae75c288c39bd2d8b2696bae8f
Author: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
Date:   Wed Mar 12 16:39:40 2014 +0530

    drm/i915: disable rings before HW status page setup
    
    Rings should be idle before issuing sync_flush
    (in intel_ring_setup_status_page). This patch moves the ring
    disabling before doing the HW status page setup.
    
    Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
    Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
    Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>


-- 
Chris Wilson, Intel Open Source Technology Centre

Re: Linux 3.16-rc2

[Thomas Meyer]

Am Montag, den 30.06.2014, 11:09 +0100 schrieb Chris Wilson:
> On Mon, Jun 30, 2014 at 12:02:20PM +0200, Pavel Machek wrote:
> > On Tue 2014-06-24 13:27:37, Chris Wilson wrote:
> > > On Tue, Jun 24, 2014 at 02:24:30PM +0200, Thomas Meyer wrote:
> > > > Am Dienstag, den 24.06.2014, 12:57 +0100 schrieb Chris Wilson:
> > > > > On Tue, Jun 24, 2014 at 02:06:24PM +0300, Jani Nikula wrote:
> > > > > > On Tue, 24 Jun 2014, Thomas Meyer <thomas@m3y3r.de> wrote:
> > > > > > > the i915 driver is still broken in 3.16-rc2. Resume from ram crashes the
> > > > > > > X server.
> > > > > > 
> > > > > > This is not new to 3.16-rc2; apparently we've had it since v3.15-rc4
> > > > > > [1]. Also related [2].
> > > > > > 
> > > > > > Chris, any fresh ideas?
> > > > > 
> > > > > Nope. The bug is https://bugs.freedesktop.org/show_bug.cgi?id=76554
> > > > > everything we know and have tried is there. Which is not much more than
> > > > > at time of the original incarnation:
> > > > > 
> > > > > commit 50aa253d820ad4577e2231202f2c8fd89f9dc4e6
> > > > > Author: Keith Packard <keithp@keithp.com>
> > > > > Date:   Tue Oct 14 17:20:35 2008 -0700
> > > > > 
> > > > >     i915: Fix up ring initialization to cover G45 oddities
> > > > >     
> > > > >     G45 appears quite sensitive to ring initialization register writes,
> > > > >     sometimes leaving the HEAD register with the START register contents. Check
> > > > >     to make sure HEAD is reset correctly when START is written, and fix it up,
> > > > >     screaming loudly.
> > > > > -Chris
> > > > > 
> > > > 
> > > > Hi,
> > > > 
> > > > so why not revert 78f2975eec9faff353a6194e854d3d39907bab68 (drm/i915:
> > > > Move all ring resets before setting the HWS page) ?
> > > 
> > > Because that patch was in response to a boot time regression.
> > 
> > It seems we are in a fairly ugly "fix one board, it breaks another" iterations, right?
> > 
> > (BTW, if you apply patch to fix this bug, could you Cc me? I have strange feeling
> > that it will break my setup... Actually, it probably makes sense to Cc all the people
> > who reported problems with ring initialization...
> > 
> > What patch caused the boot time regression you are talking about? We need to get 
> > list of commits involved in this, and revert the original one...
> 
> commit 9991ae787a0c87fe7c783b4b6f4754c3cdbb6213
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Wed Apr 2 16:36:07 2014 +0100
> 
>     drm/i915: Move all ring resets before setting the HWS page
>     
>     In commit a51435a3137ad8ae75c288c39bd2d8b2696bae8f
>     Author: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>     Date:   Wed Mar 12 16:39:40 2014 +0530
>     
>         drm/i915: disable rings before HW status page setup
>     
>     we reordered stopping the rings to do so before we set the HWS register.
>     However, there is an extra workaround for g45 to reset the rings twice,
>     and for consistency we should apply that workaround before setting the
>     HWS to be sure that the rings are truly stopped.
>     
>     Cc: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>     Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>     Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 
> commit a51435a3137ad8ae75c288c39bd2d8b2696bae8f
> Author: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> Date:   Wed Mar 12 16:39:40 2014 +0530
> 
>     drm/i915: disable rings before HW status page setup
>     
>     Rings should be idle before issuing sync_flush
>     (in intel_ring_setup_status_page). This patch moves the ring
>     disabling before doing the HW status page setup.
>     
>     Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
>     Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 

Hi,

this patch on top of v3.16-rc3-62-gd92a333 makes the resume from ram
regression go away on my machine:

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 279488a..b896ac8 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -459,22 +459,25 @@ static bool stop_ring(struct intel_engine_cs *ring)
 {
 	struct drm_i915_private *dev_priv = to_i915(ring->dev);
 
-	if (!IS_GEN2(ring->dev)) {
-		I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(STOP_RING));
-		if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) {
-			DRM_ERROR("%s :timed out trying to stop ring\n", ring->name);
-			return false;
-		}
-	}
-
+//	if (!IS_GEN2(ring->dev)) {
+//		I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(STOP_RING));
+//		if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) {
+//			DRM_ERROR("%s :timed out trying to stop ring1\n", ring->name);
+//			return false;
+//		}
+//	}
+
+	/* Stop the ring if it's running. */
 	I915_WRITE_CTL(ring, 0);
 	I915_WRITE_HEAD(ring, 0);
 	ring->write_tail(ring, 0);
+	if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000))
+		DRM_ERROR("%s :timed out trying to stop ring2\n", ring->name);
 
-	if (!IS_GEN2(ring->dev)) {
-		(void)I915_READ_CTL(ring);
-		I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING));
-	}
+//	if (!IS_GEN2(ring->dev)) {
+//		(void)I915_READ_CTL(ring);
+//		I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING));
+//	}
 
 	return (I915_READ_HEAD(ring) & HEAD_ADDR) == 0;
 }

Chris, any ideas why explicitly stopping the ring before reset, results
in this kind of misbehaviour on my machine on resume from ram?

with kind regards
thomas

Re: Linux 3.16-rc2

[Daniel Vetter]

On Wed, Jul 02, 2014 at 06:18:41PM +0200, Thomas Meyer wrote:
> Am Montag, den 30.06.2014, 11:09 +0100 schrieb Chris Wilson:
> > On Mon, Jun 30, 2014 at 12:02:20PM +0200, Pavel Machek wrote:
> > > On Tue 2014-06-24 13:27:37, Chris Wilson wrote:
> > > > On Tue, Jun 24, 2014 at 02:24:30PM +0200, Thomas Meyer wrote:
> > > > > Am Dienstag, den 24.06.2014, 12:57 +0100 schrieb Chris Wilson:
> > > > > > On Tue, Jun 24, 2014 at 02:06:24PM +0300, Jani Nikula wrote:
> > > > > > > On Tue, 24 Jun 2014, Thomas Meyer <thomas@m3y3r.de> wrote:
> > > > > > > > the i915 driver is still broken in 3.16-rc2. Resume from ram crashes the
> > > > > > > > X server.
> > > > > > > 
> > > > > > > This is not new to 3.16-rc2; apparently we've had it since v3.15-rc4
> > > > > > > [1]. Also related [2].
> > > > > > > 
> > > > > > > Chris, any fresh ideas?
> > > > > > 
> > > > > > Nope. The bug is https://bugs.freedesktop.org/show_bug.cgi?id=76554
> > > > > > everything we know and have tried is there. Which is not much more than
> > > > > > at time of the original incarnation:
> > > > > > 
> > > > > > commit 50aa253d820ad4577e2231202f2c8fd89f9dc4e6
> > > > > > Author: Keith Packard <keithp@keithp.com>
> > > > > > Date:   Tue Oct 14 17:20:35 2008 -0700
> > > > > > 
> > > > > >     i915: Fix up ring initialization to cover G45 oddities
> > > > > >     
> > > > > >     G45 appears quite sensitive to ring initialization register writes,
> > > > > >     sometimes leaving the HEAD register with the START register contents. Check
> > > > > >     to make sure HEAD is reset correctly when START is written, and fix it up,
> > > > > >     screaming loudly.
> > > > > > -Chris
> > > > > > 
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > so why not revert 78f2975eec9faff353a6194e854d3d39907bab68 (drm/i915:
> > > > > Move all ring resets before setting the HWS page) ?
> > > > 
> > > > Because that patch was in response to a boot time regression.
> > > 
> > > It seems we are in a fairly ugly "fix one board, it breaks another" iterations, right?
> > > 
> > > (BTW, if you apply patch to fix this bug, could you Cc me? I have strange feeling
> > > that it will break my setup... Actually, it probably makes sense to Cc all the people
> > > who reported problems with ring initialization...
> > > 
> > > What patch caused the boot time regression you are talking about? We need to get 
> > > list of commits involved in this, and revert the original one...
> > 
> > commit 9991ae787a0c87fe7c783b4b6f4754c3cdbb6213
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Wed Apr 2 16:36:07 2014 +0100
> > 
> >     drm/i915: Move all ring resets before setting the HWS page
> >     
> >     In commit a51435a3137ad8ae75c288c39bd2d8b2696bae8f
> >     Author: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> >     Date:   Wed Mar 12 16:39:40 2014 +0530
> >     
> >         drm/i915: disable rings before HW status page setup
> >     
> >     we reordered stopping the rings to do so before we set the HWS register.
> >     However, there is an extra workaround for g45 to reset the rings twice,
> >     and for consistency we should apply that workaround before setting the
> >     HWS to be sure that the rings are truly stopped.
> >     
> >     Cc: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> >     Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >     Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > 
> > commit a51435a3137ad8ae75c288c39bd2d8b2696bae8f
> > Author: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> > Date:   Wed Mar 12 16:39:40 2014 +0530
> > 
> >     drm/i915: disable rings before HW status page setup
> >     
> >     Rings should be idle before issuing sync_flush
> >     (in intel_ring_setup_status_page). This patch moves the ring
> >     disabling before doing the HW status page setup.
> >     
> >     Signed-off-by: Naresh Kumar Kachhi <naresh.kumar.kachhi@intel.com>
> >     Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > 
> 
> Hi,
> 
> this patch on top of v3.16-rc3-62-gd92a333 makes the resume from ram
> regression go away on my machine:

Hm, we could conditionalize this hack on IS_G4X ... Chris, thoughts?
-Daniel

> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 279488a..b896ac8 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -459,22 +459,25 @@ static bool stop_ring(struct intel_engine_cs *ring)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(ring->dev);
>  
> -	if (!IS_GEN2(ring->dev)) {
> -		I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(STOP_RING));
> -		if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) {
> -			DRM_ERROR("%s :timed out trying to stop ring\n", ring->name);
> -			return false;
> -		}
> -	}
> -
> +//	if (!IS_GEN2(ring->dev)) {
> +//		I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(STOP_RING));
> +//		if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) {
> +//			DRM_ERROR("%s :timed out trying to stop ring1\n", ring->name);
> +//			return false;
> +//		}
> +//	}
> +
> +	/* Stop the ring if it's running. */
>  	I915_WRITE_CTL(ring, 0);
>  	I915_WRITE_HEAD(ring, 0);
>  	ring->write_tail(ring, 0);
> +	if (wait_for_atomic((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000))
> +		DRM_ERROR("%s :timed out trying to stop ring2\n", ring->name);
>  
> -	if (!IS_GEN2(ring->dev)) {
> -		(void)I915_READ_CTL(ring);
> -		I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING));
> -	}
> +//	if (!IS_GEN2(ring->dev)) {
> +//		(void)I915_READ_CTL(ring);
> +//		I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING));
> +//	}
>  
>  	return (I915_READ_HEAD(ring) & HEAD_ADDR) == 0;
>  }
> 
> Chris, any ideas why explicitly stopping the ring before reset, results
> in this kind of misbehaviour on my machine on resume from ram?
> 
> with kind regards
> thomas
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: Linux 3.16-rc2

[Jiri Kosina]

On Mon, 7 Jul 2014, Daniel Vetter wrote:

> > this patch on top of v3.16-rc3-62-gd92a333 makes the resume from ram
> > regression go away on my machine:
> 
> Hm, we could conditionalize this hack on IS_G4X ... Chris, thoughts?

For the record, commenting out the code in stop_ring() the way Thomas did 
doesn't fix my problem (the one reported in [1]).

[1] https://bugs.freedesktop.org/show_bug.cgi?id=76554

-- 
Jiri Kosina
SUSE Labs

Re: Linux 3.16-rc2

[Chris Wilson]

On Mon, Jul 07, 2014 at 05:16:13PM +0200, Daniel Vetter wrote:
> On Wed, Jul 02, 2014 at 06:18:41PM +0200, Thomas Meyer wrote:
> > Hi,
> > 
> > this patch on top of v3.16-rc3-62-gd92a333 makes the resume from ram
> > regression go away on my machine:
> 
> Hm, we could conditionalize this hack on IS_G4X ... Chris, thoughts?

As different machines favour different w/a, I think the issue is mostly
timing related. It could be sequence of register writes, but we tried
different orders early on. The next experiment I guess would be to
insert small delays between each write to see if that helps. Or to write
each register twice.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: Linux 3.16-rc2

[Jiri Kosina]

On Mon, 7 Jul 2014, Chris Wilson wrote:

> > > this patch on top of v3.16-rc3-62-gd92a333 makes the resume from ram
> > > regression go away on my machine:
> > 
> > Hm, we could conditionalize this hack on IS_G4X ... Chris, thoughts?
> 
> As different machines favour different w/a, I think the issue is mostly
> timing related. It could be sequence of register writes, but we tried
> different orders early on. The next experiment I guess would be to
> insert small delays between each write to see if that helps. Or to write
> each register twice.

I actually tried to introduce rather large delays between individual 
I915_WRITE() calls in the ring initialization sequence a couple weeks ago 
already, but it resulted in complete machine lockup (which is worse than 
my usual symptoms) during resume. Therefore I probably lack the knowledge 
of internal workings of the HW that would allow me to guess what the 
reasonable timeout value should be.

Willing to test any patches.

-- 
Jiri Kosina
SUSE Labs

Re: Linux 3.16-rc2

[Daniel Vetter]

On Tue, Jul 08, 2014 at 12:15:31AM +0200, Jiri Kosina wrote:
> On Mon, 7 Jul 2014, Chris Wilson wrote:
> 
> > > > this patch on top of v3.16-rc3-62-gd92a333 makes the resume from ram
> > > > regression go away on my machine:
> > > 
> > > Hm, we could conditionalize this hack on IS_G4X ... Chris, thoughts?
> > 
> > As different machines favour different w/a, I think the issue is mostly
> > timing related. It could be sequence of register writes, but we tried
> > different orders early on. The next experiment I guess would be to
> > insert small delays between each write to see if that helps. Or to write
> > each register twice.
> 
> I actually tried to introduce rather large delays between individual 
> I915_WRITE() calls in the ring initialization sequence a couple weeks ago 
> already, but it resulted in complete machine lockup (which is worse than 
> my usual symptoms) during resume. Therefore I probably lack the knowledge 
> of internal workings of the HW that would allow me to guess what the 
> reasonable timeout value should be.

Have you used msleep or udelay? The latter just spins the cpu and might be
less dangerous.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: Linux 3.16-rc2

[Chris Wilson]

On Tue, Jul 08, 2014 at 12:15:31AM +0200, Jiri Kosina wrote:
> On Mon, 7 Jul 2014, Chris Wilson wrote:
> 
> > > > this patch on top of v3.16-rc3-62-gd92a333 makes the resume from ram
> > > > regression go away on my machine:
> > > 
> > > Hm, we could conditionalize this hack on IS_G4X ... Chris, thoughts?
> > 
> > As different machines favour different w/a, I think the issue is mostly
> > timing related. It could be sequence of register writes, but we tried
> > different orders early on. The next experiment I guess would be to
> > insert small delays between each write to see if that helps. Or to write
> > each register twice.
> 
> I actually tried to introduce rather large delays between individual 
> I915_WRITE() calls in the ring initialization sequence a couple weeks ago 
> already, but it resulted in complete machine lockup (which is worse than 
> my usual symptoms) during resume. Therefore I probably lack the knowledge 
> of internal workings of the HW that would allow me to guess what the 
> reasonable timeout value should be.
> 
> Willing to test any patches.

Are you using the extra patches on bug 76554? If not, try

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index e18ed05dc0d5..48326f9628d4 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -526,6 +526,9 @@ static int init_ring_common(struct intel_engine_cs *ring)
                        ((ringbuf->size - PAGE_SIZE) & RING_NR_PAGES)
                        | RING_VALID);
 
+       I915_WRITE_HEAD(ring, 0);
+       (void)I915_READ_HEAD(ring);
+
        /* If the head is still not zero, the ring is dead */
        if (wait_for((I915_READ_CTL(ring) & RING_VALID) != 0 &&
                     I915_READ_START(ring) == i915_gem_obj_ggtt_offset(obj) &&

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: Linux 3.16-rc2

[Jiri Kosina]

On Tue, 8 Jul 2014, Chris Wilson wrote:

> > I actually tried to introduce rather large delays between individual 
> > I915_WRITE() calls in the ring initialization sequence a couple weeks ago 
> > already, but it resulted in complete machine lockup (which is worse than 
> > my usual symptoms) during resume. Therefore I probably lack the knowledge 
> > of internal workings of the HW that would allow me to guess what the 
> > reasonable timeout value should be.
> > 
> > Willing to test any patches.
> 
> Are you using the extra patches on bug 76554? If not, try

Just for debugging.

> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index e18ed05dc0d5..48326f9628d4 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -526,6 +526,9 @@ static int init_ring_common(struct intel_engine_cs *ring)
>                         ((ringbuf->size - PAGE_SIZE) & RING_NR_PAGES)
>                         | RING_VALID);
>  
> +       I915_WRITE_HEAD(ring, 0);
> +       (void)I915_READ_HEAD(ring);
> +
>         /* If the head is still not zero, the ring is dead */
>         if (wait_for((I915_READ_CTL(ring) & RING_VALID) != 0 &&
>                      I915_READ_START(ring) == i915_gem_obj_ggtt_offset(obj) &&

Tried on top of current Linus' tree, and no improvement. ring 
initialization failure upon resume, and window redrawing hosed.

-- 
Jiri Kosina
SUSE Labs

Re: Linux 3.16-rc2

[Chris Wilson]

On Tue, Jul 08, 2014 at 02:46:57PM +0200, Jiri Kosina wrote:
> On Tue, 8 Jul 2014, Chris Wilson wrote:
> 
> > > I actually tried to introduce rather large delays between individual 
> > > I915_WRITE() calls in the ring initialization sequence a couple weeks ago 
> > > already, but it resulted in complete machine lockup (which is worse than 
> > > my usual symptoms) during resume. Therefore I probably lack the knowledge 
> > > of internal workings of the HW that would allow me to guess what the 
> > > reasonable timeout value should be.
> > > 
> > > Willing to test any patches.
> > 
> > Are you using the extra patches on bug 76554? If not, try
> 
> Just for debugging.
> 
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index e18ed05dc0d5..48326f9628d4 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -526,6 +526,9 @@ static int init_ring_common(struct intel_engine_cs *ring)
> >                         ((ringbuf->size - PAGE_SIZE) & RING_NR_PAGES)
> >                         | RING_VALID);
> >  
> > +       I915_WRITE_HEAD(ring, 0);
> > +       (void)I915_READ_HEAD(ring);
> > +
> >         /* If the head is still not zero, the ring is dead */
> >         if (wait_for((I915_READ_CTL(ring) & RING_VALID) != 0 &&
> >                      I915_READ_START(ring) == i915_gem_obj_ggtt_offset(obj) &&
> 
> Tried on top of current Linus' tree, and no improvement. ring 
> initialization failure upon resume, and window redrawing hosed.

Which error during ring-init are you hitting?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: Linux 3.16-rc2

[Jiri Kosina]

On Tue, 8 Jul 2014, Chris Wilson wrote:

> > Tried on top of current Linus' tree, and no improvement. ring 
> > initialization failure upon resume, and window redrawing hosed.
> 
> Which error during ring-init are you hitting?

It's still the same I reported in 
bugs.freedesktop.org/show_bug.cgi?id=76554

[drm:init_ring_common] *ERROR* render ring initialization failed ctl 0001f001 (valid? 1) head 000f645c tail 00000000 start 000e4000 [expected 000e4000]
[drm:__i915_drm_thaw] *ERROR* failed to re-initialize GPU, declaring wedged!

-- 
Jiri Kosina
SUSE Labs