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

[Lucas De Marchi]

On Thu, Jul 18, 2024 at 11:57:50AM GMT, Umesh Nerlige Ramappa wrote:
>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 at 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 at 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.

yeah, that's the most autoritative one. Note that it also uses "Return:"
rather than "Returns:"

Lucas De Marchi