On Wed, Jan 11, 2023 at 10:16 AM Jacek Lawrynowicz
<jacek.lawrynowicz@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
>
> On 10.01.2023 16:34, Oded Gabbay wrote:
> > On Mon, Jan 9, 2023 at 2:24 PM Jacek Lawrynowicz
> > <jacek.lawrynowicz@xxxxxxxxxxxxxxx> wrote:
> >> Read, parse and boot VPU firmware image.
> >>
> >> Co-developed-by: Andrzej Kacprowski <andrzej.kacprowski@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Andrzej Kacprowski <andrzej.kacprowski@xxxxxxxxxxxxxxx>
> >> Co-developed-by: Krystian Pradzynski <krystian.pradzynski@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Krystian Pradzynski <krystian.pradzynski@xxxxxxxxxxxxxxx>
> >> Signed-off-by: Jacek Lawrynowicz <jacek.lawrynowicz@xxxxxxxxxxxxxxx>
> >> ---
> >> drivers/accel/ivpu/Makefile | 1 +
> >> drivers/accel/ivpu/ivpu_drv.c | 131 +++++++++-
> >> drivers/accel/ivpu/ivpu_drv.h | 11 +
> >> drivers/accel/ivpu/ivpu_fw.c | 419 ++++++++++++++++++++++++++++++
> >> drivers/accel/ivpu/ivpu_fw.h | 38 +++
> >> drivers/accel/ivpu/ivpu_hw_mtl.c | 10 +
> >> drivers/accel/ivpu/vpu_boot_api.h | 349 +++++++++++++++++++++++++
> >> include/uapi/drm/ivpu_accel.h | 21 ++
> >> 8 files changed, 979 insertions(+), 1 deletion(-)
> >> create mode 100644 drivers/accel/ivpu/ivpu_fw.c
> >> create mode 100644 drivers/accel/ivpu/ivpu_fw.h
> >> create mode 100644 drivers/accel/ivpu/vpu_boot_api.h
> >>
> >> diff --git a/drivers/accel/ivpu/Makefile b/drivers/accel/ivpu/Makefile
> >> index 46595f0112e3..9fa6a76e9d79 100644
> >> --- a/drivers/accel/ivpu/Makefile
> >> +++ b/drivers/accel/ivpu/Makefile
> >> @@ -3,6 +3,7 @@
> >>
> >> intel_vpu-y := \
> >> ivpu_drv.o \
> >> + ivpu_fw.o \
> >> ivpu_gem.o \
> >> ivpu_hw_mtl.o \
> >> ivpu_ipc.o \
> >> diff --git a/drivers/accel/ivpu/ivpu_drv.c b/drivers/accel/ivpu/ivpu_drv.c
> >> index 6643ae6b5a52..53e103f64832 100644
> >> --- a/drivers/accel/ivpu/ivpu_drv.c
> >> +++ b/drivers/accel/ivpu/ivpu_drv.c
> >> @@ -14,10 +14,13 @@
> >> #include <drm/drm_ioctl.h>
> >> #include <drm/drm_prime.h>
> >>
> >> +#include "vpu_boot_api.h"
> >> #include "ivpu_drv.h"
> >> +#include "ivpu_fw.h"
> >> #include "ivpu_gem.h"
> >> #include "ivpu_hw.h"
> >> #include "ivpu_ipc.h"
> >> +#include "ivpu_jsm_msg.h"
> >> #include "ivpu_mmu.h"
> >> #include "ivpu_mmu_context.h"
> >>
> >> @@ -32,6 +35,10 @@ int ivpu_dbg_mask;
> >> module_param_named(dbg_mask, ivpu_dbg_mask, int, 0644);
> >> MODULE_PARM_DESC(dbg_mask, "Driver debug mask. See IVPU_DBG_* macros.");
> >>
> >> +int ivpu_test_mode;
> >> +module_param_named_unsafe(test_mode, ivpu_test_mode, int, 0644);
> >> +MODULE_PARM_DESC(test_mode, "Test mode: 0 - normal operation, 1 - fw unit test, 2 - null hw");
> >> +
> >> u8 ivpu_pll_min_ratio;
> >> module_param_named(pll_min_ratio, ivpu_pll_min_ratio, byte, 0644);
> >> MODULE_PARM_DESC(pll_min_ratio, "Minimum PLL ratio used to set VPU frequency");
> >> @@ -129,6 +136,28 @@ static int ivpu_get_param_ioctl(struct drm_device *dev, void *data, struct drm_f
> >> case DRM_IVPU_PARAM_CONTEXT_ID:
> >> args->value = file_priv->ctx.id;
> >> break;
> >> + case DRM_IVPU_PARAM_FW_API_VERSION:
> >> + if (args->index < VPU_FW_API_VER_NUM) {
> >> + struct vpu_firmware_header *fw_hdr;
> >> +
> >> + fw_hdr = (struct vpu_firmware_header *)vdev->fw->file->data;
> >> + args->value = fw_hdr->api_version[args->index];
> >> + } else {
> >> + ret = -EINVAL;
> >> + }
> >> + break;
> >> + case DRM_IVPU_PARAM_ENGINE_HEARTBEAT:
> >> + ret = ivpu_jsm_get_heartbeat(vdev, args->index, &args->value);
> >> + break;
> >> + case DRM_IVPU_PARAM_UNIQUE_INFERENCE_ID:
> >> + args->value = (u64)atomic64_inc_return(&vdev->unique_id_counter);
> >> + break;
> >> + case DRM_IVPU_PARAM_TILE_CONFIG:
> >> + args->value = vdev->hw->tile_fuse;
> >> + break;
> >> + case DRM_IVPU_PARAM_SKU:
> >> + args->value = vdev->hw->sku;
> >> + break;
> >> default:
> >> ret = -EINVAL;
> >> break;
> >> @@ -226,11 +255,85 @@ static const struct drm_ioctl_desc ivpu_drm_ioctls[] = {
> >> DRM_IOCTL_DEF_DRV(IVPU_BO_USERPTR, ivpu_bo_userptr_ioctl, 0),
> >> };
> >>
> >> +static int ivpu_wait_for_ready(struct ivpu_device *vdev)
> >> +{
> >> + struct ivpu_ipc_consumer cons;
> >> + struct ivpu_ipc_hdr ipc_hdr;
> >> + unsigned long timeout;
> >> + int ret;
> >> +
> >> + if (ivpu_test_mode == IVPU_TEST_MODE_FW_TEST)
> >> + return 0;
> >> +
> >> + ivpu_ipc_consumer_add(vdev, &cons, IVPU_IPC_CHAN_BOOT_MSG);
> >> +
> >> + timeout = jiffies + msecs_to_jiffies(vdev->timeout.boot);
> >> + while (1) {
> >> + ret = ivpu_ipc_irq_handler(vdev);
> >> + if (ret)
> >> + break;
> >> + ret = ivpu_ipc_receive(vdev, &cons, &ipc_hdr, NULL, 0);
> >> + if (ret != -ETIMEDOUT || time_after_eq(jiffies, timeout))
> >> + break;
> >> +
> >> + cond_resched();
> >> + }
> >> +
> >> + ivpu_ipc_consumer_del(vdev, &cons);
> >> +
> >> + if (!ret && ipc_hdr.data_addr != IVPU_IPC_BOOT_MSG_DATA_ADDR) {
> >> + ivpu_err(vdev, "Invalid VPU ready message: 0x%x\n",
> >> + ipc_hdr.data_addr);
> >> + return -EIO;
> >> + }
> >> +
> >> + if (!ret)
> >> + ivpu_info(vdev, "VPU ready message received successfully\n");
> >> + else
> >> + ivpu_hw_diagnose_failure(vdev);
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +/**
> >> + * ivpu_boot() - Start VPU firmware
> >> + * @vdev: VPU device
> >> + *
> >> + * This function is paired with ivpu_shutdown() but it doesn't power up the
> >> + * VPU because power up has to be called very early in ivpu_probe().
> >> + */
> >> +int ivpu_boot(struct ivpu_device *vdev)
> >> +{
> >> + int ret;
> >> +
> >> + /* Update boot params located at first 4KB of FW memory */
> >> + ivpu_fw_boot_params_setup(vdev, vdev->fw->mem->kvaddr);
> >> +
> >> + ret = ivpu_hw_boot_fw(vdev);
> >> + if (ret) {
> >> + ivpu_err(vdev, "Failed to start the firmware: %d\n", ret);
> >> + return ret;
> >> + }
> >> +
> >> + ret = ivpu_wait_for_ready(vdev);
> >> + if (ret) {
> >> + ivpu_err(vdev, "Failed to boot the firmware: %d\n", ret);
> >> + return ret;
> >> + }
> >> +
> >> + ivpu_hw_irq_clear(vdev);
> >> + enable_irq(vdev->irq);
> >> + ivpu_hw_irq_enable(vdev);
> >> + ivpu_ipc_enable(vdev);
> >> + return 0;
> >> +}
> >> +
> >> int ivpu_shutdown(struct ivpu_device *vdev)
> >> {
> >> int ret;
> >>
> >> ivpu_hw_irq_disable(vdev);
> >> + disable_irq(vdev->irq);
> >> ivpu_ipc_disable(vdev);
> >> ivpu_mmu_disable(vdev);
> >>
> >> @@ -348,6 +451,10 @@ static int ivpu_dev_init(struct ivpu_device *vdev)
> >> if (!vdev->mmu)
> >> return -ENOMEM;
> >>
> >> + vdev->fw = drmm_kzalloc(&vdev->drm, sizeof(*vdev->fw), GFP_KERNEL);
> >> + if (!vdev->fw)
> >> + return -ENOMEM;
> >> +
> >> vdev->ipc = drmm_kzalloc(&vdev->drm, sizeof(*vdev->ipc), GFP_KERNEL);
> >> if (!vdev->ipc)
> >> return -ENOMEM;
> >> @@ -356,6 +463,7 @@ static int ivpu_dev_init(struct ivpu_device *vdev)
> >> vdev->platform = IVPU_PLATFORM_INVALID;
> >> vdev->context_xa_limit.min = IVPU_GLOBAL_CONTEXT_MMU_SSID + 1;
> >> vdev->context_xa_limit.max = IVPU_CONTEXT_LIMIT;
> >> + atomic64_set(&vdev->unique_id_counter, 0);
> >> xa_init_flags(&vdev->context_xa, XA_FLAGS_ALLOC);
> >>
> >> ret = ivpu_pci_init(vdev);
> >> @@ -396,14 +504,34 @@ static int ivpu_dev_init(struct ivpu_device *vdev)
> >> goto err_mmu_gctx_fini;
> >> }
> >>
> >> + ret = ivpu_fw_init(vdev);
> >> + if (ret) {
> >> + ivpu_err(vdev, "Failed to initialize firmware: %d\n", ret);
> >> + goto err_mmu_gctx_fini;
> >> + }
> >> +
> >> ret = ivpu_ipc_init(vdev);
> >> if (ret) {
> >> ivpu_err(vdev, "Failed to initialize IPC: %d\n", ret);
> >> - goto err_mmu_gctx_fini;
> >> + goto err_fw_fini;
> >> + }
> >> +
> >> + ret = ivpu_fw_load(vdev);
> >> + if (ret) {
> >> + ivpu_err(vdev, "Failed to load firmware: %d\n", ret);
> >> + goto err_fw_fini;
> >> + }
> >> +
> >> + ret = ivpu_boot(vdev);
> >> + if (ret) {
> >> + ivpu_err(vdev, "Failed to boot: %d\n", ret);
> >> + goto err_fw_fini;
> >> }
> >>
> >> return 0;
> >>
> >> +err_fw_fini:
> >> + ivpu_fw_fini(vdev);
> >> err_mmu_gctx_fini:
> >> ivpu_mmu_global_context_fini(vdev);
> >> err_power_down:
> >> @@ -417,6 +545,7 @@ static void ivpu_dev_fini(struct ivpu_device *vdev)
> >> {
> >> ivpu_shutdown(vdev);
> >> ivpu_ipc_fini(vdev);
> >> + ivpu_fw_fini(vdev);
> >> ivpu_mmu_global_context_fini(vdev);
> >>
> >> drm_WARN_ON(&vdev->drm, !xa_empty(&vdev->context_xa));
> >> diff --git a/drivers/accel/ivpu/ivpu_drv.h b/drivers/accel/ivpu/ivpu_drv.h
> >> index c1e76d1fb8ba..317ae61f43bd 100644
> >> --- a/drivers/accel/ivpu/ivpu_drv.h
> >> +++ b/drivers/accel/ivpu/ivpu_drv.h
> >> @@ -74,6 +74,7 @@ struct ivpu_wa_table {
> >>
> >> struct ivpu_hw_info;
> >> struct ivpu_mmu_info;
> >> +struct ivpu_fw_info;
> >> struct ivpu_ipc_info;
> >>
> >> struct ivpu_device {
> >> @@ -86,12 +87,15 @@ struct ivpu_device {
> >> struct ivpu_wa_table wa;
> >> struct ivpu_hw_info *hw;
> >> struct ivpu_mmu_info *mmu;
> >> + struct ivpu_fw_info *fw;
> >> struct ivpu_ipc_info *ipc;
> >>
> >> struct ivpu_mmu_context gctx;
> >> struct xarray context_xa;
> >> struct xa_limit context_xa_limit;
> >>
> >> + atomic64_t unique_id_counter;
> >> +
> >> struct {
> >> int boot;
> >> int jsm;
> >> @@ -116,9 +120,16 @@ extern int ivpu_dbg_mask;
> >> extern u8 ivpu_pll_min_ratio;
> >> extern u8 ivpu_pll_max_ratio;
> >>
> >> +#define IVPU_TEST_MODE_DISABLED 0
> >> +#define IVPU_TEST_MODE_FW_TEST 1
> >> +#define IVPU_TEST_MODE_NULL_HW 2
> >> +extern int ivpu_test_mode;
> >> +
> >> struct ivpu_file_priv *ivpu_file_priv_get(struct ivpu_file_priv *file_priv);
> >> struct ivpu_file_priv *ivpu_file_priv_get_by_ctx_id(struct ivpu_device *vdev, unsigned long id);
> >> void ivpu_file_priv_put(struct ivpu_file_priv **link);
> >> +
> >> +int ivpu_boot(struct ivpu_device *vdev);
> >> int ivpu_shutdown(struct ivpu_device *vdev);
> >>
> >> static inline bool ivpu_is_mtl(struct ivpu_device *vdev)
> >> diff --git a/drivers/accel/ivpu/ivpu_fw.c b/drivers/accel/ivpu/ivpu_fw.c
> >> new file mode 100644
> >> index 000000000000..4baa0767a10d
> >> --- /dev/null
> >> +++ b/drivers/accel/ivpu/ivpu_fw.c
> >> @@ -0,0 +1,419 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (C) 2020-2023 Intel Corporation
> >> + */
> >> +
> >> +#include <linux/firmware.h>
> >> +#include <linux/highmem.h>
> >> +#include <linux/moduleparam.h>
> >> +#include <linux/pci.h>
> >> +
> >> +#include "vpu_boot_api.h"
> >> +#include "ivpu_drv.h"
> >> +#include "ivpu_fw.h"
> >> +#include "ivpu_gem.h"
> >> +#include "ivpu_hw.h"
> >> +#include "ivpu_ipc.h"
> >> +
> >> +#define FW_GLOBAL_MEM_START (2ull * SZ_1G)
> >> +#define FW_GLOBAL_MEM_END (3ull * SZ_1G)
> >> +#define FW_SHARED_MEM_SIZE SZ_256M /* Must be aligned to FW_SHARED_MEM_ALIGNMENT */
> >> +#define FW_SHARED_MEM_ALIGNMENT SZ_128K /* VPU MTRR limitation */
> >> +#define FW_RUNTIME_MAX_SIZE SZ_512M
> >> +#define FW_SHAVE_NN_MAX_SIZE SZ_2M
> >> +#define FW_RUNTIME_MIN_ADDR (FW_GLOBAL_MEM_START)
> >> +#define FW_RUNTIME_MAX_ADDR (FW_GLOBAL_MEM_END - FW_SHARED_MEM_SIZE)
> >> +#define FW_VERSION_HEADER_SIZE SZ_4K
> >> +#define FW_FILE_IMAGE_OFFSET (VPU_FW_HEADER_SIZE + FW_VERSION_HEADER_SIZE)
> >> +
> >> +#define WATCHDOG_MSS_REDIRECT 32
> >> +#define WATCHDOG_NCE_REDIRECT 33
> >> +
> >> +#define ADDR_TO_L2_CACHE_CFG(addr) ((addr) >> 31)
> >> +
> >> +#define IVPU_FW_CHECK_API(vdev, fw_hdr, name) ivpu_fw_check_api(vdev, fw_hdr, #name, \
> >> + VPU_##name##_API_VER_INDEX, \
> >> + VPU_##name##_API_VER_MAJOR, \
> >> + VPU_##name##_API_VER_MINOR)
> >> +
> >> +static char *ivpu_firmware;
> >> +module_param_named_unsafe(firmware, ivpu_firmware, charp, 0644);
> >> +MODULE_PARM_DESC(firmware, "VPU firmware binary in /lib/firmware/..");
> >> +
> >> +static int ivpu_fw_request(struct ivpu_device *vdev)
> >> +{
> >> + static const char * const fw_names[] = {
> >> + "mtl_vpu.bin",
> >> + "intel/vpu/mtl_vpu_v0.0.bin"
> >> + };
> >> + int ret = -ENOENT;
> >> + int i;
> >> +
> >> + if (ivpu_firmware)
> >> + return request_firmware(&vdev->fw->file, ivpu_firmware, vdev->drm.dev);
> >> +
> >> + for (i = 0; i < ARRAY_SIZE(fw_names); i++) {
> >> + ret = firmware_request_nowarn(&vdev->fw->file, fw_names[i], vdev->drm.dev);
> >> + if (!ret)
> >> + return 0;
> >> + }
> >> +
> >> + ivpu_err(vdev, "Failed to request firmware: %d\n", ret);
> >> + return ret;
> >> +}
> >> +
> >> +static void
> >> +ivpu_fw_check_api(struct ivpu_device *vdev, const struct vpu_firmware_header *fw_hdr,
> >> + const char *str, int index, u16 expected_major, u16 expected_minor)
> >> +{
> >> + u16 major = (u16)(fw_hdr->api_version[index] >> 16);
> >> + u16 minor = (u16)(fw_hdr->api_version[index]);
> >> +
> >> + if (major != expected_major) {
> >> + ivpu_warn(vdev, "Incompatible FW %s API version: %d.%d (expected %d.%d)\n",
> >> + str, major, minor, expected_major, expected_minor);
> >> + }
> >> + ivpu_dbg(vdev, FW_BOOT, "FW %s API version: %d.%d (expected %d.%d)\n",
> >> + str, major, minor, expected_major, expected_minor);
> >> +}
> >> +
> >> +static int ivpu_fw_parse(struct ivpu_device *vdev)
> >> +{
> >> + struct ivpu_fw_info *fw = vdev->fw;
> >> + const struct vpu_firmware_header *fw_hdr = (const void *)fw->file->data;
> >> + u64 runtime_addr, image_load_addr, runtime_size, image_size;
> >> +
> >> + if (fw->file->size <= FW_FILE_IMAGE_OFFSET) {
> >> + ivpu_err(vdev, "Firmware file is too small: %zu\n", fw->file->size);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (fw_hdr->header_version != VPU_FW_HEADER_VERSION) {
> >> + ivpu_err(vdev, "Invalid firmware header version: %u\n", fw_hdr->header_version);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + runtime_addr = fw_hdr->boot_params_load_address;
> >> + runtime_size = fw_hdr->runtime_size;
> >> + image_load_addr = fw_hdr->image_load_address;
> >> + image_size = fw_hdr->image_size;
> >> +
> >> + if (runtime_addr < FW_RUNTIME_MIN_ADDR || runtime_addr > FW_RUNTIME_MAX_ADDR) {
> >> + ivpu_err(vdev, "Invalid firmware runtime address: 0x%llx\n", runtime_addr);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (runtime_size < fw->file->size || runtime_size > FW_RUNTIME_MAX_SIZE) {
> >> + ivpu_err(vdev, "Invalid firmware runtime size: %llu\n", runtime_size);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (FW_FILE_IMAGE_OFFSET + image_size > fw->file->size) {
> >> + ivpu_err(vdev, "Invalid image size: %llu\n", image_size);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (image_load_addr < runtime_addr ||
> >> + image_load_addr + image_size > runtime_addr + runtime_size) {
> >> + ivpu_err(vdev, "Invalid firmware load address size: 0x%llx and size %llu\n",
> >> + image_load_addr, image_size);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (fw_hdr->shave_nn_fw_size > FW_SHAVE_NN_MAX_SIZE) {
> >> + ivpu_err(vdev, "SHAVE NN firmware is too big: %u\n", fw_hdr->shave_nn_fw_size);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (fw_hdr->entry_point < image_load_addr ||
> >> + fw_hdr->entry_point >= image_load_addr + image_size) {
> >> + ivpu_err(vdev, "Invalid entry point: 0x%llx\n", fw_hdr->entry_point);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + fw->runtime_addr = runtime_addr;
> >> + fw->runtime_size = runtime_size;
> >> + fw->image_load_offset = image_load_addr - runtime_addr;
> >> + fw->image_size = image_size;
> >> + fw->shave_nn_size = PAGE_ALIGN(fw_hdr->shave_nn_fw_size);
> >> +
> >> + fw->cold_boot_entry_point = fw_hdr->entry_point;
> >> + fw->entry_point = fw->cold_boot_entry_point;
> >> +
> >> + ivpu_dbg(vdev, FW_BOOT, "Header version: 0x%x, format 0x%x\n",
> >> + fw_hdr->header_version, fw_hdr->image_format);
> >> + ivpu_dbg(vdev, FW_BOOT, "Size: file %lu image %u runtime %u shavenn %u\n",
> >> + fw->file->size, fw->image_size, fw->runtime_size, fw->shave_nn_size);
> >> + ivpu_dbg(vdev, FW_BOOT, "Address: runtime 0x%llx, load 0x%llx, entry point 0x%llx\n",
> >> + fw->runtime_addr, image_load_addr, fw->entry_point);
> >> + ivpu_dbg(vdev, FW_BOOT, "FW version: %s\n", (char *)fw_hdr + VPU_FW_HEADER_SIZE);
> >> +
> >> + IVPU_FW_CHECK_API(vdev, fw_hdr, BOOT);
> >> + IVPU_FW_CHECK_API(vdev, fw_hdr, JSM);
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void ivpu_fw_release(struct ivpu_device *vdev)
> >> +{
> >> + release_firmware(vdev->fw->file);
> >> +}
> >> +
> >> +static int ivpu_fw_update_global_range(struct ivpu_device *vdev)
> >> +{
> >> + struct ivpu_fw_info *fw = vdev->fw;
> >> + u64 start = ALIGN(fw->runtime_addr + fw->runtime_size, FW_SHARED_MEM_ALIGNMENT);
> >> + u64 size = FW_SHARED_MEM_SIZE;
> >> +
> >> + if (start + size > FW_GLOBAL_MEM_END) {
> >> + ivpu_err(vdev, "No space for shared region, start %lld, size %lld\n", start, size);
> >> + return -EINVAL;
> >> + }
> >> +
> >> + ivpu_hw_init_range(&vdev->hw->ranges.global_low, start, size);
> >> + return 0;
> >> +}
> >> +
> >> +static int ivpu_fw_mem_init(struct ivpu_device *vdev)
> >> +{
> >> + struct ivpu_fw_info *fw = vdev->fw;
> >> + int ret;
> >> +
> >> + ret = ivpu_fw_update_global_range(vdev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + fw->mem = ivpu_bo_alloc_internal(vdev, fw->runtime_addr, fw->runtime_size, DRM_IVPU_BO_WC);
> >> + if (!fw->mem) {
> >> + ivpu_err(vdev, "Failed to allocate firmware runtime memory\n");
> >> + return -ENOMEM;
> >> + }
> >> +
> >> + if (fw->shave_nn_size) {
> >> + fw->mem_shave_nn = ivpu_bo_alloc_internal(vdev, vdev->hw->ranges.global_high.start,
> >> + fw->shave_nn_size, DRM_IVPU_BO_UNCACHED);
> >> + if (!fw->mem_shave_nn) {
> >> + ivpu_err(vdev, "Failed to allocate shavenn buffer\n");
> >> + ivpu_bo_free_internal(fw->mem);
> >> + return -ENOMEM;
> >> + }
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static void ivpu_fw_mem_fini(struct ivpu_device *vdev)
> >> +{
> >> + struct ivpu_fw_info *fw = vdev->fw;
> >> +
> >> + if (fw->mem_shave_nn) {
> >> + ivpu_bo_free_internal(fw->mem_shave_nn);
> >> + fw->mem_shave_nn = NULL;
> >> + }
> >> +
> >> + ivpu_bo_free_internal(fw->mem);
> >> + fw->mem = NULL;
> >> +}
> >> +
> >> +int ivpu_fw_init(struct ivpu_device *vdev)
> >> +{
> >> + int ret;
> >> +
> >> + ret = ivpu_fw_request(vdev);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + ret = ivpu_fw_parse(vdev);
> >> + if (ret)
> >> + goto err_fw_release;
> >> +
> >> + ret = ivpu_fw_mem_init(vdev);
> >> + if (ret)
> >> + goto err_fw_release;
> >> +
> >> + return 0;
> >> +
> >> +err_fw_release:
> >> + ivpu_fw_release(vdev);
> > Why do you release the firmware only on error ?
> > Why maintain the hold on the firmware file for as long as the device lives ?
> > Can't you release the file once you loaded the firmware to the device's memory ?
>
> Yes, we keep the fw image in memory for the following reasons:
> 1. It reduces the time of reset, recovery and resume without warmboot, where we need to overwite the fw.
> 2. It prevents user space from changing the fw binary while the module is loaded.
> If a new version of the fw was installed it will be used on the next insmod which prevents hard to detect bugs.
>
> Regards,
> Jacek
ok, sounds fair enough.
So you can add my r-b to this patch as well.
Oded
