| Message ID | 20220214184318.409208-6-jacopo@jmondi.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx: Destage imx7-mipi-csis with fixes on top | expand |
Hi Jacopo, Thank you for the patch. On Mon, Feb 14, 2022 at 07:43:15PM +0100, Jacopo Mondi wrote: > The pixel sampling mode controls the size of data sampled from the CSI > Rx queue. The supported sample size depends on the configuration of the > preceding block in the capture pipeline and is then dependent on the SoC > version the CSI peripheral is integrated on. > > When capturing YUV422 data if dual sample mode is available use it. > > This change is particularly relevant for the IMX8MM SoC which uses the > CSIS CSI-2 receiver which operates in dual pixel mode. > > Other SoCs should be unaffected by this change and should continue to > operate as before. > > Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > drivers/staging/media/imx/imx7-media-csi.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c > index 112096774961..a8bdfb0bb0ee 100644 > --- a/drivers/staging/media/imx/imx7-media-csi.c > +++ b/drivers/staging/media/imx/imx7-media-csi.c > @@ -426,6 +426,7 @@ static void imx7_csi_configure(struct imx7_csi *csi) > { > struct imx_media_video_dev *vdev = csi->vdev; > struct v4l2_pix_format *out_pix = &vdev->fmt; > + struct imx_media_dev *imxmd = csi->imxmd; > int width = out_pix->width; > u32 stride = 0; > u32 cr3 = BIT_FRMCNT_RST; > @@ -436,7 +437,7 @@ static void imx7_csi_configure(struct imx7_csi *csi) > cr18 &= ~(BIT_CSI_HW_ENABLE | BIT_MIPI_DATA_FORMAT_MASK | > BIT_DATA_FROM_MIPI | BIT_BASEADDR_CHG_ERR_EN | > BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL | > - BIT_DEINTERLACE_EN); > + BIT_DEINTERLACE_EN | BIT_MIPI_DOUBLE_CMPNT); > > if (out_pix->field == V4L2_FIELD_INTERLACED) { > cr18 |= BIT_DEINTERLACE_EN; > @@ -500,6 +501,13 @@ static void imx7_csi_configure(struct imx7_csi *csi) > case MEDIA_BUS_FMT_YUYV8_2X8: > case MEDIA_BUS_FMT_YUYV8_1X16: > cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B; > + > + /* If dual mode is supported use it. */ > + if (imxmd->info->sample_modes & MODE_DUAL) { > + cr18 |= BIT_MIPI_DOUBLE_CMPNT; > + cr3 |= BIT_TWO_8BIT_SENSOR; > + } I would implement this differently: case MEDIA_BUS_FMT_UYVY8_2X8: case MEDIA_BUS_FMT_YUYV8_2X8: cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B; break; case MEDIA_BUS_FMT_UYVY8_1X16: case MEDIA_BUS_FMT_YUYV8_1X16: cr3 |= BIT_TWO_8BIT_SENSOR; cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B | BIT_MIPI_DOUBLE_CMPNT; break; This would support either option here. What you will then need to change is imx7_csi_enum_mbus_code() and imx7_csi_try_fmt(), to allow/disallow the 2X8 and 1X16 variants based on the SoC. This is important for the i.MX7, which has both a CSI-2 input and a parallel input. When using the CSIS it can (and should) use double component mode, while when using the parallel input it can work in 8-bit or 16-bit mode depending on how the sensor is wired. > + > break; > } > }
Hi Laurent, On Tue, Feb 15, 2022 at 09:12:44AM +0200, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Mon, Feb 14, 2022 at 07:43:15PM +0100, Jacopo Mondi wrote: > > The pixel sampling mode controls the size of data sampled from the CSI > > Rx queue. The supported sample size depends on the configuration of the > > preceding block in the capture pipeline and is then dependent on the SoC > > version the CSI peripheral is integrated on. > > > > When capturing YUV422 data if dual sample mode is available use it. > > > > This change is particularly relevant for the IMX8MM SoC which uses the > > CSIS CSI-2 receiver which operates in dual pixel mode. > > > > Other SoCs should be unaffected by this change and should continue to > > operate as before. > > > > Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > drivers/staging/media/imx/imx7-media-csi.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c > > index 112096774961..a8bdfb0bb0ee 100644 > > --- a/drivers/staging/media/imx/imx7-media-csi.c > > +++ b/drivers/staging/media/imx/imx7-media-csi.c > > @@ -426,6 +426,7 @@ static void imx7_csi_configure(struct imx7_csi *csi) > > { > > struct imx_media_video_dev *vdev = csi->vdev; > > struct v4l2_pix_format *out_pix = &vdev->fmt; > > + struct imx_media_dev *imxmd = csi->imxmd; > > int width = out_pix->width; > > u32 stride = 0; > > u32 cr3 = BIT_FRMCNT_RST; > > @@ -436,7 +437,7 @@ static void imx7_csi_configure(struct imx7_csi *csi) > > cr18 &= ~(BIT_CSI_HW_ENABLE | BIT_MIPI_DATA_FORMAT_MASK | > > BIT_DATA_FROM_MIPI | BIT_BASEADDR_CHG_ERR_EN | > > BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL | > > - BIT_DEINTERLACE_EN); > > + BIT_DEINTERLACE_EN | BIT_MIPI_DOUBLE_CMPNT); > > > > if (out_pix->field == V4L2_FIELD_INTERLACED) { > > cr18 |= BIT_DEINTERLACE_EN; > > @@ -500,6 +501,13 @@ static void imx7_csi_configure(struct imx7_csi *csi) > > case MEDIA_BUS_FMT_YUYV8_2X8: > > case MEDIA_BUS_FMT_YUYV8_1X16: > > cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B; > > + > > + /* If dual mode is supported use it. */ > > + if (imxmd->info->sample_modes & MODE_DUAL) { > > + cr18 |= BIT_MIPI_DOUBLE_CMPNT; > > + cr3 |= BIT_TWO_8BIT_SENSOR; > > + } > > I would implement this differently: > > case MEDIA_BUS_FMT_UYVY8_2X8: > case MEDIA_BUS_FMT_YUYV8_2X8: > cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B; > break; > > case MEDIA_BUS_FMT_UYVY8_1X16: > case MEDIA_BUS_FMT_YUYV8_1X16: > cr3 |= BIT_TWO_8BIT_SENSOR; > cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B > | BIT_MIPI_DOUBLE_CMPNT; > break; > > This would support either option here. What you will then need to change > is imx7_csi_enum_mbus_code() and imx7_csi_try_fmt(), to allow/disallow > the 2X8 and 1X16 variants based on the SoC. This is important for the Exactly. And being the format list in the shared helper I decided it was really not worth adding any SoC specific code to those helpers which should ideally be nuked. Should we decouple the helpers to get to a point where we can have SoC-specific formats list ? Then I would be more than happy to use the above construct. There is one quirk I'm not sure about: How does the CSI connect to the transmitter ? It is my undertanding that in i.MX8 connects to the the CSI-2 receiver (either CSIS or the Northwest tech one). Does in older SoC revisions connects directly to a parallel sensor ? I'm asking also because there is a comment that reports /* * CSI-2 sources are supposed to use the 1X16 formats, but not * all of them comply. Support both variants. */ But if the preceding block is the CSI-2 receiver, we control what formats it exposes and we can use both 2X8 or 1X16 depending on the SoC specificities and we don't care about the CSI-2 Tx supported formats. Does that comment make sense in your opinion ? Also be aware that, in example, if we expose from the CSIS source pad both 2X8 and 1X16 we create a condition where userspace could configure the pipeline uncorrectly. Let's draw a table (for i.MX8 only as I don't know about the 7) 8MM/CSI 8MQ/CSI 8MP/ISI CSIS Dual Dual NW Single Each block would then support CSIS Dual NW Single CSI Dual/Single ISI Dual So we create a potential for a misconfiguration in 8MM with CSIS = Dual CSI = Single or for the 8MQ NW = Single CSI = Double If we don't create a list of SoC specific formats. Nothing bad, -EPIPE will be returned, but maybe we should avoid that ? > i.MX7, which has both a CSI-2 input and a parallel input. When using the > CSIS it can (and should) use double component mode, while when using the > parallel input it can work in 8-bit or 16-bit mode depending on how the > sensor is wired. I didn't know i.MX7 used the CSIS :/ Ah wait, the driver was called imx7-mipi-csi2 :/ > > > + > > break; > > } > > } > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, On Tue, Feb 15, 2022 at 09:59:18AM +0100, Jacopo Mondi wrote: > On Tue, Feb 15, 2022 at 09:12:44AM +0200, Laurent Pinchart wrote: > > On Mon, Feb 14, 2022 at 07:43:15PM +0100, Jacopo Mondi wrote: > > > The pixel sampling mode controls the size of data sampled from the CSI > > > Rx queue. The supported sample size depends on the configuration of the > > > preceding block in the capture pipeline and is then dependent on the SoC > > > version the CSI peripheral is integrated on. > > > > > > When capturing YUV422 data if dual sample mode is available use it. > > > > > > This change is particularly relevant for the IMX8MM SoC which uses the > > > CSIS CSI-2 receiver which operates in dual pixel mode. > > > > > > Other SoCs should be unaffected by this change and should continue to > > > operate as before. > > > > > > Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > drivers/staging/media/imx/imx7-media-csi.c | 10 +++++++++- > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c > > > index 112096774961..a8bdfb0bb0ee 100644 > > > --- a/drivers/staging/media/imx/imx7-media-csi.c > > > +++ b/drivers/staging/media/imx/imx7-media-csi.c > > > @@ -426,6 +426,7 @@ static void imx7_csi_configure(struct imx7_csi *csi) > > > { > > > struct imx_media_video_dev *vdev = csi->vdev; > > > struct v4l2_pix_format *out_pix = &vdev->fmt; > > > + struct imx_media_dev *imxmd = csi->imxmd; > > > int width = out_pix->width; > > > u32 stride = 0; > > > u32 cr3 = BIT_FRMCNT_RST; > > > @@ -436,7 +437,7 @@ static void imx7_csi_configure(struct imx7_csi *csi) > > > cr18 &= ~(BIT_CSI_HW_ENABLE | BIT_MIPI_DATA_FORMAT_MASK | > > > BIT_DATA_FROM_MIPI | BIT_BASEADDR_CHG_ERR_EN | > > > BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL | > > > - BIT_DEINTERLACE_EN); > > > + BIT_DEINTERLACE_EN | BIT_MIPI_DOUBLE_CMPNT); > > > > > > if (out_pix->field == V4L2_FIELD_INTERLACED) { > > > cr18 |= BIT_DEINTERLACE_EN; > > > @@ -500,6 +501,13 @@ static void imx7_csi_configure(struct imx7_csi *csi) > > > case MEDIA_BUS_FMT_YUYV8_2X8: > > > case MEDIA_BUS_FMT_YUYV8_1X16: > > > cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B; > > > + > > > + /* If dual mode is supported use it. */ > > > + if (imxmd->info->sample_modes & MODE_DUAL) { > > > + cr18 |= BIT_MIPI_DOUBLE_CMPNT; > > > + cr3 |= BIT_TWO_8BIT_SENSOR; > > > + } > > > > I would implement this differently: > > > > case MEDIA_BUS_FMT_UYVY8_2X8: > > case MEDIA_BUS_FMT_YUYV8_2X8: > > cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B; > > break; > > > > case MEDIA_BUS_FMT_UYVY8_1X16: > > case MEDIA_BUS_FMT_YUYV8_1X16: > > cr3 |= BIT_TWO_8BIT_SENSOR; > > cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B > > | BIT_MIPI_DOUBLE_CMPNT; > > break; > > > > This would support either option here. What you will then need to change > > is imx7_csi_enum_mbus_code() and imx7_csi_try_fmt(), to allow/disallow > > the 2X8 and 1X16 variants based on the SoC. This is important for the > > Exactly. And being the format list in the shared helper I decided it > was really not worth adding any SoC specific code to those helpers > which should ideally be nuked. > > Should we decouple the helpers to get to a point where we can have > SoC-specific formats list ? Then I would be more than happy to use the > above construct. Yes, we should. I have started working on this, but haven't had time to complete the work yet. > There is one quirk I'm not sure about: How does the CSI connect to the > transmitter ? It is my undertanding that in i.MX8 connects to the the > CSI-2 receiver (either CSIS or the Northwest tech one). Does in older > SoC revisions connects directly to a parallel sensor ? On i.MX7 there's a mux that allows selecting the CSI-2 receiver or the parallel input. It's implemented using the video-mux driver, see imx7s.dtsi for details. > I'm asking also because there is a comment that reports > > /* > * CSI-2 sources are supposed to use the 1X16 formats, but not > * all of them comply. Support both variants. > */ > > But if the preceding block is the CSI-2 receiver, we control what > formats it exposes and we can use both 2X8 or 1X16 depending on the > SoC specificities and we don't care about the CSI-2 Tx supported > formats. Does that comment make sense in your opinion ? I think the comment should be dropped. There are CSI-2 sensor drivers that incorrectly use the 2X8 formats instead of 1X16, but I don't think we should work around that here, the sensor drivers should be fixed instead. > Also be aware that, in example, if we expose from the CSIS source pad > both 2X8 and 1X16 we create a condition where userspace could > configure the pipeline uncorrectly. > > Let's draw a table (for i.MX8 only as I don't know about the 7) > > > 8MM/CSI 8MQ/CSI 8MP/ISI > CSIS Dual Dual > NW Single > > > Each block would then support > > CSIS Dual > NW Single > CSI Dual/Single > ISI Dual > > So we create a potential for a misconfiguration in 8MM with > > CSIS = Dual > CSI = Single > > or for the 8MQ > > NW = Single > CSI = Double > > If we don't create a list of SoC specific formats. Nothing bad, -EPIPE > will be returned, but maybe we should avoid that ? Format propagation is typically done from source to sink in userspace, so the risk of picking an incorrect format is very low (and it would be a userspace bug in any case). Nonetheless, it would be nice to avoid exposing formats that can't be used, but that can be done later. > > i.MX7, which has both a CSI-2 input and a parallel input. When using the > > CSIS it can (and should) use double component mode, while when using the > > parallel input it can work in 8-bit or 16-bit mode depending on how the > > sensor is wired. > > I didn't know i.MX7 used the CSIS :/ > Ah wait, the driver was called imx7-mipi-csi2 :/ :-) > > > + > > > break; > > > } > > > }
diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c index 112096774961..a8bdfb0bb0ee 100644 --- a/drivers/staging/media/imx/imx7-media-csi.c +++ b/drivers/staging/media/imx/imx7-media-csi.c @@ -426,6 +426,7 @@ static void imx7_csi_configure(struct imx7_csi *csi) { struct imx_media_video_dev *vdev = csi->vdev; struct v4l2_pix_format *out_pix = &vdev->fmt; + struct imx_media_dev *imxmd = csi->imxmd; int width = out_pix->width; u32 stride = 0; u32 cr3 = BIT_FRMCNT_RST; @@ -436,7 +437,7 @@ static void imx7_csi_configure(struct imx7_csi *csi) cr18 &= ~(BIT_CSI_HW_ENABLE | BIT_MIPI_DATA_FORMAT_MASK | BIT_DATA_FROM_MIPI | BIT_BASEADDR_CHG_ERR_EN | BIT_BASEADDR_SWITCH_EN | BIT_BASEADDR_SWITCH_SEL | - BIT_DEINTERLACE_EN); + BIT_DEINTERLACE_EN | BIT_MIPI_DOUBLE_CMPNT); if (out_pix->field == V4L2_FIELD_INTERLACED) { cr18 |= BIT_DEINTERLACE_EN; @@ -500,6 +501,13 @@ static void imx7_csi_configure(struct imx7_csi *csi) case MEDIA_BUS_FMT_YUYV8_2X8: case MEDIA_BUS_FMT_YUYV8_1X16: cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B; + + /* If dual mode is supported use it. */ + if (imxmd->info->sample_modes & MODE_DUAL) { + cr18 |= BIT_MIPI_DOUBLE_CMPNT; + cr3 |= BIT_TWO_8BIT_SENSOR; + } + break; } }