Re: [PATCH 2/3] mx3_camera: Support correctly the YUV222 and BAYER configurations of CSI

Re: [PATCH 2/3] mx3_camera: Support correctly the YUV222 and BAYER configurations of CSI

[Guennadi Liakhovetski]

Alberto

it would be slowly on the time to address my comments and submit updates. 
While at it, also, please update the subject - you probably meant "YUV422" 
or "YUV444" there, also below:

On Wed, 1 Dec 2010, Guennadi Liakhovetski wrote:

> On Sun, 28 Nov 2010, Alberto Panizzo wrote:
> 
> > This patch is tested and works with the OV2640 camera that output
> > YUV422 (UYVY) and RGB565 data.
> > 
> > The YUV422 format is managed to be converted in IPU internal YUV444 format
> > so this stream could be used in the future to feed directly other IPU
> > blocks.
> > The RGB565 format is managed as GENERIC and can be moved only from CSI
> > to memory.
> > 
> > Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
> > ---
> > 
> > Before applying, please give me feedback if this break in some way other
> > pixel formats!
> > 
> > 
> >  drivers/media/video/mx3_camera.c |  126 +++++++++++++++++++++++++++++++++-----
> >  1 files changed, 110 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/media/video/mx3_camera.c b/drivers/media/video/mx3_camera.c
> > index 29c5fc3..6811d6f 100644
> > --- a/drivers/media/video/mx3_camera.c
> > +++ b/drivers/media/video/mx3_camera.c
> > @@ -55,6 +55,31 @@
> >  #define CSI_SENS_CONF_EXT_VSYNC_SHIFT		15
> >  #define CSI_SENS_CONF_DIVRATIO_SHIFT		16
> >  
> > +/*
> > + * IPU support the following data formatting (44.1.1.3 Data Flows and Formats):
> > + * 1 YUV 4:4:4 or RGB—8 bits per color component
> > + * 2 YUV 4:4:4 or RGB—10 bits per color component

don't know what characters are those. Please, replace with ASCII.

Thanks
Guennadi

> > + * 3 Generic data (from sensor to the system memory only)
> > + * The formats 1 and 2 are aligned in words of 32 bits, 3 is free and not
> > + * recognized by IPU blocks.
> > + *
> > + * Taking the value of SENS_DATA_FORMAT and DATA_WIDTH, the CSI tries to
> > + * align (or rearrange) the sampled data to fit the IPU supported formats
> > + * as follows:
> > + * - CSI_SENS_CONF_DATA_FMT_RGB_YUV444: It consider the pixel as a sequence of
> > + *	3 components of width DATA_WIDTH aligning these to a 32 bit word.
> > + *	The CSI output in this case can feed other IPU blocks.
> > + * - CSI_SENS_CONF_DATA_FMT_YUV422: It consider the pixel as a sequence of
> > + *	2 components of width DATA_WIDTH were the first is the alternating U V
> 
> s/were/where/
> 
> > + *	components and the second is Y. It construct the YUV444 word repeating
> > + *	the previous U, V samples aligning the results to a 32 bit word.
> > + *	The CSI output in this case can feed other IPU blocks.
> > + * - CSI_SENS_CONF_DATA_FMT_BAYER: No rework is performed in this case.
> > + *	The sensor data is given as is, considering _every sample_ as a pixel
> > + *	data. This format (combined with the GENERIC IPU pixel formats) can
> > + *	carry all the other sensor pixel formats to the system memory.
> > + *	The CSI output in this case _can not_ feed other IPU blocks.
> > + */
> >  #define CSI_SENS_CONF_DATA_FMT_RGB_YUV444	(0UL << CSI_SENS_CONF_DATA_FMT_SHIFT)
> >  #define CSI_SENS_CONF_DATA_FMT_YUV422		(2UL << CSI_SENS_CONF_DATA_FMT_SHIFT)
> >  #define CSI_SENS_CONF_DATA_FMT_BAYER		(3UL << CSI_SENS_CONF_DATA_FMT_SHIFT)
> > @@ -323,14 +348,12 @@ static enum pixel_fmt fourcc_to_ipu_pix(__u32 fourcc)
> >  {
> >  	/* Add more formats as need arises and test possibilities appear... */
> >  	switch (fourcc) {
> > -	case V4L2_PIX_FMT_RGB565:
> > -		return IPU_PIX_FMT_RGB565;
> >  	case V4L2_PIX_FMT_RGB24:
> >  		return IPU_PIX_FMT_RGB24;
> > +	case V4L2_PIX_FMT_UYVY:
> > +		return IPU_PIX_FMT_UYVY;
> > +	case V4L2_PIX_FMT_RGB565:
> >  	case V4L2_PIX_FMT_RGB332:
> > -		return IPU_PIX_FMT_RGB332;
> > -	case V4L2_PIX_FMT_YUV422P:
> > -		return IPU_PIX_FMT_YVU422P;
> >  	default:
> >  		return IPU_PIX_FMT_GENERIC;
> >  	}
> 
> Ok, so far mx3_camera has only been used with mt9m022 and mt9t031 sensors 
> (from what I can see in the mainline), both are bayer. It can also work 
> with monochrome cameras, and that would be the IPU_PIX_FMT_GENERIC case 
> too. So, I wouldn't mind removing the rest, and only adding / fixing what 
> you've now tested / implemented with your omnivision sensor. If anyone is 
> using mx3_camera with any other formats and thinks, that they work - 
> please, shout now. I'll probably also post a separate mail with this 
> warning.
> 
> > @@ -358,9 +381,25 @@ static void mx3_videobuf_queue(struct videobuf_queue *vq,
> >  
> >  	/* This is the configuration of one sg-element */
> >  	video->out_pixel_fmt	= fourcc_to_ipu_pix(fourcc);
> > -	video->out_width	= icd->user_width;
> > -	video->out_height	= icd->user_height;
> > -	video->out_stride	= icd->user_width;
> > +
> > +	if (video->out_pixel_fmt == IPU_PIX_FMT_GENERIC) {
> > +		/*
> > +		 * IPU_PIX_FMT_GENERIC transport bytes, not pixels. So convert
> > +		 * video->out_width and stride to the correct unit.
> > +		 */
> > +		int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> > +						icd->current_fmt->host_fmt);
> > +		BUG_ON(bytes_per_line <= 0);
> > +
> > +		video->out_width	= bytes_per_line;
> > +		video->out_height	= icd->user_height;
> > +		video->out_stride	= bytes_per_line;
> > +	} else {
> > +		/* For IPU known formats the pixel unit is OK */
> > +		video->out_width	= icd->user_width;
> > +		video->out_height	= icd->user_height;
> > +		video->out_stride	= icd->user_width;
> > +	}
> >  
> >  #ifdef DEBUG
> >  	/* helps to see what DMA actually has written */
> > @@ -730,18 +769,68 @@ static int mx3_camera_get_formats(struct soc_camera_device *icd, unsigned int id
> >  	if (xlate) {
> >  		xlate->host_fmt	= fmt;
> >  		xlate->code	= code;
> > +		dev_dbg(dev, "Providing format %c%c%c%c in pass-through mode\n",
> > +			(xlate->host_fmt->fourcc >> (0*8)) & 0xFF,
> > +			(xlate->host_fmt->fourcc >> (1*8)) & 0xFF,
> > +			(xlate->host_fmt->fourcc >> (2*8)) & 0xFF,
> > +			(xlate->host_fmt->fourcc >> (3*8)) & 0xFF);
> >  		xlate++;
> > -		dev_dbg(dev, "Providing format %x in pass-through mode\n",
> > -			xlate->host_fmt->fourcc);
> 
> make it even simpler: s/xlate->host_fmt/fmt/g
> 
> >  	}
> >  
> >  	return formats;
> >  }
> >  
> > +static int samples_per_pixel(enum v4l2_mbus_pixelcode mcode)
> > +{
> > +	switch (mcode) {
> > +	case V4L2_MBUS_FMT_YUYV8_2X8:
> > +	case V4L2_MBUS_FMT_YVYU8_2X8:
> > +	case V4L2_MBUS_FMT_VYUY8_2X8:
> > +	case V4L2_MBUS_FMT_YVYU10_2X10:
> > +	case V4L2_MBUS_FMT_YUYV10_2X10:
> > +	case V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE:
> > +	case V4L2_MBUS_FMT_RGB444_2X8_PADHI_BE:
> > +	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE:
> > +	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE:
> > +	case V4L2_MBUS_FMT_RGB565_2X8_LE:
> > +	case V4L2_MBUS_FMT_RGB565_2X8_BE:
> > +	case V4L2_MBUS_FMT_BGR565_2X8_LE:
> > +	case V4L2_MBUS_FMT_BGR565_2X8_BE:
> > +		return 2;
> > +	case V4L2_MBUS_FMT_SBGGR8_1X8:
> > +	case V4L2_MBUS_FMT_SBGGR10_1X10:
> > +	case V4L2_MBUS_FMT_GREY8_1X8:
> > +	case V4L2_MBUS_FMT_Y10_1X10:
> > +	case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE:
> > +	case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE:
> 
> Are these two really 1 sample per pixel?
> 
> > +	case V4L2_MBUS_FMT_SGRBG8_1X8:
> > +		return 1;
> > +	default:
> > +		/* Add other pixel codes as needed */
> > +		return 0;
> > +	}
> > +}
> 
> Let's just do the following:
> 
> s32 soc_mbus_samples_per_pixel(const struct soc_mbus_pixelfmt *mf)
> {
> 	switch (mf->packing) {
> 	case SOC_MBUS_PACKING_NONE:
> 	case SOC_MBUS_PACKING_EXTEND16:
> 		return 1;
> 	case SOC_MBUS_PACKING_2X8_PADHI:
> 	case SOC_MBUS_PACKING_2X8_PADLO:
> 		return 2;
> 	}
> 	return -EINVAL;
> }
> EXPORT_SYMBOL(soc_mbus_samples_per_pixel);
> 
> in drivers/media/video/soc_mediabus.c, agree?
> 
> > +
> >  static void configure_geometry(struct mx3_camera_dev *mx3_cam,
> > -			       unsigned int width, unsigned int height)
> > +			       unsigned int width, unsigned int height,
> > +			       enum v4l2_mbus_pixelcode code)
> >  {
> >  	u32 ctrl, width_field, height_field;
> > +	const struct soc_mbus_pixelfmt *fmt;
> > +
> > +	fmt = soc_mbus_get_fmtdesc(code);
> > +	BUG_ON(!fmt);
> > +
> > +	if (fourcc_to_ipu_pix(fmt->fourcc) == IPU_PIX_FMT_GENERIC) {
> > +		/*
> > +		 * As we don't have an IPU native format, the CSI will be
> > +		 * configured to output BAYER and here we need to convert
> > +		 * geometry unit from pixels to samples.
> > +		 * TODO: Support vertical down sampling (YUV420)
> > +		 */
> > +		width = width * samples_per_pixel(code);
> > +		BUG_ON(!width);
> > +	}
> 
> 		width *= soc_mbus_samples_per_pixel(fmt);
> 		BUG_ON((int)width < 0);
> 
> >  
> >  	/* Setup frame size - this cannot be changed on-the-fly... */
> >  	width_field = width - 1;
> > @@ -850,7 +939,7 @@ static int mx3_camera_set_crop(struct soc_camera_device *icd,
> >  				return ret;
> >  		}
> >  
> > -		configure_geometry(mx3_cam, mf.width, mf.height);
> > +		configure_geometry(mx3_cam, mf.width, mf.height, mf.code);
> >  	}
> >  
> >  	dev_dbg(icd->dev.parent, "Sensor cropped %dx%d\n",
> > @@ -893,7 +982,7 @@ static int mx3_camera_set_fmt(struct soc_camera_device *icd,
> >  	 * mxc_v4l2_s_fmt()
> >  	 */
> >  
> > -	configure_geometry(mx3_cam, pix->width, pix->height);
> > +	configure_geometry(mx3_cam, pix->width, pix->height, xlate->code);
> >  
> >  	mf.width	= pix->width;
> >  	mf.height	= pix->height;
> > @@ -1112,10 +1201,15 @@ static int mx3_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
> >  		  (3 << CSI_SENS_CONF_DATA_FMT_SHIFT) |
> >  		  (3 << CSI_SENS_CONF_DATA_WIDTH_SHIFT));
> >  
> > -	/* TODO: Support RGB and YUV formats */
> > +	/* TODO: Support RGB_YUV444 formats */
> >  
> > -	/* This has been set in mx3_camera_activate(), but we clear it above */
> > -	sens_conf |= CSI_SENS_CONF_DATA_FMT_BAYER;
> > +	switch (xlate->code) {
> > +	case V4L2_MBUS_FMT_UYVY8_2X8:
> > +		sens_conf |= CSI_SENS_CONF_DATA_FMT_YUV422;
> > +		break;
> > +	default:
> > +		sens_conf |= CSI_SENS_CONF_DATA_FMT_BAYER;
> > +	}
> >  
> >  	if (common_flags & SOCAM_PCLK_SAMPLE_FALLING)
> >  		sens_conf |= 1 << CSI_SENS_CONF_PIX_CLK_POL_SHIFT;
> > -- 
> > 1.6.3.3
> 
> If after all the above changed your set up still works, we're cool!;)
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2/3] mx3_camera: Support correctly the YUV222 and BAYER configurations of CSI

[Guennadi Liakhovetski]

On Sat, 18 Dec 2010, Guennadi Liakhovetski wrote:

> Alberto
> 
> it would be slowly on the time to address my comments and submit updates. 
> While at it, also, please update the subject - you probably meant "YUV422" 
> or "YUV444" there, also below:

Ok, I'm dropping this patch

Alberto, I've applied and pushed your other 2 patches from this series, 
but I've dropped this one. The reason is not (only), that you didn't reply 
to my two last mails with update-requests. But because of that I took the 
time today to look deeper into detail at this patch. And as a result, I 
don't think it is correct.

Currently the mx3_camera driver transfers data from video clients (camera 
sensors) only in one mode - as raw data, 1-to-1. This is extablished in 
the way, how it creates format translation tables during the initial 
negotiation with client drivers in mx3_camera_get_formats().

Your patch is trying to add support for specific modes on CSI, but is only 
doing this in the transfer part of the driver, and not in the negotiation 
part. So, if you really need native support for various pixel formats, 
this is a wrong way to do this. If you only want to transfer data from 
your sensor into RAM and the current driver is failing for you, then this 
is a wrong way to do this, and the bug has to be found and fixed, while 
maintaining the present pass-through only model.

Thanks
Guennadi

