Re: [PATCH 5/8] staging: media: imx: Use DUAL pixel mode if available

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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;
> > >  		}
> > >  	}

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel