media: venus: amend buffer size for bitstream plane

PATCH Superseded Malathi Gottam 4 participants 12 messages Oct 9, 2018 – Nov 16, 2018

Subsystems (4)

Media (V4L/DVB) Arm Qualcomm mailing list Qualcomm media platform Qualcomm venus video accelerator driver

lore

Share & tools

lore

Malathi Gottam

author

2018-10-09 07:52 UTC #0 lore raw

PATCH 1/1 media: venus: amend buffer size for bitstream plane

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 <[email protected]>

---
 drivers/media/platform/qcom/venus/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/media/platform/qcom/venus/helpers.c +1 -1

index 2679adb..05c5423 100644
@@ -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);
 	}

Signature · 1 line

1.9.1

Tomasz Figa

2018-10-23 02:50 UTC #2 lore raw

In reply to Malathi Gottam

Hi Malathi,

Quoted 15 lines · Malathi Gottam

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 <[email protected]>
---
 drivers/media/platform/qcom/venus/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/platform/qcom/venus/helpers.c +0 -0
index 2679adb..05c5423 100644

drivers/media/platform/qcom/venus/helpers.c u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 width, u32 height) · line 654

Quoted 7 lines

drivers/media/platform/qcom/venus/helpers.c +1 -1

@@ -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

Stanimir Varbanov

2018-11-12 12:34 UTC #3 lore raw

Hi Tomasz,

Quoted 20 lines · Tomasz Figa

Hi Malathi,
Quoted 17 lines · Malathi Gottam
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 <[email protected]>
---
 drivers/media/platform/qcom/venus/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/platform/qcom/venus/helpers.c +0 -0
index 2679adb..05c5423 100644
@@ -649,7 +649,7 @@ u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 width, u32 height)
        }
 

Quoted 9 lines

drivers/media/platform/qcom/venus/helpers.c u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 width, u32 height) · line 654
Quoted 5 lines
drivers/media/platform/qcom/venus/helpers.c +1 -1
        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).

Signature · 2 lines

regards,
Stan

[email protected]

author

2018-11-13 07:28 UTC #4 lore raw

Quoted 35 lines · Stanimir Varbanov

Hi Tomasz,
Quoted 21 lines · Tomasz Figa
Hi Malathi,
Quoted 18 lines · Malathi Gottam
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 <[email protected]>
---
 drivers/media/platform/qcom/venus/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/platform/qcom/venus/helpers.c +0 -0
index 2679adb..05c5423 100644
@@ -649,7 +649,7 @@ u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 
 width, u32 height)
        }
 
Quoted 10 lines
Quoted 6 lines
drivers/media/platform/qcom/venus/helpers.c +1 -1
        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.

Quoted 2 lines

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.

Stanimir Varbanov

2018-11-13 08:12 UTC #5 lore raw

Hi Malathi,

Quoted 37 lines · On 11/13/18 9:28 AM, [email protected] wrote:

Quoted 36 lines · Stanimir Varbanov
Hi Tomasz,
Quoted 32 lines · Tomasz Figa
Hi Malathi,
Quoted 16 lines · Malathi Gottam
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 <[email protected]>
---
drivers/media/platform/qcom/venus/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/platform/qcom/venus/helpers.c +0 -0
index 2679adb..05c5423 100644
drivers/media/platform/qcom/venus/helpers.c u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 · line 655
Quoted 8 lines
drivers/media/platform/qcom/venus/helpers.c +1 -1
@@ -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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 7 lines

If so should we set the inst->input_buf_size here in venc_s_fmt?

(quoted diff) +0 -0

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

Review static const struct venus_format * · line 342

Quoted 10 lines

(quoted diff) +4 -4

-
-       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.

Quoted 27 lines

if (f->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) pfmt[0].bytesperline = ALIGN(pixmp->width, 128);

(quoted diff) +1 -0

@@ -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

(quoted diff) +2 -1

@@ -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.

Signature · 2 lines

regards,
Stan

Tomasz Figa

2018-11-13 09:13 UTC #6 lore raw

Quoted 73 lines · Stanimir Varbanov

Hi Malathi,
Quoted 37 lines · On 11/13/18 9:28 AM, [email protected] wrote:
Quoted 36 lines · Stanimir Varbanov
Hi Tomasz,
Quoted 32 lines · Tomasz Figa
Hi Malathi,
Quoted 16 lines · Malathi Gottam
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 <[email protected]>
---
 drivers/media/platform/qcom/venus/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/platform/qcom/venus/helpers.c +0 -0
index 2679adb..05c5423 100644
drivers/media/platform/qcom/venus/helpers.c u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 · line 655
Quoted 8 lines
drivers/media/platform/qcom/venus/helpers.c +1 -1
@@ -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.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 7 lines
If so should we set the inst->input_buf_size here in venc_s_fmt?
(quoted diff) +0 -0
@@ -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;
Review static const struct venus_format * · line 342
Quoted 10 lines
(quoted diff) +4 -4
-
-       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

Quoted 6 lines

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

Stanimir Varbanov

2018-11-13 10:46 UTC #7 lore raw

Hi Tomasz,

Quoted 77 lines · Tomasz Figa

Quoted 75 lines · Stanimir Varbanov
Hi Malathi,
Quoted 37 lines · On 11/13/18 9:28 AM, [email protected] wrote:
Quoted 36 lines · Stanimir Varbanov
Hi Tomasz,
Quoted 32 lines · Tomasz Figa
Hi Malathi,
Quoted 16 lines · Malathi Gottam
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 <[email protected]>
---
 drivers/media/platform/qcom/venus/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/platform/qcom/venus/helpers.c +0 -0
