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

[Umesh Nerlige Ramappa]

On Mon, Jul 08, 2024 at 05:52:00PM -0500, Lucas De Marchi wrote:
>On Mon, Jul 08, 2024 at 01:21:00PM GMT, Umesh Nerlige Ramappa wrote:
>>Add ref counting for xe_file.
>>
>>Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at 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)
>>+void xe_file_destroy(struct kref *ref)
>
>why do you export this?  I don't think anybody should be calling
>xe_file_destroy() directly and this function being executed should only
>be the outcome of kref being 0. Also, if it's exported, it shouldn't
>really have struct kref as argument, but rather a struct xe_file
>
>
>>{
>>+	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);
>>+
>>+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)
>
>I think if you just make this non-inline, passing file_destroy and
>leave it in the same place, it should work.

the put is called from xe_exec_queue.c and xe_vm.c also, so passing the 
destroy may not work.

Based on the patterns below, xe_file_put/xe_file_get will need to be 
moved to xe_device.c and then exported. It looks cleaner, hoping that'w 
what you mean.

In xe_device.h:

void xe_file_put(struct xe_file *xef);
struct xe_file *xe_file_get(struct xe_file *xef);

Thanks,
Umesh

>
>Looking for patterns in the kernel for example, see pinctrl_get(),
>pinctrl_put() and pinctrl_release().... or clk_get(), clk_put(),
>__clk_release()
>
>Lucas De Marchi
>
>>+{
>>+	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
>>