| Message ID | 1450291011-31486-2-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Reviewed-by: Nick Hoath <nicholas.hoath@intel.com> On 16/12/2015 18:36, Gordon, David S wrote: > We set up engines in forwards order, so some things (notably the > default context) are "owned" by engine 0 (the render engine, aka "RCS"). > For symmetry and to make sure such shared objects don't disappear too > early, we should generally run teardown loops in the reverse order, > so that engine 0 is processed last. > > This patch changes i915_gem_context_fini() to do that, and clarifies the > comments in i915_gem_context_{init,fini}() about the refcounting of the > default {struct intel_)context: the refcount is just ONE, no matter how > many rings exist or are active, and this refcount is nominally ascribed > to the render ring (RCS), which is set up first and now torn down last. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 900ffd0..e143ea5 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -391,7 +391,13 @@ int i915_gem_context_init(struct drm_device *dev) > for (i = 0; i < I915_NUM_RINGS; i++) { > struct intel_engine_cs *ring = &dev_priv->ring[i]; > > - /* NB: RCS will hold a ref for all rings */ > + /* > + * Although each engine has a pointer to the global default > + * context, they don't contribute to the refcount on the > + * context. We consider that RCS (which is set up first and > + * torn down last) holds this reference on behalf of all the > + * other engines > + */ > ring->default_context = ctx; > } > > @@ -431,14 +437,21 @@ void i915_gem_context_fini(struct drm_device *dev) > i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state); > } > > - for (i = 0; i < I915_NUM_RINGS; i++) { > + for (i = I915_NUM_RINGS; --i >= 0;) { > struct intel_engine_cs *ring = &dev_priv->ring[i]; > > - if (ring->last_context) > + if (ring->last_context) { > i915_gem_context_unreference(ring->last_context); > + ring->last_context = NULL; > + } > > + /* > + * These default_context pointers don't contribute to the > + * refcount on the context. We consider that RCS holds its > + * reference on behalf of all the other engines, so there's > + * just a single unreference() call below. > + */ > ring->default_context = NULL; > - ring->last_context = NULL; > } > > i915_gem_context_unreference(dctx); >
On Wed, Dec 16, 2015 at 06:36:48PM +0000, Dave Gordon wrote: > We set up engines in forwards order, so some things (notably the > default context) are "owned" by engine 0 (the render engine, aka "RCS"). > For symmetry and to make sure such shared objects don't disappear too > early, we should generally run teardown loops in the reverse order, > so that engine 0 is processed last. > > This patch changes i915_gem_context_fini() to do that, and clarifies the > comments in i915_gem_context_{init,fini}() about the refcounting of the > default {struct intel_)context: the refcount is just ONE, no matter how > many rings exist or are active, and this refcount is nominally ascribed > to the render ring (RCS), which is set up first and now torn down last. > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_context.c | 21 +++++++++++++++++---- > 1 file changed, 17 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index 900ffd0..e143ea5 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -391,7 +391,13 @@ int i915_gem_context_init(struct drm_device *dev) > for (i = 0; i < I915_NUM_RINGS; i++) { > struct intel_engine_cs *ring = &dev_priv->ring[i]; > > - /* NB: RCS will hold a ref for all rings */ > + /* > + * Although each engine has a pointer to the global default > + * context, they don't contribute to the refcount on the > + * context. We consider that RCS (which is set up first and > + * torn down last) holds this reference on behalf of all the > + * other engines > + */ Instead of piles of comments, can't we just reference-count this pointer properly? Pointers to reference-counted objects which don't hold a full reference are just fraught with peril, and doing that should imo only be done when there's really clear performance data justifying the atomic_inc/dec overhead. Init/teardown code isn't such a place. This misdesign goes back to the original execlist merge, which expanded the default context from RCS to all engines. Or do I miss something and we can't do this? Thanks, Daniel > ring->default_context = ctx; > } > > @@ -431,14 +437,21 @@ void i915_gem_context_fini(struct drm_device *dev) > i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state); > } > > - for (i = 0; i < I915_NUM_RINGS; i++) { > + for (i = I915_NUM_RINGS; --i >= 0;) { > struct intel_engine_cs *ring = &dev_priv->ring[i]; > > - if (ring->last_context) > + if (ring->last_context) { > i915_gem_context_unreference(ring->last_context); > + ring->last_context = NULL; > + } > > + /* > + * These default_context pointers don't contribute to the > + * refcount on the context. We consider that RCS holds its > + * reference on behalf of all the other engines, so there's > + * just a single unreference() call below. > + */ > ring->default_context = NULL; > - ring->last_context = NULL; > } > > i915_gem_context_unreference(dctx); > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Dec 21, 2015 at 11:48:35AM +0100, Daniel Vetter wrote: > On Wed, Dec 16, 2015 at 06:36:48PM +0000, Dave Gordon wrote: > > We set up engines in forwards order, so some things (notably the > > default context) are "owned" by engine 0 (the render engine, aka "RCS"). > > For symmetry and to make sure such shared objects don't disappear too > > early, we should generally run teardown loops in the reverse order, > > so that engine 0 is processed last. > > > > This patch changes i915_gem_context_fini() to do that, and clarifies the > > comments in i915_gem_context_{init,fini}() about the refcounting of the > > default {struct intel_)context: the refcount is just ONE, no matter how > > many rings exist or are active, and this refcount is nominally ascribed > > to the render ring (RCS), which is set up first and now torn down last. > > > > Signed-off-by: Dave Gordon <david.s.gordon@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem_context.c | 21 +++++++++++++++++---- > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > > index 900ffd0..e143ea5 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > @@ -391,7 +391,13 @@ int i915_gem_context_init(struct drm_device *dev) > > for (i = 0; i < I915_NUM_RINGS; i++) { > > struct intel_engine_cs *ring = &dev_priv->ring[i]; > > > > - /* NB: RCS will hold a ref for all rings */ > > + /* > > + * Although each engine has a pointer to the global default > > + * context, they don't contribute to the refcount on the > > + * context. We consider that RCS (which is set up first and > > + * torn down last) holds this reference on behalf of all the > > + * other engines > > + */ > > Instead of piles of comments, can't we just reference-count this pointer > properly? Pointers to reference-counted objects which don't hold a full > reference are just fraught with peril, and doing that should imo only be > done when there's really clear performance data justifying the > atomic_inc/dec overhead. Init/teardown code isn't such a place. > > This misdesign goes back to the original execlist merge, which expanded > the default context from RCS to all engines. > > Or do I miss something and we can't do this? We can. It's actually even easier to just do dev_priv->kernel_context. -Chris
On 21/12/15 11:01, Chris Wilson wrote: > On Mon, Dec 21, 2015 at 11:48:35AM +0100, Daniel Vetter wrote: >> On Wed, Dec 16, 2015 at 06:36:48PM +0000, Dave Gordon wrote: >>> We set up engines in forwards order, so some things (notably the >>> default context) are "owned" by engine 0 (the render engine, aka "RCS"). >>> For symmetry and to make sure such shared objects don't disappear too >>> early, we should generally run teardown loops in the reverse order, >>> so that engine 0 is processed last. >>> >>> This patch changes i915_gem_context_fini() to do that, and clarifies the >>> comments in i915_gem_context_{init,fini}() about the refcounting of the >>> default {struct intel_)context: the refcount is just ONE, no matter how >>> many rings exist or are active, and this refcount is nominally ascribed >>> to the render ring (RCS), which is set up first and now torn down last. >>> >>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com> >>> --- >>> drivers/gpu/drm/i915/i915_gem_context.c | 21 +++++++++++++++++---- >>> 1 file changed, 17 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c >>> index 900ffd0..e143ea5 100644 >>> --- a/drivers/gpu/drm/i915/i915_gem_context.c >>> +++ b/drivers/gpu/drm/i915/i915_gem_context.c >>> @@ -391,7 +391,13 @@ int i915_gem_context_init(struct drm_device *dev) >>> for (i = 0; i < I915_NUM_RINGS; i++) { >>> struct intel_engine_cs *ring = &dev_priv->ring[i]; >>> >>> - /* NB: RCS will hold a ref for all rings */ >>> + /* >>> + * Although each engine has a pointer to the global default >>> + * context, they don't contribute to the refcount on the >>> + * context. We consider that RCS (which is set up first and >>> + * torn down last) holds this reference on behalf of all the >>> + * other engines >>> + */ >> >> Instead of piles of comments, can't we just reference-count this pointer >> properly? Pointers to reference-counted objects which don't hold a full >> reference are just fraught with peril, and doing that should imo only be >> done when there's really clear performance data justifying the >> atomic_inc/dec overhead. Init/teardown code isn't such a place. >> >> This misdesign goes back to the original execlist merge, which expanded >> the default context from RCS to all engines. >> >> Or do I miss something and we can't do this? > > We can. It's actually even easier to just do dev_priv->kernel_context. > -Chris I'm sure it can be done, but Nick & I tried changing this section so that each engine held a reference, and it broke something else (because the last unreference then occured at a different point), hence my comment about the "the delicate and fragile load/unload dance". This is why we're trying to tidy up and clarify, bit by bit, without breaking too many of the existing assumptions at once. But ... I like Chris' idea of moving the pointer to the default context from the engine structure to dev_priv; that will simplify several of the complicated ...->engine[RCS].default_context constructs, and mean that we're change the pointers to match the lifecycle rather than vice versa. I'll try that ... .Dave.
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 900ffd0..e143ea5 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -391,7 +391,13 @@ int i915_gem_context_init(struct drm_device *dev) for (i = 0; i < I915_NUM_RINGS; i++) { struct intel_engine_cs *ring = &dev_priv->ring[i]; - /* NB: RCS will hold a ref for all rings */ + /* + * Although each engine has a pointer to the global default + * context, they don't contribute to the refcount on the + * context. We consider that RCS (which is set up first and + * torn down last) holds this reference on behalf of all the + * other engines + */ ring->default_context = ctx; } @@ -431,14 +437,21 @@ void i915_gem_context_fini(struct drm_device *dev) i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state); } - for (i = 0; i < I915_NUM_RINGS; i++) { + for (i = I915_NUM_RINGS; --i >= 0;) { struct intel_engine_cs *ring = &dev_priv->ring[i]; - if (ring->last_context) + if (ring->last_context) { i915_gem_context_unreference(ring->last_context); + ring->last_context = NULL; + } + /* + * These default_context pointers don't contribute to the + * refcount on the context. We consider that RCS holds its + * reference on behalf of all the other engines, so there's + * just a single unreference() call below. + */ ring->default_context = NULL; - ring->last_context = NULL; } i915_gem_context_unreference(dctx);
We set up engines in forwards order, so some things (notably the default context) are "owned" by engine 0 (the render engine, aka "RCS"). For symmetry and to make sure such shared objects don't disappear too early, we should generally run teardown loops in the reverse order, so that engine 0 is processed last. This patch changes i915_gem_context_fini() to do that, and clarifies the comments in i915_gem_context_{init,fini}() about the refcounting of the default {struct intel_)context: the refcount is just ONE, no matter how many rings exist or are active, and this refcount is nominally ascribed to the render ring (RCS), which is set up first and now torn down last. Signed-off-by: Dave Gordon <david.s.gordon@intel.com> --- drivers/gpu/drm/i915/i915_gem_context.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)