index 2679adb..05c5423 100644
drivers/media/platform/qcom/venus/helpers.c u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 · line 655
Quoted 8 lines
drivers/media/platform/qcom/venus/helpers.c +1 -1
@@ -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.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 7 lines
If so should we set the inst->input_buf_size here in venc_s_fmt?
(quoted diff) +0 -0
@@ -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;
Review static const struct venus_format * · line 342
Quoted 10 lines
(quoted diff) +4 -4
-
-       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)

Quoted 7 lines

Quoted 4 lines
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.

Quoted 6 lines

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.

Signature · 2 lines

regards,
Stan

Tomasz Figa

2018-11-14 03:51 UTC #8 lore raw

Quoted 92 lines · Stanimir Varbanov

Hi Tomasz,
Quoted 77 lines · Tomasz Figa
Quoted 75 lines · Stanimir Varbanov
Hi Malathi,
Quoted 37 lines · On 11/13/18 9:28 AM, [email protected] wrote:
Quoted 36 lines · Stanimir Varbanov
Hi Tomasz,
Quoted 32 lines · Tomasz Figa
Hi Malathi,
Quoted 16 lines · Malathi Gottam
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 <[email protected]>
---
 drivers/media/platform/qcom/venus/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/platform/qcom/venus/helpers.c +0 -0
index 2679adb..05c5423 100644
drivers/media/platform/qcom/venus/helpers.c u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 · line 655
Quoted 8 lines
drivers/media/platform/qcom/venus/helpers.c +1 -1
@@ -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.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 7 lines
If so should we set the inst->input_buf_size here in venc_s_fmt?
(quoted diff) +0 -0
@@ -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;
Review static const struct venus_format * · line 342
Quoted 10 lines
(quoted diff) +4 -4
-
-       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)
Quoted 7 lines
Quoted 4 lines
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.
Quoted 1 line

Quoted 8 lines

Quoted 5 lines
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

[email protected]

author

2018-11-16 04:34 UTC #9 lore raw

Quoted 85 lines · Tomasz Figa

Quoted 83 lines · Stanimir Varbanov
Hi Tomasz,
Quoted 79 lines · Tomasz Figa
Quoted 77 lines · Stanimir Varbanov
Hi Malathi,
Quoted 37 lines · On 11/13/18 9:28 AM, [email protected] wrote:
Quoted 36 lines · Stanimir Varbanov
Hi Tomasz,
Quoted 32 lines · Tomasz Figa
Hi Malathi,
Quoted 16 lines · Malathi Gottam
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 <[email protected]>
---
 drivers/media/platform/qcom/venus/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/platform/qcom/venus/helpers.c +0 -0
index 2679adb..05c5423 100644
drivers/media/platform/qcom/venus/helpers.c u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 · line 655
Quoted 8 lines
drivers/media/platform/qcom/venus/helpers.c +1 -1
@@ -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.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 7 lines
If so should we set the inst->input_buf_size here in venc_s_fmt?
(quoted diff) +0 -0
@@ -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;
Review static const struct venus_format * · line 342
Quoted 10 lines
(quoted diff) +4 -4
-
-       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

Quoted 7 lines

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
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;
+ }

Quoted 18 lines

Quoted 10 lines
Quoted 7 lines
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).

Quoted 2 lines

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.

Tomasz Figa

2018-11-16 06:02 UTC #10 lore raw

Quoted 98 lines · On Fri, Nov 16, 2018 at 1:35 PM <[email protected]> wrote:

Quoted 85 lines · Tomasz Figa
Quoted 83 lines · Stanimir Varbanov
Hi Tomasz,
Quoted 79 lines · Tomasz Figa
Quoted 77 lines · Stanimir Varbanov
Hi Malathi,
Quoted 37 lines · On 11/13/18 9:28 AM, [email protected] wrote:
Quoted 36 lines · Stanimir Varbanov
Hi Tomasz,
Quoted 32 lines · Tomasz Figa
Hi Malathi,
Quoted 16 lines · Malathi Gottam
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 <[email protected]>
---
 drivers/media/platform/qcom/venus/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/platform/qcom/venus/helpers.c +0 -0
index 2679adb..05c5423 100644
drivers/media/platform/qcom/venus/helpers.c u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 · line 655
Quoted 8 lines
drivers/media/platform/qcom/venus/helpers.c +1 -1
@@ -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.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 7 lines
If so should we set the inst->input_buf_size here in venc_s_fmt?
(quoted diff) +0 -0
@@ -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;
Review static const struct venus_format * · line 342
Quoted 10 lines
(quoted diff) +4 -4
-
-       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
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
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

Review static const struct venus_format * · line 337

Quoted 5 lines

(quoted diff) +1 -0

@@ -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.

Quoted 33 lines

(quoted diff) +4 -1
+       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;
+       }
 
 
Quoted 18 lines
Quoted 10 lines
Quoted 7 lines
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).

Quoted 7 lines

Quoted 2 lines
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

Stanimir Varbanov

2018-11-12 12:28 UTC #11 lore raw

In reply to Malathi Gottam

Hi Malathi,

Thanks for the patch!

Quoted 14 lines · Malathi Gottam

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 <[email protected]>
---
 drivers/media/platform/qcom/venus/helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/platform/qcom/venus/helpers.c +0 -0
index 2679adb..05c5423 100644

drivers/media/platform/qcom/venus/helpers.c u32 venus_helper_get_framesz(u32 v4l2_fmt, u32 width, u32 height) · line 652

Quoted 5 lines

drivers/media/platform/qcom/venus/helpers.c +1 -1

@@ -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.

Signature · 2 lines

regards,
Stan