On 8/16/2025 4:37 PM, Bryan O'Donoghue wrote:
> On 13/08/2025 10:37, Dikshita Agarwal wrote:
>> For HFI Gen1, the instances substate is changed to LOAD_RESOURCES only
>> when a START command is issues to the firmware. If STOP is called
>> without a prior START, the firmware may reject the command and throw
>> some erros.
>> Handle this by adding a substate check before issuing STOP command to
>> the firmware.
>>
>> Fixes: 11712ce70f8e ("media: iris: implement vb2 streaming ops")
>> Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>
>> Tested-by: Vikash Garodia <quic_vgarodia@quicinc.com> # X1E80100
>> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
>> ---
>> drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
>> b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
>> index
>> 5fc30d54af4dc34616cfd08813940aa0b7044a20..5f1748ab80f88393215fc2d82c5c6b4af1266090 100644
>> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
>> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
>> @@ -184,11 +184,12 @@ static int iris_hfi_gen1_session_stop(struct
>> iris_inst *inst, u32 plane)
>> u32 flush_type = 0;
>> int ret = 0;
>> - if ((V4L2_TYPE_IS_OUTPUT(plane) &&
>> - inst->state == IRIS_INST_INPUT_STREAMING) ||
>> + if (((V4L2_TYPE_IS_OUTPUT(plane) &&
>> + inst->state == IRIS_INST_INPUT_STREAMING) ||
>
> this is becoming a highly complex clause
>
> if (((V4L2_TYPE_IS_OUTPUT(plane) &&
> inst->state == IRIS_INST_INPUT_STREAMING) ||
> (V4L2_TYPE_IS_CAPTURE(plane) &&
> inst->state == IRIS_INST_OUTPUT_STREAMING) ||
> inst->state == IRIS_INST_ERROR) &&
> inst->sub_state & IRIS_INST_SUB_LOAD_RESOURCES) {
>
> can we not reduce down the number of conjunctions and dis-junctions here ?
>
> Its getting hard to follow.
>
> For example pivot on if (inst->sub_state & IRIS_INST_SUB_LOAD_RESOURCES)
>
> or make it into a switch for inst->state... no that wouldn't work
>
> Either way the complexity of this clause is indicating to me we need to do
> some decomposition.
>
> Please consider if you can rationalise the logic here and make the code
> more readable.
Sure, I will see if some of these conditions can be absorbed in caller or
if handled by vb2 framework itself.
Thanks,
Dikshita
>> (V4L2_TYPE_IS_CAPTURE(plane) &&
>> inst->state == IRIS_INST_OUTPUT_STREAMING) ||
>> - inst->state == IRIS_INST_ERROR) {
>> + inst->state == IRIS_INST_ERROR) &&
>> + inst->sub_state & IRIS_INST_SUB_LOAD_RESOURCES) {
>> reinit_completion(&inst->completion);
>> iris_hfi_gen1_packet_session_cmd(inst, &pkt,
>> HFI_CMD_SESSION_STOP);
>> ret = iris_hfi_queue_cmd_write(core, &pkt, pkt.shdr.hdr.size);
>>
>
> ---
> bod