linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] media: venus: amend buffer size for bitstream plane
@ 2018-10-09  7:52 Malathi Gottam
  2018-10-22  6:12 ` Alexandre Courbot
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Malathi Gottam @ 2018-10-09  7:52 UTC (permalink / raw)
  To: stanimir.varbanov, hverkuil, mchehab
  Cc: linux-media, linux-kernel, linux-arm-msm, acourbot, vgarodia, mgottam

For lower resolutions, incase of encoder, the compressed
frame size is more than half of the corresponding input
YUV. Keep the size as same as YUV considering worst case.

Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
---
 drivers/media/platform/qcom/venus/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
index 2679adb..05c5423 100644
--- a/drivers/media/platform/qcom/venus/helpers.c
+++ b/drivers/media/platform/qcom/venus/helpers.c
@@ -649,7 +649,7 @@ u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 width, u32 height)
 	}
 
 	if (compressed) {
-		sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2 / 2;
+		sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2;
 		return ALIGN(sz, SZ_4K);
 	}
 
-- 
1.9.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: venus: amend buffer size for bitstream plane
  2018-10-09  7:52 [PATCH] media: venus: amend buffer size for bitstream plane Malathi Gottam
@ 2018-10-22  6:12 ` Alexandre Courbot
  2018-10-23  2:50 ` Tomasz Figa
  2018-11-12 12:28 ` Stanimir Varbanov
  2 siblings, 0 replies; 12+ messages in thread
From: Alexandre Courbot @ 2018-10-22  6:12 UTC (permalink / raw)
  To: mgottam
  Cc: Stanimir Varbanov, Hans Verkuil, Mauro Carvalho Chehab,
	Linux Media Mailing List, LKML, linux-arm-msm, vgarodia

On Tue, Oct 9, 2018 at 4:52 PM Malathi Gottam <mgottam@codeaurora.org> wrote:
>
> For lower resolutions, incase of encoder, the compressed
> frame size is more than half of the corresponding input
> YUV. Keep the size as same as YUV considering worst case.
>
> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>

This fixes some issues we had with low-resolution VP8 encoding. Maybe
this can be refined some more for higher resolutions, but the current
version at least works.

Tested-by: Alexandre Courbot <acourbot@chromium.org>

> ---
>  drivers/media/platform/qcom/venus/helpers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 2679adb..05c5423 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -649,7 +649,7 @@ u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 width, u32 height)
>         }
>
>         if (compressed) {
> -               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2 / 2;
> +               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2;
>                 return ALIGN(sz, SZ_4K);
>         }
>
> --
> 1.9.1
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: venus: amend buffer size for bitstream plane
  2018-10-09  7:52 [PATCH] media: venus: amend buffer size for bitstream plane Malathi Gottam
  2018-10-22  6:12 ` Alexandre Courbot
@ 2018-10-23  2:50 ` Tomasz Figa
  2018-11-12 12:34   ` Stanimir Varbanov
  2018-11-12 12:28 ` Stanimir Varbanov
  2 siblings, 1 reply; 12+ messages in thread
From: Tomasz Figa @ 2018-10-23  2:50 UTC (permalink / raw)
  To: mgottam
  Cc: Stanimir Varbanov, Hans Verkuil, Mauro Carvalho Chehab,
	Linux Media Mailing List, Linux Kernel Mailing List,
	linux-arm-msm, Alexandre Courbot, vgarodia

Hi Malathi,

On Tue, Oct 9, 2018 at 4:58 PM Malathi Gottam <mgottam@codeaurora.org> wrote:
>
> For lower resolutions, incase of encoder, the compressed
> frame size is more than half of the corresponding input
> YUV. Keep the size as same as YUV considering worst case.
>
> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/helpers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 2679adb..05c5423 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -649,7 +649,7 @@ u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 width, u32 height)
>         }
>
>         if (compressed) {
> -               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2 / 2;
> +               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2;
>                 return ALIGN(sz, SZ_4K);
>         }

Note that the driver should not enforce one particular buffer size for
bitstream buffers unless it's a workaround for broken firmware or
hardware. The userspace should be able to select the desired size.

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: venus: amend buffer size for bitstream plane
  2018-10-09  7:52 [PATCH] media: venus: amend buffer size for bitstream plane Malathi Gottam
  2018-10-22  6:12 ` Alexandre Courbot
  2018-10-23  2:50 ` Tomasz Figa
@ 2018-11-12 12:28 ` Stanimir Varbanov
  2 siblings, 0 replies; 12+ messages in thread
From: Stanimir Varbanov @ 2018-11-12 12:28 UTC (permalink / raw)
  To: Malathi Gottam, hverkuil, mchehab
  Cc: linux-media, linux-kernel, linux-arm-msm, acourbot, vgarodia

Hi Malathi,

Thanks for the patch!

On 10/9/18 10:52 AM, Malathi Gottam wrote:
> For lower resolutions, incase of encoder, the compressed
> frame size is more than half of the corresponding input
> YUV. Keep the size as same as YUV considering worst case.
> 
> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/helpers.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
> index 2679adb..05c5423 100644
> --- a/drivers/media/platform/qcom/venus/helpers.c
> +++ b/drivers/media/platform/qcom/venus/helpers.c
> @@ -649,7 +649,7 @@ u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 width, u32 height)
>  	}
>  
>  	if (compressed) {
> -		sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2 / 2;
> +		sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2;

Could you drop this division by 2 only for lower resolutions and also
only for the encoder session? I do not want to waste memory if it is not
absolutely needed.

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: venus: amend buffer size for bitstream plane
  2018-10-23  2:50 ` Tomasz Figa
