Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 09/10] venus: vdec: allow bigger sizeimage set by clients
To:     Alexandre Courbot <acourbot@chromium.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>
References: <20190117162008.25217-1-stanimir.varbanov@linaro.org>
 <20190117162008.25217-10-stanimir.varbanov@linaro.org>
 <CAPBb6MUumC2BmWar3yUmVT8vz8x-Nr_tuMc=1VSJvmQYGdPudw@mail.gmail.com>
From:   Stanimir Varbanov <stanimir.varbanov@linaro.org>
Message-ID: <580839ce-c98b-8b88-8868-e5df8171923b@linaro.org>
Date:   Thu, 24 Jan 2019 12:05:08 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: <CAPBb6MUumC2BmWar3yUmVT8vz8x-Nr_tuMc=1VSJvmQYGdPudw@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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);

> 
>>                 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);

> 
> 
> 
>>                 inst->input_buf_size = sizes[0];
>>                 *num_buffers = max(*num_buffers, in_num);
>>                 inst->num_input_bufs = *num_buffers;
>> --
>> 2.17.1
>>

-- 
regards,
Stan