On Wed, 29 May 2019 14:31:03 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> wrote:
> On 5/29/19 2:16 PM, Boris Brezillon wrote:
> > On Wed, 29 May 2019 08:58:54 -0300
> > Mauro Carvalho Chehab <mchehab+samsung@xxxxxxxxxx> wrote:
> >
> >> 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.
> >
> > If we want max_sizeimage to be driver specific I can add it as an extra
> > arg to the fill_pixfmt() funcs.
>
> Looking more closely, only compressed formats can accept a user-specified
> sizeimage value, and this function is only called for uncompressed formats.
>
> So doesn't that mean that this sizeimage override code can be dropped?
Hehe, IIRC, you were the one asking for this :P (or maybe I
misunderstood what you suggested). I don't think we need to preserve
user-defined ->sizeimage for uncompressed fmt in the VPU driver, so I'm
perfectly fine implementing a version that overrides ->sizeimage
unconditionally.