@ 2018-11-12 12:34   ` Stanimir Varbanov
  2018-11-13  7:28     ` mgottam
  0 siblings, 1 reply; 12+ messages in thread
From: Stanimir Varbanov @ 2018-11-12 12:34 UTC (permalink / raw)
  To: Tomasz Figa, mgottam
  Cc: Hans Verkuil, Mauro Carvalho Chehab, Linux Media Mailing List,
	Linux Kernel Mailing List, linux-arm-msm, Alexandre Courbot,
	vgarodia

Hi Tomasz,

On 10/23/2018 05:50 AM, Tomasz Figa wrote:
> Hi Malathi,
> 
> On Tue, Oct 9, 2018 at 4:58 PM Malathi Gottam <mgottam@codeaurora.org> wrote:
>>
>> For lower resolutions, incase of encoder, the compressed
>> frame size is more than half of the corresponding input
>> YUV. Keep the size as same as YUV considering worst case.
>>
>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
>> ---
>>  drivers/media/platform/qcom/venus/helpers.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/qcom/venus/helpers.c b/drivers/media/platform/qcom/venus/helpers.c
>> index 2679adb..05c5423 100644
>> --- a/drivers/media/platform/qcom/venus/helpers.c
>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>> @@ -649,7 +649,7 @@ u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 width, u32 height)
>>         }
>>
>>         if (compressed) {
>> -               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2 / 2;
>> +               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2;
>>                 return ALIGN(sz, SZ_4K);
>>         }
> 
> Note that the driver should not enforce one particular buffer size for
> bitstream buffers unless it's a workaround for broken firmware or
> hardware. The userspace should be able to select the desired size.

Good point! Yes, we have to extend set_fmt to allow bigger sizeimage for
the compressed buffers (not only for encoder).

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: venus: amend buffer size for bitstream plane
  2018-11-12 12:34   ` Stanimir Varbanov
@ 2018-11-13  7:28     ` mgottam
  2018-11-13  8:12       ` Stanimir Varbanov
  0 siblings, 1 reply; 12+ messages in thread
From: mgottam @ 2018-11-13  7:28 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: Tomasz Figa, Hans Verkuil, Mauro Carvalho Chehab,
	Linux Media Mailing List, Linux Kernel Mailing List,
	linux-arm-msm, Alexandre Courbot, vgarodia

On 2018-11-12 18:04, Stanimir Varbanov wrote:
> Hi Tomasz,
> 
> On 10/23/2018 05:50 AM, Tomasz Figa wrote:
>> Hi Malathi,
>> 
>> On Tue, Oct 9, 2018 at 4:58 PM Malathi Gottam <mgottam@codeaurora.org> 
>> wrote:
>>> 
>>> For lower resolutions, incase of encoder, the compressed
>>> frame size is more than half of the corresponding input
>>> YUV. Keep the size as same as YUV considering worst case.
>>> 
>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
>>> ---
>>>  drivers/media/platform/qcom/venus/helpers.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/drivers/media/platform/qcom/venus/helpers.c 
>>> b/drivers/media/platform/qcom/venus/helpers.c
>>> index 2679adb..05c5423 100644
>>> --- a/drivers/media/platform/qcom/venus/helpers.c
>>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>>> @@ -649,7 +649,7 @@ u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 
>>> width, u32 height)
>>>         }
>>> 
>>>         if (compressed) {
>>> -               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2 / 
>>> 2;
>>> +               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2;
>>>                 return ALIGN(sz, SZ_4K);
>>>         }
>> 
>> Note that the driver should not enforce one particular buffer size for
>> bitstream buffers unless it's a workaround for broken firmware or
>> hardware. The userspace should be able to select the desired size.
> 
> Good point! Yes, we have to extend set_fmt to allow bigger sizeimage 
> for
> the compressed buffers (not only for encoder).

So Stan you meant to say that we should allow s_fmt to accept client 
specified size?
If so should we set the inst->input_buf_size here in venc_s_fmt?

@@ -333,10 +333,10 @@static const struct venus_format *
venc_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);
+       if (!pfmt[0].sizeimage)
+               pfmt[0].sizeimage = 
venus_helper_get_framesz(pixmp->pixelformat,
+                                                            
pixmp->width,
+                                                            
pixmp->height);

         if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
                 pfmt[0].bytesperline = ALIGN(pixmp->width, 128);
@@ -387,6 +387,7 @@ static int venc_s_fmt(struct file *file, void *fh, 
struct v4l2_format *f)
         venc_try_fmt_common(inst, &format);

         if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
+               inst->input_buf_size = pixmp->plane_fmt[0].sizeimage;
                 inst->out_width = format.fmt.pix_mp.width;
                 inst->out_height = format.fmt.pix_mp.height;

Similar implementation is already handled in case of decoder.

Then in queue setup, we can compare this against calculated size to 
obtain final buffer size

@@ -899,7 +900,8 @@ static int venc_queue_setup(struct vb2_queue *q,
                 sizes[0] = 
venus_helper_get_framesz(inst->fmt_out->pixfmt,
                                                     inst->width,
                                                     inst->height);
-               inst->input_buf_size = sizes[0];
+               if(inst->input_buf_size < sizes[0])
+                       inst->input_buf_size = sizes[0];
                 break;

I hope this meets are requirements.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: venus: amend buffer size for bitstream plane
  2018-11-13  7:28     ` mgottam
