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

From: Dave Gordon <david.s.gordon@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/4] drm/i915: mark the global default (intel_)context as such
Date: Wed, 16 Dec 2015 19:22:52 +0000	[thread overview]
Message-ID: <5671BA0C.9030105@intel.com> (raw)
In-Reply-To: <20151216185713.GD18452@nuc-i3427.alporthouse.com>

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@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 ...

... 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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx