Re: [PATCH 09/10] venus: vdec: allow bigger sizeimage set by clients

From: Alexandre Courbot <acourbot@chromium.org>
To: Stanimir Varbanov <stanimir.varbanov@linaro.org>
Cc: Linux Media Mailing List <linux-media@vger.kernel.org>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Hans Verkuil <hverkuil@xs4all.nl>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-arm-msm@vger.kernel.org,
	Vikash Garodia <vgarodia@codeaurora.org>,
	Tomasz Figa <tfiga@chromium.org>,
	Malathi Gottam <mgottam@codeaurora.org>
Subject: Re: [PATCH 09/10] venus: vdec: allow bigger sizeimage set by clients
Date: Fri, 25 Jan 2019 14:36:43 +0900	[thread overview]
Message-ID: <CAPBb6MXLJnoT5=WkJQb7fmT8wznnth65_Gia04QuG_1-hKAmbg@mail.gmail.com> (raw)
In-Reply-To: <580839ce-c98b-8b88-8868-e5df8171923b@linaro.org>

On Thu, Jan 24, 2019 at 7:05 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi Alex,
>
> Thanks for the comments!
>
> On 1/24/19 10:43 AM, Alexandre Courbot wrote:
> > On Fri, Jan 18, 2019 at 1:21 AM Stanimir Varbanov
> > <stanimir.varbanov@linaro.org> wrote:
> >>
> >> In most of the cases the client will know better what could be
> >> the maximum size for compressed data buffers. Change the driver
> >> to permit the user to set bigger size for the compressed buffer
> >> but make reasonable sanitation.
> >>
> >> Signed-off-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>
> >> ---
> >>  drivers/media/platform/qcom/venus/vdec.c | 18 +++++++++++++-----
> >>  1 file changed, 13 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/qcom/venus/vdec.c b/drivers/media/platform/qcom/venus/vdec.c
> >> index 282de21cf2e1..7a9370df7515 100644
> >> --- a/drivers/media/platform/qcom/venus/vdec.c
> >> +++ b/drivers/media/platform/qcom/venus/vdec.c
> >> @@ -142,6 +142,7 @@ vdec_try_fmt_common(struct venus_inst *inst, struct v4l2_format *f)
> >>         struct v4l2_pix_format_mplane *pixmp = &f->fmt.pix_mp;
> >>         struct v4l2_plane_pix_format *pfmt = pixmp->plane_fmt;
> >>         const struct venus_format *fmt;
> >> +       u32 szimage;
> >>
> >>         memset(pfmt[0].reserved, 0, sizeof(pfmt[0].reserved));
> >>         memset(pixmp->reserved, 0, sizeof(pixmp->reserved));
> >> @@ -170,14 +171,18 @@ vdec_try_fmt_common(struct venus_inst *inst, struct v4l2_format *f)
> >>         pixmp->num_planes = fmt->num_planes;
> >>         pixmp->flags = 0;
> >>
> >> -       pfmt[0].sizeimage = venus_helper_get_framesz(pixmp->pixelformat,
> >> -                                                    pixmp->width,
> >> -                                                    pixmp->height);
> >> +       szimage = venus_helper_get_framesz(pixmp->pixelformat, pixmp->width,
> >> +                                          pixmp->height);
> >>
> >> -       if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> >> +       if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
> >> +               pfmt[0].sizeimage = szimage;
> >>                 pfmt[0].bytesperline = ALIGN(pixmp->width, 128);
> >> -       else
> >> +       } else {
> >> +               pfmt[0].sizeimage = clamp_t(u32, pfmt[0].sizeimage, 0, SZ_4M);
> >> +               if (szimage > pfmt[0].sizeimage)
> >> +                       pfmt[0].sizeimage = szimage;
> >
> > pfmt[0].sizeimage = max(clamp_t(u32, pfmt[0].sizeimage, 0, SZ_4M),
> >                                         szimage)?
>
> I'm not a big fan to that calling of macro from macro :)
>
> What about this:
>
>         pfmt[0].sizeimage = clamp_t(u32, pfmt[0].sizeimage, 0, SZ_4M);
>         pfmt[0].sizeimage = max(pfmt[0].sizeimage, szimage);

Looks fine, I just wanted to make sure that we use the more expressive
"max" instead of an if statement.

>
> >
> >>                 pfmt[0].bytesperline = 0;
> >> +       }
> >>
> >>         return fmt;
> >>  }
> >> @@ -275,6 +280,7 @@ static int vdec_s_fmt(struct file *file, void *fh, struct v4l2_format *f)
> >>                 inst->ycbcr_enc = pixmp->ycbcr_enc;
> >>                 inst->quantization = pixmp->quantization;
> >>                 inst->xfer_func = pixmp->xfer_func;
> >> +               inst->input_buf_size = pixmp->plane_fmt[0].sizeimage;
> >>         }
> >>
> >>         memset(&format, 0, sizeof(format));
> >> @@ -737,6 +743,8 @@ static int vdec_queue_setup(struct vb2_queue *q,
> >>                 sizes[0] = venus_helper_get_framesz(inst->fmt_out->pixfmt,
> >>                                                     inst->out_width,
> >>                                                     inst->out_height);
> >> +               if (inst->input_buf_size > sizes[0])
> >> +                       sizes[0] = inst->input_buf_size;
> >
> >                sizes[0] = max(venus_helper_get_framesz(inst->fmt_out->pixfmt,
> >                                                    inst->out_width,
> >                                                  inst->out_height),
> >                                       inst->input_buf_size)?
>
> I think it'd be more readable that way:
>
>                 sizes[0] = max(sizes[0], inst->input_buf_size);

Ditto.