On Wed, 6 Dec 2023 16:55:42 +0000
Steven Price <steven.price@xxxxxxx> wrote:
> On 04/12/2023 17:32, Boris Brezillon wrote:
> > The panthor driver is designed in a modular way, where each logical
> > block is dealing with a specific HW-block or software feature. In order
> > for those blocks to communicate with each other, we need a central
> > panthor_device collecting all the blocks, and exposing some common
> > features, like interrupt handling, power management, reset, ...
> >
> > This what this panthor_device logical block is about.
> >
> > v3:
> > - Add acks for the MIT+GPL2 relicensing
> > - Fix 32-bit support
> > - Shorten the sections protected by panthor_device::pm::mmio_lock to fix
> > lock ordering issues.
> > - Rename panthor_device::pm::lock into panthor_device::pm::mmio_lock to
> > better reflect what this lock is protecting
> > - Use dev_err_probe()
> > - Make sure we call drm_dev_exit() when something fails half-way in
> > panthor_device_reset_work()
> > - Replace CSF_GPU_LATEST_FLUSH_ID_DEFAULT with a constant '1' and a
> > comment to explain. Also remove setting the dummy flush ID on suspend.
> > - Remove drm_WARN_ON() in panthor_exception_name()
> > - Check pirq->suspended in panthor_xxx_irq_raw_handler()
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> > Signed-off-by: Steven Price <steven.price@xxxxxxx>
> > Acked-by: Steven Price <steven.price@xxxxxxx> # MIT+GPL2 relicensing,Arm
> > Acked-by: Grant Likely <grant.likely@xxxxxxxxxx> # MIT+GPL2 relicensing,Linaro
> > Acked-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx> # MIT+GPL2 relicensing,Collabora
>
> We still have the "FIXME: this is racy"
Yeah, I still didn't find a proper solution for that.
> and there's something wrong in
> panthor_device_reset_work() (see below). But otherwise looks good.
>
> > ---
> > drivers/gpu/drm/panthor/panthor_device.c | 497 +++++++++++++++++++++++
> > drivers/gpu/drm/panthor/panthor_device.h | 381 +++++++++++++++++
> > 2 files changed, 878 insertions(+)
> > create mode 100644 drivers/gpu/drm/panthor/panthor_device.c
> > create mode 100644 drivers/gpu/drm/panthor/panthor_device.h
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
> > new file mode 100644
> > index 000000000000..40038bac147a
> > --- /dev/null
> > +++ b/drivers/gpu/drm/panthor/panthor_device.c
>
> <snip>
>
> > +
> > +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);
> > + return;
> > + }
> > +
> > + if (!drm_dev_enter(&ptdev->base, &cookie))
> > + return;
> > +
> > + panthor_sched_pre_reset(ptdev);
> > + panthor_fw_pre_reset(ptdev, true);
> > + panthor_mmu_pre_reset(ptdev);
> > + panthor_gpu_soft_reset(ptdev);
> > + panthor_gpu_l2_power_on(ptdev);
> > + panthor_mmu_post_reset(ptdev);
> > + ret = panthor_fw_post_reset(ptdev);
> > + if (ret)
>
> If ret is true then we branch, so...
>
> > + goto out_dev_exit;
> > +
> > + atomic_set(&ptdev->reset.pending, 0);
I think we want to clear the reset pending bit even if the post reset
failed.
> > + panthor_sched_post_reset(ptdev);
> > +
> > + if (ret) {
>
> ...this cannot ever be reached. I think the out_dev_exit label should be
> earlier (and renamed?)
Uh, actually we can't call panthor_device_unplug() when we're in the
drm_dev_enter/exit() section, this would cause a deadlock. This should
be moved at the end of the function, but I can't remember if I had
another good reason to move it here...
>
> > + panthor_device_unplug(ptdev);
> > + drm_err(&ptdev->base, "Failed to boot MCU after reset, making device unusable.");
> > + }
> > +
> > +out_dev_exit:
> > + drm_dev_exit(cookie);
> > +}
>
> Thanks,
> Steve
>
