Hi Mika, can you please review this patch, and verify it fixes the issues in your previous review. Thanks, Alistair. > -----Original Message----- > From: Mcaulay, Alistair > Sent: Friday, August 15, 2014 6:52 PM > To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx > Cc: Mcaulay, Alistair > Subject: [PATCH v3] drm/i915: Rework GPU reset sequence to match driver > load & thaw > > From: "McAulay, Alistair" <alistair.mcaulay@xxxxxxxxx> > > This patch is to address Daniels concerns over different code during reset: > > http://lists.freedesktop.org/archives/intel-gfx/2014-June/047758.html > > "The reason for aiming as hard as possible to use the exact same code for > driver load, gpu reset and runtime pm/system resume is that we've simply > seen too many bugs due to slight variations and unintended omissions." > > Tested using igt drv_hangman. > > V2: Cleaner way of preventing check_wedge returning -EAGAIN > V3: Clean the last_context during reset, to ensure do_switch() does the > MI_SET_CONTEXT. As per review. > Signed-off-by: McAulay, Alistair <alistair.mcaulay@xxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_drv.c | 6 +++ > drivers/gpu/drm/i915/i915_drv.h | 3 ++ > drivers/gpu/drm/i915/i915_gem.c | 4 +- > drivers/gpu/drm/i915/i915_gem_context.c | 33 +++------------- > drivers/gpu/drm/i915/i915_gem_gtt.c | 67 +++++---------------------------- > drivers/gpu/drm/i915/i915_gem_gtt.h | 3 +- > 6 files changed, 28 insertions(+), 88 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > b/drivers/gpu/drm/i915/i915_drv.c index 5e4fefd..3bfafe6 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -806,7 +806,13 @@ int i915_reset(struct drm_device *dev) > !dev_priv->ums.mm_suspended) { > dev_priv->ums.mm_suspended = 0; > > + /* Used to prevent gem_check_wedged returning -EAGAIN > during gpu reset */ > + dev_priv->gpu_error.reload_in_reset = true; > + > ret = i915_gem_init_hw(dev); > + > + dev_priv->gpu_error.reload_in_reset = false; > + > mutex_unlock(&dev->struct_mutex); > if (ret) { > DRM_ERROR("Failed hw init on reset %d\n", ret); diff > --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 991b663..116daff 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1217,6 +1217,9 @@ struct i915_gpu_error { > > /* For missed irq/seqno simulation. */ > unsigned int test_irq_rings; > + > + /* Used to prevent gem_check_wedged returning -EAGAIN during > gpu reset */ > + bool reload_in_reset; > }; > > enum modeset_restore { > diff --git a/drivers/gpu/drm/i915/i915_gem.c > b/drivers/gpu/drm/i915/i915_gem.c index ef047bc..e7396eb 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1085,7 +1085,9 @@ i915_gem_check_wedge(struct i915_gpu_error > *error, > if (i915_terminally_wedged(error)) > return -EIO; > > - return -EAGAIN; > + /* Check if GPU Reset is in progress */ > + if (!error->reload_in_reset) > + return -EAGAIN; > } > > return 0; > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > b/drivers/gpu/drm/i915/i915_gem_context.c > index de72a28..9378ad8 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -377,34 +377,17 @@ void i915_gem_context_reset(struct drm_device > *dev) > struct drm_i915_private *dev_priv = dev->dev_private; > int i; > > - /* Prevent the hardware from restoring the last context (which > hung) on > - * the next switch */ > for (i = 0; i < I915_NUM_RINGS; i++) { > struct intel_engine_cs *ring = &dev_priv->ring[i]; > - struct intel_context *dctx = ring->default_context; > struct intel_context *lctx = ring->last_context; > > - /* Do a fake switch to the default context */ > - if (lctx == dctx) > - continue; > - > - if (!lctx) > - continue; > + if (lctx) { > + if (lctx->legacy_hw_ctx.rcs_state && i == RCS) > + i915_gem_object_ggtt_unpin(lctx- > >legacy_hw_ctx.rcs_state); > > - if (dctx->legacy_hw_ctx.rcs_state && i == RCS) { > - WARN_ON(i915_gem_obj_ggtt_pin(dctx- > >legacy_hw_ctx.rcs_state, > - > get_context_alignment(dev), 0)); > - /* Fake a finish/inactive */ > - dctx->legacy_hw_ctx.rcs_state->base.write_domain > = 0; > - dctx->legacy_hw_ctx.rcs_state->active = 0; > + i915_gem_context_unreference(lctx); > + ring->last_context = NULL; > } > - > - if (lctx->legacy_hw_ctx.rcs_state && i == RCS) > - i915_gem_object_ggtt_unpin(lctx- > >legacy_hw_ctx.rcs_state); > - > - i915_gem_context_unreference(lctx); > - i915_gem_context_reference(dctx); > - ring->last_context = dctx; > } > } > > @@ -498,10 +481,6 @@ int i915_gem_context_enable(struct > drm_i915_private *dev_priv) > ppgtt->enable(ppgtt); > } > > - /* FIXME: We should make this work, even in reset */ > - if (i915_reset_in_progress(&dev_priv->gpu_error)) > - return 0; > - > BUG_ON(!dev_priv->ring[RCS].default_context); > > for_each_ring(ring, dev_priv, i) { > @@ -645,7 +624,7 @@ static int do_switch(struct intel_engine_cs *ring, > from = ring->last_context; > > if (USES_FULL_PPGTT(ring->dev)) { > - ret = ppgtt->switch_mm(ppgtt, ring, false); > + ret = ppgtt->switch_mm(ppgtt, ring); > if (ret) > goto unpin_out; > } > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > b/drivers/gpu/drm/i915/i915_gem_gtt.c > index 5188936..450c8a9 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -216,19 +216,12 @@ static gen6_gtt_pte_t > iris_pte_encode(dma_addr_t addr, > > /* Broadwell Page Directory Pointer Descriptors */ static int > gen8_write_pdp(struct intel_engine_cs *ring, unsigned entry, > - uint64_t val, bool synchronous) > + uint64_t val) > { > - struct drm_i915_private *dev_priv = ring->dev->dev_private; > int ret; > > BUG_ON(entry >= 4); > > - if (synchronous) { > - I915_WRITE(GEN8_RING_PDP_UDW(ring, entry), val >> 32); > - I915_WRITE(GEN8_RING_PDP_LDW(ring, entry), (u32)val); > - return 0; > - } > - > ret = intel_ring_begin(ring, 6); > if (ret) > return ret; > @@ -245,8 +238,7 @@ static int gen8_write_pdp(struct intel_engine_cs > *ring, unsigned entry, } > > static int gen8_mm_switch(struct i915_hw_ppgtt *ppgtt, > - struct intel_engine_cs *ring, > - bool synchronous) > + struct intel_engine_cs *ring) > { > int i, ret; > > @@ -255,7 +247,7 @@ static int gen8_mm_switch(struct i915_hw_ppgtt > *ppgtt, > > for (i = used_pd - 1; i >= 0; i--) { > dma_addr_t addr = ppgtt->pd_dma_addr[i]; > - ret = gen8_write_pdp(ring, i, addr, synchronous); > + ret = gen8_write_pdp(ring, i, addr); > if (ret) > return ret; > } > @@ -724,29 +716,10 @@ static uint32_t get_pd_offset(struct i915_hw_ppgtt > *ppgtt) } > > static int hsw_mm_switch(struct i915_hw_ppgtt *ppgtt, > - struct intel_engine_cs *ring, > - bool synchronous) > + struct intel_engine_cs *ring) > { > - struct drm_device *dev = ppgtt->base.dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > > - /* If we're in reset, we can assume the GPU is sufficiently idle to > - * manually frob these bits. Ideally we could use the ring functions, > - * except our error handling makes it quite difficult (can't use > - * intel_ring_begin, ring->flush, or intel_ring_advance) > - * > - * FIXME: We should try not to special case reset > - */ > - if (synchronous || > - i915_reset_in_progress(&dev_priv->gpu_error)) { > - WARN_ON(ppgtt != dev_priv->mm.aliasing_ppgtt); > - I915_WRITE(RING_PP_DIR_DCLV(ring), PP_DIR_DCLV_2G); > - I915_WRITE(RING_PP_DIR_BASE(ring), > get_pd_offset(ppgtt)); > - POSTING_READ(RING_PP_DIR_BASE(ring)); > - return 0; > - } > - > /* NB: TLBs must be flushed and invalidated before a switch */ > ret = ring->flush(ring, I915_GEM_GPU_DOMAINS, > I915_GEM_GPU_DOMAINS); > if (ret) > @@ -768,29 +741,10 @@ static int hsw_mm_switch(struct i915_hw_ppgtt > *ppgtt, } > > static int gen7_mm_switch(struct i915_hw_ppgtt *ppgtt, > - struct intel_engine_cs *ring, > - bool synchronous) > + struct intel_engine_cs *ring) > { > - struct drm_device *dev = ppgtt->base.dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > > - /* If we're in reset, we can assume the GPU is sufficiently idle to > - * manually frob these bits. Ideally we could use the ring functions, > - * except our error handling makes it quite difficult (can't use > - * intel_ring_begin, ring->flush, or intel_ring_advance) > - * > - * FIXME: We should try not to special case reset > - */ > - if (synchronous || > - i915_reset_in_progress(&dev_priv->gpu_error)) { > - WARN_ON(ppgtt != dev_priv->mm.aliasing_ppgtt); > - I915_WRITE(RING_PP_DIR_DCLV(ring), PP_DIR_DCLV_2G); > - I915_WRITE(RING_PP_DIR_BASE(ring), > get_pd_offset(ppgtt)); > - POSTING_READ(RING_PP_DIR_BASE(ring)); > - return 0; > - } > - > /* NB: TLBs must be flushed and invalidated before a switch */ > ret = ring->flush(ring, I915_GEM_GPU_DOMAINS, > I915_GEM_GPU_DOMAINS); > if (ret) > @@ -819,14 +773,11 @@ static int gen7_mm_switch(struct i915_hw_ppgtt > *ppgtt, } > > static int gen6_mm_switch(struct i915_hw_ppgtt *ppgtt, > - struct intel_engine_cs *ring, > - bool synchronous) > + struct intel_engine_cs *ring) > { > struct drm_device *dev = ppgtt->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > > - if (!synchronous) > - return 0; > > I915_WRITE(RING_PP_DIR_DCLV(ring), PP_DIR_DCLV_2G); > I915_WRITE(RING_PP_DIR_BASE(ring), get_pd_offset(ppgtt)); @@ - > 852,7 +803,7 @@ static int gen8_ppgtt_enable(struct i915_hw_ppgtt *ppgtt) > if (USES_FULL_PPGTT(dev)) > continue; > > - ret = ppgtt->switch_mm(ppgtt, ring, true); > + ret = ppgtt->switch_mm(ppgtt, ring); > if (ret) > goto err_out; > } > @@ -897,7 +848,7 @@ static int gen7_ppgtt_enable(struct i915_hw_ppgtt > *ppgtt) > if (USES_FULL_PPGTT(dev)) > continue; > > - ret = ppgtt->switch_mm(ppgtt, ring, true); > + ret = ppgtt->switch_mm(ppgtt, ring); > if (ret) > return ret; > } > @@ -926,7 +877,7 @@ static int gen6_ppgtt_enable(struct i915_hw_ppgtt > *ppgtt) > I915_WRITE(GFX_MODE, > _MASKED_BIT_ENABLE(GFX_PPGTT_ENABLE)); > > for_each_ring(ring, dev_priv, i) { > - int ret = ppgtt->switch_mm(ppgtt, ring, true); > + int ret = ppgtt->switch_mm(ppgtt, ring); > if (ret) > return ret; > } > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h > b/drivers/gpu/drm/i915/i915_gem_gtt.h > index 8d6f7c1..bf1e4fc 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > @@ -262,8 +262,7 @@ struct i915_hw_ppgtt { > > int (*enable)(struct i915_hw_ppgtt *ppgtt); > int (*switch_mm)(struct i915_hw_ppgtt *ppgtt, > - struct intel_engine_cs *ring, > - bool synchronous); > + struct intel_engine_cs *ring); > void (*debug_dump)(struct i915_hw_ppgtt *ppgtt, struct seq_file > *m); }; > > -- > 2.0.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
