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

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

On Thu, Jul 18, 2024 at 08:01:14AM -0500, Lucas De Marchi wrote:
>On Tue, Jul 09, 2024 at 08:41:49PM GMT, Matthew Brost wrote:
>>On Mon, Jul 08, 2024 at 05:28:33PM -0700, Umesh Nerlige Ramappa wrote:
>>>Add ref counting for xe_file.
>>>
>>>v2:
>>>- Add kernel doc for exported functions (Matt)
>>>- Instead of xe_file_destroy, export the get/put helpers (Lucas)
>>
>>Won't get into this bikeshed here.
>
>in the previous version I also pointed at other alternatives that would
>allow the inlining you were talking about. Anyway, as we said we may
>want to make the pattern uniform across the driver later
>
>>
>>>
>>>Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>>---
>>> drivers/gpu/drm/xe/xe_device.c       | 31 ++++++++++++++++++++++++++--
>>> drivers/gpu/drm/xe/xe_device.h       |  3 +++
>>> drivers/gpu/drm/xe/xe_device_types.h |  3 +++
>>> 3 files changed, 35 insertions(+), 2 deletions(-)
>>>
>>>diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>>>index c5464ad5d908..61dc0a37a4d0 100644
>>>--- a/drivers/gpu/drm/xe/xe_device.c
>>>+++ b/drivers/gpu/drm/xe/xe_device.c
>>>@@ -90,11 +90,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)
>>>+static void xe_file_destroy(struct kref *ref)
>>> {
>>>+	struct xe_file *xef = container_of(ref, struct xe_file, refcount);
>>> 	struct xe_device *xe = xef->xe;
>>>
>>> 	xa_destroy(&xef->exec_queue.xa);
>>>@@ -110,6 +113,30 @@ static void xe_file_destroy(struct xe_file *xef)
>>> 	kfree(xef);
>>> }
>>>
>>>+/**
>>>+ * xe_file_get: Take a reference to the xe file object
>>
>>s/xe_file_get/xe_file_get()
>>
>>>+ * @xef: Pointer to the xe file
>>>+ *
>>>+ * Anyone making a copy of xef must take a reference to the xe file
>>
>>s/'Anyone making a copy'/'Anyone with a pointer'
>>
>>>+ * object using this call.
>>
>>
>>Returns: xe file pointer
>>
>>>+ */
>>>+struct xe_file *xe_file_get(struct xe_file *xef)
>>>+{
>>>+	kref_get(&xef->refcount);
>>>+	return xef;
>>>+}
>>>+
>>>+/**
>>>+ * xe_file_put: Drop a reference to the xe file object
>>
>>s/xe_file_put/xe_file_put()
>>
>>With the nits fixed:
>>Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>
>agreed with the comments, but the kernel-doc ones don't appear
>correct... we document the functions like
>
>/**
> * xe_file_put - Drop a reference to the xe file object
> * @ .... arguments
> *
> * ... additional doc
> *
> * Returns: ...
> */

Again, this is not consistent across Xe. I see some files following what 
Lucas mentioned and others following this link (like Matt's suggestion):

https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation

I will follow the format in the link above and hope that we standardize 
that across Xe.

Regards,
Umesh


>
>Lucas De Marchi
>
>>
>>>+ * @xef: Pointer to the xe file
>>>+ *
>>>+ * Used to drop reference to the xef object
>>>+ */
>>>+void xe_file_put(struct xe_file *xef)
>>>+{
>>>+	kref_put(&xef->refcount, xe_file_destroy);
>>>+}
>>>+
>>> static void xe_file_close(struct drm_device *dev, struct drm_file *file)
>>> {
>>> 	struct xe_file *xef = file->driver_priv;
>>>@@ -132,7 +159,7 @@ static void xe_file_close(struct drm_device *dev, struct drm_file *file)
>>> 		xe_vm_close_and_put(vm);
>>> 	mutex_unlock(&xef->vm.lock);
>>>
>>>-	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 0a2a3e7fd402..533ccfb2567a 100644
>>>--- a/drivers/gpu/drm/xe/xe_device.h
>>>+++ b/drivers/gpu/drm/xe/xe_device.h
>>>@@ -171,4 +171,7 @@ static inline bool xe_device_wedged(struct xe_device *xe)
>>>
>>> void xe_device_declare_wedged(struct xe_device *xe);
>>>
>>>+struct xe_file *xe_file_get(struct xe_file *xef);
>>>+void xe_file_put(struct xe_file *xef);
>>>+
>>> #endif
>>>diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>>>index f0cf9020e463..70115fb8d806 100644
>>>--- a/drivers/gpu/drm/xe/xe_device_types.h
>>>+++ b/drivers/gpu/drm/xe/xe_device_types.h
>>>@@ -578,6 +578,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
>>>