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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 562D6C6FD18 for ; Wed, 19 Apr 2023 07:02:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Date:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=tyHuVmv/YmE0ohw6gZJip2b/Z/+UqIT/8tPsTl6bp+E=; b=U+/6R++2aY1Hmm ykQKDHm9qA2Ing7YocqsvLnCi68Mp7djgheGeScUvptFjt4ppob+ywVGIJaxEhwXBFETdjrghHEdf TnmbFCta7xkTZlxfcmGYJMGw1MLEPMBO8uL/kvTpN0Lh+UYwys5l6yc2pWCs+g1YFsMUzGIZramdm WThCYKNd2Hh6ClUtzBHpBd+MDkAggJMbFY4+SAzn9szT5gdpwwkN1BmsaxCaCAZmCGUXWP+LI9tpi nWJMLj5srJV3yiRpMzQcBEJDfDldZ7HSRxRGa0KU97Dq34JunjvbwAFDPmurWHDrqiGpdPWAaBS6L k0pbmQkchnFmybioRLlw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1pp1os-004L7f-2Z; Wed, 19 Apr 2023 07:01:34 +0000 Received: from mx1.tq-group.com ([93.104.207.81]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1pp1ok-004L29-2X for linux-arm-kernel@lists.infradead.org; Wed, 19 Apr 2023 07:01:32 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tq-group.com; i=@tq-group.com; q=dns/txt; s=key1; t=1681887686; x=1713423686; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=aAx05+IOOVkajk9MsdTQnVq3/7ahZFO2x1LzaCMOaXk=; b=oU8vhUqufa6tb4r++w0NV4pIfpjnikyHllgwIvVXeOoQt0l0/YNL0o45 sxZWoMfZZ+U22+Wu/AMyZ0aWmkStlsTiW7BZteLW/4drgTw7QGt0HbQuf KcKGkMq/1BRuxamTRDa411Q4ziSkSyI7u68JZwqvAfsms0+jFT7BfBcLQ 1Gy3VYst6rg24Bhgq1nG/RogR5Y+bmeo8maF79LN1WQxJS7LQVVV5bNWQ fHxMx2hJybmMk6/sEfpjp3GnMtnbxlpbRxAwNE8wMZvPHIzf9GbmSiygG rulk7NY4yN3q/cX3X7UrwXXtQ+BXT8WGhzsYoXD4sOWa9ru7ZDe3GoQow w==; X-IronPort-AV: E=Sophos;i="5.99,208,1677538800"; d="scan'208";a="30414978" Received: from unknown (HELO tq-pgp-pr1.tq-net.de) ([192.168.6.15]) by mx1-pgp.tq-group.com with ESMTP; 19 Apr 2023 09:01:21 +0200 Received: from mx1.tq-group.com ([192.168.6.7]) by tq-pgp-pr1.tq-net.de (PGP Universal service); Wed, 19 Apr 2023 09:01:21 +0200 X-PGP-Universal: processed; by tq-pgp-pr1.tq-net.de on Wed, 19 Apr 2023 09:01:21 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tq-group.com; i=@tq-group.com; q=dns/txt; s=key1; t=1681887681; x=1713423681; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=aAx05+IOOVkajk9MsdTQnVq3/7ahZFO2x1LzaCMOaXk=; b=QEe+koU3ta3uxlT+ZsLz6io6mpsRA56pRM8swko75D2uOzzO4vKlkRKz W1w795u6udsIFh/ytgpLQuefV/jn1dCJWdZBHbKUP7kO3frw4zG8yjuPT 2lanEPRiDTAY6UfGUTHh48b6i/pW2NGFhlT2I0XcHuNU4NcJpUmIsIBWV 48a9Cie6P69T8aJUXARkHMMaYKudm1miEuettWgR222ckguK4bxDL0u7v 2n2qKyYe7YGLvsqmKyibCLDBJeyMb0h2ZxdKjEJOCJWHhVR1XTSPc/3jI MT6edeNXPORZiu8m8DHnbyFYZOXiTQTVQf8ivmKXQ1mmTTO6vmQsZiQcC w==; X-IronPort-AV: E=Sophos;i="5.99,208,1677538800"; d="scan'208";a="30414975" Received: from vtuxmail01.tq-net.de ([10.115.0.20]) by mx1.tq-group.com with ESMTP; 19 Apr 2023 09:01:20 +0200 Received: from steina-w.localnet (unknown [10.123.53.21]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by vtuxmail01.tq-net.de (Postfix) with ESMTPSA id C5D27280056; Wed, 19 Apr 2023 09:01:19 +0200 (CEST) From: Alexander Stein To: Laurent Pinchart Cc: Rui Miguel Silva , Mauro Carvalho Chehab , Shawn Guo , Sascha Hauer , Fabio Estevam , Pengutronix Kernel Team , NXP Linux Team , linux-media@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v3 3/4] media: imx: imx7-media-csi: Lift width constraints for 8bpp formats Date: Wed, 19 Apr 2023 09:01:17 +0200 Message-ID: <6180938.31r3eYUQgx@steina-w> Organization: TQ-Systems GmbH In-Reply-To: <20230418155947.GI30837@pendragon.ideasonboard.com> References: <20230418122041.1318862-1-alexander.stein@ew.tq-group.com> <20230418122041.1318862-4-alexander.stein@ew.tq-group.com> <20230418155947.GI30837@pendragon.ideasonboard.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20230419_000127_326790_3064692B X-CRM114-Status: GOOD ( 32.54 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Laurent, thanks for the feedback. Am Dienstag, 18. April 2023, 17:59:47 CEST schrieb Laurent Pinchart: > Hi Alexander, > = > Thank you for the patch. > = > The commit message should state "Lift width constraint for 16bpp > formats". To be pedantic it should be called "Lift width constraint for non-8bpp = formats" :) > I would also phrase is "Relax" instead of "Lift" as it's not > completely lifted. That's true, this subtle difference should be accounted for. Thanks. > On Tue, Apr 18, 2023 at 02:20:40PM +0200, Alexander Stein wrote: > > For 8-bit formats the image_width just needs to be a multiple of 8 pixe= ls > > others just a multiple of 4 pixels. > = > This is a bit terse, and I think a word or two are missing. It could be > improved: > = > The driver unconditionally aligns the image width to multiples of 8 > pixels. The real alignment constraint is 8 bytes, as indicated by the > CSI_IMAG_PARA.IMAGE_WIDTH documentation that calls for 8 pixel alignment > for 8bpp formats and 4 pixel alignment for other formats. Thanks for this suggestion. This sounds much better. > > Signed-off-by: Alexander Stein > > --- > > Changes in v3: > > * Fix commit message (Only 8-bit formats needs multiple of 8 pixels) > > = > > drivers/media/platform/nxp/imx7-media-csi.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > = > > diff --git a/drivers/media/platform/nxp/imx7-media-csi.c > > b/drivers/media/platform/nxp/imx7-media-csi.c index > > 1315f5743b76f..730c9c57bf4bc 100644 > > --- a/drivers/media/platform/nxp/imx7-media-csi.c > > +++ b/drivers/media/platform/nxp/imx7-media-csi.c > > @@ -1146,6 +1146,7 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format > > *pixfmt,> = > > struct v4l2_rect *compose) > > = > > { > > = > > const struct imx7_csi_pixfmt *cc; > > = > > + u32 walign; > > = > > if (compose) { > > = > > compose->width =3D pixfmt->width; > > = > > @@ -1162,13 +1163,19 @@ __imx7_csi_video_try_fmt(struct v4l2_pix_format > > *pixfmt,> = > > cc =3D imx7_csi_find_pixel_format(pixfmt->pixelformat); > > = > > } > > = > > + /* Refer to CSI_IMAG_PARA.IMAGE_WIDTH description */ > > + if (cc->bpp =3D=3D 8) > > + walign =3D 8; > > + else > > + walign =3D 4; > = > Would the following convey the purpose better ? > = > /* > * The width alignment is 8 bytes as indicated by the > * CSI_IMAG_PARA.IMAGE_WIDTH documentation. Convert it to pixels. > */ > walign =3D 8 * 8 / cc->bpp; I was wondering how to shorten this calculation without using ? operator, = nice. > > + > > = > > /* > > = > > * Round up width for minimum burst size. > > * > > * TODO: Implement configurable stride support, and check what the = real > > * hardware alignment constraint on the width is. > > */ > = > We can now drop the second part of the sentence :-) The first line is > actually not very accurate anymore. How about > = > /* > * The width alignment is 8 bytes as indicated by the > * CSI_IMAG_PARA.IMAGE_WIDTH documentation. Convert it to pixels. > * > * TODO: Implement configurable stride support. > */ > walign =3D 8 * 8 / cc->bpp; > v4l_bound_align_image(&pixfmt->width, 1, 0xffff, walign, > &pixfmt->height, 1, 0xffff, 1, 0); That's neat, thanks. I'll update accordingly. Best regards, Alexander > > - v4l_bound_align_image(&pixfmt->width, 1, 0xffff, 8, > > + v4l_bound_align_image(&pixfmt->width, 1, 0xffff, walign, > > = > > &pixfmt->height, 1, 0xffff, 1, 0); > > = > > pixfmt->bytesperline =3D pixfmt->width * cc->bpp / 8; -- = TQ-Systems GmbH | M=FChlstra=DFe 2, Gut Delling | 82229 Seefeld, Germany Amtsgericht M=FCnchen, HRB 105018 Gesch=E4ftsf=FChrer: Detlef Schneider, R=FCdiger Stahl, Stefan Schneider http://www.tq-group.com/ _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel