[Intel-gfx] [PATCH v2 3/6] drm/i915: tidy up initialisation failure paths (legacy)

[Chris Wilson]

On Fri, Jan 22, 2016 at 11:10:08PM +0000, Dave Gordon wrote:
> 1. Fix intel_cleanup_ring_buffer() to handle the error cleanup case where
>    the ringbuffer has been allocated but map-and-pin failed.  Unpin it iff
>    it's previously been mapped-and-pinned.
> 
> 2. Fix the error path in intel_init_ring_buffer(), which already called
>    intel_destroy_ringbuffer_obj(), but failed to free the actual ringbuffer
>    structure. Calling intel_ringbuffer_free() instead does both in one go.
> 
> 3. With the above change, intel_destroy_ringbuffer_obj() is only called in
>    one place (intel_ringbuffer_free()), so flatten it into that function.
> 
> 4. move low-level register accesses from intel_cleanup_ring_buffer()
>    (which calls intel_stop_ring_buffer(ring) which calls stop_ring())
>    down into stop_ring() itself), which is already doing low-level
>    register accesses. Then, intel_cleanup_ring_buffer() no longer
>    needs 'dev_priv'.
> 
> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 47 +++++++++++++++------------------
>  1 file changed, 22 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 9030e2b..29de64e 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -551,6 +551,8 @@ static bool stop_ring(struct intel_engine_cs *ring)
>  		I915_WRITE_MODE(ring, _MASKED_BIT_DISABLE(STOP_RING));
>  	}
>  
> +	WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
> +
>  	return (I915_READ_HEAD(ring) & HEAD_ADDR) == 0;
>  }
>  
> @@ -2071,12 +2073,6 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
>  	return 0;
>  }
>  
> -static void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
> -{
> -	drm_gem_object_unreference(&ringbuf->obj->base);
> -	ringbuf->obj = NULL;
> -}
> -
>  static int intel_alloc_ringbuffer_obj(struct drm_device *dev,
>  				      struct intel_ringbuffer *ringbuf)
>  {
> @@ -2139,11 +2135,14 @@ struct intel_ringbuffer *
>  }
>  
>  void
> -intel_ringbuffer_free(struct intel_ringbuffer *ring)
> +intel_ringbuffer_free(struct intel_ringbuffer *ringbuf)

ringbuf?

> -	intel_destroy_ringbuffer_obj(ring);
> -	list_del(&ring->link);
> -	kfree(ring);
> +	if (ringbuf->obj) {
> +		drm_gem_object_unreference(&ringbuf->obj->base);

Already tests for NULL

> +		ringbuf->obj = NULL;

Redundant as we are aobut to free it.

> +	}
> +	list_del(&ringbuf->link);
> +	kfree(ringbuf);
>  }
>  
>  static int intel_init_ring_buffer(struct drm_device *dev,
> @@ -2171,6 +2170,13 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  	}
>  	ring->buffer = ringbuf;
>  
> +	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;
> +	}
> +
>  	if (I915_NEED_GFX_HWS(dev)) {
>  		ret = init_status_page(ring);
>  		if (ret)
> @@ -2182,14 +2188,6 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  			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;
> -	}
> -
>  	ret = i915_cmd_parser_init_ring(ring);
>  	if (ret)
>  		goto error;
> @@ -2203,19 +2201,18 @@ static int intel_init_ring_buffer(struct drm_device *dev,
>  
>  void intel_cleanup_ring_buffer(struct intel_engine_cs *ring)
>  {
> -	struct drm_i915_private *dev_priv;
> +	struct intel_ringbuffer *ringbuf;

ringbuf?

>  
>  	if (!intel_ring_initialized(ring))
>  		return;
>  
> -	dev_priv = to_i915(ring->dev);
> -
> -	if (ring->buffer) {
> +	ringbuf = ring->buffer;
> +	if (ringbuf) {
>  		intel_stop_ring_buffer(ring);
> -		WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) & MODE_IDLE) == 0);
>  
> -		intel_unpin_ringbuffer_obj(ring->buffer);
> -		intel_ringbuffer_free(ring->buffer);
> +		if (ringbuf->virtual_start)

Cleaner code, and more idiomatic, if we let unpin early return.

> +			intel_unpin_ringbuffer_obj(ringbuf);
> +		intel_ringbuffer_free(ringbuf);
>  		ring->buffer = NULL;
>  	}
>  
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Chris Wilson, Intel Open Source Technology Centre