Re: [PATCH v3 03/14] drm/panthor: Add the device logical block

[Boris Brezillon <boris.brezillon@collabora.com>]

On Wed, 6 Dec 2023 16:55:42 +0000
Steven Price <steven.price@arm.com> 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@collabora.com>
> > Signed-off-by: Steven Price <steven.price@arm.com>
> > Acked-by: Steven Price <steven.price@arm.com> # MIT+GPL2 relicensing,Arm
> > Acked-by: Grant Likely <grant.likely@linaro.org> # MIT+GPL2 relicensing,Linaro
> > Acked-by: Boris Brezillon <boris.brezillon@collabora.com> # 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
>