On Sun, May 17, 2026 at 3:54 AM Akhil P Oommen <[email protected]> wrote:
>
> On 5/14/2026 7:10 PM, Rob Clark wrote:
> > Add new UABI and implementation of PERFCNTR_CONFIG ioctl.
> >
> > A bit more work is required to configure the pwrup_reglist for the GMU
> > to restore SELect regs on exist of IFPC, before we can stop disabling
> > IFPC while global counter collection. This will follow in a later
> > commit, but will be transparent to userspace.
> >
> > Signed-off-by: Rob Clark <[email protected]>
> > Reviewed-by: Anna Maniscalco <[email protected]>
> > ---
> > drivers/gpu/drm/msm/msm_drv.c | 1 +
> > drivers/gpu/drm/msm/msm_drv.h | 2 +
> > drivers/gpu/drm/msm/msm_gpu.h | 3 +
> > drivers/gpu/drm/msm/msm_perfcntr.c | 516 +++++++++++++++++++++++++++++
> > drivers/gpu/drm/msm/msm_perfcntr.h | 51 +++
> > include/uapi/drm/msm_drm.h | 48 +++
> > 6 files changed, 621 insertions(+)
> >
[snip]
> > diff --git a/drivers/gpu/drm/msm/msm_perfcntr.c
> > b/drivers/gpu/drm/msm/msm_perfcntr.c
> > index 04407260a4e1..d6716ce0657a 100644
> > --- a/drivers/gpu/drm/msm/msm_perfcntr.c
> > +++ b/drivers/gpu/drm/msm/msm_perfcntr.c
[snip]
> > +static ssize_t
> > +msm_perfcntrs_stream_read(struct file *file, char __user *buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct msm_perfcntr_stream *stream = file->private_data;
> > + int ret;
> > +
> > + if (!(file->f_flags & O_NONBLOCK)) {
> > + ret = wait_event_interruptible(stream->poll_wq,
> > + fifo_count(stream) > 0);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + guard(mutex)(&stream->read_lock);
> > +
> > + struct circ_buf *fifo = &stream->fifo;
> > + const char *fptr = &fifo->buf[fifo->tail];
> > +
> > + /*
> > + * Note that smp_load_acquire() is not strictly required
> > + * as CIRC_CNT_TO_END() does not access the head more than
> > + * once.
> > + */
> > + count = min_t(size_t, count, fifo_count_to_end(stream));
>
> We need smp_load_acquire() to ensure all prior writes from the producer
> side are visible before the 'head' is read.
circular-buffer.rst documents the case that smp_load_acquired() is not
needed. Maybe that is assuming TSO? But I think even without TSO
address and data dependencies should be maintained, so this should
still be true on arm64?
We've been using the same in a couple other places in msm, fwiw.
> > + if (copy_to_user(buf, fptr, count))
> > + return -EFAULT;
> > +
> > + smp_store_release(&fifo->tail, (fifo->tail + count) &
> > (stream->fifo_size - 1));
> > + *ppos += count;
> > +
> > + return count;
> > +}
[snip]
> > +int
> > +msm_ioctl_perfcntr_config(struct drm_device *dev, void *data, struct
> > drm_file *file)
> > +{
> > + struct msm_drm_private *priv = dev->dev_private;
> > + const struct drm_msm_perfcntr_config *args = data;
> > + struct msm_context *ctx = file->driver_priv;
> > + struct msm_gpu *gpu = priv->gpu;
> > + int stream_fd = 0;
> > +
> > + if (!gpu || !gpu->num_perfcntr_groups)
> > + return -ENXIO;
> > +
> > + struct msm_perfcntr_state *perfcntrs = gpu->perfcntrs;
> > +
> > + /*
> > + * Validate args that don't require locks/power first:
> > + */
> > +
> > + if (args->flags & ~MSM_PERFCNTR_FLAGS)
> > + return UERR(EINVAL, dev, "invalid flags");
> > +
> > + if (args->nr_groups && !args->group_stride)
> > + return UERR(EINVAL, dev, "invalid group_stride");
> > +
> > + if (args->flags & MSM_PERFCNTR_STREAM) {
> > + if (!perfmon_capable())
> > + return UERR(EPERM, dev, "invalid permissions");
> > + if (!args->nr_groups)
> > + return UERR(EINVAL, dev, "invalid nr_groups");
> > + if (!args->period)
> > + return UERR(EINVAL, dev, "invalid sampling period");
> > + } else {
> > + if (args->period)
> > + return UERR(EINVAL, dev, "sampling period not
> > allowed");
> > + if (args->bufsz_shift)
> > + return UERR(EINVAL, dev, "sample buf size not
> > allowed");
> > + }
> > +
> > + if (args->nr_groups && !args->groups)
> > + return UERR(EINVAL, dev, "no groups");
> > +
> > + /*
> > + * To avoid iterating over the groups multiple times, allocate and
> > setup
> > + * both a ctx and global stream object. Only one of the two will be
> > + * kept in the end.
> > + */
> > +
> > + struct msm_perfcntr_context_state *perfctx __free(kfree) = kzalloc(
> > + struct_size(perfctx, reserved_counters,
> > gpu->num_perfcntr_groups),
> > + GFP_KERNEL);
> > + if (!perfctx)
> > + return -ENOMEM;
> > +
> > + struct msm_perfcntr_stream *stream __free(kfree) =
> > + kzalloc(sizeof(*stream), GFP_KERNEL);
>
> How about using GFP_KERNEL_ACCOUNT everywhere in the ioctl path to
> account the allocated memory to the process?
Maybe the circular buffer itself should be? But the size is bounded,
and I don't see much other use in drm..
> > + if (!stream)
> > + return -ENOMEM;
> > +
> > + uint32_t *group_idx __free(kfree) =
> > + kcalloc(args->nr_groups, sizeof(uint32_t), GFP_KERNEL);
> > + if (!group_idx)
> > + return -ENOMEM;
> > +
> > + stream->gpu = gpu;
> > + stream->sample_period_ns = args->period;
> > + stream->nr_groups = args->nr_groups;
> > + stream->fifo_size = 1 << args->bufsz_shift;
> > +
> > + mutex_init(&stream->read_lock);
> > +
> > + guard(mutex)(&gpu->perfcntr_lock);
> > +
> > + if (args->flags & MSM_PERFCNTR_STREAM) {
> > + if (perfcntrs->stream)
> > + return UERR(EBUSY, dev, "perfcntr stream already
> > open");
> > + }
> > +
> > + size_t bufsz = 16; /* header size includes seqno and 64b timestamp:
> > */
> > + int ret = 0;
> > +
> > + for (unsigned i = 0; i < args->nr_groups; i++) {
> > + struct drm_msm_perfcntr_group g = {0};
> > + void __user *userptr =
> > + u64_to_user_ptr(args->groups + (i *
> > args->group_stride));
> > +
> > + if (copy_from_user(&g, userptr, args->group_stride))
>
> use copy_struct_from_user?
I think this would cause a new userspace on old kernel to fail,
instead of simply ignoring new fields added at the end of the struct.
Which for better or worse is different from how drm_ioctl() works.
>
> > + return -EFAULT;
> > +
> > + if (g.pad)
> > + return UERR(EINVAL, dev, "groups[%d]: invalid pad",
> > i);
> > +
> > + int idx = get_group_idx(gpu, g.group_name,
> > sizeof(g.group_name));
> > +
> > + if (idx < 0)
> > + return UERR(EINVAL, dev, "groups[%d]: unknown group",
> > i);
> > +
> > + if (g.nr_countables > gpu->perfcntr_groups[idx].num_counters)
> > + return UERR(EINVAL, dev, "groups[%d]: too many
> > counters", i);
> > +
> > + if (args->flags & MSM_PERFCNTR_STREAM) {
> > + if (g.nr_countables && !g.countables)
> > + return UERR(EINVAL, dev, "groups[%d]: no
> > countables", i);
> > + } else {
> > + if (g.countables)
> > + return UERR(EINVAL, dev, "groups[%d]:
> > countables should be NULL", i);
> > + }
> > +
> > + int avail_counters = get_available_counters(gpu, idx,
> > args->flags);
> > + if (g.nr_countables > avail_counters) {
> > + /*
> > + * Defer error return until we process all groups, in
> > + * case there are other E2BIG groups:
> > + */
> > + ret = UERR(E2BIG, dev, "groups[%d]: too few counters
> > available", i);
> > +
> > + if (args->flags & MSM_PERFCNTR_UPDATE) {
> > + /* Let userspace know how many counters are
> > actually avail: */
> > + g.nr_countables = avail_counters;
> > + if (copy_to_user(userptr, &g,
> > args->group_stride))
> > + return -EFAULT;
> > + }
> > + }
> > +
> > + group_idx[i] = idx;
> > + perfctx->reserved_counters[idx] = g.nr_countables;
> > +
> > + if (args->flags & MSM_PERFCNTR_STREAM) {
> > + perfcntrs->groups[idx]->allocated_counters =
> > g.nr_countables;
> > +
> > + size_t sz = sizeof(uint32_t) * g.nr_countables;
> > + void __user *userptr = u64_to_user_ptr(g.countables);
> > +
> > + if
> > (copy_from_user(perfcntrs->groups[idx]->countables, userptr, sz))
> > + return -EFAULT;
> > +
> > + /* Samples are 64b per countable: */
> > + bufsz += 2 * sz;
> > + }
> > + }
> > +
> > + if (ret)
> > + return ret;
> > +
> > + if (args->flags & MSM_PERFCNTR_STREAM) {
> > + /*
> > + * Validate requested buffer size is large enough for at least
> > + * a single sample period.
> > + *
> > + * Note the circ_buf implementation needs to be 1 byte larger
> > + * than max it can hold (see CIRC_SPACE()).
> > + */
> > + if (stream->fifo_size <= bufsz)
> > + return UERR(EINVAL, dev, "required buffer size: %zu",
> > bufsz);
> > +
> > + if (stream->fifo_size > SZ_128M)
> > + return UERR(EINVAL, dev, "buffer size too big
> > (>128M): %zu", bufsz);
> > +
> > + /* There aren't enough counters to hit this limit: */
> > + WARN_ON(bufsz > SZ_128M);
> > +
> > + stream->period_size = bufsz;
> > +
> > + void *buf __free(kfree) =
> > + kmalloc(1 << args->bufsz_shift, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + stream_fd = anon_inode_getfd("[msm_perfcntrs]", &stream_fops,
> > stream, 0);
>
> This call makes the fd visible to the userspace immediately. So better
> to move it after all initializations. Ie, after
> msm_perfcntr_resume_locked().
Hmm, but this doesn't play too nicely with cleanup.h stuff, because
the no_free_ptr() stuff must come after the last point of failure.
But I think instead we can use get_unused_fd_flags() + fd_install() to
defer exposing the fd.. I'll try this.
BR,
-R
>
> > + if (stream_fd < 0)
> > + return stream_fd;
> > +
> > + INIT_WORK(&stream->sel_work, sel_worker);
> > + kthread_init_work(&stream->sample_work, sample_worker);
> > + init_waitqueue_head(&stream->poll_wq);
> > + hrtimer_setup(&stream->sample_timer, sample_timer,
> > + CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > +
> > + stream->sel_fence = ++perfcntrs->sel_seqno;
> > + stream->group_idx = no_free_ptr(group_idx);
> > + stream->fifo.buf = no_free_ptr(buf);
> > + perfcntrs->stream = no_free_ptr(stream);
> > +
> > + msm_perfcntr_resume_locked(perfcntrs->stream);
> > + } else {
> > + kfree(ctx->perfctx);
> > + ctx->perfctx = no_free_ptr(perfctx);
> > + }
> > +
> > + return stream_fd;
> > +}
> > +
> > /**
> > * msm_perfcntr_group_idx - map idx of perfcntr group to group_idx
> > * @stream: The global perfcntr stream