> On Wed, 1 Dec 2010, Guennadi Liakhovetski wrote:
> 
> > On Sun, 28 Nov 2010, Alberto Panizzo wrote:
> > 
> > > This patch is tested and works with the OV2640 camera that output
> > > YUV422 (UYVY) and RGB565 data.
> > > 
> > > The YUV422 format is managed to be converted in IPU internal YUV444 format
> > > so this stream could be used in the future to feed directly other IPU
> > > blocks.
> > > The RGB565 format is managed as GENERIC and can be moved only from CSI
> > > to memory.
> > > 
> > > Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
> > > ---
> > > 
> > > Before applying, please give me feedback if this break in some way other
> > > pixel formats!
> > > 
> > > 
> > >  drivers/media/video/mx3_camera.c |  126 +++++++++++++++++++++++++++++++++-----
> > >  1 files changed, 110 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/media/video/mx3_camera.c b/drivers/media/video/mx3_camera.c
> > > index 29c5fc3..6811d6f 100644
> > > --- a/drivers/media/video/mx3_camera.c
> > > +++ b/drivers/media/video/mx3_camera.c
> > > @@ -55,6 +55,31 @@
> > >  #define CSI_SENS_CONF_EXT_VSYNC_SHIFT		15
> > >  #define CSI_SENS_CONF_DIVRATIO_SHIFT		16
> > >  
> > > +/*
> > > + * IPU support the following data formatting (44.1.1.3 Data Flows and Formats):
> > > + * 1 YUV 4:4:4 or RGB—8 bits per color component
> > > + * 2 YUV 4:4:4 or RGB—10 bits per color component
> 
> don't know what characters are those. Please, replace with ASCII.
> 
> Thanks
> Guennadi
> 
> > > + * 3 Generic data (from sensor to the system memory only)
> > > + * The formats 1 and 2 are aligned in words of 32 bits, 3 is free and not
> > > + * recognized by IPU blocks.
> > > + *
> > > + * Taking the value of SENS_DATA_FORMAT and DATA_WIDTH, the CSI tries to
> > > + * align (or rearrange) the sampled data to fit the IPU supported formats
> > > + * as follows:
> > > + * - CSI_SENS_CONF_DATA_FMT_RGB_YUV444: It consider the pixel as a sequence of
> > > + *	3 components of width DATA_WIDTH aligning these to a 32 bit word.
> > > + *	The CSI output in this case can feed other IPU blocks.
> > > + * - CSI_SENS_CONF_DATA_FMT_YUV422: It consider the pixel as a sequence of
> > > + *	2 components of width DATA_WIDTH were the first is the alternating U V
> > 
> > s/were/where/
> > 
> > > + *	components and the second is Y. It construct the YUV444 word repeating
> > > + *	the previous U, V samples aligning the results to a 32 bit word.
> > > + *	The CSI output in this case can feed other IPU blocks.
> > > + * - CSI_SENS_CONF_DATA_FMT_BAYER: No rework is performed in this case.
> > > + *	The sensor data is given as is, considering _every sample_ as a pixel
> > > + *	data. This format (combined with the GENERIC IPU pixel formats) can
> > > + *	carry all the other sensor pixel formats to the system memory.
> > > + *	The CSI output in this case _can not_ feed other IPU blocks.
> > > + */
> > >  #define CSI_SENS_CONF_DATA_FMT_RGB_YUV444	(0UL << CSI_SENS_CONF_DATA_FMT_SHIFT)
> > >  #define CSI_SENS_CONF_DATA_FMT_YUV422		(2UL << CSI_SENS_CONF_DATA_FMT_SHIFT)
> > >  #define CSI_SENS_CONF_DATA_FMT_BAYER		(3UL << CSI_SENS_CONF_DATA_FMT_SHIFT)
> > > @@ -323,14 +348,12 @@ static enum pixel_fmt fourcc_to_ipu_pix(__u32 fourcc)
> > >  {
> > >  	/* Add more formats as need arises and test possibilities appear... */
> > >  	switch (fourcc) {
> > > -	case V4L2_PIX_FMT_RGB565:
> > > -		return IPU_PIX_FMT_RGB565;
> > >  	case V4L2_PIX_FMT_RGB24:
> > >  		return IPU_PIX_FMT_RGB24;
> > > +	case V4L2_PIX_FMT_UYVY:
> > > +		return IPU_PIX_FMT_UYVY;
> > > +	case V4L2_PIX_FMT_RGB565:
> > >  	case V4L2_PIX_FMT_RGB332:
> > > -		return IPU_PIX_FMT_RGB332;
> > > -	case V4L2_PIX_FMT_YUV422P:
> > > -		return IPU_PIX_FMT_YVU422P;
> > >  	default:
> > >  		return IPU_PIX_FMT_GENERIC;
> > >  	}
> > 
> > Ok, so far mx3_camera has only been used with mt9m022 and mt9t031 sensors 
> > (from what I can see in the mainline), both are bayer. It can also work 
> > with monochrome cameras, and that would be the IPU_PIX_FMT_GENERIC case 
> > too. So, I wouldn't mind removing the rest, and only adding / fixing what 
> > you've now tested / implemented with your omnivision sensor. If anyone is 
> > using mx3_camera with any other formats and thinks, that they work - 
> > please, shout now. I'll probably also post a separate mail with this 
> > warning.
> > 
> > > @@ -358,9 +381,25 @@ static void mx3_videobuf_queue(struct videobuf_queue *vq,
> > >  
> > >  	/* This is the configuration of one sg-element */
> > >  	video->out_pixel_fmt	= fourcc_to_ipu_pix(fourcc);
> > > -	video->out_width	= icd->user_width;
> > > -	video->out_height	= icd->user_height;
> > > -	video->out_stride	= icd->user_width;
> > > +
> > > +	if (video->out_pixel_fmt == IPU_PIX_FMT_GENERIC) {
> > > +		/*
> > > +		 * IPU_PIX_FMT_GENERIC transport bytes, not pixels. So convert
> > > +		 * video->out_width and stride to the correct unit.
> > > +		 */
> > > +		int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> > > +						icd->current_fmt->host_fmt);
> > > +		BUG_ON(bytes_per_line <= 0);
> > > +
> > > +		video->out_width	= bytes_per_line;
> > > +		video->out_height	= icd->user_height;
> > > +		video->out_stride	= bytes_per_line;
> > > +	} else {
> > > +		/* For IPU known formats the pixel unit is OK */
> > > +		video->out_width	= icd->user_width;
> > > +		video->out_height	= icd->user_height;
> > > +		video->out_stride	= icd->user_width;
> > > +	}
> > >  
> > >  #ifdef DEBUG
> > >  	/* helps to see what DMA actually has written */
> > > @@ -730,18 +769,68 @@ static int mx3_camera_get_formats(struct soc_camera_device *icd, unsigned int id
> > >  	if (xlate) {
> > >  		xlate->host_fmt	= fmt;
> > >  		xlate->code	= code;
> > > +		dev_dbg(dev, "Providing format %c%c%c%c in pass-through mode\n",
> > > +			(xlate->host_fmt->fourcc >> (0*8)) & 0xFF,
> > > +			(xlate->host_fmt->fourcc >> (1*8)) & 0xFF,
> > > +			(xlate->host_fmt->fourcc >> (2*8)) & 0xFF,
> > > +			(xlate->host_fmt->fourcc >> (3*8)) & 0xFF);
> > >  		xlate++;
> > > -		dev_dbg(dev, "Providing format %x in pass-through mode\n",
> > > -			xlate->host_fmt->fourcc);
> > 
> > make it even simpler: s/xlate->host_fmt/fmt/g
> > 
> > >  	}
> > >  
> > >  	return formats;
> > >  }
> > >  
> > > +static int samples_per_pixel(enum v4l2_mbus_pixelcode mcode)
> > > +{
> > > +	switch (mcode) {
> > > +	case V4L2_MBUS_FMT_YUYV8_2X8:
> > > +	case V4L2_MBUS_FMT_YVYU8_2X8:
> > > +	case V4L2_MBUS_FMT_VYUY8_2X8:
> > > +	case V4L2_MBUS_FMT_YVYU10_2X10:
> > > +	case V4L2_MBUS_FMT_YUYV10_2X10:
> > > +	case V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE:
> > > +	case V4L2_MBUS_FMT_RGB444_2X8_PADHI_BE:
> > > +	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE:
> > > +	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE:
> > > +	case V4L2_MBUS_FMT_RGB565_2X8_LE:
> > > +	case V4L2_MBUS_FMT_RGB565_2X8_BE:
> > > +	case V4L2_MBUS_FMT_BGR565_2X8_LE:
> > > +	case V4L2_MBUS_FMT_BGR565_2X8_BE:
> > > +		return 2;
> > > +	case V4L2_MBUS_FMT_SBGGR8_1X8:
> > > +	case V4L2_MBUS_FMT_SBGGR10_1X10:
> > > +	case V4L2_MBUS_FMT_GREY8_1X8:
> > > +	case V4L2_MBUS_FMT_Y10_1X10:
> > > +	case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE:
> > > +	case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE:
> > 
> > Are these two really 1 sample per pixel?
> > 
> > > +	case V4L2_MBUS_FMT_SGRBG8_1X8:
> > > +		return 1;
> > > +	default:
> > > +		/* Add other pixel codes as needed */
> > > +		return 0;
> > > +	}
> > > +}
> > 
> > Let's just do the following:
> > 
> > s32 soc_mbus_samples_per_pixel(const struct soc_mbus_pixelfmt *mf)
> > {
> > 	switch (mf->packing) {
> > 	case SOC_MBUS_PACKING_NONE:
> > 	case SOC_MBUS_PACKING_EXTEND16:
> > 		return 1;
> > 	case SOC_MBUS_PACKING_2X8_PADHI:
> > 	case SOC_MBUS_PACKING_2X8_PADLO:
> > 		return 2;
> > 	}
> > 	return -EINVAL;
> > }
> > EXPORT_SYMBOL(soc_mbus_samples_per_pixel);
> > 
> > in drivers/media/video/soc_mediabus.c, agree?
> > 
> > > +
> > >  static void configure_geometry(struct mx3_camera_dev *mx3_cam,
> > > -			       unsigned int width, unsigned int height)
> > > +			       unsigned int width, unsigned int height,
> > > +			       enum v4l2_mbus_pixelcode code)
> > >  {
> > >  	u32 ctrl, width_field, height_field;
> > > +	const struct soc_mbus_pixelfmt *fmt;
> > > +
> > > +	fmt = soc_mbus_get_fmtdesc(code);
> > > +	BUG_ON(!fmt);
> > > +
> > > +	if (fourcc_to_ipu_pix(fmt->fourcc) == IPU_PIX_FMT_GENERIC) {
> > > +		/*
> > > +		 * As we don't have an IPU native format, the CSI will be
> > > +		 * configured to output BAYER and here we need to convert
> > > +		 * geometry unit from pixels to samples.
> > > +		 * TODO: Support vertical down sampling (YUV420)
> > > +		 */
> > > +		width = width * samples_per_pixel(code);
> > > +		BUG_ON(!width);
> > > +	}
> > 
> > 		width *= soc_mbus_samples_per_pixel(fmt);
> > 		BUG_ON((int)width < 0);
> > 
> > >  
> > >  	/* Setup frame size - this cannot be changed on-the-fly... */
> > >  	width_field = width - 1;
> > > @@ -850,7 +939,7 @@ static int mx3_camera_set_crop(struct soc_camera_device *icd,
> > >  				return ret;
> > >  		}
> > >  
> > > -		configure_geometry(mx3_cam, mf.width, mf.height);
> > > +		configure_geometry(mx3_cam, mf.width, mf.height, mf.code);
> > >  	}
> > >  
> > >  	dev_dbg(icd->dev.parent, "Sensor cropped %dx%d\n",
> > > @@ -893,7 +982,7 @@ static int mx3_camera_set_fmt(struct soc_camera_device *icd,
> > >  	 * mxc_v4l2_s_fmt()
> > >  	 */
> > >  
> > > -	configure_geometry(mx3_cam, pix->width, pix->height);
> > > +	configure_geometry(mx3_cam, pix->width, pix->height, xlate->code);
> > >  
> > >  	mf.width	= pix->width;
> > >  	mf.height	= pix->height;
> > > @@ -1112,10 +1201,15 @@ static int mx3_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
> > >  		  (3 << CSI_SENS_CONF_DATA_FMT_SHIFT) |
> > >  		  (3 << CSI_SENS_CONF_DATA_WIDTH_SHIFT));
> > >  
> > > -	/* TODO: Support RGB and YUV formats */
> > > +	/* TODO: Support RGB_YUV444 formats */
> > >  
> > > -	/* This has been set in mx3_camera_activate(), but we clear it above */
> > > -	sens_conf |= CSI_SENS_CONF_DATA_FMT_BAYER;
> > > +	switch (xlate->code) {
> > > +	case V4L2_MBUS_FMT_UYVY8_2X8:
> > > +		sens_conf |= CSI_SENS_CONF_DATA_FMT_YUV422;
> > > +		break;
> > > +	default:
> > > +		sens_conf |= CSI_SENS_CONF_DATA_FMT_BAYER;
> > > +	}
> > >  
> > >  	if (common_flags & SOCAM_PCLK_SAMPLE_FALLING)
> > >  		sens_conf |= 1 << CSI_SENS_CONF_PIX_CLK_POL_SHIFT;
> > > -- 
> > > 1.6.3.3
> > 
> > If after all the above changed your set up still works, we're cool!;)
> > 
> > Thanks
> > Guennadi
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> > 
> 
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2/3] mx3_camera: Support correctly the YUV222 and BAYER configurations of CSI

[Alberto Panizzo]

On Thu, 2010-12-30 at 20:38 +0100, Guennadi Liakhovetski wrote:
> On Sat, 18 Dec 2010, Guennadi Liakhovetski wrote:
> 
> > Alberto
> > 
> > it would be slowly on the time to address my comments and submit updates. 
> > While at it, also, please update the subject - you probably meant "YUV422" 
> > or "YUV444" there, also below:
> 
> Ok, I'm dropping this patch
> 
> Alberto, I've applied and pushed your other 2 patches from this series, 
> but I've dropped this one. The reason is not (only), that you didn't reply 
> to my two last mails with update-requests. But because of that I took the 
> time today to look deeper into detail at this patch. And as a result, I 
> don't think it is correct.
> 
> Currently the mx3_camera driver transfers data from video clients (camera 
> sensors) only in one mode - as raw data, 1-to-1. This is extablished in 
> the way, how it creates format translation tables during the initial 
> negotiation with client drivers in mx3_camera_get_formats().
> 
> Your patch is trying to add support for specific modes on CSI, but is only 
> doing this in the transfer part of the driver, and not in the negotiation 
> part. So, if you really need native support for various pixel formats, 
> this is a wrong way to do this. If you only want to transfer data from 
> your sensor into RAM and the current driver is failing for you, then this 
> is a wrong way to do this, and the bug has to be found and fixed, while 
> maintaining the present pass-through only model.
> 
> Thanks
> Guennadi

Really sorry Guennadi!
My mailer didn't filtered this thread successfully and so I missed your 
comments! Damn!

I'll go deeper on your suggestions in these days and hope to find a good
solution.

Thank you,
Alberto!

