Re: [PATCH v2 01/34] media: introduce common helpers for video firmware handling

[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@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