On 16/12/2015 19:30, Chris Wilson wrote:
On Wed, Dec 16, 2015 at 07:22:52PM +0000, Dave Gordon wrote:
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 ...
And that the ctx was created to belong to the file still holds true.
... 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.
No, we have a flag that says this context was created belonging to a
file, with the corollary that only one context doesn't belong to any
file.
Using pointers like this to provide 'magic' secondary state information
just adds to the fragility of the driver.
So:
Reviewed-by: Nick Hoath <nicholas.hoath@xxxxxxxxx>
to the original patch.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
