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@xxxxxxxxxx>
> +{
> + 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
