On Wed, 2 Sep 2026 16:45:38 +0100 Adrián Larumbe <[email protected]> wrote:
> 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? So, if we failed to re-enable (which, again, is unlikely if not impossible), yes, this would force a 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. Silently re-enabling and notifying about the RESET disturbing the counters makes sense, because things are still functional. What I don't like with this retry approach is the fact it doesn't really solve the problem because you have a limited amount of attempts, and does add a fair amount of complexity (take the lock, issue the command, release the lock, wait, take the lock, check the state, release the lock, retry if it failed) > > > > + 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? So, EAGAIN is if your SAMPLE request is interrupted by a RESET, but counters were properly re-enabled after the RESET. This basically gets rid of the retry logic kernel side and leaves that to the UMD. EIO if when the re-enable failed. > 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. Yep, this. > > > > + > > > + 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. No, I'm actually suggesting keeping track of this information through a dedicated field. Can be: // >=0 => owns an AS ref int as_id; or: // true when the perfcnt session owns an AS ref bool owns_as_ref; and you set/reset that field in the enable/disable/reset path.