@ 2018-11-13  8:12       ` Stanimir Varbanov
  2018-11-13  9:13         ` Tomasz Figa
  0 siblings, 1 reply; 12+ messages in thread
From: Stanimir Varbanov @ 2018-11-13  8:12 UTC (permalink / raw)
  To: mgottam
  Cc: Tomasz Figa, Hans Verkuil, Mauro Carvalho Chehab,
	Linux Media Mailing List, Linux Kernel Mailing List,
	linux-arm-msm, Alexandre Courbot, vgarodia

Hi Malathi,

On 11/13/18 9:28 AM, mgottam@codeaurora.org wrote:
> On 2018-11-12 18:04, Stanimir Varbanov wrote:
>> Hi Tomasz,
>>
>> On 10/23/2018 05:50 AM, Tomasz Figa wrote:
>>> Hi Malathi,
>>>
>>> On Tue, Oct 9, 2018 at 4:58 PM Malathi Gottam
>>> <mgottam@codeaurora.org> wrote:
>>>>
>>>> For lower resolutions, incase of encoder, the compressed
>>>> frame size is more than half of the corresponding input
>>>> YUV. Keep the size as same as YUV considering worst case.
>>>>
>>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
>>>> ---
>>>>  drivers/media/platform/qcom/venus/helpers.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/platform/qcom/venus/helpers.c
>>>> b/drivers/media/platform/qcom/venus/helpers.c
>>>> index 2679adb..05c5423 100644
>>>> --- a/drivers/media/platform/qcom/venus/helpers.c
>>>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>>>> @@ -649,7 +649,7 @@ u32 venus_helper_get_framesz(u32 v4l2_fmt, u32
>>>> width, u32 height)
>>>>         }
>>>>
>>>>         if (compressed) {
>>>> -               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2 / 2;
>>>> +               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2;
>>>>                 return ALIGN(sz, SZ_4K);
>>>>         }
>>>
>>> Note that the driver should not enforce one particular buffer size for
>>> bitstream buffers unless it's a workaround for broken firmware or
>>> hardware. The userspace should be able to select the desired size.
>>
>> Good point! Yes, we have to extend set_fmt to allow bigger sizeimage for
>> the compressed buffers (not only for encoder).
> 
> So Stan you meant to say that we should allow s_fmt to accept client
> specified size?

yes but I do expect:

new_sizeimage = max(user_sizeimage, venus_helper_get_framesz)

and also user_sizeimage should be sanitized.

> If so should we set the inst->input_buf_size here in venc_s_fmt?
> 
> @@ -333,10 +333,10 @@static const struct venus_format *
> venc_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);
> +       if (!pfmt[0].sizeimage)
> +               pfmt[0].sizeimage =
> venus_helper_get_framesz(pixmp->pixelformat,
> +                                                            pixmp->width,
> +                                                           
> pixmp->height);

yes, but please make

pfmt[0].sizeimage = max(pfmt[0].sizeimage, venus_helper_get_framesz)

and IMO this should be only for CAPTURE queue i.e. inst->output_buf_size

I'm still not sure do we need it for OUTPUT encoder queue.

