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" 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);
> + panthor_sched_post_reset(ptdev);
> +
> + if (ret) {
...this cannot ever be reached. I think the out_dev_exit label should be
earlier (and renamed?)
> + 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
