| Message ID | 1402673891-14618-12-git-send-email-oscar.mateo@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jun 13, 2014 at 08:37:29AM -0700, oscar.mateo@intel.com wrote: > From: Oscar Mateo <oscar.mateo@intel.com> > > As we have said a couple of times by now, logical ring contexts have > their own ringbuffers: not only the backing pages, but the whole > management struct. > > In a previous version of the series, this was achieved with two separate > patches: > drm/i915/bdw: Allocate ringbuffer backing objects for default global LRC > drm/i915/bdw: Allocate ringbuffer for user-created LRCs > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_lrc.c | 38 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 39 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 347308e..79799d8 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -599,6 +599,7 @@ struct intel_context { > /* Execlists */ > struct { > struct drm_i915_gem_object *obj; > + struct intel_ringbuffer *ringbuf; > } engine[I915_NUM_RINGS]; > > struct i915_ctx_hang_stats hang_stats; > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index 952212f..b3a23e0 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -60,7 +60,11 @@ void intel_lr_context_free(struct intel_context *ctx) > > for (i = 0; i < I915_NUM_RINGS; i++) { > struct drm_i915_gem_object *ctx_obj = ctx->engine[i].obj; > + struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf; > + > if (ctx_obj) { > + intel_destroy_ring_buffer(ringbuf); > + kfree(ringbuf); > i915_gem_object_ggtt_unpin(ctx_obj); > drm_gem_object_unreference(&ctx_obj->base); > } > @@ -94,6 +98,7 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, > struct drm_device *dev = ring->dev; > struct drm_i915_gem_object *ctx_obj; > uint32_t context_size; > + struct intel_ringbuffer *ringbuf; > int ret; > > WARN_ON(ctx->render_obj != NULL); > @@ -114,6 +119,39 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, > return ret; > } > > + ringbuf = kzalloc(sizeof(*ringbuf), GFP_KERNEL); > + if (!ringbuf) { > + DRM_DEBUG_DRIVER("Failed to allocate ringbuffer %s\n", > + ring->name); > + i915_gem_object_ggtt_unpin(ctx_obj); > + drm_gem_object_unreference(&ctx_obj->base); > + ret = -ENOMEM; > + return ret; > + } > + > + ringbuf->size = 32 * PAGE_SIZE; > + ringbuf->effective_size = ringbuf->size; > + ringbuf->head = 0; > + ringbuf->tail = 0; > + ringbuf->space = ringbuf->size; > + ringbuf->last_retired_head = -1; > + > + /* TODO: For now we put this in the mappable region so that we can reuse > + * the existing ringbuffer code which ioremaps it. When we start > + * creating many contexts, this will no longer work and we must switch > + * to a kmapish interface. > + */ It looks like this comment still exists at the end of the series. Does it still apply or did we find that this is not an issue? Brad > + ret = intel_allocate_ring_buffer(dev, ringbuf); > + if (ret) { > + DRM_DEBUG_DRIVER("Failed to allocate ringbuffer obj %s: %d\n", > + ring->name, ret); > + kfree(ringbuf); > + i915_gem_object_ggtt_unpin(ctx_obj); > + drm_gem_object_unreference(&ctx_obj->base); > + return ret; > + } > + > + ctx->engine[ring->id].ringbuf = ringbuf; > ctx->engine[ring->id].obj = ctx_obj; > > return 0; > -- > 1.9.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47 > -----Original Message----- > From: Volkin, Bradley D > Sent: Wednesday, June 18, 2014 11:19 PM > To: Mateo Lozano, Oscar > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH 11/53] drm/i915/bdw: Allocate ringbuffers > for Logical Ring Contexts > > On Fri, Jun 13, 2014 at 08:37:29AM -0700, oscar.mateo@intel.com wrote: > > From: Oscar Mateo <oscar.mateo@intel.com> > > > > As we have said a couple of times by now, logical ring contexts have > > their own ringbuffers: not only the backing pages, but the whole > > management struct. > > > > In a previous version of the series, this was achieved with two > > separate > > patches: > > drm/i915/bdw: Allocate ringbuffer backing objects for default global > > LRC > > drm/i915/bdw: Allocate ringbuffer for user-created LRCs > > > > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 1 + > > drivers/gpu/drm/i915/intel_lrc.c | 38 > > ++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 39 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h index 347308e..79799d8 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -599,6 +599,7 @@ struct intel_context { > > /* Execlists */ > > struct { > > struct drm_i915_gem_object *obj; > > + struct intel_ringbuffer *ringbuf; > > } engine[I915_NUM_RINGS]; > > > > struct i915_ctx_hang_stats hang_stats; diff --git > > a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > > index 952212f..b3a23e0 100644 > > --- a/drivers/gpu/drm/i915/intel_lrc.c > > +++ b/drivers/gpu/drm/i915/intel_lrc.c > > @@ -60,7 +60,11 @@ void intel_lr_context_free(struct intel_context > > *ctx) > > > > for (i = 0; i < I915_NUM_RINGS; i++) { > > struct drm_i915_gem_object *ctx_obj = ctx->engine[i].obj; > > + struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf; > > + > > if (ctx_obj) { > > + intel_destroy_ring_buffer(ringbuf); > > + kfree(ringbuf); > > i915_gem_object_ggtt_unpin(ctx_obj); > > drm_gem_object_unreference(&ctx_obj->base); > > } > > @@ -94,6 +98,7 @@ int intel_lr_context_deferred_create(struct > intel_context *ctx, > > struct drm_device *dev = ring->dev; > > struct drm_i915_gem_object *ctx_obj; > > uint32_t context_size; > > + struct intel_ringbuffer *ringbuf; > > int ret; > > > > WARN_ON(ctx->render_obj != NULL); > > @@ -114,6 +119,39 @@ int intel_lr_context_deferred_create(struct > intel_context *ctx, > > return ret; > > } > > > > + ringbuf = kzalloc(sizeof(*ringbuf), GFP_KERNEL); > > + if (!ringbuf) { > > + DRM_DEBUG_DRIVER("Failed to allocate ringbuffer %s\n", > > + ring->name); > > + i915_gem_object_ggtt_unpin(ctx_obj); > > + drm_gem_object_unreference(&ctx_obj->base); > > + ret = -ENOMEM; > > + return ret; > > + } > > + > > + ringbuf->size = 32 * PAGE_SIZE; > > + ringbuf->effective_size = ringbuf->size; > > + ringbuf->head = 0; > > + ringbuf->tail = 0; > > + ringbuf->space = ringbuf->size; > > + ringbuf->last_retired_head = -1; > > + > > + /* TODO: For now we put this in the mappable region so that we can > reuse > > + * the existing ringbuffer code which ioremaps it. When we start > > + * creating many contexts, this will no longer work and we must > switch > > + * to a kmapish interface. > > + */ > > It looks like this comment still exists at the end of the series. Does it still > apply or did we find that this is not an issue? This is similar to the problem we have with pinning context regardless: the ringbuffers are 32 pages in size and we pin them (intel_allocate_ring_buffer), so we will end up fragmenting the GTT pretty heavily. This problem is not that bad with Full PPGTT, but of course it needs fixing. The good news is that the logical ring split allows me to tackle this without changing how old platforms work :) I´ll get to it. -- Oscar
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 347308e..79799d8 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -599,6 +599,7 @@ struct intel_context { /* Execlists */ struct { struct drm_i915_gem_object *obj; + struct intel_ringbuffer *ringbuf; } engine[I915_NUM_RINGS]; struct i915_ctx_hang_stats hang_stats; diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 952212f..b3a23e0 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -60,7 +60,11 @@ void intel_lr_context_free(struct intel_context *ctx) for (i = 0; i < I915_NUM_RINGS; i++) { struct drm_i915_gem_object *ctx_obj = ctx->engine[i].obj; + struct intel_ringbuffer *ringbuf = ctx->engine[i].ringbuf; + if (ctx_obj) { + intel_destroy_ring_buffer(ringbuf); + kfree(ringbuf); i915_gem_object_ggtt_unpin(ctx_obj); drm_gem_object_unreference(&ctx_obj->base); } @@ -94,6 +98,7 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, struct drm_device *dev = ring->dev; struct drm_i915_gem_object *ctx_obj; uint32_t context_size; + struct intel_ringbuffer *ringbuf; int ret; WARN_ON(ctx->render_obj != NULL); @@ -114,6 +119,39 @@ int intel_lr_context_deferred_create(struct intel_context *ctx, return ret; } + ringbuf = kzalloc(sizeof(*ringbuf), GFP_KERNEL); + if (!ringbuf) { + DRM_DEBUG_DRIVER("Failed to allocate ringbuffer %s\n", + ring->name); + i915_gem_object_ggtt_unpin(ctx_obj); + drm_gem_object_unreference(&ctx_obj->base); + ret = -ENOMEM; + return ret; + } + + ringbuf->size = 32 * PAGE_SIZE; + ringbuf->effective_size = ringbuf->size; + ringbuf->head = 0; + ringbuf->tail = 0; + ringbuf->space = ringbuf->size; + ringbuf->last_retired_head = -1; + + /* TODO: For now we put this in the mappable region so that we can reuse + * the existing ringbuffer code which ioremaps it. When we start + * creating many contexts, this will no longer work and we must switch + * to a kmapish interface. + */ + ret = intel_allocate_ring_buffer(dev, ringbuf); + if (ret) { + DRM_DEBUG_DRIVER("Failed to allocate ringbuffer obj %s: %d\n", + ring->name, ret); + kfree(ringbuf); + i915_gem_object_ggtt_unpin(ctx_obj); + drm_gem_object_unreference(&ctx_obj->base); + return ret; + } + + ctx->engine[ring->id].ringbuf = ringbuf; ctx->engine[ring->id].obj = ctx_obj; return 0;