> 
>         if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>                 pfmt[0].bytesperline = ALIGN(pixmp->width, 128);
> @@ -387,6 +387,7 @@ static int venc_s_fmt(struct file *file, void *fh,
> struct v4l2_format *f)
>         venc_try_fmt_common(inst, &format);
> 
>         if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> +               inst->input_buf_size = pixmp->plane_fmt[0].sizeimage;
>                 inst->out_width = format.fmt.pix_mp.width;
>                 inst->out_height = format.fmt.pix_mp.height;
> 
> Similar implementation is already handled in case of decoder.
> 
> Then in queue setup, we can compare this against calculated size to
> obtain final buffer size
> 
> @@ -899,7 +900,8 @@ static int venc_queue_setup(struct vb2_queue *q,
>                 sizes[0] = venus_helper_get_framesz(inst->fmt_out->pixfmt,
>                                                     inst->width,
>                                                     inst->height);
> -               inst->input_buf_size = sizes[0];
> +               if(inst->input_buf_size < sizes[0])
> +                       inst->input_buf_size = sizes[0];
>                 break;
> 
> I hope this meets are requirements.

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: venus: amend buffer size for bitstream plane
  2018-11-13  8:12       ` Stanimir Varbanov
@ 2018-11-13  9:13         ` Tomasz Figa
  2018-11-13 10:46           ` Stanimir Varbanov
  0 siblings, 1 reply; 12+ messages in thread
From: Tomasz Figa @ 2018-11-13  9:13 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: mgottam, Hans Verkuil, Mauro Carvalho Chehab,
	Linux Media Mailing List, Linux Kernel Mailing List,
	linux-arm-msm, Alexandre Courbot, vgarodia

On Tue, Nov 13, 2018 at 5:12 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi Malathi,
>
> On 11/13/18 9:28 AM, mgottam@codeaurora.org wrote:
> > On 2018-11-12 18:04, Stanimir Varbanov wrote:
> >> Hi Tomasz,
> >>
> >> On 10/23/2018 05:50 AM, Tomasz Figa wrote:
> >>> Hi Malathi,
> >>>
> >>> On Tue, Oct 9, 2018 at 4:58 PM Malathi Gottam
> >>> <mgottam@codeaurora.org> wrote:
> >>>>
> >>>> For lower resolutions, incase of encoder, the compressed
> >>>> frame size is more than half of the corresponding input
> >>>> YUV. Keep the size as same as YUV considering worst case.
> >>>>
> >>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> >>>> ---
> >>>>  drivers/media/platform/qcom/venus/helpers.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/media/platform/qcom/venus/helpers.c
> >>>> b/drivers/media/platform/qcom/venus/helpers.c
> >>>> index 2679adb..05c5423 100644
> >>>> --- a/drivers/media/platform/qcom/venus/helpers.c
> >>>> +++ b/drivers/media/platform/qcom/venus/helpers.c
> >>>> @@ -649,7 +649,7 @@ u32 venus_helper_get_framesz(u32 v4l2_fmt, u32
> >>>> width, u32 height)
> >>>>         }
> >>>>
> >>>>         if (compressed) {
> >>>> -               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2 / 2;
> >>>> +               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2;
> >>>>                 return ALIGN(sz, SZ_4K);
> >>>>         }
> >>>
> >>> Note that the driver should not enforce one particular buffer size for
> >>> bitstream buffers unless it's a workaround for broken firmware or
> >>> hardware. The userspace should be able to select the desired size.
> >>
> >> Good point! Yes, we have to extend set_fmt to allow bigger sizeimage for
> >> the compressed buffers (not only for encoder).
> >
> > So Stan you meant to say that we should allow s_fmt to accept client
> > specified size?
>
> yes but I do expect:
>
> new_sizeimage = max(user_sizeimage, venus_helper_get_framesz)
>
> and also user_sizeimage should be sanitized.
>
> > If so should we set the inst->input_buf_size here in venc_s_fmt?
> >
> > @@ -333,10 +333,10 @@static const struct venus_format *
> > venc_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);
> > +       if (!pfmt[0].sizeimage)
> > +               pfmt[0].sizeimage =
> > venus_helper_get_framesz(pixmp->pixelformat,
> > +                                                            pixmp->width,
> > +
> > pixmp->height);
>
> yes, but please make
>
> pfmt[0].sizeimage = max(pfmt[0].sizeimage, venus_helper_get_framesz)
>
> and IMO this should be only for CAPTURE queue i.e. inst->output_buf_size
>
> I'm still not sure do we need it for OUTPUT encoder queue.
>

This would be indeed only for the queues that operate on a coded
bitstream, i.e. both encoder CAPTURE and decoder OUTPUT.

For image formats, sizeimage should be calculated by the driver based
on the bytesperline and height. (Bytesperline may be fixed, if the
hardware doesn't support flexible strides, but if it does, it's
strongly recommended to use the bytesperline coming from the
application as the stride +/- any necessary sanity checks.)

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: venus: amend buffer size for bitstream plane
  2018-11-13  9:13         ` Tomasz Figa
@ 2018-11-13 10:46           ` Stanimir Varbanov
  2018-11-14  3:51             ` Tomasz Figa
  0 siblings, 1 reply; 12+ messages in thread
From: Stanimir Varbanov @ 2018-11-13 10:46 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: mgottam, Hans Verkuil, Mauro Carvalho Chehab,
	Linux Media Mailing List, Linux Kernel Mailing List,
	linux-arm-msm, Alexandre Courbot, vgarodia

Hi Tomasz,

On 11/13/18 11:13 AM, Tomasz Figa wrote:
> On Tue, Nov 13, 2018 at 5:12 PM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>>
>> Hi Malathi,
>>
>> On 11/13/18 9:28 AM, mgottam@codeaurora.org wrote:
>>> On 2018-11-12 18:04, Stanimir Varbanov wrote:
>>>> Hi Tomasz,
>>>>
>>>> On 10/23/2018 05:50 AM, Tomasz Figa wrote:
>>>>> Hi Malathi,
>>>>>
>>>>> On Tue, Oct 9, 2018 at 4:58 PM Malathi Gottam
>>>>> <mgottam@codeaurora.org> wrote:
>>>>>>
>>>>>> For lower resolutions, incase of encoder, the compressed
>>>>>> frame size is more than half of the corresponding input
>>>>>> YUV. Keep the size as same as YUV considering worst case.
>>>>>>
>>>>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
>>>>>> ---
>>>>>>  drivers/media/platform/qcom/venus/helpers.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/media/platform/qcom/venus/helpers.c
>>>>>> b/drivers/media/platform/qcom/venus/helpers.c
>>>>>> index 2679adb..05c5423 100644
>>>>>> --- a/drivers/media/platform/qcom/venus/helpers.c
>>>>>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>>>>>> @@ -649,7 +649,7 @@ u32 venus_helper_get_framesz(u32 v4l2_fmt, u32
>>>>>> width, u32 height)
>>>>>>         }
>>>>>>
>>>>>>         if (compressed) {
>>>>>> -               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2 / 2;
>>>>>> +               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2;
>>>>>>                 return ALIGN(sz, SZ_4K);
>>>>>>         }
>>>>>
>>>>> Note that the driver should not enforce one particular buffer size for
>>>>> bitstream buffers unless it's a workaround for broken firmware or
>>>>> hardware. The userspace should be able to select the desired size.
>>>>
>>>> Good point! Yes, we have to extend set_fmt to allow bigger sizeimage for
>>>> the compressed buffers (not only for encoder).
>>>
>>> So Stan you meant to say that we should allow s_fmt to accept client
>>> specified size?
>>
>> yes but I do expect:
>>
>> new_sizeimage = max(user_sizeimage, venus_helper_get_framesz)
>>
>> and also user_sizeimage should be sanitized.
>>
>>> If so should we set the inst->input_buf_size here in venc_s_fmt?
>>>
>>> @@ -333,10 +333,10 @@static const struct venus_format *
>>> venc_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);
>>> +       if (!pfmt[0].sizeimage)
>>> +               pfmt[0].sizeimage =
>>> venus_helper_get_framesz(pixmp->pixelformat,
>>> +                                                            pixmp->width,
>>> +
>>> pixmp->height);
>>
>> yes, but please make
>>
>> pfmt[0].sizeimage = max(pfmt[0].sizeimage, venus_helper_get_framesz)
>>
>> and IMO this should be only for CAPTURE queue i.e. inst->output_buf_size
>>
>> I'm still not sure do we need it for OUTPUT encoder queue.
>>
> 
> This would be indeed only for the queues that operate on a coded
> bitstream, i.e. both encoder CAPTURE and decoder OUTPUT.

