On Sat, 12 Sep 2026 00:28:16 +0100
Adrián Larumbe <[email protected]> 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, because
> that means counters will go back to 0 and can no longer be accumulated
> to values previously kept in user space.
In GPU_PERFCNT_CFG_MODE_MANUAL mode (which is the one we use), internal
counters are always cleared after each DUMP request. So, it's not so
much that counters can't be accumulated after a RESET, it's more that
we've lost data in the process, making this very sample inaccurate
(counters lower than they should be).
>
> 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.
>
> Fixes: 73e467f60acd ("drm/panfrost: Consolidate reset handling")
> Fixes: 7786fd108777 ("drm/panfrost: Expose performance counters through
> unstable ioctls")
> Signed-off-by: Adrián Larumbe <[email protected]>
> ---
> drivers/gpu/drm/panfrost/panfrost_device.c | 2 +
> drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 201
> +++++++++++++++++++---------
> drivers/gpu/drm/panfrost/panfrost_perfcnt.h | 1 +
> include/uapi/drm/panfrost_drm.h | 8 +-
> 4 files changed, 151 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c
> b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 6c65feae63aa..e774f61c642b 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -479,6 +479,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_perfcnt.c
> b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index b3f71d7fd82a..9847657179a5 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,11 +29,15 @@
>
> 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;
> + bool reset_happened;
> + bool dump_finished;
Why not store the state flags directly instead of these
dump_finished/reset_happened booleans?
> + bool owns_as_ref;
> };
>
> static void panfrost_perfcnt_hw_disable(struct panfrost_device *pfdev)
> @@ -47,36 +52,113 @@ static void panfrost_perfcnt_hw_disable(struct
> panfrost_device *pfdev)
>
> void panfrost_perfcnt_clean_cache_done(struct panfrost_device *pfdev)
> {
> + pfdev->perfcnt->dump_finished = true;
> complete(&pfdev->perfcnt->dump_comp);
> }
>
> void panfrost_perfcnt_sample_done(struct panfrost_device *pfdev)
> {
> - if (pfdev->features.selected_coherency != COHERENCY_ACE)
> + if (pfdev->features.selected_coherency != COHERENCY_ACE) {
> gpu_write(pfdev, GPU_CMD, GPU_CMD_CLEAN_CACHES);
> - else
> + } else {
> + pfdev->perfcnt->dump_finished = true;
> complete(&pfdev->perfcnt->dump_comp);
> + }
> +}
> +
> +static int panfrost_perfcnt_hw_enable(struct panfrost_device *pfdev)
> +{
> + struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> + u32 cfg, as;
> + int ret;
> +
> + ret = panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu);
> + if (ret < 0)
> + return ret;
> +
> + as = ret;
> + cfg = GPU_PERFCNT_CFG_AS(as) |
> + GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_MANUAL);
> +
> + /*
> + * Bifrost GPUs have 2 set of counters, but we're only interested by
> + * the first one for now.
> + */
> + if (panfrost_model_is_bifrost(pfdev))
> + cfg |= GPU_PERFCNT_CFG_SETSEL(perfcnt->counterset);
> +
> + gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0xffffffff);
> + gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0xffffffff);
> + gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0xffffffff);
> +
> + /*
> + * Due to PRLAM-8186 we need to disable the Tiler before we enable HW
> + * counters.
> + */
> + if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
> + gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
> + else
> + gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
> +
> + gpu_write(pfdev, GPU_PERFCNT_CFG, cfg);
> +
> + if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
> + gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
> +
> + return 0;
> }
>
> -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;
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));
> - 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;
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 (perfcnt->state &
PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET)
return -EAGAIN;
}
return 0;
>
> return ret;
> }
> @@ -87,9 +169,8 @@ static int panfrost_perfcnt_enable_locked(struct
> panfrost_device *pfdev,
> {
> struct panfrost_file_priv *user = file_priv->driver_priv;
> struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> - struct iosys_map map;
> struct drm_gem_shmem_object *bo;
> - u32 cfg, as;
> + struct iosys_map map;
> int ret;
>
> if (user == perfcnt->user)
> @@ -122,54 +203,31 @@ static int panfrost_perfcnt_enable_locked(struct
> panfrost_device *pfdev,
> ret = drm_gem_vmap(&bo->base, &map);
> if (ret)
> goto err_put_mapping;
> +
> perfcnt->buf = map.vaddr;
> + perfcnt->counterset = counterset;
>
> panfrost_gem_internal_set_label(&bo->base, "Perfcnt sample buffer");
>
> - /*
> - * Clear the counters to start from a fresh state.
> - */
> - gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
> - gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_CLEAR);
> -
> - ret = panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu);
> - if (ret < 0)
> - goto err_vunmap;
> -
> - as = ret;
> - cfg = GPU_PERFCNT_CFG_AS(as) |
> - GPU_PERFCNT_CFG_MODE(GPU_PERFCNT_CFG_MODE_MANUAL);
> -
> - /*
> - * Bifrost GPUs have 2 set of counters, but we're only interested by
> - * the first one for now.
> - */
> - if (panfrost_model_is_bifrost(pfdev))
> - cfg |= GPU_PERFCNT_CFG_SETSEL(counterset);
> -
> - gpu_write(pfdev, GPU_PRFCNT_JM_EN, 0xffffffff);
> - gpu_write(pfdev, GPU_PRFCNT_SHADER_EN, 0xffffffff);
> - gpu_write(pfdev, GPU_PRFCNT_MMU_L2_EN, 0xffffffff);
> -
> - /*
> - * Due to PRLAM-8186 we need to disable the Tiler before we enable HW
> - * counters.
> - */
> - if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
> - gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0);
> - else
> - gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
> + scoped_guard(rwsem_read, &pfdev->reset.lock) {
> + /*
> + * Clear the counters to start from a fresh state.
> + */
> + gpu_write(pfdev, GPU_INT_CLEAR,
> GPU_IRQ_PERFCNT_SAMPLE_COMPLETED);
> + gpu_write(pfdev, GPU_CMD, GPU_CMD_PERFCNT_CLEAR);
>
> - gpu_write(pfdev, GPU_PERFCNT_CFG, cfg);
> + ret = panfrost_perfcnt_hw_enable(pfdev);
> + if (ret)
> + goto err_vunmap;
>
> - if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186))
> - gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff);
> + perfcnt->reset_happened = false;
> + perfcnt->owns_as_ref = true;
This should probably be set in panfrost_perfcnt_hw_enable(), just after the
panfrost_mmu_as_get() call.
> + perfcnt->user = user;
> + }
>
> /* The BO ref is retained by the mapping. */
> drm_gem_object_put(&bo->base);
>
> - perfcnt->user = user;
> -
> return 0;
>
> err_vunmap:
> @@ -195,13 +253,16 @@ static int panfrost_perfcnt_disable_locked(struct
> panfrost_device *pfdev,
> if (user != perfcnt->user)
> return -EINVAL;
>
> - panfrost_perfcnt_hw_disable(pfdev);
> + scoped_guard(rwsem_read, &pfdev->reset.lock) {
> + panfrost_perfcnt_hw_disable(pfdev);
> + if (perfcnt->owns_as_ref)
> + panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu);
Similarly, I think it'd be preferable to have this as_put() inside
perfcnt_hw_disable().
> + perfcnt->user = NULL;
> + }
>
> - perfcnt->user = NULL;
> drm_gem_vunmap(&perfcnt->mapping->obj->base.base, &map);
> perfcnt->buf = NULL;
> panfrost_gem_close(&perfcnt->mapping->obj->base.base, file_priv);
> - panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu);
> panfrost_gem_mapping_put(perfcnt->mapping);
> perfcnt->mapping = NULL;
> pm_runtime_put_autosuspend(pfdev->base.dev);
> @@ -249,13 +310,16 @@ int panfrost_ioctl_perfcnt_dump(struct drm_device *dev,
> void *data,
> if (ret)
> return ret;
>
> + if (req->pad)
> + return -EINVAL;
> +
> mutex_lock(&perfcnt->lock);
> if (perfcnt->user != file_priv->driver_priv) {
> ret = -EINVAL;
> goto out;
> }
>
> - ret = panfrost_perfcnt_dump_locked(pfdev);
> + ret = panfrost_perfcnt_dump_locked(pfdev, &req->state);
> if (ret)
> goto out;
>
> @@ -338,3 +402,20 @@ void panfrost_perfcnt_fini(struct panfrost_device *pfdev)
> /* Disable everything before leaving. */
> panfrost_perfcnt_hw_disable(pfdev);
> }
> +
> +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;
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);
> +}
> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
> b/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
> index 8bbcf5f5fb33..8b9bc704b634 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.h
> @@ -14,5 +14,6 @@ int panfrost_ioctl_perfcnt_enable(struct drm_device *dev,
> void *data,
> struct drm_file *file_priv);
> int panfrost_ioctl_perfcnt_dump(struct drm_device *dev, void *data,
> struct drm_file *file_priv);
> +void panfrost_perfcnt_reset(struct panfrost_device *pfdev);
>
> #endif
> diff --git a/include/uapi/drm/panfrost_drm.h b/include/uapi/drm/panfrost_drm.h
> index 50d5337f35ef..97e001040543 100644
> --- a/include/uapi/drm/panfrost_drm.h
> +++ b/include/uapi/drm/panfrost_drm.h
> @@ -47,7 +47,7 @@ extern "C" {
> * them for anything but debugging purpose.
> */
> #define DRM_IOCTL_PANFROST_PERFCNT_ENABLE DRM_IOW(DRM_COMMAND_BASE +
> DRM_PANFROST_PERFCNT_ENABLE, struct drm_panfrost_perfcnt_enable)
> -#define DRM_IOCTL_PANFROST_PERFCNT_DUMP
> DRM_IOW(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_DUMP, struct
> drm_panfrost_perfcnt_dump)
> +#define DRM_IOCTL_PANFROST_PERFCNT_DUMP
> DRM_IOWR(DRM_COMMAND_BASE + DRM_PANFROST_PERFCNT_DUMP, struct
> drm_panfrost_perfcnt_dump)
>
> #define PANFROST_JD_REQ_FS (1 << 0)
> #define PANFROST_JD_REQ_CYCLE_COUNT (1 << 1)
> @@ -270,8 +270,14 @@ struct drm_panfrost_perfcnt_enable {
> __u32 counterset;
> };
>
> +/* Perfcnt dump state as influenced by a HW reset */
> +#define PANFROST_PERFCNT_SESSION_DEAD (1 << 0)
> +#define PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET (1 << 1)
> +
> struct drm_panfrost_perfcnt_dump {
> __u64 buf_ptr;
> + __u32 state;
> + __u32 pad; /* MBZ */
> };
>
> /* madvise provides a way to tell the kernel in case a buffers contents
>
