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@xxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
