On 18/12/2023 11:31, Dikshita Agarwal wrote:
Re-organize the video driver code by introducing a new folder
'vcodec' and placing 'venus' driver code inside that.
Introduce common helpers for trustzone based firmware
load/unload etc. which are placed in common folder
i.e. 'vcodec'.
Use these helpers in 'venus' driver. These helpers will be
used by 'iris' driver as well which is introduced later
in this patch series.
Signed-off-by: Dikshita Agarwal <quic_dikshita@xxxxxxxxxxx>
---
This is a very large patch, I think it needs to be broken up into
smaller chunks.
#1 Introduce common helper functions
#2 Use common helper functions
Its alot of code to try to eat in the one go.
Could you consider making patches 1-3 a standalone series to reduce the
amount of code to review here ?
* 77e7025529d7c - (HEAD -> linux-stable-master-23-12-18-iris-v2) media:
iris: add power management for encoder (21 hours ago)
* ceb6a6f023fd3 - (tag: v6.7-rc6, linux-stable/master) Linux 6.7-rc6 (2
days ago)
git diff ceb6a6f023fd3 | wc -l
21243
Also I feel it wouild give more time for the changes to "digest" though
upstream users and to find any unintended bugs.
+int load_fw(struct device *dev, const char *fw_name, phys_addr_t *mem_phys,
+ size_t *mem_size, u32 pas_id, bool use_tz)
+{
+ const struct firmware *firmware = NULL;
+ struct reserved_mem *rmem;
+ struct device_node *node;
+ void *mem_virt = NULL;
+ ssize_t fw_size = 0;
+ int ret;
+
+ if (!IS_ENABLED(CONFIG_QCOM_MDT_LOADER) ||
+ (use_tz && !qcom_scm_is_available()))
+ return -EPROBE_DEFER;
+
+ if (!fw_name || !(*fw_name))
+ return -EINVAL;
The parameter check should come before the qcom_scm_is_available()
No matter how many times you -EPROBE_DEFER -EINVAL is still -EINVAL.
+
+ *mem_phys = 0;
+ *mem_size = 0;
I don't think you need this, you don't appear to use these variables
before you assign them below.
+
+ *mem_phys = rmem->base;
+ *mem_size = rmem->size;
+
+int auth_reset_fw(u32 pas_id)
+{
+ return qcom_scm_pas_auth_and_reset(pas_id);
+}
+
+void unload_fw(u32 pas_id)
+{
+ qcom_scm_pas_shutdown(pas_id);
+}
+
Do these wrapper functions add anything ? Some kind of validity check on
the pas_id otherwise I'm not sure these are justified.
+int set_hw_state(bool resume)
+{
+ return qcom_scm_set_remote_state(resume, 0);
+}
diff --git a/drivers/media/platform/qcom/vcodec/firmware.h b/drivers/media/platform/qcom/vcodec/firmware.h
new file mode 100644
index 0000000..7d410a8
--- /dev/null
---
bod
