On 5/10/19 2:24 PM, Laurent Pinchart wrote: > 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.
Ah, OK. But why not use CREATEBUFS? You can provide your own size when allocating the buffers. Also note this patch: https://patchwork.linuxtv.org/patch/55656/ Although this is specific for compressed formats. Regards, Hans > >>> 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); >
