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

[Chris Wilson]

On Fri, Jan 29, 2016 at 07:19:26PM +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'.
> 
> v3:
>    Make intel_unpin_ringbuffer_obj() return early if ringbuffer is
>    not mapped, simplifying matters for the caller. [Chris Wilson,
>    Daniel Vetter]
>    Don't bother to clear a pointer in an object about to be freed.
>    [Chris Wilson]
> 
> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 49 ++++++++++++++++-----------------
>  1 file changed, 23 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 6f5b511..db02893 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);

This warn was guarding the release of the backing pages, to verify that
we had stopped the rings first.

> +
>  	return (I915_READ_HEAD(ring) & HEAD_ADDR) == 0;
>  }
>  
> @@ -2055,6 +2057,9 @@ static int init_phys_status_page(struct intel_engine_cs *ring)
>  
>  void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
>  {
> +	if (!ringbuf->virtual_start)
> +		return;
> +
>  	if (HAS_LLC(ringbuf->obj->base.dev) && !ringbuf->obj->stolen)
>  		vunmap(ringbuf->virtual_start);
>  	else
> @@ -2132,12 +2137,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)
>  {
> @@ -2200,11 +2199,13 @@ struct intel_ringbuffer *
>  }
>  
>  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);

No, let's not add duplicate code. If you worry,
replace to_intel_bo() with

static inline struct drm_i915_gem_object *
to_intel_bo(struct drm_gem_object *gem_obj)
{
	BUILD_BUG_ON(offsetof(struct drm_i915_gem_object, base));
	return container_of(gem_obj, struct drm_i915_gem_object, base);
}

and we can begin erradicating the half-baked checks (some places assume,
others do no-op conversions). Hindsight.

> +
> +	list_del(&ringbuf->link);
> +	kfree(ringbuf);
>  }
>  
>  static int intel_init_ring_buffer(struct drm_device *dev,
> @@ -2232,6 +2233,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)
> @@ -2243,14 +2251,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;
> @@ -2264,19 +2264,16 @@ 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;
>  
>  	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);
> +		intel_unpin_ringbuffer_obj(ringbuf);
> +		intel_ringbuffer_free(ringbuf);
>  		ring->buffer = NULL;

Conversions to a horrible name for no reason. ringbuf was a mistake,
please no more
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre