| Message ID | 1415874536-459-1-git-send-email-thomas.daniel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 18, 2014 at 12:09:54PM +0530, Deepak S wrote: > On Tuesday 18 November 2014 12:07 PM, Deepak S wrote: > >With pin specific mutex from previous patch set removed > > Oops This comment was for previous patch in the series :( Since i > reviewed the patch offline, comments got mixed :) Please forward these comments from the private discussion to the mailing list. Review isn't just about code correctness, but about communication - yes, I (and domain experts) actually read all this stuff that floats around and will jump into the discussion if there's something important or tricky being discussed. Second reason for public review is that the important part about the r-b tag isn't that review happened, but by whom. So this is all about reputation building and playing to people's various strenght. And if you do review in private nothing of that can happen, which makes the review a lot less useful. So let's extract the most value from all that engineering time we invest into reviewing and _always_ do the review in public. Thanks, Daniel
Reviewed the patch & it looks fine. Reviewed-by: "Akash Goel <akash.goels@gmail.com>" On Thu, Nov 13, 2014 at 3:58 PM, Thomas Daniel <thomas.daniel@intel.com> wrote: > Same as with the context, pinning to GGTT regardless is harmful (it > badly fragments the GGTT and can even exhaust it). > > Unfortunately, this case is also more complex than the previous one > because we need to map and access the ringbuffer in several places > along the execbuffer path (and we cannot make do by leaving the > default ringbuffer pinned, as before). Also, the context object > itself contains a pointer to the ringbuffer address that we have to > keep updated if we are going to allow the ringbuffer to move around. > > v2: Same as with the context pinning, we cannot really do it during > an interrupt. Also, pin the default ringbuffers objects regardless > (makes error capture a lot easier). > > v3: Rebased. Take a pin reference of the ringbuffer for each item > in the execlist request queue because the hardware may still be using > the ringbuffer after the MI_USER_INTERRUPT to notify the seqno update > is executed. The ringbuffer must remain pinned until the context save > is complete. No longer pin and unpin ringbuffer in > populate_lr_context() - this transient address is meaningless and the > pinning can cause a sleep while atomic. > > v4: Moved ringbuffer pin and unpin into the lr_context_pin functions. > Downgraded pinning check BUG_ONs to WARN_ONs. > > v5: Reinstated WARN_ONs for unexpected execlist states. Removed unused > variable. > > Issue: VIZ-4277 > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > Signed-off-by: Thomas Daniel <thomas.daniel@intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 102 > +++++++++++++++++++++++-------- > drivers/gpu/drm/i915/intel_ringbuffer.c | 85 +++++++++++++++----------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 3 + > 3 files changed, 128 insertions(+), 62 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c > b/drivers/gpu/drm/i915/intel_lrc.c > index f7fa0f7..ca20f91 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -202,6 +202,9 @@ enum { > }; > #define GEN8_CTX_ID_SHIFT 32 > > +static int intel_lr_context_pin(struct intel_engine_cs *ring, > + struct intel_context *ctx); > + > /** > * intel_sanitize_enable_execlists() - sanitize i915.enable_execlists > * @dev: DRM device. > @@ -339,7 +342,9 @@ static void execlists_elsp_write(struct > intel_engine_cs *ring, > spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); > } > > -static int execlists_ctx_write_tail(struct drm_i915_gem_object *ctx_obj, > u32 tail) > +static int execlists_update_context(struct drm_i915_gem_object *ctx_obj, > + struct drm_i915_gem_object *ring_obj, > + u32 tail) > { > struct page *page; > uint32_t *reg_state; > @@ -348,6 +353,7 @@ static int execlists_ctx_write_tail(struct > drm_i915_gem_object *ctx_obj, u32 tai > reg_state = kmap_atomic(page); > > reg_state[CTX_RING_TAIL+1] = tail; > + reg_state[CTX_RING_BUFFER_START+1] = > i915_gem_obj_ggtt_offset(ring_obj); > > kunmap_atomic(reg_state); > > @@ -358,21 +364,25 @@ static int execlists_submit_context(struct > intel_engine_cs *ring, > struct intel_context *to0, u32 tail0, > struct intel_context *to1, u32 tail1) > { > - struct drm_i915_gem_object *ctx_obj0; > + struct drm_i915_gem_object *ctx_obj0 = to0->engine[ring->id].state; > + struct intel_ringbuffer *ringbuf0 = to0->engine[ring->id].ringbuf; > struct drm_i915_gem_object *ctx_obj1 = NULL; > + struct intel_ringbuffer *ringbuf1 = NULL; > > - ctx_obj0 = to0->engine[ring->id].state; > BUG_ON(!ctx_obj0); > WARN_ON(!i915_gem_obj_is_pinned(ctx_obj0)); > + WARN_ON(!i915_gem_obj_is_pinned(ringbuf0->obj)); > > - execlists_ctx_write_tail(ctx_obj0, tail0); > + execlists_update_context(ctx_obj0, ringbuf0->obj, tail0); > > if (to1) { > + ringbuf1 = to1->engine[ring->id].ringbuf; > ctx_obj1 = to1->engine[ring->id].state; > BUG_ON(!ctx_obj1); > WARN_ON(!i915_gem_obj_is_pinned(ctx_obj1)); > + WARN_ON(!i915_gem_obj_is_pinned(ringbuf1->obj)); > > - execlists_ctx_write_tail(ctx_obj1, tail1); > + execlists_update_context(ctx_obj1, ringbuf1->obj, tail1); > } > > execlists_elsp_write(ring, ctx_obj0, ctx_obj1); > @@ -524,6 +534,10 @@ static int execlists_context_queue(struct > intel_engine_cs *ring, > return -ENOMEM; > req->ctx = to; > i915_gem_context_reference(req->ctx); > + > + if (to != ring->default_context) > + intel_lr_context_pin(ring, to); > + > req->ring = ring; > req->tail = tail; > > @@ -544,7 +558,7 @@ static int execlists_context_queue(struct > intel_engine_cs *ring, > > if (to == tail_req->ctx) { > WARN(tail_req->elsp_submitted != 0, > - "More than 2 already-submitted reqs > queued\n"); > + "More than 2 already-submitted reqs > queued\n"); > list_del(&tail_req->execlist_link); > list_add_tail(&tail_req->execlist_link, > &ring->execlist_retired_req_list); > @@ -732,6 +746,12 @@ void intel_execlists_retire_requests(struct > intel_engine_cs *ring) > spin_unlock_irqrestore(&ring->execlist_lock, flags); > > list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) { > + struct intel_context *ctx = req->ctx; > + struct drm_i915_gem_object *ctx_obj = > + ctx->engine[ring->id].state; > + > + if (ctx_obj && (ctx != ring->default_context)) > + intel_lr_context_unpin(ring, ctx); > intel_runtime_pm_put(dev_priv); > i915_gem_context_unreference(req->ctx); > list_del(&req->execlist_link); > @@ -803,6 +823,7 @@ static int intel_lr_context_pin(struct intel_engine_cs > *ring, > struct intel_context *ctx) > { > struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state; > + struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf; > int ret = 0; > > WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex)); > @@ -810,21 +831,35 @@ static int intel_lr_context_pin(struct > intel_engine_cs *ring, > ret = i915_gem_obj_ggtt_pin(ctx_obj, > GEN8_LR_CONTEXT_ALIGN, 0); > if (ret) > - ctx->engine[ring->id].unpin_count = 0; > + goto reset_unpin_count; > + > + ret = intel_pin_and_map_ringbuffer_obj(ring->dev, ringbuf); > + if (ret) > + goto unpin_ctx_obj; > } > > return ret; > + > +unpin_ctx_obj: > + i915_gem_object_ggtt_unpin(ctx_obj); > +reset_unpin_count: > + ctx->engine[ring->id].unpin_count = 0; > + > + return ret; > } > > void intel_lr_context_unpin(struct intel_engine_cs *ring, > struct intel_context *ctx) > { > struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state; > + struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf; > > if (ctx_obj) { > WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex)); > - if (--ctx->engine[ring->id].unpin_count == 0) > + if (--ctx->engine[ring->id].unpin_count == 0) { > + intel_unpin_ringbuffer_obj(ringbuf); > i915_gem_object_ggtt_unpin(ctx_obj); > + } > } > } > > @@ -1541,7 +1576,6 @@ populate_lr_context(struct intel_context *ctx, > struct drm_i915_gem_object *ctx_o > { > struct drm_device *dev = ring->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - struct drm_i915_gem_object *ring_obj = ringbuf->obj; > struct i915_hw_ppgtt *ppgtt = ctx->ppgtt; > struct page *page; > uint32_t *reg_state; > @@ -1587,7 +1621,9 @@ populate_lr_context(struct intel_context *ctx, > struct drm_i915_gem_object *ctx_o > reg_state[CTX_RING_TAIL] = RING_TAIL(ring->mmio_base); > reg_state[CTX_RING_TAIL+1] = 0; > reg_state[CTX_RING_BUFFER_START] = RING_START(ring->mmio_base); > - reg_state[CTX_RING_BUFFER_START+1] = > i915_gem_obj_ggtt_offset(ring_obj); > + /* Ring buffer start address is not known until the buffer is > pinned. > + * It is written to the context image in execlists_update_context() > + */ > reg_state[CTX_RING_BUFFER_CONTROL] = RING_CTL(ring->mmio_base); > reg_state[CTX_RING_BUFFER_CONTROL+1] = > ((ringbuf->size - PAGE_SIZE) & RING_NR_PAGES) | > RING_VALID; > @@ -1669,10 +1705,12 @@ void intel_lr_context_free(struct intel_context > *ctx) > ctx->engine[i].ringbuf; > struct intel_engine_cs *ring = ringbuf->ring; > > + if (ctx == ring->default_context) { > + intel_unpin_ringbuffer_obj(ringbuf); > + i915_gem_object_ggtt_unpin(ctx_obj); > + } > intel_destroy_ringbuffer_obj(ringbuf); > kfree(ringbuf); > - if (ctx == ring->default_context) > - i915_gem_object_ggtt_unpin(ctx_obj); > drm_gem_object_unreference(&ctx_obj->base); > } > } > @@ -1770,11 +1808,8 @@ int intel_lr_context_deferred_create(struct > intel_context *ctx, > if (!ringbuf) { > DRM_DEBUG_DRIVER("Failed to allocate ringbuffer %s\n", > ring->name); > - if (is_global_default_ctx) > - i915_gem_object_ggtt_unpin(ctx_obj); > - drm_gem_object_unreference(&ctx_obj->base); > ret = -ENOMEM; > - return ret; > + goto error_unpin_ctx; > } > > ringbuf->ring = ring; > @@ -1787,22 +1822,30 @@ int intel_lr_context_deferred_create(struct > intel_context *ctx, > ringbuf->space = ringbuf->size; > ringbuf->last_retired_head = -1; > > - /* TODO: For now we put this in the mappable region so that we can > reuse > - * the existing ringbuffer code which ioremaps it. When we start > - * creating many contexts, this will no longer work and we must > switch > - * to a kmapish interface. > - */ > - ret = intel_alloc_ringbuffer_obj(dev, ringbuf); > - if (ret) { > - DRM_DEBUG_DRIVER("Failed to allocate ringbuffer obj %s: > %d\n", > + if (ringbuf->obj == NULL) { > + ret = intel_alloc_ringbuffer_obj(dev, ringbuf); > + if (ret) { > + DRM_DEBUG_DRIVER( > + "Failed to allocate ringbuffer obj %s: > %d\n", > ring->name, ret); > - goto error; > + goto error_free_rbuf; > + } > + > + if (is_global_default_ctx) { > + ret = intel_pin_and_map_ringbuffer_obj(dev, > ringbuf); > + if (ret) { > + DRM_ERROR( > + "Failed to pin and map ringbuffer > %s: %d\n", > + ring->name, ret); > + goto error_destroy_rbuf; > + } > + } > + > } > > ret = populate_lr_context(ctx, ctx_obj, ring, ringbuf); > if (ret) { > DRM_DEBUG_DRIVER("Failed to populate LRC: %d\n", ret); > - intel_destroy_ringbuffer_obj(ringbuf); > goto error; > } > > @@ -1823,7 +1866,6 @@ int intel_lr_context_deferred_create(struct > intel_context *ctx, > DRM_ERROR("Init render state failed: %d\n", ret); > ctx->engine[ring->id].ringbuf = NULL; > ctx->engine[ring->id].state = NULL; > - intel_destroy_ringbuffer_obj(ringbuf); > goto error; > } > ctx->rcs_initialized = true; > @@ -1832,7 +1874,13 @@ int intel_lr_context_deferred_create(struct > intel_context *ctx, > return 0; > > error: > + if (is_global_default_ctx) > + intel_unpin_ringbuffer_obj(ringbuf); > +error_destroy_rbuf: > + intel_destroy_ringbuffer_obj(ringbuf); > +error_free_rbuf: > kfree(ringbuf); > +error_unpin_ctx: > if (is_global_default_ctx) > i915_gem_object_ggtt_unpin(ctx_obj); > drm_gem_object_unreference(&ctx_obj->base); > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > b/drivers/gpu/drm/i915/intel_ringbuffer.c > index a8f72e8..0c4aab1 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -1721,13 +1721,42 @@ static int init_phys_status_page(struct > intel_engine_cs *ring) > return 0; > } > > -void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf) > +void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf) > { > - if (!ringbuf->obj) > - return; > - > iounmap(ringbuf->virtual_start); > + ringbuf->virtual_start = NULL; > i915_gem_object_ggtt_unpin(ringbuf->obj); > +} > + > +int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, > + struct intel_ringbuffer *ringbuf) > +{ > + struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_gem_object *obj = ringbuf->obj; > + int ret; > + > + ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE); > + if (ret) > + return ret; > + > + ret = i915_gem_object_set_to_gtt_domain(obj, true); > + if (ret) { > + i915_gem_object_ggtt_unpin(obj); > + return ret; > + } > + > + ringbuf->virtual_start = ioremap_wc(dev_priv->gtt.mappable_base + > + i915_gem_obj_ggtt_offset(obj), ringbuf->size); > + if (ringbuf->virtual_start == NULL) { > + i915_gem_object_ggtt_unpin(obj); > + return -EINVAL; > + } > + > + return 0; > +} > + > +void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf) > +{ > drm_gem_object_unreference(&ringbuf->obj->base); > ringbuf->obj = NULL; > } > @@ -1735,12 +1764,7 @@ void intel_destroy_ringbuffer_obj(struct > intel_ringbuffer *ringbuf) > int intel_alloc_ringbuffer_obj(struct drm_device *dev, > struct intel_ringbuffer *ringbuf) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > struct drm_i915_gem_object *obj; > - int ret; > - > - if (ringbuf->obj) > - return 0; > > obj = NULL; > if (!HAS_LLC(dev)) > @@ -1753,30 +1777,9 @@ int intel_alloc_ringbuffer_obj(struct drm_device > *dev, > /* mark ring buffers as read-only from GPU side by default */ > obj->gt_ro = 1; > > - ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE); > - if (ret) > - goto err_unref; > - > - ret = i915_gem_object_set_to_gtt_domain(obj, true); > - if (ret) > - goto err_unpin; > - > - ringbuf->virtual_start = > - ioremap_wc(dev_priv->gtt.mappable_base + > i915_gem_obj_ggtt_offset(obj), > - ringbuf->size); > - if (ringbuf->virtual_start == NULL) { > - ret = -EINVAL; > - goto err_unpin; > - } > - > ringbuf->obj = obj; > - return 0; > > -err_unpin: > - i915_gem_object_ggtt_unpin(obj); > -err_unref: > - drm_gem_object_unreference(&obj->base); > - return ret; > + return 0; > } > > static int intel_init_ring_buffer(struct drm_device *dev, > @@ -1813,10 +1816,21 @@ static int intel_init_ring_buffer(struct > drm_device *dev, > goto error; > } > > - ret = intel_alloc_ringbuffer_obj(dev, ringbuf); > - if (ret) { > - DRM_ERROR("Failed to allocate ringbuffer %s: %d\n", > ring->name, ret); > - goto error; > + if (ringbuf->obj == NULL) { > + ret = intel_alloc_ringbuffer_obj(dev, ringbuf); > + if (ret) { > + DRM_ERROR("Failed to allocate ringbuffer %s: %d\n", > + ring->name, ret); > + goto error; > + } > + > + ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf); > + if (ret) { > + DRM_ERROR("Failed to pin and map ringbuffer %s: > %d\n", > + ring->name, ret); > + intel_destroy_ringbuffer_obj(ringbuf); > + goto error; > + } > } > > /* Workaround an erratum on the i830 which causes a hang if > @@ -1854,6 +1868,7 @@ void intel_cleanup_ring_buffer(struct > intel_engine_cs *ring) > intel_stop_ring_buffer(ring); > WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) > == 0); > > + intel_unpin_ringbuffer_obj(ringbuf); > intel_destroy_ringbuffer_obj(ringbuf); > ring->preallocated_lazy_request = NULL; > ring->outstanding_lazy_seqno = 0; > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h > b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 8c002d2..365854ad 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -382,6 +382,9 @@ intel_write_status_page(struct intel_engine_cs *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_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf); > +int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, > + struct intel_ringbuffer *ringbuf); > void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf); > int intel_alloc_ringbuffer_obj(struct drm_device *dev, > struct intel_ringbuffer *ringbuf); > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Thursday 13 November 2014 03:58 PM, Thomas Daniel wrote: > Same as with the context, pinning to GGTT regardless is harmful (it > badly fragments the GGTT and can even exhaust it). > > Unfortunately, this case is also more complex than the previous one > because we need to map and access the ringbuffer in several places > along the execbuffer path (and we cannot make do by leaving the > default ringbuffer pinned, as before). Also, the context object > itself contains a pointer to the ringbuffer address that we have to > keep updated if we are going to allow the ringbuffer to move around. > > v2: Same as with the context pinning, we cannot really do it during > an interrupt. Also, pin the default ringbuffers objects regardless > (makes error capture a lot easier). > > v3: Rebased. Take a pin reference of the ringbuffer for each item > in the execlist request queue because the hardware may still be using > the ringbuffer after the MI_USER_INTERRUPT to notify the seqno update > is executed. The ringbuffer must remain pinned until the context save > is complete. No longer pin and unpin ringbuffer in > populate_lr_context() - this transient address is meaningless and the > pinning can cause a sleep while atomic. > > v4: Moved ringbuffer pin and unpin into the lr_context_pin functions. > Downgraded pinning check BUG_ONs to WARN_ONs. > > v5: Reinstated WARN_ONs for unexpected execlist states. Removed unused > variable. > > Issue: VIZ-4277 > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > Signed-off-by: Thomas Daniel <thomas.daniel@intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 102 +++++++++++++++++++++++-------- > drivers/gpu/drm/i915/intel_ringbuffer.c | 85 +++++++++++++++----------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 3 + > 3 files changed, 128 insertions(+), 62 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index f7fa0f7..ca20f91 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -202,6 +202,9 @@ enum { > }; > #define GEN8_CTX_ID_SHIFT 32 > > +static int intel_lr_context_pin(struct intel_engine_cs *ring, > + struct intel_context *ctx); > + > /** > * intel_sanitize_enable_execlists() - sanitize i915.enable_execlists > * @dev: DRM device. > @@ -339,7 +342,9 @@ static void execlists_elsp_write(struct intel_engine_cs *ring, > spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); > } > > -static int execlists_ctx_write_tail(struct drm_i915_gem_object *ctx_obj, u32 tail) > +static int execlists_update_context(struct drm_i915_gem_object *ctx_obj, > + struct drm_i915_gem_object *ring_obj, > + u32 tail) > { > struct page *page; > uint32_t *reg_state; > @@ -348,6 +353,7 @@ static int execlists_ctx_write_tail(struct drm_i915_gem_object *ctx_obj, u32 tai > reg_state = kmap_atomic(page); > > reg_state[CTX_RING_TAIL+1] = tail; > + reg_state[CTX_RING_BUFFER_START+1] = i915_gem_obj_ggtt_offset(ring_obj); > > kunmap_atomic(reg_state); > > @@ -358,21 +364,25 @@ static int execlists_submit_context(struct intel_engine_cs *ring, > struct intel_context *to0, u32 tail0, > struct intel_context *to1, u32 tail1) > { > - struct drm_i915_gem_object *ctx_obj0; > + struct drm_i915_gem_object *ctx_obj0 = to0->engine[ring->id].state; > + struct intel_ringbuffer *ringbuf0 = to0->engine[ring->id].ringbuf; > struct drm_i915_gem_object *ctx_obj1 = NULL; > + struct intel_ringbuffer *ringbuf1 = NULL; > > - ctx_obj0 = to0->engine[ring->id].state; > BUG_ON(!ctx_obj0); > WARN_ON(!i915_gem_obj_is_pinned(ctx_obj0)); > + WARN_ON(!i915_gem_obj_is_pinned(ringbuf0->obj)); > > - execlists_ctx_write_tail(ctx_obj0, tail0); > + execlists_update_context(ctx_obj0, ringbuf0->obj, tail0); > > if (to1) { > + ringbuf1 = to1->engine[ring->id].ringbuf; > ctx_obj1 = to1->engine[ring->id].state; > BUG_ON(!ctx_obj1); > WARN_ON(!i915_gem_obj_is_pinned(ctx_obj1)); > + WARN_ON(!i915_gem_obj_is_pinned(ringbuf1->obj)); > > - execlists_ctx_write_tail(ctx_obj1, tail1); > + execlists_update_context(ctx_obj1, ringbuf1->obj, tail1); > } > > execlists_elsp_write(ring, ctx_obj0, ctx_obj1); > @@ -524,6 +534,10 @@ static int execlists_context_queue(struct intel_engine_cs *ring, > return -ENOMEM; > req->ctx = to; > i915_gem_context_reference(req->ctx); > + > + if (to != ring->default_context) > + intel_lr_context_pin(ring, to); > + > req->ring = ring; > req->tail = tail; > > @@ -544,7 +558,7 @@ static int execlists_context_queue(struct intel_engine_cs *ring, > > if (to == tail_req->ctx) { > WARN(tail_req->elsp_submitted != 0, > - "More than 2 already-submitted reqs queued\n"); > + "More than 2 already-submitted reqs queued\n"); > list_del(&tail_req->execlist_link); > list_add_tail(&tail_req->execlist_link, > &ring->execlist_retired_req_list); > @@ -732,6 +746,12 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring) > spin_unlock_irqrestore(&ring->execlist_lock, flags); > > list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) { > + struct intel_context *ctx = req->ctx; > + struct drm_i915_gem_object *ctx_obj = > + ctx->engine[ring->id].state; > + > + if (ctx_obj && (ctx != ring->default_context)) > + intel_lr_context_unpin(ring, ctx); > intel_runtime_pm_put(dev_priv); > i915_gem_context_unreference(req->ctx); > list_del(&req->execlist_link); > @@ -803,6 +823,7 @@ static int intel_lr_context_pin(struct intel_engine_cs *ring, > struct intel_context *ctx) > { > struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state; > + struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf; > int ret = 0; > > WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex)); > @@ -810,21 +831,35 @@ static int intel_lr_context_pin(struct intel_engine_cs *ring, > ret = i915_gem_obj_ggtt_pin(ctx_obj, > GEN8_LR_CONTEXT_ALIGN, 0); > if (ret) > - ctx->engine[ring->id].unpin_count = 0; > + goto reset_unpin_count; > + > + ret = intel_pin_and_map_ringbuffer_obj(ring->dev, ringbuf); > + if (ret) > + goto unpin_ctx_obj; > } > > return ret; > + > +unpin_ctx_obj: > + i915_gem_object_ggtt_unpin(ctx_obj); > +reset_unpin_count: > + ctx->engine[ring->id].unpin_count = 0; > + > + return ret; > } > > void intel_lr_context_unpin(struct intel_engine_cs *ring, > struct intel_context *ctx) > { > struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state; > + struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf; > > if (ctx_obj) { > WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex)); With pin specific mutex from previous patch set removed Reviewed-by: Deepak S<deepak.s@linux.intel.com> > - if (--ctx->engine[ring->id].unpin_count == 0) > + if (--ctx->engine[ring->id].unpin_count == 0) { > + intel_unpin_ringbuffer_obj(ringbuf); > i915_gem_object_ggtt_unpin(ctx_obj); > + } > } > } > > @@ -1541,7 +1576,6 @@ populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_o > { > struct drm_device *dev = ring->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - struct drm_i915_gem_object *ring_obj = ringbuf->obj; > struct i915_hw_ppgtt *ppgtt = ctx->ppgtt; > struct page *page; > uint32_t *reg_state; > @@ -1587,7 +1621,9 @@ populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_o > reg_state[CTX_RING_TAIL] = RING_TAIL(ring->mmio_base); > reg_state[CTX_RING_TAIL+1] = 0; > reg_state[CTX_RING_BUFFER_START] = RING_START(ring->mmio_base); > - reg_state[CTX_RING_BUFFER_START+1] = i915_gem_obj_ggtt_offset(ring_obj); > + /* Ring buffer start address is not known until the buffer is pinned. > + * It is written to the context image in execlists_update_context() > + */ > reg_state[CTX_RING_BUFFER_CONTROL] = RING_CTL(ring->mmio_base); > reg_state[CTX_RING_BUFFER_CONTROL+1] = > ((ringbuf->size - PAGE_SIZE) & RING_NR_PAGES) | RING_VALID; > @@ -1669,10 +1705,12 @@ void intel_lr_context_free(struct intel_context *ctx) > ctx->engine[i].ringbuf; > struct intel_engine_cs *ring = ringbuf->ring; > > + if (ctx == ring->default_context) { > + intel_unpin_ringbuffer_obj(ringbuf); > + i915_gem_object_ggtt_unpin(ctx_obj); > + } > intel_destroy_ringbuffer_obj(ringbuf); > kfree(ringbuf); > - if (ctx == ring->default_context) > - i915_gem_object_ggtt_unpin(ctx_obj); > drm_gem_object_unreference(&ctx_obj->base); > } > } > @@ -1770,11 +1808,8 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, > if (!ringbuf) { > DRM_DEBUG_DRIVER("Failed to allocate ringbuffer %s\n", > ring->name); > - if (is_global_default_ctx) > - i915_gem_object_ggtt_unpin(ctx_obj); > - drm_gem_object_unreference(&ctx_obj->base); > ret = -ENOMEM; > - return ret; > + goto error_unpin_ctx; > } > > ringbuf->ring = ring; > @@ -1787,22 +1822,30 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, > ringbuf->space = ringbuf->size; > ringbuf->last_retired_head = -1; > > - /* TODO: For now we put this in the mappable region so that we can reuse > - * the existing ringbuffer code which ioremaps it. When we start > - * creating many contexts, this will no longer work and we must switch > - * to a kmapish interface. > - */ > - ret = intel_alloc_ringbuffer_obj(dev, ringbuf); > - if (ret) { > - DRM_DEBUG_DRIVER("Failed to allocate ringbuffer obj %s: %d\n", > + if (ringbuf->obj == NULL) { > + ret = intel_alloc_ringbuffer_obj(dev, ringbuf); > + if (ret) { > + DRM_DEBUG_DRIVER( > + "Failed to allocate ringbuffer obj %s: %d\n", > ring->name, ret); > - goto error; > + goto error_free_rbuf; > + } > + > + if (is_global_default_ctx) { > + ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf); > + if (ret) { > + DRM_ERROR( > + "Failed to pin and map ringbuffer %s: %d\n", > + ring->name, ret); > + goto error_destroy_rbuf; > + } > + } > + > } > > ret = populate_lr_context(ctx, ctx_obj, ring, ringbuf); > if (ret) { > DRM_DEBUG_DRIVER("Failed to populate LRC: %d\n", ret); > - intel_destroy_ringbuffer_obj(ringbuf); > goto error; > } > > @@ -1823,7 +1866,6 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, > DRM_ERROR("Init render state failed: %d\n", ret); > ctx->engine[ring->id].ringbuf = NULL; > ctx->engine[ring->id].state = NULL; > - intel_destroy_ringbuffer_obj(ringbuf); > goto error; > } > ctx->rcs_initialized = true; > @@ -1832,7 +1874,13 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, > return 0; > > error: > + if (is_global_default_ctx) > + intel_unpin_ringbuffer_obj(ringbuf); > +error_destroy_rbuf: > + intel_destroy_ringbuffer_obj(ringbuf); > +error_free_rbuf: > kfree(ringbuf); > +error_unpin_ctx: > if (is_global_default_ctx) > i915_gem_object_ggtt_unpin(ctx_obj); > drm_gem_object_unreference(&ctx_obj->base); > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index a8f72e8..0c4aab1 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -1721,13 +1721,42 @@ static int init_phys_status_page(struct intel_engine_cs *ring) > return 0; > } > > -void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf) > +void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf) > { > - if (!ringbuf->obj) > - return; > - > iounmap(ringbuf->virtual_start); > + ringbuf->virtual_start = NULL; > i915_gem_object_ggtt_unpin(ringbuf->obj); > +} > + > +int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, > + struct intel_ringbuffer *ringbuf) > +{ > + struct drm_i915_private *dev_priv = to_i915(dev); > + struct drm_i915_gem_object *obj = ringbuf->obj; > + int ret; > + > + ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE); > + if (ret) > + return ret; > + > + ret = i915_gem_object_set_to_gtt_domain(obj, true); > + if (ret) { > + i915_gem_object_ggtt_unpin(obj); > + return ret; > + } > + > + ringbuf->virtual_start = ioremap_wc(dev_priv->gtt.mappable_base + > + i915_gem_obj_ggtt_offset(obj), ringbuf->size); > + if (ringbuf->virtual_start == NULL) { > + i915_gem_object_ggtt_unpin(obj); > + return -EINVAL; > + } > + > + return 0; > +} > + > +void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf) > +{ > drm_gem_object_unreference(&ringbuf->obj->base); > ringbuf->obj = NULL; > } > @@ -1735,12 +1764,7 @@ void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf) > int intel_alloc_ringbuffer_obj(struct drm_device *dev, > struct intel_ringbuffer *ringbuf) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > struct drm_i915_gem_object *obj; > - int ret; > - > - if (ringbuf->obj) > - return 0; > > obj = NULL; > if (!HAS_LLC(dev)) > @@ -1753,30 +1777,9 @@ int intel_alloc_ringbuffer_obj(struct drm_device *dev, > /* mark ring buffers as read-only from GPU side by default */ > obj->gt_ro = 1; > > - ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE); > - if (ret) > - goto err_unref; > - > - ret = i915_gem_object_set_to_gtt_domain(obj, true); > - if (ret) > - goto err_unpin; > - > - ringbuf->virtual_start = > - ioremap_wc(dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj), > - ringbuf->size); > - if (ringbuf->virtual_start == NULL) { > - ret = -EINVAL; > - goto err_unpin; > - } > - > ringbuf->obj = obj; > - return 0; > > -err_unpin: > - i915_gem_object_ggtt_unpin(obj); > -err_unref: > - drm_gem_object_unreference(&obj->base); > - return ret; > + return 0; > } > > static int intel_init_ring_buffer(struct drm_device *dev, > @@ -1813,10 +1816,21 @@ static int intel_init_ring_buffer(struct drm_device *dev, > goto error; > } > > - ret = intel_alloc_ringbuffer_obj(dev, ringbuf); > - if (ret) { > - DRM_ERROR("Failed to allocate ringbuffer %s: %d\n", ring->name, ret); > - goto error; > + if (ringbuf->obj == NULL) { > + ret = intel_alloc_ringbuffer_obj(dev, ringbuf); > + if (ret) { > + DRM_ERROR("Failed to allocate ringbuffer %s: %d\n", > + ring->name, ret); > + goto error; > + } > + > + ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf); > + if (ret) { > + DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n", > + ring->name, ret); > + intel_destroy_ringbuffer_obj(ringbuf); > + goto error; > + } > } > > /* Workaround an erratum on the i830 which causes a hang if > @@ -1854,6 +1868,7 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring) > intel_stop_ring_buffer(ring); > WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0); > > + intel_unpin_ringbuffer_obj(ringbuf); > intel_destroy_ringbuffer_obj(ringbuf); > ring->preallocated_lazy_request = NULL; > ring->outstanding_lazy_seqno = 0; > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h > index 8c002d2..365854ad 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.h > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h > @@ -382,6 +382,9 @@ intel_write_status_page(struct intel_engine_cs *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_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf); > +int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, > + struct intel_ringbuffer *ringbuf); > void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf); > int intel_alloc_ringbuffer_obj(struct drm_device *dev, > struct intel_ringbuffer *ringbuf);
On Tuesday 18 November 2014 12:07 PM, Deepak S wrote: > > On Thursday 13 November 2014 03:58 PM, Thomas Daniel wrote: >> Same as with the context, pinning to GGTT regardless is harmful (it >> badly fragments the GGTT and can even exhaust it). >> >> Unfortunately, this case is also more complex than the previous one >> because we need to map and access the ringbuffer in several places >> along the execbuffer path (and we cannot make do by leaving the >> default ringbuffer pinned, as before). Also, the context object >> itself contains a pointer to the ringbuffer address that we have to >> keep updated if we are going to allow the ringbuffer to move around. >> >> v2: Same as with the context pinning, we cannot really do it during >> an interrupt. Also, pin the default ringbuffers objects regardless >> (makes error capture a lot easier). >> >> v3: Rebased. Take a pin reference of the ringbuffer for each item >> in the execlist request queue because the hardware may still be using >> the ringbuffer after the MI_USER_INTERRUPT to notify the seqno update >> is executed. The ringbuffer must remain pinned until the context save >> is complete. No longer pin and unpin ringbuffer in >> populate_lr_context() - this transient address is meaningless and the >> pinning can cause a sleep while atomic. >> >> v4: Moved ringbuffer pin and unpin into the lr_context_pin functions. >> Downgraded pinning check BUG_ONs to WARN_ONs. >> >> v5: Reinstated WARN_ONs for unexpected execlist states. Removed unused >> variable. >> >> Issue: VIZ-4277 >> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> >> Signed-off-by: Thomas Daniel <thomas.daniel@intel.com> >> --- >> drivers/gpu/drm/i915/intel_lrc.c | 102 >> +++++++++++++++++++++++-------- >> drivers/gpu/drm/i915/intel_ringbuffer.c | 85 >> +++++++++++++++----------- >> drivers/gpu/drm/i915/intel_ringbuffer.h | 3 + >> 3 files changed, 128 insertions(+), 62 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c >> b/drivers/gpu/drm/i915/intel_lrc.c >> index f7fa0f7..ca20f91 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -202,6 +202,9 @@ enum { >> }; >> #define GEN8_CTX_ID_SHIFT 32 >> +static int intel_lr_context_pin(struct intel_engine_cs *ring, >> + struct intel_context *ctx); >> + >> /** >> * intel_sanitize_enable_execlists() - sanitize i915.enable_execlists >> * @dev: DRM device. >> @@ -339,7 +342,9 @@ static void execlists_elsp_write(struct >> intel_engine_cs *ring, >> spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); >> } >> -static int execlists_ctx_write_tail(struct drm_i915_gem_object >> *ctx_obj, u32 tail) >> +static int execlists_update_context(struct drm_i915_gem_object >> *ctx_obj, >> + struct drm_i915_gem_object *ring_obj, >> + u32 tail) >> { >> struct page *page; >> uint32_t *reg_state; >> @@ -348,6 +353,7 @@ static int execlists_ctx_write_tail(struct >> drm_i915_gem_object *ctx_obj, u32 tai >> reg_state = kmap_atomic(page); >> reg_state[CTX_RING_TAIL+1] = tail; >> + reg_state[CTX_RING_BUFFER_START+1] = >> i915_gem_obj_ggtt_offset(ring_obj); >> kunmap_atomic(reg_state); >> @@ -358,21 +364,25 @@ static int execlists_submit_context(struct >> intel_engine_cs *ring, >> struct intel_context *to0, u32 tail0, >> struct intel_context *to1, u32 tail1) >> { >> - struct drm_i915_gem_object *ctx_obj0; >> + struct drm_i915_gem_object *ctx_obj0 = to0->engine[ring->id].state; >> + struct intel_ringbuffer *ringbuf0 = to0->engine[ring->id].ringbuf; >> struct drm_i915_gem_object *ctx_obj1 = NULL; >> + struct intel_ringbuffer *ringbuf1 = NULL; >> - ctx_obj0 = to0->engine[ring->id].state; >> BUG_ON(!ctx_obj0); >> WARN_ON(!i915_gem_obj_is_pinned(ctx_obj0)); >> + WARN_ON(!i915_gem_obj_is_pinned(ringbuf0->obj)); >> - execlists_ctx_write_tail(ctx_obj0, tail0); >> + execlists_update_context(ctx_obj0, ringbuf0->obj, tail0); >> if (to1) { >> + ringbuf1 = to1->engine[ring->id].ringbuf; >> ctx_obj1 = to1->engine[ring->id].state; >> BUG_ON(!ctx_obj1); >> WARN_ON(!i915_gem_obj_is_pinned(ctx_obj1)); >> + WARN_ON(!i915_gem_obj_is_pinned(ringbuf1->obj)); >> - execlists_ctx_write_tail(ctx_obj1, tail1); >> + execlists_update_context(ctx_obj1, ringbuf1->obj, tail1); >> } >> execlists_elsp_write(ring, ctx_obj0, ctx_obj1); >> @@ -524,6 +534,10 @@ static int execlists_context_queue(struct >> intel_engine_cs *ring, >> return -ENOMEM; >> req->ctx = to; >> i915_gem_context_reference(req->ctx); >> + >> + if (to != ring->default_context) >> + intel_lr_context_pin(ring, to); >> + >> req->ring = ring; >> req->tail = tail; >> @@ -544,7 +558,7 @@ static int execlists_context_queue(struct >> intel_engine_cs *ring, >> if (to == tail_req->ctx) { >> WARN(tail_req->elsp_submitted != 0, >> - "More than 2 already-submitted reqs queued\n"); >> + "More than 2 already-submitted reqs queued\n"); >> list_del(&tail_req->execlist_link); >> list_add_tail(&tail_req->execlist_link, >> &ring->execlist_retired_req_list); >> @@ -732,6 +746,12 @@ void intel_execlists_retire_requests(struct >> intel_engine_cs *ring) >> spin_unlock_irqrestore(&ring->execlist_lock, flags); >> list_for_each_entry_safe(req, tmp, &retired_list, >> execlist_link) { >> + struct intel_context *ctx = req->ctx; >> + struct drm_i915_gem_object *ctx_obj = >> + ctx->engine[ring->id].state; >> + >> + if (ctx_obj && (ctx != ring->default_context)) >> + intel_lr_context_unpin(ring, ctx); >> intel_runtime_pm_put(dev_priv); >> i915_gem_context_unreference(req->ctx); >> list_del(&req->execlist_link); >> @@ -803,6 +823,7 @@ static int intel_lr_context_pin(struct >> intel_engine_cs *ring, >> struct intel_context *ctx) >> { >> struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state; >> + struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf; >> int ret = 0; >> WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex)); >> @@ -810,21 +831,35 @@ static int intel_lr_context_pin(struct >> intel_engine_cs *ring, >> ret = i915_gem_obj_ggtt_pin(ctx_obj, >> GEN8_LR_CONTEXT_ALIGN, 0); >> if (ret) >> - ctx->engine[ring->id].unpin_count = 0; >> + goto reset_unpin_count; >> + >> + ret = intel_pin_and_map_ringbuffer_obj(ring->dev, ringbuf); >> + if (ret) >> + goto unpin_ctx_obj; >> } >> return ret; >> + >> +unpin_ctx_obj: >> + i915_gem_object_ggtt_unpin(ctx_obj); >> +reset_unpin_count: >> + ctx->engine[ring->id].unpin_count = 0; >> + >> + return ret; >> } >> void intel_lr_context_unpin(struct intel_engine_cs *ring, >> struct intel_context *ctx) >> { >> struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state; >> + struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf; >> if (ctx_obj) { >> WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex)); > > With pin specific mutex from previous patch set removed Oops This comment was for previous patch in the series :( Since i reviewed the patch offline, comments got mixed :) Anyways you patch looks fine, Reviewed-by: Deepak S<deepak.s@linux.intel.com> > Reviewed-by: Deepak S<deepak.s@linux.intel.com> > >> - if (--ctx->engine[ring->id].unpin_count == 0) >> + if (--ctx->engine[ring->id].unpin_count == 0) { >> + intel_unpin_ringbuffer_obj(ringbuf); >> i915_gem_object_ggtt_unpin(ctx_obj); >> + } >> } >> } >> @@ -1541,7 +1576,6 @@ populate_lr_context(struct intel_context >> *ctx, struct drm_i915_gem_object *ctx_o >> { >> struct drm_device *dev = ring->dev; >> struct drm_i915_private *dev_priv = dev->dev_private; >> - struct drm_i915_gem_object *ring_obj = ringbuf->obj; >> struct i915_hw_ppgtt *ppgtt = ctx->ppgtt; >> struct page *page; >> uint32_t *reg_state; >> @@ -1587,7 +1621,9 @@ populate_lr_context(struct intel_context *ctx, >> struct drm_i915_gem_object *ctx_o >> reg_state[CTX_RING_TAIL] = RING_TAIL(ring->mmio_base); >> reg_state[CTX_RING_TAIL+1] = 0; >> reg_state[CTX_RING_BUFFER_START] = RING_START(ring->mmio_base); >> - reg_state[CTX_RING_BUFFER_START+1] = >> i915_gem_obj_ggtt_offset(ring_obj); >> + /* Ring buffer start address is not known until the buffer is >> pinned. >> + * It is written to the context image in execlists_update_context() >> + */ >> reg_state[CTX_RING_BUFFER_CONTROL] = RING_CTL(ring->mmio_base); >> reg_state[CTX_RING_BUFFER_CONTROL+1] = >> ((ringbuf->size - PAGE_SIZE) & RING_NR_PAGES) | >> RING_VALID; >> @@ -1669,10 +1705,12 @@ void intel_lr_context_free(struct >> intel_context *ctx) >> ctx->engine[i].ringbuf; >> struct intel_engine_cs *ring = ringbuf->ring; >> + if (ctx == ring->default_context) { >> + intel_unpin_ringbuffer_obj(ringbuf); >> + i915_gem_object_ggtt_unpin(ctx_obj); >> + } >> intel_destroy_ringbuffer_obj(ringbuf); >> kfree(ringbuf); >> - if (ctx == ring->default_context) >> - i915_gem_object_ggtt_unpin(ctx_obj); >> drm_gem_object_unreference(&ctx_obj->base); >> } >> } >> @@ -1770,11 +1808,8 @@ int intel_lr_context_deferred_create(struct >> intel_context *ctx, >> if (!ringbuf) { >> DRM_DEBUG_DRIVER("Failed to allocate ringbuffer %s\n", >> ring->name); >> - if (is_global_default_ctx) >> - i915_gem_object_ggtt_unpin(ctx_obj); >> - drm_gem_object_unreference(&ctx_obj->base); >> ret = -ENOMEM; >> - return ret; >> + goto error_unpin_ctx; >> } >> ringbuf->ring = ring; >> @@ -1787,22 +1822,30 @@ int intel_lr_context_deferred_create(struct >> intel_context *ctx, >> ringbuf->space = ringbuf->size; >> ringbuf->last_retired_head = -1; >> - /* TODO: For now we put this in the mappable region so that we >> can reuse >> - * the existing ringbuffer code which ioremaps it. When we start >> - * creating many contexts, this will no longer work and we must >> switch >> - * to a kmapish interface. >> - */ >> - ret = intel_alloc_ringbuffer_obj(dev, ringbuf); >> - if (ret) { >> - DRM_DEBUG_DRIVER("Failed to allocate ringbuffer obj %s: %d\n", >> + if (ringbuf->obj == NULL) { >> + ret = intel_alloc_ringbuffer_obj(dev, ringbuf); >> + if (ret) { >> + DRM_DEBUG_DRIVER( >> + "Failed to allocate ringbuffer obj %s: %d\n", >> ring->name, ret); >> - goto error; >> + goto error_free_rbuf; >> + } >> + >> + if (is_global_default_ctx) { >> + ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf); >> + if (ret) { >> + DRM_ERROR( >> + "Failed to pin and map ringbuffer %s: %d\n", >> + ring->name, ret); >> + goto error_destroy_rbuf; >> + } >> + } >> + >> } >> ret = populate_lr_context(ctx, ctx_obj, ring, ringbuf); >> if (ret) { >> DRM_DEBUG_DRIVER("Failed to populate LRC: %d\n", ret); >> - intel_destroy_ringbuffer_obj(ringbuf); >> goto error; >> } >> @@ -1823,7 +1866,6 @@ int intel_lr_context_deferred_create(struct >> intel_context *ctx, >> DRM_ERROR("Init render state failed: %d\n", ret); >> ctx->engine[ring->id].ringbuf = NULL; >> ctx->engine[ring->id].state = NULL; >> - intel_destroy_ringbuffer_obj(ringbuf); >> goto error; >> } >> ctx->rcs_initialized = true; >> @@ -1832,7 +1874,13 @@ int intel_lr_context_deferred_create(struct >> intel_context *ctx, >> return 0; >> error: >> + if (is_global_default_ctx) >> + intel_unpin_ringbuffer_obj(ringbuf); >> +error_destroy_rbuf: >> + intel_destroy_ringbuffer_obj(ringbuf); >> +error_free_rbuf: >> kfree(ringbuf); >> +error_unpin_ctx: >> if (is_global_default_ctx) >> i915_gem_object_ggtt_unpin(ctx_obj); >> drm_gem_object_unreference(&ctx_obj->base); >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c >> b/drivers/gpu/drm/i915/intel_ringbuffer.c >> index a8f72e8..0c4aab1 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >> @@ -1721,13 +1721,42 @@ static int init_phys_status_page(struct >> intel_engine_cs *ring) >> return 0; >> } >> -void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf) >> +void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf) >> { >> - if (!ringbuf->obj) >> - return; >> - >> iounmap(ringbuf->virtual_start); >> + ringbuf->virtual_start = NULL; >> i915_gem_object_ggtt_unpin(ringbuf->obj); >> +} >> + >> +int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, >> + struct intel_ringbuffer *ringbuf) >> +{ >> + struct drm_i915_private *dev_priv = to_i915(dev); >> + struct drm_i915_gem_object *obj = ringbuf->obj; >> + int ret; >> + >> + ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE); >> + if (ret) >> + return ret; >> + >> + ret = i915_gem_object_set_to_gtt_domain(obj, true); >> + if (ret) { >> + i915_gem_object_ggtt_unpin(obj); >> + return ret; >> + } >> + >> + ringbuf->virtual_start = ioremap_wc(dev_priv->gtt.mappable_base + >> + i915_gem_obj_ggtt_offset(obj), ringbuf->size); >> + if (ringbuf->virtual_start == NULL) { >> + i915_gem_object_ggtt_unpin(obj); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> +void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf) >> +{ >> drm_gem_object_unreference(&ringbuf->obj->base); >> ringbuf->obj = NULL; >> } >> @@ -1735,12 +1764,7 @@ void intel_destroy_ringbuffer_obj(struct >> intel_ringbuffer *ringbuf) >> int intel_alloc_ringbuffer_obj(struct drm_device *dev, >> struct intel_ringbuffer *ringbuf) >> { >> - struct drm_i915_private *dev_priv = to_i915(dev); >> struct drm_i915_gem_object *obj; >> - int ret; >> - >> - if (ringbuf->obj) >> - return 0; >> obj = NULL; >> if (!HAS_LLC(dev)) >> @@ -1753,30 +1777,9 @@ int intel_alloc_ringbuffer_obj(struct >> drm_device *dev, >> /* mark ring buffers as read-only from GPU side by default */ >> obj->gt_ro = 1; >> - ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE); >> - if (ret) >> - goto err_unref; >> - >> - ret = i915_gem_object_set_to_gtt_domain(obj, true); >> - if (ret) >> - goto err_unpin; >> - >> - ringbuf->virtual_start = >> - ioremap_wc(dev_priv->gtt.mappable_base + >> i915_gem_obj_ggtt_offset(obj), >> - ringbuf->size); >> - if (ringbuf->virtual_start == NULL) { >> - ret = -EINVAL; >> - goto err_unpin; >> - } >> - >> ringbuf->obj = obj; >> - return 0; >> -err_unpin: >> - i915_gem_object_ggtt_unpin(obj); >> -err_unref: >> - drm_gem_object_unreference(&obj->base); >> - return ret; >> + return 0; >> } >> static int intel_init_ring_buffer(struct drm_device *dev, >> @@ -1813,10 +1816,21 @@ static int intel_init_ring_buffer(struct >> drm_device *dev, >> goto error; >> } >> - ret = intel_alloc_ringbuffer_obj(dev, ringbuf); >> - if (ret) { >> - DRM_ERROR("Failed to allocate ringbuffer %s: %d\n", >> ring->name, ret); >> - goto error; >> + if (ringbuf->obj == NULL) { >> + ret = intel_alloc_ringbuffer_obj(dev, ringbuf); >> + if (ret) { >> + DRM_ERROR("Failed to allocate ringbuffer %s: %d\n", >> + ring->name, ret); >> + goto error; >> + } >> + >> + ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf); >> + if (ret) { >> + DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n", >> + ring->name, ret); >> + intel_destroy_ringbuffer_obj(ringbuf); >> + goto error; >> + } >> } >> /* Workaround an erratum on the i830 which causes a hang if >> @@ -1854,6 +1868,7 @@ void intel_cleanup_ring_buffer(struct >> intel_engine_cs *ring) >> intel_stop_ring_buffer(ring); >> WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & >> MODE_IDLE) == 0); >> + intel_unpin_ringbuffer_obj(ringbuf); >> intel_destroy_ringbuffer_obj(ringbuf); >> ring->preallocated_lazy_request = NULL; >> ring->outstanding_lazy_seqno = 0; >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h >> b/drivers/gpu/drm/i915/intel_ringbuffer.h >> index 8c002d2..365854ad 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h >> @@ -382,6 +382,9 @@ intel_write_status_page(struct intel_engine_cs >> *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_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf); >> +int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, >> + struct intel_ringbuffer *ringbuf); >> void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf); >> int intel_alloc_ringbuffer_obj(struct drm_device *dev, >> struct intel_ringbuffer *ringbuf); > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Monday 17 November 2014 07:59 PM, Daniel Vetter wrote: > On Tue, Nov 18, 2014 at 12:09:54PM +0530, Deepak S wrote: >> On Tuesday 18 November 2014 12:07 PM, Deepak S wrote: >>> With pin specific mutex from previous patch set removed >> Oops This comment was for previous patch in the series :( Since i >> reviewed the patch offline, comments got mixed :) > Please forward these comments from the private discussion to the mailing > list. Review isn't just about code correctness, but about communication - > yes, I (and domain experts) actually read all this stuff that floats > around and will jump into the discussion if there's something important or > tricky being discussed. > > Second reason for public review is that the important part about the r-b > tag isn't that review happened, but by whom. So this is all about > reputation building and playing to people's various strenght. And if you > do review in private nothing of that can happen, which makes the review a > lot less useful. So let's extract the most value from all that engineering > time we invest into reviewing and _always_ do the review in public. > > Thanks, Daniel Thanks Daniel. I will make sure to add the comments to mail list :)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index f7fa0f7..ca20f91 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -202,6 +202,9 @@ enum { }; #define GEN8_CTX_ID_SHIFT 32 +static int intel_lr_context_pin(struct intel_engine_cs *ring, + struct intel_context *ctx); + /** * intel_sanitize_enable_execlists() - sanitize i915.enable_execlists * @dev: DRM device. @@ -339,7 +342,9 @@ static void execlists_elsp_write(struct intel_engine_cs *ring, spin_unlock_irqrestore(&dev_priv->uncore.lock, flags); } -static int execlists_ctx_write_tail(struct drm_i915_gem_object *ctx_obj, u32 tail) +static int execlists_update_context(struct drm_i915_gem_object *ctx_obj, + struct drm_i915_gem_object *ring_obj, + u32 tail) { struct page *page; uint32_t *reg_state; @@ -348,6 +353,7 @@ static int execlists_ctx_write_tail(struct drm_i915_gem_object *ctx_obj, u32 tai reg_state = kmap_atomic(page); reg_state[CTX_RING_TAIL+1] = tail; + reg_state[CTX_RING_BUFFER_START+1] = i915_gem_obj_ggtt_offset(ring_obj); kunmap_atomic(reg_state); @@ -358,21 +364,25 @@ static int execlists_submit_context(struct intel_engine_cs *ring, struct intel_context *to0, u32 tail0, struct intel_context *to1, u32 tail1) { - struct drm_i915_gem_object *ctx_obj0; + struct drm_i915_gem_object *ctx_obj0 = to0->engine[ring->id].state; + struct intel_ringbuffer *ringbuf0 = to0->engine[ring->id].ringbuf; struct drm_i915_gem_object *ctx_obj1 = NULL; + struct intel_ringbuffer *ringbuf1 = NULL; - ctx_obj0 = to0->engine[ring->id].state; BUG_ON(!ctx_obj0); WARN_ON(!i915_gem_obj_is_pinned(ctx_obj0)); + WARN_ON(!i915_gem_obj_is_pinned(ringbuf0->obj)); - execlists_ctx_write_tail(ctx_obj0, tail0); + execlists_update_context(ctx_obj0, ringbuf0->obj, tail0); if (to1) { + ringbuf1 = to1->engine[ring->id].ringbuf; ctx_obj1 = to1->engine[ring->id].state; BUG_ON(!ctx_obj1); WARN_ON(!i915_gem_obj_is_pinned(ctx_obj1)); + WARN_ON(!i915_gem_obj_is_pinned(ringbuf1->obj)); - execlists_ctx_write_tail(ctx_obj1, tail1); + execlists_update_context(ctx_obj1, ringbuf1->obj, tail1); } execlists_elsp_write(ring, ctx_obj0, ctx_obj1); @@ -524,6 +534,10 @@ static int execlists_context_queue(struct intel_engine_cs *ring, return -ENOMEM; req->ctx = to; i915_gem_context_reference(req->ctx); + + if (to != ring->default_context) + intel_lr_context_pin(ring, to); + req->ring = ring; req->tail = tail; @@ -544,7 +558,7 @@ static int execlists_context_queue(struct intel_engine_cs *ring, if (to == tail_req->ctx) { WARN(tail_req->elsp_submitted != 0, - "More than 2 already-submitted reqs queued\n"); + "More than 2 already-submitted reqs queued\n"); list_del(&tail_req->execlist_link); list_add_tail(&tail_req->execlist_link, &ring->execlist_retired_req_list); @@ -732,6 +746,12 @@ void intel_execlists_retire_requests(struct intel_engine_cs *ring) spin_unlock_irqrestore(&ring->execlist_lock, flags); list_for_each_entry_safe(req, tmp, &retired_list, execlist_link) { + struct intel_context *ctx = req->ctx; + struct drm_i915_gem_object *ctx_obj = + ctx->engine[ring->id].state; + + if (ctx_obj && (ctx != ring->default_context)) + intel_lr_context_unpin(ring, ctx); intel_runtime_pm_put(dev_priv); i915_gem_context_unreference(req->ctx); list_del(&req->execlist_link); @@ -803,6 +823,7 @@ static int intel_lr_context_pin(struct intel_engine_cs *ring, struct intel_context *ctx) { struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state; + struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf; int ret = 0; WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex)); @@ -810,21 +831,35 @@ static int intel_lr_context_pin(struct intel_engine_cs *ring, ret = i915_gem_obj_ggtt_pin(ctx_obj, GEN8_LR_CONTEXT_ALIGN, 0); if (ret) - ctx->engine[ring->id].unpin_count = 0; + goto reset_unpin_count; + + ret = intel_pin_and_map_ringbuffer_obj(ring->dev, ringbuf); + if (ret) + goto unpin_ctx_obj; } return ret; + +unpin_ctx_obj: + i915_gem_object_ggtt_unpin(ctx_obj); +reset_unpin_count: + ctx->engine[ring->id].unpin_count = 0; + + return ret; } void intel_lr_context_unpin(struct intel_engine_cs *ring, struct intel_context *ctx) { struct drm_i915_gem_object *ctx_obj = ctx->engine[ring->id].state; + struct intel_ringbuffer *ringbuf = ctx->engine[ring->id].ringbuf; if (ctx_obj) { WARN_ON(!mutex_is_locked(&ring->dev->struct_mutex)); - if (--ctx->engine[ring->id].unpin_count == 0) + if (--ctx->engine[ring->id].unpin_count == 0) { + intel_unpin_ringbuffer_obj(ringbuf); i915_gem_object_ggtt_unpin(ctx_obj); + } } } @@ -1541,7 +1576,6 @@ populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_o { struct drm_device *dev = ring->dev; struct drm_i915_private *dev_priv = dev->dev_private; - struct drm_i915_gem_object *ring_obj = ringbuf->obj; struct i915_hw_ppgtt *ppgtt = ctx->ppgtt; struct page *page; uint32_t *reg_state; @@ -1587,7 +1621,9 @@ populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_o reg_state[CTX_RING_TAIL] = RING_TAIL(ring->mmio_base); reg_state[CTX_RING_TAIL+1] = 0; reg_state[CTX_RING_BUFFER_START] = RING_START(ring->mmio_base); - reg_state[CTX_RING_BUFFER_START+1] = i915_gem_obj_ggtt_offset(ring_obj); + /* Ring buffer start address is not known until the buffer is pinned. + * It is written to the context image in execlists_update_context() + */ reg_state[CTX_RING_BUFFER_CONTROL] = RING_CTL(ring->mmio_base); reg_state[CTX_RING_BUFFER_CONTROL+1] = ((ringbuf->size - PAGE_SIZE) & RING_NR_PAGES) | RING_VALID; @@ -1669,10 +1705,12 @@ void intel_lr_context_free(struct intel_context *ctx) ctx->engine[i].ringbuf; struct intel_engine_cs *ring = ringbuf->ring; + if (ctx == ring->default_context) { + intel_unpin_ringbuffer_obj(ringbuf); + i915_gem_object_ggtt_unpin(ctx_obj); + } intel_destroy_ringbuffer_obj(ringbuf); kfree(ringbuf); - if (ctx == ring->default_context) - i915_gem_object_ggtt_unpin(ctx_obj); drm_gem_object_unreference(&ctx_obj->base); } } @@ -1770,11 +1808,8 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, if (!ringbuf) { DRM_DEBUG_DRIVER("Failed to allocate ringbuffer %s\n", ring->name); - if (is_global_default_ctx) - i915_gem_object_ggtt_unpin(ctx_obj); - drm_gem_object_unreference(&ctx_obj->base); ret = -ENOMEM; - return ret; + goto error_unpin_ctx; } ringbuf->ring = ring; @@ -1787,22 +1822,30 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, ringbuf->space = ringbuf->size; ringbuf->last_retired_head = -1; - /* TODO: For now we put this in the mappable region so that we can reuse - * the existing ringbuffer code which ioremaps it. When we start - * creating many contexts, this will no longer work and we must switch - * to a kmapish interface. - */ - ret = intel_alloc_ringbuffer_obj(dev, ringbuf); - if (ret) { - DRM_DEBUG_DRIVER("Failed to allocate ringbuffer obj %s: %d\n", + if (ringbuf->obj == NULL) { + ret = intel_alloc_ringbuffer_obj(dev, ringbuf); + if (ret) { + DRM_DEBUG_DRIVER( + "Failed to allocate ringbuffer obj %s: %d\n", ring->name, ret); - goto error; + goto error_free_rbuf; + } + + if (is_global_default_ctx) { + ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf); + if (ret) { + DRM_ERROR( + "Failed to pin and map ringbuffer %s: %d\n", + ring->name, ret); + goto error_destroy_rbuf; + } + } + } ret = populate_lr_context(ctx, ctx_obj, ring, ringbuf); if (ret) { DRM_DEBUG_DRIVER("Failed to populate LRC: %d\n", ret); - intel_destroy_ringbuffer_obj(ringbuf); goto error; } @@ -1823,7 +1866,6 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, DRM_ERROR("Init render state failed: %d\n", ret); ctx->engine[ring->id].ringbuf = NULL; ctx->engine[ring->id].state = NULL; - intel_destroy_ringbuffer_obj(ringbuf); goto error; } ctx->rcs_initialized = true; @@ -1832,7 +1874,13 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, return 0; error: + if (is_global_default_ctx) + intel_unpin_ringbuffer_obj(ringbuf); +error_destroy_rbuf: + intel_destroy_ringbuffer_obj(ringbuf); +error_free_rbuf: kfree(ringbuf); +error_unpin_ctx: if (is_global_default_ctx) i915_gem_object_ggtt_unpin(ctx_obj); drm_gem_object_unreference(&ctx_obj->base); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index a8f72e8..0c4aab1 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1721,13 +1721,42 @@ static int init_phys_status_page(struct intel_engine_cs *ring) return 0; } -void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf) +void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf) { - if (!ringbuf->obj) - return; - iounmap(ringbuf->virtual_start); + ringbuf->virtual_start = NULL; i915_gem_object_ggtt_unpin(ringbuf->obj); +} + +int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, + struct intel_ringbuffer *ringbuf) +{ + struct drm_i915_private *dev_priv = to_i915(dev); + struct drm_i915_gem_object *obj = ringbuf->obj; + int ret; + + ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE); + if (ret) + return ret; + + ret = i915_gem_object_set_to_gtt_domain(obj, true); + if (ret) { + i915_gem_object_ggtt_unpin(obj); + return ret; + } + + ringbuf->virtual_start = ioremap_wc(dev_priv->gtt.mappable_base + + i915_gem_obj_ggtt_offset(obj), ringbuf->size); + if (ringbuf->virtual_start == NULL) { + i915_gem_object_ggtt_unpin(obj); + return -EINVAL; + } + + return 0; +} + +void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf) +{ drm_gem_object_unreference(&ringbuf->obj->base); ringbuf->obj = NULL; } @@ -1735,12 +1764,7 @@ void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf) int intel_alloc_ringbuffer_obj(struct drm_device *dev, struct intel_ringbuffer *ringbuf) { - struct drm_i915_private *dev_priv = to_i915(dev); struct drm_i915_gem_object *obj; - int ret; - - if (ringbuf->obj) - return 0; obj = NULL; if (!HAS_LLC(dev)) @@ -1753,30 +1777,9 @@ int intel_alloc_ringbuffer_obj(struct drm_device *dev, /* mark ring buffers as read-only from GPU side by default */ obj->gt_ro = 1; - ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE); - if (ret) - goto err_unref; - - ret = i915_gem_object_set_to_gtt_domain(obj, true); - if (ret) - goto err_unpin; - - ringbuf->virtual_start = - ioremap_wc(dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj), - ringbuf->size); - if (ringbuf->virtual_start == NULL) { - ret = -EINVAL; - goto err_unpin; - } - ringbuf->obj = obj; - return 0; -err_unpin: - i915_gem_object_ggtt_unpin(obj); -err_unref: - drm_gem_object_unreference(&obj->base); - return ret; + return 0; } static int intel_init_ring_buffer(struct drm_device *dev, @@ -1813,10 +1816,21 @@ static int intel_init_ring_buffer(struct drm_device *dev, goto error; } - ret = intel_alloc_ringbuffer_obj(dev, ringbuf); - if (ret) { - DRM_ERROR("Failed to allocate ringbuffer %s: %d\n", ring->name, ret); - goto error; + if (ringbuf->obj == NULL) { + ret = intel_alloc_ringbuffer_obj(dev, ringbuf); + if (ret) { + DRM_ERROR("Failed to allocate ringbuffer %s: %d\n", + ring->name, ret); + goto error; + } + + ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf); + if (ret) { + DRM_ERROR("Failed to pin and map ringbuffer %s: %d\n", + ring->name, ret); + intel_destroy_ringbuffer_obj(ringbuf); + goto error; + } } /* Workaround an erratum on the i830 which causes a hang if @@ -1854,6 +1868,7 @@ void intel_cleanup_ring_buffer(struct intel_engine_cs *ring) intel_stop_ring_buffer(ring); WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0); + intel_unpin_ringbuffer_obj(ringbuf); intel_destroy_ringbuffer_obj(ringbuf); ring->preallocated_lazy_request = NULL; ring->outstanding_lazy_seqno = 0; diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 8c002d2..365854ad 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -382,6 +382,9 @@ intel_write_status_page(struct intel_engine_cs *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_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf); +int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev, + struct intel_ringbuffer *ringbuf); void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf); int intel_alloc_ringbuffer_obj(struct drm_device *dev, struct intel_ringbuffer *ringbuf);