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
