Messages in this thread | | | Date | Tue, 19 Dec 2023 11:40:08 +0000 | Subject | Re: [PATCH v2 01/34] media: introduce common helpers for video firmware handling | From | Bryan O'Donoghue <> |
| |
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@quicinc.com> > ---
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
| |