On Wed, 2 Apr 2014 16:36:07 +0100
Chris Wilson <chris at chris-wilson.co.uk> wrote:
> In commit a51435a3137ad8ae75c288c39bd2d8b2696bae8f
> Author: Naresh Kumar Kachhi <naresh.kumar.kachhi at 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 at intel.com>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> 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 at virtuousgeek.org>
--
Jesse Barnes, Intel Open Source Technology Center