On Thu, Sep 03, 2015 at 05:56:20PM +0300, Mika Kuoppala wrote:
> "Zanoni, Paulo R" <paulo.r.zanoni@xxxxxxxxx> writes:
>
> > Em Qui, 2015-09-03 às 13:01 +0100, Chris Wilson escreveu:
> >> A small, very small, step to sharing the duplicate code between
> >> execlists and legacy submission engines, starting with the ringbuffer
> >> allocation code.
> >>
> >> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >> Cc: Arun Siluvery <arun.siluvery@xxxxxxxxxxxxxxx>
> >> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
> >> Cc: Dave Gordon <david.s.gordon@xxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/intel_lrc.c | 49 +++++-------------
> >> drivers/gpu/drm/i915/intel_ringbuffer.c | 89 ++++++++++++++++++++---
> >> ----------
> >> drivers/gpu/drm/i915/intel_ringbuffer.h | 8 +--
> >> 3 files changed, 70 insertions(+), 76 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> >> b/drivers/gpu/drm/i915/intel_lrc.c
> >> index 40cbba4ea4ba..28a712e7d2d0 100644
> >> --- a/drivers/gpu/drm/i915/intel_lrc.c
> >> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> >> @@ -2340,8 +2340,7 @@ void intel_lr_context_free(struct intel_context
> >> *ctx)
> >> i915_gem_object_ggtt_unpin(ctx_obj);
> >> }
> >> WARN_ON(ctx->engine[ring->id].pin_count);
> >> - intel_destroy_ringbuffer_obj(ringbuf);
> >> - kfree(ringbuf);
> >> + intel_ringbuffer_free(ringbuf);
> >> drm_gem_object_unreference(&ctx_obj->base);
> >> }
> >> }
> >> @@ -2442,42 +2441,20 @@ int intel_lr_context_deferred_create(struct
> >> intel_context *ctx,
> >> I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
> >> }
> >>
> >> - ringbuf = kzalloc(sizeof(*ringbuf), GFP_KERNEL);
> >> - if (!ringbuf) {
> >> - DRM_DEBUG_DRIVER("Failed to allocate ringbuffer
> >> %s\n",
> >
> > We got rid of this message, but I suppose it's not a problem, since it
> > was not a loud error message.
> >
> >
> >> - ring->name);
> >> - ret = -ENOMEM;
> >> + ringbuf = intel_engine_create_ringbuffer(ring, 4 *
> >> PAGE_SIZE);
> >> + if (IS_ERR(ringbuf)) {
> >> + ret = PTR_ERR(ringbuf);
> >> goto error_unpin_ctx;
> >> }
> >>
> >> - ringbuf->ring = ring;
> >> -
> >> - ringbuf->size = 4 * PAGE_SIZE;
> >> - ringbuf->effective_size = ringbuf->size;
> >> - ringbuf->head = 0;
> >> - ringbuf->tail = 0;
> >> - ringbuf->last_retired_head = -1;
> >> - intel_ring_update_space(ringbuf);
> >> -
> >> - if (ringbuf->obj == NULL) {
> >> - ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
> >> + if (is_global_default_ctx) {
> >> + ret = intel_pin_and_map_ringbuffer_obj(dev,
> >> ringbuf);
> >> if (ret) {
> >> - DRM_DEBUG_DRIVER(
> >> - "Failed to allocate ringbuffer obj
> >> %s: %d\n",
> >> - ring->name, ret);
> >> - goto error_free_rbuf;
> >> + DRM_ERROR(
> >> + "Failed to pin and map ringbuffer
> >> %s: %d\n",
> >> + ring->name, ret);
> >> + goto error_ringbuf;
> >> }
> >> -
> >> - if (is_global_default_ctx) {
> >> - 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_destroy_rbuf;
> >> - }
> >> - }
> >> -
> >> }
> >>
> >> ret = populate_lr_context(ctx, ctx_obj, ring, ringbuf);
> >> @@ -2519,10 +2496,8 @@ int intel_lr_context_deferred_create(struct
> >> intel_context *ctx,
> >> error:
> >> if (is_global_default_ctx)
> >> intel_unpin_ringbuffer_obj(ringbuf);
> >> -error_destroy_rbuf:
> >> - intel_destroy_ringbuffer_obj(ringbuf);
> >> -error_free_rbuf:
> >> - kfree(ringbuf);
> >> +error_ringbuf:
> >> + intel_ringbuffer_free(ringbuf);
> >> error_unpin_ctx:
> >> if (is_global_default_ctx)
> >> i915_gem_object_ggtt_unpin(ctx_obj);
> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> index 6e6b8db996ef..20a75bb516ac 100644
> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >> @@ -1996,14 +1996,14 @@ int intel_pin_and_map_ringbuffer_obj(struct
> >> drm_device *dev,
> >> return 0;
> >> }
> >>
> >> -void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
> >> +static void intel_destroy_ringbuffer_obj(struct intel_ringbuffer
> >> *ringbuf)
> >> {
> >> drm_gem_object_unreference(&ringbuf->obj->base);
> >> ringbuf->obj = NULL;
> >> }
> >>
> >> -int intel_alloc_ringbuffer_obj(struct drm_device *dev,
> >> - struct intel_ringbuffer *ringbuf)
> >> +static int intel_alloc_ringbuffer_obj(struct drm_device *dev,
> >> + struct intel_ringbuffer
> >> *ringbuf)
> >> {
> >> struct drm_i915_gem_object *obj;
> >>
> >> @@ -2023,6 +2023,48 @@ int intel_alloc_ringbuffer_obj(struct
> >> drm_device *dev,
> >> return 0;
> >> }
> >>
> >> +struct intel_ringbuffer *
> >> +intel_engine_create_ringbuffer(struct intel_engine_cs *engine, int
> >> size)
> >> +{
> >> + struct intel_ringbuffer *ring;
> >> + int ret;
> >> +
> >> + ring = kzalloc(sizeof(*ring), GFP_KERNEL);
> >> + if (ring == NULL)
> >
> > The error message referenced above should probably be here.
> >
> >
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + ring->ring = engine;
> >> +
> >> + ring->size = size;
> >> + /* Workaround an erratum on the i830 which causes a hang if
> >> + * the TAIL pointer points to within the last 2 cachelines
> >> + * of the buffer.
> >> + */
> >> + ring->effective_size = size;
> >> + if (IS_I830(engine->dev) || IS_845G(engine->dev))
> >> + ring->effective_size -= 2 * CACHELINE_BYTES;
> >> +
> >> + ring->last_retired_head = -1;
> >> + intel_ring_update_space(ring);
> >> +
> >> + ret = intel_alloc_ringbuffer_obj(engine->dev, ring);
> >> + if (ret) {
> >> + DRM_ERROR("Failed to allocate ringbuffer %s: %d\n",
> >
> > Shouldn't this be "Failed to allocate ringbuffer obj %s: %d" ?
> >
> >> + engine->name, ret);
> >> + kfree(ring);
> >> + return ERR_PTR(ret);
> >> + }
> >> +
> >> + return ring;
> >> +}
> >> +
> >> +void
> >> +intel_ringbuffer_free(struct intel_ringbuffer *ring)
> >> +{
> >> + intel_destroy_ringbuffer_obj(ring);
> >> + kfree(ring);
> >> +}
> >> +
> >> static int intel_init_ring_buffer(struct drm_device *dev,
> >> struct intel_engine_cs *ring)
> >> {
> >> @@ -2031,22 +2073,20 @@ static int intel_init_ring_buffer(struct
> >> drm_device *dev,
> >>
> >> WARN_ON(ring->buffer);
> >>
> >> - ringbuf = kzalloc(sizeof(*ringbuf), GFP_KERNEL);
> >> - if (!ringbuf)
> >> - return -ENOMEM;
> >> - ring->buffer = ringbuf;
> >> -
> >> ring->dev = dev;
> >> INIT_LIST_HEAD(&ring->active_list);
> >> INIT_LIST_HEAD(&ring->request_list);
> >> INIT_LIST_HEAD(&ring->execlist_queue);
> >> i915_gem_batch_pool_init(dev, &ring->batch_pool);
> >> - ringbuf->size = 32 * PAGE_SIZE;
> >> - ringbuf->ring = ring;
> >> memset(ring->semaphore.sync_seqno, 0, sizeof(ring
> >> ->semaphore.sync_seqno));
> >>
> >> init_waitqueue_head(&ring->irq_queue);
> >>
> >> + ringbuf = intel_engine_create_ringbuffer(ring, 32 *
> >> PAGE_SIZE);
> >> + if (IS_ERR(ringbuf))
> >> + return PTR_ERR(ringbuf);
> >> + ring->buffer = ringbuf;
> >> +
> >> if (I915_NEED_GFX_HWS(dev)) {
> >> ret = init_status_page(ring);
> >> if (ret)
> >> @@ -2058,15 +2098,6 @@ static int intel_init_ring_buffer(struct
> >> drm_device *dev,
> >> goto error;
> >> }
> >>
> >> - WARN_ON(ringbuf->obj);
> >> -
> >> - ret = intel_alloc_ringbuffer_obj(dev, ringbuf);
> >> - if (ret) {
> >> - DRM_ERROR("Failed to allocate ringbuffer %s: %d\n",
> >> - ring->name, ret);
> >> - goto error;
> >> - }
> >> -
> >> ret = intel_pin_and_map_ringbuffer_obj(dev, ringbuf);
> >> if (ret) {
> >> DRM_ERROR("Failed to pin and map ringbuffer %s:
> >> %d\n",
> >> @@ -2075,14 +2106,6 @@ static int intel_init_ring_buffer(struct
> >> drm_device *dev,
> >> goto error;
> >> }
> >>
> >> - /* Workaround an erratum on the i830 which causes a hang if
> >> - * the TAIL pointer points to within the last 2 cachelines
> >> - * of the buffer.
> >> - */
> >> - ringbuf->effective_size = ringbuf->size;
> >> - if (IS_I830(dev) || IS_845G(dev))
> >> - ringbuf->effective_size -= 2 * CACHELINE_BYTES;
> >> -
> >> ret = i915_cmd_parser_init_ring(ring);
> >> if (ret)
> >> goto error;
> >> @@ -2090,7 +2113,7 @@ static int intel_init_ring_buffer(struct
> >> drm_device *dev,
> >> return 0;
> >
> > The "goto error" right above the return 0 changes its behavior: it
> > wasn't calling intel_destroy_ringbuffer_obj(), but I suppose this is a
> > fix. I'm just pointing it since I'm not 100% sure.
> >
>
> I would say its a fix because we lost the ref at that point.
>
> Reviewed-by: Mika Kuoppala <mika.kuoppala@xxxxxxxxx>
>
> > Everything seems correct, so with or without changes:
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
Queued for -next, thanks for the patch.
-Daniel
> >
> >
> >>
> >> error:
> >> - kfree(ringbuf);
> >> + intel_ringbuffer_free(ringbuf);
> >> ring->buffer = NULL;
> >> return ret;
> >> }
> >> @@ -2098,19 +2121,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);
> >> - ringbuf = ring->buffer;
> >>
> >> intel_stop_ring_buffer(ring);
> >> WARN_ON(!IS_GEN2(ring->dev) && (I915_READ_MODE(ring) &
> >> MODE_IDLE) == 0);
> >>
> >> - intel_unpin_ringbuffer_obj(ringbuf);
> >> - intel_destroy_ringbuffer_obj(ringbuf);
> >> + intel_unpin_ringbuffer_obj(ring->buffer);
> >> + intel_ringbuffer_free(ring->buffer);
> >> + ring->buffer = NULL;
> >>
> >> if (ring->cleanup)
> >> ring->cleanup(ring);
> >> @@ -2119,9 +2141,6 @@ void intel_cleanup_ring_buffer(struct
> >> intel_engine_cs *ring)
> >>
> >> i915_cmd_parser_fini_ring(ring);
> >> i915_gem_batch_pool_fini(&ring->batch_pool);
> >> -
> >> - kfree(ringbuf);
> >> - ring->buffer = NULL;
> >> }
> >>
> >> static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
> >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> index 95b0b4b55fa6..49fa41dc0eb6 100644
> >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> >> @@ -420,12 +420,12 @@ intel_write_status_page(struct intel_engine_cs
> >> *ring,
> >> #define I915_GEM_HWS_SCRATCH_INDEX 0x40
> >> #define I915_GEM_HWS_SCRATCH_ADDR (I915_GEM_HWS_SCRATCH_INDEX <<
> >> MI_STORE_DWORD_INDEX_SHIFT)
> >>
> >> -void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf);
> >> +struct intel_ringbuffer *
> >> +intel_engine_create_ringbuffer(struct intel_engine_cs *engine, int
> >> size);
> >> int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
> >> struct intel_ringbuffer
> >> *ringbuf);
> >> -void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf);
> >> -int intel_alloc_ringbuffer_obj(struct drm_device *dev,
> >> - struct intel_ringbuffer *ringbuf);
> >> +void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf);
> >> +void intel_ringbuffer_free(struct intel_ringbuffer *ring);
> >>
> >> void intel_stop_ring_buffer(struct intel_engine_cs *ring);
> >> void intel_cleanup_ring_buffer(struct intel_engine_cs *ring);
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