Thanks for the confirmation.

> 
> For image formats, sizeimage should be calculated by the driver based
> on the bytesperline and height. (Bytesperline may be fixed, if the
> hardware doesn't support flexible strides, but if it does, it's
> strongly recommended to use the bytesperline coming from the
> application as the stride +/- any necessary sanity checks.)

the hw should support stride but I'm not sure is that exposed by the
firmware interface.

-- 
regards,
Stan

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: venus: amend buffer size for bitstream plane
  2018-11-13 10:46           ` Stanimir Varbanov
@ 2018-11-14  3:51             ` Tomasz Figa
  2018-11-16  4:34               ` mgottam
  0 siblings, 1 reply; 12+ messages in thread
From: Tomasz Figa @ 2018-11-14  3:51 UTC (permalink / raw)
  To: Stanimir Varbanov
  Cc: mgottam, Hans Verkuil, Mauro Carvalho Chehab,
	Linux Media Mailing List, Linux Kernel Mailing List,
	linux-arm-msm, Alexandre Courbot, vgarodia

On Tue, Nov 13, 2018 at 7:46 PM Stanimir Varbanov
<stanimir.varbanov@linaro.org> wrote:
>
> Hi Tomasz,
>
> On 11/13/18 11:13 AM, Tomasz Figa wrote:
> > On Tue, Nov 13, 2018 at 5:12 PM Stanimir Varbanov
> > <stanimir.varbanov@linaro.org> wrote:
> >>
> >> Hi Malathi,
> >>
> >> On 11/13/18 9:28 AM, mgottam@codeaurora.org wrote:
> >>> On 2018-11-12 18:04, Stanimir Varbanov wrote:
> >>>> Hi Tomasz,
> >>>>
> >>>> On 10/23/2018 05:50 AM, Tomasz Figa wrote:
> >>>>> Hi Malathi,
> >>>>>
> >>>>> On Tue, Oct 9, 2018 at 4:58 PM Malathi Gottam
> >>>>> <mgottam@codeaurora.org> wrote:
> >>>>>>
> >>>>>> For lower resolutions, incase of encoder, the compressed
> >>>>>> frame size is more than half of the corresponding input
> >>>>>> YUV. Keep the size as same as YUV considering worst case.
> >>>>>>
> >>>>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> >>>>>> ---
> >>>>>>  drivers/media/platform/qcom/venus/helpers.c | 2 +-
> >>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/media/platform/qcom/venus/helpers.c
> >>>>>> b/drivers/media/platform/qcom/venus/helpers.c
> >>>>>> index 2679adb..05c5423 100644
> >>>>>> --- a/drivers/media/platform/qcom/venus/helpers.c
> >>>>>> +++ b/drivers/media/platform/qcom/venus/helpers.c
> >>>>>> @@ -649,7 +649,7 @@ u32 venus_helper_get_framesz(u32 v4l2_fmt, u32
> >>>>>> width, u32 height)
> >>>>>>         }
> >>>>>>
> >>>>>>         if (compressed) {
> >>>>>> -               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2 / 2;
> >>>>>> +               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2;
> >>>>>>                 return ALIGN(sz, SZ_4K);
> >>>>>>         }
> >>>>>
> >>>>> Note that the driver should not enforce one particular buffer size for
> >>>>> bitstream buffers unless it's a workaround for broken firmware or
> >>>>> hardware. The userspace should be able to select the desired size.
> >>>>
> >>>> Good point! Yes, we have to extend set_fmt to allow bigger sizeimage for
> >>>> the compressed buffers (not only for encoder).
> >>>
> >>> So Stan you meant to say that we should allow s_fmt to accept client
> >>> specified size?
> >>
> >> yes but I do expect:
> >>
> >> new_sizeimage = max(user_sizeimage, venus_helper_get_framesz)
> >>
> >> and also user_sizeimage should be sanitized.
> >>
> >>> If so should we set the inst->input_buf_size here in venc_s_fmt?
> >>>
> >>> @@ -333,10 +333,10 @@static const struct venus_format *
> >>> venc_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);
> >>> +       if (!pfmt[0].sizeimage)
> >>> +               pfmt[0].sizeimage =
> >>> venus_helper_get_framesz(pixmp->pixelformat,
> >>> +                                                            pixmp->width,
> >>> +
> >>> pixmp->height);
> >>
> >> yes, but please make
> >>
> >> pfmt[0].sizeimage = max(pfmt[0].sizeimage, venus_helper_get_framesz)
> >>
> >> and IMO this should be only for CAPTURE queue i.e. inst->output_buf_size
> >>
> >> I'm still not sure do we need it for OUTPUT encoder queue.
> >>
> >
> > This would be indeed only for the queues that operate on a coded
> > bitstream, i.e. both encoder CAPTURE and decoder OUTPUT.
>
> Thanks for the confirmation.
>
> >
> > For image formats, sizeimage should be calculated by the driver based
> > on the bytesperline and height. (Bytesperline may be fixed, if the
> > hardware doesn't support flexible strides, but if it does, it's
> > strongly recommended to use the bytesperline coming from the
> > application as the stride +/- any necessary sanity checks.)
>
> the hw should support stride but I'm not sure is that exposed by the
> firmware interface.

After thinking a bit more on this, there is actually some redundancy
between format width and crop width, since one should be normally able
to just set the format width to the buffer stride and crop to the
buffer width and have arbitrary strides supported (+/- hw alignment
requirements, but that's something that has to always be accounted
for).

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: venus: amend buffer size for bitstream plane
  2018-11-14  3:51             ` Tomasz Figa
