Message-ID: <6vnoxwsc2wy3spnm6w4e53usikceemy5bz5frbqsmwc53atyzn@r2tbbdqoojx3>
Date: Mon, 27 Jan 2025 15:43:27 +0000
From: Adrián Larumbe <adrian.larumbe@...labora.com>
To: Lukas Zapolskas <lukas.zapolskas@....com>
Cc: Boris Brezillon <boris.brezillon@...labora.com>, 
	Steven Price <steven.price@....com>, Liviu Dudau <liviu.dudau@....com>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>, dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	Mihail Atanassov <mihail.atanassov@....com>, nd@....com
Subject: Re: [RFC v2 5/8] drm/panthor: Introduce sampling sessions to handle
 userspace clients

On 11.12.2024 16:50, Lukas Zapolskas wrote:
> To allow for combining the requests from multiple userspace clients, an
> intermediary layer between the HW/FW interfaces and userspace is
> created, containing the information for the counter requests and
> tracking of insert and extract indices. Each session starts inactive and
> must be explicitly activated via PERF_CONTROL.START, and explicitly
> stopped via PERF_CONTROL.STOP. Userspace identifies a single client with
> its session ID and the panthor file it is associated with.
> 
> The SAMPLE and STOP commands both produce a single sample when called,
> and these samples can be disambiguated via the opaque user data field
> passed in the PERF_CONTROL uAPI. If this functionality is not desired,
> these fields can be kept as zero, as the kernel copies this value into
> the corresponding sample without attempting to interpret it.
> 
> Currently, only manual sampling sessions are supported, providing
> samples when userspace calls PERF_CONTROL.SAMPLE, and only a single
> session is allowed at a time. Multiple sessions and periodic sampling
> will be enabled in following patches.
> 
> No protected is provided against the 32-bit hardware counter overflows,

Spelling: protected

> so for the moment it is up to userspace to ensure that the counters are
> sampled at a reasonable frequency.

> The counter set enum is added to the uapi to clarify the restrictions on
> calling the interface.
> 
> Signed-off-by: Lukas Zapolskas <lukas.zapolskas@....com>
> ---
>  drivers/gpu/drm/panthor/panthor_device.h |   3 +
>  drivers/gpu/drm/panthor/panthor_drv.c    |   1 +
>  drivers/gpu/drm/panthor/panthor_perf.c   | 697 ++++++++++++++++++++++-
>  include/uapi/drm/panthor_drm.h           |  50 +-
>  4 files changed, 732 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
> index aca33d03036c..9ed1e9aed521 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.h
> +++ b/drivers/gpu/drm/panthor/panthor_device.h
> @@ -210,6 +210,9 @@ struct panthor_file {
>  	/** @ptdev: Device attached to this file. */
>  	struct panthor_device *ptdev;
>  
> +	/** @drm_file: Corresponding drm_file */
> +	struct drm_file *drm_file;

I think you could do away with this member, wrote more about this below.

>  	/** @vms: VM pool attached to this file. */
>  	struct panthor_vm_pool *vms;
>  
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index 458175f58b15..2848ab442d10 100644
> --- a/drivers/gpu/drm/panthor/panthor_drv.c
> +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> @@ -1505,6 +1505,7 @@ panthor_open(struct drm_device *ddev, struct drm_file *file)
>  	}
>  
>  	pfile->ptdev = ptdev;
> +	pfile->drm_file = file;

Same as above, feel like this is not necessary.

