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