Date:	Mon, 7 Jul 2014 17:16:13 +0200
From:	Daniel Vetter <daniel@...ll.ch>
To:	Thomas Meyer <thomas@...3r.de>
Cc:	Chris Wilson <chris@...is-wilson.co.uk>, jikos@...e.cz,
	"intel-gfx@...ts.freedesktop.org" <intel-gfx@...ts.freedesktop.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Pavel Machek <pavel@....cz>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [Intel-gfx] Linux 3.16-rc2

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@...3r.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@...thp.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@...is-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@...el.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@...el.com>
> >     Signed-off-by: Chris Wilson <chris@...is-wilson.co.uk>
> >     Reviewed-by: Jesse Barnes <jbarnes@...tuousgeek.org>
> >     Signed-off-by: Daniel Vetter <daniel.vetter@...ll.ch>
> > 
> > 
> > commit a51435a3137ad8ae75c288c39bd2d8b2696bae8f
> > Author: Naresh Kumar Kachhi <naresh.kumar.kachhi@...el.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@...el.com>
> >     Reviewed-by: Chris Wilson <chris@...is-wilson.co.uk>
> >     Signed-off-by: Daniel Vetter <daniel.vetter@...ll.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@...ts.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists