From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Subject: Re: [PATCH v6 03/16] media: v4l2-common: Support custom imagesize in fill_pixfmt() Date: Wed, 29 May 2019 14:39:53 +0200 Message-ID: <20190529143953.50018efa@collabora.com> References: <20190528170232.2091-1-ezequiel@collabora.com> <20190528170232.2091-4-ezequiel@collabora.com> <20190529082809.0b9f3553@coco.lan> <20190529085854.6c689f8d@coco.lan> <20190529141633.19c5079b@collabora.com> <223a757d-f008-398c-64ce-18b0aaf390c5@xs4all.nl> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <223a757d-f008-398c-64ce-18b0aaf390c5-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+glpar-linux-rockchip=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Hans Verkuil Cc: Nicolas Dufresne , Heiko Stuebner , Jonas Karlman , Tomasz Figa , linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Hans Verkuil , Philipp Zabel , Mauro Carvalho Chehab , kernel-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org, Ezequiel Garcia , linux-media-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-rockchip.vger.kernel.org On Wed, 29 May 2019 14:31:03 +0200 Hans Verkuil wrote: > On 5/29/19 2:16 PM, Boris Brezillon wrote: > > On Wed, 29 May 2019 08:58:54 -0300 > > Mauro Carvalho Chehab wrote: > > > >> Em Wed, 29 May 2019 13:43:20 +0200 > >> Hans Verkuil escreveu: > >> > >>> On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote: > >>>> Em Tue, 28 May 2019 14:02:19 -0300 > >>>> Ezequiel Garcia escreveu: > >>>> > >>>>> From: Boris Brezillon > >>>>> > >>>>> 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 > >>>>> --- > >>>>> 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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.0 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5A38AC28CC0 for ; Wed, 29 May 2019 12:40:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 38D2B208C3 for ; Wed, 29 May 2019 12:40:00 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727146AbfE2Mj7 (ORCPT ); Wed, 29 May 2019 08:39:59 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:52640 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726779AbfE2Mj6 (ORCPT ); Wed, 29 May 2019 08:39:58 -0400 Received: from localhost (unknown [IPv6:2a01:e0a:2c:6930:5cf4:84a1:2763:fe0d]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: bbrezillon) by bhuna.collabora.co.uk (Postfix) with ESMTPSA id 728F0260CA6; Wed, 29 May 2019 13:39:56 +0100 (BST) Date: Wed, 29 May 2019 14:39:53 +0200 From: Boris Brezillon To: Hans Verkuil Cc: Mauro Carvalho Chehab , Ezequiel Garcia , linux-media@vger.kernel.org, Hans Verkuil , kernel@collabora.com, Nicolas Dufresne , Tomasz Figa , linux-rockchip@lists.infradead.org, Heiko Stuebner , Jonas Karlman , Philipp Zabel Subject: Re: [PATCH v6 03/16] media: v4l2-common: Support custom imagesize in fill_pixfmt() Message-ID: <20190529143953.50018efa@collabora.com> In-Reply-To: <223a757d-f008-398c-64ce-18b0aaf390c5@xs4all.nl> References: <20190528170232.2091-1-ezequiel@collabora.com> <20190528170232.2091-4-ezequiel@collabora.com> <20190529082809.0b9f3553@coco.lan> <20190529085854.6c689f8d@coco.lan> <20190529141633.19c5079b@collabora.com> <223a757d-f008-398c-64ce-18b0aaf390c5@xs4all.nl> Organization: Collabora X-Mailer: Claws Mail 3.17.3 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org On Wed, 29 May 2019 14:31:03 +0200 Hans Verkuil wrote: > On 5/29/19 2:16 PM, Boris Brezillon wrote: > > On Wed, 29 May 2019 08:58:54 -0300 > > Mauro Carvalho Chehab wrote: > > > >> Em Wed, 29 May 2019 13:43:20 +0200 > >> Hans Verkuil escreveu: > >> > >>> On 5/29/19 1:28 PM, Mauro Carvalho Chehab wrote: > >>>> Em Tue, 28 May 2019 14:02:19 -0300 > >>>> Ezequiel Garcia escreveu: > >>>> > >>>>> From: Boris Brezillon > >>>>> > >>>>> 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 > >>>>> --- > >>>>> 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.