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

[Nick Hoath]

On 16/12/2015 18:36, Gordon, David S 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'.
>
Reviewed-by: Nick Hoath <nicholas.hoath at intel.com>

> 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 eefce9a..2853754 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -549,6 +549,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;
>   }
>
> @@ -2057,12 +2059,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)
>   {
> @@ -2125,11 +2121,14 @@ intel_engine_create_ringbuffer(struct intel_engine_cs *engine, int size)
>   }
>
>   void
> -intel_ringbuffer_free(struct intel_ringbuffer *ring)
> +intel_ringbuffer_free(struct intel_ringbuffer *ringbuf)
>   {
> -	intel_destroy_ringbuffer_obj(ring);
> -	list_del(&ring->link);
> -	kfree(ring);
> +	if (ringbuf->obj) {
> +		drm_gem_object_unreference(&ringbuf->obj->base);
> +		ringbuf->obj = NULL;
> +	}
> +	list_del(&ringbuf->link);
> +	kfree(ringbuf);
>   }
>
>   static int intel_init_ring_buffer(struct drm_device *dev,
> @@ -2157,6 +2156,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)
> @@ -2168,14 +2174,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;
> @@ -2189,19 +2187,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);
> -
> -	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)
> +			intel_unpin_ringbuffer_obj(ringbuf);
> +		intel_ringbuffer_free(ringbuf);
>   		ring->buffer = NULL;
>   	}
>
>