From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 7760F1365 for ; Tue, 15 Feb 2022 07:46:34 +0000 (UTC) Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 8DAF1315; Tue, 15 Feb 2022 08:46:32 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1644911192; bh=3/lC35RitvdJBNfH1zjCySTRPjfDuRUAGttAih7W9es=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=qe4/HoIUkiT5aUSTSTDvmrN3B02OAxwznAgrb0wgTGdFJO96yn1nn4rw+kxTmByky X9Ii/k3VstDvixGlAtqqWz0mD2GlCswArHak7KFfGArt8S8n+b49a7IB219i4CXk9z meHt1zsQlDLf8jiYAvYekV250eWM6Ph7DWM9uyT4= Date: Tue, 15 Feb 2022 09:46:26 +0200 From: Laurent Pinchart To: Jacopo Mondi , Sakari Ailus Cc: slongerbeam@gmail.com, p.zabel@pengutronix.de, shawnguo@kernel.org, s.hauer@pengutronix.de, festevam@gmail.com, mchehab@kernel.org, hverkuil-cisco@xs4all.nl, martin.kepplinger@puri.sm, rmfrfs@gmail.com, xavier.roumegue@oss.nxp.com, alexander.stein@ew.tq-group.com, dorota.czaplejewicz@puri.sm, kernel@pengutronix.de, linux-imx@nxp.com, linux-media@vger.kernel.org, linux-staging@lists.linux.dev, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 8/8] media: imx: imx-mipi-csis: Add RGB/BGR888 Message-ID: References: <20220214184318.409208-1-jacopo@jmondi.org> <20220214184318.409208-9-jacopo@jmondi.org> Precedence: bulk X-Mailing-List: linux-staging@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20220214184318.409208-9-jacopo@jmondi.org> Hi Jacopo, (Adding Sakari to the recipients) Thank you for the patch. On Mon, Feb 14, 2022 at 07:43:18PM +0100, Jacopo Mondi wrote: > Add support for the RGB888_1X24 and BGR888_1X24 image formats. > > Signed-off-by: Jacopo Mondi > --- > drivers/media/platform/imx/imx-mipi-csis.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c > index 9e0a478dba75..0d5870b3010a 100644 > --- a/drivers/media/platform/imx/imx-mipi-csis.c > +++ b/drivers/media/platform/imx/imx-mipi-csis.c > @@ -366,6 +366,16 @@ static const struct csis_pix_format mipi_csis_formats[] = { > .data_type = MIPI_CSI2_DATA_TYPE_RGB565, > .width = 16, > }, > + { }, { to match the rest of the array. Same below. > + .code = MEDIA_BUS_FMT_RGB888_1X24, > + .data_type = MIPI_CSI2_DATA_TYPE_RGB888, > + .width = 24, > + }, > + { > + .code = MEDIA_BUS_FMT_BGR888_1X24, > + .data_type = MIPI_CSI2_DATA_TYPE_RGB888, > + .width = 24, > + }, CSI-2 specifies the order of bits on the bus for RGB888, with data being transmitted in the B, G, R order. The recommended format when stored in memory is V4L2_PIX_FMT_BGR24 (B in byte 0, G in byte 1, R in byte 2). There is no recommended deserialized bus format though. On the output side of the CSIS, we have information. Given figure 13-23 ("Pixel alignment") in the i.MX8MP reference manual, and the definition of RGB_SWAP in MIPI_CSI2_ISP_CONFIG_CH0 that reads Swapping RGB sequence 0 MSB is R and LSB is B. 1 MSB is B and LSB is R (swapped). I think MEDIA_BUS_FMT_RGB888_1X24 is appropriate. On the input side of the CSIS, however, it's a different story, similar to YUV formats. For YUV 4:2:2 we have picked MEDIA_BUS_FMT_UYVY8_1X16 arbitrarily to represent the CSI-2 bus format. It doesn't correspond to the physical reality, but it doesn't matter much. We thus need to do the same for RGB888. If we follow the same convention as for YUV 4:2:2, which transmits data in the U, Y, V, Y order, we should then pick BGR888_1X24. However, the CSIS driver would then need to translate from the input format BGR888_1X24 to the output format RGB888_1X24, which adds a bit of complexity. Picking RGB888_1X24 for the CSI-2 bus is thus tempting, but it's a choice that will affect all drivers, so I wouldn't make this based solely on ease of implementation in a particular driver. I'm thus tempted to go for BGR888_1X24 to be consistent with YUV 4:2:2. Another option would be to add a new RGB888_CSI2 media bus format. In retrospect we should have done the same for YUV 4:2:2. Mistakes happen. Sakari, what do you think ? If we go for BGR888_1X24, the translation between BGR888_1X24 and RGB888_1X24 may not be that hard to implement. You could add an output field to the csis_pix_format structure to store the output media bus code for a given input code, and I think the implementation would then remain relatively simple. Finally, we can also support the swapped RGB format (which is non-standard in CSI-2 but is supported by some sensors, the same way as YUV permutations are often supported too), but it will require setting RGB_SWAP. You can add a rgb_swap field to csis_pix_format for this. I'd split this patch in two, adding MEDIA_BUS_FMT_RGB888_1X24 first, and then adding MEDIA_BUS_FMT_BGR888_1X24 with the new rgb_swap field. The first patch should capture the above explanation. > /* RAW (Bayer and greyscale) formats. */ > { > .code = MEDIA_BUS_FMT_SBGGR8_1X8, -- Regards, Laurent Pinchart 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 F123EC433EF for ; Tue, 15 Feb 2022 07:47:49 +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:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=KBIIzAYHMQV8EM147/KnG0oiMUHmlpq8aOGFwVei0ic=; b=ixuwXQ2nEwyI12 qYNvVHsZh1DRovCLUCiQfG8grgUnk3l61jbVtIjp86MGPJZYBJDaE0xXw4UQ4WOkegZ3HYylKfR0K NBo+2cv2588dlKANUgEEj+JxxJDlP+S56inE+aiD75UwWJaq9CqIdQipI5baBbDNGKSRBlhtiawuY 932VzbqJCKRxjVVgjGEm9mQA1I2z/qa+SuJOELZqoNiuL4/B8LemZtRgOhLm3w34EO4pphcAj15Q6 yV9Szysk1DDqAZfvEHpeIY7hF0UzcLm8uu8dG0/JikvvEqNLziyujdfrdaBtdgY4bMNWgzdZ6uRRc bGHNk5IE0H98wHtXHq3w==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nJsXm-001Knb-7n; Tue, 15 Feb 2022 07:46:38 +0000 Received: from perceval.ideasonboard.com ([213.167.242.64]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nJsXi-001Kll-6e for linux-arm-kernel@lists.infradead.org; Tue, 15 Feb 2022 07:46:35 +0000 Received: from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 8DAF1315; Tue, 15 Feb 2022 08:46:32 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1644911192; bh=3/lC35RitvdJBNfH1zjCySTRPjfDuRUAGttAih7W9es=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=qe4/HoIUkiT5aUSTSTDvmrN3B02OAxwznAgrb0wgTGdFJO96yn1nn4rw+kxTmByky X9Ii/k3VstDvixGlAtqqWz0mD2GlCswArHak7KFfGArt8S8n+b49a7IB219i4CXk9z meHt1zsQlDLf8jiYAvYekV250eWM6Ph7DWM9uyT4= Date: Tue, 15 Feb 2022 09:46:26 +0200 From: Laurent Pinchart To: Jacopo Mondi , Sakari Ailus Cc: slongerbeam@gmail.com, p.zabel@pengutronix.de, shawnguo@kernel.org, s.hauer@pengutronix.de, festevam@gmail.com, mchehab@kernel.org, hverkuil-cisco@xs4all.nl, martin.kepplinger@puri.sm, rmfrfs@gmail.com, xavier.roumegue@oss.nxp.com, alexander.stein@ew.tq-group.com, dorota.czaplejewicz@puri.sm, kernel@pengutronix.de, linux-imx@nxp.com, linux-media@vger.kernel.org, linux-staging@lists.linux.dev, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 8/8] media: imx: imx-mipi-csis: Add RGB/BGR888 Message-ID: References: <20220214184318.409208-1-jacopo@jmondi.org> <20220214184318.409208-9-jacopo@jmondi.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220214184318.409208-9-jacopo@jmondi.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220214_234634_423678_917D43E8 X-CRM114-Status: GOOD ( 28.02 ) 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="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Hi Jacopo, (Adding Sakari to the recipients) Thank you for the patch. On Mon, Feb 14, 2022 at 07:43:18PM +0100, Jacopo Mondi wrote: > Add support for the RGB888_1X24 and BGR888_1X24 image formats. > > Signed-off-by: Jacopo Mondi > --- > drivers/media/platform/imx/imx-mipi-csis.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c > index 9e0a478dba75..0d5870b3010a 100644 > --- a/drivers/media/platform/imx/imx-mipi-csis.c > +++ b/drivers/media/platform/imx/imx-mipi-csis.c > @@ -366,6 +366,16 @@ static const struct csis_pix_format mipi_csis_formats[] = { > .data_type = MIPI_CSI2_DATA_TYPE_RGB565, > .width = 16, > }, > + { }, { to match the rest of the array. Same below. > + .code = MEDIA_BUS_FMT_RGB888_1X24, > + .data_type = MIPI_CSI2_DATA_TYPE_RGB888, > + .width = 24, > + }, > + { > + .code = MEDIA_BUS_FMT_BGR888_1X24, > + .data_type = MIPI_CSI2_DATA_TYPE_RGB888, > + .width = 24, > + }, CSI-2 specifies the order of bits on the bus for RGB888, with data being transmitted in the B, G, R order. The recommended format when stored in memory is V4L2_PIX_FMT_BGR24 (B in byte 0, G in byte 1, R in byte 2). There is no recommended deserialized bus format though. On the output side of the CSIS, we have information. Given figure 13-23 ("Pixel alignment") in the i.MX8MP reference manual, and the definition of RGB_SWAP in MIPI_CSI2_ISP_CONFIG_CH0 that reads Swapping RGB sequence 0 MSB is R and LSB is B. 1 MSB is B and LSB is R (swapped). I think MEDIA_BUS_FMT_RGB888_1X24 is appropriate. On the input side of the CSIS, however, it's a different story, similar to YUV formats. For YUV 4:2:2 we have picked MEDIA_BUS_FMT_UYVY8_1X16 arbitrarily to represent the CSI-2 bus format. It doesn't correspond to the physical reality, but it doesn't matter much. We thus need to do the same for RGB888. If we follow the same convention as for YUV 4:2:2, which transmits data in the U, Y, V, Y order, we should then pick BGR888_1X24. However, the CSIS driver would then need to translate from the input format BGR888_1X24 to the output format RGB888_1X24, which adds a bit of complexity. Picking RGB888_1X24 for the CSI-2 bus is thus tempting, but it's a choice that will affect all drivers, so I wouldn't make this based solely on ease of implementation in a particular driver. I'm thus tempted to go for BGR888_1X24 to be consistent with YUV 4:2:2. Another option would be to add a new RGB888_CSI2 media bus format. In retrospect we should have done the same for YUV 4:2:2. Mistakes happen. Sakari, what do you think ? If we go for BGR888_1X24, the translation between BGR888_1X24 and RGB888_1X24 may not be that hard to implement. You could add an output field to the csis_pix_format structure to store the output media bus code for a given input code, and I think the implementation would then remain relatively simple. Finally, we can also support the swapped RGB format (which is non-standard in CSI-2 but is supported by some sensors, the same way as YUV permutations are often supported too), but it will require setting RGB_SWAP. You can add a rgb_swap field to csis_pix_format for this. I'd split this patch in two, adding MEDIA_BUS_FMT_RGB888_1X24 first, and then adding MEDIA_BUS_FMT_BGR888_1X24 with the new rgb_swap field. The first patch should capture the above explanation. > /* RAW (Bayer and greyscale) formats. */ > { > .code = MEDIA_BUS_FMT_SBGGR8_1X8, -- Regards, Laurent Pinchart _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel