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

[Umesh Nerlige Ramappa]

On Tue, Jul 09, 2024 at 10:11:47AM -0500, Lucas De Marchi wrote:
>On Tue, Jul 09, 2024 at 12:28:43AM GMT, Matthew Brost wrote:
>>On Mon, Jul 08, 2024 at 04:26:00PM -0700, Umesh Nerlige Ramappa wrote:
>>>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);
>
>yes

ok, posted v2 with this change.

>
>>>
>>
>>My opinion is the way you have it is correct.
>>
>>Most of Xe is coded this way...
>>
>>mbrost at lstrano-desk:xe$ grep _get\( *.h | grep inline
>>xe_bo.h:static inline struct xe_bo *xe_bo_get(struct xe_bo *bo)
>>xe_device.h:static inline struct xe_file *xe_file_get(struct xe_file *xef)
>>xe_exec_queue.h:static inline struct xe_exec_queue *xe_exec_queue_get(struct xe_exec_queue *q)
>>xe_lrc.h:static inline struct xe_lrc *xe_lrc_get(struct xe_lrc *lrc)
>>xe_sched_job.h:static inline struct xe_sched_job *xe_sched_job_get(struct xe_sched_job *job)
>>xe_vm.h:static inline struct xe_vm *xe_vm_get(struct xe_vm *vm)
>>
>>I'm kinda confused by what Lucas is suggesting.
>
>just don't make xe_file_destroy() non-static, which would basically
>ignore the refcounting. xe_file_destroy() should rather be local to the
>.c implementing xe_file_put().
>

In the above instances from grep, most of the destroy helpers are indeed 
exported. Those might need to be changed as per this review comment.

Thanks,
Umesh
>
>Lucas De Marchi