Re: [PATCH v2 17/23] media: iris: Track flush responses to prevent premature completion

[Vikash Garodia <quic_vgarodia@quicinc.com>]

On 4/28/2025 2:59 PM, Dikshita Agarwal wrote:
> Currently, two types of flush commands are queued to the firmware,
> the first flush queued as part of sequence change, does not wait for a
> response, while the second flush queued as part of stop, expects a
> completion response before proceeding further.
> 
> Due to timing issue, the flush response corresponding to the first
> command could arrive after the second flush is issued. This casuses the
> driver to incorrectly assume that the second flush has completed,
> leading to the premature signaling of flush_completion.
> 
> To address this, introduce a counter to track the number of pending
> flush responses and signal flush completion only when all expected
> responses are received.
> 
> Cc: stable@vger.kernel.org
> Fixes: 11712ce70f8e ("media: iris: implement vb2 streaming ops")
> Signed-off-by: Dikshita Agarwal <quic_dikshita@quicinc.com>
> ---
>  .../media/platform/qcom/iris/iris_hfi_gen1_command.c    |  4 +++-
>  .../media/platform/qcom/iris/iris_hfi_gen1_response.c   | 17 +++++++++++------
>  drivers/media/platform/qcom/iris/iris_instance.h        |  2 ++
>  3 files changed, 16 insertions(+), 7 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 f9f3e2d2ce29..ef3ca676d2ea 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_command.c
> @@ -208,8 +208,10 @@ static int iris_hfi_gen1_session_stop(struct iris_inst *inst, u32 plane)
>  		flush_pkt.flush_type = flush_type;
>  
>  		ret = iris_hfi_queue_cmd_write(core, &flush_pkt, flush_pkt.shdr.hdr.size);
> -		if (!ret)
> +		if (!ret) {
> +			inst->flush_responses_pending++;
>  			ret = iris_wait_for_session_response(inst, true);
> +		}
>  	}
>  
>  	return ret;
> diff --git a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> index dfca45d85759..01338baf3788 100644
> --- a/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> +++ b/drivers/media/platform/qcom/iris/iris_hfi_gen1_response.c
> @@ -207,7 +207,8 @@ static void iris_hfi_gen1_event_seq_changed(struct iris_inst *inst,
>  		flush_pkt.shdr.hdr.pkt_type = HFI_CMD_SESSION_FLUSH;
>  		flush_pkt.shdr.session_id = inst->session_id;
>  		flush_pkt.flush_type = HFI_FLUSH_OUTPUT;
> -		iris_hfi_queue_cmd_write(inst->core, &flush_pkt, flush_pkt.shdr.hdr.size);
> +		if (!iris_hfi_queue_cmd_write(inst->core, &flush_pkt, flush_pkt.shdr.hdr.size))
> +			inst->flush_responses_pending++;
>  	}
>  
>  	iris_vdec_src_change(inst);
> @@ -408,7 +409,9 @@ static void iris_hfi_gen1_session_ftb_done(struct iris_inst *inst, void *packet)
>  		flush_pkt.shdr.hdr.pkt_type = HFI_CMD_SESSION_FLUSH;
>  		flush_pkt.shdr.session_id = inst->session_id;
>  		flush_pkt.flush_type = HFI_FLUSH_OUTPUT;
> -		iris_hfi_queue_cmd_write(core, &flush_pkt, flush_pkt.shdr.hdr.size);
> +		if (!iris_hfi_queue_cmd_write(core, &flush_pkt, flush_pkt.shdr.hdr.size))
> +			inst->flush_responses_pending++;
> +
>  		iris_inst_sub_state_change_drain_last(inst);
>  
>  		return;
> @@ -570,7 +573,6 @@ static void iris_hfi_gen1_handle_response(struct iris_core *core, void *response
>  	const struct iris_hfi_gen1_response_pkt_info *pkt_info;
>  	struct device *dev = core->dev;
>  	struct hfi_session_pkt *pkt;
> -	struct completion *done;
>  	struct iris_inst *inst;
>  	bool found = false;
>  	u32 i;
> @@ -631,9 +633,12 @@ static void iris_hfi_gen1_handle_response(struct iris_core *core, void *response
>  			if (shdr->error_type != HFI_ERR_NONE)
>  				iris_inst_change_state(inst, IRIS_INST_ERROR);
>  
> -			done = pkt_info->pkt == HFI_MSG_SESSION_FLUSH ?
> -				&inst->flush_completion : &inst->completion;
> -			complete(done);
> +			if (pkt_info->pkt == HFI_MSG_SESSION_FLUSH) {
> +				if (--inst->flush_responses_pending <= 0)
No need to check for < 0 condition as its an unsigned int. Signal when equals 0.

With above change, mark
Reviewed-by: Vikash Garodia <quic_vgarodia@quicinc.com>

> +					complete(&inst->flush_completion);
> +			} else {
> +				complete(&inst->completion);
> +			}
>  		}
>  		mutex_unlock(&inst->lock);
>  
> diff --git a/drivers/media/platform/qcom/iris/iris_instance.h b/drivers/media/platform/qcom/iris/iris_instance.h
> index 5150237f0020..9ed197799ee7 100644
> --- a/drivers/media/platform/qcom/iris/iris_instance.h
> +++ b/drivers/media/platform/qcom/iris/iris_instance.h
> @@ -27,6 +27,7 @@
>   * @crop: structure of crop info
>   * @completion: structure of signal completions
>   * @flush_completion: structure of signal completions for flush cmd
> + * @flush_responses_pending: counter to track number of pending flush responses
>   * @fw_caps: array of supported instance firmware capabilities
>   * @buffers: array of different iris buffers
>   * @fw_min_count: minimnum count of buffers needed by fw
> @@ -59,6 +60,7 @@ struct iris_inst {
>  	struct iris_hfi_rect_desc	crop;
>  	struct completion		completion;
>  	struct completion		flush_completion;
> +	u32				flush_responses_pending;
>  	struct platform_inst_fw_cap	fw_caps[INST_FW_CAP_MAX];
>  	struct iris_buffers		buffers[BUF_TYPE_MAX];
>  	u32				fw_min_count;
>