media: i.MX27 camera: fix picture source width

While using a mt9m001 (monochrome) camera the final output falsely gets horizontally divided into two pictures.

The issue was git bisected to commit f410991dcf1f

  |  [media] i.MX27 camera: add support for YUV420 format
  |
  |  This patch uses channel 2 of the eMMa-PrP to convert
  |  format provided by the sensor to YUV420.
  |
  |  This format is very useful since it is used by the
  |  internal H.264 encoder.

It sets PICTURE_X_SIZE in register PRP_SRC_FRAME_SIZE to its full width while before that commit it was divided by two:

(quoted diff) +1 -1

-   writel(((bytesperline >> 1) << 16) | icd->user_height,
+           writel((icd->user_width << 16) | icd->user_height,
                    pcdev->base_emma + PRP_SRC_FRAME_SIZE);

i.mx27 reference manual (41.6.12 PrP Source Frame Size Register) says:

PICTURE_X_SIZE. These bits set the frame width to be processed in number of pixels. In YUV 4:2:0 mode, Cb and Cr widths are taken as PICTURE_X_SIZE/2 pixels. In YUV 4:2:0 mode, this value should be a multiple of 8-pixels. In other modes (RGB, YUV 4:2:2 and YUV 4:4:4) it should be a multiple of 4 pixels.

This patch reverts to PICTURE_X_SIZE/2 for channel 1.

Tested on Kernel 3.4, merged to 3.8rc.

Signed-off-by Christoph Fritz <[email protected]>

---
 drivers/media/platform/soc_camera/mx2_camera.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

While using a mt9m001 (monochrome) camera the final output falsely gets horizontally divided into two pictures.

The issue was git bisected to commit f410991dcf1f

  |  [media] i.MX27 camera: add support for YUV420 format
  |
  |  This patch uses channel 2 of the eMMa-PrP to convert
  |  format provided by the sensor to YUV420.
  |
  |  This format is very useful since it is used by the
  |  internal H.264 encoder.

It sets PICTURE_X_SIZE in register PRP_SRC_FRAME_SIZE to its full width while before that commit it was divided by two:

(quoted diff) +1 -1

-   writel(((bytesperline >> 1) << 16) | icd->user_height,
+           writel((icd->user_width << 16) | icd->user_height,
                    pcdev->base_emma + PRP_SRC_FRAME_SIZE);

i.mx27 reference manual (41.6.12 PrP Source Frame Size Register) says:

PICTURE_X_SIZE. These bits set the frame width to be processed in number of pixels. In YUV 4:2:0 mode, Cb and Cr widths are taken as PICTURE_X_SIZE/2 pixels. In YUV 4:2:0 mode, this value should be a multiple of 8-pixels. In other modes (RGB, YUV 4:2:2 and YUV 4:4:4) it should be a multiple of 4 pixels.

This patch reverts to PICTURE_X_SIZE/2 for channel 1.

Tested on Kernel 3.4, merged to 3.8rc.

Signed-off-by Christoph Fritz <[email protected]>

---
 drivers/media/platform/soc_camera/mx2_camera.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/media/platform/soc_camera/mx2_camera.c +4 -2

index 8bda2c9..795bd3f 100644
@@ -778,11 +778,11 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
 	struct mx2_camera_dev *pcdev = ici->priv;
 	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
 
-	writel((pcdev->s_width << 16) | pcdev->s_height,
-	       pcdev->base_emma + PRP_SRC_FRAME_SIZE);
 	writel(prp->cfg.src_pixel,
 	       pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
 	if (prp->cfg.channel == 1) {
+		writel(((bytesperline >> 1) << 16) | pcdev->s_height,
+			pcdev->base_emma + PRP_SRC_FRAME_SIZE);
 		writel((icd->user_width << 16) | icd->user_height,
 			pcdev->base_emma + PRP_CH1_OUT_IMAGE_SIZE);
 		writel(bytesperline,

@@ -790,6 +790,8 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
 		writel(prp->cfg.ch1_pixel,
 			pcdev->base_emma + PRP_CH1_PIXEL_FORMAT_CNTL);
 	} else { /* channel 2 */
+		writel((pcdev->s_width << 16) | pcdev->s_height,
+			pcdev->base_emma + PRP_SRC_FRAME_SIZE);
 		writel((icd->user_width << 16) | icd->user_height,
 			pcdev->base_emma + PRP_CH2_OUT_IMAGE_SIZE);
 	}

Signature · 1 line

1.7.10.4

(Javier's opinion requested)

I'm no expert in i.MX27 hardware, would be great to have an ack from someone, intensively working in this area. Javier, what do you think? Is this really correct always for channel 1, or should this calculation depend on the pixel format?

Thanks
Guennadi

Quoted 28 lines · Christoph Fritz

While using a mt9m001 (monochrome) camera the final output falsely gets horizontally divided into two pictures.

The issue was git bisected to commit f410991dcf1f

  |  [media] i.MX27 camera: add support for YUV420 format
  |
  |  This patch uses channel 2 of the eMMa-PrP to convert
  |  format provided by the sensor to YUV420.
  |
  |  This format is very useful since it is used by the
  |  internal H.264 encoder.

It sets PICTURE_X_SIZE in register PRP_SRC_FRAME_SIZE to its full width while before that commit it was divided by two:

(quoted diff) +1 -1

-   writel(((bytesperline >> 1) << 16) | icd->user_height,
+           writel((icd->user_width << 16) | icd->user_height,
                    pcdev->base_emma + PRP_SRC_FRAME_SIZE);

i.mx27 reference manual (41.6.12 PrP Source Frame Size Register) says:

PICTURE_X_SIZE. These bits set the frame width to be processed in number of pixels. In YUV 4:2:0 mode, Cb and Cr widths are taken as PICTURE_X_SIZE/2 pixels. In YUV 4:2:0 mode, this value should be a multiple of 8-pixels. In other modes (RGB, YUV 4:2:2 and YUV 4:4:4) it should be a multiple of 4 pixels.

Quoted 17 lines

This patch reverts to PICTURE_X_SIZE/2 for channel 1.

Tested on Kernel 3.4, merged to 3.8rc.

Signed-off-by Christoph Fritz <[email protected]>

---
 drivers/media/platform/soc_camera/mx2_camera.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/media/platform/soc_camera/mx2_camera.c +0 -0

index 8bda2c9..795bd3f 100644
@@ -778,11 +778,11 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
      struct mx2_camera_dev *pcdev = ici->priv;
      struct mx2_fmt_cfg *prp = pcdev->emma_prp;
 

drivers/media/platform/soc_camera/mx2_camera.c static void mx27_camera_emma_buf_init(struct soc_camera_device *icd, · line 785

Quoted 7 lines

drivers/media/platform/soc_camera/mx2_camera.c +2 -2

-     writel((pcdev->s_width << 16) | pcdev->s_height,
-            pcdev->base_emma + PRP_SRC_FRAME_SIZE);
      writel(prp->cfg.src_pixel,
             pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
      if (prp->cfg.channel == 1) {
+             writel(((bytesperline >> 1) << 16) | pcdev->s_height,
+                     pcdev->base_emma + PRP_SRC_FRAME_SIZE);

Quoted 6 lines

drivers/media/platform/soc_camera/mx2_camera.c +0 -0

              writel((icd->user_width << 16) | icd->user_height,
                      pcdev->base_emma + PRP_CH1_OUT_IMAGE_SIZE);
              writel(bytesperline,

@@ -790,6 +790,8 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
              writel(prp->cfg.ch1_pixel,
                      pcdev->base_emma + PRP_CH1_PIXEL_FORMAT_CNTL);

Quoted 8 lines

drivers/media/platform/soc_camera/mx2_camera.c +2 -1

      } else { /* channel 2 */
+             writel((pcdev->s_width << 16) | pcdev->s_height,
+                     pcdev->base_emma + PRP_SRC_FRAME_SIZE);
              writel((icd->user_width << 16) | icd->user_height,
                      pcdev->base_emma + PRP_CH2_OUT_IMAGE_SIZE);
      }
--

1.7.10.4

Hi,
sorry for the long delay. I missed this one.

Quoted 7 lines · Guennadi Liakhovetski

(Javier's opinion requested)

I'm no expert in i.MX27 hardware, would be great to have an ack from someone, intensively working in this area. Javier, what do you think? Is this really correct always for channel 1, or should this calculation depend on the pixel format?

Quoted 33 lines

Thanks
Guennadi

Quoted 28 lines · Christoph Fritz

While using a mt9m001 (monochrome) camera the final output falsely gets horizontally divided into two pictures.

The issue was git bisected to commit f410991dcf1f

  |  [media] i.MX27 camera: add support for YUV420 format
  |
  |  This patch uses channel 2 of the eMMa-PrP to convert
  |  format provided by the sensor to YUV420.
  |
  |  This format is very useful since it is used by the
  |  internal H.264 encoder.

It sets PICTURE_X_SIZE in register PRP_SRC_FRAME_SIZE to its full width while before that commit it was divided by two:

(quoted diff) +1 -1

-   writel(((bytesperline >> 1) << 16) | icd->user_height,
+           writel((icd->user_width << 16) | icd->user_height,
                    pcdev->base_emma + PRP_SRC_FRAME_SIZE);

i.mx27 reference manual (41.6.12 PrP Source Frame Size Register) says:

PICTURE_X_SIZE. These bits set the frame width to be processed in number of pixels. In YUV 4:2:0 mode, Cb and Cr widths are taken as PICTURE_X_SIZE/2 pixels. In YUV 4:2:0 mode, this value should be a multiple of 8-pixels. In other modes (RGB, YUV 4:2:2 and YUV 4:4:4) it should be a multiple of 4 pixels.

Note that, according to the description in the datasheet, PICTURE_X_SIZE is specified in pixels, not bytes. So, it is not dependant on the format used.

Quoted 17 lines

Quoted 17 lines

This patch reverts to PICTURE_X_SIZE/2 for channel 1.

Tested on Kernel 3.4, merged to 3.8rc.

Signed-off-by Christoph Fritz <[email protected]>

---
 drivers/media/platform/soc_camera/mx2_camera.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/media/platform/soc_camera/mx2_camera.c +0 -0

index 8bda2c9..795bd3f 100644
@@ -778,11 +778,11 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
      struct mx2_camera_dev *pcdev = ici->priv;
      struct mx2_fmt_cfg *prp = pcdev->emma_prp;
 

Quoted 7 lines

drivers/media/platform/soc_camera/mx2_camera.c static void mx27_camera_emma_buf_init(struct soc_camera_device *icd, · line 785

Quoted 7 lines

drivers/media/platform/soc_camera/mx2_camera.c +2 -2

-     writel((pcdev->s_width << 16) | pcdev->s_height,
-            pcdev->base_emma + PRP_SRC_FRAME_SIZE);
      writel(prp->cfg.src_pixel,
             pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
      if (prp->cfg.channel == 1) {
+             writel(((bytesperline >> 1) << 16) | pcdev->s_height,
+                     pcdev->base_emma + PRP_SRC_FRAME_SIZE);

If you use bytesperline here you are making this operation dependant on the mbus format.
You should use s_width instead and there is no reason to divide it by 2 since it is already in pixels, as well as PRP_SRC_FRAME_SIZE.

Quoted 6 lines

Quoted 6 lines

drivers/media/platform/soc_camera/mx2_camera.c +0 -0

              writel((icd->user_width << 16) | icd->user_height,
                      pcdev->base_emma + PRP_CH1_OUT_IMAGE_SIZE);
              writel(bytesperline,

@@ -790,6 +790,8 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
              writel(prp->cfg.ch1_pixel,
                      pcdev->base_emma + PRP_CH1_PIXEL_FORMAT_CNTL);

Quoted 8 lines

Quoted 8 lines

drivers/media/platform/soc_camera/mx2_camera.c +2 -1

      } else { /* channel 2 */
+             writel((pcdev->s_width << 16) | pcdev->s_height,
+                     pcdev->base_emma + PRP_SRC_FRAME_SIZE);
              writel((icd->user_width << 16) | icd->user_height,
                      pcdev->base_emma + PRP_CH2_OUT_IMAGE_SIZE);
      }
--

1.7.10.4

We are using channel 1 here daily for capturing YUV422 and have not experience the problem you point out.

What mbus format are you using? Could you please check if the s_width value that your sensor mt9m001 returns is correct? Remember it should be in pixels, not in bytes.

Quoted 3 lines · javier Martin

What mbus format are you using? Could you please check if the s_width value that your sensor mt9m001 returns is correct? Remember it should be in pixels, not in bytes.

Thanks for looking at this. But here's my question: for a pass-through mode mx2_camera uses a 16-bpp (RGB565) format. But what if it's actually an 8-bpp format, don't you then have to adjust line-width register settings? If you don't do that, the camera interface would expect a double number of bytes per line, so, it could get confused by HSYNC coming after half-line?

Quoted 6 lines · Guennadi Liakhovetski

Quoted 3 lines · javier Martin

What mbus format are you using? Could you please check if the s_width value that your sensor mt9m001 returns is correct? Remember it should be in pixels, not in bytes.

Quoted 6 lines

Thanks for looking at this. But here's my question: for a pass-through mode mx2_camera uses a 16-bpp (RGB565) format. But what if it's actually an 8-bpp format, don't you then have to adjust line-width register settings? If you don't do that, the camera interface would expect a double number of bytes per line, so, it could get confused by HSYNC coming after half-line?

To emphasize this: I'm using here a mt9m001 (monochrome) camera with an 8-bpp format.

Hi Guernnadi, Christoph,

Quoted 7 lines · Christoph Fritz

Quoted 6 lines · Guennadi Liakhovetski

Quoted 3 lines · javier Martin

What mbus format are you using? Could you please check if the s_width value that your sensor mt9m001 returns is correct? Remember it should be in pixels, not in bytes.

Quoted 6 lines

Quoted 6 lines

Thanks for looking at this. But here's my question: for a pass-through mode mx2_camera uses a 16-bpp (RGB565) format. But what if it's actually an 8-bpp format, don't you then have to adjust line-width register settings? If you don't do that, the camera interface would expect a double number of bytes per line, so, it could get confused by HSYNC coming after half-line?

You are right.

Quoted 2 lines

To emphasize this: I'm using here a mt9m001 (monochrome) camera with an 8-bpp format.

Ok, now that makes sense.

Then, what you should do is apply your patch conditionally so that you don't break other working cases:
- Channel 1 is being used.
- Channel 1 is in pass-through mode.

- The sensor uses an 8-bpp format.

Quoted 24 lines · javier Martin

Hi Guernnadi, Christoph,

Quoted 7 lines · Christoph Fritz

Quoted 6 lines · Guennadi Liakhovetski

Quoted 3 lines · javier Martin

What mbus format are you using? Could you please check if the s_width value that your sensor mt9m001 returns is correct? Remember it should be in pixels, not in bytes.

Quoted 6 lines

Quoted 6 lines

Thanks for looking at this. But here's my question: for a pass-through mode mx2_camera uses a 16-bpp (RGB565) format. But what if it's actually an 8-bpp format, don't you then have to adjust line-width register settings? If you don't do that, the camera interface would expect a double number of bytes per line, so, it could get confused by HSYNC coming after half-line?

You are right.

Quoted 2 lines

To emphasize this: I'm using here a mt9m001 (monochrome) camera with an 8-bpp format.

Ok, now that makes sense.

Quoted 4 lines

Then, what you should do is apply your patch conditionally so that you don't break other working cases:
- Channel 1 is being used.
- Channel 1 is in pass-through mode.

which would be

        if (!prp->in_fmt && !prp->out_fmt)

Quoted 1 line

- The sensor uses an 8-bpp format.

No, the format in unimportant - you pretend to use a 16-bit format, so, your "simulated" line is always bytesperline / 2 pseudo-pixels long.

Christoph, in your next comment please add a comment something like

/*
* In pass-through we configure EMMA with a 16-bpp format,
* set the line-width accordingly.
*/