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

[Alberto Panizzo <maramaopercheseimorto@gmail.com>]

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!