lkml.org 
[lkml]   [2011]   [Jan]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 2/2] Fix capture issues for non 8-bit per pixel formats
    From
    Date
    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!




    \
     
     \ /
      Last update: 2011-01-17 10:45    [W:0.065 / U:33.044 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site