On Sat, Sep 24, 2011 at 03:23:05PM -0700, Ben Widawsky wrote:
> Under certain circumstances, an IOTLB flush never completes and the hardware
> just locks up. The fix is meant to idle the GPU both before and after
> any unmap occurs.
>
> This patch also disables the warning which is meant to detect mismatches
> between when we think the command parse and arbiter should be idle, and
> when it actually is.
>
> Cc: Dave Airlie <airl...@redhat.com>
> Cc: David Woodhouse <dw...@infradead.org>
> Cc: Keith Packard <kei...@keithp.com>
> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> Signed-off-by: Ben Widawsky <b...@bwidawsk.net>
A few nitpicks below ...
> ---
> drivers/char/agp/intel-gtt.c | 16 ++++++++++++++
> drivers/gpu/drm/i915/i915_gem.c | 32 +++++++++++++++++++---------
> drivers/gpu/drm/i915/i915_gem_gtt.c | 39
> +++++++++++++++++++++++++++++++++++
> include/drm/intel-gtt.h | 2 +
> 4 files changed, 79 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
> index 8515101..47ca42f 100644
> --- a/drivers/char/agp/intel-gtt.c
> +++ b/drivers/char/agp/intel-gtt.c
> @@ -923,6 +923,9 @@ static int intel_fake_agp_insert_entries(struct
> agp_memory *mem,
> {
> int ret = -EINVAL;
>
> + if (intel_private.base.do_idle_maps)
> + return -ENODEV;
This reminds me to kick out intel-agp support for ilk+. Upstream never
ever supported ums without gem on these, and that's the only reason we
have this agp compat code still lying around.
> +
> if (intel_private.clear_fake_agp) {
> int start = intel_private.base.stolen_size / PAGE_SIZE;
> int end = intel_private.base.gtt_mappable_entries;
> @@ -985,6 +988,9 @@ static int intel_fake_agp_remove_entries(struct
> agp_memory *mem,
> if (mem->page_count == 0)
> return 0;
>
> + if (intel_private.base.do_idle_maps)
> + return -ENODEV;
> +
> intel_gtt_clear_range(pg_start, mem->page_count);
>
> if (intel_private.base.needs_dmar) {
> @@ -1180,8 +1186,12 @@ static void gen6_cleanup(void)
> static int i9xx_setup(void)
> {
> u32 reg_addr;
> + u16 gmch_ctrl;
> + const unsigned short gpu_devid = intel_private.pcidev->device;
>
> pci_read_config_dword(intel_private.pcidev, I915_MMADDR, ®_addr);
> + pci_read_config_word(intel_private.bridge_dev,
> + I830_GMCH_CTRL, &gmch_ctrl);
>
> reg_addr &= 0xfff80000;
>
> @@ -1211,6 +1221,12 @@ static int i9xx_setup(void)
> intel_private.gtt_bus_addr = reg_addr + gtt_offset;
> }
>
> + if ((gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_HB ||
> + gpu_devid == PCI_DEVICE_ID_INTEL_IRONLAKE_M_IG) &&
> + (gmch_ctrl & G4x_GMCH_SIZE_VT_EN) &&
> + intel_private.base.needs_dmar)
> + intel_private.base.do_idle_maps = 1;
Can you extract that horrible condition into e.g.
NEEDS_ILK_IOMMU_WORKAROUND or something like that? I think future me will
go wtf on this without the context ...
> +
> intel_i9xx_setup_flush();
>
> return 0;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 61ce1b7..d369e48 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2268,27 +2268,39 @@ static int i915_ring_idle(struct intel_ring_buffer
> *ring)
> return i915_wait_request(ring, i915_gem_next_request_seqno(ring));
> }
>
> +static int
> +flush_rings(drm_i915_private_t *dev_priv)
> +{
> + int i, ret;
> +
> + /* Flush everything onto the inactive list. */
> + for (i = 0; i < I915_NUM_RINGS; i++) {
> + ret = i915_ring_idle(&dev_priv->ring[i]);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> int
> i915_gpu_idle(struct drm_device *dev)
> {
> drm_i915_private_t *dev_priv = dev->dev_private;
> bool lists_empty;
> - int ret, i;
>
> lists_empty = (list_empty(&dev_priv->mm.flushing_list) &&
> list_empty(&dev_priv->mm.active_list));
> if (lists_empty) {
> - WARN_ON(!(I915_READ(MI_MODE) & MI_RINGS_IDLE));
> - return 0;
> -
> - /* Flush everything onto the inactive list. */
> - for (i = 0; i < I915_NUM_RINGS; i++) {
> - ret = i915_ring_idle(&dev_priv->ring[i]);
> - if (ret)
> - return ret;
> + /* If we require idle maps, enforce the ring is idle */
> + bool ring_idle = (I915_READ(MI_MODE) & MI_RINGS_IDLE) != 0;
> + if (!ring_idle && dev_priv->mm.gtt->do_idle_maps)
> + return flush_rings(dev_priv);
> + else
> + return 0;
Can you extract this hack into its own patch? This way
- the actual work-around would stand out more clearly
- we put more emphasive on the fact that we seem to lose track of things.
> }
>
> - return 0;
> + return flush_rings(dev_priv);
> }
>
> static int sandybridge_write_fence_reg(struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7a709cd..0c6226b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -49,10 +49,39 @@ static unsigned int cache_level_to_agp_type(struct
> drm_device *dev,
> }
> }
>
> +static bool do_idling(struct drm_i915_private *dev_priv)
> +{
> + bool ret = dev_priv->mm.interruptible;
> +
> + if (dev_priv->mm.gtt->do_idle_maps) {
> + dev_priv->mm.interruptible = false;
> + if (i915_gpu_idle(dev_priv->dev))
> + DRM_ERROR("couldn't idle GPU %d\n", ret);
> + }
I don't like the uninterruptible sleep in core gem - this is only for
modesetting where backtracking is just not really feasible. I've quickly
checked the code and I haven't seen any problems in handing back the
-ERESTARTSYS to gem_object_unbind.
> +
> + return ret;
> +}
> +
> +static void undo_idling(struct drm_i915_private *dev_priv, bool idle)
> +{
> + int ret;
> + if (!dev_priv->mm.gtt->do_idle_maps)
> + return;
> +
> + /*NB: since GTT mappings don't actually touch the GPU, this is not
> + * strictly necessary.
> + */
> + ret = i915_gpu_idle(dev_priv->dev);
> + if (ret)
> + DRM_ERROR("couldn't idle GPU %d\n", ret);
> + dev_priv->mm.interruptible = idle;
> +}
> +
> void i915_gem_restore_gtt_mappings(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_i915_gem_object *obj;
> + bool idle;
Patch-dirt? 2 more below ...
>
> /* First fill our portion of the GTT with scratch pages */
> intel_gtt_clear_range(dev_priv->mm.gtt_start / PAGE_SIZE,
> @@ -71,6 +100,7 @@ int i915_gem_gtt_bind_object(struct drm_i915_gem_object
> *obj)
> struct drm_device *dev = obj->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> unsigned int agp_type = cache_level_to_agp_type(dev, obj->cache_level);
> + bool idle;
> int ret;
>
> if (dev_priv->mm.gtt->needs_dmar) {
> @@ -100,6 +130,7 @@ void i915_gem_gtt_rebind_object(struct
> drm_i915_gem_object *obj,
> struct drm_device *dev = obj->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> unsigned int agp_type = cache_level_to_agp_type(dev, cache_level);
> + bool idle;
>
> if (dev_priv->mm.gtt->needs_dmar) {
> BUG_ON(!obj->sg_list);
> @@ -117,6 +148,12 @@ void i915_gem_gtt_rebind_object(struct
> drm_i915_gem_object *obj,
>
> void i915_gem_gtt_unbind_object(struct drm_i915_gem_object *obj)
> {
> + struct drm_device *dev = obj->base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + bool idle;
> +
> + idle = do_idling(dev_priv);
> +
> intel_gtt_clear_range(obj->gtt_space->start >> PAGE_SHIFT,
> obj->base.size >> PAGE_SHIFT);
>
> @@ -124,4 +161,6 @@ void i915_gem_gtt_unbind_object(struct
> drm_i915_gem_object *obj)
> intel_gtt_unmap_memory(obj->sg_list, obj->num_sg);
> obj->sg_list = NULL;
> }
> +
> + undo_idling(dev_priv, idle);
> }
> diff --git a/include/drm/intel-gtt.h b/include/drm/intel-gtt.h
> index 9e343c0..b174620 100644
> --- a/include/drm/intel-gtt.h
> +++ b/include/drm/intel-gtt.h
> @@ -13,6 +13,8 @@ const struct intel_gtt {
> unsigned int gtt_mappable_entries;
> /* Whether i915 needs to use the dmar apis or not. */
> unsigned int needs_dmar : 1;
> + /* Whether we idle the gpu before mapping/unmapping */
> + unsigned int do_idle_maps : 1;
> } *intel_gtt_get(void);
>
> void intel_gtt_chipset_flush(void);
> --
> 1.7.6.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Mail: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
