Received-SPF: pass (google.com: domain of linux-kernel+bounces-116952-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Content-Type: text/plain;
	charset=utf-8
Precedence: bulk
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3774.400.31\))
Subject: Re: [PATCH] drm/panfrost: fix power transition timeout warnings
From: Christian Hewitt <christianshewitt@gmail.com>
In-Reply-To: <bad16bba-5469-4f20-bf23-7f3c2953edc5@arm.com>
Date: Mon, 25 Mar 2024 15:36:40 +0400
Cc: Boris Brezillon <boris.brezillon@collabora.com>,
 Rob Herring <robh@kernel.org>,
 Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
 Maxime Ripard <mripard@kernel.org>,
 Thomas Zimmermann <tzimmermann@suse.de>,
 David Airlie <airlied@gmail.com>,
 Daniel Vetter <daniel@ffwll.ch>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>,
 dri-devel@lists.freedesktop.org,
 LKML <linux-kernel@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <620A6EF4-4361-4B5F-889B-268091FB72D9@gmail.com>
References: <20240322164525.2617508-1-christianshewitt@gmail.com>
 <bad16bba-5469-4f20-bf23-7f3c2953edc5@arm.com>
To: Steven Price <steven.price@arm.com>

> On 25 Mar 2024, at 2:28=E2=80=AFpm, Steven Price =
<steven.price@arm.com> wrote:
>=20
> On 22/03/2024 16:45, Christian Hewitt wrote:
>> Increase the timeout value to prevent system logs on Amlogic boards =
flooding
>> with power transition warnings:
>>=20
>> [   13.047638] panfrost ffe40000.gpu: shader power transition timeout
>> [   13.048674] panfrost ffe40000.gpu: l2 power transition timeout
>> [   13.937324] panfrost ffe40000.gpu: shader power transition timeout
>> [   13.938351] panfrost ffe40000.gpu: l2 power transition timeout
>> ...
>> [39829.506904] panfrost ffe40000.gpu: shader power transition timeout
>> [39829.507938] panfrost ffe40000.gpu: l2 power transition timeout
>> [39949.508369] panfrost ffe40000.gpu: shader power transition timeout
>> [39949.509405] panfrost ffe40000.gpu: l2 power transition timeout
>>=20
>> The 2000 value has been found through trial and error testing with =
devices
>> using G52 and G31 GPUs.
>=20
> How close to 2ms did you need in your trial and error testing? I'm
> wondering if we should increase it further in case this might still
> trigger occasionally?

I backed it off progressively but still saw occasional messages at 1.6ms
so padded it a little with 2ms, and those systems haven=E2=80=99t shown =
errors
since so I currently see it as a =E2=80=99safe=E2=80=99 value. The one =
possible wildcard
is testing with older T820/T628 boards; but that needs to wait until =
I=E2=80=99m
back home from a long trip and able to test them. The possible theory
being that older/slower systems might require more time. Worst case =
I=E2=80=99ll
have to send another change.

> kbase seems to have a 5s (5000ms!) timeout before it will actually
> complain. But equally it doesn't busy wait on the registers in the =
same
> way as panfrost, so the impact to the rest of the system of a long =
wait
> is less.
>=20
> But 2ms doesn't sound an unreasonable timeout so:
>=20
> Reviewed-by: Steven Price <steven.price@arm.com>
>=20
>> Fixes: 22aa1a209018 ("drm/panfrost: Really power off GPU cores in =
panfrost_gpu_power_off()")
>> Signed-off-by: Christian Hewitt <christianshewitt@gmail.com>> ---
>> drivers/gpu/drm/panfrost/panfrost_gpu.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>=20
>> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c =
b/drivers/gpu/drm/panfrost/panfrost_gpu.c
>> index 9063ce254642..fd8e44992184 100644
>> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
>> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
>> @@ -441,19 +441,19 @@ void panfrost_gpu_power_off(struct =
panfrost_device *pfdev)
>>=20
>> gpu_write(pfdev, SHADER_PWROFF_LO, pfdev->features.shader_present);
>> ret =3D readl_relaxed_poll_timeout(pfdev->iomem + SHADER_PWRTRANS_LO,
>> -  val, !val, 1, 1000);
>> +  val, !val, 1, 2000);
>> if (ret)
>> dev_err(pfdev->dev, "shader power transition timeout");
>>=20
>> gpu_write(pfdev, TILER_PWROFF_LO, pfdev->features.tiler_present);
>> ret =3D readl_relaxed_poll_timeout(pfdev->iomem + TILER_PWRTRANS_LO,
>> -  val, !val, 1, 1000);
>> +  val, !val, 1, 2000);
>> if (ret)
>> dev_err(pfdev->dev, "tiler power transition timeout");
>=20
> As Angelo points out the tiler probably doesn't need such a long
> timeout, but I can't see the harm in consistency so I'm happy with =
this
> change. If my memory of the hardware is correct then the tiler power =
off
> actually does very little and so I wouldn't expect it to take very =
long.

I=E2=80=99ve seen tiler timeouts once I think and thus included it, but =
not since
the values were increased. As long as it=E2=80=99s acceptable I won=E2=80=99=
t over-think
it but if more testing is needed I can look at it more.

> Steve
>=20
>> gpu_write(pfdev, L2_PWROFF_LO, pfdev->features.l2_present);
>> ret =3D readl_poll_timeout(pfdev->iomem + L2_PWRTRANS_LO,
>> -  val, !val, 0, 1000);
>> +  val, !val, 0, 2000);
>> if (ret)
>> dev_err(pfdev->dev, "l2 power transition timeout");
>> }

Christian=