Hi Hans,
On Fri, May 10, 2019 at 02:17:32PM +0200, Hans Verkuil wrote:
> On 5/10/19 1:28 PM, Laurent Pinchart wrote:
> > On Fri, May 10, 2019 at 10:57:26AM +0200, Hans Verkuil wrote:
> >> 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 <[email protected]>
> >>> ---
> >>> 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.
> >
> > Kieran recently ran into an issue related to this, when sharing buffers
> > between a CSI-2 receiver and an ISP. The ISP has alignment constraints
> > in both the horizontal and vertical directions on the line stride and
> > total image size. Out software framework currently allocates buffers
> > from the CSI-2 receiver which doesn't have those constraints, and not
> > being able to specify sizeimage is thus a problem.
>
> Not being able to specify sizeimage where? From userspace? Sorry, I don't
> quite understand the specific issue here.
Yes, from userspace.
> > We have worked around this by allocating buffers on the ISP side but
> > that departed from the normal way of operation, and it may not be a good
> > idea to carry that forward.
> >
> > I wonder how we should solve these issues long term. A central memory
> > allocator is probably the way to go.
> >
> >>> } 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);
--
Regards,
Laurent Pinchart
