On Wed, Apr 02, 2014 at 02:58:54PM -0700, Jesse Barnes wrote: > On Wed, 2 Apr 2014 16:36:07 +0100 > Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> wrote: > > > In commit a51435a3137ad8ae75c288c39bd2d8b2696bae8f > > Author: Naresh Kumar Kachhi <naresh.kumar.kachhi@xxxxxxxxx> > > 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@xxxxxxxxx> > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > drivers/gpu/drm/i915/intel_ringbuffer.c | 54 +++++++++++++++++++++------------ > > drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + > > 3 files changed, 36 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 393f93ecd41a..6dd35f849781 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -915,6 +915,7 @@ enum punit_power_well { > > # define MI_FLUSH_ENABLE (1 << 12) > > # define ASYNC_FLIP_PERF_DISABLE (1 << 14) > > # define MODE_IDLE (1 << 9) > > +# define STOP_RING (1 << 8) > > > > #define GEN6_GT_MODE 0x20d0 > > #define GEN7_GT_MODE 0x7008 > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > > index 475391ce671a..98ba7d594718 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > @@ -439,32 +439,41 @@ static void ring_setup_phys_status_page(struct intel_ring_buffer *ring) > > I915_WRITE(HWS_PGA, addr); > > } > > > > -static int init_ring_common(struct intel_ring_buffer *ring) > > +static bool stop_ring(struct intel_ring_buffer *ring) > > { > > - struct drm_device *dev = ring->dev; > > - struct drm_i915_private *dev_priv = dev->dev_private; > > - struct drm_i915_gem_object *obj = ring->obj; > > - int ret = 0; > > - u32 head; > > + struct drm_i915_private *dev_priv = to_i915(ring->dev); > > > > - gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); > > + 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; > > + } > > + } > > > > - /* 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 ring\n", ring->name); > > > > - if (I915_NEED_GFX_HWS(dev)) > > - intel_ring_setup_status_page(ring); > > - else > > - ring_setup_phys_status_page(ring); > > + if (!IS_GEN2(ring->dev)) { > > + (void)I915_READ_CTL(ring); > > + I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING)); > > + } > > > > - head = I915_READ_HEAD(ring) & HEAD_ADDR; > > + return (I915_READ_HEAD(ring) & HEAD_ADDR) == 0; > > +} > > > > - /* G45 ring initialization fails to reset head to zero */ > > - if (head != 0) { > > +static int init_ring_common(struct intel_ring_buffer *ring) > > +{ > > + struct drm_device *dev = ring->dev; > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + struct drm_i915_gem_object *obj = ring->obj; > > + int ret = 0; > > + > > + gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL); > > + > > + if (!stop_ring(ring)) { > > + /* G45 ring initialization often fails to reset head to zero */ > > DRM_DEBUG_KMS("%s head not reset to zero " > > "ctl %08x head %08x tail %08x start %08x\n", > > ring->name, > > @@ -473,9 +482,7 @@ static int init_ring_common(struct intel_ring_buffer *ring) > > I915_READ_TAIL(ring), > > I915_READ_START(ring)); > > > > - I915_WRITE_HEAD(ring, 0); > > - > > - if (I915_READ_HEAD(ring) & HEAD_ADDR) { > > + if (!stop_ring(ring)) { > > DRM_ERROR("failed to set %s head to zero " > > "ctl %08x head %08x tail %08x start %08x\n", > > ring->name, > > @@ -483,9 +490,16 @@ static int init_ring_common(struct intel_ring_buffer *ring) > > I915_READ_HEAD(ring), > > I915_READ_TAIL(ring), > > I915_READ_START(ring)); > > + ret = -EIO; > > + goto out; > > } > > } > > > > + if (I915_NEED_GFX_HWS(dev)) > > + intel_ring_setup_status_page(ring); > > + else > > + ring_setup_phys_status_page(ring); > > + > > /* 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 > > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > > index 270a6a973438..2b91c4b4d34b 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > > @@ -34,6 +34,7 @@ struct intel_hw_status_page { > > #define I915_WRITE_IMR(ring, val) I915_WRITE(RING_IMR((ring)->mmio_base), val) > > > > #define I915_READ_MODE(ring) I915_READ(RING_MI_MODE((ring)->mmio_base)) > > +#define I915_WRITE_MODE(ring, val) I915_WRITE(RING_MI_MODE((ring)->mmio_base), val) > > > > enum intel_ring_hangcheck_action { > > HANGCHECK_IDLE = 0, > > Bad Chris, mixing a nice refactor and a nice fix in the same patch. > I'll still give you a cookie though. > > Reviewed-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> Queued for -next, thanks for the patch. I'll let the other settle a bit more. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx