| Message ID | 1441281700-17814-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@chris-wilson.co.uk> > Cc: Arun Siluvery <arun.siluvery@linux.intel.com> > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > Cc: Dave Gordon <david.s.gordon@intel.com> > --- > 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. Everything seems correct, so with or without changes: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > 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);
On Thu, Sep 03, 2015 at 02:01:51PM +0000, Zanoni, Paulo R wrote: > 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@chris-wilson.co.uk> > > Cc: Arun Siluvery <arun.siluvery@linux.intel.com> > > Cc: Mika Kuoppala <mika.kuoppala@intel.com> > > Cc: Dave Gordon <david.s.gordon@intel.com> > > --- > > 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. I additionally added these to patch 2. I removed the ones in intel_lrc that were simply repeating the error message (albeit in a more generic fashion due to loss of information). At this level an oom squalk followed by ENOMEM reported to userspace is fairly obvious (and definitely not our error). -Chris
"Zanoni, Paulo R" <paulo.r.zanoni@intel.com> 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@chris-wilson.co.uk> >> Cc: Arun Siluvery <arun.siluvery@linux.intel.com> >> Cc: Mika Kuoppala <mika.kuoppala@intel.com> >> Cc: Dave Gordon <david.s.gordon@intel.com> >> --- >> 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@intel.com> > Everything seems correct, so with or without changes: > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > >> >> 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);
On Thu, Sep 03, 2015 at 05:56:20PM +0300, Mika Kuoppala wrote: > "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> 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@chris-wilson.co.uk> > >> Cc: Arun Siluvery <arun.siluvery@linux.intel.com> > >> Cc: Mika Kuoppala <mika.kuoppala@intel.com> > >> Cc: Dave Gordon <david.s.gordon@intel.com> > >> --- > >> 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@intel.com> > > > Everything seems correct, so with or without changes: > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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", - 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) + 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", + 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; 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);
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@chris-wilson.co.uk> Cc: Arun Siluvery <arun.siluvery@linux.intel.com> Cc: Mika Kuoppala <mika.kuoppala@intel.com> Cc: Dave Gordon <david.s.gordon@intel.com> --- 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(-)