[Intel-gfx] [PATCH 1/2] drm/i915: Refactor common ringbuffer allocation code
Mika Kuoppala
mika.kuoppala at linux.intel.com
Thu Sep 3 07:56:20 PDT 2015
"Zanoni, Paulo R" <paulo.r.zanoni at intel.com> writes:
> Em Qui, 2015-09-03 às 13:01 +0100, Chris Wilson escreveu:
>> A small, very small, step to sharing the duplicate code between
>> execlists and legacy submission engines, starting with the ringbuffer
>> allocation code.
>>
>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Arun Siluvery <arun.siluvery at linux.intel.com>
>> Cc: Mika Kuoppala <mika.kuoppala at intel.com>
>> Cc: Dave Gordon <david.s.gordon at intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_lrc.c | 49 +++++-------------
>> drivers/gpu/drm/i915/intel_ringbuffer.c | 89 ++++++++++++++++++++---
>> ----------
>> drivers/gpu/drm/i915/intel_ringbuffer.h | 8 +--
>> 3 files changed, 70 insertions(+), 76 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
>> b/drivers/gpu/drm/i915/intel_lrc.c
>> index 40cbba4ea4ba..28a712e7d2d0 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -2340,8 +2340,7 @@ void intel_lr_context_free(struct intel_context
>> *ctx)
>> i915_gem_object_ggtt_unpin(ctx_obj);
>> }
>> WARN_ON(ctx->engine[ring->id].pin_count);
>> - intel_destroy_ringbuffer_obj(ringbuf);
>> - kfree(ringbuf);
>> + intel_ringbuffer_free(ringbuf);
>> drm_gem_object_unreference(&ctx_obj->base);
>> }
>> }
>> @@ -2442,42 +2441,20 @@ int intel_lr_context_deferred_create(struct
>> intel_context *ctx,
>> I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
>> }
>>
>> - ringbuf = kzalloc(sizeof(*ringbuf), GFP_KERNEL);
>> - if (!ringbuf) {
>> - DRM_DEBUG_DRIVER("Failed to allocate ringbuffer
>> %s\n",
>
> We got rid of this message, but I suppose it's not a problem, since it
> was not a loud error message.
>
>
>> - ring->name);
>> - ret = -ENOMEM;
>> + ringbuf = intel_engine_create_ringbuffer(ring, 4 *
>> PAGE_SIZE);
>> + if (IS_ERR(ringbuf)) {
>> + ret = PTR_ERR(ringbuf);
>> goto error_unpin_ctx;
>> }
>>
>> - ringbuf->ring = ring;
>> -
>> - ringbuf->size = 4 * PAGE_SIZE;
>> - ringbuf->effective_size = ringbuf->size;
>> - ringbuf->head = 0;
>> - ringbuf->tail = 0;
>> - ringbuf->last_retired_head = -1;
>> - intel_ring_update_space(ringbuf);
>> -
>> - if (ringbuf->obj == NULL) {
>> - ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
>> + if (is_global_default_ctx) {
>> + ret = intel_pin_and_map_ringbuffer_obj(dev,
>> ringbuf);
>> if (ret) {
>> - DRM_DEBUG_DRIVER(
>> - "Failed to allocate ringbuffer obj
>> %s: %d\n",
>> - ring->name, ret);
>> - goto error_free_rbuf;
>> + DRM_ERROR(
>> + "Failed to pin and map ringbuffer
>> %s: %d\n",
>> + ring->name, ret);
>> + goto error_ringbuf;
>> }
>> -
>> - 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);
>> @@ -2519,10 +2496,8 @@ int intel_lr_context_deferred_create(struct
>> intel_context *ctx,
>> 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_ringbuf:
>> + intel_ringbuffer_free(ringbuf);
>> error_unpin_ctx:
>> if (is_global_default_ctx)
>> i915_gem_object_ggtt_unpin(ctx_obj);
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> index 6e6b8db996ef..20a75bb516ac 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
>> @@ -1996,14 +1996,14 @@ int intel_pin_and_map_ringbuffer_obj(struct
>> drm_device *dev,
>> return 0;
>> }
>>
>> -void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
>> +static void intel_destroy_ringbuffer_obj(struct intel_ringbuffer
>> *ringbuf)
>> {
>> drm_gem_object_unreference(&ringbuf->obj->base);
>> ringbuf->obj = NULL;
>> }
>>
>> -int intel_alloc_ringbuffer_obj(struct drm_device *dev,
>> - struct intel_ringbuffer *ringbuf)
>> +static int intel_alloc_ringbuffer_obj(struct drm_device *dev,
>> + struct intel_ringbuffer
>> *ringbuf)
>> {
>> struct drm_i915_gem_object *obj;
>>
>> @@ -2023,6 +2023,48 @@ int intel_alloc_ringbuffer_obj(struct
>> drm_device *dev,
>> return 0;
>> }
>>
>> +struct intel_ringbuffer *
>> +intel_engine_create_ringbuffer(struct intel_engine_cs *engine, int
>> size)
>> +{
>> + struct intel_ringbuffer *ring;
>> + int ret;
>> +
>> + ring = kzalloc(sizeof(*ring), GFP_KERNEL);
>> + if (ring == NULL)
>
> The error message referenced above should probably be here.
>
>
>> + return ERR_PTR(-ENOMEM);
>> +
>> + ring->ring = engine;
>> +
>> + ring->size = size;
>> + /* 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 = size;
>> + if (IS_I830(engine->dev) || IS_845G(engine->dev))
>> + ring->effective_size -= 2 * CACHELINE_BYTES;
>> +
>> + ring->last_retired_head = -1;
>> + intel_ring_update_space(ring);
>> +
>> + ret = intel_alloc_ringbuffer_obj(engine->dev, ring);
>> + if (ret) {
>> + DRM_ERROR("Failed to allocate ringbuffer %s: %d\n",
>
> Shouldn't this be "Failed to allocate ringbuffer obj %s: %d" ?
>
>> + engine->name, ret);
>> + kfree(ring);
>> + return ERR_PTR(ret);
>> + }
>> +
>> + return ring;
>> +}
>> +
>> +void
>> +intel_ringbuffer_free(struct intel_ringbuffer *ring)
>> +{
>> + intel_destroy_ringbuffer_obj(ring);
>> + kfree(ring);
>> +}
>> +
>> static int intel_init_ring_buffer(struct drm_device *dev,
>> struct intel_engine_cs *ring)
>> {
>> @@ -2031,22 +2073,20 @@ static int intel_init_ring_buffer(struct
>> drm_device *dev,
>>
>> WARN_ON(ring->buffer);
>>
>> - ringbuf = kzalloc(sizeof(*ringbuf), GFP_KERNEL);
>> - if (!ringbuf)
>> - return -ENOMEM;
>> - ring->buffer = ringbuf;
>> -
>> ring->dev = dev;
>> INIT_LIST_HEAD(&ring->active_list);
>> INIT_LIST_HEAD(&ring->request_list);
>> INIT_LIST_HEAD(&ring->execlist_queue);
>> i915_gem_batch_pool_init(dev, &ring->batch_pool);
>> - ringbuf->size = 32 * PAGE_SIZE;
>> - ringbuf->ring = ring;
>> memset(ring->semaphore.sync_seqno, 0, sizeof(ring
>> ->semaphore.sync_seqno));
>>
>> init_waitqueue_head(&ring->irq_queue);
>>
>> + ringbuf = intel_engine_create_ringbuffer(ring, 32 *
>> PAGE_SIZE);
>> + if (IS_ERR(ringbuf))
>> + return PTR_ERR(ringbuf);
>> + ring->buffer = ringbuf;
>> +
>> if (I915_NEED_GFX_HWS(dev)) {
>> ret = init_status_page(ring);
>> if (ret)
>> @@ -2058,15 +2098,6 @@ static int intel_init_ring_buffer(struct
>> drm_device *dev,
>> goto error;
>> }
>>
>> - WARN_ON(ringbuf->obj);
>> -
>> - 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",
>> @@ -2075,14 +2106,6 @@ static int intel_init_ring_buffer(struct
>> drm_device *dev,
>> goto error;
>> }
>>
>> - /* Workaround an erratum on the i830 which causes a hang if
>> - * the TAIL pointer points to within the last 2 cachelines
>> - * of the buffer.
>> - */
>> - ringbuf->effective_size = ringbuf->size;
>> - if (IS_I830(dev) || IS_845G(dev))
>> - ringbuf->effective_size -= 2 * CACHELINE_BYTES;
>> -
>> ret = i915_cmd_parser_init_ring(ring);
>> if (ret)
>> goto error;
>> @@ -2090,7 +2113,7 @@ static int intel_init_ring_buffer(struct
>> drm_device *dev,
>> return 0;
>
> The "goto error" right above the return 0 changes its behavior: it
> wasn't calling intel_destroy_ringbuffer_obj(), but I suppose this is a
> fix. I'm just pointing it since I'm not 100% sure.
>
I would say its a fix because we lost the ref at that point.
Reviewed-by: Mika Kuoppala <mika.kuoppala at intel.com>
> Everything seems correct, so with or without changes:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
>
>>
>> error:
>> - kfree(ringbuf);
>> + intel_ringbuffer_free(ringbuf);
>> ring->buffer = NULL;
>> return ret;
>> }
>> @@ -2098,19 +2121,18 @@ error:
>> void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
>> {
>> struct drm_i915_private *dev_priv;
>> - struct intel_ringbuffer *ringbuf;
>>
>> if (!intel_ring_initialized(ring))
>> return;
>>
>> dev_priv = to_i915(ring->dev);
>> - ringbuf = ring->buffer;
>>
>> 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);
>> + intel_unpin_ringbuffer_obj(ring->buffer);
>> + intel_ringbuffer_free(ring->buffer);
>> + ring->buffer = NULL;
>>
>> if (ring->cleanup)
>> ring->cleanup(ring);
>> @@ -2119,9 +2141,6 @@ void intel_cleanup_ring_buffer(struct
>> intel_engine_cs *ring)
>>
>> i915_cmd_parser_fini_ring(ring);
>> i915_gem_batch_pool_fini(&ring->batch_pool);
>> -
>> - kfree(ringbuf);
>> - ring->buffer = NULL;
>> }
>>
>> static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> index 95b0b4b55fa6..49fa41dc0eb6 100644
>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
>> @@ -420,12 +420,12 @@ intel_write_status_page(struct intel_engine_cs
>> *ring,
>> #define I915_GEM_HWS_SCRATCH_INDEX 0x40
>> #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);
>> +struct intel_ringbuffer *
>> +intel_engine_create_ringbuffer(struct intel_engine_cs *engine, int
>> size);
>> 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);
>> +void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf);
>> +void intel_ringbuffer_free(struct intel_ringbuffer *ring);
>>
>> void intel_stop_ring_buffer(struct intel_engine_cs *ring);
>> void intel_cleanup_ring_buffer(struct intel_engine_cs *ring);
More information about the Intel-gfx
mailing list