>  
>  	ret = panthor_vm_pool_create(pfile);
>  	if (ret)
> diff --git a/drivers/gpu/drm/panthor/panthor_perf.c b/drivers/gpu/drm/panthor/panthor_perf.c
> index 6498279ec036..42d8b6f8c45d 100644
> --- a/drivers/gpu/drm/panthor/panthor_perf.c
> +++ b/drivers/gpu/drm/panthor/panthor_perf.c
> @@ -3,16 +3,162 @@
>  /* Copyright 2024 Arm ltd. */
>  
>  #include <drm/drm_file.h>
> +#include <drm/drm_gem.h>
>  #include <drm/drm_gem_shmem_helper.h>
>  #include <drm/drm_managed.h>
> +#include <drm/drm_print.h>
>  #include <drm/panthor_drm.h>
>  
> +#include <linux/circ_buf.h>
> +#include <linux/iosys-map.h>
> +#include <linux/pm_runtime.h>
> +
>  #include "panthor_device.h"
>  #include "panthor_fw.h"
>  #include "panthor_gpu.h"
>  #include "panthor_perf.h"
>  #include "panthor_regs.h"
>  
> +/**
> + * PANTHOR_PERF_EM_BITS - Number of bits in a user-facing enable mask. This must correspond
> + *                        to the maximum number of counters available for selection on the newest
> + *                        Mali GPUs (128 as of the Mali-Gx15).
> + */
> +#define PANTHOR_PERF_EM_BITS (BITS_PER_TYPE(u64) * 2)
> +
> +/**
> + * enum panthor_perf_session_state - Session state bits.
> + */
> +enum panthor_perf_session_state {
> +	/** @PANTHOR_PERF_SESSION_ACTIVE: The session is active and can be used for sampling. */
> +	PANTHOR_PERF_SESSION_ACTIVE = 0,
> +
> +	/**
> +	 * @PANTHOR_PERF_SESSION_OVERFLOW: The session encountered an overflow in one of the
> +	 *                                 counters during the last sampling period. This flag
> +	 *                                 gets propagated as part of samples emitted for this
> +	 *                                 session, to ensure the userspace client can gracefully
> +	 *                                 handle this data corruption.
> +	 */

How would a client normally deal with data corruption in a sample?

> +	PANTHOR_PERF_SESSION_OVERFLOW,
> +
> +	/** @PANTHOR_PERF_SESSION_MAX: Bits needed to represent the state. Must be last.*/
> +	PANTHOR_PERF_SESSION_MAX,
> +};
> +
> +struct panthor_perf_enable_masks {
> +	/**
> +	 * @link: List node used to keep track of the enable masks aggregated by the sampler.
> +	 */
> +	struct list_head link;
> +
> +	/** @refs: Number of references taken out on an instantiated enable mask. */
> +	struct kref refs;
> +
> +	/**
> +	 * @mask: Array of bitmasks indicating the counters userspace requested, where
> +	 *        one bit represents a single counter. Used to build the firmware configuration
> +	 *        and ensure that userspace clients obtain only the counters they requested.
> +	 */
> +	DECLARE_BITMAP(mask, PANTHOR_PERF_EM_BITS)[DRM_PANTHOR_PERF_BLOCK_MAX];
> +};
> +
> +struct panthor_perf_counter_block {
> +	struct drm_panthor_perf_block_header header;
> +	u64 counters[];
> +};

I think I remember reading in the spec thata block header was 12 bytes in length
but the one defined here seems to have many more fields.

