Skip to content

Commit

Permalink
drm/i915/gem: Add an intermediate proto_context struct (v5)
Browse files Browse the repository at this point in the history 
The current context uAPI allows for two methods of setting context
parameters: SET_CONTEXT_PARAM and CONTEXT_CREATE_EXT_SETPARAM.  The
former is allowed to be called at any time while the later happens as
part of GEM_CONTEXT_CREATE.  Currently, everything settable via one is
settable via the other.  While some params are fairly simple and setting
them on a live context is harmless such the context priority, others are
far trickier such as the VM or the set of engines.  In order to swap out
the VM, for instance, we have to delay until all current in-flight work
is complete, swap in the new VM, and then continue.  This leads to a
plethora of potential race conditions we'd really rather avoid.

Unfortunately, both methods of setting the VM and the engine set are in
active use today so we can't simply disallow setting the VM or engine
set vial SET_CONTEXT_PARAM.  In order to work around this wart, this
commit adds a proto-context struct which contains all the context create
parameters.

v2 (Daniel Vetter):
 - Better commit message
 - Use __set/clear_bit instead of set/clear_bit because there's no race
   and we don't need the atomics

v3 (Daniel Vetter):
 - Use manual bitops and BIT() instead of __set_bit

v4 (Daniel Vetter):
 - Add a changelog to the commit message
 - Better hyperlinking in docs
 - Create the default PPGTT in i915_gem_create_context

v5 (Daniel Vetter):
 - Hand-roll the initialization of UCONTEXT_PERSISTENCE

Signed-off-by: Jason Ekstrand <jason@jlekstrand.net>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20210708154835.528166-17-jason@jlekstrand.net
  • Loading branch information
Jason Ekstrand authored and Daniel Vetter committed Jul 8, 2021
1 parent f8a9a5c commit a34857d
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 17 deletions.
84 changes: 69 additions & 15 deletions drivers/gpu/drm/i915/gem/i915_gem_context.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,43 @@ static int validate_priority(struct drm_i915_private *i915,
return 0;
}

static void proto_context_close(struct i915_gem_proto_context *pc)
{
if (pc->vm)
i915_vm_put(pc->vm);
kfree(pc);
}

static struct i915_gem_proto_context *
proto_context_create(struct drm_i915_private *i915, unsigned int flags)
{
struct i915_gem_proto_context *pc, *err;

pc = kzalloc(sizeof(*pc), GFP_KERNEL);
if (!pc)
return ERR_PTR(-ENOMEM);

pc->user_flags = BIT(UCONTEXT_BANNABLE) |
BIT(UCONTEXT_RECOVERABLE);
if (i915->params.enable_hangcheck)
pc->user_flags |= BIT(UCONTEXT_PERSISTENCE);
pc->sched.priority = I915_PRIORITY_NORMAL;

if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE) {
if (!HAS_EXECLISTS(i915)) {
err = ERR_PTR(-EINVAL);
goto proto_close;
}
pc->single_timeline = true;
}

return pc;

proto_close:
proto_context_close(pc);
return err;
}

static struct i915_address_space *
context_get_vm_rcu(struct i915_gem_context *ctx)
{
Expand Down Expand Up @@ -660,7 +697,8 @@ static int __context_set_persistence(struct i915_gem_context *ctx, bool state)
}

static struct i915_gem_context *
__create_context(struct drm_i915_private *i915)
__create_context(struct drm_i915_private *i915,
const struct i915_gem_proto_context *pc)
{
struct i915_gem_context *ctx;
struct i915_gem_engines *e;
Expand All @@ -673,7 +711,7 @@ __create_context(struct drm_i915_private *i915)

kref_init(&ctx->ref);
ctx->i915 = i915;
ctx->sched.priority = I915_PRIORITY_NORMAL;
ctx->sched = pc->sched;
mutex_init(&ctx->mutex);
INIT_LIST_HEAD(&ctx->link);

Expand All @@ -696,9 +734,7 @@ __create_context(struct drm_i915_private *i915)
* is no remap info, it will be a NOP. */
ctx->remap_slice = ALL_L3_SLICES(i915);

i915_gem_context_set_bannable(ctx);
i915_gem_context_set_recoverable(ctx);
__context_set_persistence(ctx, true /* cgroup hook? */);
ctx->user_flags = pc->user_flags;

for (i = 0; i < ARRAY_SIZE(ctx->hang_timestamp); i++)
ctx->hang_timestamp[i] = jiffies - CONTEXT_FAST_HANG_JIFFIES;
Expand Down Expand Up @@ -786,20 +822,22 @@ static void __assign_ppgtt(struct i915_gem_context *ctx,
}

static struct i915_gem_context *
i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags)
i915_gem_create_context(struct drm_i915_private *i915,
const struct i915_gem_proto_context *pc)
{
struct i915_gem_context *ctx;
int ret;

if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE &&
!HAS_EXECLISTS(i915))
return ERR_PTR(-EINVAL);

ctx = __create_context(i915);
ctx = __create_context(i915, pc);
if (IS_ERR(ctx))
return ctx;

