* [PATCH] drm/i915: Double check ring is idle before declaring the GPU wedged @ 2014-08-11 8:21 Chris Wilson 2014-08-11 9:30 ` Damien Lespiau 0 siblings, 1 reply; 6+ messages in thread From: Chris Wilson @ 2014-08-11 8:21 UTC (permalink / raw) To: intel-gfx During ring initialisation, sometimes we observe, though not in production hardware, that the idle flag is not set even though the ring is empty. Double check before giving up. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Damien Lespiau <damien.lespiau@intel.com> --- drivers/gpu/drm/i915/intel_ringbuffer.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index a0831c309eab..d72d5e0e693d 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -467,7 +467,12 @@ static bool stop_ring(struct intel_engine_cs *ring) I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(STOP_RING)); if (wait_for((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) { DRM_ERROR("%s : timed out trying to stop ring\n", ring->name); - return false; + /* Sometimes we observe that the idle flag is not + * set even though the ring is empty. So double + * check before giving up. + */ + if (I915_READ_HEAD(ring) != I915_READ_TAIL(ring)) + return false; } } -- 2.1.0.rc1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Double check ring is idle before declaring the GPU wedged 2014-08-11 8:21 [PATCH] drm/i915: Double check ring is idle before declaring the GPU wedged Chris Wilson @ 2014-08-11 9:30 ` Damien Lespiau 2014-08-11 9:35 ` Chris Wilson 0 siblings, 1 reply; 6+ messages in thread From: Damien Lespiau @ 2014-08-11 9:30 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx On Mon, Aug 11, 2014 at 09:21:35AM +0100, Chris Wilson wrote: > During ring initialisation, sometimes we observe, though not in > production hardware, that the idle flag is not set even though the ring > is empty. Double check before giving up. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Damien Lespiau <damien.lespiau@intel.com> > --- > drivers/gpu/drm/i915/intel_ringbuffer.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index a0831c309eab..d72d5e0e693d 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -467,7 +467,12 @@ static bool stop_ring(struct intel_engine_cs *ring) > I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(STOP_RING)); > if (wait_for((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) { > DRM_ERROR("%s : timed out trying to stop ring\n", ring->name); > - return false; > + /* Sometimes we observe that the idle flag is not > + * set even though the ring is empty. So double > + * check before giving up. > + */ > + if (I915_READ_HEAD(ring) != I915_READ_TAIL(ring)) > + return false; That means we propably want to just put the user visible error message there as well? -- Damien ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Double check ring is idle before declaring the GPU wedged 2014-08-11 9:30 ` Damien Lespiau @ 2014-08-11 9:35 ` Chris Wilson 2014-08-11 10:07 ` Damien Lespiau 0 siblings, 1 reply; 6+ messages in thread From: Chris Wilson @ 2014-08-11 9:35 UTC (permalink / raw) To: Damien Lespiau; +Cc: intel-gfx On Mon, Aug 11, 2014 at 10:30:09AM +0100, Damien Lespiau wrote: > On Mon, Aug 11, 2014 at 09:21:35AM +0100, Chris Wilson wrote: > > During ring initialisation, sometimes we observe, though not in > > production hardware, that the idle flag is not set even though the ring > > is empty. Double check before giving up. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Damien Lespiau <damien.lespiau@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ringbuffer.c | 7 ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > > index a0831c309eab..d72d5e0e693d 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > @@ -467,7 +467,12 @@ static bool stop_ring(struct intel_engine_cs *ring) > > I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(STOP_RING)); > > if (wait_for((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) { > > DRM_ERROR("%s : timed out trying to stop ring\n", ring->name); > > - return false; > > + /* Sometimes we observe that the idle flag is not > > + * set even though the ring is empty. So double > > + * check before giving up. > > + */ > > + if (I915_READ_HEAD(ring) != I915_READ_TAIL(ring)) > > + return false; > > That means we propably want to just put the user visible error message > there as well? It is still a 1 second timeout, so having a warning there that something is wrong is important I thought. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Double check ring is idle before declaring the GPU wedged 2014-08-11 9:35 ` Chris Wilson @ 2014-08-11 10:07 ` Damien Lespiau 2014-08-11 10:14 ` Chris Wilson 2014-08-11 11:34 ` Daniel Vetter 0 siblings, 2 replies; 6+ messages in thread From: Damien Lespiau @ 2014-08-11 10:07 UTC (permalink / raw) To: Chris Wilson, intel-gfx On Mon, Aug 11, 2014 at 10:35:25AM +0100, Chris Wilson wrote: > On Mon, Aug 11, 2014 at 10:30:09AM +0100, Damien Lespiau wrote: > > On Mon, Aug 11, 2014 at 09:21:35AM +0100, Chris Wilson wrote: > > > During ring initialisation, sometimes we observe, though not in > > > production hardware, that the idle flag is not set even though the ring > > > is empty. Double check before giving up. > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Damien Lespiau <damien.lespiau@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_ringbuffer.c | 7 ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > > > index a0831c309eab..d72d5e0e693d 100644 > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > > @@ -467,7 +467,12 @@ static bool stop_ring(struct intel_engine_cs *ring) > > > I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(STOP_RING)); > > > if (wait_for((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) { > > > DRM_ERROR("%s : timed out trying to stop ring\n", ring->name); > > > - return false; > > > + /* Sometimes we observe that the idle flag is not > > > + * set even though the ring is empty. So double > > > + * check before giving up. > > > + */ > > > + if (I915_READ_HEAD(ring) != I915_READ_TAIL(ring)) > > > + return false; > > > > That means we propably want to just put the user visible error message > > there as well? > > It is still a 1 second timeout, so having a warning there that something > is wrong is important I thought. Ah, I missed the "not in production hw" bits of the commit message, It may mean simulation and then the guess is that flag is not implemented. If that makes us not totally give up, I guess that's something. Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> -- Damien ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Double check ring is idle before declaring the GPU wedged 2014-08-11 10:07 ` Damien Lespiau @ 2014-08-11 10:14 ` Chris Wilson 2014-08-11 11:34 ` Daniel Vetter 1 sibling, 0 replies; 6+ messages in thread From: Chris Wilson @ 2014-08-11 10:14 UTC (permalink / raw) To: Damien Lespiau; +Cc: intel-gfx On Mon, Aug 11, 2014 at 11:07:10AM +0100, Damien Lespiau wrote: > On Mon, Aug 11, 2014 at 10:35:25AM +0100, Chris Wilson wrote: > > On Mon, Aug 11, 2014 at 10:30:09AM +0100, Damien Lespiau wrote: > > > On Mon, Aug 11, 2014 at 09:21:35AM +0100, Chris Wilson wrote: > > > > During ring initialisation, sometimes we observe, though not in > > > > production hardware, that the idle flag is not set even though the ring > > > > is empty. Double check before giving up. > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > Cc: Damien Lespiau <damien.lespiau@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_ringbuffer.c | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > > > > index a0831c309eab..d72d5e0e693d 100644 > > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > > > @@ -467,7 +467,12 @@ static bool stop_ring(struct intel_engine_cs *ring) > > > > I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(STOP_RING)); > > > > if (wait_for((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) { > > > > DRM_ERROR("%s : timed out trying to stop ring\n", ring->name); > > > > - return false; > > > > + /* Sometimes we observe that the idle flag is not > > > > + * set even though the ring is empty. So double > > > > + * check before giving up. > > > > + */ > > > > + if (I915_READ_HEAD(ring) != I915_READ_TAIL(ring)) > > > > + return false; > > > > > > That means we propably want to just put the user visible error message > > > there as well? > > > > It is still a 1 second timeout, so having a warning there that something > > is wrong is important I thought. > > Ah, I missed the "not in production hw" bits of the commit message, It > may mean simulation and then the guess is that flag is not implemented. > If that makes us not totally give up, I guess that's something. I was trying to avoid saying that it was only to work around sim bugs :) Otherwise, it looks to be a useful piece of paranoia and logging against hw trying to screw us over. -Chris -- Chris Wilson, Intel Open Source Technology Centre ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: Double check ring is idle before declaring the GPU wedged 2014-08-11 10:07 ` Damien Lespiau 2014-08-11 10:14 ` Chris Wilson @ 2014-08-11 11:34 ` Daniel Vetter 1 sibling, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2014-08-11 11:34 UTC (permalink / raw) To: Damien Lespiau; +Cc: intel-gfx On Mon, Aug 11, 2014 at 11:07:10AM +0100, Damien Lespiau wrote: > On Mon, Aug 11, 2014 at 10:35:25AM +0100, Chris Wilson wrote: > > On Mon, Aug 11, 2014 at 10:30:09AM +0100, Damien Lespiau wrote: > > > On Mon, Aug 11, 2014 at 09:21:35AM +0100, Chris Wilson wrote: > > > > During ring initialisation, sometimes we observe, though not in > > > > production hardware, that the idle flag is not set even though the ring > > > > is empty. Double check before giving up. > > > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > > Cc: Damien Lespiau <damien.lespiau@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_ringbuffer.c | 7 ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > > > > index a0831c309eab..d72d5e0e693d 100644 > > > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > > > @@ -467,7 +467,12 @@ static bool stop_ring(struct intel_engine_cs *ring) > > > > I915_WRITE_MODE(ring, _MASKED_BIT_ENABLE(STOP_RING)); > > > > if (wait_for((I915_READ_MODE(ring) & MODE_IDLE) != 0, 1000)) { > > > > DRM_ERROR("%s : timed out trying to stop ring\n", ring->name); > > > > - return false; > > > > + /* Sometimes we observe that the idle flag is not > > > > + * set even though the ring is empty. So double > > > > + * check before giving up. > > > > + */ > > > > + if (I915_READ_HEAD(ring) != I915_READ_TAIL(ring)) > > > > + return false; > > > > > > That means we propably want to just put the user visible error message > > > there as well? > > > > It is still a 1 second timeout, so having a warning there that something > > is wrong is important I thought. > > Ah, I missed the "not in production hw" bits of the commit message, It > may mean simulation and then the guess is that flag is not implemented. > If that makes us not totally give up, I guess that's something. > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> Queued for -next, thanks for the patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-11 11:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-08-11 8:21 [PATCH] drm/i915: Double check ring is idle before declaring the GPU wedged Chris Wilson 2014-08-11 9:30 ` Damien Lespiau 2014-08-11 9:35 ` Chris Wilson 2014-08-11 10:07 ` Damien Lespiau 2014-08-11 10:14 ` Chris Wilson 2014-08-11 11:34 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.