Re: [PATCH v9 15/16] drm/panfrost: Fix races between perfcnt and reset sequence

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 23 Sep 2026 02:39:10 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> 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.

> > > +	/*
> > > +	 * 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.

> 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).

> 
> > 	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.

> 
> > 	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);  
> > > +}