| Message ID | 1361009964.5028.3.camel@mars (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
(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 On Sat, 16 Feb 2013, Christoph Fritz wrote: > 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: > > - 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 <chf.fritz@googlemail.com> > --- > drivers/media/platform/soc_camera/mx2_camera.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/soc_camera/mx2_camera.c b/drivers/media/platform/soc_camera/mx2_camera.c > index 8bda2c9..795bd3f 100644 > --- a/drivers/media/platform/soc_camera/mx2_camera.c > +++ b/drivers/media/platform/soc_camera/mx2_camera.c > @@ -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); > } > -- > 1.7.10.4 > > > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, sorry for the long delay. I missed this one. On 5 March 2013 18:56, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > (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 > > On Sat, 16 Feb 2013, Christoph Fritz wrote: > >> 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: >> >> - 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. >> 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 <chf.fritz@googlemail.com> >> --- >> drivers/media/platform/soc_camera/mx2_camera.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/platform/soc_camera/mx2_camera.c b/drivers/media/platform/soc_camera/mx2_camera.c >> index 8bda2c9..795bd3f 100644 >> --- a/drivers/media/platform/soc_camera/mx2_camera.c >> +++ b/drivers/media/platform/soc_camera/mx2_camera.c >> @@ -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); 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. >> 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); >> } >> -- >> 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.
Hi Javier On Thu, 7 Mar 2013, javier Martin wrote: > Hi, > sorry for the long delay. I missed this one. > > On 5 March 2013 18:56, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > > (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 > > > > On Sat, 16 Feb 2013, Christoph Fritz wrote: > > > >> 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: > >> > >> - 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. Yes, looks like it should operate in pixels, not bytes. > >> 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 <chf.fritz@googlemail.com> > >> --- > >> drivers/media/platform/soc_camera/mx2_camera.c | 6 ++++-- > >> 1 file changed, 4 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/media/platform/soc_camera/mx2_camera.c b/drivers/media/platform/soc_camera/mx2_camera.c > >> index 8bda2c9..795bd3f 100644 > >> --- a/drivers/media/platform/soc_camera/mx2_camera.c > >> +++ b/drivers/media/platform/soc_camera/mx2_camera.c > >> @@ -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); > > 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. > > >> 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); > >> } > >> -- > >> 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. 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? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2013-03-12 at 08:58 +0100, Guennadi Liakhovetski wrote: > On Thu, 7 Mar 2013, javier Martin wrote: > > 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? To emphasize this: I'm using here a mt9m001 (monochrome) camera with an 8-bpp format. Thanks -- Christoph -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guernnadi, Christoph, On 12 March 2013 09:25, Christoph Fritz <chf.fritz@googlemail.com> wrote: > On Tue, 2013-03-12 at 08:58 +0100, Guennadi Liakhovetski wrote: >> On Thu, 7 Mar 2013, javier Martin wrote: > >> > 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? You are right. > 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. Regards.
On Tue, 12 Mar 2013, javier Martin wrote: > Hi Guernnadi, Christoph, > > On 12 March 2013 09:25, Christoph Fritz <chf.fritz@googlemail.com> wrote: > > On Tue, 2013-03-12 at 08:58 +0100, Guennadi Liakhovetski wrote: > >> On Thu, 7 Mar 2013, javier Martin wrote: > > > >> > 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? > > You are right. > > > 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. which would be if (!prp->in_fmt && !prp->out_fmt) > - 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. */ Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12 March 2013 10:39, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > On Tue, 12 Mar 2013, javier Martin wrote: > >> Hi Guernnadi, Christoph, >> >> On 12 March 2013 09:25, Christoph Fritz <chf.fritz@googlemail.com> wrote: >> > On Tue, 2013-03-12 at 08:58 +0100, Guennadi Liakhovetski wrote: >> >> On Thu, 7 Mar 2013, javier Martin wrote: >> > >> >> > 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? >> >> You are right. >> >> > 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. > > which would be > > if (!prp->in_fmt && !prp->out_fmt) Yes, that would do. >> - 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. OK. > 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. > */
diff --git a/drivers/media/platform/soc_camera/mx2_camera.c b/drivers/media/platform/soc_camera/mx2_camera.c index 8bda2c9..795bd3f 100644 --- a/drivers/media/platform/soc_camera/mx2_camera.c +++ b/drivers/media/platform/soc_camera/mx2_camera.c @@ -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); }
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: - 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 <chf.fritz@googlemail.com> --- drivers/media/platform/soc_camera/mx2_camera.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)