Re: [PATCH v5 10/16] drm/msm: Add basic perfcntr infrastructure

[Anna Maniscalco <anna.maniscalco2000@gmail.com>]

On 5/11/26 14:59, Rob Clark wrote:
> Add the basic infrastructure for tracking assigned perfcntrs.
> 
> Signed-off-by: Rob Clark <robin.clark@oss.qualcomm.com>
> ---
>  drivers/gpu/drm/msm/Makefile               |   1 +
>  drivers/gpu/drm/msm/adreno/adreno_device.c |   8 +-
>  drivers/gpu/drm/msm/adreno/adreno_gpu.c    |   5 +-
>  drivers/gpu/drm/msm/msm_drv.h              |   6 +
>  drivers/gpu/drm/msm/msm_gpu.c              |  12 ++
>  drivers/gpu/drm/msm/msm_gpu.h              |  57 +++++++++-
>  drivers/gpu/drm/msm/msm_perfcntr.c         | 124 +++++++++++++++++++++
>  drivers/gpu/drm/msm/msm_perfcntr.h         |  23 ++++
>  8 files changed, 230 insertions(+), 6 deletions(-)
>  create mode 100644 drivers/gpu/drm/msm/msm_perfcntr.c
> 
> diff --git a/drivers/gpu/drm/msm/Makefile b/drivers/gpu/drm/msm/Makefile
> index 337634e7e247..2466cb32dac5 100644
> --- a/drivers/gpu/drm/msm/Makefile
> +++ b/drivers/gpu/drm/msm/Makefile
> @@ -122,6 +122,7 @@ msm-y += \
>  	msm_gpu_devfreq.o \
>  	msm_io_utils.o \
>  	msm_iommu.o \
> +	msm_perfcntr.o \
>  	msm_rd.o \
>  	msm_ringbuffer.o \
>  	msm_submitqueue.o \
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> index fc38331ce640..7f20320ef66a 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> @@ -307,8 +307,10 @@ MODULE_DEVICE_TABLE(of, dt_match);
>  static int adreno_runtime_resume(struct device *dev)
>  {
>  	struct msm_gpu *gpu = dev_to_gpu(dev);
> -
> -	return gpu->funcs->pm_resume(gpu);
> +	int ret = gpu->funcs->pm_resume(gpu);
> +	if (!ret)
> +		ret = msm_perfcntr_resume(gpu);
> +	return ret;
>  }
>  
>  static int adreno_runtime_suspend(struct device *dev)
> @@ -322,6 +324,8 @@ static int adreno_runtime_suspend(struct device *dev)
>  	 */
>  	WARN_ON_ONCE(gpu->active_submits);
>  
> +	msm_perfcntr_suspend(gpu);
> +
>  	return gpu->funcs->pm_suspend(gpu);
>  }
>  
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index 72b71e9e44f0..ee0bcf985934 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -702,11 +702,10 @@ void adreno_recover(struct msm_gpu *gpu)
>  	struct drm_device *dev = gpu->dev;
>  	int ret;
>  
> -	// XXX pm-runtime??  we *need* the device to be off after this
> -	// so maybe continuing to call ->pm_suspend/resume() is better?
> -
> +	msm_perfcntr_suspend(gpu);
>  	gpu->funcs->pm_suspend(gpu);
>  	gpu->funcs->pm_resume(gpu);
> +	msm_perfcntr_resume(gpu);
>  
>  	ret = msm_gpu_hw_init(gpu);
>  	if (ret) {
> diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
> index e53e4f220bed..f00b2e7aeb91 100644
> --- a/drivers/gpu/drm/msm/msm_drv.h
> +++ b/drivers/gpu/drm/msm/msm_drv.h
> @@ -235,6 +235,12 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>  int msm_ioctl_vm_bind(struct drm_device *dev, void *data,
>  		      struct drm_file *file);
>  
> +int msm_perfcntr_resume(struct msm_gpu *gpu);
> +void msm_perfcntr_suspend(struct msm_gpu *gpu);
> +
> +struct msm_perfcntr_state * msm_perfcntr_init(struct msm_gpu *gpu);
> +void msm_perfcntr_cleanup(struct msm_gpu *gpu);
> +
>  #ifdef CONFIG_DEBUG_FS
>  unsigned long msm_gem_shrinker_shrink(struct drm_device *dev, unsigned long nr_to_scan);
>  #endif
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 1bac70473f80..bf6845e5719e 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -1028,6 +1028,17 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>  
>  	refcount_set(&gpu->sysprof_active, 1);
>  
> +	mutex_init(&gpu->perfcntr_lock);
> +
> +	if (gpu->num_perfcntr_groups > 0) {
> +		gpu->perfcntrs = msm_perfcntr_init(gpu);
> +		if (IS_ERR(gpu->perfcntrs)) {
> +			ret = PTR_ERR(gpu->perfcntrs);
> +			gpu->perfcntrs = NULL;
> +			goto fail;
> +		}
> +	}
> +
>  	return 0;
>  
>  fail:
> @@ -1066,6 +1077,7 @@ void msm_gpu_cleanup(struct msm_gpu *gpu)
>  	}
>  
>  	msm_devfreq_cleanup(gpu);
> +	msm_perfcntr_cleanup(gpu);
>  
>  	platform_set_drvdata(gpu->pdev, NULL);
>  }
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index 19484774f369..be922641a14f 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -25,6 +25,7 @@ struct msm_gem_vm_log_entry;
>  struct msm_gpu_state;
>  struct msm_context;
>  struct msm_perfcntr_group;
> +struct msm_perfcntr_stream;
>  
>  struct msm_gpu_config {
>  	const char *ioname;
> @@ -93,6 +94,13 @@ struct msm_gpu_funcs {
>  	 */
>  	bool (*progress)(struct msm_gpu *gpu, struct msm_ringbuffer *ring);
>  	void (*sysprof_setup)(struct msm_gpu *gpu);
> +
> +	/* Configure perfcntr SELect regs: */
> +	void (*perfcntr_configure)(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
> +				   const struct msm_perfcntr_stream *stream);
> +
> +	/* Flush perfcntrs before reading (optional): */
> +	void (*perfcntr_flush)(struct msm_gpu *gpu);
>  };
>  
>  /* Additional state for iommu faults: */
> @@ -266,6 +274,11 @@ struct msm_gpu {
>  
>  	const struct msm_perfcntr_group *perfcntr_groups;
>  	unsigned num_perfcntr_groups;
> +
> +	struct msm_perfcntr_state *perfcntrs;
> +
> +	/** @perfcntr_lock: protects perfcntr related state */
> +	struct mutex perfcntr_lock;
>  };
>  
>  static inline struct msm_gpu *dev_to_gpu(struct device *dev)
> @@ -311,10 +324,52 @@ static inline bool msm_gpu_active(struct msm_gpu *gpu)
>  	return false;
>  }
>  
> +/**
> + * struct msm_perfcntr_group_state - Tracking for the currently allocated counter state
> + */
> +struct msm_perfcntr_group_state {
> +	/**
> +	 * @allocated_counters:
> +	 *
> +	 * allocated counters for global counter collection.  The
> +	 * corresponding counters are allocated from highest to
> +	 * lowest, to minimize chance of conflict with old userspace
> +	 * allocating from lowest to highest.
> +	 */
> +	unsigned allocated_counters;
> +
> +	/**
> +	 * @countables:
> +	 *
> +	 * The correspnding SELect reg values for the allocated counters
> +	 */
> +	uint32_t countables[];
> +};
> +
> +/**
> + * struct msm_perfcntr_state - overall global perfcntr state
> + */
> +struct msm_perfcntr_state {
> +	/** @stream: current global counter stream if active */
> +	struct msm_perfcntr_stream *stream;
> +
> +	/**
> +	 * @groups: Global perfcntr stream group state.
> +	 *
> +	 * Conceptually this is part of msm_perfcntr_stream state, but is
> +	 * statically pre-allocated when the gpu is initialized to simplify
> +	 * error path cleanup in PERFCNTR_CONFIG ioctl.  (__free(kfree)
> +	 * doesn't really help with variable length arrays of allocated
> +	 * pointers.)
> +	 */
> +	struct msm_perfcntr_group_state *groups[];
> +};
> +
>  static inline bool
>  msm_gpu_sysprof_no_perfcntr_zap(struct msm_gpu *gpu)
>  {
> -	return refcount_read(&gpu->sysprof_active) > 1;
> +	return (refcount_read(&gpu->sysprof_active) > 1) ||
> +		(gpu->perfcntrs && READ_ONCE(gpu->perfcntrs->stream));
>  }
>  
>  static inline bool
> diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c b/drivers/gpu/drm/msm/msm_perfcntr.c
> new file mode 100644
> index 000000000000..dad98c96863c
> --- /dev/null
> +++ b/drivers/gpu/drm/msm/msm_perfcntr.c
> @@ -0,0 +1,124 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + */
> +
> +#include "msm_drv.h"
> +#include "msm_gpu.h"
> +#include "msm_perfcntr.h"
> +
> +static int
> +msm_perfcntr_resume_locked(struct msm_perfcntr_stream *stream)
> +{
> +	return 0;
> +}
> +
> +int
> +msm_perfcntr_resume(struct msm_gpu *gpu)
> +{
> +	if (!gpu->perfcntrs)
> +		return 0;
> +	guard(mutex)(&gpu->perfcntr_lock);
> +	return msm_perfcntr_resume_locked(gpu->perfcntrs->stream);
> +}
> +
> +static void
> +msm_perfcntr_suspend_locked(struct msm_perfcntr_stream *stream)
> +{
> +}
> +
> +void
> +msm_perfcntr_suspend(struct msm_gpu *gpu)
> +{
> +	if (!gpu->perfcntr_groups)
> +		return;
> +	guard(mutex)(&gpu->perfcntr_lock);
> +	msm_perfcntr_suspend_locked(gpu->perfcntrs->stream);
> +}
> +
> +/**
> + * msm_perfcntr_group_idx - map idx of perfcntr group to group_idx
> + * @stream: The global perfcntr stream
> + * @n: The requested group_idx
> + *
> + * The PERFCNTR_CONFIG ioctl requested N counters/countables per perfcntr
> + * group, but the order of groups is not required to match the order they
> + * are defined in the perfcntr tables (which is not stable/UABI, only the
> + * group names are UABI).
> + *
> + * But the order samples are returned in the stream should match the
> + * order they are requested in the PERFCNTR_CONFIG ioctl.  This helper
> + * handles the order remapping.
> + *
> + * Returns an index into gpu->perfcntr_groups[] and perfcntrs->groups[].
> + */
> +uint32_t
> +msm_perfcntr_group_idx(const struct msm_perfcntr_stream *stream, uint32_t n)
> +{
> +	WARN_ON_ONCE(n > stream->nr_groups);
> +	return stream->group_idx[n];
> +}
> +
> +/**
> + * msm_perfcntr_counter_base - get idx of the first counter in group
> + * @stream: The global perfcntr stream
> + * @group_idx: the index of the counter group
> + *
> + * For global counter collection, counters are allocated from the end
> + * (last counter) to minimize the chance of conflict with an old UMD
> + * which predates PERFCNTR_CONFIG ioctl (since UMD assigned from 0..N-1).
As is this comment seems to imply that userspace wont't be allocating from the start anymore which isn't the case.
May I suggest:
For global counter collection, counters are allocated from the end while UMD
allocates them from the start. Since UMD always allocated them from the start
it also minimizes the chance of conflict when using old UMD which predates
PERFCNTR_CONFIG ioctl.

Aside from this nit series is:
Reviewed-by: Anna Maniscalco <anna.maniscalco2000@gmail.com>> + *
> + * Returns the index of first counter to use.  An index into
> + * msm_perfcntr_group::counters[].
> + */
> +uint32_t
> +msm_perfcntr_counter_base(const struct msm_perfcntr_stream *stream, uint32_t group_idx)
> +{
> +	struct msm_gpu *gpu = stream->gpu;
> +	struct msm_perfcntr_state *perfcntrs = gpu->perfcntrs;
> +	unsigned num_counters = gpu->perfcntr_groups[group_idx].num_counters;
> +	unsigned allocated_counters = perfcntrs->groups[group_idx]->allocated_counters;
> +
> +	return num_counters - allocated_counters;
> +}
> +
> +struct msm_perfcntr_state *
> +msm_perfcntr_init(struct msm_gpu *gpu)
> +{
> +	struct msm_perfcntr_state *perfcntrs;
> +	struct device *dev = &gpu->pdev->dev;
> +	size_t sz;
> +
> +	sz = struct_size(perfcntrs, groups, gpu->num_perfcntr_groups);
> +	perfcntrs = devm_kzalloc(dev, sz, GFP_KERNEL);
> +	if (!perfcntrs)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (unsigned i = 0; i < gpu->num_perfcntr_groups; i++) {
> +		const struct msm_perfcntr_group *group =
> +			&gpu->perfcntr_groups[i];
> +
> +		sz = struct_size(perfcntrs->groups[i], countables, group->num_counters);
> +		perfcntrs->groups[i] = devm_kzalloc(dev, sz, GFP_KERNEL);
> +		if (!perfcntrs->groups[i]) {
> +			msm_perfcntr_cleanup(gpu);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +	}
> +
> +	return perfcntrs;
> +}
> +
> +void
> +msm_perfcntr_cleanup(struct msm_gpu *gpu)
> +{
> +	struct msm_perfcntr_state *perfcntrs = gpu->perfcntrs;
> +	struct device *dev = &gpu->pdev->dev;
> +
> +	gpu->perfcntrs = NULL;
> +
> +	for (unsigned i = 0; i < gpu->num_perfcntr_groups; i++)
> +		devm_kfree(dev, perfcntrs->groups[i]);
> +
> +	devm_kfree(dev, perfcntrs);
> +}
> diff --git a/drivers/gpu/drm/msm/msm_perfcntr.h b/drivers/gpu/drm/msm/msm_perfcntr.h
> index 7f0654182496..bfda19e01535 100644
> --- a/drivers/gpu/drm/msm/msm_perfcntr.h
> +++ b/drivers/gpu/drm/msm/msm_perfcntr.h
> @@ -35,6 +35,29 @@ struct msm_perfcntr_group {
>     const struct msm_perfcntr_counter *counters;
>  };
>  
> +/**
> + * struct msm_perfcntr_stream - state for a single open stream fd
> + */
> +struct msm_perfcntr_stream {
> +	/** @gpu: Back-link to the GPU */
> +	struct msm_gpu *gpu;
> +
> +	/** @nr_groups: # of counter groups with enabled counters */
> +	uint32_t nr_groups;
> +
> +	/**
> +	 * @group_idx: array of nr_groups
> +	 *
> +	 * Maps the order of groups in PERFCNTR_CONFIG ioctl to group idx,
> +	 * so that results in the results stream can be ordered to match
> +	 * the ioctl call that setup the stream
> +	 */
> +	uint32_t *group_idx;
> +};
> +
> +uint32_t msm_perfcntr_group_idx(const struct msm_perfcntr_stream *stream, uint32_t n);
> +uint32_t msm_perfcntr_counter_base(const struct msm_perfcntr_stream *stream, uint32_t group_idx);
> +
>  /**
>   * struct msm_perfcntr_context_state - per-msm_context counter state
>   *