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

["Adrián Larumbe" <adrian.larumbe@collabora.com>]

On 23.09.2026 10:08, Boris Brezillon wrote:
> 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.

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