2023-12-18 11:34:01

by Dikshita Agarwal

[permalink] [raw]
Subject: [PATCH v2 10/34] media: iris: add PIL functionality for video firmware

Load/unload firmware in memory via mdt loader.
Firmware loading is part of core initialization
and unloading is part of core de-initialization.
This also changes the core states accordingly.

Signed-off-by: Dikshita Agarwal <[email protected]>
---
drivers/media/platform/qcom/vcodec/iris/Makefile | 10 ++-
.../media/platform/qcom/vcodec/iris/iris_core.c | 70 +++++++++++++++++++++
.../media/platform/qcom/vcodec/iris/iris_core.h | 8 +++
.../media/platform/qcom/vcodec/iris/iris_helpers.c | 15 +++++
.../media/platform/qcom/vcodec/iris/iris_helpers.h | 13 ++++
drivers/media/platform/qcom/vcodec/iris/iris_hfi.c | 72 ++++++++++++++++++++++
drivers/media/platform/qcom/vcodec/iris/iris_hfi.h | 14 +++++
.../media/platform/qcom/vcodec/iris/iris_probe.c | 19 +++++-
.../media/platform/qcom/vcodec/iris/iris_state.c | 9 ++-
9 files changed, 225 insertions(+), 5 deletions(-)
create mode 100644 drivers/media/platform/qcom/vcodec/iris/iris_core.c
create mode 100644 drivers/media/platform/qcom/vcodec/iris/iris_helpers.c
create mode 100644 drivers/media/platform/qcom/vcodec/iris/iris_helpers.h
create mode 100644 drivers/media/platform/qcom/vcodec/iris/iris_hfi.c
create mode 100644 drivers/media/platform/qcom/vcodec/iris/iris_hfi.h

diff --git a/drivers/media/platform/qcom/vcodec/iris/Makefile b/drivers/media/platform/qcom/vcodec/iris/Makefile
index 59798e5d..74bd344 100644
--- a/drivers/media/platform/qcom/vcodec/iris/Makefile
+++ b/drivers/media/platform/qcom/vcodec/iris/Makefile
@@ -1,7 +1,11 @@
-iris-objs += ../hfi_queue.o
+iris-objs += ../hfi_queue.o ../firmware.o

iris-objs += iris_probe.o \
- resources.o \
- iris_state.o
+ iris_state.o \
+ iris_core.o \
+ iris_state.o \
+ iris_helpers.o \
+ iris_hfi.o \
+ resources.o

obj-$(CONFIG_VIDEO_QCOM_IRIS) += iris.o
diff --git a/drivers/media/platform/qcom/vcodec/iris/iris_core.c b/drivers/media/platform/qcom/vcodec/iris/iris_core.c
new file mode 100644
index 0000000..ba8960d
--- /dev/null
+++ b/drivers/media/platform/qcom/vcodec/iris/iris_core.c
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include "iris_core.h"
+#include "iris_helpers.h"
+#include "iris_hfi.h"
+#include "iris_state.h"
+
+static int iris_core_deinit_locked(struct iris_core *core)
+{
+ int ret;
+
+ ret = check_core_lock(core);
+ if (ret)
+ return ret;
+
+ if (core->state == IRIS_CORE_DEINIT)
+ return 0;
+
+ iris_hfi_core_deinit(core);
+
+ iris_change_core_state(core, IRIS_CORE_DEINIT);
+
+ return ret;
+}
+
+int iris_core_deinit(struct iris_core *core)
+{
+ int ret;
+
+ mutex_lock(&core->lock);
+ ret = iris_core_deinit_locked(core);
+ mutex_unlock(&core->lock);
+
+ return ret;
+}
+
+int iris_core_init(struct iris_core *core)
+{
+ int ret = 0;
+
+ mutex_lock(&core->lock);
+ if (core_in_valid_state(core)) {
+ goto unlock;
+ } else if (core->state == IRIS_CORE_ERROR) {
+ ret = -EINVAL;
+ goto unlock;
+ }
+
+ if (iris_change_core_state(core, IRIS_CORE_INIT_WAIT)) {
+ iris_change_core_state(core, IRIS_CORE_ERROR);
+ ret = -EINVAL;
+ goto unlock;
+ }
+
+ ret = iris_hfi_core_init(core);
+ if (ret) {
+ iris_change_core_state(core, IRIS_CORE_ERROR);
+ dev_err(core->dev, "%s: core init failed\n", __func__);
+ iris_core_deinit_locked(core);
+ goto unlock;
+ }
+
+unlock:
+ mutex_unlock(&core->lock);
+
+ return ret;
+}
diff --git a/drivers/media/platform/qcom/vcodec/iris/iris_core.h b/drivers/media/platform/qcom/vcodec/iris/iris_core.h
index 77124f9..2740ff1 100644
--- a/drivers/media/platform/qcom/vcodec/iris/iris_core.h
+++ b/drivers/media/platform/qcom/vcodec/iris/iris_core.h
@@ -11,6 +11,7 @@

