On Tue, Dec 17, 2024 at 10:24:57AM +0100, Boris Brezillon wrote:
> If a reset is scheduled when the suspend happens, we drop the
> reset-pending info on the floor assuming the resume will fix things,
> but the resume logic might try a fast reset. If we're lucky, the
> fast reset fails and we fallback to a slow reset, but if the FW was
> corrupted in a way that makes it partially functional (it boots but
> doesn't quite do what it's expected to do), we won't notice immediately
> that things are not working correctly, leading to a new reset further
> down the road.
>
> Fixes: 5fe909cae118 ("drm/panthor: Add the device logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/panthor/panthor_device.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> index 2c817e65e6be..3285ac42d2cd 100644
> --- a/drivers/gpu/drm/panthor/panthor_device.c
> +++ b/drivers/gpu/drm/panthor/panthor_device.c
> @@ -128,14 +128,11 @@ static void panthor_device_reset_work(struct work_struct *work)
> struct panthor_device *ptdev = container_of(work, struct panthor_device, reset.work);
> int ret = 0, cookie;
>
> - if (atomic_read(&ptdev->pm.state) != PANTHOR_DEVICE_PM_STATE_ACTIVE) {
> - /*
> - * No need for a reset as the device has been (or will be)
> - * powered down
> - */
> - atomic_set(&ptdev->reset.pending, 0);
> + /* If the device is entering suspend, we don't reset. A slow reset will
> + * be forced at resume time instead.
> + */
> + if (atomic_read(&ptdev->pm.state) != PANTHOR_DEVICE_PM_STATE_ACTIVE)
> return;
> - }
>
> if (!drm_dev_enter(&ptdev->base, &cookie))
> return;
> @@ -473,6 +470,14 @@ int panthor_device_resume(struct device *dev)
>
> if (panthor_device_is_initialized(ptdev) &&
> drm_dev_enter(&ptdev->base, &cookie)) {
> + /* If there was a reset pending at the time we suspended the
> + * device, we force a slow reset.
> + */
> + if (atomic_read(&ptdev->reset.pending)) {
> + ptdev->reset.fast = false;
> + atomic_set(&ptdev->reset.pending, 0);
> + }
> +
> ret = panthor_device_resume_hw_components(ptdev);
> if (ret && ptdev->reset.fast) {
> drm_err(&ptdev->base, "Fast reset failed, trying a slow reset");
> @@ -489,9 +494,6 @@ int panthor_device_resume(struct device *dev)
> goto err_suspend_devfreq;
> }
>
> - if (atomic_read(&ptdev->reset.pending))
> - queue_work(ptdev->reset.wq, &ptdev->reset.work);
> -
If you remove this, why do we need a reset workqueue at all? Nothing gets scheduled in it.
Best regards,
Liviu
> /* Clear all IOMEM mappings pointing to this device after we've
> * resumed. This way the fake mappings pointing to the dummy pages
> * are removed and the real iomem mapping will be restored on next
> --
> 2.47.0
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
