> Subject: [PATCH 2/6] drm/i915: Preserve ring buffers objects across > resume > > Tearing down the ring buffers across resume is overkill, risks unnecessary > failure and increases fragmentation. > > After failure, since the device is still active we may end up trying to write into > the dangling iomapping and trigger an oops. > > v2: stop_ringbuffers() was meant to call stop(ring) not > cleanup(ring) during resume! > > Reported-by: Jae-hyeon Park <jhyeon@xxxxxxxxx> > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=72351 > References: https://bugs.freedesktop.org/show_bug.cgi?id=76554 > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_gem.c | 20 +++- > drivers/gpu/drm/i915/intel_ringbuffer.c | 168 +++++++++++++++++-------------- > - > drivers/gpu/drm/i915/intel_ringbuffer.h | 1 + > 3 files changed, 105 insertions(+), 84 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c > b/drivers/gpu/drm/i915/i915_gem.c index 6370a761d137..89e736c00ddc > 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2383,6 +2383,11 @@ static void i915_gem_reset_ring_cleanup(struct > drm_i915_private *dev_priv, > > i915_gem_free_request(request); > } > + > + /* These may not have been flush before the reset, do so now */ > + kfree(ring->preallocated_lazy_request); > + ring->preallocated_lazy_request = NULL; > + ring->outstanding_lazy_seqno = 0; > } > > void i915_gem_restore_fences(struct drm_device *dev) @@ -2423,8 +2428,6 > @@ void i915_gem_reset(struct drm_device *dev) > for_each_ring(ring, dev_priv, i) > i915_gem_reset_ring_cleanup(dev_priv, ring); > > - i915_gem_cleanup_ringbuffer(dev); > - > i915_gem_context_reset(dev); > > i915_gem_restore_fences(dev); > @@ -4232,6 +4235,17 @@ void i915_gem_vma_destroy(struct i915_vma > *vma) > kfree(vma); > } > > +static void > +i915_gem_stop_ringbuffers(struct drm_device *dev) { > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_ring_buffer *ring; > + int i; > + > + for_each_ring(ring, dev_priv, i) > + intel_stop_ring_buffer(ring); > +} > + > int > i915_gem_suspend(struct drm_device *dev) { @@ -4253,7 +4267,7 @@ > i915_gem_suspend(struct drm_device *dev) > i915_gem_evict_everything(dev); > > i915_kernel_lost_context(dev); > - i915_gem_cleanup_ringbuffer(dev); > + i915_gem_stop_ringbuffers(dev); > > /* Hack! Don't let anybody do execbuf while we don't control the chip. > * We need to replace this with a semaphore, or something. > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 9bed8764a62a..cce09f5a4426 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -1301,45 +1301,39 @@ static void cleanup_status_page(struct > intel_ring_buffer *ring) > > static int init_status_page(struct intel_ring_buffer *ring) { > - struct drm_device *dev = ring->dev; > struct drm_i915_gem_object *obj; > - int ret; > > - obj = i915_gem_alloc_object(dev, 4096); > - if (obj == NULL) { > - DRM_ERROR("Failed to allocate status page\n"); > - ret = -ENOMEM; > - goto err; > - } > + if ((obj = ring->status_page.obj) == NULL) { > + int ret; > > - ret = i915_gem_object_set_cache_level(obj, I915_CACHE_LLC); > - if (ret) > - goto err_unref; > + obj = i915_gem_alloc_object(ring->dev, 4096); > + if (obj == NULL) { > + DRM_ERROR("Failed to allocate status page\n"); > + return -ENOMEM; > + } > > - ret = i915_gem_obj_ggtt_pin(obj, 4096, 0); > - if (ret) > - goto err_unref; > + ret = i915_gem_object_set_cache_level(obj, > I915_CACHE_LLC); > + if (ret) > + goto err_unref; > + > + ret = i915_gem_obj_ggtt_pin(obj, 4096, 0); > + if (ret) { > +err_unref: > + drm_gem_object_unreference(&obj->base); > + return ret; > + } > + > + ring->status_page.obj = obj; > + } > > ring->status_page.gfx_addr = i915_gem_obj_ggtt_offset(obj); > ring->status_page.page_addr = kmap(sg_page(obj->pages->sgl)); > - if (ring->status_page.page_addr == NULL) { > - ret = -ENOMEM; > - goto err_unpin; > - } > - ring->status_page.obj = obj; > memset(ring->status_page.page_addr, 0, PAGE_SIZE); > > DRM_DEBUG_DRIVER("%s hws offset: 0x%08x\n", > ring->name, ring->status_page.gfx_addr); > > return 0; > - > -err_unpin: > - i915_gem_object_ggtt_unpin(obj); > -err_unref: > - drm_gem_object_unreference(&obj->base); > -err: > - return ret; > } > > static int init_phys_status_page(struct intel_ring_buffer *ring) @@ -1359,44 > +1353,23 @@ static int init_phys_status_page(struct intel_ring_buffer *ring) > return 0; > } > > -static int intel_init_ring_buffer(struct drm_device *dev, > - struct intel_ring_buffer *ring) > +static int allocate_ring_buffer(struct intel_ring_buffer *ring) > { > + struct drm_device *dev = ring->dev; > + struct drm_i915_private *dev_priv = to_i915(dev); > struct drm_i915_gem_object *obj; > - struct drm_i915_private *dev_priv = dev->dev_private; > int ret; > > - ring->dev = dev; > - INIT_LIST_HEAD(&ring->active_list); > - INIT_LIST_HEAD(&ring->request_list); > - ring->size = 32 * PAGE_SIZE; > - memset(ring->sync_seqno, 0, sizeof(ring->sync_seqno)); > - > - init_waitqueue_head(&ring->irq_queue); > - > - if (I915_NEED_GFX_HWS(dev)) { > - ret = init_status_page(ring); > - if (ret) > - return ret; > - } else { > - BUG_ON(ring->id != RCS); > - ret = init_phys_status_page(ring); > - if (ret) > - return ret; > - } > + if (ring->obj) > + return 0; > > obj = NULL; > if (!HAS_LLC(dev)) > obj = i915_gem_object_create_stolen(dev, ring->size); > if (obj == NULL) > obj = i915_gem_alloc_object(dev, ring->size); > - if (obj == NULL) { > - DRM_ERROR("Failed to allocate ringbuffer\n"); > - ret = -ENOMEM; > - goto err_hws; > - } > - > - ring->obj = obj; > + if (obj == NULL) > + return -ENOMEM; > > ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE); > if (ret) > @@ -1410,55 +1383,72 @@ static int intel_init_ring_buffer(struct drm_device > *dev, > ioremap_wc(dev_priv->gtt.mappable_base + > i915_gem_obj_ggtt_offset(obj), > ring->size); > if (ring->virtual_start == NULL) { > - DRM_ERROR("Failed to map ringbuffer.\n"); > ret = -EINVAL; > goto err_unpin; > } > > - ret = ring->init(ring); > - if (ret) > - goto err_unmap; > + ring->obj = obj; > + return 0; > + > +err_unpin: > + i915_gem_object_ggtt_unpin(obj); > +err_unref: > + drm_gem_object_unreference(&obj->base); > + return ret; > +} > + > +static int intel_init_ring_buffer(struct drm_device *dev, > + struct intel_ring_buffer *ring) > +{ > + int ret; > + > + ring->dev = dev; > + INIT_LIST_HEAD(&ring->active_list); > + INIT_LIST_HEAD(&ring->request_list); > + ring->size = 32 * PAGE_SIZE; > + memset(ring->sync_seqno, 0, sizeof(ring->sync_seqno)); > + > + init_waitqueue_head(&ring->irq_queue); > + > + if (I915_NEED_GFX_HWS(dev)) { > + ret = init_status_page(ring); > + if (ret) > + return ret; > + } else { > + BUG_ON(ring->id != RCS); > + ret = init_phys_status_page(ring); > + if (ret) > + return ret; > + } > + > + ret = allocate_ring_buffer(ring); > + if (ret) { > + DRM_ERROR("Failed to allocate ringbuffer %s: %d\n", ring- > >name, ret); > + return ret; > + } > > /* Workaround an erratum on the i830 which causes a hang if > * the TAIL pointer points to within the last 2 cachelines > * of the buffer. > */ > ring->effective_size = ring->size; > - if (IS_I830(ring->dev) || IS_845G(ring->dev)) > + if (IS_I830(dev) || IS_845G(dev)) > ring->effective_size -= 2 * CACHELINE_BYTES; > > i915_cmd_parser_init_ring(ring); > > - return 0; > - > -err_unmap: > - iounmap(ring->virtual_start); > -err_unpin: > - i915_gem_object_ggtt_unpin(obj); > -err_unref: > - drm_gem_object_unreference(&obj->base); > - ring->obj = NULL; > -err_hws: > - cleanup_status_page(ring); > - return ret; > + return ring->init(ring); > } > > void intel_cleanup_ring_buffer(struct intel_ring_buffer *ring) { > - struct drm_i915_private *dev_priv; > - int ret; > + struct drm_i915_private *dev_priv = to_i915(ring->dev); > > if (ring->obj == NULL) > return; > > - /* Disable the ring buffer. The ring must be idle at this point */ > - dev_priv = ring->dev->dev_private; > - ret = intel_ring_idle(ring); > - if (ret && !i915_reset_in_progress(&dev_priv->gpu_error)) > - DRM_ERROR("failed to quiesce %s whilst cleaning up: %d\n", > - ring->name, ret); > - > - I915_WRITE_CTL(ring, 0); > + intel_stop_ring_buffer(ring); > + WARN_ON((I915_READ_MODE(ring) & MODE_IDLE) == 0); > > iounmap(ring->virtual_start); > > @@ -2248,3 +2238,19 @@ intel_ring_invalidate_all_caches(struct > intel_ring_buffer *ring) > ring->gpu_caches_dirty = false; > return 0; > } > + > +void > +intel_stop_ring_buffer(struct intel_ring_buffer *ring) { > + int ret; > + > + if (ring->obj == NULL) > + return; It´s probably a good idea to use the intel_ring_initialized() macro. Other than that: Reviewed-by: Oscar Mateo <oscar.mateo@xxxxxxxxx> > + > + ret = intel_ring_idle(ring); > + if (ret && !i915_reset_in_progress(&to_i915(ring->dev)->gpu_error)) > + DRM_ERROR("failed to quiesce %s whilst cleaning up: %d\n", > + ring->name, ret); > + > + stop_ring(ring); > +} > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h > b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 413cdc74ed53..54839165eb6d 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -263,6 +263,7 @@ intel_write_status_page(struct intel_ring_buffer *ring, > #define I915_GEM_HWS_SCRATCH_INDEX 0x30 > #define I915_GEM_HWS_SCRATCH_ADDR (I915_GEM_HWS_SCRATCH_INDEX > << MI_STORE_DWORD_INDEX_SHIFT) > > +void intel_stop_ring_buffer(struct intel_ring_buffer *ring); > void intel_cleanup_ring_buffer(struct intel_ring_buffer *ring); > > int __must_check intel_ring_begin(struct intel_ring_buffer *ring, int n); > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