> 
> > On Wed, 1 Dec 2010, Guennadi Liakhovetski wrote:
> > 
> > > On Sun, 28 Nov 2010, Alberto Panizzo wrote:
> > > 
> > > > This patch is tested and works with the OV2640 camera that output
> > > > YUV422 (UYVY) and RGB565 data.
> > > > 
> > > > The YUV422 format is managed to be converted in IPU internal YUV444 format
> > > > so this stream could be used in the future to feed directly other IPU
> > > > blocks.
> > > > The RGB565 format is managed as GENERIC and can be moved only from CSI
> > > > to memory.
> > > > 
> > > > Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
> > > > ---
> > > > 
> > > > Before applying, please give me feedback if this break in some way other
> > > > pixel formats!
> > > > 
> > > > 
> > > >  drivers/media/video/mx3_camera.c |  126 +++++++++++++++++++++++++++++++++-----
> > > >  1 files changed, 110 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/video/mx3_camera.c b/drivers/media/video/mx3_camera.c
> > > > index 29c5fc3..6811d6f 100644
> > > > --- a/drivers/media/video/mx3_camera.c
> > > > +++ b/drivers/media/video/mx3_camera.c
> > > > @@ -55,6 +55,31 @@
> > > >  #define CSI_SENS_CONF_EXT_VSYNC_SHIFT		15
> > > >  #define CSI_SENS_CONF_DIVRATIO_SHIFT		16
> > > >  
> > > > +/*
> > > > + * IPU support the following data formatting (44.1.1.3 Data Flows and Formats):
> > > > + * 1 YUV 4:4:4 or RGB—8 bits per color component
> > > > + * 2 YUV 4:4:4 or RGB—10 bits per color component
> > 
> > don't know what characters are those. Please, replace with ASCII.
> > 
> > Thanks
> > Guennadi
> > 
> > > > + * 3 Generic data (from sensor to the system memory only)
> > > > + * The formats 1 and 2 are aligned in words of 32 bits, 3 is free and not
> > > > + * recognized by IPU blocks.
> > > > + *
> > > > + * Taking the value of SENS_DATA_FORMAT and DATA_WIDTH, the CSI tries to
> > > > + * align (or rearrange) the sampled data to fit the IPU supported formats
> > > > + * as follows:
> > > > + * - CSI_SENS_CONF_DATA_FMT_RGB_YUV444: It consider the pixel as a sequence of
> > > > + *	3 components of width DATA_WIDTH aligning these to a 32 bit word.
> > > > + *	The CSI output in this case can feed other IPU blocks.
> > > > + * - CSI_SENS_CONF_DATA_FMT_YUV422: It consider the pixel as a sequence of
> > > > + *	2 components of width DATA_WIDTH were the first is the alternating U V
> > > 
> > > s/were/where/
> > > 
> > > > + *	components and the second is Y. It construct the YUV444 word repeating
> > > > + *	the previous U, V samples aligning the results to a 32 bit word.
> > > > + *	The CSI output in this case can feed other IPU blocks.
> > > > + * - CSI_SENS_CONF_DATA_FMT_BAYER: No rework is performed in this case.
> > > > + *	The sensor data is given as is, considering _every sample_ as a pixel
> > > > + *	data. This format (combined with the GENERIC IPU pixel formats) can
> > > > + *	carry all the other sensor pixel formats to the system memory.
> > > > + *	The CSI output in this case _can not_ feed other IPU blocks.
> > > > + */
> > > >  #define CSI_SENS_CONF_DATA_FMT_RGB_YUV444	(0UL << CSI_SENS_CONF_DATA_FMT_SHIFT)
> > > >  #define CSI_SENS_CONF_DATA_FMT_YUV422		(2UL << CSI_SENS_CONF_DATA_FMT_SHIFT)
> > > >  #define CSI_SENS_CONF_DATA_FMT_BAYER		(3UL << CSI_SENS_CONF_DATA_FMT_SHIFT)
> > > > @@ -323,14 +348,12 @@ static enum pixel_fmt fourcc_to_ipu_pix(__u32 fourcc)
> > > >  {
> > > >  	/* Add more formats as need arises and test possibilities appear... */
> > > >  	switch (fourcc) {
> > > > -	case V4L2_PIX_FMT_RGB565:
> > > > -		return IPU_PIX_FMT_RGB565;
> > > >  	case V4L2_PIX_FMT_RGB24:
> > > >  		return IPU_PIX_FMT_RGB24;
> > > > +	case V4L2_PIX_FMT_UYVY:
> > > > +		return IPU_PIX_FMT_UYVY;
> > > > +	case V4L2_PIX_FMT_RGB565:
> > > >  	case V4L2_PIX_FMT_RGB332:
> > > > -		return IPU_PIX_FMT_RGB332;
> > > > -	case V4L2_PIX_FMT_YUV422P:
> > > > -		return IPU_PIX_FMT_YVU422P;
> > > >  	default:
> > > >  		return IPU_PIX_FMT_GENERIC;
> > > >  	}
> > > 
> > > Ok, so far mx3_camera has only been used with mt9m022 and mt9t031 sensors 
> > > (from what I can see in the mainline), both are bayer. It can also work 
> > > with monochrome cameras, and that would be the IPU_PIX_FMT_GENERIC case 
> > > too. So, I wouldn't mind removing the rest, and only adding / fixing what 
> > > you've now tested / implemented with your omnivision sensor. If anyone is 
> > > using mx3_camera with any other formats and thinks, that they work - 
> > > please, shout now. I'll probably also post a separate mail with this 
> > > warning.
> > > 
> > > > @@ -358,9 +381,25 @@ static void mx3_videobuf_queue(struct videobuf_queue *vq,
> > > >  
> > > >  	/* This is the configuration of one sg-element */
> > > >  	video->out_pixel_fmt	= fourcc_to_ipu_pix(fourcc);
> > > > -	video->out_width	= icd->user_width;
> > > > -	video->out_height	= icd->user_height;
> > > > -	video->out_stride	= icd->user_width;
> > > > +
> > > > +	if (video->out_pixel_fmt == IPU_PIX_FMT_GENERIC) {
> > > > +		/*
> > > > +		 * IPU_PIX_FMT_GENERIC transport bytes, not pixels. So convert
> > > > +		 * video->out_width and stride to the correct unit.
> > > > +		 */
> > > > +		int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> > > > +						icd->current_fmt->host_fmt);
> > > > +		BUG_ON(bytes_per_line <= 0);
> > > > +
> > > > +		video->out_width	= bytes_per_line;
> > > > +		video->out_height	= icd->user_height;
> > > > +		video->out_stride	= bytes_per_line;
> > > > +	} else {
> > > > +		/* For IPU known formats the pixel unit is OK */
> > > > +		video->out_width	= icd->user_width;
> > > > +		video->out_height	= icd->user_height;
> > > > +		video->out_stride	= icd->user_width;
> > > > +	}
> > > >  
> > > >  #ifdef DEBUG
> > > >  	/* helps to see what DMA actually has written */
> > > > @@ -730,18 +769,68 @@ static int mx3_camera_get_formats(struct soc_camera_device *icd, unsigned int id
> > > >  	if (xlate) {
> > > >  		xlate->host_fmt	= fmt;
> > > >  		xlate->code	= code;
> > > > +		dev_dbg(dev, "Providing format %c%c%c%c in pass-through mode\n",
> > > > +			(xlate->host_fmt->fourcc >> (0*8)) & 0xFF,
> > > > +			(xlate->host_fmt->fourcc >> (1*8)) & 0xFF,
> > > > +			(xlate->host_fmt->fourcc >> (2*8)) & 0xFF,
> > > > +			(xlate->host_fmt->fourcc >> (3*8)) & 0xFF);
> > > >  		xlate++;
> > > > -		dev_dbg(dev, "Providing format %x in pass-through mode\n",
> > > > -			xlate->host_fmt->fourcc);
> > > 
> > > make it even simpler: s/xlate->host_fmt/fmt/g
> > > 
> > > >  	}
> > > >  
> > > >  	return formats;
> > > >  }
> > > >  
> > > > +static int samples_per_pixel(enum v4l2_mbus_pixelcode mcode)
> > > > +{
> > > > +	switch (mcode) {
> > > > +	case V4L2_MBUS_FMT_YUYV8_2X8:
> > > > +	case V4L2_MBUS_FMT_YVYU8_2X8:
> > > > +	case V4L2_MBUS_FMT_VYUY8_2X8:
> > > > +	case V4L2_MBUS_FMT_YVYU10_2X10:
> > > > +	case V4L2_MBUS_FMT_YUYV10_2X10:
> > > > +	case V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE:
> > > > +	case V4L2_MBUS_FMT_RGB444_2X8_PADHI_BE:
> > > > +	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE:
> > > > +	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE:
> > > > +	case V4L2_MBUS_FMT_RGB565_2X8_LE:
> > > > +	case V4L2_MBUS_FMT_RGB565_2X8_BE:
> > > > +	case V4L2_MBUS_FMT_BGR565_2X8_LE:
> > > > +	case V4L2_MBUS_FMT_BGR565_2X8_BE:
> > > > +		return 2;
> > > > +	case V4L2_MBUS_FMT_SBGGR8_1X8:
> > > > +	case V4L2_MBUS_FMT_SBGGR10_1X10:
> > > > +	case V4L2_MBUS_FMT_GREY8_1X8:
> > > > +	case V4L2_MBUS_FMT_Y10_1X10:
> > > > +	case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE:
> > > > +	case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE:
> > > 
> > > Are these two really 1 sample per pixel?
> > > 
> > > > +	case V4L2_MBUS_FMT_SGRBG8_1X8:
> > > > +		return 1;
> > > > +	default:
> > > > +		/* Add other pixel codes as needed */
> > > > +		return 0;
> > > > +	}
> > > > +}
> > > 
> > > Let's just do the following:
> > > 
> > > s32 soc_mbus_samples_per_pixel(const struct soc_mbus_pixelfmt *mf)
> > > {
> > > 	switch (mf->packing) {
> > > 	case SOC_MBUS_PACKING_NONE:
> > > 	case SOC_MBUS_PACKING_EXTEND16:
> > > 		return 1;
> > > 	case SOC_MBUS_PACKING_2X8_PADHI:
> > > 	case SOC_MBUS_PACKING_2X8_PADLO:
> > > 		return 2;
> > > 	}
> > > 	return -EINVAL;
> > > }
> > > EXPORT_SYMBOL(soc_mbus_samples_per_pixel);
> > > 
> > > in drivers/media/video/soc_mediabus.c, agree?
> > > 
> > > > +
> > > >  static void configure_geometry(struct mx3_camera_dev *mx3_cam,
> > > > -			       unsigned int width, unsigned int height)
> > > > +			       unsigned int width, unsigned int height,
> > > > +			       enum v4l2_mbus_pixelcode code)
> > > >  {
> > > >  	u32 ctrl, width_field, height_field;
> > > > +	const struct soc_mbus_pixelfmt *fmt;
> > > > +
> > > > +	fmt = soc_mbus_get_fmtdesc(code);
> > > > +	BUG_ON(!fmt);
> > > > +
> > > > +	if (fourcc_to_ipu_pix(fmt->fourcc) == IPU_PIX_FMT_GENERIC) {
> > > > +		/*
> > > > +		 * As we don't have an IPU native format, the CSI will be
> > > > +		 * configured to output BAYER and here we need to convert
> > > > +		 * geometry unit from pixels to samples.
> > > > +		 * TODO: Support vertical down sampling (YUV420)
> > > > +		 */
> > > > +		width = width * samples_per_pixel(code);
> > > > +		BUG_ON(!width);
> > > > +	}
> > > 
> > > 		width *= soc_mbus_samples_per_pixel(fmt);
> > > 		BUG_ON((int)width < 0);
> > > 
> > > >  
> > > >  	/* Setup frame size - this cannot be changed on-the-fly... */
> > > >  	width_field = width - 1;
> > > > @@ -850,7 +939,7 @@ static int mx3_camera_set_crop(struct soc_camera_device *icd,
> > > >  				return ret;
> > > >  		}
> > > >  
> > > > -		configure_geometry(mx3_cam, mf.width, mf.height);
> > > > +		configure_geometry(mx3_cam, mf.width, mf.height, mf.code);
> > > >  	}
> > > >  
> > > >  	dev_dbg(icd->dev.parent, "Sensor cropped %dx%d\n",
> > > > @@ -893,7 +982,7 @@ static int mx3_camera_set_fmt(struct soc_camera_device *icd,
> > > >  	 * mxc_v4l2_s_fmt()
> > > >  	 */
> > > >  
> > > > -	configure_geometry(mx3_cam, pix->width, pix->height);
> > > > +	configure_geometry(mx3_cam, pix->width, pix->height, xlate->code);
> > > >  
> > > >  	mf.width	= pix->width;
> > > >  	mf.height	= pix->height;
> > > > @@ -1112,10 +1201,15 @@ static int mx3_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
> > > >  		  (3 << CSI_SENS_CONF_DATA_FMT_SHIFT) |
> > > >  		  (3 << CSI_SENS_CONF_DATA_WIDTH_SHIFT));
> > > >  
> > > > -	/* TODO: Support RGB and YUV formats */
> > > > +	/* TODO: Support RGB_YUV444 formats */
> > > >  
> > > > -	/* This has been set in mx3_camera_activate(), but we clear it above */
> > > > -	sens_conf |= CSI_SENS_CONF_DATA_FMT_BAYER;
> > > > +	switch (xlate->code) {
> > > > +	case V4L2_MBUS_FMT_UYVY8_2X8:
> > > > +		sens_conf |= CSI_SENS_CONF_DATA_FMT_YUV422;
> > > > +		break;
> > > > +	default:
> > > > +		sens_conf |= CSI_SENS_CONF_DATA_FMT_BAYER;
> > > > +	}
> > > >  
> > > >  	if (common_flags & SOCAM_PCLK_SAMPLE_FALLING)
> > > >  		sens_conf |= 1 << CSI_SENS_CONF_PIX_CLK_POL_SHIFT;
> > > > -- 
> > > > 1.6.3.3
> > > 
> > > If after all the above changed your set up still works, we're cool!;)
> > > 
> > > Thanks
> > > Guennadi
> > > ---
> > > Guennadi Liakhovetski, Ph.D.
> > > Freelance Open-Source Software Developer
> > > http://www.open-technology.de/
> > > 
> > 
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> > 
> 
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 2/3] mx3_camera: Support correctly the YUV222 and BAYER configurations of CSI

[Alberto Panizzo]

On Thu, 2010-12-30 at 20:38 +0100, Guennadi Liakhovetski wrote:
> On Sat, 18 Dec 2010, Guennadi Liakhovetski wrote:
> 
> > Alberto
> > 
> > it would be slowly on the time to address my comments and submit updates. 
> > While at it, also, please update the subject - you probably meant "YUV422" 
> > or "YUV444" there, also below:
> 
> Ok, I'm dropping this patch
> 
> Alberto, I've applied and pushed your other 2 patches from this series, 
> but I've dropped this one. The reason is not (only), that you didn't reply 
> to my two last mails with update-requests. But because of that I took the 
> time today to look deeper into detail at this patch. And as a result, I 
> don't think it is correct.
> 
> Currently the mx3_camera driver transfers data from video clients (camera 
> sensors) only in one mode - as raw data, 1-to-1. This is extablished in 
> the way, how it creates format translation tables during the initial 
> negotiation with client drivers in mx3_camera_get_formats().
> 
> Your patch is trying to add support for specific modes on CSI, but is only 
> doing this in the transfer part of the driver, and not in the negotiation 
> part. So, if you really need native support for various pixel formats, 
> this is a wrong way to do this. If you only want to transfer data from 
> your sensor into RAM and the current driver is failing for you, then this 
> is a wrong way to do this, and the bug has to be found and fixed, while 
> maintaining the present pass-through only model.
> 

This patch shows that IPU and CSI manage parameters in different 
units. It shows that an unknown at the CSI pixel format, that require n
bytes per pixel, have to be considered generic on the IPU side and the
parameters of the DMA and CSI have to be set properly to support it.
In this way also 10-bit wide pixels formats can be managed in pass
through mode, setting properly the IPU and CSI parameters.

So, this patch shows that the CSI can manage successfully n-byte wide
pixel codes (tested with n = 1,2) without breaking the old behaviour
of providing 10-bit wide pixel formats with 8-bit wide ones.

The next step is to uniform also the pixel-code translations at this 
type of management. Being able to capture real 10-bit wide samples.

This patch also, make use of a native functionality of the CSI: capture 
a YUV422 format. 
In this case the CSI convert this pixel format to YUV444 sent to the 
BUS  and the IPU re-pack the YUV444 to YUV422 into the memory.

This shows how CSI and IPU manage formats different than the generic 
one and open the way to understand how to support the communication
between agents of the IPU encoding chain.

Maybe the last part is misleading you and can be dropped out from this 
patch as an enhancement: the YUV422 interleaved format can be 
successfully managed as CSI-BAYER/IPU-GENERIC one, the same as rgb565.
Supporting the CSI-YUV422 is a plus only to show how the CSI works.

Best Regards,
Alberto!

> Thanks
> Guennadi
> 
> > On Wed, 1 Dec 2010, Guennadi Liakhovetski wrote:
> > 
> > > On Sun, 28 Nov 2010, Alberto Panizzo wrote:
> > > 
> > > > This patch is tested and works with the OV2640 camera that output
> > > > YUV422 (UYVY) and RGB565 data.
> > > > 
> > > > The YUV422 format is managed to be converted in IPU internal YUV444 format
> > > > so this stream could be used in the future to feed directly other IPU
> > > > blocks.
> > > > The RGB565 format is managed as GENERIC and can be moved only from CSI
> > > > to memory.
> > > > 
> > > > Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
> > > > ---
> > > > 
> > > > Before applying, please give me feedback if this break in some way other
> > > > pixel formats!
> > > > 
> > > > 
> > > >  drivers/media/video/mx3_camera.c |  126 +++++++++++++++++++++++++++++++++-----
> > > >  1 files changed, 110 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/video/mx3_camera.c b/drivers/media/video/mx3_camera.c
> > > > index 29c5fc3..6811d6f 100644
> > > > --- a/drivers/media/video/mx3_camera.c
> > > > +++ b/drivers/media/video/mx3_camera.c
> > > > @@ -55,6 +55,31 @@
> > > >  #define CSI_SENS_CONF_EXT_VSYNC_SHIFT		15
> > > >  #define CSI_SENS_CONF_DIVRATIO_SHIFT		16
> > > >  
> > > > +/*
> > > > + * IPU support the following data formatting (44.1.1.3 Data Flows and Formats):
> > > > + * 1 YUV 4:4:4 or RGB—8 bits per color component
> > > > + * 2 YUV 4:4:4 or RGB—10 bits per color component
> > 
> > don't know what characters are those. Please, replace with ASCII.
> > 
> > Thanks
> > Guennadi
> > 
> > > > + * 3 Generic data (from sensor to the system memory only)
> > > > + * The formats 1 and 2 are aligned in words of 32 bits, 3 is free and not
> > > > + * recognized by IPU blocks.
> > > > + *
> > > > + * Taking the value of SENS_DATA_FORMAT and DATA_WIDTH, the CSI tries to
> > > > + * align (or rearrange) the sampled data to fit the IPU supported formats
> > > > + * as follows:
> > > > + * - CSI_SENS_CONF_DATA_FMT_RGB_YUV444: It consider the pixel as a sequence of
> > > > + *	3 components of width DATA_WIDTH aligning these to a 32 bit word.
> > > > + *	The CSI output in this case can feed other IPU blocks.
> > > > + * - CSI_SENS_CONF_DATA_FMT_YUV422: It consider the pixel as a sequence of
> > > > + *	2 components of width DATA_WIDTH were the first is the alternating U V
> > > 
> > > s/were/where/
> > > 
> > > > + *	components and the second is Y. It construct the YUV444 word repeating
> > > > + *	the previous U, V samples aligning the results to a 32 bit word.
> > > > + *	The CSI output in this case can feed other IPU blocks.
> > > > + * - CSI_SENS_CONF_DATA_FMT_BAYER: No rework is performed in this case.
> > > > + *	The sensor data is given as is, considering _every sample_ as a pixel
> > > > + *	data. This format (combined with the GENERIC IPU pixel formats) can
> > > > + *	carry all the other sensor pixel formats to the system memory.
> > > > + *	The CSI output in this case _can not_ feed other IPU blocks.
> > > > + */
> > > >  #define CSI_SENS_CONF_DATA_FMT_RGB_YUV444	(0UL << CSI_SENS_CONF_DATA_FMT_SHIFT)
> > > >  #define CSI_SENS_CONF_DATA_FMT_YUV422		(2UL << CSI_SENS_CONF_DATA_FMT_SHIFT)
> > > >  #define CSI_SENS_CONF_DATA_FMT_BAYER		(3UL << CSI_SENS_CONF_DATA_FMT_SHIFT)
> > > > @@ -323,14 +348,12 @@ static enum pixel_fmt fourcc_to_ipu_pix(__u32 fourcc)
> > > >  {
> > > >  	/* Add more formats as need arises and test possibilities appear... */
> > > >  	switch (fourcc) {
> > > > -	case V4L2_PIX_FMT_RGB565:
> > > > -		return IPU_PIX_FMT_RGB565;
> > > >  	case V4L2_PIX_FMT_RGB24:
> > > >  		return IPU_PIX_FMT_RGB24;
> > > > +	case V4L2_PIX_FMT_UYVY:
> > > > +		return IPU_PIX_FMT_UYVY;
> > > > +	case V4L2_PIX_FMT_RGB565:
> > > >  	case V4L2_PIX_FMT_RGB332:
> > > > -		return IPU_PIX_FMT_RGB332;
> > > > -	case V4L2_PIX_FMT_YUV422P:
> > > > -		return IPU_PIX_FMT_YVU422P;
> > > >  	default:
> > > >  		return IPU_PIX_FMT_GENERIC;
> > > >  	}
> > > 
> > > Ok, so far mx3_camera has only been used with mt9m022 and mt9t031 sensors 
> > > (from what I can see in the mainline), both are bayer. It can also work 
> > > with monochrome cameras, and that would be the IPU_PIX_FMT_GENERIC case 
> > > too. So, I wouldn't mind removing the rest, and only adding / fixing what 
> > > you've now tested / implemented with your omnivision sensor. If anyone is 
> > > using mx3_camera with any other formats and thinks, that they work - 
> > > please, shout now. I'll probably also post a separate mail with this 
> > > warning.
> > > 
> > > > @@ -358,9 +381,25 @@ static void mx3_videobuf_queue(struct videobuf_queue *vq,
> > > >  
> > > >  	/* This is the configuration of one sg-element */
> > > >  	video->out_pixel_fmt	= fourcc_to_ipu_pix(fourcc);
> > > > -	video->out_width	= icd->user_width;
> > > > -	video->out_height	= icd->user_height;
> > > > -	video->out_stride	= icd->user_width;
> > > > +
> > > > +	if (video->out_pixel_fmt == IPU_PIX_FMT_GENERIC) {
> > > > +		/*
> > > > +		 * IPU_PIX_FMT_GENERIC transport bytes, not pixels. So convert
> > > > +		 * video->out_width and stride to the correct unit.
> > > > +		 */
> > > > +		int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> > > > +						icd->current_fmt->host_fmt);
> > > > +		BUG_ON(bytes_per_line <= 0);
> > > > +
> > > > +		video->out_width	= bytes_per_line;
> > > > +		video->out_height	= icd->user_height;
> > > > +		video->out_stride	= bytes_per_line;
> > > > +	} else {
> > > > +		/* For IPU known formats the pixel unit is OK */
> > > > +		video->out_width	= icd->user_width;
> > > > +		video->out_height	= icd->user_height;
> > > > +		video->out_stride	= icd->user_width;
> > > > +	}
> > > >  
> > > >  #ifdef DEBUG
> > > >  	/* helps to see what DMA actually has written */
> > > > @@ -730,18 +769,68 @@ static int mx3_camera_get_formats(struct soc_camera_device *icd, unsigned int id
> > > >  	if (xlate) {
> > > >  		xlate->host_fmt	= fmt;
> > > >  		xlate->code	= code;
> > > > +		dev_dbg(dev, "Providing format %c%c%c%c in pass-through mode\n",
> > > > +			(xlate->host_fmt->fourcc >> (0*8)) & 0xFF,
> > > > +			(xlate->host_fmt->fourcc >> (1*8)) & 0xFF,
> > > > +			(xlate->host_fmt->fourcc >> (2*8)) & 0xFF,
> > > > +			(xlate->host_fmt->fourcc >> (3*8)) & 0xFF);
> > > >  		xlate++;
> > > > -		dev_dbg(dev, "Providing format %x in pass-through mode\n",
> > > > -			xlate->host_fmt->fourcc);
> > > 
> > > make it even simpler: s/xlate->host_fmt/fmt/g
> > > 
> > > >  	}
> > > >  
> > > >  	return formats;
> > > >  }
> > > >  
> > > > +static int samples_per_pixel(enum v4l2_mbus_pixelcode mcode)
> > > > +{
> > > > +	switch (mcode) {
> > > > +	case V4L2_MBUS_FMT_YUYV8_2X8:
> > > > +	case V4L2_MBUS_FMT_YVYU8_2X8:
> > > > +	case V4L2_MBUS_FMT_VYUY8_2X8:
> > > > +	case V4L2_MBUS_FMT_YVYU10_2X10:
> > > > +	case V4L2_MBUS_FMT_YUYV10_2X10:
> > > > +	case V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE:
> > > > +	case V4L2_MBUS_FMT_RGB444_2X8_PADHI_BE:
> > > > +	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE:
> > > > +	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE:
> > > > +	case V4L2_MBUS_FMT_RGB565_2X8_LE:
> > > > +	case V4L2_MBUS_FMT_RGB565_2X8_BE:
> > > > +	case V4L2_MBUS_FMT_BGR565_2X8_LE:
> > > > +	case V4L2_MBUS_FMT_BGR565_2X8_BE:
> > > > +		return 2;
> > > > +	case V4L2_MBUS_FMT_SBGGR8_1X8:
> > > > +	case V4L2_MBUS_FMT_SBGGR10_1X10:
> > > > +	case V4L2_MBUS_FMT_GREY8_1X8:
> > > > +	case V4L2_MBUS_FMT_Y10_1X10:
> > > > +	case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE:
> > > > +	case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE:
> > > 
> > > Are these two really 1 sample per pixel?
> > > 
> > > > +	case V4L2_MBUS_FMT_SGRBG8_1X8:
> > > > +		return 1;
> > > > +	default:
> > > > +		/* Add other pixel codes as needed */
> > > > +		return 0;
> > > > +	}
> > > > +}
> > > 
> > > Let's just do the following:
> > > 
> > > s32 soc_mbus_samples_per_pixel(const struct soc_mbus_pixelfmt *mf)
> > > {
> > > 	switch (mf->packing) {
> > > 	case SOC_MBUS_PACKING_NONE:
> > > 	case SOC_MBUS_PACKING_EXTEND16:
> > > 		return 1;
> > > 	case SOC_MBUS_PACKING_2X8_PADHI:
> > > 	case SOC_MBUS_PACKING_2X8_PADLO:
> > > 		return 2;
> > > 	}
> > > 	return -EINVAL;
> > > }
> > > EXPORT_SYMBOL(soc_mbus_samples_per_pixel);
> > > 
> > > in drivers/media/video/soc_mediabus.c, agree?
> > > 
> > > > +
> > > >  static void configure_geometry(struct mx3_camera_dev *mx3_cam,
> > > > -			       unsigned int width, unsigned int height)
> > > > +			       unsigned int width, unsigned int height,
> > > > +			       enum v4l2_mbus_pixelcode code)
> > > >  {
> > > >  	u32 ctrl, width_field, height_field;
> > > > +	const struct soc_mbus_pixelfmt *fmt;
> > > > +
> > > > +	fmt = soc_mbus_get_fmtdesc(code);
> > > > +	BUG_ON(!fmt);
> > > > +
> > > > +	if (fourcc_to_ipu_pix(fmt->fourcc) == IPU_PIX_FMT_GENERIC) {
> > > > +		/*
> > > > +		 * As we don't have an IPU native format, the CSI will be
> > > > +		 * configured to output BAYER and here we need to convert
> > > > +		 * geometry unit from pixels to samples.
> > > > +		 * TODO: Support vertical down sampling (YUV420)
> > > > +		 */
> > > > +		width = width * samples_per_pixel(code);
> > > > +		BUG_ON(!width);
> > > > +	}
> > > 
> > > 		width *= soc_mbus_samples_per_pixel(fmt);
> > > 		BUG_ON((int)width < 0);
> > > 
> > > >  
> > > >  	/* Setup frame size - this cannot be changed on-the-fly... */
> > > >  	width_field = width - 1;
> > > > @@ -850,7 +939,7 @@ static int mx3_camera_set_crop(struct soc_camera_device *icd,
> > > >  				return ret;
> > > >  		}
> > > >  
> > > > -		configure_geometry(mx3_cam, mf.width, mf.height);
> > > > +		configure_geometry(mx3_cam, mf.width, mf.height, mf.code);
> > > >  	}
> > > >  
> > > >  	dev_dbg(icd->dev.parent, "Sensor cropped %dx%d\n",
> > > > @@ -893,7 +982,7 @@ static int mx3_camera_set_fmt(struct soc_camera_device *icd,
> > > >  	 * mxc_v4l2_s_fmt()
> > > >  	 */
> > > >  
> > > > -	configure_geometry(mx3_cam, pix->width, pix->height);
> > > > +	configure_geometry(mx3_cam, pix->width, pix->height, xlate->code);
> > > >  
> > > >  	mf.width	= pix->width;
> > > >  	mf.height	= pix->height;
> > > > @@ -1112,10 +1201,15 @@ static int mx3_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
> > > >  		  (3 << CSI_SENS_CONF_DATA_FMT_SHIFT) |
> > > >  		  (3 << CSI_SENS_CONF_DATA_WIDTH_SHIFT));
> > > >  
> > > > -	/* TODO: Support RGB and YUV formats */
> > > > +	/* TODO: Support RGB_YUV444 formats */
> > > >  
> > > > -	/* This has been set in mx3_camera_activate(), but we clear it above */
> > > > -	sens_conf |= CSI_SENS_CONF_DATA_FMT_BAYER;
> > > > +	switch (xlate->code) {
> > > > +	case V4L2_MBUS_FMT_UYVY8_2X8:
> > > > +		sens_conf |= CSI_SENS_CONF_DATA_FMT_YUV422;
> > > > +		break;
> > > > +	default:
> > > > +		sens_conf |= CSI_SENS_CONF_DATA_FMT_BAYER;
> > > > +	}
> > > >  
> > > >  	if (common_flags & SOCAM_PCLK_SAMPLE_FALLING)
> > > >  		sens_conf |= 1 << CSI_SENS_CONF_PIX_CLK_POL_SHIFT;
> > > > -- 
> > > > 1.6.3.3
> > > 
> > > If after all the above changed your set up still works, we're cool!;)
> > > 
> > > Thanks
> > > Guennadi
> > > ---
> > > Guennadi Liakhovetski, Ph.D.
> > > Freelance Open-Source Software Developer
> > > http://www.open-technology.de/
> > > 
> > 
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> > http://www.open-technology.de/
> > 
> 
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Alberto!

        Be Persistent!
                - Greg Kroah-Hartman (FOSDEM 2010)

Re: [PATCH 2/3] mx3_camera: Support correctly the YUV222 and BAYER configurations of CSI

[Guennadi Liakhovetski]

On Mon, 3 Jan 2011, Alberto Panizzo wrote:

> On Thu, 2010-12-30 at 20:38 +0100, Guennadi Liakhovetski wrote:
> > On Sat, 18 Dec 2010, Guennadi Liakhovetski wrote:
> > 
> > > Alberto
> > > 
> > > it would be slowly on the time to address my comments and submit updates. 
> > > While at it, also, please update the subject - you probably meant "YUV422" 
> > > or "YUV444" there, also below:
> > 
> > Ok, I'm dropping this patch
> > 
> > Alberto, I've applied and pushed your other 2 patches from this series, 
> > but I've dropped this one. The reason is not (only), that you didn't reply 
> > to my two last mails with update-requests. But because of that I took the 
> > time today to look deeper into detail at this patch. And as a result, I 
> > don't think it is correct.
> > 
> > Currently the mx3_camera driver transfers data from video clients (camera 
> > sensors) only in one mode - as raw data, 1-to-1. This is extablished in 
> > the way, how it creates format translation tables during the initial 
> > negotiation with client drivers in mx3_camera_get_formats().
> > 
> > Your patch is trying to add support for specific modes on CSI, but is only 
> > doing this in the transfer part of the driver, and not in the negotiation 
> > part. So, if you really need native support for various pixel formats, 
> > this is a wrong way to do this. If you only want to transfer data from 
> > your sensor into RAM and the current driver is failing for you, then this 
> > is a wrong way to do this, and the bug has to be found and fixed, while 
> > maintaining the present pass-through only model.
> > 
> 
> This patch shows that IPU and CSI manage parameters in different 
> units. It shows that an unknown at the CSI pixel format, that require n
> bytes per pixel, have to be considered generic on the IPU side and the
> parameters of the DMA and CSI have to be set properly to support it.
> In this way also 10-bit wide pixels formats can be managed in pass
> through mode, setting properly the IPU and CSI parameters.
> 
> So, this patch shows that the CSI can manage successfully n-byte wide
> pixel codes (tested with n = 1,2) without breaking the old behaviour
> of providing 10-bit wide pixel formats with 8-bit wide ones.
> 
> The next step is to uniform also the pixel-code translations at this 
> type of management. Being able to capture real 10-bit wide samples.
> 
> This patch also, make use of a native functionality of the CSI: capture 
> a YUV422 format. 
> In this case the CSI convert this pixel format to YUV444 sent to the 
> BUS  and the IPU re-pack the YUV444 to YUV422 into the memory.
> 
> This shows how CSI and IPU manage formats different than the generic 
> one and open the way to understand how to support the communication
> between agents of the IPU encoding chain.
> 
> Maybe the last part is misleading you and can be dropped out from this 
> patch as an enhancement: the YUV422 interleaved format can be 
> successfully managed as CSI-BAYER/IPU-GENERIC one, the same as rgb565.
> Supporting the CSI-YUV422 is a plus only to show how the CSI works.

Let's try slowly again:

1. The current mainline driver doesn't work for you, right? What exactly 
is failing and how? What fourcc format?

2. Do you think, it would be possible to fix the driver to also support 
your use-case with the present generic / pass-through mode? Have you tried 
this? Could you try? That would be a bug-fix.

3. After the first two questions are answered, then we can think about 
extending the driver by adding native support forvarious specific formats.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2/3] mx3_camera: Support correctly the YUV222 and BAYER configurations of CSI

[Alberto Panizzo]

On Mon, 2011-01-03 at 20:37 +0100, Guennadi Liakhovetski wrote:
> On Mon, 3 Jan 2011, Alberto Panizzo wrote:
> 
> > On Thu, 2010-12-30 at 20:38 +0100, Guennadi Liakhovetski wrote:
> > > On Sat, 18 Dec 2010, Guennadi Liakhovetski wrote:
> > > 
> > > > Alberto
> > > > 
> > > > it would be slowly on the time to address my comments and submit updates. 
> > > > While at it, also, please update the subject - you probably meant "YUV422" 
> > > > or "YUV444" there, also below:
> > > 
> > > Ok, I'm dropping this patch
> > > 
> > > Alberto, I've applied and pushed your other 2 patches from this series, 
> > > but I've dropped this one. The reason is not (only), that you didn't reply 
> > > to my two last mails with update-requests. But because of that I took the 
> > > time today to look deeper into detail at this patch. And as a result, I 
> > > don't think it is correct.
> > > 
> > > Currently the mx3_camera driver transfers data from video clients (camera 
> > > sensors) only in one mode - as raw data, 1-to-1. This is extablished in 
> > > the way, how it creates format translation tables during the initial 
> > > negotiation with client drivers in mx3_camera_get_formats().
> > > 
> > > Your patch is trying to add support for specific modes on CSI, but is only 
> > > doing this in the transfer part of the driver, and not in the negotiation 
> > > part. So, if you really need native support for various pixel formats, 
> > > this is a wrong way to do this. If you only want to transfer data from 
> > > your sensor into RAM and the current driver is failing for you, then this 
> > > is a wrong way to do this, and the bug has to be found and fixed, while 
> > > maintaining the present pass-through only model.
> > > 
> > 
> > This patch shows that IPU and CSI manage parameters in different 
> > units. It shows that an unknown at the CSI pixel format, that require n
> > bytes per pixel, have to be considered generic on the IPU side and the
> > parameters of the DMA and CSI have to be set properly to support it.
> > In this way also 10-bit wide pixels formats can be managed in pass
> > through mode, setting properly the IPU and CSI parameters.
> > 
> > So, this patch shows that the CSI can manage successfully n-byte wide
> > pixel codes (tested with n = 1,2) without breaking the old behaviour
> > of providing 10-bit wide pixel formats with 8-bit wide ones.
> > 
> > The next step is to uniform also the pixel-code translations at this 
> > type of management. Being able to capture real 10-bit wide samples.
> > 
> > This patch also, make use of a native functionality of the CSI: capture 
> > a YUV422 format. 
> > In this case the CSI convert this pixel format to YUV444 sent to the 
> > BUS  and the IPU re-pack the YUV444 to YUV422 into the memory.
> > 
> > This shows how CSI and IPU manage formats different than the generic 
> > one and open the way to understand how to support the communication
> > between agents of the IPU encoding chain.
> > 
> > Maybe the last part is misleading you and can be dropped out from this 
> > patch as an enhancement: the YUV422 interleaved format can be 
> > successfully managed as CSI-BAYER/IPU-GENERIC one, the same as rgb565.
> > Supporting the CSI-YUV422 is a plus only to show how the CSI works.
> 
> Let's try slowly again:
> 
> 1. The current mainline driver doesn't work for you, right? What exactly 
> is failing and how? What fourcc format?

Yes, does not work for me for both YUV422 interleaved and rgb565.
What is captured is an image that have in the bottom half the violet 
color and in the upper half, half of the real image (divided 
vertically) with even rows on the left and odd rows on the right.


> 2. Do you think, it would be possible to fix the driver to also support 
> your use-case with the present generic / pass-through mode? Have you tried 
> this? Could you try? That would be a bug-fix.

Yes, this is the way I told about: dividing the geometry fixes from the 
special YUV422 support.

> 
> 3. After the first two questions are answered, then we can think about 
> extending the driver by adding native support forvarious specific formats.

Yes, sure. I'll try to explain better what the single patches are 
fixing, improving especially for these core functionality.


Best Regards,
Alberto!

Re: [PATCH 2/3] mx3_camera: Support correctly the YUV222 and BAYER configurations of CSI

[Alberto Panizzo]

On Mon, 2011-01-03 at 23:07 +0100, Alberto Panizzo wrote:
> On Mon, 2011-01-03 at 20:37 +0100, Guennadi Liakhovetski wrote:
> > On Mon, 3 Jan 2011, Alberto Panizzo wrote:
> > 
> > > On Thu, 2010-12-30 at 20:38 +0100, Guennadi Liakhovetski wrote:
> > > > On Sat, 18 Dec 2010, Guennadi Liakhovetski wrote:
> > > > 
> > > > > Alberto
> > > > > 
> > > > > it would be slowly on the time to address my comments and submit updates. 
> > > > > While at it, also, please update the subject - you probably meant "YUV422" 
> > > > > or "YUV444" there, also below:
> > > > 
> > > > Ok, I'm dropping this patch
> > > > 
> > > > Alberto, I've applied and pushed your other 2 patches from this series, 
> > > > but I've dropped this one. The reason is not (only), that you didn't reply 
> > > > to my two last mails with update-requests. But because of that I took the 
> > > > time today to look deeper into detail at this patch. And as a result, I 
> > > > don't think it is correct.
> > > > 
> > > > Currently the mx3_camera driver transfers data from video clients (camera 
> > > > sensors) only in one mode - as raw data, 1-to-1. This is extablished in 
> > > > the way, how it creates format translation tables during the initial 
> > > > negotiation with client drivers in mx3_camera_get_formats().
> > > > 
> > > > Your patch is trying to add support for specific modes on CSI, but is only 
> > > > doing this in the transfer part of the driver, and not in the negotiation 
> > > > part. So, if you really need native support for various pixel formats, 
> > > > this is a wrong way to do this. If you only want to transfer data from 
> > > > your sensor into RAM and the current driver is failing for you, then this 
> > > > is a wrong way to do this, and the bug has to be found and fixed, while 
> > > > maintaining the present pass-through only model.
> > > > 
> > > 
> > > This patch shows that IPU and CSI manage parameters in different 
> > > units. It shows that an unknown at the CSI pixel format, that require n
> > > bytes per pixel, have to be considered generic on the IPU side and the
> > > parameters of the DMA and CSI have to be set properly to support it.
> > > In this way also 10-bit wide pixels formats can be managed in pass
> > > through mode, setting properly the IPU and CSI parameters.
> > > 
> > > So, this patch shows that the CSI can manage successfully n-byte wide
> > > pixel codes (tested with n = 1,2) without breaking the old behaviour
> > > of providing 10-bit wide pixel formats with 8-bit wide ones.
> > > 
> > > The next step is to uniform also the pixel-code translations at this 
> > > type of management. Being able to capture real 10-bit wide samples.
> > > 
> > > This patch also, make use of a native functionality of the CSI: capture 
> > > a YUV422 format. 
> > > In this case the CSI convert this pixel format to YUV444 sent to the 
> > > BUS  and the IPU re-pack the YUV444 to YUV422 into the memory.
> > > 
> > > This shows how CSI and IPU manage formats different than the generic 
> > > one and open the way to understand how to support the communication
> > > between agents of the IPU encoding chain.
> > > 
> > > Maybe the last part is misleading you and can be dropped out from this 
> > > patch as an enhancement: the YUV422 interleaved format can be 
> > > successfully managed as CSI-BAYER/IPU-GENERIC one, the same as rgb565.
> > > Supporting the CSI-YUV422 is a plus only to show how the CSI works.
> > 
> > Let's try slowly again:
> > 
> > 1. The current mainline driver doesn't work for you, right? What exactly 
> > is failing and how? What fourcc format?
> 
> Yes, does not work for me for both YUV422 interleaved and rgb565.
> What is captured is an image that have in the bottom half the violet 
> color and in the upper half, half of the real image (divided 
> vertically) with even rows on the left and odd rows on the right.
> 
> 
> > 2. Do you think, it would be possible to fix the driver to also support 
> > your use-case with the present generic / pass-through mode? Have you tried 
> > this? Could you try? That would be a bug-fix.
> 
> Yes, this is the way I told about: dividing the geometry fixes from the 
> special YUV422 support.
> 
> > 
> > 3. After the first two questions are answered, then we can think about 
> > extending the driver by adding native support forvarious specific formats.
> 
> Yes, sure. I'll try to explain better what the single patches are 
> fixing, improving especially for these core functionality.

Sorry for lateness, some feedback now.
I was engaged on finding how to test BG10 (SBGGR10) to be sure that
also this format is usable with my patch since gstreamer support only 
BG81.
And the patch works! I will post it as soon as I can.

Alberto!

[PATCH 0/2] Fix the way mx3_camera manage non 8-bpp pixel formats

[Alberto Panizzo]

This patch series enable the mx3_camera driver to manage correctly
pixel formats like RGB565 UYVY and SBGGR10

This patch series applies to the staging/for_2.6.38-rc1
and I hope it will reach the mainline in one of the .38-rcN

Best regards,
Alberto!

[PATCH 1/2] soc_mediabus: export a useful method to obtain the number of samples that makes up a pixel format

[Alberto Panizzo]

Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
---
 drivers/media/video/soc_mediabus.c |   14 ++++++++++++++
 include/media/soc_mediabus.h       |    1 +
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/soc_mediabus.c b/drivers/media/video/soc_mediabus.c
index 9139121..5bba424 100644
--- a/drivers/media/video/soc_mediabus.c
+++ b/drivers/media/video/soc_mediabus.c
@@ -132,6 +132,20 @@ static const struct soc_mbus_pixelfmt mbus_fmt[] = {
 	},
 };
 
+s32 soc_mbus_samples_per_pixel(const struct soc_mbus_pixelfmt *mf)
+{
+	switch (mf->packing) {
+	case SOC_MBUS_PACKING_NONE:
+	case SOC_MBUS_PACKING_EXTEND16:
+		return 1;
+	case SOC_MBUS_PACKING_2X8_PADHI:
+	case SOC_MBUS_PACKING_2X8_PADLO:
+		return 2;
+	}
+	return -EINVAL;
+}
+EXPORT_SYMBOL(soc_mbus_samples_per_pixel);
+
 s32 soc_mbus_bytes_per_line(u32 width, const struct soc_mbus_pixelfmt *mf)
 {
 	switch (mf->packing) {
diff --git a/include/media/soc_mediabus.h b/include/media/soc_mediabus.h
index 037cd7b..f21cbd0 100644
--- a/include/media/soc_mediabus.h
+++ b/include/media/soc_mediabus.h
@@ -61,5 +61,6 @@ struct soc_mbus_pixelfmt {
 const struct soc_mbus_pixelfmt *soc_mbus_get_fmtdesc(
 	enum v4l2_mbus_pixelcode code);
 s32 soc_mbus_bytes_per_line(u32 width, const struct soc_mbus_pixelfmt *mf);
+s32 soc_mbus_samples_per_pixel(const struct soc_mbus_pixelfmt *mf);
 
 #endif
-- 
1.7.1

[PATCH 2/2] Fix capture issues for non 8-bit per pixel formats

[Alberto Panizzo]

If the camera was set to output formats like RGB565 YUYV or SBGGR10,
the resulting image was scrambled due to erroneous interpretations of
horizontal parameter's units.

This patch in fourcc_to_ipu_pix, eliminate also the pixel formats mappings
that, first are not used within mainline code and second, standing at
the datasheets, they will not work properly:

 The IPU internal bus support only the following data formatting
 (44.1.1.3 Data Flows and Formats):
  1 YUV 4:4:4 or RGB-8 bits per color component
  2 YUV 4:4:4 or RGB-10 bits per color component
  3 Generic data (from sensor to the system memory only)

 And format conversions are done:
  - from memory: de-packing from other formats to IPU supported ones
  - to memory: packing in the inverse order.

 So, assigning a packing/depacking strategy to the IPU for that formats
 will produce a packing to memory and not the inverse.

In the end in mx3_camera_get_formats, is fixed an erroneous debug message
that try to shows info from an invalid xlate pointer.

Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
---
 drivers/media/video/mx3_camera.c |   66 +++++++++++++++++++++++++++++--------
 1 files changed, 51 insertions(+), 15 deletions(-)

diff --git a/drivers/media/video/mx3_camera.c b/drivers/media/video/mx3_camera.c
index b9cb4a4..6b9d019 100644
--- a/drivers/media/video/mx3_camera.c
+++ b/drivers/media/video/mx3_camera.c
@@ -324,14 +324,10 @@ static enum pixel_fmt fourcc_to_ipu_pix(__u32 fourcc)
 {
 	/* Add more formats as need arises and test possibilities appear... */
 	switch (fourcc) {
-	case V4L2_PIX_FMT_RGB565:
-		return IPU_PIX_FMT_RGB565;
 	case V4L2_PIX_FMT_RGB24:
 		return IPU_PIX_FMT_RGB24;
-	case V4L2_PIX_FMT_RGB332:
-		return IPU_PIX_FMT_RGB332;
-	case V4L2_PIX_FMT_YUV422P:
-		return IPU_PIX_FMT_YVU422P;
+	case V4L2_PIX_FMT_UYVY:
+	case V4L2_PIX_FMT_RGB565:
 	default:
 		return IPU_PIX_FMT_GENERIC;
 	}
@@ -359,9 +355,31 @@ static void mx3_videobuf_queue(struct videobuf_queue *vq,
 
 	/* This is the configuration of one sg-element */
 	video->out_pixel_fmt	= fourcc_to_ipu_pix(fourcc);
-	video->out_width	= icd->user_width;
-	video->out_height	= icd->user_height;
-	video->out_stride	= icd->user_width;
+
+	if (video->out_pixel_fmt == IPU_PIX_FMT_GENERIC) {
+		/*
+		 * If the IPU DMA channel is configured to transport
+		 * generic 8-bit data, we have to set up correctly the
+		 * geometry parameters upon the current pixel format.
+		 * So, since the DMA horizontal parameters are expressed
+		 * in bytes not pixels, convert these in the right unit.
+		 */
+		int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
+						icd->current_fmt->host_fmt);
+		BUG_ON(bytes_per_line <= 0);
+
+		video->out_width	= bytes_per_line;
+		video->out_height	= icd->user_height;
+		video->out_stride	= bytes_per_line;
+	} else {
+		/*
+		 * For IPU known formats the pixel unit will be managed
+		 * successfully by the IPU code
+		 */
+		video->out_width	= icd->user_width;
+		video->out_height	= icd->user_height;
+		video->out_stride	= icd->user_width;
+	}
 
 #ifdef DEBUG
 	/* helps to see what DMA actually has written */
@@ -734,18 +752,36 @@ static int mx3_camera_get_formats(struct soc_camera_device *icd, unsigned int id
 	if (xlate) {
 		xlate->host_fmt	= fmt;
 		xlate->code	= code;
+		dev_dbg(dev, "Providing format %c%c%c%c in pass-through mode\n",
+			(fmt->fourcc >> (0*8)) & 0xFF,
+			(fmt->fourcc >> (1*8)) & 0xFF,
+			(fmt->fourcc >> (2*8)) & 0xFF,
+			(fmt->fourcc >> (3*8)) & 0xFF);
 		xlate++;
-		dev_dbg(dev, "Providing format %x in pass-through mode\n",
-			xlate->host_fmt->fourcc);
 	}
 
 	return formats;
 }
 
 static void configure_geometry(struct mx3_camera_dev *mx3_cam,
-			       unsigned int width, unsigned int height)
+			       unsigned int width, unsigned int height,
+			       enum v4l2_mbus_pixelcode code)
 {
 	u32 ctrl, width_field, height_field;
+	const struct soc_mbus_pixelfmt *fmt;
+
+	fmt = soc_mbus_get_fmtdesc(code);
+	BUG_ON(!fmt);
+
+	if (fourcc_to_ipu_pix(fmt->fourcc) == IPU_PIX_FMT_GENERIC) {
+		/*
+		 * As the CSI will be configured to output BAYER, here
+		 * the width parameter count the number of samples to
+		 * capture to complete the whole image width.
+		 */
+		width *= soc_mbus_samples_per_pixel(fmt);
+		BUG_ON(!width);
+	}
 
 	/* Setup frame size - this cannot be changed on-the-fly... */
 	width_field = width - 1;
@@ -854,7 +890,7 @@ static int mx3_camera_set_crop(struct soc_camera_device *icd,
 				return ret;
 		}
 
-		configure_geometry(mx3_cam, mf.width, mf.height);
+		configure_geometry(mx3_cam, mf.width, mf.height, mf.code);
 	}
 
 	dev_dbg(icd->dev.parent, "Sensor cropped %dx%d\n",
@@ -897,7 +933,7 @@ static int mx3_camera_set_fmt(struct soc_camera_device *icd,
 	 * mxc_v4l2_s_fmt()
 	 */
 
-	configure_geometry(mx3_cam, pix->width, pix->height);
+	configure_geometry(mx3_cam, pix->width, pix->height, xlate->code);
 
 	mf.width	= pix->width;
 	mf.height	= pix->height;
@@ -1152,7 +1188,7 @@ static int mx3_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 
 	csi_reg_write(mx3_cam, sens_conf | dw, CSI_SENS_CONF);
 
-	dev_dbg(dev, "Set SENS_CONF to %x\n", sens_conf | dw);
+	dev_info(dev, "Set SENS_CONF to %x\n", sens_conf | dw);
 
 	return 0;
 }
-- 
1.7.1

Re: [PATCH 2/2] Fix capture issues for non 8-bit per pixel formats

[Guennadi Liakhovetski]

On Wed, 12 Jan 2011, Alberto Panizzo wrote:

> If the camera was set to output formats like RGB565 YUYV or SBGGR10,
> the resulting image was scrambled due to erroneous interpretations of
> horizontal parameter's units.
> 
> This patch in fourcc_to_ipu_pix, eliminate also the pixel formats mappings
> that, first are not used within mainline code and second, standing at
> the datasheets, they will not work properly:
> 
>  The IPU internal bus support only the following data formatting
>  (44.1.1.3 Data Flows and Formats):
>   1 YUV 4:4:4 or RGB-8 bits per color component
>   2 YUV 4:4:4 or RGB-10 bits per color component
>   3 Generic data (from sensor to the system memory only)
> 
>  And format conversions are done:
>   - from memory: de-packing from other formats to IPU supported ones

did you mean "unpacking" (also below)?

>   - to memory: packing in the inverse order.
> 
>  So, assigning a packing/depacking strategy to the IPU for that formats
>  will produce a packing to memory and not the inverse.
> 
> In the end in mx3_camera_get_formats, is fixed an erroneous debug message
> that try to shows info from an invalid xlate pointer.
> 
> Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
> ---
>  drivers/media/video/mx3_camera.c |   66 +++++++++++++++++++++++++++++--------
>  1 files changed, 51 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/video/mx3_camera.c b/drivers/media/video/mx3_camera.c
> index b9cb4a4..6b9d019 100644
> --- a/drivers/media/video/mx3_camera.c
> +++ b/drivers/media/video/mx3_camera.c
> @@ -324,14 +324,10 @@ static enum pixel_fmt fourcc_to_ipu_pix(__u32 fourcc)
>  {
>  	/* Add more formats as need arises and test possibilities appear... */
>  	switch (fourcc) {
> -	case V4L2_PIX_FMT_RGB565:
> -		return IPU_PIX_FMT_RGB565;
>  	case V4L2_PIX_FMT_RGB24:
>  		return IPU_PIX_FMT_RGB24;
> -	case V4L2_PIX_FMT_RGB332:
> -		return IPU_PIX_FMT_RGB332;
> -	case V4L2_PIX_FMT_YUV422P:
> -		return IPU_PIX_FMT_YVU422P;
> +	case V4L2_PIX_FMT_UYVY:
> +	case V4L2_PIX_FMT_RGB565:
>  	default:
>  		return IPU_PIX_FMT_GENERIC;
>  	}
> @@ -359,9 +355,31 @@ static void mx3_videobuf_queue(struct videobuf_queue *vq,
>  
>  	/* This is the configuration of one sg-element */
>  	video->out_pixel_fmt	= fourcc_to_ipu_pix(fourcc);
> -	video->out_width	= icd->user_width;
> -	video->out_height	= icd->user_height;
> -	video->out_stride	= icd->user_width;
> +
> +	if (video->out_pixel_fmt == IPU_PIX_FMT_GENERIC) {
> +		/*
> +		 * If the IPU DMA channel is configured to transport
> +		 * generic 8-bit data, we have to set up correctly the
> +		 * geometry parameters upon the current pixel format.
> +		 * So, since the DMA horizontal parameters are expressed
> +		 * in bytes not pixels, convert these in the right unit.
> +		 */
> +		int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> +						icd->current_fmt->host_fmt);
> +		BUG_ON(bytes_per_line <= 0);
> +
> +		video->out_width	= bytes_per_line;
> +		video->out_height	= icd->user_height;
> +		video->out_stride	= bytes_per_line;
> +	} else {
> +		/*
> +		 * For IPU known formats the pixel unit will be managed
> +		 * successfully by the IPU code
> +		 */
> +		video->out_width	= icd->user_width;
> +		video->out_height	= icd->user_height;
> +		video->out_stride	= icd->user_width;
> +	}
>  
>  #ifdef DEBUG
>  	/* helps to see what DMA actually has written */
> @@ -734,18 +752,36 @@ static int mx3_camera_get_formats(struct soc_camera_device *icd, unsigned int id
>  	if (xlate) {
>  		xlate->host_fmt	= fmt;
>  		xlate->code	= code;
> +		dev_dbg(dev, "Providing format %c%c%c%c in pass-through mode\n",
> +			(fmt->fourcc >> (0*8)) & 0xFF,
> +			(fmt->fourcc >> (1*8)) & 0xFF,
> +			(fmt->fourcc >> (2*8)) & 0xFF,
> +			(fmt->fourcc >> (3*8)) & 0xFF);
>  		xlate++;
> -		dev_dbg(dev, "Providing format %x in pass-through mode\n",
> -			xlate->host_fmt->fourcc);
>  	}
>  
>  	return formats;
>  }
>  
>  static void configure_geometry(struct mx3_camera_dev *mx3_cam,
> -			       unsigned int width, unsigned int height)
> +			       unsigned int width, unsigned int height,
> +			       enum v4l2_mbus_pixelcode code)
>  {
>  	u32 ctrl, width_field, height_field;
> +	const struct soc_mbus_pixelfmt *fmt;
> +
> +	fmt = soc_mbus_get_fmtdesc(code);
> +	BUG_ON(!fmt);
> +
> +	if (fourcc_to_ipu_pix(fmt->fourcc) == IPU_PIX_FMT_GENERIC) {
> +		/*
> +		 * As the CSI will be configured to output BAYER, here
> +		 * the width parameter count the number of samples to
> +		 * capture to complete the whole image width.
> +		 */
> +		width *= soc_mbus_samples_per_pixel(fmt);

Wouldn't

+		width = soc_mbus_bytes_per_line(width, fmt);

produce the same result here? Then we wouldn't need your patch 1/2 at all.

> +		BUG_ON(!width);
> +	}
>  
>  	/* Setup frame size - this cannot be changed on-the-fly... */
>  	width_field = width - 1;
> @@ -854,7 +890,7 @@ static int mx3_camera_set_crop(struct soc_camera_device *icd,
>  				return ret;
>  		}
>  
> -		configure_geometry(mx3_cam, mf.width, mf.height);
> +		configure_geometry(mx3_cam, mf.width, mf.height, mf.code);
>  	}
>  
>  	dev_dbg(icd->dev.parent, "Sensor cropped %dx%d\n",
> @@ -897,7 +933,7 @@ static int mx3_camera_set_fmt(struct soc_camera_device *icd,
>  	 * mxc_v4l2_s_fmt()
>  	 */
>  
> -	configure_geometry(mx3_cam, pix->width, pix->height);
> +	configure_geometry(mx3_cam, pix->width, pix->height, xlate->code);
>  
>  	mf.width	= pix->width;
>  	mf.height	= pix->height;
> @@ -1152,7 +1188,7 @@ static int mx3_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
>  
>  	csi_reg_write(mx3_cam, sens_conf | dw, CSI_SENS_CONF);
>  
> -	dev_dbg(dev, "Set SENS_CONF to %x\n", sens_conf | dw);
> +	dev_info(dev, "Set SENS_CONF to %x\n", sens_conf | dw);

This was unintentional, right?

>  
>  	return 0;
>  }
> -- 
> 1.7.1

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2/2] Fix capture issues for non 8-bit per pixel formats

[Alberto Panizzo]

On Sat, 2011-01-15 at 22:35 +0100, Guennadi Liakhovetski wrote:
> On Wed, 12 Jan 2011, Alberto Panizzo wrote:
> 
> > If the camera was set to output formats like RGB565 YUYV or SBGGR10,
> > the resulting image was scrambled due to erroneous interpretations of
> > horizontal parameter's units.
> > 
> > This patch in fourcc_to_ipu_pix, eliminate also the pixel formats mappings
> > that, first are not used within mainline code and second, standing at
> > the datasheets, they will not work properly:
> > 
> >  The IPU internal bus support only the following data formatting
> >  (44.1.1.3 Data Flows and Formats):
> >   1 YUV 4:4:4 or RGB-8 bits per color component
> >   2 YUV 4:4:4 or RGB-10 bits per color component
> >   3 Generic data (from sensor to the system memory only)
> > 
> >  And format conversions are done:
> >   - from memory: de-packing from other formats to IPU supported ones
> 
> did you mean "unpacking" (also below)?
> 
> >   - to memory: packing in the inverse order.

I mean:

 - from memory: unpacking from other formats to IPU supported ones
 - to memory: packing in the inverse order.

To indicate the oneway direction of packing and unpacking. I'll
resend the patch with the updated comment.

> > 
> >  So, assigning a packing/depacking strategy to the IPU for that formats
> >  will produce a packing to memory and not the inverse.
> > 
> > In the end in mx3_camera_get_formats, is fixed an erroneous debug message
> > that try to shows info from an invalid xlate pointer.
> > 
> > Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
> > ---
> >  drivers/media/video/mx3_camera.c |   66 +++++++++++++++++++++++++++++--------
> >  1 files changed, 51 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/media/video/mx3_camera.c b/drivers/media/video/mx3_camera.c
> > index b9cb4a4..6b9d019 100644
> > --- a/drivers/media/video/mx3_camera.c
> > +++ b/drivers/media/video/mx3_camera.c
> > @@ -324,14 +324,10 @@ static enum pixel_fmt fourcc_to_ipu_pix(__u32 fourcc)
> >  {
> >  	/* Add more formats as need arises and test possibilities appear... */
> >  	switch (fourcc) {
> > -	case V4L2_PIX_FMT_RGB565:
> > -		return IPU_PIX_FMT_RGB565;
> >  	case V4L2_PIX_FMT_RGB24:
> >  		return IPU_PIX_FMT_RGB24;
> > -	case V4L2_PIX_FMT_RGB332:
> > -		return IPU_PIX_FMT_RGB332;
> > -	case V4L2_PIX_FMT_YUV422P:
> > -		return IPU_PIX_FMT_YVU422P;
> > +	case V4L2_PIX_FMT_UYVY:
> > +	case V4L2_PIX_FMT_RGB565:
> >  	default:
> >  		return IPU_PIX_FMT_GENERIC;
> >  	}
> > @@ -359,9 +355,31 @@ static void mx3_videobuf_queue(struct videobuf_queue *vq,
> >  
> >  	/* This is the configuration of one sg-element */
> >  	video->out_pixel_fmt	= fourcc_to_ipu_pix(fourcc);
> > -	video->out_width	= icd->user_width;
> > -	video->out_height	= icd->user_height;
> > -	video->out_stride	= icd->user_width;
> > +
> > +	if (video->out_pixel_fmt == IPU_PIX_FMT_GENERIC) {
> > +		/*
> > +		 * If the IPU DMA channel is configured to transport
> > +		 * generic 8-bit data, we have to set up correctly the
> > +		 * geometry parameters upon the current pixel format.
> > +		 * So, since the DMA horizontal parameters are expressed
> > +		 * in bytes not pixels, convert these in the right unit.
> > +		 */
> > +		int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> > +						icd->current_fmt->host_fmt);
> > +		BUG_ON(bytes_per_line <= 0);
> > +
> > +		video->out_width	= bytes_per_line;
> > +		video->out_height	= icd->user_height;
> > +		video->out_stride	= bytes_per_line;
> > +	} else {
> > +		/*
> > +		 * For IPU known formats the pixel unit will be managed
> > +		 * successfully by the IPU code
> > +		 */
> > +		video->out_width	= icd->user_width;
> > +		video->out_height	= icd->user_height;
> > +		video->out_stride	= icd->user_width;
> > +	}
> >  
> >  #ifdef DEBUG
> >  	/* helps to see what DMA actually has written */
> > @@ -734,18 +752,36 @@ static int mx3_camera_get_formats(struct soc_camera_device *icd, unsigned int id
> >  	if (xlate) {
> >  		xlate->host_fmt	= fmt;
> >  		xlate->code	= code;
> > +		dev_dbg(dev, "Providing format %c%c%c%c in pass-through mode\n",
> > +			(fmt->fourcc >> (0*8)) & 0xFF,
> > +			(fmt->fourcc >> (1*8)) & 0xFF,
> > +			(fmt->fourcc >> (2*8)) & 0xFF,
> > +			(fmt->fourcc >> (3*8)) & 0xFF);
> >  		xlate++;
> > -		dev_dbg(dev, "Providing format %x in pass-through mode\n",
> > -			xlate->host_fmt->fourcc);
> >  	}
> >  
> >  	return formats;
> >  }
> >  
> >  static void configure_geometry(struct mx3_camera_dev *mx3_cam,
> > -			       unsigned int width, unsigned int height)
> > +			       unsigned int width, unsigned int height,
> > +			       enum v4l2_mbus_pixelcode code)
> >  {
> >  	u32 ctrl, width_field, height_field;
> > +	const struct soc_mbus_pixelfmt *fmt;
> > +
> > +	fmt = soc_mbus_get_fmtdesc(code);
> > +	BUG_ON(!fmt);
> > +
> > +	if (fourcc_to_ipu_pix(fmt->fourcc) == IPU_PIX_FMT_GENERIC) {
> > +		/*
> > +		 * As the CSI will be configured to output BAYER, here
> > +		 * the width parameter count the number of samples to
> > +		 * capture to complete the whole image width.
> > +		 */
> > +		width *= soc_mbus_samples_per_pixel(fmt);
> 
> Wouldn't
> 
> +		width = soc_mbus_bytes_per_line(width, fmt);
> 
> produce the same result here? Then we wouldn't need your patch 1/2 at all.

No, because CSI units are number of samples. This differs from number of
bytes for formats like SBGGR10 (EXTEND16 formats).
I tested this working.

> 
> > +		BUG_ON(!width);
> > +	}
> >  
> >  	/* Setup frame size - this cannot be changed on-the-fly... */
> >  	width_field = width - 1;
> > @@ -854,7 +890,7 @@ static int mx3_camera_set_crop(struct soc_camera_device *icd,
> >  				return ret;
> >  		}
> >  
> > -		configure_geometry(mx3_cam, mf.width, mf.height);
> > +		configure_geometry(mx3_cam, mf.width, mf.height, mf.code);
> >  	}
> >  
> >  	dev_dbg(icd->dev.parent, "Sensor cropped %dx%d\n",
> > @@ -897,7 +933,7 @@ static int mx3_camera_set_fmt(struct soc_camera_device *icd,
> >  	 * mxc_v4l2_s_fmt()
> >  	 */
> >  
> > -	configure_geometry(mx3_cam, pix->width, pix->height);
> > +	configure_geometry(mx3_cam, pix->width, pix->height, xlate->code);
> >  
> >  	mf.width	= pix->width;
> >  	mf.height	= pix->height;
> > @@ -1152,7 +1188,7 @@ static int mx3_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
> >  
> >  	csi_reg_write(mx3_cam, sens_conf | dw, CSI_SENS_CONF);
> >  
> > -	dev_dbg(dev, "Set SENS_CONF to %x\n", sens_conf | dw);
> > +	dev_info(dev, "Set SENS_CONF to %x\n", sens_conf | dw);
> 
> This was unintentional, right?

Yes, unintentional, thanks!!

I'll resend the patch right now.

Best Regards,
Alberto!

[PATCH 2/2 v2] Fix capture issues for non 8-bit per pixel formats

[Alberto Panizzo]

If the camera was set to output formats like RGB565 YUYV or SBGGR10,
the resulting image was scrambled due to erroneous interpretations of
horizontal parameter's units.

This patch in fourcc_to_ipu_pix, eliminate also the pixel formats mappings
that, first are not used within mainline code and second, standing at
the datasheets, they will not work properly:

 The IPU internal bus support only the following data formatting
 (44.1.1.3 Data Flows and Formats):
  1 YUV 4:4:4 or RGB-8 bits per color component
  2 YUV 4:4:4 or RGB-10 bits per color component
  3 Generic data (from sensor to the system memory only)

 And format conversions are done:
  - from memory: unpacking from other formats to IPU supported ones
  - to memory: packing in the inverse order.

 So, assigning a packing/depacking strategy to the IPU for that formats
 will produce a packing to memory and not the inverse.

Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
---
 drivers/media/video/mx3_camera.c |   64 +++++++++++++++++++++++++++++--------
 1 files changed, 50 insertions(+), 14 deletions(-)

diff --git a/drivers/media/video/mx3_camera.c b/drivers/media/video/mx3_camera.c
index b9cb4a4..9391006 100644
--- a/drivers/media/video/mx3_camera.c
+++ b/drivers/media/video/mx3_camera.c
@@ -324,14 +324,10 @@ static enum pixel_fmt fourcc_to_ipu_pix(__u32 fourcc)
 {
 	/* Add more formats as need arises and test possibilities appear... */
 	switch (fourcc) {
-	case V4L2_PIX_FMT_RGB565:
-		return IPU_PIX_FMT_RGB565;
 	case V4L2_PIX_FMT_RGB24:
 		return IPU_PIX_FMT_RGB24;
-	case V4L2_PIX_FMT_RGB332:
-		return IPU_PIX_FMT_RGB332;
-	case V4L2_PIX_FMT_YUV422P:
-		return IPU_PIX_FMT_YVU422P;
+	case V4L2_PIX_FMT_UYVY:
+	case V4L2_PIX_FMT_RGB565:
 	default:
 		return IPU_PIX_FMT_GENERIC;
 	}
@@ -359,9 +355,31 @@ static void mx3_videobuf_queue(struct videobuf_queue *vq,
 
 	/* This is the configuration of one sg-element */
 	video->out_pixel_fmt	= fourcc_to_ipu_pix(fourcc);
-	video->out_width	= icd->user_width;
-	video->out_height	= icd->user_height;
-	video->out_stride	= icd->user_width;
+
+	if (video->out_pixel_fmt == IPU_PIX_FMT_GENERIC) {
+		/*
+		 * If the IPU DMA channel is configured to transport
+		 * generic 8-bit data, we have to set up correctly the
+		 * geometry parameters upon the current pixel format.
+		 * So, since the DMA horizontal parameters are expressed
+		 * in bytes not pixels, convert these in the right unit.
+		 */
+		int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
+						icd->current_fmt->host_fmt);
+		BUG_ON(bytes_per_line <= 0);
+
+		video->out_width	= bytes_per_line;
+		video->out_height	= icd->user_height;
+		video->out_stride	= bytes_per_line;
+	} else {
+		/*
+		 * For IPU known formats the pixel unit will be managed
+		 * successfully by the IPU code
+		 */
+		video->out_width	= icd->user_width;
+		video->out_height	= icd->user_height;
+		video->out_stride	= icd->user_width;
+	}
 
 #ifdef DEBUG
 	/* helps to see what DMA actually has written */
@@ -734,18 +752,36 @@ static int mx3_camera_get_formats(struct soc_camera_device *icd, unsigned int id
 	if (xlate) {
 		xlate->host_fmt	= fmt;
 		xlate->code	= code;
+		dev_dbg(dev, "Providing format %c%c%c%c in pass-through mode\n",
+			(fmt->fourcc >> (0*8)) & 0xFF,
+			(fmt->fourcc >> (1*8)) & 0xFF,
+			(fmt->fourcc >> (2*8)) & 0xFF,
+			(fmt->fourcc >> (3*8)) & 0xFF);
 		xlate++;
-		dev_dbg(dev, "Providing format %x in pass-through mode\n",
-			xlate->host_fmt->fourcc);
 	}
 
 	return formats;
 }
 
 static void configure_geometry(struct mx3_camera_dev *mx3_cam,
-			       unsigned int width, unsigned int height)
+			       unsigned int width, unsigned int height,
+			       enum v4l2_mbus_pixelcode code)
 {
 	u32 ctrl, width_field, height_field;
+	const struct soc_mbus_pixelfmt *fmt;
+
+	fmt = soc_mbus_get_fmtdesc(code);
+	BUG_ON(!fmt);
+
+	if (fourcc_to_ipu_pix(fmt->fourcc) == IPU_PIX_FMT_GENERIC) {
+		/*
+		 * As the CSI will be configured to output BAYER, here
+		 * the width parameter count the number of samples to
+		 * capture to complete the whole image width.
+		 */
+		width *= soc_mbus_samples_per_pixel(fmt);
+		BUG_ON(!width);
+	}
 
 	/* Setup frame size - this cannot be changed on-the-fly... */
 	width_field = width - 1;
@@ -854,7 +890,7 @@ static int mx3_camera_set_crop(struct soc_camera_device *icd,
 				return ret;
 		}
 
-		configure_geometry(mx3_cam, mf.width, mf.height);
+		configure_geometry(mx3_cam, mf.width, mf.height, mf.code);
 	}
 
 	dev_dbg(icd->dev.parent, "Sensor cropped %dx%d\n",
@@ -897,7 +933,7 @@ static int mx3_camera_set_fmt(struct soc_camera_device *icd,
 	 * mxc_v4l2_s_fmt()
 	 */
 
-	configure_geometry(mx3_cam, pix->width, pix->height);
+	configure_geometry(mx3_cam, pix->width, pix->height, xlate->code);
 
 	mf.width	= pix->width;
 	mf.height	= pix->height;
-- 
1.7.1

Re: [PATCH 2/3] mx3_camera: Support correctly the YUV222 and BAYER configurations of CSI

[Alberto Panizzo]

On Wed, 2010-12-01 at 19:54 +0100, Guennadi Liakhovetski wrote:
> On Sun, 28 Nov 2010, Alberto Panizzo wrote:
> 
> > This patch is tested and works with the OV2640 camera that output
> > YUV422 (UYVY) and RGB565 data.
> > 
> > The YUV422 format is managed to be converted in IPU internal YUV444 format
> > so this stream could be used in the future to feed directly other IPU
> > blocks.
> > The RGB565 format is managed as GENERIC and can be moved only from CSI
> > to memory.
> > 
> > Signed-off-by: Alberto Panizzo <maramaopercheseimorto@gmail.com>
> > ---
> > 
> > Before applying, please give me feedback if this break in some way other
> > pixel formats!
> > 
> > 
> >  drivers/media/video/mx3_camera.c |  126 +++++++++++++++++++++++++++++++++-----
> >  1 files changed, 110 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/media/video/mx3_camera.c b/drivers/media/video/mx3_camera.c
> > index 29c5fc3..6811d6f 100644
> > --- a/drivers/media/video/mx3_camera.c
> > +++ b/drivers/media/video/mx3_camera.c
> > @@ -55,6 +55,31 @@
> >  #define CSI_SENS_CONF_EXT_VSYNC_SHIFT		15
> >  #define CSI_SENS_CONF_DIVRATIO_SHIFT		16
> >  
> > +/*
> > + * IPU support the following data formatting (44.1.1.3 Data Flows and Formats):
> > + * 1 YUV 4:4:4 or RGB—8 bits per color component
> > + * 2 YUV 4:4:4 or RGB—10 bits per color component
> > + * 3 Generic data (from sensor to the system memory only)
> > + * The formats 1 and 2 are aligned in words of 32 bits, 3 is free and not
> > + * recognized by IPU blocks.
> > + *
> > + * Taking the value of SENS_DATA_FORMAT and DATA_WIDTH, the CSI tries to
> > + * align (or rearrange) the sampled data to fit the IPU supported formats
> > + * as follows:
> > + * - CSI_SENS_CONF_DATA_FMT_RGB_YUV444: It consider the pixel as a sequence of
> > + *	3 components of width DATA_WIDTH aligning these to a 32 bit word.
> > + *	The CSI output in this case can feed other IPU blocks.
> > + * - CSI_SENS_CONF_DATA_FMT_YUV422: It consider the pixel as a sequence of
> > + *	2 components of width DATA_WIDTH were the first is the alternating U V
> 
> s/were/where/
> 
> > + *	components and the second is Y. It construct the YUV444 word repeating
> > + *	the previous U, V samples aligning the results to a 32 bit word.
> > + *	The CSI output in this case can feed other IPU blocks.
> > + * - CSI_SENS_CONF_DATA_FMT_BAYER: No rework is performed in this case.
> > + *	The sensor data is given as is, considering _every sample_ as a pixel
> > + *	data. This format (combined with the GENERIC IPU pixel formats) can
> > + *	carry all the other sensor pixel formats to the system memory.
> > + *	The CSI output in this case _can not_ feed other IPU blocks.
> > + */
> >  #define CSI_SENS_CONF_DATA_FMT_RGB_YUV444	(0UL << CSI_SENS_CONF_DATA_FMT_SHIFT)
> >  #define CSI_SENS_CONF_DATA_FMT_YUV422		(2UL << CSI_SENS_CONF_DATA_FMT_SHIFT)
> >  #define CSI_SENS_CONF_DATA_FMT_BAYER		(3UL << CSI_SENS_CONF_DATA_FMT_SHIFT)
> > @@ -323,14 +348,12 @@ static enum pixel_fmt fourcc_to_ipu_pix(__u32 fourcc)
> >  {
> >  	/* Add more formats as need arises and test possibilities appear... */
> >  	switch (fourcc) {
> > -	case V4L2_PIX_FMT_RGB565:
> > -		return IPU_PIX_FMT_RGB565;
> >  	case V4L2_PIX_FMT_RGB24:
> >  		return IPU_PIX_FMT_RGB24;
> > +	case V4L2_PIX_FMT_UYVY:
> > +		return IPU_PIX_FMT_UYVY;
> > +	case V4L2_PIX_FMT_RGB565:
> >  	case V4L2_PIX_FMT_RGB332:
> > -		return IPU_PIX_FMT_RGB332;
> > -	case V4L2_PIX_FMT_YUV422P:
> > -		return IPU_PIX_FMT_YVU422P;
> >  	default:
> >  		return IPU_PIX_FMT_GENERIC;
> >  	}
> 
> Ok, so far mx3_camera has only been used with mt9m022 and mt9t031 sensors 
> (from what I can see in the mainline), both are bayer. It can also work 
> with monochrome cameras, and that would be the IPU_PIX_FMT_GENERIC case 
> too. So, I wouldn't mind removing the rest, and only adding / fixing what 
> you've now tested / implemented with your omnivision sensor. If anyone is 
> using mx3_camera with any other formats and thinks, that they work - 
> please, shout now. I'll probably also post a separate mail with this 
> warning.

This change follows what I described upward. The CSI can manage only RGB 8 bits
per component or YUV 4:4:4. Marking the stream with other types (take rgb565 as
example) produce a packing-to-memory task from what the CSI is setup to transfer
(RGB-8 or YUV4:4:4) to the IPU pixel format chosen. So the only way to manage
pixel formats that are not native for the CSI is through: BAYER format for CSI
and IPU_PIX_FMT_GENERIC for IPU.

> 
> > @@ -358,9 +381,25 @@ static void mx3_videobuf_queue(struct videobuf_queue *vq,
> >  
> >  	/* This is the configuration of one sg-element */
> >  	video->out_pixel_fmt	= fourcc_to_ipu_pix(fourcc);
> > -	video->out_width	= icd->user_width;
> > -	video->out_height	= icd->user_height;
> > -	video->out_stride	= icd->user_width;
> > +
> > +	if (video->out_pixel_fmt == IPU_PIX_FMT_GENERIC) {
> > +		/*
> > +		 * IPU_PIX_FMT_GENERIC transport bytes, not pixels. So convert
> > +		 * video->out_width and stride to the correct unit.
> > +		 */
> > +		int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> > +						icd->current_fmt->host_fmt);
> > +		BUG_ON(bytes_per_line <= 0);
> > +
> > +		video->out_width	= bytes_per_line;
> > +		video->out_height	= icd->user_height;
> > +		video->out_stride	= bytes_per_line;
> > +	} else {
> > +		/* For IPU known formats the pixel unit is OK */
> > +		video->out_width	= icd->user_width;
> > +		video->out_height	= icd->user_height;
> > +		video->out_stride	= icd->user_width;
> > +	}
> >  
> >  #ifdef DEBUG
> >  	/* helps to see what DMA actually has written */
> > @@ -730,18 +769,68 @@ static int mx3_camera_get_formats(struct soc_camera_device *icd, unsigned int id
> >  	if (xlate) {
> >  		xlate->host_fmt	= fmt;
> >  		xlate->code	= code;
> > +		dev_dbg(dev, "Providing format %c%c%c%c in pass-through mode\n",
> > +			(xlate->host_fmt->fourcc >> (0*8)) & 0xFF,
> > +			(xlate->host_fmt->fourcc >> (1*8)) & 0xFF,
> > +			(xlate->host_fmt->fourcc >> (2*8)) & 0xFF,
> > +			(xlate->host_fmt->fourcc >> (3*8)) & 0xFF);
> >  		xlate++;
> > -		dev_dbg(dev, "Providing format %x in pass-through mode\n",
> > -			xlate->host_fmt->fourcc);
> 
> make it even simpler: s/xlate->host_fmt/fmt/g
> 
> >  	}
> >  
> >  	return formats;
> >  }
> >  
> > +static int samples_per_pixel(enum v4l2_mbus_pixelcode mcode)
> > +{
> > +	switch (mcode) {
> > +	case V4L2_MBUS_FMT_YUYV8_2X8:
> > +	case V4L2_MBUS_FMT_YVYU8_2X8:
> > +	case V4L2_MBUS_FMT_VYUY8_2X8:
> > +	case V4L2_MBUS_FMT_YVYU10_2X10:
> > +	case V4L2_MBUS_FMT_YUYV10_2X10:
> > +	case V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE:
> > +	case V4L2_MBUS_FMT_RGB444_2X8_PADHI_BE:
> > +	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_LE:
> > +	case V4L2_MBUS_FMT_RGB555_2X8_PADHI_BE:
> > +	case V4L2_MBUS_FMT_RGB565_2X8_LE:
> > +	case V4L2_MBUS_FMT_RGB565_2X8_BE:
> > +	case V4L2_MBUS_FMT_BGR565_2X8_LE:
> > +	case V4L2_MBUS_FMT_BGR565_2X8_BE:
> > +		return 2;
> > +	case V4L2_MBUS_FMT_SBGGR8_1X8:
> > +	case V4L2_MBUS_FMT_SBGGR10_1X10:
> > +	case V4L2_MBUS_FMT_GREY8_1X8:
> > +	case V4L2_MBUS_FMT_Y10_1X10:
> > +	case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_LE:
> > +	case V4L2_MBUS_FMT_SBGGR10_2X8_PADHI_BE:
> 
> Are these two really 1 sample per pixel?

This is a way to maintain the previous behaviour that I consider wrong
and object of a second patch.

SBGGR10 pixel types are managed in a strange way within the mx3_camera code
I think because the previous code were not able to manage pixel formats
with multiple bytes for pixel.

That's why there is a first handshake of the pixel format in mx3_camera_get_formats
with the needs of a "pixel format translation" to support SBGGR10 through
real SBGGR8.

This translation is not needed with the tools I introduce in this patch
and the future is moving in this way. But what I want to do is to show
first that translation is not needed (without breaking any userspace code)
and next fix it.

> 
> > +	case V4L2_MBUS_FMT_SGRBG8_1X8:
> > +		return 1;
> > +	default:
> > +		/* Add other pixel codes as needed */
> > +		return 0;
> > +	}
> > +}
> 
> Let's just do the following:
> 
> s32 soc_mbus_samples_per_pixel(const struct soc_mbus_pixelfmt *mf)
> {
> 	switch (mf->packing) {
> 	case SOC_MBUS_PACKING_NONE:
> 	case SOC_MBUS_PACKING_EXTEND16:
> 		return 1;
> 	case SOC_MBUS_PACKING_2X8_PADHI:
> 	case SOC_MBUS_PACKING_2X8_PADLO:
> 		return 2;
> 	}
> 	return -EINVAL;
> }
> EXPORT_SYMBOL(soc_mbus_samples_per_pixel);
> 
> in drivers/media/video/soc_mediabus.c, agree?

Yes I agree. Shall this code be pushed in a different patch?
...I think yes because of it could be useful also for other drivers..

But this code will be used only when we will get rid of the
pixel code translations.

> 
> > +
> >  static void configure_geometry(struct mx3_camera_dev *mx3_cam,
> > -			       unsigned int width, unsigned int height)
> > +			       unsigned int width, unsigned int height,
> > +			       enum v4l2_mbus_pixelcode code)
> >  {
> >  	u32 ctrl, width_field, height_field;
> > +	const struct soc_mbus_pixelfmt *fmt;
> > +
> > +	fmt = soc_mbus_get_fmtdesc(code);
> > +	BUG_ON(!fmt);
> > +
> > +	if (fourcc_to_ipu_pix(fmt->fourcc) == IPU_PIX_FMT_GENERIC) {
> > +		/*
> > +		 * As we don't have an IPU native format, the CSI will be
> > +		 * configured to output BAYER and here we need to convert
> > +		 * geometry unit from pixels to samples.
> > +		 * TODO: Support vertical down sampling (YUV420)
> > +		 */
> > +		width = width * samples_per_pixel(code);
> > +		BUG_ON(!width);
> > +	}
> 
> 		width *= soc_mbus_samples_per_pixel(fmt);
> 		BUG_ON((int)width < 0);
> 
> >  
> >  	/* Setup frame size - this cannot be changed on-the-fly... */
> >  	width_field = width - 1;
> > @@ -850,7 +939,7 @@ static int mx3_camera_set_crop(struct soc_camera_device *icd,
> >  				return ret;
> >  		}
> >  
> > -		configure_geometry(mx3_cam, mf.width, mf.height);
> > +		configure_geometry(mx3_cam, mf.width, mf.height, mf.code);
> >  	}
> >  
> >  	dev_dbg(icd->dev.parent, "Sensor cropped %dx%d\n",
> > @@ -893,7 +982,7 @@ static int mx3_camera_set_fmt(struct soc_camera_device *icd,
> >  	 * mxc_v4l2_s_fmt()
> >  	 */
> >  
> > -	configure_geometry(mx3_cam, pix->width, pix->height);
> > +	configure_geometry(mx3_cam, pix->width, pix->height, xlate->code);
> >  
> >  	mf.width	= pix->width;
> >  	mf.height	= pix->height;
> > @@ -1112,10 +1201,15 @@ static int mx3_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
> >  		  (3 << CSI_SENS_CONF_DATA_FMT_SHIFT) |
> >  		  (3 << CSI_SENS_CONF_DATA_WIDTH_SHIFT));
> >  
> > -	/* TODO: Support RGB and YUV formats */
> > +	/* TODO: Support RGB_YUV444 formats */
> >  
> > -	/* This has been set in mx3_camera_activate(), but we clear it above */
> > -	sens_conf |= CSI_SENS_CONF_DATA_FMT_BAYER;
> > +	switch (xlate->code) {
> > +	case V4L2_MBUS_FMT_UYVY8_2X8:
> > +		sens_conf |= CSI_SENS_CONF_DATA_FMT_YUV422;
> > +		break;
> > +	default:
> > +		sens_conf |= CSI_SENS_CONF_DATA_FMT_BAYER;
> > +	}
> >  
> >  	if (common_flags & SOCAM_PCLK_SAMPLE_FALLING)
> >  		sens_conf |= 1 << CSI_SENS_CONF_PIX_CLK_POL_SHIFT;
> > -- 
> > 1.6.3.3
> 
> If after all the above changed your set up still works, we're cool!;)
> 
> Thanks
> Guennadi

All other code-style changes are object of the version 2 of this patch :)

Guennadi, how do you consider the path I've shown? Can I continue in this
way or shall I present a patch that get rid of translations and manage
all the pixel format in the way I understood the manual speak about?

I prefer this way that maintain the usability of the whole set of pixel codes
for everyone after this step and then fix together the translations. 
I don't hold a camera that output a grey format..

Best Regards,
Alberto!

Re: [PATCH 2/3] mx3_camera: Support correctly the YUV222 and BAYER configurations of CSI

[Guennadi Liakhovetski]

On Mon, 3 Jan 2011, Alberto Panizzo wrote:

[snip]

> Guennadi, how do you consider the path I've shown? Can I continue in this
> way or shall I present a patch that get rid of translations and manage
> all the pixel format in the way I understood the manual speak about?
> 
> I prefer this way that maintain the usability of the whole set of pixel codes
> for everyone after this step and then fix together the translations. 
> I don't hold a camera that output a grey format..

Sorry, I'm not sure I understand what exactly you're proposing. Please, 
just look at my last reply, in which I explain, why I don't think this is 
a good way to fix things. You don't need to support various formats 
natively on CSI / IPU to be able to just pass data 1-to-1. If that is your 
only purpose, please, test it that way, and if it is broken, please, fix 
it. This would be good even if you actually want to support formats 
natively eventually, because that would also fix other formats with 
similar sample-per-pixel values. When this is fixed, if you want to 
actually support formats natively to perform some hardware-assisted format 
conversions, that also can be done, but then, please, explain this clearly 
in your patch (series).

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/