On Tue, Dec 17, 2024 at 12:06:59PM +0100, Boris Brezillon wrote:
> 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 ;-).
Sorry, I've only looked at panthor_device.c and brain was not fully engaged.
I think a more relevant comment is that panthor_device_schedule_reset() is still called in a few
places and that in turn queues work in reset.wq. Also, the firmware watchdog uses that workqueue.
Patch looks good to me,
Reviewed-by: Liviu Dudau <liviu.dudau@xxxxxxx>
Best regards,
Liviu
>
> >
> > 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! /
---------------
¯\_(ツ)_/¯
