[PATCH v2 1/8] [ANDROID]: Add a new xe_user structure

Fri Aug 22 20:09:51 UTC 2025

On Fri, Aug 22, 2025 at 10:01:25AM -0700, Matthew Brost wrote:
> On Fri, Aug 22, 2025 at 11:48:54AM -0400, Rodrigo Vivi wrote:
> > On Fri, Aug 22, 2025 at 08:59:23AM +0000, Aakash Deep Sarkar wrote:
> > > For Android GPU work period event we need to track the runtime
> > > on the GPU for each user id. This means we can have multiple
> > > xe files opened by different processes/threads belonging to
> > > the same user id. All these xe files need to be grouped together
> > > so that one can easily identify these while calculating the
> > > run time for the given user id.
> > > 
> > > Currently, the xe driver doesn't record the user id of the
> > > calling process. Also, all the xe files created using open
> > > call are clubbed together inside the xe device structure
> > > with no way to distinguish between them based on the user id
> > > of the calling process.
> > 
> > I thought I had already given this feedback, but I'm not sure if
> > I forgot or if I was just ignored. I'm sorry either way.
> > 
> > Android is not a justification. Please keep 'Android' mentions
> > and ralated 'Android' justifications in the cover letter ONLY!
> > 
> > The patch needs to make sense by itself. The patch needs to make
> > sense in the currently linux upstream.
> > 
> 
> So what are the rules here? There is another series [1] floating around
> with a justification that Android needs this. Does that mean we can't
> accept any Andriod only code upstream?

I'm sorry for not being clear. Of course we can accept Android code
upstream. I wish we had more (all?) Android code upstream ;)

But we cannot add in xe, for instance, something with the namespace
/sys/kernel/tracing/events/power/gpu_work_period where
gpu_work_period is a definition in the Android tree only.

We could add xe_work_period.

But my comment was more with the justification of the patches itself.

The series tells a history. It explains the goals and motivations in its
cover letter. There it is okay to tell that we are adding this
gpu_work_period to attend to the Android requirements.

Then, it patch itself is a small piece of the puzzle telling what it
is doing and why, but in technical terms, without having to tell
that we are doing for Android.

And also every patch in the mailing list needs to come with [PATCH]

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#include-patch-in-the-subject

in our case it is okay to accept [RFC] or [CI], but we should limit our
creativity there ;)

But the most important rule that we have to respect imho is this:
https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#open-source-userspace-requirements

so, if we are targeting this upstream we need to justify based on the
admin using perf or trace directly. Commit messages should explain based
on admin usages of these APIs directly. Documentation added along with
the code should also demonstrate the usage with perf and or trace.

They should never rely on closed source Android applications that
might be using 'gpu_work_period'. That is unacceptable by these rules.

Thanks,
Rodrigo.

