Re: [PATCH 2/5] drm/xe: Add ref counting for xe_file

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Mon, Jul 08, 2024 at 09:23:15PM +0000, Matthew Brost wrote:
>On Mon, Jul 08, 2024 at 01:21:00PM -0700, Umesh Nerlige Ramappa wrote:
>> Add ref counting for xe_file.
>>
>
>Patch LGTM, but a nit I'd make this first patch in the series.
>
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_device.c       |  7 +++++--
>>  drivers/gpu/drm/xe/xe_device.h       | 12 ++++++++++++
>>  drivers/gpu/drm/xe/xe_device_types.h |  3 +++
>>  3 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index babb697652d5..e6eacf1bcce0 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -87,11 +87,14 @@ static int xe_file_open(struct drm_device *dev, struct drm_file *file)
>>  	spin_unlock(&xe->clients.lock);
>>
>>  	file->driver_priv = xef;
>> +	kref_init(&xef->refcount);
>> +
>>  	return 0;
>>  }
>>
>> -static void xe_file_destroy(struct xe_file *xef)
>
>Kernel doc as this is exported.

will add

>
>> +void xe_file_destroy(struct kref *ref)
>>  {
>> +	struct xe_file *xef = container_of(ref, struct xe_file, refcount);
>>  	struct xe_device *xe = xef->xe;
>>  	struct xe_vm *vm;
>>  	unsigned long idx;
>> @@ -130,7 +133,7 @@ static void xe_file_close(struct drm_device *dev, struct drm_file *file)
>>  		xe_exec_queue_put(q);
>>  	}
>>
>> -	xe_file_destroy(xef);
>> +	xe_file_put(xef);
>>  }
>>
>>  static const struct drm_ioctl_desc xe_ioctls[] = {
>> diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
>> index bb07f5669dbb..2a8b370b1fda 100644
>> --- a/drivers/gpu/drm/xe/xe_device.h
>> +++ b/drivers/gpu/drm/xe/xe_device.h
>> @@ -169,5 +169,17 @@ static inline bool xe_device_wedged(struct xe_device *xe)
>>  }
>>
>>  void xe_device_declare_wedged(struct xe_device *xe);
>> +void xe_file_destroy(struct kref *ref);
>> +
>
>I forget if this is a hard rule, but public static inlines likely should
>have kernl doc too.

Not seeing kernel doc in other places in xe where similar inlines 
(get/put) are defined, so did a quick check here - 
https://docs.kernel.org/doc-guide/kernel-doc.html. Looks like kernel-doc 
is a low priority for static functions (no specific mention of inline 
though).  The get/put are self documenting, so IMO, I would avoid 
documenting them, unless there are any other concerns.

Thanks,
Umesh
  
>
>Matt
>
>> +static inline struct xe_file *xe_file_get(struct xe_file *xef)
>> +{
>> +	kref_get(&xef->refcount);
>> +	return xef;
>> +}
>> +
>> +static inline void xe_file_put(struct xe_file *xef)
>> +{
>> +	kref_put(&xef->refcount, xe_file_destroy);
>> +}
>>
>>  #endif
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index c37be471d11c..ec2b0fb6afe1 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -566,6 +566,9 @@ struct xe_file {
>>
>>  	/** @client: drm client */
>>  	struct xe_drm_client *client;
>> +
>> +	/** @refcount: ref count of this xe file */
>> +	struct kref refcount;
>>  };
>>
>>  #endif
>> --
>> 2.38.1
>>