Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <329f5120-42c5-e5df-a138-c8b634b6a550@linaro.org>
Date:   Tue, 12 Apr 2022 13:14:27 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH 1/2] media: venus: do not queue internal buffers from
 previous sequence
Content-Language: en-US
To:     Vikash Garodia <quic_vgarodia@quicinc.com>,
        linux-media@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org
References: <1649747356-3207-1-git-send-email-quic_vgarodia@quicinc.com>
 <1649747356-3207-2-git-send-email-quic_vgarodia@quicinc.com>
From:   Stanimir Varbanov <stanimir.varbanov@linaro.org>
In-Reply-To: <1649747356-3207-2-git-send-email-quic_vgarodia@quicinc.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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