On Tue, 17 Dec 2024 10:56:14 +0000
Liviu Dudau <liviu.dudau@xxxxxxx> wrote:
> 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.
git grep reset\.wq drivers/gpu/drm/panthor/ reports a few users ;-).
>
> 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
> >
>