> 
> Matt
> 
> [1] https://patchwork.freedesktop.org/series/152701/
> 
> > This also means no --subject-prefix=ANDROID.
> > 
> > Please!
> > 
> > > 
> > > To remedy these limitations we are adding another layer of
> > > indirection between xe device and xe file. xe device will
> > > now have a list of xe users each with a given user id; and each
> > > xe user will have a list of xe files each of which is created
> > > by a process that is associated with this user id.
> > > 
> > > The lifetime of the xe user structure should be between when
> > > a process with a new user id has opened the xe device; and when
> > > the last xe file belonging to this user id is closed.
> > > 
> > > Signed-off-by: Aakash Deep Sarkar <aakash.deep.sarkar at intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/Makefile  |  2 +
> > >  drivers/gpu/drm/xe/xe_user.c | 59 ++++++++++++++++++++++++++
> > >  drivers/gpu/drm/xe/xe_user.h | 81 ++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 142 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/xe/xe_user.c
> > >  create mode 100644 drivers/gpu/drm/xe/xe_user.h
> > > 
> > > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> > > index 8e0c3412a757..89212fc7ef44 100644
> > > --- a/drivers/gpu/drm/xe/Makefile
> > > +++ b/drivers/gpu/drm/xe/Makefile
> > > @@ -325,6 +325,8 @@ ifeq ($(CONFIG_DEBUG_FS),y)
> > >  
> > >  	xe-$(CONFIG_PCI_IOV) += xe_gt_sriov_pf_debugfs.o
> > >  
> > > +	xe-y += xe_user.o
> > > +
> > >  	xe-$(CONFIG_DRM_XE_DISPLAY) += \
> > >  		i915-display/intel_display_debugfs.o \
> > >  		i915-display/intel_display_debugfs_params.o \
> > > diff --git a/drivers/gpu/drm/xe/xe_user.c b/drivers/gpu/drm/xe/xe_user.c
> > > new file mode 100644
> > > index 000000000000..8c285a68115a
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/xe/xe_user.c
> > > @@ -0,0 +1,59 @@
> > > +// SPDX-License-Identifier: MIT
> > > +/*
> > > + * Copyright © 2023 Intel Corporation
> > > + */
> > > +
> > > +#include <linux/slab.h>
> > > +
> > > +#include "xe_user.h"
> > > +
> > > +/**
> > > + * worker thread to emit gpu work period event for this xe user
> > > + * @work: work instance for this xe user
> > > + *
> > > + * Return: void
> > > + */
> > > +static inline void work_period_worker(struct work_struct *work)
> > > +{
> > > +	//TODO: Implement this worker
> > > +}
> > > +
> > > +/**
> > > + * xe_user_alloc() - Allocate xe user
> > > + * @void: No arg
> > > + *
> > > + * Allocate xe user struct to track activity on the gpu
> > > + * by the application. Call this API whenever a new app
> > > + * has opened xe device.
> > > + *
> > > + * Return: pointer to user struct or NULL if can't allocate
> > > + */
> > > +struct xe_user *xe_user_alloc(void)
> > > +{
> > > +	struct xe_user *user;
> > > +
> > > +	user = kzalloc(sizeof(*user), GFP_KERNEL);
> > > +	if (!user)
> > > +		return NULL;
> > > +
> > > +	kref_init(&user->refcount);
> > > +	mutex_init(&user->filelist_lock);
> > > +	INIT_LIST_HEAD(&user->filelist);
> > > +	//TODO: Add a hook into xe device
> > > +	INIT_WORK(&user->work, work_period_worker);
> > > +	return user;
> > > +}
> > > +
> > > +/**
> > > + * __xe_user_free() - Free user struct
> > > + * @kref: The reference
> > > + *
> > > + * Return: void
> > > + */
> > > +void __xe_user_free(struct kref *kref)
> > > +{
> > > +	struct xe_user *user =
> > > +		container_of(kref, struct xe_user, refcount);
> > > +
> > > +	kfree(user);
> > > +}
> > > diff --git a/drivers/gpu/drm/xe/xe_user.h b/drivers/gpu/drm/xe/xe_user.h
> > > new file mode 100644
> > > index 000000000000..e52f66d3f3b0
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/xe/xe_user.h
> > > @@ -0,0 +1,81 @@
> > > +/* SPDX-License-Identifier: MIT */
> > > +/*
> > > + * Copyright © 2023 Intel Corporation
> > > + */
> > > +
> > > +#ifndef _XE_USER_H_
> > > +#define _XE_USER_H_
> > > +
> > > +#include <linux/kref.h>
> > > +#include <linux/list.h>
> > > +#include <linux/workqueue.h>
> > > +
> > > +/**
> > > + * This is a per process/user id structure for a xe device
> > > + * client. It is allocated when a new process/app opens the
> > > + * xe device and destroyed when the last xe file belonging
> > > + * to this user id is destroyed.
> > > + */
> > > +struct xe_user {
> > > +	/**
> > > +	 * @refcount: reference count
> > > +	 */
> > > +	struct kref refcount;
> > > +
> > > +	/**
> > > +	 * @xe: pointer to the xe_device
> > > +	 */
> > > +	struct xe_device *xe;
> > > +
> > > +	/**
> > > +	 * @filelist_lock: lock protecting the filelist
> > > +	 */
> > > +	struct mutex filelist_lock;
> > > +
> > > +	/**
> > > +	 * @filelist: list of xe files belonging to this xe user
> > > +	 */
> > > +	struct list_head filelist;
> > > +
> > > +	/**
> > > +	 * @work: work to emit the gpu work period event for this
> > > +	 * xe user
> > > +	 */
> > > +	struct work_struct work;
> > > +
> > > +	/**
> > > +	 * @uid: user id for this xe_user
> > > +	 */
> > > +	u32 uid;
> > > +
> > > +	/**
> > > +	 * @active_duration_ns: sum total of xe_file.active_duration_ns
> > > +	 * for all xe files belonging to this xe user
> > > +	 */
> > > +	u64 active_duration_ns;
> > > +
> > > +	/**
> > > +	 * @last_timestamp_ns: timestamp in ns when we last emitted event
> > > +	 * for this xe user
> > > +	 */
> > > +	u64 last_timestamp_ns;
> > > +};
> > > +
> > > +struct xe_user *xe_user_alloc(void);
> > > +
> > > +static inline struct xe_user *
> > > +xe_user_get(struct xe_user *user)
> > > +{
> > > +	kref_get(&user->refcount);
> > > +	return user;
> > > +}
> > > +
> > > +void __xe_user_free(struct kref *kref);
> > > +
> > > +static inline void xe_user_put(struct xe_user *user)
> > > +{
> > > +	kref_put(&user->refcount, __xe_user_free);
> > > +}
> > > +
> > > +#endif // _XE_USER_H_
> > > +
> > > -- 
> > > 2.49.0
> > > 