On Thu, Jan 24, 2019 at 7:05 PM Stanimir Varbanov
<stanimir.varbanov@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >> 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.
