On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote:
> Em Tue, 28 May 2019 14:02:19 -0300
> Ezequiel Garcia <ezequ...@collabora.com> escreveu:
>
>> From: Boris Brezillon <boris.brezil...@collabora.com>
>>
>> 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.brezil...@collabora.com>
>> ---
>> 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.
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
>
