On Fri, 25 Sep 2026 19:45:36 +0100, Adrián Larumbe
<[email protected]> wrote:
> diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> index b3f71d7fd82a..96c3c41cd269 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c
> @@ -58,44 +70,154 @@ 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)
> +{
> + struct panfrost_perfcnt *perfcnt = pfdev->perfcnt;
> + u32 cfg, as;
> + int ret;
> +
I might add a lockdep_assert_held(&pfdev->reset.lock); here.
> [ ... skip 152 lines ... ]
> + if (perfcnt->user != user)
> + return -EBUSY;
> +
> + if (perfcnt->state & PANFROST_PERFCNT_SESSION_DEAD)
> + drm_WARN_ON(&pfdev->base,
> + panfrost_perfcnt_disable_locked(pfdev,
> file_priv));
Forgot to add an 'else return 0'. Also checking the state flag here is racy
with the reset sequence,
which could keep coming through, because perfcnt->user hasn't changed.
Also, on a second thought, it seems going through perfcnt_disable to recreate
a whole new session
from scratch is an overkill, since the only thing we need is an MMU reference
for the perfcnt
BO's AS. On top of that, panfrost_perfcnt_disable_locked() might power the
device off and then
getting a PM reference right below might power it up again, a completely
pointless cycle.
So maybe:
if (perfcnt->user == user) {
scoped_guard(rwsem_read, &pfdev->reset.lock) {
if (perfcnt->state & PANFROST_PERFCNT_SESSION_DEAD) {
ret = panfrost_perfcnt_hw_enable(pfdev);
if (!ret)
perfcnt->state = 0;
return ret;
}
}
return 0;
}
if (perfcnt->user != NULL)
return -EBUSY;
An alterantive would be expecting UM to issue a perfcnt_disable before being
able to enable a new session.
--
Adrián Larumbe <[email protected]>
