On 01.09.2026 16:03, Boris Brezillon wrote:
> On Fri, 28 Aug 2026 21:56:56 +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.
> >
> > The new perfcnt-aware reset sequence also takes care to reestablish
> > perfcnt to its original configuration if there was an enabled session.
> >
> > Signed-off-by: Adrián Larumbe <[email protected]>
> > ---
> > drivers/gpu/drm/panfrost/panfrost_device.c | 1 +
> > drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 189
> > ++++++++++++++++++++--------
> > drivers/gpu/drm/panfrost/panfrost_perfcnt.h | 1 +
> > include/uapi/drm/panfrost_drm.h | 3 +-
> > 4 files changed, 140 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c
> > b/drivers/gpu/drm/panfrost/panfrost_device.c
> > index c8c5dc26b03d..471bd4b037e6 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> > @@ -498,6 +498,7 @@ void panfrost_device_reset(struct panfrost_device
> > *pfdev)
> > panfrost_gpu_power_on(pfdev);
> > panfrost_mmu_reset(pfdev);
> > panfrost_jm_reset_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..0564aa2753f9 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"
> > @@ -25,14 +26,18 @@
> > #define BYTES_PER_COUNTER 4
> > #define BLOCKS_PER_COREGROUP 8
> > #define V4_SHADERS_PER_COREGROUP 4
> > +#define PERFCNT_DUMP_MAX_RETRIES 5
> >
> > 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 reset_failed;
> > };
> >
> > static void panfrost_perfcnt_hw_disable(struct panfrost_device *pfdev)
> > @@ -58,25 +63,107 @@ void panfrost_perfcnt_sample_done(struct
> > panfrost_device *pfdev)
> > complete(&pfdev->perfcnt->dump_comp);
> > }
> >
> > -static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfdev)
> > +static int panfrost_perfcnt_hw_enable(struct panfrost_device *pfdev)
> > {
> > - u64 gpuva;
> > + struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> > + u32 cfg, as;
> > 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);
> > + 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,
> > + u64 *reset_happened)
> > +{
> > + struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> > + u64 gpuva = perfcnt->mapping->mmnode.start << PAGE_SHIFT;
> > + s64 retries = PERFCNT_DUMP_MAX_RETRIES;
> > + bool retry;
> > + int ret;
> > +
> > +dump_retry:
> > + retry = false;
> > + scoped_guard(rwsem_read, &pfdev->reset.lock) {
> > + *reset_happened = perfcnt->reset_happened;
> > + perfcnt->reset_happened = false;
> > + if (perfcnt->reset_failed) {
>
> I'd rather treat that as a terminal fault (returning -EIO, and maybe
> reflecting the perf session as dead through some state flag) in order
> to force the user to re-create a session. The only case where it would
> fail is if as_get() fails, and it's not supposed to fail after a reset.
> This should simplify the dump logic quite a bit.
Does that mean forcing the user to go through the disable/enable dance?
I thought because in the event of a reset, UM would still like to claim
ownership
of perfcnt, recovering within the dump ioctl itself and notifying that a reset
happened
so that it can respond to counters also being reset was the quickest way around.
> > + ret = panfrost_perfcnt_hw_enable(pfdev);
> > + if (ret)
> > + return ret;
> > + perfcnt->reset_failed = false;
> > + }
> > +
> > + 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. The downside is in the gap between the completion is
> > flagged
> > + * by the GPU IRQ handler and the sempahore taken once again, yet
> > another reset
> > + * could happen, which would lead to a valid sample being discarded.
> > This is
> > + * a solution of compromise between ensuring synchronisation with the
> > reset
> > + * sequence and letting it happen as quickly as possible for jobs to be
> > rerun.
> > + */
> > +
> > 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) {
> > + if (ret > 0) {
> > + if (perfcnt->reset_happened) {
> > + if (--retries >= 0)
> > + retry = true;
> > + else
> > + ret = -EBUSY;
> > + } else {
> > + ret = 0;
> > + }
> > + } else if (!ret) {
> > + ret = -ETIMEDOUT;
> > + }
> > + }
>
> I think it's fine to return -EAGAIN here and skip the retry, because
> what's important is the re-enable part: get the counting running again
> as soon as we can, so that, if the user-side sampling is done at a low
> rate, we still get non-zero values between two dumps when a reset
> occurred in the middle.
I'm a bit confused about the uAPI you have in mind. When would we return
-EAGAIN and when -EIO?
I guess the former would signal that a dump ioctl can be re-attempted, while
the latter that
a whole perfcnt_disable/enable cycle needs to happen before issuing a new dump.
> > +
> > + if (retry)
> > + goto dump_retry;
> >
> > return ret;
> > }
> > @@ -87,9 +174,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,7 +208,9 @@ 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");
> >
> > @@ -132,44 +220,19 @@ static int panfrost_perfcnt_enable_locked(struct
> > panfrost_device *pfdev,
> > 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);
> > + scoped_guard(rwsem_read, &pfdev->reset.lock) {
> > + ret = panfrost_perfcnt_hw_enable(pfdev);
> > + if (ret)
> > + goto err_vunmap;
> >
> > - /*
> > - * 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);
> > + perfcnt->reset_happened = false;
> > + perfcnt->reset_failed = false;
> > + 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 +258,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->reset_failed)
> > + panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu);
>
> Let's not conflate !reset_failed and owns_as_slot, even if they are
> related in practice.
I did this so that mmu->as_count would be kept balanced in case a reset
happened but
panfrost_mmu_as_get() for perfcnt's AS failed during recovery in
panfrost_perfcnt_hw_enable().
Because mmu->as_count can only be 0 during perfcnt_disabled when perfcnt_reset
has failed,
I thought this conflation was reasonable. Alternatively I could just poke into
mmu->as_count
to make sure it's greater than 0, but that sounds like inspecting private MMU
implementation
details from the perfcnt subsystem and it doesn't look good.
> > + 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);
> > @@ -255,7 +321,7 @@ int panfrost_ioctl_perfcnt_dump(struct drm_device *dev,
> > void *data,
> > goto out;
> > }
> >
> > - ret = panfrost_perfcnt_dump_locked(pfdev);
> > + ret = panfrost_perfcnt_dump_locked(pfdev, &req->hw_reset);
> > if (ret)
> > goto out;
> >
> > @@ -338,3 +404,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->reset_failed = !!panfrost_perfcnt_hw_enable(pfdev);
> > + perfcnt->reset_happened = true;
> > + 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..3bbf9220103d 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)
> > @@ -272,6 +272,7 @@ struct drm_panfrost_perfcnt_enable {
> >
> > struct drm_panfrost_perfcnt_dump {
> > __u64 buf_ptr;
> > + __u64 hw_reset;
>
> Let's make this a
>
> __u32 state;
> __u32 pad;
>
> and define state flags like:
>
> #define PANFROST_PERFCNT_SESSION_DEAD (1 << 0)
> #define PANFROST_PERFCNT_SESSION_INTERRUPTED_BY_RESET (1 << 1)
>
> so we can easily extend the thing without introducing new fields or
> renaming hw_reset.
>
> > };
> >
> > /* madvise provides a way to tell the kernel in case a buffers contents
> >
Adrian Larumbe