#include "../hfi_queue.h"
#include "iris_state.h"
+#include "resources.h"

/**
* struct iris_core - holds core parameters valid for all instances
@@ -36,6 +37,8 @@
* @message_queue: shared interface queue to receive responses from firmware
* @debug_queue: shared interface queue to receive debug info from firmware
* @sfr: SFR register memory
+ * @lock: a lock for this strucure
+ * @use_tz: a flag that suggests presence of trustzone
*/

struct iris_core {
@@ -60,6 +63,11 @@ struct iris_core {
struct iface_q_info message_queue;
struct iface_q_info debug_queue;
struct mem_desc sfr;
+ struct mutex lock; /* lock for core structure */
+ unsigned int use_tz;
};

+int iris_core_init(struct iris_core *core);
+int iris_core_deinit(struct iris_core *core);
+
#endif
diff --git a/drivers/media/platform/qcom/vcodec/iris/iris_helpers.c b/drivers/media/platform/qcom/vcodec/iris/iris_helpers.c
new file mode 100644
index 0000000..22c706a
--- /dev/null
+++ b/drivers/media/platform/qcom/vcodec/iris/iris_helpers.c
@@ -0,0 +1,15 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include "iris_helpers.h"
+
+int check_core_lock(struct iris_core *core)
+{
+ bool fatal = !mutex_is_locked(&core->lock);
+
+ WARN_ON(fatal);
+
+ return fatal ? -EINVAL : 0;
+}
diff --git a/drivers/media/platform/qcom/vcodec/iris/iris_helpers.h b/drivers/media/platform/qcom/vcodec/iris/iris_helpers.h
new file mode 100644
index 0000000..314a8d75
--- /dev/null
+++ b/drivers/media/platform/qcom/vcodec/iris/iris_helpers.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _IRIS_HELPERS_H_
+#define _IRIS_HELPERS_H_
+
+#include "iris_core.h"
+
+int check_core_lock(struct iris_core *core);
+
+#endif
diff --git a/drivers/media/platform/qcom/vcodec/iris/iris_hfi.c b/drivers/media/platform/qcom/vcodec/iris/iris_hfi.c
new file mode 100644
index 0000000..4f51a8c
--- /dev/null
+++ b/drivers/media/platform/qcom/vcodec/iris/iris_hfi.c
@@ -0,0 +1,72 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include "../firmware.h"
+#include "iris_helpers.h"
+#include "iris_hfi.h"
+
+#define CP_START 0
+#define CP_SIZE 0x25800000
+#define CP_NONPIXEL_START 0x01000000
+#define CP_NONPIXEL_SIZE 0x24800000
+
+#define FW_NAME "vpu30_4v.mbn"
+#define IRIS_PAS_ID 9
+
+int iris_hfi_core_init(struct iris_core *core)
+{
+ phys_addr_t mem_phys = 0;
+ size_t mem_size = 0;
+ int ret;
+
+ ret = check_core_lock(core);
+ if (ret)
+ return ret;
+
+ ret = hfi_queue_init(core->dev, &core->iface_q_table, &core->sfr,
+ &core->command_queue, &core->message_queue,
+ &core->debug_queue, core);
+ if (ret)
+ goto error;
+
+ core->use_tz = use_tz(core->dev);
+ if (!core->use_tz)
+ goto error;
+
+ ret = load_fw(core->dev, FW_NAME, &mem_phys, &mem_size,
+ IRIS_PAS_ID, core->use_tz);
+ if (ret)
+ goto error;
+
+ ret = auth_reset_fw(IRIS_PAS_ID);
+ if (ret)
+ goto error;
+
+ ret = protect_secure_region(CP_START, CP_SIZE, CP_NONPIXEL_START,
+ CP_NONPIXEL_SIZE, IRIS_PAS_ID);
+
+ return ret;
+
+error:
+ dev_err(core->dev, "%s(): failed with ret %d\n", __func__, ret);
+
+ return ret;
+}
+
+int iris_hfi_core_deinit(struct iris_core *core)
+{
+ int ret;
+
+ ret = check_core_lock(core);
+ if (ret)
+ return ret;
+
+ if (core->state == IRIS_CORE_DEINIT)
+ return 0;
+
+ unload_fw(IRIS_PAS_ID);
+
+ return ret;
+}
diff --git a/drivers/media/platform/qcom/vcodec/iris/iris_hfi.h b/drivers/media/platform/qcom/vcodec/iris/iris_hfi.h
new file mode 100644
index 0000000..fcf9f28
--- /dev/null
+++ b/drivers/media/platform/qcom/vcodec/iris/iris_hfi.h
@@ -0,0 +1,14 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2022-2023 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#ifndef _IRIS_HFI_H_
+#define _IRIS_HFI_H_
+
+#include "iris_core.h"
+
+int iris_hfi_core_init(struct iris_core *core);
+int iris_hfi_core_deinit(struct iris_core *core);
+
+#endif
diff --git a/drivers/media/platform/qcom/vcodec/iris/iris_probe.c b/drivers/media/platform/qcom/vcodec/iris/iris_probe.c
index fd349a3..f39b4aa 100644
--- a/drivers/media/platform/qcom/vcodec/iris/iris_probe.c
+++ b/drivers/media/platform/qcom/vcodec/iris/iris_probe.c
@@ -51,6 +51,7 @@ static void iris_remove(struct platform_device *pdev)
if (!core)
return;

+ iris_core_deinit(core);
hfi_queue_deinit(core->dev, &core->iface_q_table, &core->sfr,
&core->command_queue, &core->message_queue,
&core->debug_queue);
@@ -58,6 +59,9 @@ static void iris_remove(struct platform_device *pdev)
video_unregister_device(core->vdev_dec);

v4l2_device_unregister(&core->v4l2_dev);
+
+ mutex_destroy(&core->lock);
+ core->state = IRIS_CORE_DEINIT;
}

