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@xxxxxxxxxxx>
> > Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
> > ---
> > 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
