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

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

"Zanoni, Paulo R" <paulo.r.zanoni@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@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.
>

I would say its a fix because we lost the ref at that point.

Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>

> 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