* Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats
2021-05-16 2:42 ` [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats Laurent Pinchart
@ 2021-05-17 10:21 ` Rui Miguel Silva
2021-05-19 0:23 ` [RFC PATCH 2/3 v1.1] " Laurent Pinchart
2021-05-17 10:21 ` [RFC PATCH 2/3] " Frieder Schrempf
` (2 subsequent siblings)
3 siblings, 1 reply; 20+ messages in thread
From: Rui Miguel Silva @ 2021-05-17 10:21 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: kernel, Fabio Estevam, linux-imx, Steve Longerbeam,
Philipp Zabel, Marek Vasut, Marco Felsch, Martin Kepplinger,
Dorota Czaplejewicz
Hi Laurent,
On Sun May 16, 2021 at 3:42 AM WEST, Laurent Pinchart wrote:
> Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> capture with an OV5640 sensor connected over CSI-2, showed that the
> TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
> larger than 8 bits. Do so, even if the reference manual doesn't clearly
> describe the effect of the field.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index f85a2f5f1413..256b9aa978f0 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -422,6 +422,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> int width = out_pix->width;
> u32 stride = 0;
> u32 cr1, cr18;
> + u32 cr3 = 0;
>
> cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>
> @@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> case MEDIA_BUS_FMT_SGBRG10_1X10:
> case MEDIA_BUS_FMT_SGRBG10_1X10:
> case MEDIA_BUS_FMT_SRGGB10_1X10:
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
> break;
> case MEDIA_BUS_FMT_Y12_1X12:
> @@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> case MEDIA_BUS_FMT_SGBRG12_1X12:
> case MEDIA_BUS_FMT_SGRBG12_1X12:
> case MEDIA_BUS_FMT_SRGGB12_1X12:
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
> break;
> case MEDIA_BUS_FMT_Y14_1X14:
> @@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> case MEDIA_BUS_FMT_SGBRG14_1X14:
> case MEDIA_BUS_FMT_SGRBG14_1X14:
> case MEDIA_BUS_FMT_SRGGB14_1X14:
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
> break;
> /*
> @@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>
> imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
> imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16, CSI_CSICR2);
> - imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
> + imx7_csi_reg_write(csi, BIT_FRMCNT_RST | cr3, CSI_CSICR3);
I would prefer if you initialize cr3 with BIT_FRMCNT_RST above at
declaration or even better bellow after cr18 read or something like
that, would make it more coherent with the rest of the cr's handling.
other than that LGTM.
------
Cheers,
Rui
> imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
>
> imx7_csi_reg_write(csi, (width * out_pix->height) >> 2, CSI_CSIRXCNT);
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 20+ messages in thread
* [RFC PATCH 2/3 v1.1] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats
2021-05-17 10:21 ` Rui Miguel Silva
@ 2021-05-19 0:23 ` Laurent Pinchart
2021-05-19 14:07 ` Rui Miguel Silva
0 siblings, 1 reply; 20+ messages in thread
From: Laurent Pinchart @ 2021-05-19 0:23 UTC (permalink / raw)
To: linux-media
Cc: kernel, Fabio Estevam, linux-imx, Steve Longerbeam,
Philipp Zabel, Marek Vasut, Marco Felsch, Martin Kepplinger,
Dorota Czaplejewicz, Rui Miguel Silva
Sample code from NXP, as well as experiments on i.MX8MM with RAW10
capture with an OV5640 sensor connected over CSI-2, showed that the
TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
larger than 8 bits. Do so, even if the reference manual doesn't clearly
describe the effect of the field.
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:
- Initialize cr3 to BIT_FRMCNT_RST
---
drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index f85a2f5f1413..5ae9ed1849e9 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -421,6 +421,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
struct v4l2_pix_format *out_pix = &vdev->fmt;
int width = out_pix->width;
u32 stride = 0;
+ u32 cr3 = BIT_FRMCNT_RST;
u32 cr1, cr18;
cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
@@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
case MEDIA_BUS_FMT_SGBRG10_1X10:
case MEDIA_BUS_FMT_SGRBG10_1X10:
case MEDIA_BUS_FMT_SRGGB10_1X10:
+ cr3 |= BIT_TWO_8BIT_SENSOR;
cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
break;
case MEDIA_BUS_FMT_Y12_1X12:
@@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
case MEDIA_BUS_FMT_SGBRG12_1X12:
case MEDIA_BUS_FMT_SGRBG12_1X12:
case MEDIA_BUS_FMT_SRGGB12_1X12:
+ cr3 |= BIT_TWO_8BIT_SENSOR;
cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
break;
case MEDIA_BUS_FMT_Y14_1X14:
@@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
case MEDIA_BUS_FMT_SGBRG14_1X14:
case MEDIA_BUS_FMT_SGRBG14_1X14:
case MEDIA_BUS_FMT_SRGGB14_1X14:
+ cr3 |= BIT_TWO_8BIT_SENSOR;
cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
break;
/*
@@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16, CSI_CSICR2);
- imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
+ imx7_csi_reg_write(csi, cr3, CSI_CSICR3);
imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
imx7_csi_reg_write(csi, (width * out_pix->height) >> 2, CSI_CSIRXCNT);
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 2/3 v1.1] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats
2021-05-19 0:23 ` [RFC PATCH 2/3 v1.1] " Laurent Pinchart
@ 2021-05-19 14:07 ` Rui Miguel Silva
0 siblings, 0 replies; 20+ messages in thread
From: Rui Miguel Silva @ 2021-05-19 14:07 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: kernel, Fabio Estevam, linux-imx, Steve Longerbeam,
Philipp Zabel, Marek Vasut, Marco Felsch, Martin Kepplinger,
Dorota Czaplejewicz
Thanks Laurent,
On Wed May 19, 2021 at 1:23 AM WEST, Laurent Pinchart wrote:
> Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> capture with an OV5640 sensor connected over CSI-2, showed that the
> TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
> larger than 8 bits. Do so, even if the reference manual doesn't clearly
> describe the effect of the field.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Rui Miguel Silva <rmfrfs@gmail.com>
------
Cheers,
Rui
> ---
> Changes since v1:
>
> - Initialize cr3 to BIT_FRMCNT_RST
> ---
> drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index f85a2f5f1413..5ae9ed1849e9 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -421,6 +421,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> struct v4l2_pix_format *out_pix = &vdev->fmt;
> int width = out_pix->width;
> u32 stride = 0;
> + u32 cr3 = BIT_FRMCNT_RST;
> u32 cr1, cr18;
>
> cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
> @@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> case MEDIA_BUS_FMT_SGBRG10_1X10:
> case MEDIA_BUS_FMT_SGRBG10_1X10:
> case MEDIA_BUS_FMT_SRGGB10_1X10:
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
> break;
> case MEDIA_BUS_FMT_Y12_1X12:
> @@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> case MEDIA_BUS_FMT_SGBRG12_1X12:
> case MEDIA_BUS_FMT_SGRBG12_1X12:
> case MEDIA_BUS_FMT_SRGGB12_1X12:
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
> break;
> case MEDIA_BUS_FMT_Y14_1X14:
> @@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> case MEDIA_BUS_FMT_SGBRG14_1X14:
> case MEDIA_BUS_FMT_SGRBG14_1X14:
> case MEDIA_BUS_FMT_SRGGB14_1X14:
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
> break;
> /*
> @@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>
> imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
> imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16, CSI_CSICR2);
> - imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
> + imx7_csi_reg_write(csi, cr3, CSI_CSICR3);
> imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
>
> imx7_csi_reg_write(csi, (width * out_pix->height) >> 2, CSI_CSIRXCNT);
> --
> Regards,
>
> Laurent Pinchart
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats
2021-05-16 2:42 ` [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats Laurent Pinchart
2021-05-17 10:21 ` Rui Miguel Silva
@ 2021-05-17 10:21 ` Frieder Schrempf
2021-05-19 0:16 ` Laurent Pinchart
2021-05-18 11:28 ` Martin Kepplinger
2021-07-28 8:54 ` Martin Kepplinger
3 siblings, 1 reply; 20+ messages in thread
From: Frieder Schrempf @ 2021-05-17 10:21 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: Rui Miguel Silva, kernel, Fabio Estevam, linux-imx,
Steve Longerbeam, Philipp Zabel, Marek Vasut, Marco Felsch,
Martin Kepplinger, Dorota Czaplejewicz
On 16.05.21 04:42, Laurent Pinchart wrote:
> Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> capture with an OV5640 sensor connected over CSI-2, showed that the
> TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
> larger than 8 bits. Do so, even if the reference manual doesn't clearly
> describe the effect of the field.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
For the ADV7280-M I also have the diffs below applied. Do you think setting BIT_MIPI_DOUBLE_CMPNT is specific to MEDIA_BUS_FMT_UYVY8_2X8? In the RM it mentions YUV422 in the description of BIT_MIPI_DOUBLE_CMPNT and without setting it, the colors are all wrong.
I know this is not really related to this patch. I'm just wondering how to properly support my setup.
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -346,6 +346,11 @@ struct csis_pix_format {
static const struct csis_pix_format mipi_csis_formats[] = {
/* YUV formats. */
+ {
+ .code = MEDIA_BUS_FMT_UYVY8_2X8,
+ .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
+ .width = 8,
+ },
{
.code = MEDIA_BUS_FMT_UYVY8_1X16,
.data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -504,7 +504,7 @@ static void __mipi_csis_set_format(struct csi_state *state)
/* Color format */
val = mipi_csis_read(state, MIPI_CSIS_ISP_CONFIG_CH(0));
val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK);
- val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type);
+ val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type) | MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
mipi_csis_write(state, MIPI_CSIS_ISP_CONFIG_CH(0), val);
/* Pixel resolution */
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -492,7 +492,8 @@ static void imx7_csi_configure(struct imx7_csi *csi)
case MEDIA_BUS_FMT_UYVY8_1X16:
case MEDIA_BUS_FMT_YUYV8_2X8:
case MEDIA_BUS_FMT_YUYV8_1X16:
- cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
+ cr3 |= BIT_TWO_8BIT_SENSOR;
+ cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B | BIT_MIPI_DOUBLE_CMPNT;
break;
}
}
> ---
> drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index f85a2f5f1413..256b9aa978f0 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -422,6 +422,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> int width = out_pix->width;
> u32 stride = 0;
> u32 cr1, cr18;
> + u32 cr3 = 0;
>
> cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>
> @@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> case MEDIA_BUS_FMT_SGBRG10_1X10:
> case MEDIA_BUS_FMT_SGRBG10_1X10:
> case MEDIA_BUS_FMT_SRGGB10_1X10:
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
> break;
> case MEDIA_BUS_FMT_Y12_1X12:
> @@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> case MEDIA_BUS_FMT_SGBRG12_1X12:
> case MEDIA_BUS_FMT_SGRBG12_1X12:
> case MEDIA_BUS_FMT_SRGGB12_1X12:
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
> break;
> case MEDIA_BUS_FMT_Y14_1X14:
> @@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> case MEDIA_BUS_FMT_SGBRG14_1X14:
> case MEDIA_BUS_FMT_SGRBG14_1X14:
> case MEDIA_BUS_FMT_SRGGB14_1X14:
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
> break;
> /*
> @@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>
> imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
> imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16, CSI_CSICR2);
> - imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
> + imx7_csi_reg_write(csi, BIT_FRMCNT_RST | cr3, CSI_CSICR3);
> imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
>
> imx7_csi_reg_write(csi, (width * out_pix->height) >> 2, CSI_CSIRXCNT);
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats
2021-05-17 10:21 ` [RFC PATCH 2/3] " Frieder Schrempf
@ 2021-05-19 0:16 ` Laurent Pinchart
2021-05-25 9:59 ` Frieder Schrempf
0 siblings, 1 reply; 20+ messages in thread
From: Laurent Pinchart @ 2021-05-19 0:16 UTC (permalink / raw)
To: Frieder Schrempf
Cc: linux-media, Rui Miguel Silva, kernel, Fabio Estevam, linux-imx,
Steve Longerbeam, Philipp Zabel, Marek Vasut, Marco Felsch,
Martin Kepplinger, Dorota Czaplejewicz
Hi Frieder,
On Mon, May 17, 2021 at 12:21:17PM +0200, Frieder Schrempf wrote:
> On 16.05.21 04:42, Laurent Pinchart wrote:
> > Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> > capture with an OV5640 sensor connected over CSI-2, showed that the
> > TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
> > larger than 8 bits. Do so, even if the reference manual doesn't clearly
> > describe the effect of the field.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> For the ADV7280-M I also have the diffs below applied. Do you think
> setting BIT_MIPI_DOUBLE_CMPNT is specific to MEDIA_BUS_FMT_UYVY8_2X8?
Do you need MEDIA_BUS_FMT_UYVY8_2X8 ? Neither MEDIA_BUS_FMT_UYVY8_1X16
nor MEDIA_BUS_FMT_UYVY8_2X8 match exactly how YUV 4:2:2 data is
transmitted over CSI-2. V4L2 uses MEDIA_BUS_FMT_UYVY8_1X16 as a
convention.
> In the RM it mentions YUV422 in the description of
> BIT_MIPI_DOUBLE_CMPNT and without setting it, the colors are all
> wrong.
That's interesting. I've tested YUV 4:2:2 with an OV5640 sensor, and I
don't recall having to set the MIPI_DOUBLE_CMPNT field. I'll try to
retest.
> I know this is not really related to this patch. I'm just wondering
> how to properly support my setup.
It's hard to tell :-( The MIPI_CSI2_ISP_CONFIG PIXEL_MODE and PARALLEL
fields are not well documented, and neither is the CSI_CR18
MIPI_DOUBLE_CMPNT field. While the CSIS and the CSI bridge are
documented, how they're integrated isn't described. So far, I can only
guess.
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -346,6 +346,11 @@ struct csis_pix_format {
>
> static const struct csis_pix_format mipi_csis_formats[] = {
> /* YUV formats. */
> + {
> + .code = MEDIA_BUS_FMT_UYVY8_2X8,
> + .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
> + .width = 8,
> + },
> {
> .code = MEDIA_BUS_FMT_UYVY8_1X16,
> .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
>
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -504,7 +504,7 @@ static void __mipi_csis_set_format(struct csi_state *state)
> /* Color format */
> val = mipi_csis_read(state, MIPI_CSIS_ISP_CONFIG_CH(0));
> val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK);
> - val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type);
> + val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type) | MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
> mipi_csis_write(state, MIPI_CSIS_ISP_CONFIG_CH(0), val);
>
> /* Pixel resolution */
>
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -492,7 +492,8 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> case MEDIA_BUS_FMT_UYVY8_1X16:
> case MEDIA_BUS_FMT_YUYV8_2X8:
> case MEDIA_BUS_FMT_YUYV8_1X16:
> - cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> + cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B | BIT_MIPI_DOUBLE_CMPNT;
I notice that you set both PIXEL_MODE_DUAL and MIPI_DOUBLE_CMPNT. Have
you tried setting neither ?
Have you also tried using MEDIA_BUS_FMT_UYVY8_1X16 ? The difference
between MEDIA_BUS_FMT_UYVY8_2X8 and MEDIA_BUS_FMT_UYVY8_1X16 in this
driver is the width value passed to v4l2_get_link_freq(). With
MEDIA_BUS_FMT_UYVY8_2X8 you'll end up with a computed link frequency
equal to half of the actual value, and thus a wrong Ths_settle value. It
shuold have no other influence though.
> break;
> }
> }
>
> > ---
> > drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> > index f85a2f5f1413..256b9aa978f0 100644
> > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > @@ -422,6 +422,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> > int width = out_pix->width;
> > u32 stride = 0;
> > u32 cr1, cr18;
> > + u32 cr3 = 0;
> >
> > cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
> >
> > @@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> > case MEDIA_BUS_FMT_SGBRG10_1X10:
> > case MEDIA_BUS_FMT_SGRBG10_1X10:
> > case MEDIA_BUS_FMT_SRGGB10_1X10:
> > + cr3 |= BIT_TWO_8BIT_SENSOR;
> > cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
> > break;
> > case MEDIA_BUS_FMT_Y12_1X12:
> > @@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> > case MEDIA_BUS_FMT_SGBRG12_1X12:
> > case MEDIA_BUS_FMT_SGRBG12_1X12:
> > case MEDIA_BUS_FMT_SRGGB12_1X12:
> > + cr3 |= BIT_TWO_8BIT_SENSOR;
> > cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
> > break;
> > case MEDIA_BUS_FMT_Y14_1X14:
> > @@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> > case MEDIA_BUS_FMT_SGBRG14_1X14:
> > case MEDIA_BUS_FMT_SGRBG14_1X14:
> > case MEDIA_BUS_FMT_SRGGB14_1X14:
> > + cr3 |= BIT_TWO_8BIT_SENSOR;
> > cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
> > break;
> > /*
> > @@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >
> > imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
> > imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16, CSI_CSICR2);
> > - imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
> > + imx7_csi_reg_write(csi, BIT_FRMCNT_RST | cr3, CSI_CSICR3);
> > imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
> >
> > imx7_csi_reg_write(csi, (width * out_pix->height) >> 2, CSI_CSIRXCNT);
> >
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats
2021-05-19 0:16 ` Laurent Pinchart
@ 2021-05-25 9:59 ` Frieder Schrempf
2021-06-10 14:49 ` Laurent Pinchart
0 siblings, 1 reply; 20+ messages in thread
From: Frieder Schrempf @ 2021-05-25 9:59 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, Rui Miguel Silva, kernel, Fabio Estevam, linux-imx,
Steve Longerbeam, Philipp Zabel, Marek Vasut, Marco Felsch,
Martin Kepplinger, Dorota Czaplejewicz
Hi Laurent,
On 19.05.21 02:16, Laurent Pinchart wrote:
> Hi Frieder,
>
> On Mon, May 17, 2021 at 12:21:17PM +0200, Frieder Schrempf wrote:
>> On 16.05.21 04:42, Laurent Pinchart wrote:
>>> Sample code from NXP, as well as experiments on i.MX8MM with RAW10
>>> capture with an OV5640 sensor connected over CSI-2, showed that the
>>> TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
>>> larger than 8 bits. Do so, even if the reference manual doesn't clearly
>>> describe the effect of the field.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> For the ADV7280-M I also have the diffs below applied. Do you think
>> setting BIT_MIPI_DOUBLE_CMPNT is specific to MEDIA_BUS_FMT_UYVY8_2X8?
>
> Do you need MEDIA_BUS_FMT_UYVY8_2X8 ? Neither MEDIA_BUS_FMT_UYVY8_1X16
> nor MEDIA_BUS_FMT_UYVY8_2X8 match exactly how YUV 4:2:2 data is
> transmitted over CSI-2. V4L2 uses MEDIA_BUS_FMT_UYVY8_1X16 as a
> convention.
I just use MEDIA_BUS_FMT_UYVY8_2X8 as the ADV7280 driver sets it. But if the convention is to use MEDIA_BUS_FMT_UYVY8_1X16 for YUV422, then maybe the ADV driver needs to be fixed.
>
>> In the RM it mentions YUV422 in the description of
>> BIT_MIPI_DOUBLE_CMPNT and without setting it, the colors are all
>> wrong.
>
> That's interesting. I've tested YUV 4:2:2 with an OV5640 sensor, and I
> don't recall having to set the MIPI_DOUBLE_CMPNT field. I'll try to
> retest.
>
>> I know this is not really related to this patch. I'm just wondering
>> how to properly support my setup.
>
> It's hard to tell :-( The MIPI_CSI2_ISP_CONFIG PIXEL_MODE and PARALLEL
> fields are not well documented, and neither is the CSI_CR18
> MIPI_DOUBLE_CMPNT field. While the CSIS and the CSI bridge are
> documented, how they're integrated isn't described. So far, I can only
> guess.
>
>> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
>> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
>> @@ -346,6 +346,11 @@ struct csis_pix_format {
>>
>> static const struct csis_pix_format mipi_csis_formats[] = {
>> /* YUV formats. */
>> + {
>> + .code = MEDIA_BUS_FMT_UYVY8_2X8,
>> + .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
>> + .width = 8,
>> + },
>> {
>> .code = MEDIA_BUS_FMT_UYVY8_1X16,
>> .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
>>
>> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
>> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
>> @@ -504,7 +504,7 @@ static void __mipi_csis_set_format(struct csi_state *state)
>> /* Color format */
>> val = mipi_csis_read(state, MIPI_CSIS_ISP_CONFIG_CH(0));
>> val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK);
>> - val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type);
>> + val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type) | MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
>> mipi_csis_write(state, MIPI_CSIS_ISP_CONFIG_CH(0), val);
>>
>> /* Pixel resolution */
>>
>> --- a/drivers/staging/media/imx/imx7-media-csi.c
>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
>> @@ -492,7 +492,8 @@ static void imx7_csi_configure(struct imx7_csi *csi)
>> case MEDIA_BUS_FMT_UYVY8_1X16:
>> case MEDIA_BUS_FMT_YUYV8_2X8:
>> case MEDIA_BUS_FMT_YUYV8_1X16:
>> - cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
>> + cr3 |= BIT_TWO_8BIT_SENSOR;
>> + cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B | BIT_MIPI_DOUBLE_CMPNT;
>
> I notice that you set both PIXEL_MODE_DUAL and MIPI_DOUBLE_CMPNT. Have
> you tried setting neither ?
Yes, but as soon as I don't set PIXEL_MODE_DUAL, I get overflow errors from the MIPI CSI2 controller and it doesn't work at all.
>
> Have you also tried using MEDIA_BUS_FMT_UYVY8_1X16 ? The difference
> between MEDIA_BUS_FMT_UYVY8_2X8 and MEDIA_BUS_FMT_UYVY8_1X16 in this
> driver is the width value passed to v4l2_get_link_freq(). With
> MEDIA_BUS_FMT_UYVY8_2X8 you'll end up with a computed link frequency
> equal to half of the actual value, and thus a wrong Ths_settle value. It
> shuold have no other influence though.
The link frequency calculation doesn't work for me at the moment, as the ADV driver doesn't provide any of the controls V4L2_CID_LINK_FREQ or V4L2_CID_PIXEL_RATE. So for now I just hardcoded the Ths_settle value.
Thanks for your efforts!
Frieder
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats
2021-05-25 9:59 ` Frieder Schrempf
@ 2021-06-10 14:49 ` Laurent Pinchart
0 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2021-06-10 14:49 UTC (permalink / raw)
To: Frieder Schrempf
Cc: linux-media, Rui Miguel Silva, kernel, Fabio Estevam, linux-imx,
Steve Longerbeam, Philipp Zabel, Marek Vasut, Marco Felsch,
Martin Kepplinger, Dorota Czaplejewicz
Hi Frieder,
On Tue, May 25, 2021 at 11:59:40AM +0200, Frieder Schrempf wrote:
> On 19.05.21 02:16, Laurent Pinchart wrote:
> > On Mon, May 17, 2021 at 12:21:17PM +0200, Frieder Schrempf wrote:
> >> On 16.05.21 04:42, Laurent Pinchart wrote:
> >>> Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> >>> capture with an OV5640 sensor connected over CSI-2, showed that the
> >>> TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for formats
> >>> larger than 8 bits. Do so, even if the reference manual doesn't clearly
> >>> describe the effect of the field.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>
> >> For the ADV7280-M I also have the diffs below applied. Do you think
> >> setting BIT_MIPI_DOUBLE_CMPNT is specific to MEDIA_BUS_FMT_UYVY8_2X8?
> >
> > Do you need MEDIA_BUS_FMT_UYVY8_2X8 ? Neither MEDIA_BUS_FMT_UYVY8_1X16
> > nor MEDIA_BUS_FMT_UYVY8_2X8 match exactly how YUV 4:2:2 data is
> > transmitted over CSI-2. V4L2 uses MEDIA_BUS_FMT_UYVY8_1X16 as a
> > convention.
>
> I just use MEDIA_BUS_FMT_UYVY8_2X8 as the ADV7280 driver sets it. But
> if the convention is to use MEDIA_BUS_FMT_UYVY8_1X16 for YUV422, then
> maybe the ADV driver needs to be fixed.
That would be the ideal situation. We need to be careful about not
breaking backward compatibility, but apart from that,
MEDIA_BUS_FMT_UYVY8_1X16 is the way to go.
I started writing a documentation patch, and found it it's already
documented :-) Documentation/userspace-api/media/v4l/subdev-formats.rst
states:
The media bus pixel codes document parallel formats. Should the
pixel data be transported over a serial bus, the media bus pixel
code that describes a parallel format that transfers a sample on a
single clock cycle is used. For instance, both
MEDIA_BUS_FMT_BGR888_1X24 and MEDIA_BUS_FMT_BGR888_3X8 are used on
parallel busses for transferring an 8 bits per sample BGR data,
whereas on serial busses the data in this format is only referred to
using MEDIA_BUS_FMT_BGR888_1X24. This is because there is
effectively only a single way to transport that format on the serial
busses.
> >> In the RM it mentions YUV422 in the description of
> >> BIT_MIPI_DOUBLE_CMPNT and without setting it, the colors are all
> >> wrong.
> >
> > That's interesting. I've tested YUV 4:2:2 with an OV5640 sensor, and I
> > don't recall having to set the MIPI_DOUBLE_CMPNT field. I'll try to
> > retest.
> >
> >> I know this is not really related to this patch. I'm just wondering
> >> how to properly support my setup.
> >
> > It's hard to tell :-( The MIPI_CSI2_ISP_CONFIG PIXEL_MODE and PARALLEL
> > fields are not well documented, and neither is the CSI_CR18
> > MIPI_DOUBLE_CMPNT field. While the CSIS and the CSI bridge are
> > documented, how they're integrated isn't described. So far, I can only
> > guess.
> >
> >> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> >> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> >> @@ -346,6 +346,11 @@ struct csis_pix_format {
> >>
> >> static const struct csis_pix_format mipi_csis_formats[] = {
> >> /* YUV formats. */
> >> + {
> >> + .code = MEDIA_BUS_FMT_UYVY8_2X8,
> >> + .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
> >> + .width = 8,
> >> + },
> >> {
> >> .code = MEDIA_BUS_FMT_UYVY8_1X16,
> >> .data_type = MIPI_CSI2_DATA_TYPE_YUV422_8,
> >>
> >> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> >> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> >> @@ -504,7 +504,7 @@ static void __mipi_csis_set_format(struct csi_state *state)
> >> /* Color format */
> >> val = mipi_csis_read(state, MIPI_CSIS_ISP_CONFIG_CH(0));
> >> val &= ~(MIPI_CSIS_ISPCFG_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK);
> >> - val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type);
> >> + val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type) | MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL;
> >> mipi_csis_write(state, MIPI_CSIS_ISP_CONFIG_CH(0), val);
> >>
> >> /* Pixel resolution */
> >>
> >> --- a/drivers/staging/media/imx/imx7-media-csi.c
> >> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> >> @@ -492,7 +492,8 @@ static void imx7_csi_configure(struct imx7_csi *csi)
> >> case MEDIA_BUS_FMT_UYVY8_1X16:
> >> case MEDIA_BUS_FMT_YUYV8_2X8:
> >> case MEDIA_BUS_FMT_YUYV8_1X16:
> >> - cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
> >> + cr3 |= BIT_TWO_8BIT_SENSOR;
> >> + cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B | BIT_MIPI_DOUBLE_CMPNT;
> >
> > I notice that you set both PIXEL_MODE_DUAL and MIPI_DOUBLE_CMPNT. Have
> > you tried setting neither ?
>
> Yes, but as soon as I don't set PIXEL_MODE_DUAL, I get overflow errors
> from the MIPI CSI2 controller and it doesn't work at all.
What PIXEL_MODE_DUAL and MIPI_DOUBLE_CMPNT do exactly is not totally
clear, we'll have to iron it out to ensure drivers can pick the right
settings automatically.
> > Have you also tried using MEDIA_BUS_FMT_UYVY8_1X16 ? The difference
> > between MEDIA_BUS_FMT_UYVY8_2X8 and MEDIA_BUS_FMT_UYVY8_1X16 in this
> > driver is the width value passed to v4l2_get_link_freq(). With
> > MEDIA_BUS_FMT_UYVY8_2X8 you'll end up with a computed link frequency
> > equal to half of the actual value, and thus a wrong Ths_settle value. It
> > shuold have no other influence though.
>
> The link frequency calculation doesn't work for me at the moment, as
> the ADV driver doesn't provide any of the controls V4L2_CID_LINK_FREQ
> or V4L2_CID_PIXEL_RATE. So for now I just hardcoded the Ths_settle
> value.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats
2021-05-16 2:42 ` [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats Laurent Pinchart
2021-05-17 10:21 ` Rui Miguel Silva
2021-05-17 10:21 ` [RFC PATCH 2/3] " Frieder Schrempf
@ 2021-05-18 11:28 ` Martin Kepplinger
2021-07-28 8:54 ` Martin Kepplinger
3 siblings, 0 replies; 20+ messages in thread
From: Martin Kepplinger @ 2021-05-18 11:28 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: Rui Miguel Silva, kernel, Fabio Estevam, linux-imx,
Steve Longerbeam, Philipp Zabel, Marek Vasut, Marco Felsch,
Dorota Czaplejewicz
Am Sonntag, dem 16.05.2021 um 05:42 +0300 schrieb Laurent Pinchart:
> Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> capture with an OV5640 sensor connected over CSI-2, showed that the
> TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for
> formats
> larger than 8 bits. Do so, even if the reference manual doesn't
> clearly
> describe the effect of the field.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c
> b/drivers/staging/media/imx/imx7-media-csi.c
> index f85a2f5f1413..256b9aa978f0 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -422,6 +422,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
> int width = out_pix->width;
> u32 stride = 0;
> u32 cr1, cr18;
> + u32 cr3 = 0;
>
> cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>
> @@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
> case MEDIA_BUS_FMT_SGBRG10_1X10:
> case MEDIA_BUS_FMT_SGRBG10_1X10:
> case MEDIA_BUS_FMT_SRGGB10_1X10:
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
> break;
> case MEDIA_BUS_FMT_Y12_1X12:
> @@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
> case MEDIA_BUS_FMT_SGBRG12_1X12:
> case MEDIA_BUS_FMT_SGRBG12_1X12:
> case MEDIA_BUS_FMT_SRGGB12_1X12:
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
> break;
> case MEDIA_BUS_FMT_Y14_1X14:
> @@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
> case MEDIA_BUS_FMT_SGBRG14_1X14:
> case MEDIA_BUS_FMT_SGRBG14_1X14:
> case MEDIA_BUS_FMT_SRGGB14_1X14:
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
> break;
> /*
> @@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
>
> imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
> imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16,
> CSI_CSICR2);
> - imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
> + imx7_csi_reg_write(csi, BIT_FRMCNT_RST | cr3, CSI_CSICR3);
> imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
>
> imx7_csi_reg_write(csi, (width * out_pix->height) >> 2,
> CSI_CSIRXCNT);
I can confirm that BIT_TWO_8BIT_SENSOR is needed for raw10 bayer
formats for me here.
Tested-by: Martin Kepplinger <martin.kepplinger@puri.sm>
thanks,
martin
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats
2021-05-16 2:42 ` [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats Laurent Pinchart
` (2 preceding siblings ...)
2021-05-18 11:28 ` Martin Kepplinger
@ 2021-07-28 8:54 ` Martin Kepplinger
2021-07-28 13:20 ` Laurent Pinchart
3 siblings, 1 reply; 20+ messages in thread
From: Martin Kepplinger @ 2021-07-28 8:54 UTC (permalink / raw)
To: Laurent Pinchart, linux-media
Cc: Rui Miguel Silva, kernel, Fabio Estevam, linux-imx,
Steve Longerbeam, Philipp Zabel, Marek Vasut, Marco Felsch,
Dorota Czaplejewicz
Am Sonntag, dem 16.05.2021 um 05:42 +0300 schrieb Laurent Pinchart:
> Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> capture with an OV5640 sensor connected over CSI-2, showed that the
> TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for
> formats
> larger than 8 bits. Do so, even if the reference manual doesn't
> clearly
> describe the effect of the field.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c
> b/drivers/staging/media/imx/imx7-media-csi.c
> index f85a2f5f1413..256b9aa978f0 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -422,6 +422,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
> int width = out_pix->width;
> u32 stride = 0;
> u32 cr1, cr18;
> + u32 cr3 = 0;
>
> cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
>
> @@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
> case MEDIA_BUS_FMT_SGBRG10_1X10:
> case MEDIA_BUS_FMT_SGRBG10_1X10:
> case MEDIA_BUS_FMT_SRGGB10_1X10:
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
> break;
> case MEDIA_BUS_FMT_Y12_1X12:
> @@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
> case MEDIA_BUS_FMT_SGBRG12_1X12:
> case MEDIA_BUS_FMT_SGRBG12_1X12:
> case MEDIA_BUS_FMT_SRGGB12_1X12:
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
> break;
> case MEDIA_BUS_FMT_Y14_1X14:
> @@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
> case MEDIA_BUS_FMT_SGBRG14_1X14:
> case MEDIA_BUS_FMT_SGRBG14_1X14:
> case MEDIA_BUS_FMT_SRGGB14_1X14:
> + cr3 |= BIT_TWO_8BIT_SENSOR;
> cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
> break;
> /*
> @@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi
> *csi)
>
> imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
> imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16,
> CSI_CSICR2);
> - imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
> + imx7_csi_reg_write(csi, BIT_FRMCNT_RST | cr3, CSI_CSICR3);
> imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
>
> imx7_csi_reg_write(csi, (width * out_pix->height) >> 2,
> CSI_CSIRXCNT);
this patch series looks good to me - I need it for the driver to run on
imx8mq.
Reviewed-by: Martin Kepplinger <martin.kepplinger@puri.sm>
Could you either rebase and resend as non-RFC or queue them up more
directly?
thank you,
martin
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [RFC PATCH 2/3] media: imx: imx7-media-csi: Set TWO_8BIT_SENSOR for >= 10-bit formats
2021-07-28 8:54 ` Martin Kepplinger
@ 2021-07-28 13:20 ` Laurent Pinchart
0 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2021-07-28 13:20 UTC (permalink / raw)
To: Martin Kepplinger
Cc: linux-media, Rui Miguel Silva, kernel, Fabio Estevam, linux-imx,
Steve Longerbeam, Philipp Zabel, Marek Vasut, Marco Felsch,
Dorota Czaplejewicz
Hi Martin,
On Wed, Jul 28, 2021 at 10:54:23AM +0200, Martin Kepplinger wrote:
> Am Sonntag, dem 16.05.2021 um 05:42 +0300 schrieb Laurent Pinchart:
> > Sample code from NXP, as well as experiments on i.MX8MM with RAW10
> > capture with an OV5640 sensor connected over CSI-2, showed that the
> > TWO_8BIT_SENSOR field of the CSICR3 register needs to be set for
> > formats
> > larger than 8 bits. Do so, even if the reference manual doesn't
> > clearly
> > describe the effect of the field.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > drivers/staging/media/imx/imx7-media-csi.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/media/imx/imx7-media-csi.c
> > b/drivers/staging/media/imx/imx7-media-csi.c
> > index f85a2f5f1413..256b9aa978f0 100644
> > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > @@ -422,6 +422,7 @@ static void imx7_csi_configure(struct imx7_csi
> > *csi)
> > int width = out_pix->width;
> > u32 stride = 0;
> > u32 cr1, cr18;
> > + u32 cr3 = 0;
> >
> > cr18 = imx7_csi_reg_read(csi, CSI_CSICR18);
> >
> > @@ -464,6 +465,7 @@ static void imx7_csi_configure(struct imx7_csi
> > *csi)
> > case MEDIA_BUS_FMT_SGBRG10_1X10:
> > case MEDIA_BUS_FMT_SGRBG10_1X10:
> > case MEDIA_BUS_FMT_SRGGB10_1X10:
> > + cr3 |= BIT_TWO_8BIT_SENSOR;
> > cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
> > break;
> > case MEDIA_BUS_FMT_Y12_1X12:
> > @@ -471,6 +473,7 @@ static void imx7_csi_configure(struct imx7_csi
> > *csi)
> > case MEDIA_BUS_FMT_SGBRG12_1X12:
> > case MEDIA_BUS_FMT_SGRBG12_1X12:
> > case MEDIA_BUS_FMT_SRGGB12_1X12:
> > + cr3 |= BIT_TWO_8BIT_SENSOR;
> > cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
> > break;
> > case MEDIA_BUS_FMT_Y14_1X14:
> > @@ -478,6 +481,7 @@ static void imx7_csi_configure(struct imx7_csi
> > *csi)
> > case MEDIA_BUS_FMT_SGBRG14_1X14:
> > case MEDIA_BUS_FMT_SGRBG14_1X14:
> > case MEDIA_BUS_FMT_SRGGB14_1X14:
> > + cr3 |= BIT_TWO_8BIT_SENSOR;
> > cr18 |= BIT_MIPI_DATA_FORMAT_RAW14;
> > break;
> > /*
> > @@ -510,7 +514,7 @@ static void imx7_csi_configure(struct imx7_csi
> > *csi)
> >
> > imx7_csi_reg_write(csi, cr1, CSI_CSICR1);
> > imx7_csi_reg_write(csi, BIT_DMA_BURST_TYPE_RFF_INCR16,
> > CSI_CSICR2);
> > - imx7_csi_reg_write(csi, BIT_FRMCNT_RST, CSI_CSICR3);
> > + imx7_csi_reg_write(csi, BIT_FRMCNT_RST | cr3, CSI_CSICR3);
> > imx7_csi_reg_write(csi, cr18, CSI_CSICR18);
> >
> > imx7_csi_reg_write(csi, (width * out_pix->height) >> 2,
> > CSI_CSIRXCNT);
>
> this patch series looks good to me - I need it for the driver to run on
> imx8mq.
>
> Reviewed-by: Martin Kepplinger <martin.kepplinger@puri.sm>
>
> Could you either rebase and resend as non-RFC or queue them up more
> directly?
I've just sent a pull request that includes v1.1 of this patch. I'm
sorry I missed your Reviewed-by tag :-S
https://lore.kernel.org/linux-media/YQFY5FS8v3Y3KkEA@pendragon.ideasonboard.com/T/#u
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 20+ messages in thread