Re: [PATCH v2 08/24] media: iris: Allow stop on firmware only if start was issued.

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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.
>   	    (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