Re: [Intel-gfx] [PATCH 3/9] drm/i915: Make GEM contexts track DRM clients

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Lucas De Marchi <lucas.demarchi@intel.com>
Cc: Intel-gfx@lists.freedesktop.org, Chris Wilson <chris@chris-wilson.co.uk>
Subject: Re: [Intel-gfx] [PATCH 3/9] drm/i915: Make GEM contexts track DRM clients
Date: Fri, 11 Sep 2020 09:52:10 +0100	[thread overview]
Message-ID: <87f8514f-1cae-51a3-e6ad-caf87fb3ebf1@linux.intel.com> (raw)
In-Reply-To: <20200911064406.zkzlupalayyd3wp5@ldmartin-desk1>

On 11/09/2020 07:44, Lucas De Marchi wrote:
> On Fri, Sep 04, 2020 at 01:59:28PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> If we make GEM contexts keep a reference to i915_drm_client for the whole
>> of their lifetime, we can consolidate the current task pid and name usage
>> by getting it from the client.
>>
>> v2:
>> * Don't bother supporting selftests contexts from debugfs. (Chris)
>>
>> v3:
>> * Trivial rebase.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>> drivers/gpu/drm/i915/gem/i915_gem_context.c   | 19 +++++++++---
>> .../gpu/drm/i915/gem/i915_gem_context_types.h | 13 ++-------
>> drivers/gpu/drm/i915/i915_debugfs.c           | 29 +++++++------------
>> drivers/gpu/drm/i915/i915_gpu_error.c         | 22 ++++++++------
>> 4 files changed, 41 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c 
>> b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> index 5919cc5f8348..ab8d25eda204 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>> @@ -336,8 +336,13 @@ static struct i915_gem_engines 
>> *default_engines(struct i915_gem_context *ctx)
>>
>> static void i915_gem_context_free(struct i915_gem_context *ctx)
>> {
>> +    struct i915_drm_client *client = ctx->client;
>> +
>>     GEM_BUG_ON(!i915_gem_context_is_closed(ctx));
>>
>> +    if (client)
>> +        i915_drm_client_put(client);
>> +
>>     spin_lock(&ctx->i915->gem.contexts.lock);
>>     list_del(&ctx->link);
>>     spin_unlock(&ctx->i915->gem.contexts.lock);
>> @@ -348,7 +353,6 @@ static void i915_gem_context_free(struct 
>> i915_gem_context *ctx)
>>     if (ctx->timeline)
>>         intel_timeline_put(ctx->timeline);
>>
>> -    put_pid(ctx->pid);
>>     mutex_destroy(&ctx->mutex);
>>
>>     kfree_rcu(ctx, rcu);
>> @@ -936,6 +940,7 @@ static int gem_context_register(struct 
>> i915_gem_context *ctx,
>>                 u32 *id)
>> {
>>     struct drm_i915_private *i915 = ctx->i915;
>> +    struct i915_drm_client *client;
>>     struct i915_address_space *vm;
>>     int ret;
>>
>> @@ -947,9 +952,13 @@ static int gem_context_register(struct 
>> i915_gem_context *ctx,
>>         WRITE_ONCE(vm->file, fpriv); /* XXX */
>>     mutex_unlock(&ctx->mutex);
>>
>> -    ctx->pid = get_task_pid(current, PIDTYPE_PID);
>> +    client = i915_drm_client_get(fpriv->client);
>> +
>> +    rcu_read_lock();
>>     snprintf(ctx->name, sizeof(ctx->name), "%s[%d]",
>> -         current->comm, pid_nr(ctx->pid));
>> +         rcu_dereference(client->name),
>> +         pid_nr(rcu_dereference(client->pid)));
>> +    rcu_read_unlock();
>>
>>     /* And finally expose ourselves to userspace via the idr */
>>     ret = xa_alloc(&fpriv->context_xa, id, ctx, xa_limit_32b, 
>> GFP_KERNEL);
>> @@ -960,10 +969,12 @@ static int gem_context_register(struct 
>> i915_gem_context *ctx,
>>     list_add_tail(&ctx->link, &i915->gem.contexts.list);
>>     spin_unlock(&i915->gem.contexts.lock);
>>
>> +    ctx->client = client;
> 
> as per 5f7cceabf104 ("drm/i915/gem: Delay tracking the GEM context until 
> it is registered")
> shouldn't you finish construct ctx before adding it to the list?

Looks like it, well spotted, thanks!

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx