Re: [PATCH 04/14] media: sun6i-csi: Fix the image storage bpp for 10/12-bit Bayer formats

From: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
To: Helen Koike <helen.koike@collabora.com>
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org,
	linux-sunxi@googlegroups.com,
	"Mauro Carvalho Chehab" <mchehab@kernel.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Maxime Ripard" <mripard@kernel.org>,
	"Chen-Yu Tsai" <wens@csie.org>,
	"Yong Deng" <yong.deng@magewell.com>,
	"Kishon Vijay Abraham I" <kishon@ti.com>,
	"Vinod Koul" <vkoul@kernel.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Philipp Zabel" <p.zabel@pengutronix.de>,
	"Hans Verkuil" <hans.verkuil@cisco.com>,
	"Sakari Ailus" <sakari.ailus@linux.intel.com>,
	"Thomas Petazzoni" <thomas.petazzoni@bootlin.com>,
	"Hans Verkuil" <hverkuil@xs4all.nl>,
	kevin.lhopital@hotmail.com,
	"Kévin L'hôpital" <kevin.lhopital@bootlin.com>
Subject: Re: [PATCH 04/14] media: sun6i-csi: Fix the image storage bpp for 10/12-bit Bayer formats
Date: Wed, 4 Nov 2020 11:56:19 +0100	[thread overview]
Message-ID: <20201104105619.GG285779@aptenodytes> (raw)
In-Reply-To: <60fc4f85-e08f-fec6-5687-005add5cbeed@collabora.com>

[-- Attachment #1: Type: text/plain, Size: 2912 bytes --]

Hi Helen,

On Fri 30 Oct 20, 19:45, Helen Koike wrote:
> Hi Paul,
> 
> On 10/23/20 2:45 PM, Paul Kocialkowski wrote:
> > Both 10 and 12-bit Bayer formats are stored aligned as 16-bit values
> > in memory, not unaligned 10 or 12 bits.
> > 
> > Since the current code for retreiving the bpp is used only to
> > calculate the memory storage size of the picture (which is what
> > pixel formats describe, unlike media bus formats), fix it there.
> > 
> > Fixes: 5cc7522d8965 ("media: sun6i: Add support for Allwinner CSI V3s")
> > Co-developed-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > Signed-off-by: Kévin L'hôpital <kevin.lhopital@bootlin.com>
> > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>
> > ---
> >  .../platform/sunxi/sun6i-csi/sun6i_csi.h      | 20 +++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h
> > index c626821aaedb..7f2be70ae641 100644
> > --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h
> > +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h
> > @@ -86,7 +86,7 @@ void sun6i_csi_update_buf_addr(struct sun6i_csi *csi, dma_addr_t addr);
> >   */
> >  void sun6i_csi_set_stream(struct sun6i_csi *csi, bool enable);
> >  
> > -/* get bpp form v4l2 pixformat */
> > +/* get memory storage bpp from v4l2 pixformat */
> >  static inline int sun6i_csi_get_bpp(unsigned int pixformat)
> >  {
> >  	switch (pixformat) {
> > @@ -96,15 +96,6 @@ static inline int sun6i_csi_get_bpp(unsigned int pixformat)
> >  	case V4L2_PIX_FMT_SRGGB8:
> >  	case V4L2_PIX_FMT_JPEG:
> >  		return 8;
> > -	case V4L2_PIX_FMT_SBGGR10:
> > -	case V4L2_PIX_FMT_SGBRG10:
> > -	case V4L2_PIX_FMT_SGRBG10:
> > -	case V4L2_PIX_FMT_SRGGB10:
> > -		return 10;
> > -	case V4L2_PIX_FMT_SBGGR12:
> > -	case V4L2_PIX_FMT_SGBRG12:
> > -	case V4L2_PIX_FMT_SGRBG12:
> > -	case V4L2_PIX_FMT_SRGGB12:
> >  	case V4L2_PIX_FMT_HM12:
> >  	case V4L2_PIX_FMT_NV12:
> >  	case V4L2_PIX_FMT_NV21:
> > @@ -121,6 +112,15 @@ static inline int sun6i_csi_get_bpp(unsigned int pixformat)
> >  	case V4L2_PIX_FMT_RGB565:
> >  	case V4L2_PIX_FMT_RGB565X:
> >  		return 16;
> > +	case V4L2_PIX_FMT_SBGGR10:
> > +	case V4L2_PIX_FMT_SGBRG10:
> > +	case V4L2_PIX_FMT_SGRBG10:
> > +	case V4L2_PIX_FMT_SRGGB10:
> > +	case V4L2_PIX_FMT_SBGGR12:
> > +	case V4L2_PIX_FMT_SGBRG12:
> > +	case V4L2_PIX_FMT_SGRBG12:
> > +	case V4L2_PIX_FMT_SRGGB12:
> > +		return 16;
> >  	case V4L2_PIX_FMT_RGB24:
> >  	case V4L2_PIX_FMT_BGR24:
> >  		return 24;
> > 
> 
> Instead of updating this table, how about using v4l2_format_info() instead?

Yes that would be a very good thing to do indeed!

Thanks,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]