On 5/3/19 1:47 PM, Boris Brezillon wrote:
> Users can define custom sizeimage and bytesperline 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.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> ---
> Hello Hans,
>
> The sizeimage/bytesperline check on !MPLANE formats is still not 100%
> sure, as custom bytesperline might induce bigger sizeimage than what
> we calculate.
>
> I tried implementing something smarter taking the per-component plane
> bpp + hdiv param as we discussed the other day but decided to step
> back after realizing the per-component plane macro block might also
> differ at least in theory (not sure that's true in practice) and that
> has an impact on bytesperline too.
>
> Let me know how you want to handle that case.
>
> Regards,
>
> Boris
>
> Changes in v5:
> * New patch
> ---
> drivers/media/v4l2-core/v4l2-common.c | 54 +++++++++++++++++++--------
> 1 file changed, 39 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> index 3c6f5c115fc5..37bfc984a8b5 100644
> --- a/drivers/media/v4l2-core/v4l2-common.c
> +++ b/drivers/media/v4l2-core/v4l2-common.c
> @@ -563,9 +563,10 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_mplane *pixfmt,
> pixfmt->num_planes = info->mem_planes;
>
> if (info->mem_planes == 1) {
> + u32 bytesperline, sizeimage = 0;
> +
> plane = &pixfmt->plane_fmt[0];
> - plane->bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
> - plane->sizeimage = 0;
> + bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
>
> for (i = 0; i < info->comp_planes; i++) {
> unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> @@ -576,10 +577,17 @@ 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);
> }
> +
> + /*
> + * The user might have specified custom sizeimage/bytesperline,
> + * only override them if they're not big enough.
> + */
> + plane->sizeimage = max(sizeimage, plane->sizeimage);
> + plane->bytesperline = max(bytesperline, plane->bytesperline);
Let's just set bytesperline, ignoring the value the user supplied. There are very
few drivers that allow the user to set bytesperline anyway, so it's not a big deal
to drop support for that for now. We can add it back later.
Just add a comment that a user-defined bytesperline value isn't currently supported.
Regards,
Hans
> } else {
> for (i = 0; i < info->comp_planes; i++) {
> unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> @@ -591,10 +599,20 @@ 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);
> +
> + /*
> + * The user might have specified custom
> + * sizeimage/bytesperline, only override them if
> + * they're not big enough.
> + */
> + plane->bytesperline = max_t(u32,
> + info->bpp[i] *
> + DIV_ROUND_UP(aligned_width, hdiv),
> + plane->bytesperline);
> + plane->sizeimage = max_t(u32,
> + plane->bytesperline *
> + DIV_ROUND_UP(aligned_height, vdiv),
> + plane->sizeimage);
> }
> }
> return 0;
> @@ -605,6 +623,7 @@ int v4l2_fill_pixfmt(struct v4l2_pix_format *pixfmt, u32 pixelformat,
> u32 width, u32 height)
> {
> const struct v4l2_format_info *info;
> + u32 bytesperline, sizeimage = 0;
> int i;
>
> info = v4l2_format_info(pixelformat);
> @@ -618,8 +637,7 @@ 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;
> + bytesperline = ALIGN(width, v4l2_format_block_width(info, 0)) * info->bpp[0];
>
> for (i = 0; i < info->comp_planes; i++) {
> unsigned int hdiv = (i == 0) ? 1 : info->hdiv;
> @@ -629,11 +647,17 @@ 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);
> }
> +
> + /*
> + * The user might have specified its own sizeimage/bytesperline values,
> + * only override them if they're not big enough.
> + */
> + pixfmt->sizeimage = max(sizeimage, pixfmt->sizeimage);
> + pixfmt->bytesperline = max(bytesperline, pixfmt->bytesperline);
> +
> return 0;
> }
> EXPORT_SYMBOL_GPL(v4l2_fill_pixfmt);
>
