On 5/10/19 1:28 PM, Laurent Pinchart wrote:
> Hi Hans,
>
> (CC'ing Kieran)
>
> 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 <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.
>
> 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.
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);
>
