Re: [PATCH 1/2] media: venus: do not queue internal buffers from previous sequence

[Stanimir Varbanov <stanimir.varbanov@linaro.org>]

Hi Vikash,

Thanks for the patch!

On 4/12/22 10:09, Vikash Garodia wrote:
> During reconfig (DRC) event from firmware, it is not guaranteed that
> all the DPB(internal) buffers would be released by the firmware. Some
> buffers might be released gradually while processing frames from the
> new sequence. These buffers now stay idle in the dpblist.
> In subsequent call to queue the DPBs to firmware, these idle buffers
> should not be queued. The fix identifies those buffers and free them.
> ---
>  drivers/media/platform/qcom/venus/helpers.c | 37 ++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 0bca95d..0602f03 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -90,12 +90,31 @@ bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt)
>  }
>  EXPORT_SYMBOL_GPL(venus_helper_check_codec);
>  
> +static int venus_helper_free_dpb_buf(struct venus_inst *inst, struct intbuf *buf)

This function should return void. Also, venus_helper_ prefix is not
needed since if you follow the helpers.c style the prefix is only used
for exported helper functions.

I guess it could be:

static void free_dpb_buf(struct venus_inst *inst, struct intbuf *buf)

With this fixed:

Acked-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

> +{
> +	ida_free(&inst->dpb_ids, buf->dpb_out_tag);
> +
> +	list_del_init(&buf->list);
> +	dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da,
> +		       buf->attrs);
> +	kfree(buf);
> +
> +	return 0;
> +}
> +
>  int venus_helper_queue_dpb_bufs(struct venus_inst *inst)
>  {
> -	struct intbuf *buf;
> +	struct intbuf *buf, *next;
> +	unsigned int dpb_size = 0;
>  	int ret = 0;
>  
> -	list_for_each_entry(buf, &inst->dpbbufs, list) {
> +	if (inst->dpb_buftype == HFI_BUFFER_OUTPUT)
> +		dpb_size = inst->output_buf_size;
> +	else if (inst->dpb_buftype == HFI_BUFFER_OUTPUT2)
> +		dpb_size = inst->output2_buf_size;
> +
> +
> +	list_for_each_entry_safe(buf, next, &inst->dpbbufs, list) {
>  		struct hfi_frame_data fdata;
>  
>  		memset(&fdata, 0, sizeof(fdata));
> @@ -106,6 +125,12 @@ int venus_helper_queue_dpb_bufs(struct venus_inst *inst)
>  		if (buf->owned_by == FIRMWARE)
>  			continue;
>  
> +		/* free buffer from previous sequence which was released later */
> +		if (dpb_size > buf->size) {
> +			venus_helper_free_dpb_buf(inst, buf);
> +			continue;
> +		}
> +
>  		fdata.clnt_data = buf->dpb_out_tag;
>  
>  		ret = hfi_session_process_buf(inst, &fdata);
> @@ -127,13 +152,7 @@ int venus_helper_free_dpb_bufs(struct venus_inst *inst)
>  	list_for_each_entry_safe(buf, n, &inst->dpbbufs, list) {
>  		if (buf->owned_by == FIRMWARE)
>  			continue;
> -
> -		ida_free(&inst->dpb_ids, buf->dpb_out_tag);
> -
> -		list_del_init(&buf->list);
> -		dma_free_attrs(inst->core->dev, buf->size, buf->va, buf->da,
> -			       buf->attrs);
> -		kfree(buf);
> +		venus_helper_free_dpb_buf(inst, buf);
>  	}
>  
>  	if (list_empty(&inst->dpbbufs))

-- 
regards,
Stan