[Intel-gfx] [PATCH 1/2] drm/i915: Refactor common ringbuffer allocation code
Zanoni, Paulo R
paulo.r.zanoni at intel.com
Thu Sep 3 07:01:51 PDT 2015
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.
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