[PATCH v3] media: venus: amend buffer size for bitstream plane

[PATCH v3] media: venus: amend buffer size for bitstream plane

[Malathi Gottam]

Accept the buffer size requested by client and compare it
against driver calculated size and set the maximum to
bitstream plane.

Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
---
 drivers/media/platform/qcom/venus/venc.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
index ce85962..e43dd3d 100644
--- a/drivers/media/platform/qcom/venus/venc.c
+++ b/drivers/media/platform/qcom/venus/venc.c
@@ -303,6 +303,7 @@ static int venc_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *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 sizeimage;
 
 	memset(pfmt[0].reserved, 0, sizeof(pfmt[0].reserved));
 	memset(pixmp->reserved, 0, sizeof(pixmp->reserved));
@@ -334,9 +335,10 @@ static int venc_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
 	pixmp->num_planes = fmt->num_planes;
 	pixmp->flags = 0;
 
-	pfmt[0].sizeimage = venus_helper_get_framesz(pixmp->pixelformat,
-						     pixmp->width,
-						     pixmp->height);
+	sizeimage = venus_helper_get_framesz(pixmp->pixelformat,
+					     pixmp->width,
+					     pixmp->height);
+	pfmt[0].sizeimage = max(ALIGN(pfmt[0].sizeimage, SZ_4K), sizeimage);
 
 	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
 		pfmt[0].bytesperline = ALIGN(pixmp->width, 128);
@@ -408,8 +410,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;
+	}
 
 	return 0;
 }
@@ -908,6 +912,7 @@ static int venc_queue_setup(struct vb2_queue *q,
 		sizes[0] = venus_helper_get_framesz(inst->fmt_cap->pixfmt,
 						    inst->width,
 						    inst->height);
+		sizes[0] = max(sizes[0], inst->output_buf_size);
 		inst->output_buf_size = sizes[0];
 		break;
 	default:
-- 
1.9.1

Re: [PATCH v3] media: venus: amend buffer size for bitstream plane

[Stanimir Varbanov]

Hi Malathi,

Thanks for the patch!

On 11/26/18 12:12 PM, Malathi Gottam wrote:
> Accept the buffer size requested by client and compare it
> against driver calculated size and set the maximum to
> bitstream plane.
> 
> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> ---
>  drivers/media/platform/qcom/venus/venc.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)

Acked-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

