| Message ID | 1395943218-7708-32-git-send-email-oscar.mateo@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I already got a fair review comment from Brad Volkin on this: he proposes to do this instead
struct i915_hw_context {
struct i915_address_space *vm;
struct {
struct drm_i915_gem_object *ctx_obj;
struct intel_ringbuffer *ringbuf;
} engine[I915_MAX_RINGS];
...
};
This is: instead of creating extra contexts with the same Context ID, modify the current i915_hw_context to work with all engines. I agree this alternative looks less *hackish*, but I want to get eyes on it (several things need careful consideration if we do this, e.g.: should the hang_stats also be per-engine?)
> -----Original Message-----
> From: Mateo Lozano, Oscar
> Sent: Thursday, March 27, 2014 6:00 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Mateo Lozano, Oscar
> Subject: [PATCH 31/49] drm/i915/bdw: Introduce dependent contexts
>
> From: Oscar Mateo <oscar.mateo@intel.com>
>
> From here on, we define a stand-alone context as the first context with a given
> ID to be created for a new fd or a new context create ioctl. This is the one we
> can easily find using integer ID management. On the other hand, dependent
> contexts are subsequently created with the same ID and simply hang from the
> stand-alone one.
>
> This patch, together with the two previous and the next, are meant to solve a
> big problem we have: with execlists, we need contexts to work with all engines,
> and we cannot reuse one context for more than one engine.
>
> Because, on a new fd or a context create ioctl, we really don't know which
> engine is going to be used later on, we are going to create at that point a
> "blank" context and assign it to an engine on a deferred way (during the
> execbuffer, to be precise). If later on, we execbuffer on a different engine, we
> create a new dependent context on the previous.
>
> Note: I have tried to colour this patch in a different way, using a different struct
> (a "context group") to hold the context ID from where the per-engine contexts
> hang, but it makes legacy contexts unnecessary complex.
>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 6 +++++-
> drivers/gpu/drm/i915/i915_gem_context.c | 17 +++++++++++++--
> drivers/gpu/drm/i915/i915_lrc.c | 37
> ++++++++++++++++++++++++++++++---
> 3 files changed, 54 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h index 91b0886..d9470a4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -602,6 +602,9 @@ struct i915_hw_context {
> struct i915_address_space *vm;
>
> struct list_head link;
> +
> + /* Advanced contexts only */
> + struct list_head dependent_contexts;
> };
>
> struct i915_fbc {
> @@ -2321,7 +2324,8 @@ int gen8_gem_context_init(struct drm_device *dev);
> void gen8_gem_context_fini(struct drm_device *dev); struct i915_hw_context
> *gen8_gem_create_context(struct drm_device *dev,
> struct intel_engine *ring,
> - struct drm_i915_file_private *file_priv, bool
> create_vm);
> + struct drm_i915_file_private *file_priv,
> + struct i915_hw_context *standalone_ctx, bool
> create_vm);
> void gen8_gem_context_free(struct i915_hw_context *ctx);
>
> /* i915_gem_evict.c */
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> b/drivers/gpu/drm/i915/i915_gem_context.c
> index 6baa5ab..17015b2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -271,6 +271,8 @@ __create_hw_context(struct drm_device *dev,
> * is no remap info, it will be a NOP. */
> ctx->remap_slice = (1 << NUM_L3_SLICES(dev)) - 1;
>
> + INIT_LIST_HEAD(&ctx->dependent_contexts);
> +
> return ctx;
>
> err_out:
> @@ -511,6 +513,12 @@ int i915_gem_context_enable(struct
> drm_i915_private *dev_priv) static int context_idr_cleanup(int id, void *p, void
> *data) {
> struct i915_hw_context *ctx = p;
> + struct i915_hw_context *cursor, *tmp;
> +
> + list_for_each_entry_safe(cursor, tmp, &ctx->dependent_contexts,
> dependent_contexts) {
> + list_del(&cursor->dependent_contexts);
> + i915_gem_context_unreference(cursor);
> + }
>
> /* Ignore the default context because close will handle it */
> if (i915_gem_context_is_default(ctx))
> @@ -543,7 +551,7 @@ int i915_gem_context_open(struct drm_device *dev,
> struct drm_file *file)
> if (dev_priv->lrc_enabled)
> file_priv->private_default_ctx =
> gen8_gem_create_context(dev,
> &dev_priv->ring[RCS],
> file_priv,
> - USES_FULL_PPGTT(dev));
> + NULL,
> USES_FULL_PPGTT(dev));
> else
> file_priv->private_default_ctx = i915_gem_create_context(dev,
> file_priv,
> USES_FULL_PPGTT(dev)); @@ -805,7 +813,7 @@ int
> i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
>
> if (dev_priv->lrc_enabled)
> ctx = gen8_gem_create_context(dev, &dev_priv->ring[RCS],
> - file_priv, USES_FULL_PPGTT(dev));
> + file_priv, NULL,
> USES_FULL_PPGTT(dev));
> else
> ctx = i915_gem_create_context(dev, file_priv,
> USES_FULL_PPGTT(dev));
> @@ -825,6 +833,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device
> *dev, void *data,
> struct drm_i915_gem_context_destroy *args = data;
> struct drm_i915_file_private *file_priv = file->driver_priv;
> struct i915_hw_context *ctx;
> + struct i915_hw_context *cursor, *tmp;
> int ret;
>
> if (args->ctx_id == DEFAULT_CONTEXT_ID) @@ -841,6 +850,10 @@ int
> i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> }
>
> idr_remove(&ctx->file_priv->context_idr, ctx->id);
> + list_for_each_entry_safe(cursor, tmp, &ctx->dependent_contexts,
> dependent_contexts) {
> + list_del(&cursor->dependent_contexts);
> + i915_gem_context_unreference(cursor);
> + }
> i915_gem_context_unreference(ctx);
> mutex_unlock(&dev->struct_mutex);
>
> diff --git a/drivers/gpu/drm/i915/i915_lrc.c b/drivers/gpu/drm/i915/i915_lrc.c
> index 124e5f2..99011cc 100644
> --- a/drivers/gpu/drm/i915/i915_lrc.c
> +++ b/drivers/gpu/drm/i915/i915_lrc.c
> @@ -195,23 +195,54 @@ intel_populate_lrc(struct i915_hw_context *ctx,
> return 0;
> }
>
> +static void assert_on_ppgtt_release(struct kref *kref) {
> + WARN(1, "Are we trying to free the aliasing PPGTT?\n"); }
> +
> struct i915_hw_context *
> gen8_gem_create_context(struct drm_device *dev,
> struct intel_engine *ring,
> struct drm_i915_file_private *file_priv,
> + struct i915_hw_context *standalone_ctx,
> bool create_vm)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct i915_hw_context *ctx = NULL;
> struct drm_i915_gem_object *ring_obj = NULL;
> struct intel_ringbuffer *ringbuf = NULL;
> + bool is_dependent;
> int ret;
>
> - ctx = i915_gem_create_context(dev, file_priv, create_vm);
> + /* NB: a standalone context is the first context with a given id to be
> + * created for a new fd. Dependent contexts simply hang from the
> stand-alone,
> + * sharing their ID and their PPGTT */
> + is_dependent = (file_priv != NULL) && (standalone_ctx != NULL);
> +
> + ctx = i915_gem_create_context(dev, is_dependent? NULL : file_priv,
> + is_dependent? false : create_vm);
> if (IS_ERR_OR_NULL(ctx))
> return ctx;
>
> - if (file_priv) {
> + if (is_dependent) {
> + struct i915_hw_ppgtt *ppgtt;
> +
> + /* We take the same PPGTT as the standalone */
> + ppgtt = ctx_to_ppgtt(ctx);
> + kref_put(&ppgtt->ref, assert_on_ppgtt_release);
> + ppgtt = ctx_to_ppgtt(standalone_ctx);
> + ctx->vm = &ppgtt->base;
> + kref_get(&ppgtt->ref);
> +
> + ctx->file_priv = file_priv;
> + ctx->id = standalone_ctx->id;
> + ctx->remap_slice = standalone_ctx->remap_slice;
> +
> + list_add_tail(&ctx->dependent_contexts,
> + &standalone_ctx->dependent_contexts);
> + }
> +
> + if (file_priv && !is_dependent) {
> ret = i915_gem_obj_ggtt_pin(ctx->obj, GEN8_CONTEXT_ALIGN,
> 0);
> if (ret) {
> DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret); @@ -
> 337,7 +368,7 @@ int gen8_gem_context_init(struct drm_device *dev)
>
> for_each_ring(ring, dev_priv, ring_id) {
> ring->default_context = gen8_gem_create_context(dev, ring,
> - NULL, (ring_id == RCS));
> + NULL, NULL, (ring_id == RCS));
> if (IS_ERR_OR_NULL(ring->default_context)) {
> ret = PTR_ERR(ring->default_context);
> DRM_DEBUG_DRIVER("Create ctx failed: %d\n", ret);
> --
> 1.9.0
Hey Damien, > I already got a fair review comment from Brad Volkin on this: he proposes to > do this instead > > struct i915_hw_context { > struct i915_address_space *vm; > struct { > struct drm_i915_gem_object *ctx_obj; > struct intel_ringbuffer *ringbuf; > } engine[I915_MAX_RINGS]; > ... > }; > > This is: instead of creating extra contexts with the same Context ID, modify the > current i915_hw_context to work with all engines. I agree this alternative looks > less *hackish*, but I want to get eyes on it (several things need careful > consideration if we do this, e.g.: should the hang_stats also be per-engine?) After looking at the execlists code, does this also make sense to you? Cheers, Oscar
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 91b0886..d9470a4 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -602,6 +602,9 @@ struct i915_hw_context { struct i915_address_space *vm; struct list_head link; + + /* Advanced contexts only */ + struct list_head dependent_contexts; }; struct i915_fbc { @@ -2321,7 +2324,8 @@ int gen8_gem_context_init(struct drm_device *dev); void gen8_gem_context_fini(struct drm_device *dev); struct i915_hw_context *gen8_gem_create_context(struct drm_device *dev, struct intel_engine *ring, - struct drm_i915_file_private *file_priv, bool create_vm); + struct drm_i915_file_private *file_priv, + struct i915_hw_context *standalone_ctx, bool create_vm); void gen8_gem_context_free(struct i915_hw_context *ctx); /* i915_gem_evict.c */ diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 6baa5ab..17015b2 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -271,6 +271,8 @@ __create_hw_context(struct drm_device *dev, * is no remap info, it will be a NOP. */ ctx->remap_slice = (1 << NUM_L3_SLICES(dev)) - 1; + INIT_LIST_HEAD(&ctx->dependent_contexts); + return ctx; err_out: @@ -511,6 +513,12 @@ int i915_gem_context_enable(struct drm_i915_private *dev_priv) static int context_idr_cleanup(int id, void *p, void *data) { struct i915_hw_context *ctx = p; + struct i915_hw_context *cursor, *tmp; + + list_for_each_entry_safe(cursor, tmp, &ctx->dependent_contexts, dependent_contexts) { + list_del(&cursor->dependent_contexts); + i915_gem_context_unreference(cursor); + } /* Ignore the default context because close will handle it */ if (i915_gem_context_is_default(ctx)) @@ -543,7 +551,7 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file) if (dev_priv->lrc_enabled) file_priv->private_default_ctx = gen8_gem_create_context(dev, &dev_priv->ring[RCS], file_priv, - USES_FULL_PPGTT(dev)); + NULL, USES_FULL_PPGTT(dev)); else file_priv->private_default_ctx = i915_gem_create_context(dev, file_priv, USES_FULL_PPGTT(dev)); @@ -805,7 +813,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data, if (dev_priv->lrc_enabled) ctx = gen8_gem_create_context(dev, &dev_priv->ring[RCS], - file_priv, USES_FULL_PPGTT(dev)); + file_priv, NULL, USES_FULL_PPGTT(dev)); else ctx = i915_gem_create_context(dev, file_priv, USES_FULL_PPGTT(dev)); @@ -825,6 +833,7 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data, struct drm_i915_gem_context_destroy *args = data; struct drm_i915_file_private *file_priv = file->driver_priv; struct i915_hw_context *ctx; + struct i915_hw_context *cursor, *tmp; int ret; if (args->ctx_id == DEFAULT_CONTEXT_ID) @@ -841,6 +850,10 @@ int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data, } idr_remove(&ctx->file_priv->context_idr, ctx->id); + list_for_each_entry_safe(cursor, tmp, &ctx->dependent_contexts, dependent_contexts) { + list_del(&cursor->dependent_contexts); + i915_gem_context_unreference(cursor); + } i915_gem_context_unreference(ctx); mutex_unlock(&dev->struct_mutex); diff --git a/drivers/gpu/drm/i915/i915_lrc.c b/drivers/gpu/drm/i915/i915_lrc.c index 124e5f2..99011cc 100644 --- a/drivers/gpu/drm/i915/i915_lrc.c +++ b/drivers/gpu/drm/i915/i915_lrc.c @@ -195,23 +195,54 @@ intel_populate_lrc(struct i915_hw_context *ctx, return 0; } +static void assert_on_ppgtt_release(struct kref *kref) +{ + WARN(1, "Are we trying to free the aliasing PPGTT?\n"); +} + struct i915_hw_context * gen8_gem_create_context(struct drm_device *dev, struct intel_engine *ring, struct drm_i915_file_private *file_priv, + struct i915_hw_context *standalone_ctx, bool create_vm) { struct drm_i915_private *dev_priv = dev->dev_private; struct i915_hw_context *ctx = NULL; struct drm_i915_gem_object *ring_obj = NULL; struct intel_ringbuffer *ringbuf = NULL; + bool is_dependent; int ret; - ctx = i915_gem_create_context(dev, file_priv, create_vm); + /* NB: a standalone context is the first context with a given id to be + * created for a new fd. Dependent contexts simply hang from the stand-alone, + * sharing their ID and their PPGTT */ + is_dependent = (file_priv != NULL) && (standalone_ctx != NULL); + + ctx = i915_gem_create_context(dev, is_dependent? NULL : file_priv, + is_dependent? false : create_vm); if (IS_ERR_OR_NULL(ctx)) return ctx; - if (file_priv) { + if (is_dependent) { + struct i915_hw_ppgtt *ppgtt; + + /* We take the same PPGTT as the standalone */ + ppgtt = ctx_to_ppgtt(ctx); + kref_put(&ppgtt->ref, assert_on_ppgtt_release); + ppgtt = ctx_to_ppgtt(standalone_ctx); + ctx->vm = &ppgtt->base; + kref_get(&ppgtt->ref); + + ctx->file_priv = file_priv; + ctx->id = standalone_ctx->id; + ctx->remap_slice = standalone_ctx->remap_slice; + + list_add_tail(&ctx->dependent_contexts, + &standalone_ctx->dependent_contexts); + } + + if (file_priv && !is_dependent) { ret = i915_gem_obj_ggtt_pin(ctx->obj, GEN8_CONTEXT_ALIGN, 0); if (ret) { DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret); @@ -337,7 +368,7 @@ int gen8_gem_context_init(struct drm_device *dev) for_each_ring(ring, dev_priv, ring_id) { ring->default_context = gen8_gem_create_context(dev, ring, - NULL, (ring_id == RCS)); + NULL, NULL, (ring_id == RCS)); if (IS_ERR_OR_NULL(ring->default_context)) { ret = PTR_ERR(ring->default_context); DRM_DEBUG_DRIVER("Create ctx failed: %d\n", ret);