Em Wed, 29 May 2019 13:43:20 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote:
> > Em Tue, 28 May 2019 14:02:19 -0300
> > Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> escreveu:
> >
> >> From: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> >>
> >> Users can define custom sizeimage as long as they're big enough to
> >> store the amount of pixels required for a specific width/height under a
> >> specific format. Avoid overriding those fields in this case.
> >>
> >> We could possibly do the same for bytesperline, but it gets tricky when
> >> dealing with !MPLANE definitions, so this case is omitted for now and
> >> ->bytesperline is always overwritten with the value calculated in
> >> fill_pixfmt().
> >>
> >> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> >> ---
> >> Changes from v5:
> >> * Overwrite bytesperline with the value calculated in fill_pixfmt()
> >>
> >> Changes from v4:
> >> * New patch
> >>
> >> drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-------
> >> 1 file changed, 43 insertions(+), 15 deletions(-)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> >> index b2d1e55d9561..fd286f6e17d7 100644
> >> --- a/drivers/media/v4l2-core/v4l2-common.c
> >> +++ b/drivers/media/v4l2-core/v4l2-common.c
> >> @@ -585,9 +585,9 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
> >> pixfmt->num_planes = info->mem_planes;
> >>
> >> if (info->mem_planes == 1) {
> >> + u32 sizeimage = 0;
> >> +
> >> plane = &pixfmt->plane_fmt[0];
> >> - plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
> >> - plane->sizeimage = 0;
> >>
> >> for (i = 0; i < info->comp_planes; i++) {
> >> unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> >> @@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
> >> aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
> >> aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> >>
> >> - plane->sizeimage += info->bpp[i] *
> >> - DIV_ROUND_UP(aligned_width, hdiv) *
> >> - DIV_ROUND_UP(aligned_height, vdiv);
> >> + sizeimage += info->bpp[i] *
> >> + DIV_ROUND_UP(aligned_width, hdiv) *
> >> + DIV_ROUND_UP(aligned_height, vdiv);
> >> }
> >> +
> >> + /* Custom bytesperline value is not supported yet. */
> >> + plane->bytesperline = ALIGN(width,
> >> + v4l2_format_block_width(info, 0)) *
> >> + info->bpp[0];
> >> +
> >> + /*
> >> + * The user might have specified a custom sizeimage, only
> >> + * override it if it's not big enough.
> >> + */
> >> + plane->sizeimage = max(sizeimage, plane->sizeimage);
> >
> > No upper limit? That doesn't sound a good idea to me, specially since some
> > (broken) app might not be memset the format to zero before filling the ioctl
> > structure.
> >
> > Perhaps we could do something like:
> >
> > sizeimage = min (sizeimage, 2 * plane->sizeimage)
> >
> > or something similar that would be reasonable.
>
> I've no idea what's sane.
>
> Buffers can be really large. The largest video resolution defined by CTA-861-G
> is 10240x4320, so at 4 bytes per pixel that's 0x0a8c0000. So perhaps we can
> use min(sizeimage, 0x10000000)? Although we should probably use the clamp function
> instead of min/max.
Well, the max is driver-specific.
For example, for a camera with a max resolution of 640x480 with 2 bytes
per pixel as the max format can only be
max_size = 640*480*2 (plus some alignment value if pertinent)
It sounds to me that the best would be to have a callback function
or value filled by the drivers that would support custom sizeimage.
The core could actually calculate during init (by asking the driver
to a very big resolution and getting the returned value), but
it sounds better to let the drivers to explicitly calculate it.
>
> Regards,
>
> Hans
>
> >
> >> } else {
> >> for (i = 0; i < info->comp_planes; i++) {
> >> unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> >> @@ -613,10 +624,19 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
> >> aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> >>
> >> plane = &pixfmt->plane_fmt[i];
> >> - plane->bytesperline =
> >> - info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv);
> >> - plane->sizeimage =
> >> - plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv);
> >
> >> +
> >> + /* Custom bytesperline value is not supported yet. */
> >
> > Supporting custom bytesperline seems too risky of breaking apps.
> > So, I would drop this comment.
> >
> >
> >> + plane->bytesperline = info->bpp[i] *
> >> + DIV_ROUND_UP(aligned_width, hdiv);
> >
> >> +
> >> + /*
> >> + * The user might have specified a custom sizeimage,
> >> + * only override it if it's not big enough.
> >> + */
> >> + plane->sizeimage = max_t(u32,
> >> + plane->bytesperline *
> >> + DIV_ROUND_UP(aligned_height, vdiv),
> >> + plane->sizeimage);
> >> }
> >> }
> >> return 0;
> >> @@ -627,6 +647,7 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
> >> u32 width, u32 height)
> >> {
> >> const struct v4l2_format_info *info;
> >> + u32 sizeimage = 0;
> >> int i;
> >>
> >> info = v4l2_format_info(pixelformat);
> >> @@ -640,8 +661,6 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
> >> pixfmt->width = width;
> >> pixfmt->height = height;
> >> pixfmt->pixelformat = pixelformat;
> >> - pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
> >> - pixfmt->sizeimage = 0;
> >>
> >> for (i = 0; i < info->comp_planes; i++) {
> >> unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> >> @@ -651,11 +670,20 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
> >>
> >> aligned_width = ALIGN(width, v4l2_format_block_width(info, i));
> >> aligned_height = ALIGN(height, v4l2_format_block_height(info, i));
> >> -
> >> - pixfmt->sizeimage += info->bpp[i] *
> >> - DIV_ROUND_UP(aligned_width, hdiv) *
> >> - DIV_ROUND_UP(aligned_height, vdiv);
> >> + sizeimage += info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv) *
> >> + DIV_ROUND_UP(aligned_height, vdiv);
> >> }
> >> +
> >> + /* Custom bytesperline value is not supported yet. */
> >> + pixfmt->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) *
> >> + info->bpp[0];
> >> +
> >> + /*
> >> + * The user might have specified its own sizeimage value, only override
> >> + * it if it's not big enough.
> >> + */
> >> + pixfmt->sizeimage = max(sizeimage, pixfmt->sizeimage);
> >> +
> >
> > Same comment applies here: We need to sanitize it from too big sizeimages.
> >
> >> return 0;
> >> }
> >> EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
> >
> >
> >
> > Thanks,
> > Mauro
> >
>
Thanks,
Mauro
