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@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