> +struct panthor_perf_session {
> +	DECLARE_BITMAP(state, PANTHOR_PERF_SESSION_MAX);

I'm wondering, because I don't remember having seen this pattern before.
Is it common in kernel code to declare bitmaps for masks of enum values in this way?

> +	/**
> +	 * @user_sample_size: The size of a single sample as exposed to userspace. For the sake of
> +	 *                    simplicity, the current implementation exposes the same structure
> +	 *                    as provided by firmware, after annotating the sample and the blocks,
> +	 *                    and zero-extending the counters themselves (to account for in-kernel
> +	 *                    accumulation).
> +	 *
> +	 *                    This may also allow further memory-optimizations of compressing the
> +	 *                    sample to provide only requested blocks, if deemed to be worth the
> +	 *                    additional complexity.
> +	 */
> +	size_t user_sample_size;
> +
> +	/**
> +	 * @sample_freq_ns: Period between subsequent sample requests. Zero indicates that
> +	 *                  userspace will be responsible for requesting samples.
> +	 */
> +	u64 sample_freq_ns;
> +
> +	/** @sample_start_ns: Sample request time, obtained from a monotonic raw clock. */
> +	u64 sample_start_ns;
> +
> +	/**
> +	 * @user_data: Opaque handle passed in when starting a session, requesting a sample (for
> +	 *             manual sampling sessions only) and when stopping a session. This handle
> +	 *             allows the disambiguation of a sample in the ringbuffer.
> +	 */
> +	u64 user_data;
> +
> +	/**
> +	 * @eventfd: Event file descriptor context used to signal userspace of a new sample
> +	 *           being emitted.
> +	 */
> +	struct eventfd_ctx *eventfd;
> +
> +	/**
> +	 * @enabled_counters: This session's requested counters. Note that these cannot change
> +	 *                    for the lifetime of the session.
> +	 */
> +	struct panthor_perf_enable_masks *enabled_counters;

It seems the enable mask for a session is tied to the session's lifetime. In
panthor_perf_session_setup(), you create one and then increase its reference
count from within panthor_perf_sampler_add(), which is not being called from
anywhere else. Maybe in that case you could do without the reference count and
have a non-pointer struct panthor_perf_enable_masks member here?

> +	/** @ringbuf_slots: Slots in the user-facing ringbuffer. */
> +	size_t ringbuf_slots;
> +
> +	/** @ring_buf: BO for the userspace ringbuffer. */
> +	struct drm_gem_object *ring_buf;
> +
> +	/**
> +	 * @control_buf: BO for the insert and extract indices.
> +	 */
> +	struct drm_gem_object *control_buf;
> +
> +	/**
> +	 * @extract_idx: The extract index is used by userspace to indicate the position of the
> +	 *               consumer in the ringbuffer.
> +	 */
> +	u32 *extract_idx;
> +
> +	/**
> +	 * @insert_idx: The insert index is used by the kernel to indicate the position of the
> +	 *              latest sample exposed to userspace.
> +	 */
> +	u32 *insert_idx;
> +
> +	/** @samples: The mapping of the @ring_buf into the kernel's VA space. */
> +	u8 *samples;
> +
> +	/**
> +	 * @waiting: The list node used by the sampler to track the sessions waiting for a sample.
> +	 */
> +	struct list_head waiting;
> +
> +	/**
> +	 * @pfile: The panthor file which was used to create a session, used for the postclose
> +	 *         handling and to prevent a misconfigured userspace from closing unrelated
> +	 *         sessions.
> +	 */
> +	struct panthor_file *pfile;
> +
> +	/**
> +	 * @ref: Session reference count. The sample delivery to userspace is asynchronous, meaning
> +	 *       the lifetime of the session must extend at least until the sample is exposed to
> +	 *       userspace.
> +	 */
> +	struct kref ref;
> +};
> +
> +
>  struct panthor_perf {
>  	/**
>  	 * @block_set: The global counter set configured onto the HW.
> @@ -63,39 +209,154 @@ void panthor_perf_info_init(struct panthor_device *ptdev)
>  	perf_info->shader_blocks = hweight64(ptdev->gpu_info.shader_present);
>  }
>  
> -int panthor_perf_session_setup(struct panthor_device *ptdev, struct panthor_perf *perf,
> -		struct drm_panthor_perf_cmd_setup *setup_args,
> -		struct panthor_file *pfile)
> +static struct panthor_perf_enable_masks *panthor_perf_em_new(void)
>  {
> -	return -EOPNOTSUPP;
> +	struct panthor_perf_enable_masks *em = kmalloc(sizeof(*em), GFP_KERNEL);
> +
> +	if (!em)
> +		return ERR_PTR(-ENOMEM);
> +
> +	INIT_LIST_HEAD(&em->link);
> +
> +	kref_init(&em->refs);
> +
> +	return em;
>  }
>  
> -int panthor_perf_session_teardown(struct panthor_file *pfile, struct panthor_perf *perf,
> -		u32 sid)
> +static struct panthor_perf_enable_masks *panthor_perf_create_em(struct drm_panthor_perf_cmd_setup
> +		*setup_args)
>  {
> -	return -EOPNOTSUPP;
> +	struct panthor_perf_enable_masks *em = panthor_perf_em_new();
> +
> +	if (IS_ERR_OR_NULL(em))
> +		return em;
> +
> +	bitmap_from_arr64(em->mask[DRM_PANTHOR_PERF_BLOCK_FW],
> +			setup_args->fw_enable_mask, PANTHOR_PERF_EM_BITS);
> +	bitmap_from_arr64(em->mask[DRM_PANTHOR_PERF_BLOCK_CSG],
> +			setup_args->csg_enable_mask, PANTHOR_PERF_EM_BITS);
> +	bitmap_from_arr64(em->mask[DRM_PANTHOR_PERF_BLOCK_CSHW],
> +			setup_args->cshw_enable_mask, PANTHOR_PERF_EM_BITS);
> +	bitmap_from_arr64(em->mask[DRM_PANTHOR_PERF_BLOCK_TILER],
> +			setup_args->tiler_enable_mask, PANTHOR_PERF_EM_BITS);
> +	bitmap_from_arr64(em->mask[DRM_PANTHOR_PERF_BLOCK_MEMSYS],
> +			setup_args->memsys_enable_mask, PANTHOR_PERF_EM_BITS);
> +	bitmap_from_arr64(em->mask[DRM_PANTHOR_PERF_BLOCK_SHADER],
> +			setup_args->shader_enable_mask, PANTHOR_PERF_EM_BITS);

To save some repetition, maybe do this, although it might depend on uAPI
structures being arranged in the right way, and the compiler not inserting
unusual padding between consecutive members:

unsigned int block; u64 *mask;
for (mask = &setup_args->fw_enable_mask[0], block = DRM_PANTHOR_PERF_BLOCK_FW;
     block < DRM_PANTHOR_PERF_BLOCK_LAST; block++, mask += 2)
	bitmap_from_arr64(em->mask[block], mask, PANTHOR_PERF_EM_BITS);

> +	return em;
>  }
>  
> -int panthor_perf_session_start(struct panthor_file *pfile, struct panthor_perf *perf,
> -		u32 sid, u64 user_data)
> +static void panthor_perf_destroy_em_kref(struct kref *em_kref)
>  {
> -	return -EOPNOTSUPP;
> +	struct panthor_perf_enable_masks *em = container_of(em_kref, typeof(*em), refs);
> +
> +	if (!list_empty(&em->link))
> +		return;

Could this lead to a situation where the enable mask's refcnt reaches 0,
but because it hadn't yet been removed from the session's list, the
mask object is never freed?

> +	kfree(em);
>  }
>  
> -int panthor_perf_session_stop(struct panthor_file *pfile, struct panthor_perf *perf,
> -		u32 sid, u64 user_data)
> +static size_t get_annotated_block_size(size_t counters_per_block)
>  {
> -		return -EOPNOTSUPP;
> +	return struct_size_t(struct panthor_perf_counter_block, counters, counters_per_block);
>  }
>  
> -int panthor_perf_session_sample(struct panthor_file *pfile, struct panthor_perf *perf,
> -		u32 sid, u64 user_data)
> +static u32 session_read_extract_idx(struct panthor_perf_session *session)
> +{
> +	/* Userspace will update their own extract index to indicate that a sample is consumed
> +	 * from the ringbuffer, and we must ensure we read the latest value.
> +	 */
> +	return smp_load_acquire(session->extract_idx);
> +}
> +
> +static u32 session_read_insert_idx(struct panthor_perf_session *session)
> +{
> +	return *session->insert_idx;
> +}
> +
> +static void session_get(struct panthor_perf_session *session)
> +{
> +	kref_get(&session->ref);
> +}
> +
> +static void session_free(struct kref *ref)
> +{
> +	struct panthor_perf_session *session = container_of(ref, typeof(*session), ref);
> +
> +	if (session->samples) {
> +		struct iosys_map map = IOSYS_MAP_INIT_VADDR(session->samples);
> +
> +		drm_gem_vunmap_unlocked(session->ring_buf, &map);
> +		drm_gem_object_put(session->ring_buf);
> +	}
> +
> +	if (session->insert_idx && session->extract_idx) {

I think none of these could ever be NULLif session setup succeeded.

> +		struct iosys_map map = IOSYS_MAP_INIT_VADDR(session->extract_idx);
> +
> +		drm_gem_vunmap_unlocked(session->control_buf, &map);
> +		drm_gem_object_put(session->control_buf);
> +	}
> +
> +	kref_put(&session->enabled_counters->refs, panthor_perf_destroy_em_kref);
> +	eventfd_ctx_put(session->eventfd);
> +
> +	devm_kfree(session->pfile->ptdev->base.dev, session);

What is the point of using devm allocations in this case, if we always free
the session manually?

> +}
> +
> +static void session_put(struct panthor_perf_session *session)
> +{
> +	kref_put(&session->ref, session_free);
> +}
> +
> +/**
> + * session_find - Find a session associated with the given session ID and
> + *                panthor_file.
> + * @pfile: Panthor file.
> + * @perf: Panthor perf.
> + * @sid: Session ID.
> + *
> + * The reference count of a valid session is increased to ensure it does not disappear
> + * in the window between the XA lock being dropped and the internal session functions
> + * being called.
> + *
> + * Return: valid session pointer or an ERR_PTR.
> + */
> +static struct panthor_perf_session *session_find(struct panthor_file *pfile,
> +		struct panthor_perf *perf, u32 sid)
>  {
> -	return -EOPNOTSUPP;
> +	struct panthor_perf_session *session;
>  
> +	if (!perf)
> +		return ERR_PTR(-EINVAL);
> +
> +	xa_lock(&perf->sessions);
> +	session = xa_load(&perf->sessions, sid);
> +
> +	if (!session || xa_is_err(session)) {
> +		xa_unlock(&perf->sessions);
> +		return ERR_PTR(-EBADF);

I think we should return NULL in case !session holds true, for panthor_perf_session_start to catch it and return -EINVAL.

> +	}
> +
> +	if (session->pfile != pfile) {
> +		xa_unlock(&perf->sessions);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	session_get(session);
> +	xa_unlock(&perf->sessions);
> +
> +	return session;
>  }
>  
> -void panthor_perf_session_destroy(struct panthor_file *pfile, struct panthor_perf *perf) { }
> +static size_t session_get_max_sample_size(const struct drm_panthor_perf_info *const info)

Since this seems to be the size of the sample given to UM, maybe renaming it to
contain _user_ would make its purpose more apparent.

> +{
> +	const size_t block_size = get_annotated_block_size(info->counters_per_block);
> +	const size_t block_nr = info->cshw_blocks + info->csg_blocks + info->fw_blocks +
> +		info->tiler_blocks + info->memsys_blocks + info->shader_blocks;
> +
> +	return sizeof(struct drm_panthor_perf_sample_header) + (block_size * block_nr);
> +}
>  
>  /**
>   * panthor_perf_init - Initialize the performance counter subsystem.
> @@ -130,6 +391,399 @@ int panthor_perf_init(struct panthor_device *ptdev)
>  	ptdev->perf = perf;
>  
>  	return 0;
> +
> +}
> +
> +static int session_validate_set(u8 set)
> +{
> +	if (set > DRM_PANTHOR_PERF_SET_TERTIARY)
> +		return -EINVAL;
> +
> +	if (set == DRM_PANTHOR_PERF_SET_PRIMARY)
> +		return 0;
> +
> +	if (set > DRM_PANTHOR_PERF_SET_PRIMARY)
> +		return capable(CAP_PERFMON) ? 0 : -EACCES;

I'm a bit clueless about the capability API, so I don't quite understand how
this is the way whe decide whether a counter set is legal.

> +	return -EINVAL;
> +}
> +
> +/**
> + * panthor_perf_session_setup - Create a user-visible session.
> + *
> + * @ptdev: Handle to the panthor device.
> + * @perf: Handle to the perf control structure.
> + * @setup_args: Setup arguments passed in via ioctl.
> + * @pfile: Panthor file associated with the request.
> + *
> + * Creates a new session associated with the session ID returned. When initialized, the
> + * session must explicitly request sampling to start with a successive call to PERF_CONTROL.START.
> + *
> + * Return: non-negative session identifier on success or negative error code on failure.
> + */
> +int panthor_perf_session_setup(struct panthor_device *ptdev, struct panthor_perf *perf,
> +		struct drm_panthor_perf_cmd_setup *setup_args,
> +		struct panthor_file *pfile)
> +{
> +	struct panthor_perf_session *session;
> +	struct drm_gem_object *ringbuffer;
> +	struct drm_gem_object *control;
> +	const size_t slots = setup_args->sample_slots;
> +	struct panthor_perf_enable_masks *em;
> +	struct iosys_map rb_map, ctrl_map;
> +	size_t user_sample_size;
> +	int session_id;
> +	int ret;
> +
> +	ret = session_validate_set(setup_args->block_set);
> +	if (ret)
> +		return ret;
> +
> +	session = devm_kzalloc(ptdev->base.dev, sizeof(*session), GFP_KERNEL);
> +	if (ZERO_OR_NULL_PTR(session))
> +		return -ENOMEM;
> +
> +	ringbuffer = drm_gem_object_lookup(pfile->drm_file, setup_args->ringbuf_handle);
> +	if (!ringbuffer) {
> +		ret = -EINVAL;
> +		goto cleanup_session;
> +	}

I guess this would never be the same ringbuffer we created in
panthor_perf_sampler_init(). I don't think it can be, because that one was is
created as a kernel bo, and has no public facing GEM handler. But then this
means we're doing a copy between the FW ringbuffer and the user-supplied
one. However, I remember from past conversations that the goal of a new
implementation was to avoid doing many perfcnt sample copies between the kernel
and UM, because that would require a huge bandwith when the sample period is
small.

> +	control = drm_gem_object_lookup(pfile->drm_file, setup_args->control_handle);
> +	if (!control) {
> +		ret = -EINVAL;
> +		goto cleanup_ringbuf;
> +	}
> +
> +	user_sample_size = session_get_max_sample_size(&ptdev->perf_info) * slots;
> +
> +	if (ringbuffer->size != PFN_ALIGN(user_sample_size)) {
> +		ret = -ENOMEM;
> +		goto cleanup_control;
> +	}

How is information about the max sample size given to UM? I guess through the
values returned through the getparam ioctl(), specifically sample_header_size
and block_header_size?

> +	ret = drm_gem_vmap_unlocked(ringbuffer, &rb_map);
> +	if (ret)
> +		goto cleanup_control;
> +
> +
> +	ret = drm_gem_vmap_unlocked(control, &ctrl_map);
> +	if (ret)
> +		goto cleanup_ring_map;
> +
> +	session->eventfd = eventfd_ctx_fdget(setup_args->fd);
> +	if (IS_ERR_OR_NULL(session->eventfd)) {
> +		ret = PTR_ERR_OR_ZERO(session->eventfd) ?: -EINVAL;
> +		goto cleanup_control_map;
> +	}

I think eventfd_ctx_fdget can only return error values, so there's no need to
check for NULL or ZERO.

> +	em = panthor_perf_create_em(setup_args);
> +	if (IS_ERR_OR_NULL(em)) {
> +		ret = -ENOMEM;
> +		goto cleanup_eventfd;
> +	}
> +
> +	INIT_LIST_HEAD(&session->waiting);
> +	session->extract_idx = ctrl_map.vaddr;
> +	*session->extract_idx = 0;
> +	session->insert_idx = session->extract_idx + 1;
> +	*session->insert_idx = 0;
> +
> +	session->samples = rb_map.vaddr;

I think you might've forgotten this:

        session->ringbuf_slots = slots;

> +	/* TODO This will need validation when we support periodic sampling sessions */
> +	if (setup_args->sample_freq_ns) {
> +		ret = -EOPNOTSUPP;
> +		goto cleanup_em;
> +	}
> +
> +	session->sample_freq_ns = setup_args->sample_freq_ns;
> +	session->user_sample_size = user_sample_size;
> +	session->enabled_counters = em;
> +	session->ring_buf = ringbuffer;
> +	session->control_buf = control;
> +	session->pfile = pfile;
> +
> +	ret = xa_alloc_cyclic(&perf->sessions, &session_id, session, perf->session_range,
> +			&perf->next_session, GFP_KERNEL);

What do we need the next_session index for?

> +	if (ret < 0)
> +		goto cleanup_em;
> +
> +	kref_init(&session->ref);
> +
> +	return session_id;
> +
> +cleanup_em:
> +	kref_put(&em->refs, panthor_perf_destroy_em_kref);
> +
> +cleanup_eventfd:
> +	eventfd_ctx_put(session->eventfd);
> +
> +cleanup_control_map:
> +	drm_gem_vunmap_unlocked(control, &ctrl_map);
> +
> +cleanup_ring_map:
> +	drm_gem_vunmap_unlocked(ringbuffer, &rb_map);
> +
> +cleanup_control:
> +	drm_gem_object_put(control);
> +
> +cleanup_ringbuf:
> +	drm_gem_object_put(ringbuffer);
> +
> +cleanup_session:
> +	devm_kfree(ptdev->base.dev, session);
> +
> +	return ret;
> +}
> +
> +static int session_stop(struct panthor_perf *perf, struct panthor_perf_session *session,
> +		u64 user_data)
> +{
> +	if (!test_bit(PANTHOR_PERF_SESSION_ACTIVE, session->state))
> +		return 0;
> +
> +	const u32 extract_idx = session_read_extract_idx(session);
> +	const u32 insert_idx = session_read_insert_idx(session);
> +
> +	/* Must have at least one slot remaining in the ringbuffer to sample. */
> +	if (WARN_ON_ONCE(!CIRC_SPACE_TO_END(insert_idx, extract_idx, session->ringbuf_slots)))
> +		return -EBUSY;
> +
> +	session->user_data = user_data;
> +
> +	clear_bit(PANTHOR_PERF_SESSION_ACTIVE, session->state);
> +
> +	/* TODO Calls to the FW interface will go here in later patches. */
> +	return 0;
> +}
> +
> +static int session_start(struct panthor_perf *perf, struct panthor_perf_session *session,
> +		u64 user_data)
> +{
> +	if (test_bit(PANTHOR_PERF_SESSION_ACTIVE, session->state))
> +		return 0;
> +
> +	set_bit(PANTHOR_PERF_SESSION_ACTIVE, session->state);
> +
> +	/*
> +	 * For manual sampling sessions, a start command does not correspond to a sample,
> +	 * and so the user data gets discarded.
> +	 */
> +	if (session->sample_freq_ns)
> +		session->user_data = user_data;
> +
> +	/* TODO Calls to the FW interface will go here in later patches. */
> +	return 0;
> +}
> +
> +static int session_sample(struct panthor_perf *perf, struct panthor_perf_session *session,
> +		u64 user_data)
> +{
> +	if (!test_bit(PANTHOR_PERF_SESSION_ACTIVE, session->state))
> +		return -EACCES;
> +
> +	const u32 extract_idx = session_read_extract_idx(session);
> +	const u32 insert_idx = session_read_insert_idx(session);
> +
> +	/* Manual sampling for periodic sessions is forbidden. */
> +	if (session->sample_freq_ns)
> +		return -EINVAL;
> +
> +	/*
> +	 * Must have at least two slots remaining in the ringbuffer to sample: one for
> +	 * the current sample, and one for a stop sample, since a stop command should
> +	 * always be acknowledged by taking a final sample and stopping the session.
> +	 */
> +	if (CIRC_SPACE_TO_END(insert_idx, extract_idx, session->ringbuf_slots) < 2)
> +		return -EBUSY;
> +
> +	session->sample_start_ns = ktime_get_raw_ns();
> +	session->user_data = user_data;
> +
> +	/* TODO Calls to the FW interface will go here in later patches. */
> +	return 0;
> +}
> +
> +static int session_destroy(struct panthor_perf *perf, struct panthor_perf_session *session)
> +{
> +	session_put(session);
> +
> +	return 0;
> +}
> +
> +static int session_teardown(struct panthor_perf *perf, struct panthor_perf_session *session)
> +{
> +	if (test_bit(PANTHOR_PERF_SESSION_ACTIVE, session->state))
> +		return -EINVAL;
> +
> +	if (!list_empty(&session->waiting))
> +		return -EBUSY;
> +
> +	return session_destroy(perf, session);
> +}
> +
> +/**
> + * panthor_perf_session_teardown - Teardown the session associated with the @sid.
> + * @pfile: Open panthor file.
> + * @perf: Handle to the perf control structure.
> + * @sid: Session identifier.
> + *
> + * Destroys a stopped session where the last sample has been explicitly consumed
> + * or discarded. Active sessions will be ignored.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +int panthor_perf_session_teardown(struct panthor_file *pfile, struct panthor_perf *perf, u32 sid)
> +{
> +	int err;
> +	struct panthor_perf_session *session;
> +
> +	xa_lock(&perf->sessions);
> +	session = __xa_store(&perf->sessions, sid, NULL, GFP_KERNEL);

Why not xa_erase() here instead?

> +	if (xa_is_err(session)) {
> +		err = xa_err(session);
> +		goto restore;
> +	}
> +
> +	if (session->pfile != pfile) {
> +		err = -EINVAL;
> +		goto restore;
> +	}
> +
> +	session_get(session);
> +	xa_unlock(&perf->sessions);
> +
> +	err = session_teardown(perf, session);
> +
> +	session_put(session);

I haven't made sure that reference counting is balanced, but noticed session_teardown() is already
putting the session's kref. I'll have a deeper look into it later on.

> +
> +	return err;
> +
> +restore:
> +	__xa_store(&perf->sessions, sid, session, GFP_KERNEL);
> +	xa_unlock(&perf->sessions);
> +
> +	return err;
> +}
> +
> +/**
> + * panthor_perf_session_start - Start sampling on a stopped session.
> + * @pfile: Open panthor file.
> + * @perf: Handle to the panthor perf control structure.
> + * @sid: Session identifier for the desired session.
> + * @user_data: An opaque value passed in from userspace.
> + *
> + * A session counts as stopped when it is created or when it is explicitly stopped after being
> + * started. Starting an active session is treated as a no-op.
> + *
> + * The @user_data parameter will be associated with all subsequent samples for a periodic
> + * sampling session and will be ignored for manual sampling ones in favor of the user data
> + * passed in the PERF_CONTROL.SAMPLE ioctl call.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +int panthor_perf_session_start(struct panthor_file *pfile, struct panthor_perf *perf,
> +		u32 sid, u64 user_data)
> +{
> +	struct panthor_perf_session *session = session_find(pfile, perf, sid);
> +	int err;
> +
> +	if (IS_ERR_OR_NULL(session))
> +		return IS_ERR(session) ? PTR_ERR(session) : -EINVAL;
> +
> +	err = session_start(perf, session, user_data);
> +
> +	session_put(session);
> +
> +	return err;
> +}
> +
> +/**
> + * panthor_perf_session_stop - Stop sampling on an active session.
> + * @pfile: Open panthor file.
> + * @perf: Handle to the panthor perf control structure.
> + * @sid: Session identifier for the desired session.
> + * @user_data: An opaque value passed in from userspace.
> + *
> + * A session counts as active when it has been explicitly started via the PERF_CONTROL.START
> + * ioctl. Stopping a stopped session is treated as a no-op.
> + *
> + * To ensure data is not lost when sampling is stopping, there must always be at least one slot
> + * available for the final automatic sample, and the stop command will be rejected if there is not.
> + *
> + * The @user_data will always be associated with the final sample.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +int panthor_perf_session_stop(struct panthor_file *pfile, struct panthor_perf *perf,
> +		u32 sid, u64 user_data)
> +{
> +	struct panthor_perf_session *session = session_find(pfile, perf, sid);
> +	int err;
> +
> +	if (IS_ERR_OR_NULL(session))
> +		return IS_ERR(session) ? PTR_ERR(session) : -EINVAL;
> +
> +	err = session_stop(perf, session, user_data);
> +
> +	session_put(session);
> +
> +	return err;
> +}
> +
> +/**
> + * panthor_perf_session_sample - Request a sample on a manual sampling session.
> + * @pfile: Open panthor file.
> + * @perf: Handle to the panthor perf control structure.
> + * @sid: Session identifier for the desired session.
> + * @user_data: An opaque value passed in from userspace.
> + *
> + * Only an active manual sampler is permitted to request samples directly. Failing to meet either
> + * of these conditions will cause the sampling request to be rejected. Requesting a manual sample
> + * with a full ringbuffer will see the request being rejected.
> + *
> + * The @user_data will always be unambiguously associated one-to-one with the resultant sample.
> + *
> + * Return: 0 on success, negative error code on failure.
> + */
> +int panthor_perf_session_sample(struct panthor_file *pfile, struct panthor_perf *perf,
> +		u32 sid, u64 user_data)
> +{
> +	struct panthor_perf_session *session = session_find(pfile, perf, sid);
> +	int err;
> +
> +	if (IS_ERR_OR_NULL(session))
> +		return IS_ERR(session) ? PTR_ERR(session) : -EINVAL;
> +
> +	err = session_sample(perf, session, user_data);
> +
> +	session_put(session);
> +
> +	return err;
> +}
> +
> +/**
> + * panthor_perf_session_destroy - Destroy a sampling session associated with the @pfile.
> + * @perf: Handle to the panthor perf control structure.
> + * @pfile: The file being closed.
> + *
> + * Must be called when the corresponding userspace process is destroyed and cannot close its
> + * own sessions. As such, we offer no guarantees about data delivery.
> + */
> +void panthor_perf_session_destroy(struct panthor_file *pfile, struct panthor_perf *perf)
> +{
> +	unsigned long sid;
> +	struct panthor_perf_session *session;
> +
> +	xa_for_each(&perf->sessions, sid, session)
> +	{
> +		if (session->pfile == pfile) {
> +			session_destroy(perf, session);
> +			xa_erase(&perf->sessions, sid);
> +		}
> +	}
>  }
>  
>  /**
> @@ -146,10 +800,17 @@ void panthor_perf_unplug(struct panthor_device *ptdev)
>  	if (!perf)
>  		return;
>  
> -	if (!xa_empty(&perf->sessions))
> +	if (!xa_empty(&perf->sessions)) {
> +		unsigned long sid;
> +		struct panthor_perf_session *session;
> +
>  		drm_err(&ptdev->base,
>  				"Performance counter sessions active when unplugging the driver!");
>  
> +		xa_for_each(&perf->sessions, sid, session)
> +			session_destroy(perf, session);
> +	}
> +
>  	xa_destroy(&perf->sessions);
>  
>  	devm_kfree(ptdev->base.dev, ptdev->perf);
> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> index 8a431431da6b..576d3ad46e6d 100644
> --- a/include/uapi/drm/panthor_drm.h
> +++ b/include/uapi/drm/panthor_drm.h
> @@ -458,6 +458,12 @@ enum drm_panthor_perf_block_type {
>  
>  	/** @DRM_PANTHOR_PERF_BLOCK_SHADER: A shader core counter block. */
>  	DRM_PANTHOR_PERF_BLOCK_SHADER,
> +
> +	/** @DRM_PANTHOR_PERF_BLOCK_LAST: Internal use only. */
> +	DRM_PANTHOR_PERF_BLOCK_LAST = DRM_PANTHOR_PERF_BLOCK_SHADER,
> +
> +	/** @DRM_PANTHOR_PERF_BLOCK_MAX: Internal use only. */
> +	DRM_PANTHOR_PERF_BLOCK_MAX = DRM_PANTHOR_PERF_BLOCK_LAST + 1,
>  };
>  
>  /**
> @@ -1368,6 +1374,44 @@ struct drm_panthor_perf_control {
>  	__u64 pointer;
>  };
>  
> +/**
> + * enum drm_panthor_perf_counter_set - The counter set to be requested from the hardware.
> + *
> + * The hardware supports a single performance counter set at a time, so requesting any set other
> + * than the primary may fail if another process is sampling at the same time.
> + *
> + * If in doubt, the primary counter set has the most commonly used counters and requires no
> + * additional permissions to open.
> + */
> +enum drm_panthor_perf_counter_set {
> +	/**
> +	 * @DRM_PANTHOR_PERF_SET_PRIMARY: The default set configured on the hardware.
> +	 *
> +	 * This is the only set for which all counters in all blocks are defined.
> +	 */
> +	DRM_PANTHOR_PERF_SET_PRIMARY,
> +
> +	/**
> +	 * @DRM_PANTHOR_PERF_SET_SECONDARY: The secondary performance counter set.
> +	 *
> +	 * Some blocks may not have any defined counters for this set, and the block will
> +	 * have the UNAVAILABLE block state permanently set in the block header.
> +	 *
> +	 * Accessing this set requires the calling process to have the CAP_PERFMON capability.
> +	 */
> +	DRM_PANTHOR_PERF_SET_SECONDARY,
> +
> +	/**
> +	 * @DRM_PANTHOR_PERF_SET_TERTIARY: The tertiary performance counter set.
> +	 *
> +	 * Some blocks may not have any defined counters for this set, and the block will have
> +	 * the UNAVAILABLE block state permanently set in the block header. Note that the
> +	 * tertiary set has the fewest defined counter blocks.
> +	 *
> +	 * Accessing this set requires the calling process to have the CAP_PERFMON capability.
> +	 */
> +	DRM_PANTHOR_PERF_SET_TERTIARY,
> +};
>  
>  /**
>   * struct drm_panthor_perf_cmd_setup - Arguments passed to DRM_PANTHOR_IOCTL_PERF_CONTROL
> @@ -1375,13 +1419,17 @@ struct drm_panthor_perf_control {
>   */
>  struct drm_panthor_perf_cmd_setup {
>  	/**
> -	 * @block_set: Set of performance counter blocks.
> +	 * @block_set: Set of performance counter blocks, member of
> +	 *             enum drm_panthor_perf_block_set.
>  	 *
>  	 * This is a global configuration and only one set can be active at a time. If
>  	 * another client has already requested a counter set, any further requests
>  	 * for a different counter set will fail and return an -EBUSY.
>  	 *
>  	 * If the requested set does not exist, the request will fail and return an -EINVAL.
> +	 *
> +	 * Some sets have additional requirements to be enabled, and the setup request will
> +	 * fail with an -EACCES if these requirements are not satisfied.
>  	 */

Is this what we check inside session_validate_set() ?

>  	__u8 block_set;
>  
> -- 
> 2.25.1


Adrian Larumbe

Powered by blists - more mailing lists