Re: [PATCH 1/2] drm/i915: Refactor common ringbuffer allocation code

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

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@chris-wilson.co.uk>
> Cc: Arun Siluvery <arun.siluvery@linux.intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Dave Gordon <david.s.gordon@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@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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx