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

[Dikshita Agarwal <quic_dikshita@quicinc.com>]

On 9/24/2024 8:46 PM, Bryan O'Donoghue wrote:
> 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 ?
> 
These NULL checks here are actually not needed as we will make sure this is
filled in platform data. Will remove these checks in next version and will
see if bound check is required.
> ---
> bod