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

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

On Thu, 24 Sep 2026 19:09:32 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> Formerly, the reset sequence would race with panfrost_mmu_as_put()
> when tearing down a perfcnt session. On top of that, poking GPU
> registers to program a perfcnt session or obtaining a dump might lead to
> undefined behaviour when done at the same time a reset was ongoing.
> 
> Use the reset r/w semaphore to govern access to the hardware at reset
> time. On top of that, expand the DRM uAPI for the perfcnt DUMP operation
> so that userspace can be made aware of a reset having happened. UM needs
> to know about this condition because counter data is inaccurate after
> a reset, so the best approach is simply to try again.
> 
> The new perfcnt-aware reset sequence also takes care to reestablish
> perfcnt to its original configuration if there was an enabled session,
> or else flags the current session as dead if that failed.
> 
> Also bump DRM driver minor to reflect the new DUMP IOCTL req field.
> 
> Fixes: 73e467f60acd ("drm/panfrost: Consolidate reset handling")
> Fixes: 7786fd108777 ("drm/panfrost: Expose performance counters through unstable ioctls")
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c  |   2 +
>  drivers/gpu/drm/panfrost/panfrost_drv.c     |   3 +-
>  drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 200 ++++++++++++++++++++--------
>  drivers/gpu/drm/panfrost/panfrost_perfcnt.h |   1 +
>  include/uapi/drm/panfrost_drm.h             |   8 +-
>  5 files changed, 155 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 801ca07ccb28..98de04362517 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -483,6 +483,8 @@ void panfrost_device_reset(struct panfrost_device *pfdev, bool enable_job_int)
>  	panfrost_jm_reset_interrupts(pfdev);
>  	if (enable_job_int)
>  		panfrost_jm_enable_interrupts(pfdev);
> +
> +	panfrost_perfcnt_reset(pfdev);
>  }
>  
>  static int panfrost_device_runtime_resume(struct device *dev)
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 571a26b84126..de9b1c115181 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -808,6 +808,7 @@ static const struct file_operations panfrost_drm_driver_fops = {
>   * - 1.6 - adds PANFROST_BO_MAP_WB, PANFROST_IOCTL_SYNC_BO,
>   *	   PANFROST_IOCTL_QUERY_BO_INFO and
>   *	   DRM_PANFROST_PARAM_SELECTED_COHERENCY
> + * - 1.7 - adds PERFCNT_DUMP req state field
>   */
>  static const struct drm_driver panfrost_drm_driver = {
>  	.driver_features	= DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ,
> @@ -820,7 +821,7 @@ static const struct drm_driver panfrost_drm_driver = {
>  	.name			= "panfrost",
>  	.desc			= "panfrost DRM",
>  	.major			= 1,
> -	.minor			= 6,
> +	.minor			= 7,
>  
>  	.gem_create_object	= panfrost_gem_create_object,
>  	.gem_prime_import	= panfrost_gem_prime_import,
> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index b3f71d7fd82a..91983d450c24 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> @@ -11,6 +11,7 @@
>  #include <drm/drm_file.h>
>  #include <drm/drm_gem_shmem_helper.h>
>  #include <drm/panfrost_drm.h>
> +#include <drm/drm_print.h>
>  
>  #include "panfrost_device.h"
>  #include "panfrost_features.h"
> @@ -28,21 +29,31 @@
>  
>  struct panfrost_perfcnt {
>  	struct panfrost_gem_mapping *mapping;
> +	unsigned int counterset;
>  	size_t bosize;
>  	void *buf;
>  	struct panfrost_file_priv *user;
>  	struct mutex lock;
>  	struct completion dump_comp;
> +	unsigned int state;

nit:
	u32 state;

to be consistent with the uAPI field.

> +	bool owns_as_ref;
>  };
>  

[...]

> +static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev, u32 *state)
> +{
> +	struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> +	u64 gpuva = perfcnt->mapping->mmnode.start << PAGE_SHIFT;
> +	int ret;
> +
> +	scoped_guard(rwsem_read, &pfdev->reset.lock) {
> +		*state = perfcnt->state;
> +		if (perfcnt->state & PANFROST_PERFCNT_SESSION_DEAD)
> +			return -EIO;
> +
> +		perfcnt->state = 0;
> +
> +		reinit_completion(&pfdev->perfcnt->dump_comp);
> +
> +		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);
> +	}
> +
> +	/*
> +	 * 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));
> +
> +	/* A reset might come through in the gap between the completion returning and the following
> +	 * check, but because no sample was produced, we don't care to relay the state back to UM
> +	 */
>  	if (!ret)
> -		ret = -ETIMEDOUT;
> -	else if (ret > 0)
> -		ret = 0;
> +		return -ETIMEDOUT;
> +
> +	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;
> +
> +		/* If we faced a reset during our SAMPLE, the user needs to try again. */
> +		if (new_state & PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET)
> +			return -EAGAIN;

Okay, so I think we have a weird situation where sometimes the state
should be ignored when an error is returned (we don't
propagate the state on ETIMEDOUT), and sometimes not (if EAGAIN or EIO
is returned, the state has been propagated and cleared). This is problematic
if INTERRUPTED_BY_RESET is set and EAGAIN is returned, because on the next
DUMP this flag will have disappeared, and if userspace didn't store the
information on the EAGAIN, it wouldn't know the counters are disjoint.

We either need to propagate the state all the time, and document the errno
where userspace should not ignore it (EAGAIN, ETIMEDOUT and EIO), or we need
to preserve the INTERRUPTED_BY_RESET bit when EAGAIN is returned (basically
move the perfcnt->state = 0 below the if (INTERRUPTED_BY_RESET) branch, and
restore the state that was cleared in the first scoped_guard()).

Once addressed, this is

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>