| Message ID | 20220214184318.409208-1-jacopo@jmondi.org |
|---|---|
| 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:16PM +0100, Jacopo Mondi wrote: > Bits 13 and 12 of the ISP_CONFIGn register configure the PIXEL_MODE > which specifies the sampling size, in pixel component units, on the > CSI-2 output data interface when data are transferred to memory. > > The register description in the chip manual specifies that DUAL mode > should be used for YUV422 data but does not clarify the reason. > > Verify if other YUV formats require the same setting and what is the > appropriate setting for RAW and sRGB formats. If it's an action item, shouldn't it be in a TODO comment in the code instead ? While it shouldn't be difficult to test this in RAW8 mode, I'd leave it for later, as I don't want to get into the rabbit hole of adding S[RGB]{4}8_0_5X16 or S[RGB]{4}10_0_5X20 formats now :-) > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com> > --- > drivers/media/platform/imx/imx-mipi-csis.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/imx/imx-mipi-csis.c b/drivers/media/platform/imx/imx-mipi-csis.c > index f433758c8935..98a7538a6ce3 100644 > --- a/drivers/media/platform/imx/imx-mipi-csis.c > +++ b/drivers/media/platform/imx/imx-mipi-csis.c > @@ -173,6 +173,7 @@ > #define MIPI_CSIS_ISPCFG_PIXEL_MODE_SINGLE (0 << 12) > #define MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL (1 << 12) > #define MIPI_CSIS_ISPCFG_PIXEL_MODE_QUAD (2 << 12) /* i.MX8M[MNP] only */ > +#define MIPI_CSIS_ISPCFG_PIXEL_MASK (3 << 12) > #define MIPI_CSIS_ISPCFG_ALIGN_32BIT BIT(11) > #define MIPI_CSIS_ISPCFG_FMT(fmt) ((fmt) << 2) > #define MIPI_CSIS_ISPCFG_FMT_MASK (0x3f << 2) > @@ -506,7 +507,12 @@ 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_ALIGN_32BIT | MIPI_CSIS_ISPCFG_FMT_MASK > + | MIPI_CSIS_ISPCFG_PIXEL_MASK); > + > + if (state->csis_fmt->data_type == MIPI_CSI2_DATA_TYPE_YUV422_8) > + val |= MIPI_CSIS_ISPCFG_PIXEL_MODE_DUAL; > + > val |= MIPI_CSIS_ISPCFG_FMT(state->csis_fmt->data_type); > mipi_csis_write(state, MIPI_CSIS_ISP_CONFIG_CH(0), val); >
Hi Jacopo, On Tue, Feb 15, 2022 at 09:36:31AM +0100, Jacopo Mondi wrote: > On Mon, Feb 14, 2022 at 09:15:07PM +0200, Laurent Pinchart wrote: > > On Mon, Feb 14, 2022 at 07:43:13PM +0100, Jacopo Mondi wrote: > > > The imx7-media-csi driver controls the CSI (CMOS Sensor Interface) > > > peripheral available on several SoC of different generations. > > > > > > The current situation when it comes to compatible strings is rather > > > confused: > > > - Bindings document imx6ul, imx7 and imx8mm > > > - Driver supports imx6ul, imx7 and imx8mq > > > - The IMX8MM and IMX8MQ DTS use the correct compatible strings with a > > > fallback to the generic "imx7-csi" identifier: > > > arch/arm64/boot/dts/freescale/imx8mq.dtsi: compatible = "fsl,imx8mq-csi", > > > arch/arm64/boot/dts/freescale/imx8mm.dtsi: compatible = "fsl,imx8mm-csi", > > > > > > Tidy-up the situation by adding the IMX8MQ compatible string to the > > > bindings documentation andathe IMX8MM identifier to the driver, to allow > > > to distinguish the SoC the CSI peripheral is integrated on in the > > > following patches. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml | 1 + > > > drivers/staging/media/imx/imx7-media-csi.c | 2 ++ > > > > I think Rob would prefer this being split in two patches, and I think it > > would make sense, as you're fixing two separate issues. > > > > > 2 files changed, 3 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml > > > index 4f7b78265336..0f1505d85111 100644 > > > --- a/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml > > > +++ b/Documentation/devicetree/bindings/media/nxp,imx7-csi.yaml > > > @@ -21,6 +21,7 @@ properties: > > > - fsl,imx7-csi > > > - fsl,imx6ul-csi > > > - items: > > > + - const: fsl,imx8mq-csi > > > - const: fsl,imx8mm-csi > > > - const: fsl,imx7-csi > > > > I don't think you intended to require the following: > > > > compatible = "fsl,imx8mq-csi", "fsl,imx8mm-csi", "fsl,imx7-csi"; > > No, I kind of superficially added the mq version where the mm was > already and went on :) > > Care to explain why currently we have two const for the "8mm" and the > "imx7" versions ? Because the imx8mm version was considered compatible with the imx7, so - items: - const: fsl,imx8mm-csi - const: fsl,imx7-csi will validate compatible = "fsl,imx8mm-csi", "fsl,imx7-csi"; The first imx8mm compatible string is ignored by the driver, but documented to support future drivers changes that would require differentiating between the two versions. > > You probably want > > > > properties: > > compatible: > > oneOf: > > - enum: > > + - fsl,imx8mq-csi > > - fsl,imx7-csi > > - fsl,imx6ul-csi > > - items: > > - const: fsl,imx8mm-csi > > - const: fsl,imx7-csi > > > > instead. > > I'm not aware of how how many revisions of the imx7 and imx6 versions > exists, nor how they differ, but the existing distinction feels a bit > weird. > > The const items should be the compatible fallbacks, should them be > generic, why is 8mm among them ? Shouldn't we specify the precise SoC > version in the list of possible enum items only ? No, the const items are not the compatible fallbacks. imx8mm isn't a generic fallback. "items" requires *all* items to be present in order to validate. > Something like > > oneOf: > - enum: > - fsl,imx8mq-csi > - fsl,imx8mm-csi > - fsl,imx6ul-csi > - const: > - fsl,imx7-csi That's not a valid schema. > > In example I see: > > arch/arm64/boot/dts/freescale/imx8mq.dtsi: compatible = "fsl,imx8mq-csi", "fsl,imx7-csi"; > > Where this should either be > compatible = "fsl,imx8mq-csi" > or > compatible = "fsl,imx8mm-csi", "fsl,imx7-csi"; > > ? > > > > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c > > > index 32311fc0e2a4..59100e409709 100644 > > > --- a/drivers/staging/media/imx/imx7-media-csi.c > > > +++ b/drivers/staging/media/imx/imx7-media-csi.c > > > @@ -162,6 +162,7 @@ > > > enum imx_csi_model { > > > IMX7_CSI_IMX7 = 0, > > > IMX7_CSI_IMX8MQ, > > > + IMX7_CSI_IMX8MM, > > > }; > > > > > > struct imx7_csi { > > > @@ -1277,6 +1278,7 @@ static int imx7_csi_remove(struct platform_device *pdev) > > > > > > static const struct of_device_id imx7_csi_of_match[] = { > > > { .compatible = "fsl,imx8mq-csi", .data = (void *)IMX7_CSI_IMX8MQ }, > > > + { .compatible = "fsl,imx8mm-csi", .data = (void *)IMX7_CSI_IMX8MM }, > > > > This isn't needed, as the i.MX8MM CSI bridgge is considered fully > > backward-compatible with the i.MX7 version. I'd introduce this change in > > the patch where you start using IMX7_CSI_IMX8MM, and I would then add > > the following to the DT binding: > > > > properties: > > compatible: > > oneOf: > > - enum: > > - fsl,imx8mq-csi > > + - fsl,imx8mm-csi > > - fsl,imx7-csi > > - fsl,imx6ul-csi > > - items: > > - const: fsl,imx8mm-csi > > - const: fsl,imx7-csi > > > > to allow setting > > > > compatible = "fsl,imx8mm-csi"; > > > > without the imx7 fallback if we consider the i.MX8MM version different. > > If the driver can operate correctly on the i.MX8MM when using the i.MX7 > > fallback code paths (possibly minor issues that are not considered > > fatal, such as missing features) then you could skip this binding > > change. > > Sorry, but shouldn't: > > compatible = "fsl,imx8mm-csi", fsl,imx7-csi" > > allow me to match on imx8mm already, without the above change. > > I think what I don't get is why imx8mm is a 'generic fallback' in > first place. See above. > > > { .compatible = "fsl,imx7-csi", .data = (void *)IMX7_CSI_IMX7 }, > > > { .compatible = "fsl,imx6ul-csi", .data = (void *)IMX7_CSI_IMX7 }, > > > { },
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; > > > } > > > }
Hello, this series includes patches from two series previously sent https://lore.kernel.org/linux-media/20220119112024.11339-1-jacopo@jmondi.org/ https://lore.kernel.org/linux-media/20220211180216.290133-1-jacopo@jmondi.org/ Which can now be marked as superseded. The first 2 patches performs the de-staging of the imx7-mipi-csis driver and takes into account comments recevied there. The rest of the series builds on top of the comment received on: https://lore.kernel.org/linux-media/20220119112024.11339-3-jacopo@jmondi.org/ If DUAL pixel mode is used in the CSIS driver, then the CSI block of the IMX8MM SoC needs to be operated in dual mode as well. To do so, create per-SoC configurations in imx7-media-csi.c and only set dual mode for the MM model leaving the other ones untouched as they connect to a different CSI-2 receiver which instead operates in single mode. I've only tested on i.MX8MP which is not affected by these changes, so I hope I've not broke anything. Laurent could you test on MM to see if it works now ? On top two small patches I was carrying in my tree to add more formats to the CSIS driver. Series based on top of the most recent media master branch. Thanks j Jacopo Mondi (9): media: imx: De-stage imx7-mipi-csis media: imx: Rename imx7-mipi-csis.c to imx-mipi-csis.c staging: media: imx: Add more compatible strings staging: media: imx: Define per-SoC info staging: media: imx: Use DUAL pixel mode if available media: imx: imx-mipi-csis: Set PIXEL_MODE for YUV422 media: imx: imx-mipi-csis: Add RGB565_1X16 media: imx: imx-mipi-csis: Add RGB/BGR888 Documentation/admin-guide/media/imx7.rst | 2 +- ...-mipi-csi2.yaml => nxp,imx-mipi-csi2.yaml} | 2 +- .../bindings/media/nxp,imx7-csi.yaml | 1 + MAINTAINERS | 4 +- drivers/media/platform/Kconfig | 1 + drivers/media/platform/Makefile | 1 + drivers/media/platform/imx/Kconfig | 24 +++++++++ drivers/media/platform/imx/Makefile | 1 + .../platform/imx/imx-mipi-csis.c} | 44 ++++++++++++++-- drivers/staging/media/imx/Makefile | 1 - drivers/staging/media/imx/imx-media.h | 44 ++++++++++++++++ drivers/staging/media/imx/imx7-media-csi.c | 52 ++++++++++++++----- 12 files changed, 153 insertions(+), 24 deletions(-) rename Documentation/devicetree/bindings/media/{nxp,imx7-mipi-csi2.yaml => nxp,imx-mipi-csi2.yaml} (98%) create mode 100644 drivers/media/platform/imx/Kconfig create mode 100644 drivers/media/platform/imx/Makefile rename drivers/{staging/media/imx/imx7-mipi-csis.c => media/platform/imx/imx-mipi-csis.c} (97%) -- 2.35.0