Re: [PATCH v3 20/29] media: iris: subscribe parameters and properties to firmware for hfi_gen2

Bryan O'Donoghue
2024-09-24T15:16:09Z

On 27/08/2024 11:05, Dikshita Agarwal via B4 Relay wrote:
> From: Vedang Nagar <quic_vnagar@quicinc.com>
>
> For hfi_gen2, subscribe for different bitstream parameters to
> firmware to get notified for change in any of the subscribed
> parameters.
>
> Signed-off-by: Vedang Nagar <quic_vnagar@quicinc.com>
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> ---
> drivers/media/platform/qcom/iris/iris_hfi_gen2.h | 6 +
> .../platform/qcom/iris/iris_hfi_gen2_command.c | 179 +++++++++++++++++++++
> .../platform/qcom/iris/iris_hfi_gen2_defines.h | 9 ++
> .../platform/qcom/iris/iris_platform_common.h | 4 +
> .../platform/qcom/iris/iris_platform_sm8550.c | 13 ++
> 5 files changed, 211 insertions(+)
>
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2.h b/drivers/media/platform/qcom/iris/iris_hfi_gen2.h
> index 8170c1fef569..5fbbab844025 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2.h
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2.h
> @@ -18,12 +18,18 @@ struct iris_core;
> *
> * @inst: pointer to iris_instance structure
> * @packet: HFI packet
> + * @ipsc_properties_set: boolean to set ipsc properties to fw
> + * @opsc_properties_set: boolean to set opsc properties to fw
> * @src_subcr_params: subscription params to fw on input port
> + * @dst_subcr_params: subscription params to fw on output port
> */
> struct iris_inst_hfi_gen2 {
> struct iris_inst inst;
> struct iris_hfi_header *packet;
> + bool ipsc_properties_set;
> + bool opsc_properties_set;
> struct hfi_subscription_params src_subcr_params;
> + struct hfi_subscription_params dst_subcr_params;
> };
>
> void iris_hfi_gen2_command_ops_init(struct iris_core *core);
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> index e50f00021f6d..791b535a3584 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen2_command.c
> @@ -472,6 +472,9 @@ static int iris_hfi_gen2_session_open(struct iris_inst *inst)
> if (inst->state != IRIS_INST_DEINIT)
> return -EALREADY;
>
> + inst_hfi_gen2->ipsc_properties_set = false;
> + inst_hfi_gen2->opsc_properties_set = false;
> +
> inst_hfi_gen2->packet = kzalloc(4096, GFP_KERNEL);
> if (!inst_hfi_gen2->packet)
> return -ENOMEM;
> @@ -536,9 +539,185 @@ static int iris_hfi_gen2_session_close(struct iris_inst *inst)
> return ret;
> }
>
> +static int iris_hfi_gen2_session_subscribe_mode(struct iris_inst *inst,
> + u32 cmd, u32 plane, u32 payload_type,
> + void *payload, u32 payload_size)
> +{
> + struct iris_inst_hfi_gen2 *inst_hfi_gen2 = to_iris_inst_hfi_gen2(inst);
> +
> + iris_hfi_gen2_packet_session_command(inst,
> + cmd,
> + (HFI_HOST_FLAGS_RESPONSE_REQUIRED |
> + HFI_HOST_FLAGS_INTR_REQUIRED),
> + iris_hfi_gen2_get_port(plane),
> + inst->session_id,
> + payload_type,
> + payload,
> + payload_size);
> +
> + return iris_hfi_queue_cmd_write(inst->core, inst_hfi_gen2->packet,
> + inst_hfi_gen2->packet->size);
> +}
> +
> +static int iris_hfi_gen2_subscribe_change_param(struct iris_inst *inst, u32 plane)
> +{
> + struct iris_inst_hfi_gen2 *inst_hfi_gen2 = to_iris_inst_hfi_gen2(inst);
> + struct hfi_subscription_params subsc_params;
> + u32 prop_type, payload_size, payload_type;
> + struct iris_core *core = inst->core;
> + const u32 *change_param = NULL;
> + u32 change_param_size = 0;
> + u32 payload[32] = {0};
> + u32 hfi_port = 0;
> + int ret;
> + u32 i;
> +
> + if ((V4L2_TYPE_IS_OUTPUT(plane) && inst_hfi_gen2->ipsc_properties_set) ||
> + (V4L2_TYPE_IS_CAPTURE(plane) && inst_hfi_gen2->opsc_properties_set)) {
> + dev_err(core->dev, "invalid plane\n");
> + return 0;
> + }
> +
> + change_param = core->iris_platform_data->input_config_params;
> + change_param_size = core->iris_platform_data->input_config_params_size;
> +
> + if (!change_param || !change_param_size)
> + return -EINVAL;

That's an odd one, checking for zero but _not_ bounds checking
chanage_param_size < (sizeof(payload)/sizeof(u32)) - 1

I'm not sure where inpug_config_param_size gets populated but I'd rather
check that type of parameter - for exactly that reason - than the
defensive coding done on your inputs elsewhere.

TL;DR why do you trust change_param_size < your array size but not
change_param_size >= 1 ?

---
bod