> 
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index ce85962..e43dd3d 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -303,6 +303,7 @@ static int venc_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *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 sizeimage;
>  
>  	memset(pfmt[0].reserved, 0, sizeof(pfmt[0].reserved));
>  	memset(pixmp->reserved, 0, sizeof(pixmp->reserved));
> @@ -334,9 +335,10 @@ static int venc_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
>  	pixmp->num_planes = fmt->num_planes;
>  	pixmp->flags = 0;
>  
> -	pfmt[0].sizeimage = venus_helper_get_framesz(pixmp->pixelformat,
> -						     pixmp->width,
> -						     pixmp->height);
> +	sizeimage = venus_helper_get_framesz(pixmp->pixelformat,
> +					     pixmp->width,
> +					     pixmp->height);
> +	pfmt[0].sizeimage = max(ALIGN(pfmt[0].sizeimage, SZ_4K), sizeimage);
>  
>  	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>  		pfmt[0].bytesperline = ALIGN(pixmp->width, 128);
> @@ -408,8 +410,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;
> +	}
>  
>  	return 0;
>  }
> @@ -908,6 +912,7 @@ static int venc_queue_setup(struct vb2_queue *q,
>  		sizes[0] = venus_helper_get_framesz(inst->fmt_cap->pixfmt,
>  						    inst->width,
>  						    inst->height);
> +		sizes[0] = max(sizes[0], inst->output_buf_size);
>  		inst->output_buf_size = sizes[0];
>  		break;
>  	default:
> 

-- 
regards,
Stan

Re: [PATCH v3] media: venus: amend buffer size for bitstream plane

[Hans Verkuil]

On 11/26/2018 11:12 AM, Malathi Gottam wrote:
> Accept the buffer size requested by client and compare it
> against driver calculated size and set the maximum to
> bitstream plane.
> 
> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>

Sorry, this isn't allowed. It is the driver that sets the sizeimage value,
never the other way around.

If you need to allocate larger buffers, then use VIDIOC_CREATE_BUFS instead
of VIDIOC_REQBUFS.

What problem are you trying to solve with this patch?

Regards,

	Hans

> ---
>  drivers/media/platform/qcom/venus/venc.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/qcom/venus/venc.c b/drivers/media/platform/qcom/venus/venc.c
> index ce85962..e43dd3d 100644
> --- a/drivers/media/platform/qcom/venus/venc.c
> +++ b/drivers/media/platform/qcom/venus/venc.c
> @@ -303,6 +303,7 @@ static int venc_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *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 sizeimage;
>  
>  	memset(pfmt[0].reserved, 0, sizeof(pfmt[0].reserved));
>  	memset(pixmp->reserved, 0, sizeof(pixmp->reserved));
> @@ -334,9 +335,10 @@ static int venc_enum_fmt(struct file *file, void *fh, struct v4l2_fmtdesc *f)
>  	pixmp->num_planes = fmt->num_planes;
>  	pixmp->flags = 0;
>  
> -	pfmt[0].sizeimage = venus_helper_get_framesz(pixmp->pixelformat,
> -						     pixmp->width,
> -						     pixmp->height);
> +	sizeimage = venus_helper_get_framesz(pixmp->pixelformat,
> +					     pixmp->width,
> +					     pixmp->height);
> +	pfmt[0].sizeimage = max(ALIGN(pfmt[0].sizeimage, SZ_4K), sizeimage);
>  
>  	if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
>  		pfmt[0].bytesperline = ALIGN(pixmp->width, 128);
> @@ -408,8 +410,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;
> +	}
>  
>  	return 0;
>  }
> @@ -908,6 +912,7 @@ static int venc_queue_setup(struct vb2_queue *q,
>  		sizes[0] = venus_helper_get_framesz(inst->fmt_cap->pixfmt,
>  						    inst->width,
>  						    inst->height);
> +		sizes[0] = max(sizes[0], inst->output_buf_size);
>  		inst->output_buf_size = sizes[0];
>  		break;
>  	default:
> 

Re: [PATCH v3] media: venus: amend buffer size for bitstream plane

[Stanimir Varbanov]

Hi Hans,

On 11/26/18 3:37 PM, Hans Verkuil wrote:
> On 11/26/2018 11:12 AM, Malathi Gottam wrote:
>> Accept the buffer size requested by client and compare it
>> against driver calculated size and set the maximum to
>> bitstream plane.
>>
>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> 
> Sorry, this isn't allowed. It is the driver that sets the sizeimage value,
> never the other way around.

I think for decoders (OUTPUT queue) and encoders (CAPTURE queue) we
allowed userspace to set sizeimage for buffers. See [1] Initialization
paragraph point 2:

    ``sizeimage``
       desired size of ``CAPTURE`` buffers; the encoder may adjust it to
       match hardware requirements

Similar patch we be needed for decoder as well.

> 
> If you need to allocate larger buffers, then use VIDIOC_CREATE_BUFS instead
> of VIDIOC_REQBUFS.
> 
> What problem are you trying to solve with this patch?
> 
> Regards,
> 
> 	Hans
> 

-- 
regards,
Stan

[1] https://www.spinics.net/lists/linux-media/msg142049.html

Re: [PATCH v3] media: venus: amend buffer size for bitstream plane

[Hans Verkuil]

On 11/26/2018 03:57 PM, Stanimir Varbanov wrote:
> Hi Hans,
> 
> On 11/26/18 3:37 PM, Hans Verkuil wrote:
>> On 11/26/2018 11:12 AM, Malathi Gottam wrote:
>>> Accept the buffer size requested by client and compare it
>>> against driver calculated size and set the maximum to
>>> bitstream plane.
>>>
>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
>>
>> Sorry, this isn't allowed. It is the driver that sets the sizeimage value,
>> never the other way around.
> 
> I think for decoders (OUTPUT queue) and encoders (CAPTURE queue) we
> allowed userspace to set sizeimage for buffers. See [1] Initialization
> paragraph point 2:
> 
>     ``sizeimage``
>        desired size of ``CAPTURE`` buffers; the encoder may adjust it to
>        match hardware requirements
> 
> Similar patch we be needed for decoder as well.

I may have missed that change since it wasn't present in v1 of the stateful
encoder spec.

Tomasz, what was the reason for this change? I vaguely remember some thread
about this, but I forgot the details. Since this would be a departure of
the current API this should be explained in more detail.

I don't really see the point of requiring userspace to fill this in. For
stateful codecs it can just return some reasonable size. Possibly calculated
using the provided width/height values or (if those are 0) some default value.

Ditto for decoders.

Stanimir, I certainly cannot merge this until this has been fully nailed down
as it would be a departure from the current API.

And looking at the venus patch I do not see how it helps userspace.

Regards,

	Hans

> 
>>
>> If you need to allocate larger buffers, then use VIDIOC_CREATE_BUFS instead
>> of VIDIOC_REQBUFS.
>>
>> What problem are you trying to solve with this patch?
>>
>> Regards,
>>
>> 	Hans
>>
> 

Re: [PATCH v3] media: venus: amend buffer size for bitstream plane

[Tomasz Figa]

Hi Hans,

On Tue, Nov 27, 2018 at 12:24 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/26/2018 03:57 PM, Stanimir Varbanov wrote:
> > Hi Hans,
> >
> > On 11/26/18 3:37 PM, Hans Verkuil wrote:
> >> On 11/26/2018 11:12 AM, Malathi Gottam wrote:
> >>> Accept the buffer size requested by client and compare it
> >>> against driver calculated size and set the maximum to
> >>> bitstream plane.
> >>>
> >>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> >>
> >> Sorry, this isn't allowed. It is the driver that sets the sizeimage value,
> >> never the other way around.
> >
> > I think for decoders (OUTPUT queue) and encoders (CAPTURE queue) we
> > allowed userspace to set sizeimage for buffers. See [1] Initialization
> > paragraph point 2:
> >
> >     ``sizeimage``
> >        desired size of ``CAPTURE`` buffers; the encoder may adjust it to
> >        match hardware requirements
> >
> > Similar patch we be needed for decoder as well.
>
> I may have missed that change since it wasn't present in v1 of the stateful
> encoder spec.

It's been there from the very beginning, even before I started working
on it. Actually, even the early slides from Kamil mention the
application setting the buffer size for compressed streams:
https://events.static.linuxfound.org/images/stories/pdf/lceu2012_debski.pdf

>
> Tomasz, what was the reason for this change? I vaguely remember some thread
> about this, but I forgot the details. Since this would be a departure of
> the current API this should be explained in more detail.

The kernel is not the place to encode assumptions about optimal
bitstream chunk sizes. It depends on the use case and the application
should be able decide. It may for example want to use smaller buffers,
optimizing for the well compressible video streams and just reallocate
if bigger chunks are needed.

>
> I don't really see the point of requiring userspace to fill this in. For
> stateful codecs it can just return some reasonable size. Possibly calculated
> using the provided width/height values or (if those are 0) some default value.

How do we decide what's "reasonable"? Would it be reasonable for all
applications?

>
> Ditto for decoders.
>
> Stanimir, I certainly cannot merge this until this has been fully nailed down
> as it would be a departure from the current API.

It would not be a departure, because I can see existing stateful
drivers behaving like that:
https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c#L1444
https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c#L469

Also, Chromium has been setting the size on its own for long time
using its own heuristics.

>
> And looking at the venus patch I do not see how it helps userspace.
>
> Regards,
>
>         Hans
>
> >
> >>
> >> If you need to allocate larger buffers, then use VIDIOC_CREATE_BUFS instead
> >> of VIDIOC_REQBUFS.

CREATE_BUFS wouldn't work, because one needs to use TRY_FMT to obtain
a format for it and the format returned by it would have the sizeimage
as hardcoded in the driver...

Best regards,
Tomasz

Re: [PATCH v3] media: venus: amend buffer size for bitstream plane

[Hans Verkuil]

On 11/26/2018 04:44 PM, Tomasz Figa wrote:
> Hi Hans,
> 
> On Tue, Nov 27, 2018 at 12:24 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 11/26/2018 03:57 PM, Stanimir Varbanov wrote:
>>> Hi Hans,
>>>
>>> On 11/26/18 3:37 PM, Hans Verkuil wrote:
>>>> On 11/26/2018 11:12 AM, Malathi Gottam wrote:
>>>>> Accept the buffer size requested by client and compare it
>>>>> against driver calculated size and set the maximum to
>>>>> bitstream plane.
>>>>>
>>>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
>>>>
>>>> Sorry, this isn't allowed. It is the driver that sets the sizeimage value,
>>>> never the other way around.
>>>
>>> I think for decoders (OUTPUT queue) and encoders (CAPTURE queue) we
>>> allowed userspace to set sizeimage for buffers. See [1] Initialization
>>> paragraph point 2:
>>>
>>>     ``sizeimage``
>>>        desired size of ``CAPTURE`` buffers; the encoder may adjust it to
>>>        match hardware requirements
>>>
>>> Similar patch we be needed for decoder as well.
>>
>> I may have missed that change since it wasn't present in v1 of the stateful
>> encoder spec.
> 
> It's been there from the very beginning, even before I started working
> on it. Actually, even the early slides from Kamil mention the
> application setting the buffer size for compressed streams:
> https://events.static.linuxfound.org/images/stories/pdf/lceu2012_debski.pdf
> 
>>
>> Tomasz, what was the reason for this change? I vaguely remember some thread
>> about this, but I forgot the details. Since this would be a departure of
>> the current API this should be explained in more detail.
> 
> The kernel is not the place to encode assumptions about optimal
> bitstream chunk sizes. It depends on the use case and the application
> should be able decide. It may for example want to use smaller buffers,
> optimizing for the well compressible video streams and just reallocate
> if bigger chunks are needed.
> 
>>
>> I don't really see the point of requiring userspace to fill this in. For
>> stateful codecs it can just return some reasonable size. Possibly calculated
>> using the provided width/height values or (if those are 0) some default value.
> 
> How do we decide what's "reasonable"? Would it be reasonable for all
> applications?

In theory it should be the minimum size that the hardware supports. But it is
silly to i.e. provide the size of one PAGE as the minimum. In practice you
probably want to set sizeimage to something larger than that. Depending on
the typical compression ratio perhaps 5 or 10% of what a raw YUV 4:2:0 frame
would be.

> 
>>
>> Ditto for decoders.
>>
>> Stanimir, I certainly cannot merge this until this has been fully nailed down
>> as it would be a departure from the current API.
> 
> It would not be a departure, because I can see existing stateful
> drivers behaving like that:
> https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c#L1444
> https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c#L469

Yes, and that's out of spec. Clearly v4l2-compliance doesn't test for this.
It should have been caught at least for the mtk driver.

> 
> Also, Chromium has been setting the size on its own for long time
> using its own heuristics.
> 
>>
>> And looking at the venus patch I do not see how it helps userspace.
>>
>> Regards,
>>
>>         Hans
>>
>>>
>>>>
>>>> If you need to allocate larger buffers, then use VIDIOC_CREATE_BUFS instead
>>>> of VIDIOC_REQBUFS.
> 
> CREATE_BUFS wouldn't work, because one needs to use TRY_FMT to obtain
> a format for it and the format returned by it would have the sizeimage
> as hardcoded in the driver...

???

Userspace can change the sizeimage to whatever it wants before calling
CREATE_BUFS as long as it is >= the sizeimage of the current format.

If we want to allow smaller sizes, then I think that would not be
unreasonable for stateful codecs. I actually think that drivers can
already do this in queue_setup(), but the spec would have to be updated
a bit.

Regards,

	Hans

> 
> Best regards,
> Tomasz
> 

Re: [PATCH v3] media: venus: amend buffer size for bitstream plane

[Tomasz Figa]

On Tue, Nov 27, 2018 at 1:00 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/26/2018 04:44 PM, Tomasz Figa wrote:
> > Hi Hans,
> >
> > On Tue, Nov 27, 2018 at 12:24 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 11/26/2018 03:57 PM, Stanimir Varbanov wrote:
> >>> Hi Hans,
> >>>
> >>> On 11/26/18 3:37 PM, Hans Verkuil wrote:
> >>>> On 11/26/2018 11:12 AM, Malathi Gottam wrote:
> >>>>> Accept the buffer size requested by client and compare it
> >>>>> against driver calculated size and set the maximum to
> >>>>> bitstream plane.
> >>>>>
> >>>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> >>>>
> >>>> Sorry, this isn't allowed. It is the driver that sets the sizeimage value,
> >>>> never the other way around.
> >>>
> >>> I think for decoders (OUTPUT queue) and encoders (CAPTURE queue) we
> >>> allowed userspace to set sizeimage for buffers. See [1] Initialization
> >>> paragraph point 2:
> >>>
> >>>     ``sizeimage``
> >>>        desired size of ``CAPTURE`` buffers; the encoder may adjust it to
> >>>        match hardware requirements
> >>>
> >>> Similar patch we be needed for decoder as well.
> >>
> >> I may have missed that change since it wasn't present in v1 of the stateful
> >> encoder spec.
> >
> > It's been there from the very beginning, even before I started working
> > on it. Actually, even the early slides from Kamil mention the
> > application setting the buffer size for compressed streams:
> > https://events.static.linuxfound.org/images/stories/pdf/lceu2012_debski.pdf
> >
> >>
> >> Tomasz, what was the reason for this change? I vaguely remember some thread
> >> about this, but I forgot the details. Since this would be a departure of
> >> the current API this should be explained in more detail.
> >
> > The kernel is not the place to encode assumptions about optimal
> > bitstream chunk sizes. It depends on the use case and the application
> > should be able decide. It may for example want to use smaller buffers,
> > optimizing for the well compressible video streams and just reallocate
> > if bigger chunks are needed.
> >
> >>
> >> I don't really see the point of requiring userspace to fill this in. For
> >> stateful codecs it can just return some reasonable size. Possibly calculated
> >> using the provided width/height values or (if those are 0) some default value.
> >
> > How do we decide what's "reasonable"? Would it be reasonable for all
> > applications?
>
> In theory it should be the minimum size that the hardware supports. But it is
> silly to i.e. provide the size of one PAGE as the minimum. In practice you
> probably want to set sizeimage to something larger than that. Depending on
> the typical compression ratio perhaps 5 or 10% of what a raw YUV 4:2:0 frame
> would be.
>
> >
> >>
> >> Ditto for decoders.
> >>
> >> Stanimir, I certainly cannot merge this until this has been fully nailed down
> >> as it would be a departure from the current API.
> >
> > It would not be a departure, because I can see existing stateful
> > drivers behaving like that:
> > https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c#L1444
> > https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c#L469
>
> Yes, and that's out of spec. Clearly v4l2-compliance doesn't test for this.
> It should have been caught at least for the mtk driver.
>

Perhaps we should make it a part of the spec then?

Actually I'm not really sure if we can say that this is out of spec
There has been no clear spec for the stateful codecs for many years,
with drivers doing wildly whatever they like and applications ending
up relying on those quirks.

My spec actually attempts to incorporate what was decided on the
earlier summits, including what's in Kamil's slides, the drivers are
already doing and existing applications rely on. The sizeimage
handling is just a part of it.

> >
> > Also, Chromium has been setting the size on its own for long time
> > using its own heuristics.
> >
> >>
> >> And looking at the venus patch I do not see how it helps userspace.
> >>
> >> Regards,
> >>
> >>         Hans
> >>
> >>>
> >>>>
> >>>> If you need to allocate larger buffers, then use VIDIOC_CREATE_BUFS instead
> >>>> of VIDIOC_REQBUFS.
> >
> > CREATE_BUFS wouldn't work, because one needs to use TRY_FMT to obtain
> > a format for it and the format returned by it would have the sizeimage
> > as hardcoded in the driver...
>
> ???
>
> Userspace can change the sizeimage to whatever it wants before calling
> CREATE_BUFS as long as it is >= the sizeimage of the current format.
>
> If we want to allow smaller sizes, then I think that would not be
> unreasonable for stateful codecs. I actually think that drivers can
> already do this in queue_setup(), but the spec would have to be updated
> a bit.

Existing applications rely on REQBUFS honoring the size they set in
sizeimage, though...

Best regards,
Tomasz

Re: [PATCH v3] media: venus: amend buffer size for bitstream plane

[Hans Verkuil]

On 11/26/2018 05:07 PM, Tomasz Figa wrote:
> On Tue, Nov 27, 2018 at 1:00 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 11/26/2018 04:44 PM, Tomasz Figa wrote:
>>> Hi Hans,
>>>
>>> On Tue, Nov 27, 2018 at 12:24 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> On 11/26/2018 03:57 PM, Stanimir Varbanov wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On 11/26/18 3:37 PM, Hans Verkuil wrote:
>>>>>> On 11/26/2018 11:12 AM, Malathi Gottam wrote:
>>>>>>> Accept the buffer size requested by client and compare it
>>>>>>> against driver calculated size and set the maximum to
>>>>>>> bitstream plane.
>>>>>>>
>>>>>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
>>>>>>
>>>>>> Sorry, this isn't allowed. It is the driver that sets the sizeimage value,
>>>>>> never the other way around.
>>>>>
>>>>> I think for decoders (OUTPUT queue) and encoders (CAPTURE queue) we
>>>>> allowed userspace to set sizeimage for buffers. See [1] Initialization
>>>>> paragraph point 2:
>>>>>
>>>>>     ``sizeimage``
>>>>>        desired size of ``CAPTURE`` buffers; the encoder may adjust it to
>>>>>        match hardware requirements
>>>>>
>>>>> Similar patch we be needed for decoder as well.
>>>>
>>>> I may have missed that change since it wasn't present in v1 of the stateful
>>>> encoder spec.
>>>
>>> It's been there from the very beginning, even before I started working
>>> on it. Actually, even the early slides from Kamil mention the
>>> application setting the buffer size for compressed streams:
>>> https://events.static.linuxfound.org/images/stories/pdf/lceu2012_debski.pdf
>>>
>>>>
>>>> Tomasz, what was the reason for this change? I vaguely remember some thread
>>>> about this, but I forgot the details. Since this would be a departure of
>>>> the current API this should be explained in more detail.
>>>
>>> The kernel is not the place to encode assumptions about optimal
>>> bitstream chunk sizes. It depends on the use case and the application
>>> should be able decide. It may for example want to use smaller buffers,
>>> optimizing for the well compressible video streams and just reallocate
>>> if bigger chunks are needed.
>>>
>>>>
>>>> I don't really see the point of requiring userspace to fill this in. For
>>>> stateful codecs it can just return some reasonable size. Possibly calculated
>>>> using the provided width/height values or (if those are 0) some default value.
>>>
>>> How do we decide what's "reasonable"? Would it be reasonable for all
>>> applications?
>>
>> In theory it should be the minimum size that the hardware supports. But it is
>> silly to i.e. provide the size of one PAGE as the minimum. In practice you
>> probably want to set sizeimage to something larger than that. Depending on
>> the typical compression ratio perhaps 5 or 10% of what a raw YUV 4:2:0 frame
>> would be.
>>
>>>
>>>>
>>>> Ditto for decoders.
>>>>
>>>> Stanimir, I certainly cannot merge this until this has been fully nailed down
>>>> as it would be a departure from the current API.
>>>
>>> It would not be a departure, because I can see existing stateful
>>> drivers behaving like that:
>>> https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c#L1444
>>> https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c#L469
>>
>> Yes, and that's out of spec. Clearly v4l2-compliance doesn't test for this.
>> It should have been caught at least for the mtk driver.
>>
> 
> Perhaps we should make it a part of the spec then?
> 
> Actually I'm not really sure if we can say that this is out of spec
> There has been no clear spec for the stateful codecs for many years,
> with drivers doing wildly whatever they like and applications ending
> up relying on those quirks.

The VIDIOC_S_FMT spec for v4l2_pix_format is quite clear that it is the
driver that sets this value. The spec for v4l2_plane_pix_format is
unfortunately not so clear.

> My spec actually attempts to incorporate what was decided on the
> earlier summits, including what's in Kamil's slides, the drivers are
> already doing and existing applications rely on. The sizeimage
> handling is just a part of it.
> 
>>>
>>> Also, Chromium has been setting the size on its own for long time
>>> using its own heuristics.
>>>
>>>>
>>>> And looking at the venus patch I do not see how it helps userspace.
>>>>
>>>> Regards,
>>>>
>>>>         Hans
>>>>
>>>>>
>>>>>>
>>>>>> If you need to allocate larger buffers, then use VIDIOC_CREATE_BUFS instead
>>>>>> of VIDIOC_REQBUFS.
>>>
>>> CREATE_BUFS wouldn't work, because one needs to use TRY_FMT to obtain
>>> a format for it and the format returned by it would have the sizeimage
>>> as hardcoded in the driver...
>>
>> ???
>>
>> Userspace can change the sizeimage to whatever it wants before calling
>> CREATE_BUFS as long as it is >= the sizeimage of the current format.
>>
>> If we want to allow smaller sizes, then I think that would not be
>> unreasonable for stateful codecs. I actually think that drivers can
>> already do this in queue_setup(), but the spec would have to be updated
>> a bit.
> 
> Existing applications rely on REQBUFS honoring the size they set in
> sizeimage, though...

REQBUFS, yes. But not CREATE_BUFS. Which is why that ioctl was added in the
first place.

However (and now I remember the real problem with CREATE_BUFS) the spec for
CREATE_BUFS says that it will not change the provided sizeimage value. So
any adjustments required due to specific alignment requirements won't be
applied, all CREATE_BUFS can do is to reject it.

So what this boils down to is a change to the spec:

For compressed formats (and only those!) userspace can set sizeimage to a
proposed value. The driver may either ignore it and just set its own value,
or modify it to satisfy HW requirements. The returned value will be used
by REQBUFS when it allocates buffers.

I think this is reasonable, provided the spec is updated accordingly.

As far as I can tell this shouldn't cause any backwards compatibility
problems, and it should be easy to test in v4l2-compliance.

Regards,

	Hans

Re: [PATCH v3] media: venus: amend buffer size for bitstream plane

[Alexandre Courbot]

Hi Hans,

On Tue, Nov 27, 2018 at 1:41 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/26/2018 05:07 PM, Tomasz Figa wrote:
> > On Tue, Nov 27, 2018 at 1:00 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 11/26/2018 04:44 PM, Tomasz Figa wrote:
> >>> Hi Hans,
> >>>
> >>> On Tue, Nov 27, 2018 at 12:24 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>
> >>>> On 11/26/2018 03:57 PM, Stanimir Varbanov wrote:
> >>>>> Hi Hans,
> >>>>>
> >>>>> On 11/26/18 3:37 PM, Hans Verkuil wrote:
> >>>>>> On 11/26/2018 11:12 AM, Malathi Gottam wrote:
> >>>>>>> Accept the buffer size requested by client and compare it
> >>>>>>> against driver calculated size and set the maximum to
> >>>>>>> bitstream plane.
> >>>>>>>
> >>>>>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> >>>>>>
> >>>>>> Sorry, this isn't allowed. It is the driver that sets the sizeimage value,
> >>>>>> never the other way around.
> >>>>>
> >>>>> I think for decoders (OUTPUT queue) and encoders (CAPTURE queue) we
> >>>>> allowed userspace to set sizeimage for buffers. See [1] Initialization
> >>>>> paragraph point 2:
> >>>>>
> >>>>>     ``sizeimage``
> >>>>>        desired size of ``CAPTURE`` buffers; the encoder may adjust it to
> >>>>>        match hardware requirements
> >>>>>
> >>>>> Similar patch we be needed for decoder as well.
> >>>>
> >>>> I may have missed that change since it wasn't present in v1 of the stateful
> >>>> encoder spec.
> >>>
> >>> It's been there from the very beginning, even before I started working
> >>> on it. Actually, even the early slides from Kamil mention the
> >>> application setting the buffer size for compressed streams:
> >>> https://events.static.linuxfound.org/images/stories/pdf/lceu2012_debski.pdf
> >>>
> >>>>
> >>>> Tomasz, what was the reason for this change? I vaguely remember some thread
> >>>> about this, but I forgot the details. Since this would be a departure of
> >>>> the current API this should be explained in more detail.
> >>>
> >>> The kernel is not the place to encode assumptions about optimal
> >>> bitstream chunk sizes. It depends on the use case and the application
> >>> should be able decide. It may for example want to use smaller buffers,
> >>> optimizing for the well compressible video streams and just reallocate
> >>> if bigger chunks are needed.
> >>>
> >>>>
> >>>> I don't really see the point of requiring userspace to fill this in. For
> >>>> stateful codecs it can just return some reasonable size. Possibly calculated
> >>>> using the provided width/height values or (if those are 0) some default value.
> >>>
> >>> How do we decide what's "reasonable"? Would it be reasonable for all
> >>> applications?
> >>
> >> In theory it should be the minimum size that the hardware supports. But it is
> >> silly to i.e. provide the size of one PAGE as the minimum. In practice you
> >> probably want to set sizeimage to something larger than that. Depending on
> >> the typical compression ratio perhaps 5 or 10% of what a raw YUV 4:2:0 frame
> >> would be.
> >>
> >>>
> >>>>
> >>>> Ditto for decoders.
> >>>>
> >>>> Stanimir, I certainly cannot merge this until this has been fully nailed down
> >>>> as it would be a departure from the current API.
> >>>
> >>> It would not be a departure, because I can see existing stateful
> >>> drivers behaving like that:
> >>> https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c#L1444
> >>> https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c#L469
> >>
> >> Yes, and that's out of spec. Clearly v4l2-compliance doesn't test for this.
> >> It should have been caught at least for the mtk driver.
> >>
> >
> > Perhaps we should make it a part of the spec then?
> >
> > Actually I'm not really sure if we can say that this is out of spec
> > There has been no clear spec for the stateful codecs for many years,
> > with drivers doing wildly whatever they like and applications ending
> > up relying on those quirks.
>
> The VIDIOC_S_FMT spec for v4l2_pix_format is quite clear that it is the
> driver that sets this value. The spec for v4l2_plane_pix_format is
> unfortunately not so clear.
>
> > My spec actually attempts to incorporate what was decided on the
> > earlier summits, including what's in Kamil's slides, the drivers are
> > already doing and existing applications rely on. The sizeimage
> > handling is just a part of it.
> >
> >>>
> >>> Also, Chromium has been setting the size on its own for long time
> >>> using its own heuristics.
> >>>
> >>>>
> >>>> And looking at the venus patch I do not see how it helps userspace.
> >>>>
> >>>> Regards,
> >>>>
> >>>>         Hans
> >>>>
> >>>>>
> >>>>>>
> >>>>>> If you need to allocate larger buffers, then use VIDIOC_CREATE_BUFS instead
> >>>>>> of VIDIOC_REQBUFS.
> >>>
> >>> CREATE_BUFS wouldn't work, because one needs to use TRY_FMT to obtain
> >>> a format for it and the format returned by it would have the sizeimage
> >>> as hardcoded in the driver...
> >>
> >> ???
> >>
> >> Userspace can change the sizeimage to whatever it wants before calling
> >> CREATE_BUFS as long as it is >= the sizeimage of the current format.
> >>
> >> If we want to allow smaller sizes, then I think that would not be
> >> unreasonable for stateful codecs. I actually think that drivers can
> >> already do this in queue_setup(), but the spec would have to be updated
> >> a bit.
> >
> > Existing applications rely on REQBUFS honoring the size they set in
> > sizeimage, though...
>
> REQBUFS, yes. But not CREATE_BUFS. Which is why that ioctl was added in the
> first place.
>
> However (and now I remember the real problem with CREATE_BUFS) the spec for
> CREATE_BUFS says that it will not change the provided sizeimage value. So
> any adjustments required due to specific alignment requirements won't be
> applied, all CREATE_BUFS can do is to reject it.
>
> So what this boils down to is a change to the spec:
>
> For compressed formats (and only those!) userspace can set sizeimage to a
> proposed value. The driver may either ignore it and just set its own value,
> or modify it to satisfy HW requirements. The returned value will be used
> by REQBUFS when it allocates buffers.
>
> I think this is reasonable, provided the spec is updated accordingly.
>
> As far as I can tell this shouldn't cause any backwards compatibility
> problems, and it should be easy to test in v4l2-compliance.

Do you mean that this patch is acceptable provided the stateful codec
specification is updated accordingly?

For our (Chromium) needs this seems to do the job, so:

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

Although I would like to also have the equivalent for the decoder's
OUTPUT queue, either as a v4 or a follow-up patch.

Re: [PATCH v3] media: venus: amend buffer size for bitstream plane

[Tomasz Figa]

On Tue, Nov 27, 2018 at 1:41 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 11/26/2018 05:07 PM, Tomasz Figa wrote:
> > On Tue, Nov 27, 2018 at 1:00 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>
> >> On 11/26/2018 04:44 PM, Tomasz Figa wrote:
> >>> Hi Hans,
> >>>
> >>> On Tue, Nov 27, 2018 at 12:24 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >>>>
> >>>> On 11/26/2018 03:57 PM, Stanimir Varbanov wrote:
> >>>>> Hi Hans,
> >>>>>
> >>>>> On 11/26/18 3:37 PM, Hans Verkuil wrote:
> >>>>>> On 11/26/2018 11:12 AM, Malathi Gottam wrote:
> >>>>>>> Accept the buffer size requested by client and compare it
> >>>>>>> against driver calculated size and set the maximum to
> >>>>>>> bitstream plane.
> >>>>>>>
> >>>>>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
> >>>>>>
> >>>>>> Sorry, this isn't allowed. It is the driver that sets the sizeimage value,
> >>>>>> never the other way around.
> >>>>>
> >>>>> I think for decoders (OUTPUT queue) and encoders (CAPTURE queue) we
> >>>>> allowed userspace to set sizeimage for buffers. See [1] Initialization
> >>>>> paragraph point 2:
> >>>>>
> >>>>>     ``sizeimage``
> >>>>>        desired size of ``CAPTURE`` buffers; the encoder may adjust it to
> >>>>>        match hardware requirements
> >>>>>
> >>>>> Similar patch we be needed for decoder as well.
> >>>>
> >>>> I may have missed that change since it wasn't present in v1 of the stateful
> >>>> encoder spec.
> >>>
> >>> It's been there from the very beginning, even before I started working
> >>> on it. Actually, even the early slides from Kamil mention the
> >>> application setting the buffer size for compressed streams:
> >>> https://events.static.linuxfound.org/images/stories/pdf/lceu2012_debski.pdf
> >>>
> >>>>
> >>>> Tomasz, what was the reason for this change? I vaguely remember some thread
> >>>> about this, but I forgot the details. Since this would be a departure of
> >>>> the current API this should be explained in more detail.
> >>>
> >>> The kernel is not the place to encode assumptions about optimal
> >>> bitstream chunk sizes. It depends on the use case and the application
> >>> should be able decide. It may for example want to use smaller buffers,
> >>> optimizing for the well compressible video streams and just reallocate
> >>> if bigger chunks are needed.
> >>>
> >>>>
> >>>> I don't really see the point of requiring userspace to fill this in. For
> >>>> stateful codecs it can just return some reasonable size. Possibly calculated
> >>>> using the provided width/height values or (if those are 0) some default value.
> >>>
> >>> How do we decide what's "reasonable"? Would it be reasonable for all
> >>> applications?
> >>
> >> In theory it should be the minimum size that the hardware supports. But it is
> >> silly to i.e. provide the size of one PAGE as the minimum. In practice you
> >> probably want to set sizeimage to something larger than that. Depending on
> >> the typical compression ratio perhaps 5 or 10% of what a raw YUV 4:2:0 frame
> >> would be.
> >>
> >>>
> >>>>
> >>>> Ditto for decoders.
> >>>>
> >>>> Stanimir, I certainly cannot merge this until this has been fully nailed down
> >>>> as it would be a departure from the current API.
> >>>
> >>> It would not be a departure, because I can see existing stateful
> >>> drivers behaving like that:
> >>> https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c#L1444
> >>> https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c#L469
> >>
> >> Yes, and that's out of spec. Clearly v4l2-compliance doesn't test for this.
> >> It should have been caught at least for the mtk driver.
> >>
> >
> > Perhaps we should make it a part of the spec then?
> >
> > Actually I'm not really sure if we can say that this is out of spec
> > There has been no clear spec for the stateful codecs for many years,
> > with drivers doing wildly whatever they like and applications ending
> > up relying on those quirks.
>
> The VIDIOC_S_FMT spec for v4l2_pix_format is quite clear that it is the
> driver that sets this value. The spec for v4l2_plane_pix_format is
> unfortunately not so clear.

Yeah, it's clear in that it's set by the driver.

    Size in bytes of the buffer to hold a complete image, set by the driver.
    Usually this is bytesperline times height. When the image consists of
    variable length compressed data this is the maximum number of bytes required
    to hold an image.

I'm not sure what "an image" means for bitstreams, though.

>
> > My spec actually attempts to incorporate what was decided on the
> > earlier summits, including what's in Kamil's slides, the drivers are
> > already doing and existing applications rely on. The sizeimage
> > handling is just a part of it.
> >
> >>>
> >>> Also, Chromium has been setting the size on its own for long time
> >>> using its own heuristics.
> >>>
> >>>>
> >>>> And looking at the venus patch I do not see how it helps userspace.
> >>>>
> >>>> Regards,
> >>>>
> >>>>         Hans
> >>>>
> >>>>>
> >>>>>>
> >>>>>> If you need to allocate larger buffers, then use VIDIOC_CREATE_BUFS instead
> >>>>>> of VIDIOC_REQBUFS.
> >>>
> >>> CREATE_BUFS wouldn't work, because one needs to use TRY_FMT to obtain
> >>> a format for it and the format returned by it would have the sizeimage
> >>> as hardcoded in the driver...
> >>
> >> ???
> >>
> >> Userspace can change the sizeimage to whatever it wants before calling
> >> CREATE_BUFS as long as it is >= the sizeimage of the current format.
> >>
> >> If we want to allow smaller sizes, then I think that would not be
> >> unreasonable for stateful codecs. I actually think that drivers can
> >> already do this in queue_setup(), but the spec would have to be updated
> >> a bit.
> >
> > Existing applications rely on REQBUFS honoring the size they set in
> > sizeimage, though...
>
> REQBUFS, yes. But not CREATE_BUFS. Which is why that ioctl was added in the
> first place.
>
> However (and now I remember the real problem with CREATE_BUFS) the spec for
> CREATE_BUFS says that it will not change the provided sizeimage value. So
> any adjustments required due to specific alignment requirements won't be
> applied, all CREATE_BUFS can do is to reject it.
>
> So what this boils down to is a change to the spec:
>
> For compressed formats (and only those!) userspace can set sizeimage to a
> proposed value. The driver may either ignore it and just set its own value,
> or modify it to satisfy HW requirements. The returned value will be used
> by REQBUFS when it allocates buffers.
>
> I think this is reasonable, provided the spec is updated accordingly.
>
> As far as I can tell this shouldn't cause any backwards compatibility
> problems, and it should be easy to test in v4l2-compliance.

Thanks, I think that's exactly what we needed here. IMHO it doesn't
actually change the spec, just codify what has been passed verbally
for the last few years.

I'll add it to the descriptions of v4l2_pix_format and
v4l2_plane_pix_format structs, in the next version of my documentation
patches.

Best regards,
Tomasz

Re: [PATCH v3] media: venus: amend buffer size for bitstream plane

[Hans Verkuil]

On 11/27/2018 09:16 AM, Alexandre Courbot wrote:
> Hi Hans,
> 
> On Tue, Nov 27, 2018 at 1:41 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 11/26/2018 05:07 PM, Tomasz Figa wrote:
>>> On Tue, Nov 27, 2018 at 1:00 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>
>>>> On 11/26/2018 04:44 PM, Tomasz Figa wrote:
>>>>> Hi Hans,
>>>>>
>>>>> On Tue, Nov 27, 2018 at 12:24 AM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>>>>
>>>>>> On 11/26/2018 03:57 PM, Stanimir Varbanov wrote:
>>>>>>> Hi Hans,
>>>>>>>
>>>>>>> On 11/26/18 3:37 PM, Hans Verkuil wrote:
>>>>>>>> On 11/26/2018 11:12 AM, Malathi Gottam wrote:
>>>>>>>>> Accept the buffer size requested by client and compare it
>>>>>>>>> against driver calculated size and set the maximum to
>>>>>>>>> bitstream plane.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Malathi Gottam <mgottam@codeaurora.org>
>>>>>>>>
>>>>>>>> Sorry, this isn't allowed. It is the driver that sets the sizeimage value,
>>>>>>>> never the other way around.
>>>>>>>
>>>>>>> I think for decoders (OUTPUT queue) and encoders (CAPTURE queue) we
>>>>>>> allowed userspace to set sizeimage for buffers. See [1] Initialization
>>>>>>> paragraph point 2:
>>>>>>>
>>>>>>>     ``sizeimage``
>>>>>>>        desired size of ``CAPTURE`` buffers; the encoder may adjust it to
>>>>>>>        match hardware requirements
>>>>>>>
>>>>>>> Similar patch we be needed for decoder as well.
>>>>>>
>>>>>> I may have missed that change since it wasn't present in v1 of the stateful
>>>>>> encoder spec.
>>>>>
>>>>> It's been there from the very beginning, even before I started working
>>>>> on it. Actually, even the early slides from Kamil mention the
>>>>> application setting the buffer size for compressed streams:
>>>>> https://events.static.linuxfound.org/images/stories/pdf/lceu2012_debski.pdf
>>>>>
>>>>>>
>>>>>> Tomasz, what was the reason for this change? I vaguely remember some thread
>>>>>> about this, but I forgot the details. Since this would be a departure of
>>>>>> the current API this should be explained in more detail.
>>>>>
>>>>> The kernel is not the place to encode assumptions about optimal
>>>>> bitstream chunk sizes. It depends on the use case and the application
>>>>> should be able decide. It may for example want to use smaller buffers,
>>>>> optimizing for the well compressible video streams and just reallocate
>>>>> if bigger chunks are needed.
>>>>>
>>>>>>
>>>>>> I don't really see the point of requiring userspace to fill this in. For
>>>>>> stateful codecs it can just return some reasonable size. Possibly calculated
>>>>>> using the provided width/height values or (if those are 0) some default value.
>>>>>
>>>>> How do we decide what's "reasonable"? Would it be reasonable for all
>>>>> applications?
>>>>
>>>> In theory it should be the minimum size that the hardware supports. But it is
>>>> silly to i.e. provide the size of one PAGE as the minimum. In practice you
>>>> probably want to set sizeimage to something larger than that. Depending on
>>>> the typical compression ratio perhaps 5 or 10% of what a raw YUV 4:2:0 frame
>>>> would be.
>>>>
>>>>>
>>>>>>
>>>>>> Ditto for decoders.
>>>>>>
>>>>>> Stanimir, I certainly cannot merge this until this has been fully nailed down
>>>>>> as it would be a departure from the current API.
>>>>>
>>>>> It would not be a departure, because I can see existing stateful
>>>>> drivers behaving like that:
>>>>> https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c#L1444
>>>>> https://elixir.bootlin.com/linux/v4.20-rc4/source/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c#L469
>>>>
>>>> Yes, and that's out of spec. Clearly v4l2-compliance doesn't test for this.
>>>> It should have been caught at least for the mtk driver.
>>>>
>>>
>>> Perhaps we should make it a part of the spec then?
>>>
>>> Actually I'm not really sure if we can say that this is out of spec
>>> There has been no clear spec for the stateful codecs for many years,
>>> with drivers doing wildly whatever they like and applications ending
>>> up relying on those quirks.
>>
>> The VIDIOC_S_FMT spec for v4l2_pix_format is quite clear that it is the
>> driver that sets this value. The spec for v4l2_plane_pix_format is
>> unfortunately not so clear.
>>
>>> My spec actually attempts to incorporate what was decided on the
>>> earlier summits, including what's in Kamil's slides, the drivers are
>>> already doing and existing applications rely on. The sizeimage
>>> handling is just a part of it.
>>>
>>>>>
>>>>> Also, Chromium has been setting the size on its own for long time
>>>>> using its own heuristics.
>>>>>
>>>>>>
>>>>>> And looking at the venus patch I do not see how it helps userspace.
>>>>>>
>>>>>> Regards,
>>>>>>
>>>>>>         Hans
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> If you need to allocate larger buffers, then use VIDIOC_CREATE_BUFS instead
>>>>>>>> of VIDIOC_REQBUFS.
>>>>>
>>>>> CREATE_BUFS wouldn't work, because one needs to use TRY_FMT to obtain
>>>>> a format for it and the format returned by it would have the sizeimage
>>>>> as hardcoded in the driver...
>>>>
>>>> ???
>>>>
>>>> Userspace can change the sizeimage to whatever it wants before calling
>>>> CREATE_BUFS as long as it is >= the sizeimage of the current format.
>>>>
>>>> If we want to allow smaller sizes, then I think that would not be
>>>> unreasonable for stateful codecs. I actually think that drivers can
>>>> already do this in queue_setup(), but the spec would have to be updated
>>>> a bit.
>>>
>>> Existing applications rely on REQBUFS honoring the size they set in
>>> sizeimage, though...
>>
>> REQBUFS, yes. But not CREATE_BUFS. Which is why that ioctl was added in the
>> first place.
>>
>> However (and now I remember the real problem with CREATE_BUFS) the spec for
>> CREATE_BUFS says that it will not change the provided sizeimage value. So
>> any adjustments required due to specific alignment requirements won't be
>> applied, all CREATE_BUFS can do is to reject it.
>>
>> So what this boils down to is a change to the spec:
>>
>> For compressed formats (and only those!) userspace can set sizeimage to a
>> proposed value. The driver may either ignore it and just set its own value,
>> or modify it to satisfy HW requirements. The returned value will be used
>> by REQBUFS when it allocates buffers.
>>
>> I think this is reasonable, provided the spec is updated accordingly.
>>
>> As far as I can tell this shouldn't cause any backwards compatibility
>> problems, and it should be easy to test in v4l2-compliance.
> 
> Do you mean that this patch is acceptable provided the stateful codec
> specification is updated accordingly?

Yes. Although I would update the spec in a separate patch. It is not really
part of the codec spec as such, more a prerequisite.

Regards,

	Hans

> For our (Chromium) needs this seems to do the job, so:
> 
> Tested-by: Alexandre Courbot <acourbot@chromium.org>
> 
> Although I would like to also have the equivalent for the decoder's
> OUTPUT queue, either as a v4 or a follow-up patch.
>