@ 2018-11-16  4:34               ` mgottam
  2018-11-16  6:02                 ` Tomasz Figa
  0 siblings, 1 reply; 12+ messages in thread
From: mgottam @ 2018-11-16  4:34 UTC (permalink / raw)
  To: Tomasz Figa
  Cc: Stanimir Varbanov, Hans Verkuil, Mauro Carvalho Chehab,
	Linux Media Mailing List, Linux Kernel Mailing List,
	linux-arm-msm, Alexandre Courbot, vgarodia

On 2018-11-14 09:21, Tomasz Figa wrote:
> On Tue, Nov 13, 2018 at 7:46 PM Stanimir Varbanov
> <stanimir.varbanov@linaro.org> wrote:
>> 
>> Hi Tomasz,
>> 
>> On 11/13/18 11:13 AM, Tomasz Figa wrote:
>> > On Tue, Nov 13, 2018 at 5:12 PM Stanimir Varbanov
>> > <stanimir.varbanov@linaro.org> wrote:
>> >>
>> >> Hi Malathi,
>> >>
>> >> On 11/13/18 9:28 AM, mgottam@codeaurora.org wrote:
>> >>> On 2018-11-12 18:04, Stanimir Varbanov wrote:
>> >>>> Hi Tomasz,
>> >>>>
>> >>>> On 10/23/2018 05:50 AM, Tomasz Figa wrote:
>> >>>>> Hi Malathi,
>> >>>>>
>> >>>>> On Tue, Oct 9, 2018 at 4:58 PM Malathi Gottam
>> >>>>> <mgottam@codeaurora.org> wrote:
>> >>>>>>
>> >>>>>> For lower resolutions, incase of encoder, the compressed
>> >>>>>> frame size is more than half of the corresponding input
>> >>>>>> YUV. Keep the size as same as YUV considering worst case.
>> >>>>>>
>> >>>>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
>> >>>>>> ---
>> >>>>>>  drivers/media/platform/qcom/venus/helpers.c | 2 +-
>> >>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>>>>>
>> >>>>>> diff --git a/drivers/media/platform/qcom/venus/helpers.c
>> >>>>>> b/drivers/media/platform/qcom/venus/helpers.c
>> >>>>>> index 2679adb..05c5423 100644
>> >>>>>> --- a/drivers/media/platform/qcom/venus/helpers.c
>> >>>>>> +++ b/drivers/media/platform/qcom/venus/helpers.c
>> >>>>>> @@ -649,7 +649,7 @@ u32 venus_helper_get_framesz(u32 v4l2_fmt, u32
>> >>>>>> width, u32 height)
>> >>>>>>         }
>> >>>>>>
>> >>>>>>         if (compressed) {
>> >>>>>> -               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2 / 2;
>> >>>>>> +               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2;
>> >>>>>>                 return ALIGN(sz, SZ_4K);
>> >>>>>>         }
>> >>>>>
>> >>>>> Note that the driver should not enforce one particular buffer size for
>> >>>>> bitstream buffers unless it's a workaround for broken firmware or
>> >>>>> hardware. The userspace should be able to select the desired size.
>> >>>>
>> >>>> Good point! Yes, we have to extend set_fmt to allow bigger sizeimage for
>> >>>> the compressed buffers (not only for encoder).
>> >>>
>> >>> So Stan you meant to say that we should allow s_fmt to accept client
>> >>> specified size?
>> >>
>> >> yes but I do expect:
>> >>
>> >> new_sizeimage = max(user_sizeimage, venus_helper_get_framesz)
>> >>
>> >> and also user_sizeimage should be sanitized.
>> >>
>> >>> If so should we set the inst->input_buf_size here in venc_s_fmt?
>> >>>
>> >>> @@ -333,10 +333,10 @@static const struct venus_format *
>> >>> venc_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);
>> >>> +       if (!pfmt[0].sizeimage)
>> >>> +               pfmt[0].sizeimage =
>> >>> venus_helper_get_framesz(pixmp->pixelformat,
>> >>> +                                                            pixmp->width,
>> >>> +
>> >>> pixmp->height);
>> >>
>> >> yes, but please make
>> >>
>> >> pfmt[0].sizeimage = max(pfmt[0].sizeimage, venus_helper_get_framesz)
>> >>
>> >> and IMO this should be only for CAPTURE queue i.e. inst->output_buf_size
>> >>
>> >> I'm still not sure do we need it for OUTPUT encoder queue.
>> >>
>> >
>> > This would be indeed only for the queues that operate on a coded
>> > bitstream, i.e. both encoder CAPTURE and decoder OUTPUT.
>> 
>> Thanks for the confirmation.

