On 23.09.2026 10:08, Boris Brezillon wrote:
> On Wed, 23 Sep 2026 02:39:10 +0100
> Adrián Larumbe <[email protected]> wrote:
>
> > > > -static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev)
> > > > +static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev,
> > > > u32 *state)
> > > > {
> > > > - u64 gpuva;
> > > > + struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> > > > + u64 gpuva = perfcnt->mapping->mmnode.start << PAGE_SHIFT;
> > > > int ret;
> > > >
> > > > - reinit_completion(&pfdev->perfcnt->dump_comp);
> > > > - gpuva = pfdev->perfcnt->mapping->mmnode.start << PAGE_SHIFT;
> > > > - gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva));
> > > > - gpu_write(pfdev, GPU_PERFCNT_BASE_HI, upper_32_bits(gpuva));
> > > > - gpu_write(pfdev, GPU_INT_CLEAR,
> > > > - GPU_IRQ_CLEAN_CACHES_COMPLETED |
> > > > - GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
> > > > - gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_SAMPLE);
> > > > + scoped_guard(rwsem_read, &pfdev->reset.lock) {
> > > > + perfcnt->dump_finished = false;
> > > > + *state = 0;
> > > > +
> > > > + if (!perfcnt->owns_as_ref) {
> > > > + *state = PANFROST_PERFCNT_SESSION_DEAD;
> > > > + return -EIO;
> > > > + }
> > > > +
> > > > + if (perfcnt->reset_happened) {
> > > > + *state =
> > > > PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET;
> > > > + perfcnt->reset_happened = false;
> > > > + }
> > >
> > > *state = perfcnt->state;
> >
> > I'm thinking maybe we don't need to set the ioctl output state here. Even
> > if there was
> > a reset, if we recovered from it and didn't lose the AS reference, we might
> > be able to
> > go forward as usual. In that case, counters would be measured since the
> > latest reset
> > rather than the last successful dump, although maybe this is not an issue.
>
> I think userspace needs to know about INTERRUPTED_BY_RESET, at the very
> least. But honestly, I'd keep it simple and propagate the state
> regardless of the flags userspace might or might not care about.
Agreed.
> > > > + /*
> > > > + * Here we release the reset semaphore because perfcnt should
> > > > not get in the way
> > > > + * of a HW reset. Besides, a legitimate reset might be issued
> > > > during the wait.
> > > > + */
> > > > ret =
> > > > wait_for_completion_interruptible_timeout(&pfdev->perfcnt->dump_comp,
> > > >
> > > > msecs_to_jiffies(1000));
> > > > - if (!ret)
> > > > - ret = -ETIMEDOUT;
> > > > - else if (ret > 0)
> > > > - ret = 0;
> > > > +
> > > > + scoped_guard(rwsem_read, &pfdev->reset.lock) {
> > > > + /* Either sample finished or reset happened */
> > > > + if (ret > 0) {
> > > > + ret = perfcnt->dump_finished ? 0 :
> > > > + perfcnt->owns_as_ref ? -EAGAIN : -EIO;
> > > > +
> > > > + } else if (!ret) {
> > > > + ret = -ETIMEDOUT;
> > > > + }
> > > > +
> > > > + if (perfcnt->reset_happened)
> > > > + *state |=
> > > > PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET;
> > > > + if (!perfcnt->owns_as_ref)
> > > > + *state |= PANFROST_PERFCNT_SESSION_DEAD;
> > > > + }
> > >
> > > if (!ret)
> > > return -ETIMEDOUT;
> >
> > I was doing this check inside the guard because, in the time between the
> > completion
> > is signalled and the semaphore locked, another reset could come right
> > through.
>
> I wouldn't worry about late signaling to be honest, the timeout is
> 1sec, which is way more than I would expect a DUMP request to take in
> a normal situation. Now, we might want to propagate the state in that
> case too, so it might make sense to return ETIMEDOUT inside the
> scoped_guard, after the state has been copied.
On a second thought, I agree with you. Not only is the completion timeout much
longer
than the usual sample period, but also, in the event of a timeout (and no
counters being
copied back to UM), I don't think a perfcnt user would care much about the
reset state.
> > And then because I believed counter values were accumulated rather than
> > reset between
> > consecutive dumps, it was best to notify the user as soon as possible, even
> > if that
> > meant losing one legitimate frame.
> >
> > > scoped_guard(rwsem_read, &pfdev->reset.lock) {
> > > u32 new_state = perfcnt->state;
> > >
> > > *state |= new_state;
> > > if (new_state & PANFROST_PERFCNT_SESSION_DEAD)
> > > return -EIO;
> > >
> > > perfcnt->state = 0;
> >
> > I think this should not be here, because we're already resetting the state
> > before
> > running the PERFCNT_SAMPLE GPU command, and also we need to check it right
> > below.
>
> I think we want to reset in both places, especially if we propagate the
> state before the DUMP too, otherwise we might leave the
> INTERRUPTED_BY_RESET bit behind even though we already informed
> userspace about this event in the previous DUMP request. And yes, the
> following test should have been on new_state instead of perfcnt->state,
> indeed.
> >
> > > /* If we faced a reset during our SAMPLE, the user needs to try
> > > again. */
> > > if (perfcnt->state &
> > > PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET)
> > > return -EAGAIN;
> > > }
> >
> > The main reason I added the panfrost_perfcnt::dump_finished flag was that
> > when a reset happens,
> > the GPU IRQ handler might've been already running in response to a finished
> > perfcnt sample
> > command. However, the reset sequence might come off first and cancel the
> > completion, leaving
> > us with a ret > 0 despite sampling having succeeded.
> >
> > I'm thinking rather than trying to sort of patch this up precariously as I
> > did in this revision,
> > I'll leave it the way you suggested and address the race between the GPU
> > IRQ handler and the
> > reset sequence in a future series.
>
> Yes, that's exactly what needs to be done: reset should synchronize
> IRQs, mask/suspend them while the reset sequence is happening, and
> unmask them when it's done. As for the completion of a DUMP request
> interrupted by a RESET, we want a ret > 0 (AKA no-timeout) in that
> case, but the perfcnt::state should be updated prior to that, so that
> this function knows exactly what to expect from this completion: if
> SESSION_INTERRUPTED_BY_RESET is set, it's likely that the DUMP didn't
> complete which is why I return EAGAIN in that case, but we can also have
> a dedicated flag for DUMP_COMPLETE if you want to be accurate (when
> set, DUMP is effective, when not set EAGAIN).
I'll leave dealing with the problem of synchronisation between reset and the IRQ
handlers to a later patch series, so in the meantime,
SESSION_INTERRUPTED_BY_RESET
should always lead to EAGAIN being returned.
> >
> > > return 0;
> >
> > We still need to return -ERESTARTSYS when
> > wait_for_completion_interruptible_timeout()
> > is interrupted from UM.
>
> Oh, absolutely.
>
> >
> > > > +void panfrost_perfcnt_reset(struct panfrost_device *pfdev)
> > > > +{
> > > > + struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> > > > +
> > > > + if (drm_WARN_ON(&pfdev->base, !perfcnt))
> > > > + return;
> > > > +
> > > > + lockdep_assert_held(&pfdev->reset.lock);
> > > > +
> > > > + if (!perfcnt->user)
> > > > + return;
> > > > +
> > > > + perfcnt->owns_as_ref = !panfrost_perfcnt_hw_enable(pfdev);
> > > > + perfcnt->reset_happened = true;
> > > > + complete(&perfcnt->dump_comp);
> > >
> > > /* All active AS are released during the MMU post_reset. */
> > > perfcnt->owns_as_ref = false;
> >
> > I'm guessing this should go away as soon as I move it into
> > panfrost_perfcnt_hw_enable().
>
> If you reset it at the beginning of panfrost_perfcnt_hw_enable(), sure,
> but I think it's easier to assume that when
> panfrost_perfcnt_hw_enable() is called, the AS is not owned, and reset
> it here instead.
Noted.
> >
> > > perfcnt->state |= PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET;
> > > if (panfrost_perfcnt_hw_enable(pfdev))
> > > perfcnt->state |= PANFROST_PERFCNT_SESSION_DEAD;
> > >
> > > /* Unblock pending sample requests. */
> > > complete(&perfcnt->dump_comp);
> > > > +}
Adrian Larumbe
