Re: [PATCH v3 08/14] drm/panthor: Add the FW logical block
On 22/01/2024 16:34, Boris Brezillon wrote:
> On Mon, 18 Dec 2023 21:25:16 +
> Chris Diamand wrote:
>
>>> +void panthor_fw_unplug(struct panthor_device *ptdev)
>>> +{
>>> + struct panthor_fw_section *section;
>>> +
>>> + cancel_delayed_work_sync(>fw->watchdog.ping_work);
>>> +
>>> + /* Make sure the IRQ handler can be called after that
>>> point. */
>>> + if (ptdev->fw->irq.irq)
>>> + panthor_job_irq_suspend(>fw->irq);
>>> +
>>> + panthor_fw_stop(ptdev);
>>> +
>>> + if (ptdev->fw->vm)
>>> + panthor_vm_idle(ptdev->fw->vm);
>>> +
>>> + list_for_each_entry(section, >fw->sections, node)
>>> + panthor_kernel_bo_destroy(panthor_fw_vm(ptdev),
>>> section->mem);
>>
>> Here's where we destroy the potentially invalid section->mem.
>>
>> Note that the issues are twofold:
>> Firstly, section->mem could be an error pointer as mentioned earlier.
>> Secondly, panthor_kernel_bo_destroy() doesn't actually check for
>> error values or even NULL.
>>
>> It would probably be worth adding NULL checks to
>> panthor_kernel_bo_destroy() and panthor_kernel_bo_vunmap() to protect
>> against this.
>
> I ended up implementing your suggestion, because it simplifies all
> call-sites in panthor_sched.c too. The new version defer the
> IS_ERR_OR_NULL() check to panthor_kernel_bo_destroy().
Sounds good to me - thanks!
Re: [PATCH v3 08/14] drm/panthor: Add the FW logical block
On 04/12/2023 17:33, Boris Brezillon wrote:
> Contains everything that's FW related, that includes the code dealing
> with the microcontroller unit (MCU) that's running the FW, and anything
> related to allocating memory shared between the FW and the CPU.
>
> A few global FW events are processed in the IRQ handler, the rest is
> forwarded to the scheduler, since scheduling is the primary reason for
> the FW existence, and also the main source of FW <-> kernel
> interactions.
>
> v3:
> - Make the FW path more future-proof (Liviu)
> - Use one waitqueue for all FW events
> - Simplify propagation of FW events to the scheduler logic
> - Drop the panthor_fw_mem abstraction and use panthor_kernel_bo instead
> - Account for the panthor_vm changes
> - Replace magic number with 0x7fff with ~0 to better signify that
> it's the maximum permitted value.
> - More accurate rounding when computing the firmware timeout.
> - Add a 'sub iterator' helper function. This also adds a check that a
> firmware entry doesn't overflow the firmware image.
> - Drop __packed from FW structures, natural alignment is good enough.
> - Other minor code improvements.
>
> Signed-off-by: Boris Brezillon
> Signed-off-by: Steven Price
> ---
> drivers/gpu/drm/panthor/panthor_fw.c | 1332 ++
> drivers/gpu/drm/panthor/panthor_fw.h | 504 ++
> 2 files changed, 1836 insertions(+)
> create mode 100644 drivers/gpu/drm/panthor/panthor_fw.c
> create mode 100644 drivers/gpu/drm/panthor/panthor_fw.h
>
> diff --git a/drivers/gpu/drm/panthor/panthor_fw.c
> b/drivers/gpu/drm/panthor/panthor_fw.c
> new file mode 100644
> index ..85afe769f567
> --- /dev/null
> +++ b/drivers/gpu/drm/panthor/panthor_fw.c
...
> +static int panthor_fw_load_section_entry(struct panthor_device *ptdev,
> + const struct firmware *fw,
> + struct panthor_fw_binary_iter *iter,
> + u32 ehdr)
> +{
> + struct panthor_fw_binary_section_entry_hdr hdr;
> + struct panthor_fw_section *section;
> + u32 section_size;
> + u32 name_len;
> + int ret;
> +
> + ret = panthor_fw_binary_iter_read(ptdev, iter, , sizeof(hdr));
> + if (ret)
> + return ret;
> +
> + if (hdr.data.end < hdr.data.start) {
> + drm_err(>base, "Firmware corrupted, data.end <
> data.start (0x%x < 0x%x)\n",
> + hdr.data.end, hdr.data.start);
> + return -EINVAL;
> + }
> +
> + if (hdr.va.end < hdr.va.start) {
> + drm_err(>base, "Firmware corrupted, hdr.va.end <
> hdr.va.start (0x%x < 0x%x)\n",
> + hdr.va.end, hdr.va.start);
> + return -EINVAL;
> + }
> +
> + if (hdr.data.end > fw->size) {
> + drm_err(>base, "Firmware corrupted, file truncated?
> data_end=0x%x > fw size=0x%zx\n",
> + hdr.data.end, fw->size);
> + return -EINVAL;
> + }
> +
> + if ((hdr.va.start & ~PAGE_MASK) != 0 ||
> + (hdr.va.end & ~PAGE_MASK) != 0) {
> + drm_err(>base, "Firmware corrupted, virtual addresses
> not page aligned: 0x%x-0x%x\n",
> + hdr.va.start, hdr.va.end);
> + return -EINVAL;
> + }
> +
> + if (hdr.flags & ~CSF_FW_BINARY_IFACE_ENTRY_RD_SUPPORTED_FLAGS) {
> + drm_err(>base, "Firmware contains interface with
> unsupported flags (0x%x)\n",
> + hdr.flags);
> + return -EINVAL;
> + }
> +
> + if (hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_RD_PROT) {
> + drm_warn(>base,
> + "Firmware protected mode entry not be supported,
> ignoring");
> + return 0;
> + }
> +
> + if (hdr.va.start == CSF_MCU_SHARED_REGION_START &&
> + !(hdr.flags & CSF_FW_BINARY_IFACE_ENTRY_RD_SHARED)) {
> + drm_err(>base,
> + "Interface at 0x%llx must be shared",
> CSF_MCU_SHARED_REGION_START);
> + return -EINVAL;
> + }
> +
> + name_len = iter->size - iter->offset;
> +
> + section = drmm_kzalloc(>base, sizeof(*section), GFP_KERNEL);
> + if (!section)
> + return -ENOMEM;
> +
> + list_add_tail(>node, >fw->sections);
> + section->flags = hdr.flags;
> + section->data.size = hdr.data.end - hdr.data.start;
> +
> + if (section->data.size > 0) {
> + void *data = drmm_kmalloc(>base, section->data.size,
> GFP_KERNEL);
> +
> + if (!data)
> + return -ENOMEM;
> +
> + memcpy(data, fw->data + hdr.data.start, section->data.size);
> + section->data.buf = data;
> + }
> +
> + if (name_len > 0) {
> + char *name = drmm_kmalloc(>base, name_len + 1,
> GFP_KERNEL);
> +
> + if (!name)
> + return -ENOMEM;
> +
> + memcpy(name, iter->data + iter->offset, name_len);
> +
Re: [PATCH v3 01/14] drm/panthor: Add uAPI
> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> new file mode 100644
> index ..6d815df5e829
> --- /dev/null
> +++ b/include/uapi/drm/panthor_drm.h
> @@ -0,0 +1,892 @@
> +/* SPDX-License-Identifier: MIT */
> +/* Copyright (C) 2023 Collabora ltd. */
> +#ifndef _PANTHOR_DRM_H_
> +#define _PANTHOR_DRM_H_
> +
> +#include "drm.h"
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif
> +
> +/**
> + * DOC: Introduction
...
> +/**
> + * struct drm_panthor_group_submit - Arguments passed to
> DRM_IOCTL_PANTHOR_VM_BIND
> + */
> +struct drm_panthor_group_submit {
> + /** @group_handle: Handle of the group to queue jobs to. */
> + __u32 group_handle;
> +
> + /** @pad: MBZ. */
> + __u32 pad;
> +
> + /** @queue_submits: Array of drm_panthor_queue_submit objects. */
> + struct drm_panthor_obj_array queue_submits;
> +};
Hi! Very minor nit - but shouldn't the comment above say
DRM_IOCTL_PANTHOR_GROUP_SUBMIT, not VM_BIND?
> +
> +/**
> + * enum drm_panthor_group_state_flags - Group state flags
> + */
> +enum drm_panthor_group_state_flags {
> + /**
> + * @DRM_PANTHOR_GROUP_STATE_TIMEDOUT: Group had unfinished jobs.
> + *
> + * When a group ends up with this flag set, no jobs can be submitted to
> its queues.
> + */
> + DRM_PANTHOR_GROUP_STATE_TIMEDOUT = 1 << 0,
> +
> + /**
> + * @DRM_PANTHOR_GROUP_STATE_FATAL_FAULT: Group had fatal faults.
> + *
> + * When a group ends up with this flag set, no jobs can be submitted to
> its queues.
> + */
> + DRM_PANTHOR_GROUP_STATE_FATAL_FAULT = 1 << 1,
> +};
> +
> +/**
> + * struct drm_panthor_group_get_state - Arguments passed to
> DRM_IOCTL_PANTHOR_GROUP_GET_STATE
> + *
> + * Used to query the state of a group and decide whether a new group should
> be created to
> + * replace it.
> + */
> +struct drm_panthor_group_get_state {
> + /** @group_handle: Handle of the group to query state on */
> + __u32 group_handle;
> +
> + /**
> + * @state: Combination of DRM_PANTHOR_GROUP_STATE_* flags encoding the
> + * group state.
> + */
> + __u32 state;
> +
> + /** @fatal_queues: Bitmask of queues that faced fatal faults. */
> + __u32 fatal_queues;
> +
> + /** @pad: MBZ */
> + __u32 pad;
> +};
> +
> +/**
> + * struct drm_panthor_tiler_heap_create - Arguments passed to
> DRM_IOCTL_PANTHOR_TILER_HEAP_CREATE
> + */
> +struct drm_panthor_tiler_heap_create {
> + /** @vm_id: VM ID the tiler heap should be mapped to */
> + __u32 vm_id;
> +
> + /** @initial_chunk_count: Initial number of chunks to allocate. */
> + __u32 initial_chunk_count;
> +
> + /** @chunk_size: Chunk size. Must be a power of two at least 256KB
> large. */
> + __u32 chunk_size;
> +
> + /** @max_chunks: Maximum number of chunks that can be allocated. */
> + __u32 max_chunks;
> +
> + /**
> + * @target_in_flight: Maximum number of in-flight render passes.
> + *
> + * If the heap has more than tiler jobs in-flight, the FW will wait for
> render
> + * passes to finish before queuing new tiler jobs.
> + */
> + __u32 target_in_flight;
> +
> + /** @handle: Returned heap handle. Passed back to DESTROY_TILER_HEAP. */
> + __u32 handle;
> +
> + /** @tiler_heap_ctx_gpu_va: Returned heap GPU virtual address returned
> */
> + __u64 tiler_heap_ctx_gpu_va;
> +
> + /**
> + * @first_heap_chunk_gpu_va: First heap chunk.
> + *
> + * The tiler heap is formed of heap chunks forming a single-link list.
> This
> + * is the first element in the list.
> + */
> + __u64 first_heap_chunk_gpu_va;
> +};
> +
> +/**
> + * struct drm_panthor_tiler_heap_destroy - Arguments passed to
> DRM_IOCTL_PANTHOR_TILER_HEAP_DESTROY
> + */
> +struct drm_panthor_tiler_heap_destroy {
> + /** @handle: Handle of the tiler heap to destroy */
> + __u32 handle;
> +
> + /** @pad: Padding field, MBZ. */
> + __u32 pad;
> +};
> +
> +#if defined(__cplusplus)
> +}
> +#endif
> +
> +#endif /* _PANTHOR_DRM_H_ */
Cheers,
Chris
