From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from bali.collaboradmins.com (bali.collaboradmins.com [148.251.105.195]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 2DAC32C027F for ; Wed, 17 Jun 2026 14:43:52 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=148.251.105.195 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781707433; cv=none; b=RfvpIsN+QzyjJ1eh476AmqN0q7DNlEDAmuAvh4X8ltkCNqturCnAoxqOdZ+7L1qahbagVVvmucXvt3cEyLEg3gCw9upTVxvj71c9pHgHREbwjKcRGfzcQlwC7eD+1Fpmy6m0BzBAabkoS5vD89SMvwwLiVkKduoQRHWmf6yOLC4= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781707433; c=relaxed/simple; bh=XKhIixanpR5xhkkxUJswWvn7EcxBcQC0W02lN47Om2I=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=CmUUbY8RO2mOjD8dznqDrHj7OpUinBnhskQve7HwUXsKGNeg4gHlkmMpf35ZF8ZMkbOUjmIi7WaDO1BoPPGUQkRVxpBQblh5yIeTKH5n6nt2wH9oVr+V1/3z/bMsmPMipNE36ChzeLSvTE8IaV2uZjG5Q29StP4k792A36vtpB0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com; spf=pass smtp.mailfrom=collabora.com; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b=VhaWp9gf; arc=none smtp.client-ip=148.251.105.195 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=collabora.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=collabora.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=collabora.com header.i=@collabora.com header.b="VhaWp9gf" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=collabora.com; s=mail; t=1781707430; bh=XKhIixanpR5xhkkxUJswWvn7EcxBcQC0W02lN47Om2I=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=VhaWp9gfBJQTL73NNjmHaCxfz5KLwUXy5aJzGWZCybSBCGw8zmCkW8CPgZcW7SGo3 YxYT4qx1rvWOUJLL4pfWv9J/6UDP4ue4uNAIfbj6K7AfBqKkBSlUnSm4I1pHxRqd2h DxpkG26gMLFLiZyDuh7xSu6l7mdKHb82I/Z2vZswk3miRLqWtkJJkPImSd0c2+rhox BL/Pv5H+NkUhBLHo971pGtdK6MZQ1AqtFMEklz0+4D2o1wHAS+QUHNqvVrR+37sEDT dv2hTU/gbVt7CQmt8/CssBDGhrmjIFzzQbMvrsrG+mHqAgUGvJgnN+BNc+1FeV4A4m +7a28kPHaViqQ== Received: from fedora-2.home (unknown [100.64.0.11]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (prime256v1) server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: bbrezillon) by bali.collaboradmins.com (Postfix) with ESMTPSA id 8F50417E05DE; Wed, 17 Jun 2026 16:43:49 +0200 (CEST) Date: Wed, 17 Jun 2026 16:43:44 +0200 From: Boris Brezillon To: =?UTF-8?B?QWRyacOhbg==?= Larumbe Cc: Rob Herring , Steven Price , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter , Faith Ekstrand , "Marty E. Plummer" , Tomeu Vizoso , Eric Anholt , Alyssa Rosenzweig , Robin Murphy , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, Collabora Kernel Team , Neil Armstrong , Claude Subject: Re: [PATCH v2 5/7] drm/panfrost: Make reset sequence deal with an active HWPerf session Message-ID: <20260617164344.62a36a94@fedora-2.home> In-Reply-To: <178164954145.482538.4666042173411800435.b4-reply@b4> References: <20260604-claude-fixes-v2-0-57c6bd4c1655@collabora.com> <20260604-claude-fixes-v2-5-57c6bd4c1655@collabora.com> <20260604202613.4afe53f1@fedora-2.home> <178164954145.482538.4666042173411800435.b4-reply@b4> Organization: Collabora X-Mailer: Claws Mail 4.4.0 (GTK 3.24.52; x86_64-redhat-linux-gnu) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Tue, 16 Jun 2026 23:39:01 +0100 Adri=C3=A1n Larumbe wrote: > On 2026-06-04 20:26:13+02:00, Boris Brezillon wrote: > > On Thu, 04 Jun 2026 18:35:24 +0100 > > Adri=C3=A1n Larumbe wrote: > > =20 > > > Right now, if there's a HW reset and an HWPerf session is active, > > > panfrost_mmu_reset() will reset the AS count for every single open fi= le's > > > mmu struct back to 0, and also invalidate their AS numbers. Then, when > > > disabling hwperf, panfrost_mmu_as_put() will WARN that mmu->as_count = is > > > less than zero. > > >=20 > > > Fix this by introducing a perfcnt HW reset path. > > >=20 > > > The choice was made to render perfcnt unusable after reset, so that a > > > user might have to reprogram it with a full disable/enable sequence > > > before requesting more perfcnt dumps. =20 > >=20 > > Can't we do better than that. We store the config of the perf session, > > so if a reset is in progress, we can simply block on it, restore the > > old config in the post_reset path, and get going with the DUMP request > > once the reset has been done. There's certainly something to do to > > report the discontinuity to userspace, but that's something we can > > reflect through and extra field added to drm_panfrost_perfcnt_dump. =20 >=20 > Do you mean perfcnt_dump() should wait until the reset is finished in the= ioctl > itself? Yes. You shouldn't be allowed to interact with the HW while a reset is in progress (or even pending), because you're guaranteed it won't do what you want. > However it makes me wonder what we could do when the reset happens and > there's an going perfcnt_enable() ioctl, and the reset might render the i= nitialised > state void. That's exactly the sort of issues I'm talking about. Given reset interferes with the perfcnt config, you have to make sure you restore the previous state after a reset, and report back the transient failure on the next DUMP request coming from userspace. >=20 > > > Reported-by: Claude > > > Closes: https://gitlab.freedesktop.org/panfrost/linux/-/work_items/88 > > > Signed-off-by: Adri=C3=A1n Larumbe > > > Fixes: 7786fd108777 ("drm/panfrost: Expose performance counters throu= gh unstable ioctls") > > > --- > > > drivers/gpu/drm/panfrost/panfrost_device.c | 1 + > > > drivers/gpu/drm/panfrost/panfrost_perfcnt.c | 46 +++++++++++++++++++= +++++++++- > > > drivers/gpu/drm/panfrost/panfrost_perfcnt.h | 1 + > > > 3 files changed, 47 insertions(+), 1 deletion(-) > > >=20 > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu= /drm/panfrost/panfrost_device.c > > > index 87b372c9e675..2805d50c1b9b 100644 > > > --- a/drivers/gpu/drm/panfrost/panfrost_device.c > > > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c > > > @@ -426,6 +426,7 @@ bool panfrost_exception_needs_reset(const struct = panfrost_device *pfdev, > > > =20 > > > void panfrost_device_reset(struct panfrost_device *pfdev, bool enabl= e_job_int) > > > { > > > + panfrost_perfcnt_reset(pfdev); > > > panfrost_gpu_soft_reset(pfdev); > > > =20 > > > panfrost_gpu_power_on(pfdev); > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c b/drivers/gp= u/drm/panfrost/panfrost_perfcnt.c > > > index ad1156678e91..c2087ea705fe 100644 > > > --- a/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > > > +++ b/drivers/gpu/drm/panfrost/panfrost_perfcnt.c > > > @@ -33,6 +33,7 @@ struct panfrost_perfcnt { > > > struct panfrost_file_priv *user; > > > struct mutex lock; > > > struct completion dump_comp; > > > + atomic_t hw_reset_happened; > > > }; > > > =20 > > > static void panfrost_perfcnt_gpu_disable(struct panfrost_device *pfd= ev) > > > @@ -57,9 +58,13 @@ void panfrost_perfcnt_sample_done(struct panfrost_= device *pfdev) > > > =20 > > > static int panfrost_perfcnt_dump_locked(struct panfrost_device *pfde= v) > > > { > > > + struct panfrost_perfcnt *perfcnt =3D pfdev->perfcnt; > > > u64 gpuva; > > > int ret; > > > =20 > > > + if (atomic_read(&perfcnt->hw_reset_happened)) > > > + return -EIO; > > > + > > > reinit_completion(&pfdev->perfcnt->dump_comp); > > > gpuva =3D pfdev->perfcnt->mapping->mmnode.start << PAGE_SHIFT; > > > gpu_write(pfdev, GPU_PERFCNT_BASE_LO, lower_32_bits(gpuva)); > > > @@ -140,6 +145,15 @@ static int panfrost_perfcnt_enable_locked(struct= panfrost_device *pfdev, > > > goto err_vunmap; > > > } > > > =20 > > > + /* If a reset is ongoing, the AS we get right below will be torn > > > + * down, so rather than waiting until this becomes obvious in a > > > + * perfcnt_dump() ioctl, we ask the user to try again slightly late= r. > > > + */ > > > + if (atomic_read(&pfdev->reset.pending)) { > > > + ret =3D -EAGAIN; > > > + goto err_vunmap; > > > + } > > > + > > > ret =3D panfrost_mmu_as_get(pfdev, perfcnt->mapping->mmu); > > > if (ret < 0) > > > goto err_vunmap; > > > @@ -173,6 +187,16 @@ static int panfrost_perfcnt_enable_locked(struct= panfrost_device *pfdev, > > > if (panfrost_has_hw_issue(pfdev, HW_ISSUE_8186)) > > > gpu_write(pfdev, GPU_PRFCNT_TILER_EN, 0xffffffff); > > > =20 > > > + /* If a reset happened, we've no way of knowing whether it was betw= een the time we called > > > + * panfrost_mmu_as_get() or before perfcnt_enable(), so clearing th= is flag and going forward > > > + * isn't possible. We must clear the flag and try again in the hope= s no resets will happen > > > + * between this and the next ioctl invocation. > > > + */ > > > + if (atomic_cmpxchg(&perfcnt->hw_reset_happened, 1, 0)) { > > > + ret =3D EAGAIN; > > > + goto err_disable; > > > + } =20 > >=20 > > This should really be transparent to the user, apart from reporting > > that samples might have been lost because of the reset. > > =20 > > > + > > > /* The BO ref is retained by the mapping. */ > > > drm_gem_object_put(&bo->base); > > > =20 > > > @@ -180,6 +204,8 @@ static int panfrost_perfcnt_enable_locked(struct = panfrost_device *pfdev, > > > =20 > > > return 0; > > > =20 > > > +err_disable: > > > + panfrost_perfcnt_gpu_disable(pfdev); > > > err_vunmap: > > > drm_gem_vunmap(&bo->base, &map); > > > err_put_mapping: > > > @@ -209,7 +235,8 @@ static int panfrost_perfcnt_disable_locked(struct= panfrost_device *pfdev, > > > drm_gem_vunmap(&perfcnt->mapping->obj->base.base, &map); > > > perfcnt->buf =3D NULL; > > > panfrost_gem_close(&perfcnt->mapping->obj->base.base, file_priv); > > > - panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu); > > > + if (!atomic_read(&perfcnt->hw_reset_happened)) > > > + panfrost_mmu_as_put(pfdev, perfcnt->mapping->mmu); =20 > >=20 > > Why not call panfrost_mmu_as_put() in panfrost_perfcnt_pre_reset(), > > which is called before panfrost_mmu_reset()? If we do that, the AS > > should be returned, and we can add a panfrost_perfcnt_post_reset() > > that's called after panfrost_mmu_reset() and which re-acquires the AS > > and re-instantiate the perfcnt settings. =20 >=20 > I was doing this revision 1, but then that could lead to races because pa= nfrost_perfcnt_reset() > and panfrost_perfcnt_disable_locked() could call panfrost_mmu_as_put() ri= ght after another, > leaving the as_count at -1. That's something you solve with locking/proper-serialization. Calling panfrost_mmu_as_put() conditionally in the disable path is just papering over the race. >=20 > > > panfrost_gem_mapping_put(perfcnt->mapping); > > > perfcnt->mapping =3D NULL; > > > pm_runtime_put_autosuspend(pfdev->base.dev); > > > @@ -346,3 +373,20 @@ void panfrost_perfcnt_fini(struct panfrost_devic= e *pfdev) > > > /* Disable everything before leaving. */ > > > panfrost_perfcnt_gpu_disable(pfdev); > > > } > > > + > > > +void panfrost_perfcnt_reset(struct panfrost_device *pfdev) > > > +{ > > > + struct panfrost_perfcnt *perfcnt =3D pfdev->perfcnt; > > > + > > > + /* Since this function will be called either from a scheduled HW re= set > > > + * or a runtime resume, tearing down any perfcnt resources means we= 're > > > + * doomed to deadlocking with perfcnt_{enable/disable}, since we'd = have > > > + * to take the perfecnt lock. On top of that, it'd also violate DMA= fence > > > + * signalling rules because GFP_KERNEL allocations are made with th= e perfcnt > > > + * lock taken in perfcnt_enable. =20 > >=20 > > Question is, do we really need these allocation to happen with the lock > > held? And if yes, can't we protect perfcnt ops with a separate reset > > lock? =20 >=20 > I'd rather do the latter, I think Panthor is doing something like that al= ready? Allocating resources before acquiring the lock to check if another session exists sounds like a better option IMHO. Yes, you might end up releasing everything because another active session exists, but that's fine.