On 16/12/15 18:57, Chris Wilson wrote:
On Wed, Dec 16, 2015 at 06:36:49PM +0000, Dave Gordon wrote:
Some of the LRC-specific context-destruction code has to special-case
the global default context, because the HWSP is part of that context. At
present it deduces it indirectly by checking for the backpointer from
the engine to the context, but that's an unsafe assumption if the setup
and teardown code is reorganised. (It could also test !ctx->file_priv,
but again that's a detail that might be subject to change).
So here we explicitly flag the default context at the point of creation,
and then reorganise the code in intel_lr_context_free() not to rely on
the ring->default_pointer (still) being set up; to iterate over engines
in reverse (as this is teardown code); and to reduce the nesting level
so it's easier to read.
Signed-off-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
#define intel_context_is_global(ctx) ((ctx)->file_priv == NULL)
The last sentence of the first paragraph of the commit message above
notes that we *could* use that as a test, but I don't regard it as a
safe test, in either direction. That is, it could give a false negative
if we someday associate some (internal) fd with the default context, or
(more likely) a false positive if the file association were broken and
the pointer nulled in an earlier stage of the teardown of a non-global
(user-created) context.
int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
struct drm_file *file)
{
struct drm_i915_gem_context_destroy *args = data;
struct drm_i915_file_private *file_priv = file->driver_priv;
struct intel_context *ctx;
int ret;
if (args->ctx_id == DEFAULT_CONTEXT_HANDLE)
return -ENOENT;
ret = i915_mutex_lock_interruptible(dev);
if (ret)
return ret;
ctx = i915_gem_context_get(file_priv, args->ctx_id);
if (IS_ERR(ctx)) {
mutex_unlock(&dev->struct_mutex);
return PTR_ERR(ctx);
}
idr_remove(&ctx->file_priv->context_idr, ctx->user_handle);
i915_gem_context_unreference(ctx);
mutex_unlock(&dev->struct_mutex);
DRM_DEBUG_DRIVER("HW context %d destroyed\n", args->ctx_id);
return 0;
}
At present, i915_gem_context_destroy_ioctl() above removes the context
from the file's list-of-contexts but DOESN'T clear the ctx->file_priv,
which means there's a somewhat inconsistent (but transient) state during
which a soon-to-be-destroyed context links to a file, but the file
doesn't have a link back. It probably doesn't matter, because the code
holds the mutex across the two operations ...
... unless of course the context's refcount isn't 1 at this point, in
which case I suppose someone else *might* go from the context to the
file and then be mystified as to why the context isn't on the list ...
... and if we changed the code above, then file_priv would *always* be
NULL by the time the destructor was called!
So it's surely safer to have a flag that explicitly says "I'm the global
default context" than to guess based on some other contingent property.
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