So in case of encoder, adhering to the above comments

@@ -333,10 +333,10 @@static const struct venus_format *
venc_try_fmt_common(struct venus_inst *inst, struct v4l2_format *f)

+       sizeimage = venus_helper_get_framesz(pixmp->pixelformat,
                                                      pixmp->width,
                                                      pixmp->height);
+       pfmt[0].sizeimage = max(ALIGN(pfmt[0].sizeimage, SZ_4K), 
sizeimage);

@@ -408,8 +412,10 @@ static int venc_s_fmt(struct file *file, void *fh, 
struct v4l2_format *f)

         if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
                 inst->fmt_out = fmt;
-       else if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
+       else if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
                 inst->fmt_cap = fmt;
+               inst->output_buf_size = pixmp->plane_fmt[0].sizeimage;
+       }


>> 
>> >
>> > For image formats, sizeimage should be calculated by the driver based
>> > on the bytesperline and height. (Bytesperline may be fixed, if the
>> > hardware doesn't support flexible strides, but if it does, it's
>> > strongly recommended to use the bytesperline coming from the
>> > application as the stride +/- any necessary sanity checks.)
>> 
>> the hw should support stride but I'm not sure is that exposed by the
>> firmware interface.
> 
> After thinking a bit more on this, there is actually some redundancy
> between format width and crop width, since one should be normally able
> to just set the format width to the buffer stride and crop to the
> buffer width and have arbitrary strides supported (+/- hw alignment
> requirements, but that's something that has to always be accounted
> for).
> 
> Best regards,
> Tomasz

I hope the above change, takes into consideration the application
provided format width and also uses it in calculation of sizeimage which
is compared against application provided size aligned.


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] media: venus: amend buffer size for bitstream plane
  2018-11-16  4:34               ` mgottam
@ 2018-11-16  6:02                 ` Tomasz Figa
  0 siblings, 0 replies; 12+ messages in thread
From: Tomasz Figa @ 2018-11-16  6:02 UTC (permalink / raw)
  To: mgottam
  Cc: Stanimir Varbanov, Hans Verkuil, Mauro Carvalho Chehab,
	Linux Media Mailing List, Linux Kernel Mailing List,
	linux-arm-msm, Alexandre Courbot, vgarodia

