On Mon, May 18, 2026 at 6:46 AM Rob Clark <[email protected]> wrote:
>
> 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?
Ok, I think ordering is only maintained within a single core. So
probably the circular-buffer.rst docs are making too stong of an
assumption about x86/TSO..
> 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.
Turns out FD_PREPARE()/fd_publish() work well for this
BR,
-R
> 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
