| Message ID | 20210516024216.4576-3-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,1/3] dt-bindings: media: nxp,imx7-csi: Add i.MX8MM support | expand |
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
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); >
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
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
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
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
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
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
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
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);
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(-)