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

[Steven Price <steven.price@arm.com>]

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" 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