static int iris_probe(struct platform_device *pdev)
@@ -72,6 +76,9 @@ static int iris_probe(struct platform_device *pdev)
return -ENOMEM;
core->dev = dev;

+ core->state = IRIS_CORE_DEINIT;
+ mutex_init(&core->lock);
+
core->reg_base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(core->reg_base))
return PTR_ERR(core->reg_base);
@@ -120,10 +127,20 @@ static int iris_probe(struct platform_device *pdev)
goto err_vdev_unreg;
}

+ ret = iris_core_init(core);
+ if (ret) {
+ dev_err_probe(core->dev, ret, "%s: core init failed\n", __func__);
+ goto err_queue_deinit;
+ }
+
return ret;

+err_queue_deinit:
+ hfi_queue_deinit(core->dev, &core->iface_q_table, &core->sfr,
+ &core->command_queue, &core->message_queue,
+ &core->debug_queue);
err_vdev_unreg:
- iris_unregister_video_device(core);
+ video_unregister_device(core->vdev_dec);
err_v4l2_unreg:
v4l2_device_unregister(&core->v4l2_dev);

diff --git a/drivers/media/platform/qcom/vcodec/iris/iris_state.c b/drivers/media/platform/qcom/vcodec/iris/iris_state.c
index 22557af..83bbc6b 100644
--- a/drivers/media/platform/qcom/vcodec/iris/iris_state.c
+++ b/drivers/media/platform/qcom/vcodec/iris/iris_state.c
@@ -4,6 +4,7 @@
*/

