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.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS,T_DKIMWL_WL_HIGH,URIBL_BLOCKED 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 74C89C28CC0 for ; Wed, 29 May 2019 14:02:46 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 44C1C23406 for ; Wed, 29 May 2019 14:02:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1559138566; bh=F++wGo2ho/0IgxrLXKehVMGTgbbLao+2mx4BROEzAbI=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=pev73YLIc+FU9rRBaJdXbwZs+LamVNm834jySAIK20/gUHmUxqtClzOqfRQh/61ge QezVWVCM4XPL374oXXQYFP6VXxN5hulM4JJklC5buIFKhV0rqYVaNLsHxbw10/zs8r 1sTSK4ULJ1UKXmex/UmIOrC6WRY8FayqPyiaG4y4= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727471AbfE2OCp (ORCPT ); Wed, 29 May 2019 10:02:45 -0400 Received: from casper.infradead.org ([85.118.1.10]:51628 "EHLO casper.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727445AbfE2OCo (ORCPT ); Wed, 29 May 2019 10:02:44 -0400 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=Content-Transfer-Encoding:Content-Type: MIME-Version:References:In-Reply-To:Message-ID:Subject:Cc:To:From:Date:Sender :Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help: List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=DzWIwTHRXYowZ370y2QK4BpFgTP2UTXGBlIiUm4zAP4=; b=f6dyF6mLf9voR8Rq30jhEZGUQR 4Z1xXXTdUW2Yb0vWwej9nxCrRWliRCVOUIdB6vTEsB2ebjIRweAyQJfo5K9AmAkTX86HB8x4uR2N3 1TMLhl6gd5w+m7JXDeqn6V4S3rkpwjylMEOMmFvsjyuByKfDjgU6JRD/zhIZxspdLlPWmQG5mbwk1 HoCUCLxKhKcTAerDKizfZPcT07p1vueKKS6wkmN+vYwXJRSYxFG0bNQAUBom4Hd0OjBHFGU/JdR1q PvUS6/TYkMh0LsBEwmpkHC/fWDNEB2IvnoYU5ORIPgKe8ATCGObMoPpFASCX3npzWiyav+lUz5I42 /ky/ksQw==; Received: from 177.132.232.81.dynamic.adsl.gvt.net.br ([177.132.232.81] helo=coco.lan) by casper.infradead.org with esmtpsa (Exim 4.90_1 #2 (Red Hat Linux)) id 1hVzA7-0001ls-LQ; Wed, 29 May 2019 14:02:40 +0000 Date: Wed, 29 May 2019 11:02:33 -0300 From: Mauro Carvalho Chehab To: Nicolas Dufresne Cc: Ezequiel Garcia , linux-media@vger.kernel.org, Hans Verkuil , kernel@collabora.com, Tomasz Figa , linux-rockchip@lists.infradead.org, Heiko Stuebner , Jonas Karlman , Philipp Zabel , Boris Brezillon Subject: Re: [PATCH v6 03/16] media: v4l2-common: Support custom imagesize in fill_pixfmt() Message-ID: <20190529110233.0ff81aff@coco.lan> In-Reply-To: <4ef64e22f4035fccf09fbb7f0be04a9778b1230d.camel@collabora.com> References: <20190528170232.2091-1-ezequiel@collabora.com> <20190528170232.2091-4-ezequiel@collabora.com> <20190529082809.0b9f3553@coco.lan> <4ef64e22f4035fccf09fbb7f0be04a9778b1230d.camel@collabora.com> 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=UTF-8 Content-Transfer-Encoding: quoted-printable Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-media@vger.kernel.org Em Wed, 29 May 2019 09:54:09 -0400 Nicolas Dufresne escreveu: > Hi Mauro, >=20 > Le mercredi 29 mai 2019 =C3=A0 08:28 -0300, Mauro Carvalho Chehab a =C3= =A9crit : > > Em Tue, 28 May 2019 14:02:19 -0300 > > Ezequiel Garcia escreveu: > >=20 > > > From: Boris Brezillon > > >=20 > > > 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. > > >=20 > > > We could possibly do the same for bytesperline, but it gets tricky wh= en > > > dealing with !MPLANE definitions, so this case is omitted for now and > > > ->bytesperline is always overwritten with the value calculated in > > > fill_pixfmt(). > > >=20 > > > Signed-off-by: Boris Brezillon > > > --- > > > Changes from v5: > > > * Overwrite bytesperline with the value calculated in fill_pixfmt() > > >=20 > > > Changes from v4: > > > * New patch > > >=20 > > > drivers/media/v4l2-core/v4l2-common.c | 58 ++++++++++++++++++++-----= -- > > > 1 file changed, 43 insertions(+), 15 deletions(-) > > >=20 > > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4= l2-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_mp= lane *pixfmt, > > > pixfmt->num_planes =3D info->mem_planes; > > > =20 > > > if (info->mem_planes =3D=3D 1) { > > > + u32 sizeimage =3D 0; > > > + > > > plane =3D &pixfmt->plane_fmt[0]; > > > - plane->bytesperline =3D ALIGN(width, v4l2_format_block_width(info,= 0)) * info->bpp[0]; > > > - plane->sizeimage =3D 0; > > > =20 > > > for (i =3D 0; i < info->comp_planes; i++) { > > > unsigned int hdiv =3D (i =3D=3D 0) ? 1 : info->hdiv; > > > @@ -598,10 +598,21 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_= mplane *pixfmt, > > > aligned_width =3D ALIGN(width, v4l2_format_block_width(info, i)); > > > aligned_height =3D ALIGN(height, v4l2_format_block_height(info, i= )); > > > =20 > > > - plane->sizeimage +=3D info->bpp[i] * > > > - DIV_ROUND_UP(aligned_width, hdiv) * > > > - DIV_ROUND_UP(aligned_height, vdiv); > > > + sizeimage +=3D info->bpp[i] * > > > + DIV_ROUND_UP(aligned_width, hdiv) * > > > + DIV_ROUND_UP(aligned_height, vdiv); > > > } > > > + > > > + /* Custom bytesperline value is not supported yet. */ > > > + plane->bytesperline =3D 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 =3D max(sizeimage, plane->sizeimage); > >=20 > > No upper limit? That doesn't sound a good idea to me, specially since s= ome > > (broken) app might not be memset the format to zero before filling the = ioctl > > structure. > >=20 > > Perhaps we could do something like: > >=20 > > sizeimage =3D min (sizeimage, 2 * plane->sizeimage) > >=20 > > or something similar that would be reasonable. > >=20 > > > } else { > > > for (i =3D 0; i < info->comp_planes; i++) { > > > unsigned int hdiv =3D (i =3D=3D 0) ? 1 : info->hdiv; > > > @@ -613,10 +624,19 @@ int v4l2_fill_pixfmt_mp(struct v4l2_pix_format_= mplane *pixfmt, > > > aligned_height =3D ALIGN(height, v4l2_format_block_height(info, i= )); > > > =20 > > > plane =3D &pixfmt->plane_fmt[i]; > > > - plane->bytesperline =3D > > > - info->bpp[i] * DIV_ROUND_UP(aligned_width, hdiv); > > > - plane->sizeimage =3D > > > - plane->bytesperline * DIV_ROUND_UP(aligned_height, vdiv); > > > + > > > + /* Custom bytesperline value is not supported yet. */ > >=20 > > Supporting custom bytesperline seems too risky of breaking apps.=20 > > So, I would drop this comment. >=20 > We will really need this in the long run in many drivers in order to > allow import/export of DMABuf. Without such adaptive feature, we have a > software limitation that forces bouncing memory. I have already > discussed about adding this feature notably in vivid and uvcvideo on > IRC and in conference, which both have no restriction the memory > alignment, so should allow importing any kind of video layout. >=20 > We already have a partial userspace implementation for this in > GStreamer and upstream driver submission should come when the IP is > considered stable enough. I understand the need. I'm not against an implementation for such feature, provided that it won't break anything. I guess one of the things we miss at V4L2 API is an indication from userspace about what it supports. I mean, just like we have the caps flags where the Kernel reports what it supports, we could have a similar "userspace caps" field. > Why I think it won't break userspace is that the correct way to use > these read-only members of V4L2 struct is to set these to 0, which is > also documented. Yeah, the apps I'm aware of usually call memset() before filling V4L2 structs. On those, adding this behavior would be ok. Yet, I'm not sure if 100% of (open source) apps do that. > Adding upper bound seems like a good idea though. Agreed. >=20 > Thanks, Mauro