if (HAS_FULL_PPGTT(i915)) {
if (pc->vm) {
/* __assign_ppgtt() requires this mutex to be held */
mutex_lock(&ctx->mutex);
__assign_ppgtt(ctx, pc->vm);
mutex_unlock(&ctx->mutex);
} else if (HAS_FULL_PPGTT(i915)) {
struct i915_ppgtt *ppgtt;

ppgtt = i915_ppgtt_create(&i915->gt);
Expand All @@ -810,14 +848,16 @@ i915_gem_create_context(struct drm_i915_private *i915, unsigned int flags)
return ERR_CAST(ppgtt);
}

/* __assign_ppgtt() requires this mutex to be held */
mutex_lock(&ctx->mutex);
__assign_ppgtt(ctx, &ppgtt->vm);
mutex_unlock(&ctx->mutex);

/* __assign_ppgtt() takes another reference for us */
i915_vm_put(&ppgtt->vm);
}

if (flags & I915_CONTEXT_CREATE_FLAGS_SINGLE_TIMELINE) {
if (pc->single_timeline) {
ret = drm_syncobj_create(&ctx->syncobj,
DRM_SYNCOBJ_CREATE_SIGNALED,
NULL);
Expand Down Expand Up @@ -883,6 +923,7 @@ int i915_gem_context_open(struct drm_i915_private *i915,
struct drm_file *file)
{
struct drm_i915_file_private *file_priv = file->driver_priv;
struct i915_gem_proto_context *pc;
struct i915_gem_context *ctx;
int err;
u32 id;
Expand All @@ -892,7 +933,14 @@ int i915_gem_context_open(struct drm_i915_private *i915,
/* 0 reserved for invalid/unassigned ppgtt */
xa_init_flags(&file_priv->vm_xa, XA_FLAGS_ALLOC1);

ctx = i915_gem_create_context(i915, 0);
pc = proto_context_create(i915, 0);
if (IS_ERR(pc)) {
err = PTR_ERR(pc);
goto err;
}

ctx = i915_gem_create_context(i915, pc);
proto_context_close(pc);
if (IS_ERR(ctx)) {
err = PTR_ERR(ctx);
goto err;
Expand Down Expand Up @@ -1884,6 +1932,7 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
{
struct drm_i915_private *i915 = to_i915(dev);
struct drm_i915_gem_context_create_ext *args = data;
struct i915_gem_proto_context *pc;
struct create_ext ext_data;
int ret;
u32 id;
Expand All @@ -1906,7 +1955,12 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
return -EIO;
}

ext_data.ctx = i915_gem_create_context(i915, args->flags);
pc = proto_context_create(i915, args->flags);
if (IS_ERR(pc))
return PTR_ERR(pc);

ext_data.ctx = i915_gem_create_context(i915, pc);
proto_context_close(pc);
if (IS_ERR(ext_data.ctx))
return PTR_ERR(ext_data.ctx);

Expand Down
22 changes: 22 additions & 0 deletions drivers/gpu/drm/i915/gem/i915_gem_context_types.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,28 @@ struct i915_gem_engines_iter {
const struct i915_gem_engines *engines;
};

/**
* struct i915_gem_proto_context - prototype context
*
* The struct i915_gem_proto_context represents the creation parameters for
* a struct i915_gem_context. This is used to gather parameters provided
* either through creation flags or via SET_CONTEXT_PARAM so that, when we
* create the final i915_gem_context, those parameters can be immutable.
*/
struct i915_gem_proto_context {
/** @vm: See &i915_gem_context.vm */
struct i915_address_space *vm;

/** @user_flags: See &i915_gem_context.user_flags */
unsigned long user_flags;

/** @sched: See &i915_gem_context.sched */
struct i915_sched_attr sched;

/** @single_timeline: See See &i915_gem_context.syncobj */
bool single_timeline;
};

/**
* struct i915_gem_context - client state
*
Expand Down
16 changes: 14 additions & 2 deletions drivers/gpu/drm/i915/gem/selftests/mock_context.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,17 @@ void mock_init_contexts(struct drm_i915_private *i915)
struct i915_gem_context *
live_context(struct drm_i915_private *i915, struct file *file)
{
struct i915_gem_proto_context *pc;
struct i915_gem_context *ctx;
int err;
u32 id;

ctx = i915_gem_create_context(i915, 0);
pc = proto_context_create(i915, 0);
if (IS_ERR(pc))
return ERR_CAST(pc);

ctx = i915_gem_create_context(i915, pc);
proto_context_close(pc);
if (IS_ERR(ctx))
return ctx;

Expand Down Expand Up @@ -142,8 +148,14 @@ struct i915_gem_context *
kernel_context(struct drm_i915_private *i915)
{
struct i915_gem_context *ctx;
struct i915_gem_proto_context *pc;

pc = proto_context_create(i915, 0);
if (IS_ERR(pc))
return ERR_CAST(pc);

ctx = i915_gem_create_context(i915, 0);
ctx = i915_gem_create_context(i915, pc);
proto_context_close(pc);
if (IS_ERR(ctx))
return ctx;

Expand Down

0 comments on commit a34857d

Please sign in to comment.