[Intel-gfx] [PATCH 2/4] drm/i915: mark the global default (intel_)context as such

[Chris Wilson]

On Thu, Dec 17, 2015 at 11:09:54AM +0000, Nick Hoath wrote:
> 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 at intel.com>
> >>>
> >>>#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.

It's primary. Ownership information is fundamental to the driver.
The change is irrelevant to the bugfix.

If you want to make such a big change, eliminate the default_ctx from
execlists.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre