Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <1649769311-22622-1-git-send-email-quic_vgarodia@quicinc.com> <1649769311-22622-2-git-send-email-quic_vgarodia@quicinc.com>
In-Reply-To: <1649769311-22622-2-git-send-email-quic_vgarodia@quicinc.com>
From:   Fritz Koenig <frkoenig@chromium.org>
Date:   Wed, 13 Apr 2022 10:57:14 -0700
Message-ID: <CAMfZQbzm-YCK-Skz0BcOmq4rDX1HjeEapECNNaUNkj7pjm5uCw@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] media: venus: do not queue internal buffers from
 previous sequence
To:     Vikash Garodia <quic_vgarodia@quicinc.com>
Cc:     Linux Media Mailing List <linux-media@vger.kernel.org>,
        Stanimir Varbanov <stanimir.varbanov@linaro.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Tested-by: Fritz Koenig <frkoenig@chromium.org>


On Tue, Apr 12, 2022 at 6:23 AM Vikash Garodia
<quic_vgarodia@quicinc.com> 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.
>
> Signed-off-by: Vikash Garodia <quic_vgarodia@quicinc.com>
> Acked-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> ---
>  drivers/media/platform/qcom/venus/helpers.c | 34 +++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 0bca95d..fa01edd 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -90,12 +90,28 @@ bool venus_helper_check_codec(struct venus_inst *inst, u32 v4l2_pixfmt)
>  }
>  EXPORT_SYMBOL_GPL(venus_helper_check_codec);
>
> +static void free_dpb_buf(struct venus_inst *inst, struct intbuf *buf)
> +{
> +       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);
> +}
> +
>  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 +122,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) {
> +                       free_dpb_buf(inst, buf);
> +                       continue;
> +               }
> +
>                 fdata.clnt_data = buf->dpb_out_tag;
>
>                 ret = hfi_session_process_buf(inst, &fdata);
> @@ -127,13 +149,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);
> +               free_dpb_buf(inst, buf);
>         }
>
>         if (list_empty(&inst->dpbbufs))
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>