On Fri, Nov 16, 2018 at 1:35 PM <mgottam@codeaurora.org> wrote:
>
> On 2018-11-14 09:21, Tomasz Figa wrote:
> > On Tue, Nov 13, 2018 at 7:46 PM Stanimir Varbanov
> > <stanimir.varbanov@linaro.org> wrote:
> >>
> >> Hi Tomasz,
> >>
> >> On 11/13/18 11:13 AM, Tomasz Figa wrote:
> >> > On Tue, Nov 13, 2018 at 5:12 PM Stanimir Varbanov
> >> > <stanimir.varbanov@linaro.org> wrote:
> >> >>
> >> >> Hi Malathi,
> >> >>
> >> >> On 11/13/18 9:28 AM, mgottam@codeaurora.org wrote:
> >> >>> On 2018-11-12 18:04, Stanimir Varbanov wrote:
> >> >>>> Hi Tomasz,
> >> >>>>
> >> >>>> On 10/23/2018 05:50 AM, Tomasz Figa wrote:
> >> >>>>> Hi Malathi,
> >> >>>>>
> >> >>>>> On Tue, Oct 9, 2018 at 4:58 PM Malathi Gottam
> >> >>>>> <mgottam@codeaurora.org> wrote:
> >> >>>>>>
> >> >>>>>> For lower resolutions, incase of encoder, the compressed
> >> >>>>>> frame size is more than half of the corresponding input
> >> >>>>>> YUV. Keep the size as same as YUV considering worst case.
> >> >>>>>>
> >> >>>>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> >> >>>>>> ---
> >> >>>>>>  drivers/media/platform/qcom/venus/helpers.c | 2 +-
> >> >>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >>>>>>
> >> >>>>>> diff --git a/drivers/media/platform/qcom/venus/helpers.c
> >> >>>>>> b/drivers/media/platform/qcom/venus/helpers.c
> >> >>>>>> index 2679adb..05c5423 100644
> >> >>>>>> --- a/drivers/media/platform/qcom/venus/helpers.c
> >> >>>>>> +++ b/drivers/media/platform/qcom/venus/helpers.c
> >> >>>>>> @@ -649,7 +649,7 @@ u32 venus_helper_get_framesz(u32 v4l2_fmt, u32
> >> >>>>>> width, u32 height)
> >> >>>>>>         }
> >> >>>>>>
> >> >>>>>>         if (compressed) {
> >> >>>>>> -               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2 / 2;
> >> >>>>>> +               sz = ALIGN(height, 32) * ALIGN(width, 32) * 3 / 2;
> >> >>>>>>                 return ALIGN(sz, SZ_4K);
> >> >>>>>>         }
> >> >>>>>
> >> >>>>> Note that the driver should not enforce one particular buffer size for
> >> >>>>> bitstream buffers unless it's a workaround for broken firmware or
> >> >>>>> hardware. The userspace should be able to select the desired size.
> >> >>>>
> >> >>>> Good point! Yes, we have to extend set_fmt to allow bigger sizeimage for
> >> >>>> the compressed buffers (not only for encoder).
> >> >>>
> >> >>> So Stan you meant to say that we should allow s_fmt to accept client
> >> >>> specified size?
> >> >>
> >> >> yes but I do expect:
> >> >>
> >> >> new_sizeimage = max(user_sizeimage, venus_helper_get_framesz)
> >> >>
> >> >> and also user_sizeimage should be sanitized.
> >> >>
> >> >>> If so should we set the inst->input_buf_size here in venc_s_fmt?
> >> >>>
> >> >>> @@ -333,10 +333,10 @@static const struct venus_format *
> >> >>> venc_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);
> >> >>> +       if (!pfmt[0].sizeimage)
> >> >>> +               pfmt[0].sizeimage =
> >> >>> venus_helper_get_framesz(pixmp->pixelformat,
> >> >>> +                                                            pixmp->width,
> >> >>> +
> >> >>> pixmp->height);
> >> >>
> >> >> yes, but please make
> >> >>
> >> >> pfmt[0].sizeimage = max(pfmt[0].sizeimage, venus_helper_get_framesz)
> >> >>
> >> >> and IMO this should be only for CAPTURE queue i.e. inst->output_buf_size
> >> >>
> >> >> I'm still not sure do we need it for OUTPUT encoder queue.
> >> >>
> >> >
> >> > This would be indeed only for the queues that operate on a coded
> >> > bitstream, i.e. both encoder CAPTURE and decoder OUTPUT.
> >>
> >> Thanks for the confirmation.
>
> So in case of encoder, adhering to the above comments
>
> @@ -333,10 +333,10 @@static const struct venus_format *
> venc_try_fmt_common(struct venus_inst *inst, struct v4l2_format *f)
>
> +       sizeimage = venus_helper_get_framesz(pixmp->pixelformat,
>                                                       pixmp->width,
>                                                       pixmp->height);

Note that in current version of the specification the application is
not expected to provide the width and height on the CAPTURE queue for
the encoder, so you would end up with 0 here. That said, I can see a
value in setting those as a hint to the driver, so perhaps it's not a
bad idea to add it to the API.

> +       pfmt[0].sizeimage = max(ALIGN(pfmt[0].sizeimage, SZ_4K),
> sizeimage);
>
> @@ -408,8 +412,10 @@ static int venc_s_fmt(struct file *file, void *fh,
> struct v4l2_format *f)
>
>          if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>                  inst->fmt_out = fmt;
> -       else if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)
> +       else if (f->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
>                  inst->fmt_cap = fmt;
> +               inst->output_buf_size = pixmp->plane_fmt[0].sizeimage;
> +       }
>
>
> >>
> >> >
> >> > For image formats, sizeimage should be calculated by the driver based
> >> > on the bytesperline and height. (Bytesperline may be fixed, if the
> >> > hardware doesn't support flexible strides, but if it does, it's
> >> > strongly recommended to use the bytesperline coming from the
> >> > application as the stride +/- any necessary sanity checks.)
> >>
> >> the hw should support stride but I'm not sure is that exposed by the
> >> firmware interface.
> >
> > After thinking a bit more on this, there is actually some redundancy
> > between format width and crop width, since one should be normally able
> > to just set the format width to the buffer stride and crop to the
> > buffer width and have arbitrary strides supported (+/- hw alignment
> > requirements, but that's something that has to always be accounted
> > for).
> >
> > Best regards,
> > Tomasz
>
> I hope the above change, takes into consideration the application
> provided format width and also uses it in calculation of sizeimage which
> is compared against application provided size aligned.
>

I think it should work +/- the one minor remark above. Thanks.

Best regards,
Tomasz

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-11-16  6:03 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-09  7:52 [PATCH] media: venus: amend buffer size for bitstream plane Malathi Gottam
2018-10-22  6:12 ` Alexandre Courbot
2018-10-23  2:50 ` Tomasz Figa
2018-11-12 12:34   ` Stanimir Varbanov
2018-11-13  7:28     ` mgottam
2018-11-13  8:12       ` Stanimir Varbanov
2018-11-13  9:13         ` Tomasz Figa
2018-11-13 10:46           ` Stanimir Varbanov
2018-11-14  3:51             ` Tomasz Figa
2018-11-16  4:34               ` mgottam
2018-11-16  6:02                 ` Tomasz Figa
2018-11-12 12:28 ` Stanimir Varbanov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).