#include "iris_core.h"
+#include "iris_helpers.h"
#include "iris_state.h"

#define IRIS_STATE(name)[IRIS_CORE_##name] = "CORE_"#name
@@ -52,6 +53,12 @@ static bool iris_allow_core_state_change(struct iris_core *core,
int iris_change_core_state(struct iris_core *core,
enum iris_core_state request_state)
{
+ int ret;
+
+ ret = check_core_lock(core);
+ if (ret)
+ return ret;
+
if (core->state == request_state)
return 0;

@@ -60,5 +67,5 @@ int iris_change_core_state(struct iris_core *core,

core->state = request_state;

- return 0;
+ return ret;
}
--
2.7.4



2023-12-18 21:41:07

by Konrad Dybcio

[permalink] [raw]
Subject: Re: [PATCH v2 10/34] media: iris: add PIL functionality for video firmware



On 12/18/23 12:32, Dikshita Agarwal wrote:
> Load/unload firmware in memory via mdt loader.
> Firmware loading is part of core initialization
> and unloading is part of core de-initialization.
> This also changes the core states accordingly.
>
> Signed-off-by: Dikshita Agarwal <[email protected]>
> ---
[...]

> +
> +#include "iris_core.h"
> +#include "iris_helpers.h"
> +#include "iris_hfi.h"
> +#include "iris_state.h"
> +
> +static int iris_core_deinit_locked(struct iris_core *core)
I suppose you meant to call this something like _nolock, as
you're calling it with a lock around it

> +{
> + int ret;
> +
> + ret = check_core_lock(core);
> + if (ret)
> + return ret;
> +
> + if (core->state == IRIS_CORE_DEINIT)
> + return 0;
> +
> + iris_hfi_core_deinit(core);
> +
> + iris_change_core_state(core, IRIS_CORE_DEINIT);
You're casually ignoring the return value of the two
above funcs here :/

> +
> + return ret;
> +}
> +
> +int iris_core_deinit(struct iris_core *core)
> +{
> + int ret;
> +
> + mutex_lock(&core->lock);
> + ret = iris_core_deinit_locked(core);
> + mutex_unlock(&core->lock);
> +
> + return ret;
> +}
> +
> +int iris_core_init(struct iris_core *core)
> +{
> + int ret = 0;
> +
> + mutex_lock(&core->lock);
You may be interested in scoped mutexes

> + if (core_in_valid_state(core)) {
> + goto unlock;
> + } else if (core->state == IRIS_CORE_ERROR) {
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> + if (iris_change_core_state(core, IRIS_CORE_INIT_WAIT)) {
> + iris_change_core_state(core, IRIS_CORE_ERROR);
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> + ret = iris_hfi_core_init(core);
> + if (ret) {
> + iris_change_core_state(core, IRIS_CORE_ERROR);
> + dev_err(core->dev, "%s: core init failed\n", __func__);
__func__ still seems overly verbose in my eyes

[...]

> +
> +int check_core_lock(struct iris_core *core)
> +{
> + bool fatal = !mutex_is_locked(&core->lock);
> +
> + WARN_ON(fatal);
> +
> + return fatal ? -EINVAL : 0;
You can simply inline this:

if (WARN_ON(!mutex_is_locked(&core->lock))
return -EINVAL;

[...]
> +#define CP_START 0
> +#define CP_SIZE 0x25800000
> +#define CP_NONPIXEL_START 0x01000000
> +#define CP_NONPIXEL_SIZE 0x24800000
> +
> +#define FW_NAME "vpu30_4v.mbn"
This doesn't scale, at all.

Konrad

2023-12-20 08:16:58

by Dikshita Agarwal

[permalink] [raw]
Subject: Re: [PATCH v2 10/34] media: iris: add PIL functionality for video firmware



On 12/19/2023 3:10 AM, Konrad Dybcio wrote:
>
>
> On 12/18/23 12:32, Dikshita Agarwal wrote:
>> Load/unload firmware in memory via mdt loader.
>> Firmware loading is part of core initialization
>> and unloading is part of core de-initialization.
>> This also changes the core states accordingly.
>>
>> Signed-off-by: Dikshita Agarwal <[email protected]>
>> ---
> [...]
>
>> +
>> +#include "iris_core.h"
>> +#include "iris_helpers.h"
>> +#include "iris_hfi.h"
>> +#include "iris_state.h"
>> +
>> +static int iris_core_deinit_locked(struct iris_core *core)
> I suppose you meant to call this something like _nolock, as
> you're calling it with a lock around it
>
We are trying to coney that this particular API should be called with
locked context only.
doesn't _nolock mean other way? please correct if my understanding is wrong.
>> +{
>> +    int ret;
>> +
>> +    ret = check_core_lock(core);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (core->state == IRIS_CORE_DEINIT)
>> +        return 0;
>> +
>> +    iris_hfi_core_deinit(core);
>> +
>> +    iris_change_core_state(core, IRIS_CORE_DEINIT);
> You're casually ignoring the return value of the two
> above funcs here :/
>
Right, since this is the tear-down sequence, we don't care of the return
value here.
>> +
>> +    return ret;
>> +}
>> +
>> +int iris_core_deinit(struct iris_core *core)
>> +{
>> +    int ret;
>> +
>> +    mutex_lock(&core->lock);
>> +    ret = iris_core_deinit_locked(core);
>> +    mutex_unlock(&core->lock);
>> +
>> +    return ret;
>> +}
>> +
>> +int iris_core_init(struct iris_core *core)
>> +{
>> +    int ret = 0;
>> +
>> +    mutex_lock(&core->lock);
> You may be interested in scoped mutexes
>
Will explore this.
>> +    if (core_in_valid_state(core)) {
>> +        goto unlock;
>> +    } else if (core->state == IRIS_CORE_ERROR) {
>> +        ret = -EINVAL;
>> +        goto unlock;
>> +    }
>> +
>> +    if (iris_change_core_state(core, IRIS_CORE_INIT_WAIT)) {
>> +        iris_change_core_state(core, IRIS_CORE_ERROR);
>> +        ret = -EINVAL;
>> +        goto unlock;
>> +    }
>> +
>> +    ret = iris_hfi_core_init(core);
>> +    if (ret) {
>> +        iris_change_core_state(core, IRIS_CORE_ERROR);
>> +        dev_err(core->dev, "%s: core init failed\n", __func__);
> __func__ still seems overly verbose in my eyes
>
Sure, will remove such instances.
> [...]
>
>> +
>> +int check_core_lock(struct iris_core *core)
>> +{
>> +    bool fatal = !mutex_is_locked(&core->lock);
>> +
>> +    WARN_ON(fatal);
>> +
>> +    return fatal ? -EINVAL : 0;
> You can simply inline this:
>
> if (WARN_ON(!mutex_is_locked(&core->lock))
>     return -EINVAL;
>
Thanks for the suggestion, will update this.
> [...]
>> +#define CP_START           0
>> +#define CP_SIZE            0x25800000
>> +#define CP_NONPIXEL_START  0x01000000
>> +#define CP_NONPIXEL_SIZE   0x24800000
>> +
>> +#define FW_NAME "vpu30_4v.mbn"
> This doesn't scale, at all.
>
As mentioned in previous patches, we have not introduced platform specific
file yet, when I introduce that in later patch, this will be moved to
platform specific file.
> Konrad