[00/23] media: imx: imx7-mipi-csis: Add i.MX8MM support

Message ID	20210413023014.28797-1-laurent.pinchart@ideasonboard.com (mailing list archive)
Headers	show Return-Path: <linux-media-owner@kernel.org> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com> To: linux-media@vger.kernel.org Cc: Rui Miguel Silva <rmfrfs@gmail.com>, kernel@pengutronix.de, Fabio Estevam <festevam@gmail.com>, linux-imx@nxp.com, Steve Longerbeam <slongerbeam@gmail.com>, Philipp Zabel <p.zabel@pengutronix.de>, Marek Vasut <marex@denx.de>, Rob Herring <robh+dt@kernel.org>, devicetree@vger.kernel.org Subject: [PATCH 00/23] media: imx: imx7-mipi-csis: Add i.MX8MM support Date: Tue, 13 Apr 2021 05:29:51 +0300 Message-Id: <20210413023014.28797-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	media: imx: imx7-mipi-csis: Add i.MX8MM support \| expand [00/23] media: imx: imx7-mipi-csis: Add i.MX8MM support [01/23] media: imx: imx7_mipi_csis: Fix logging of only error event counters [02/23] media: imx: imx7_mipi_csis: Count the CSI-2 debug interrupts [03/23] media: imx: imx7_mipi_csis: Update ISP_CONFIG macros for quad pixel mode [04/23] media: imx: imx7_mipi_csis: Move static data to top of mipi_csis_dump_regs() [05/23] media: imx: imx7_mipi_csis: Minimize locking in get/set format [06/23] media: imx: imx7_mipi_csis: Don't set subdev data [07/23] media: imx: imx7-mipi-csis: Reorganize code in sections [08/23] media: imx: imx7_mipi_csis: Set the CLKSETTLE register field [09/23] media: imx: imx7_mipi_csis: Drop unused csis_hw_reset structure [10/23] media: imx: imx7_mipi_csis: Store CSI-2 data type in format structure [11/23] media: imx: imx7_mipi_csis: Drop csi_state phy field [12/23] media: imx: imx7_mipi_csis: Rename mipi_sd to sd [13/23] media: imx: imx7_mipi_csis: Rename csi_state flag field to state [14/23] media: imx: imx7_mipi_csis: Turn csi_state irq field into local variable [15/23] media: imx: imx7_mipi_csis: Don't pass pdev to mipi_csis_parse_dt() [16/23] media: imx: imx7_mipi_csis: Pass csi_state to mipi_csis_subdev_init() [17/23] media: imx: imx7_mipi_csis: Drop csi_state pdev field [18/23] media: imx: imx7_mipi_csis: Make csi_state num_clocks field unsigned [19/23] media: imx: imx7_mipi_csis: Reorganize csi_state structure [20/23] media: imx: imx7_mipi_csis: Reorganize mipi_csis_probe() [21/23] media: imx: imx7_mipi_csis: Reject invalid data-lanes settings [22/23] dt-bindings: media: nxp,imx7-mipi-csi2: Add i.MX8MM support [23/23] media: imx: imx7_mipi_csis: Add i.MX8MM support [24/23] media: imx: imx7_mipi_csis: Update MAINTAINERS

Laurent Pinchart April 13, 2021, 2:29 a.m. UTC

Hello,

This patch series adds support for the CSIS found in the NXP i.MX8MM SoC
to the imx7-mipi-csis driver.

The CSIS is an IP core from Samsung, integrated in different NXP SoCs.
The driver currently supports v3.3 of the CSIS, found in SoCs from the
i.MX6 and i.MX7 families. This series extends the driver to support
v3.6.3 of the IP, found in i.MX8MM and other members of the i.MX8
family.

The first 21 patches are miscellaneous cleanups and improvements. Please
see individual patches for details.

Patch 22/23 extends the imx7-mipi-csis DT bindings with i.MX8MM support.
Support for other members of the i.MX8 family will come later, and for
SoCs including an ISI IP core (such as the i.MX8MP) this will require
more work to handle additional glue logic.

Patch 23/23 finaly extends the imx7-mipi-csis driver accordingly.

The changes in the integration of the CSIS between i.MX7 and i.MX8, as
described in the DT bindings, have been found through reading of
reference manuals and BSP source code, with different sources of
information contradicting each other. A confirmation from NXP would be
nice (in particular regarding the clocks).

Laurent Pinchart (23):
  media: imx: imx7_mipi_csis: Fix logging of only error event counters
  media: imx: imx7_mipi_csis: Count the CSI-2 debug interrupts
  media: imx: imx7_mipi_csis: Update ISP_CONFIG macros for quad pixel
    mode
  media: imx: imx7_mipi_csis: Move static data to top of
    mipi_csis_dump_regs()
  media: imx: imx7_mipi_csis: Minimize locking in get/set format
  media: imx: imx7_mipi_csis: Don't set subdev data
  media: imx: imx7-mipi-csis: Reorganize code in sections
  media: imx: imx7_mipi_csis: Set the CLKSETTLE register field
  media: imx: imx7_mipi_csis: Drop unused csis_hw_reset structure
  media: imx: imx7_mipi_csis: Store CSI-2 data type in format structure
  media: imx: imx7_mipi_csis: Drop csi_state phy field
  media: imx: imx7_mipi_csis: Rename mipi_sd to sd
  media: imx: imx7_mipi_csis: Rename csi_state flag field to state
  media: imx: imx7_mipi_csis: Turn csi_state irq field into local
    variable
  media: imx: imx7_mipi_csis: Don't pass pdev to mipi_csis_parse_dt()
  media: imx: imx7_mipi_csis: Pass csi_state to mipi_csis_subdev_init()
  media: imx: imx7_mipi_csis: Drop csi_state pdev field
  media: imx: imx7_mipi_csis: Make csi_state num_clocks field unsigned
  media: imx: imx7_mipi_csis: Reorganize csi_state structure
  media: imx: imx7_mipi_csis: Reorganize mipi_csis_probe()
  media: imx: imx7_mipi_csis: Reject invalid data-lanes settings
  dt-bindings: media: nxp,imx7-mipi-csi2: Add i.MX8MM support
  media: imx: imx7_mipi_csis: Add i.MX8MM support

 .../bindings/media/nxp,imx7-mipi-csi2.yaml    | 108 +-
 drivers/staging/media/imx/imx7-mipi-csis.c    | 943 ++++++++++--------
 2 files changed, 622 insertions(+), 429 deletions(-)

Rui Miguel Silva April 15, 2021, 9:27 a.m. UTC | #1

Hey Laurent,
On Tue, Apr 13, 2021 at 05:29:51AM +0300, Laurent Pinchart wrote:
> Hello,
> 
> This patch series adds support for the CSIS found in the NXP i.MX8MM SoC
> to the imx7-mipi-csis driver.
> 
> The CSIS is an IP core from Samsung, integrated in different NXP SoCs.
> The driver currently supports v3.3 of the CSIS, found in SoCs from the
> i.MX6 and i.MX7 families. This series extends the driver to support
> v3.6.3 of the IP, found in i.MX8MM and other members of the i.MX8
> family.
> 
> The first 21 patches are miscellaneous cleanups and improvements. Please
> see individual patches for details.
> 
> Patch 22/23 extends the imx7-mipi-csis DT bindings with i.MX8MM support.
> Support for other members of the i.MX8 family will come later, and for
> SoCs including an ISI IP core (such as the i.MX8MP) this will require
> more work to handle additional glue logic.
> 
> Patch 23/23 finaly extends the imx7-mipi-csis driver accordingly.
> 
> The changes in the integration of the CSIS between i.MX7 and i.MX8, as
> described in the DT bindings, have been found through reading of
> reference manuals and BSP source code, with different sources of
> information contradicting each other. A confirmation from NXP would be
> nice (in particular regarding the clocks).

Thanks a lot for this series, looks all very good to me.

Will only ask you, as we already talked, to add your name in
the bindings file as maintainer and extend also the MAINTAINERS file
entry with your details.

You have a lot more hw/code working with this driver and also
time/expertise.

It can be in a follow patch no need to spin a new series for this.

For all patches in this series:
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>

Thanks,
------
Cheers,
     Rui

> 
> Laurent Pinchart (23):
>   media: imx: imx7_mipi_csis: Fix logging of only error event counters
>   media: imx: imx7_mipi_csis: Count the CSI-2 debug interrupts
>   media: imx: imx7_mipi_csis: Update ISP_CONFIG macros for quad pixel
>     mode
>   media: imx: imx7_mipi_csis: Move static data to top of
>     mipi_csis_dump_regs()
>   media: imx: imx7_mipi_csis: Minimize locking in get/set format
>   media: imx: imx7_mipi_csis: Don't set subdev data
>   media: imx: imx7-mipi-csis: Reorganize code in sections
>   media: imx: imx7_mipi_csis: Set the CLKSETTLE register field
>   media: imx: imx7_mipi_csis: Drop unused csis_hw_reset structure
>   media: imx: imx7_mipi_csis: Store CSI-2 data type in format structure
>   media: imx: imx7_mipi_csis: Drop csi_state phy field
>   media: imx: imx7_mipi_csis: Rename mipi_sd to sd
>   media: imx: imx7_mipi_csis: Rename csi_state flag field to state
>   media: imx: imx7_mipi_csis: Turn csi_state irq field into local
>     variable
>   media: imx: imx7_mipi_csis: Don't pass pdev to mipi_csis_parse_dt()
>   media: imx: imx7_mipi_csis: Pass csi_state to mipi_csis_subdev_init()
>   media: imx: imx7_mipi_csis: Drop csi_state pdev field
>   media: imx: imx7_mipi_csis: Make csi_state num_clocks field unsigned
>   media: imx: imx7_mipi_csis: Reorganize csi_state structure
>   media: imx: imx7_mipi_csis: Reorganize mipi_csis_probe()
>   media: imx: imx7_mipi_csis: Reject invalid data-lanes settings
>   dt-bindings: media: nxp,imx7-mipi-csi2: Add i.MX8MM support
>   media: imx: imx7_mipi_csis: Add i.MX8MM support
> 
>  .../bindings/media/nxp,imx7-mipi-csi2.yaml    | 108 +-
>  drivers/staging/media/imx/imx7-mipi-csis.c    | 943 ++++++++++--------
>  2 files changed, 622 insertions(+), 429 deletions(-)
> 
> -- 
> Regards,
> 
> Laurent Pinchart
>

Laurent Pinchart April 18, 2021, 8:21 p.m. UTC | #2

Hi Rui,

On Thu, Apr 15, 2021 at 10:27:55AM +0100, Rui Miguel Silva wrote:
> On Tue, Apr 13, 2021 at 05:29:51AM +0300, Laurent Pinchart wrote:
> > Hello,
> > 
> > This patch series adds support for the CSIS found in the NXP i.MX8MM SoC
> > to the imx7-mipi-csis driver.
> > 
> > The CSIS is an IP core from Samsung, integrated in different NXP SoCs.
> > The driver currently supports v3.3 of the CSIS, found in SoCs from the
> > i.MX6 and i.MX7 families. This series extends the driver to support
> > v3.6.3 of the IP, found in i.MX8MM and other members of the i.MX8
> > family.
> > 
> > The first 21 patches are miscellaneous cleanups and improvements. Please
> > see individual patches for details.
> > 
> > Patch 22/23 extends the imx7-mipi-csis DT bindings with i.MX8MM support.
> > Support for other members of the i.MX8 family will come later, and for
> > SoCs including an ISI IP core (such as the i.MX8MP) this will require
> > more work to handle additional glue logic.
> > 
> > Patch 23/23 finaly extends the imx7-mipi-csis driver accordingly.
> > 
> > The changes in the integration of the CSIS between i.MX7 and i.MX8, as
> > described in the DT bindings, have been found through reading of
> > reference manuals and BSP source code, with different sources of
> > information contradicting each other. A confirmation from NXP would be
> > nice (in particular regarding the clocks).
> 
> Thanks a lot for this series, looks all very good to me.
> 
> Will only ask you, as we already talked, to add your name in
> the bindings file as maintainer and extend also the MAINTAINERS file
> entry with your details.
> 
> You have a lot more hw/code working with this driver and also
> time/expertise.

Not sure about time, or even expertise, I'm trying my best :-) Thank you
for all your reviews. These drivers still need lots of love, and without
you patches wouldn't receive any tag.

> It can be in a follow patch no need to spin a new series for this.

Done, posted as v1.1 of 22/23, and a new 24/23. I'll send a pull request
after receiving an ack on 24/23.

> For all patches in this series:
> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>

Thank you.

> > Laurent Pinchart (23):
> >   media: imx: imx7_mipi_csis: Fix logging of only error event counters
> >   media: imx: imx7_mipi_csis: Count the CSI-2 debug interrupts
> >   media: imx: imx7_mipi_csis: Update ISP_CONFIG macros for quad pixel
> >     mode
> >   media: imx: imx7_mipi_csis: Move static data to top of
> >     mipi_csis_dump_regs()
> >   media: imx: imx7_mipi_csis: Minimize locking in get/set format
> >   media: imx: imx7_mipi_csis: Don't set subdev data
> >   media: imx: imx7-mipi-csis: Reorganize code in sections
> >   media: imx: imx7_mipi_csis: Set the CLKSETTLE register field
> >   media: imx: imx7_mipi_csis: Drop unused csis_hw_reset structure
> >   media: imx: imx7_mipi_csis: Store CSI-2 data type in format structure
> >   media: imx: imx7_mipi_csis: Drop csi_state phy field
> >   media: imx: imx7_mipi_csis: Rename mipi_sd to sd
> >   media: imx: imx7_mipi_csis: Rename csi_state flag field to state
> >   media: imx: imx7_mipi_csis: Turn csi_state irq field into local
> >     variable
> >   media: imx: imx7_mipi_csis: Don't pass pdev to mipi_csis_parse_dt()
> >   media: imx: imx7_mipi_csis: Pass csi_state to mipi_csis_subdev_init()
> >   media: imx: imx7_mipi_csis: Drop csi_state pdev field
> >   media: imx: imx7_mipi_csis: Make csi_state num_clocks field unsigned
> >   media: imx: imx7_mipi_csis: Reorganize csi_state structure
> >   media: imx: imx7_mipi_csis: Reorganize mipi_csis_probe()
> >   media: imx: imx7_mipi_csis: Reject invalid data-lanes settings
> >   dt-bindings: media: nxp,imx7-mipi-csi2: Add i.MX8MM support
> >   media: imx: imx7_mipi_csis: Add i.MX8MM support
> > 
> >  .../bindings/media/nxp,imx7-mipi-csi2.yaml    | 108 +-
> >  drivers/staging/media/imx/imx7-mipi-csis.c    | 943 ++++++++++--------
> >  2 files changed, 622 insertions(+), 429 deletions(-)

Tim Harvey April 21, 2021, 3:27 p.m. UTC | #3

On Mon, Apr 12, 2021 at 7:31 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hello,
>
> This patch series adds support for the CSIS found in the NXP i.MX8MM SoC
> to the imx7-mipi-csis driver.
>
> The CSIS is an IP core from Samsung, integrated in different NXP SoCs.
> The driver currently supports v3.3 of the CSIS, found in SoCs from the
> i.MX6 and i.MX7 families. This series extends the driver to support
> v3.6.3 of the IP, found in i.MX8MM and other members of the i.MX8
> family.
>
> The first 21 patches are miscellaneous cleanups and improvements. Please
> see individual patches for details.
>
> Patch 22/23 extends the imx7-mipi-csis DT bindings with i.MX8MM support.
> Support for other members of the i.MX8 family will come later, and for
> SoCs including an ISI IP core (such as the i.MX8MP) this will require
> more work to handle additional glue logic.
>
> Patch 23/23 finaly extends the imx7-mipi-csis driver accordingly.
>
> The changes in the integration of the CSIS between i.MX7 and i.MX8, as
> described in the DT bindings, have been found through reading of
> reference manuals and BSP source code, with different sources of
> information contradicting each other. A confirmation from NXP would be
> nice (in particular regarding the clocks).
>
> Laurent Pinchart (23):
>   media: imx: imx7_mipi_csis: Fix logging of only error event counters
>   media: imx: imx7_mipi_csis: Count the CSI-2 debug interrupts
>   media: imx: imx7_mipi_csis: Update ISP_CONFIG macros for quad pixel
>     mode
>   media: imx: imx7_mipi_csis: Move static data to top of
>     mipi_csis_dump_regs()
>   media: imx: imx7_mipi_csis: Minimize locking in get/set format
>   media: imx: imx7_mipi_csis: Don't set subdev data
>   media: imx: imx7-mipi-csis: Reorganize code in sections
>   media: imx: imx7_mipi_csis: Set the CLKSETTLE register field
>   media: imx: imx7_mipi_csis: Drop unused csis_hw_reset structure
>   media: imx: imx7_mipi_csis: Store CSI-2 data type in format structure
>   media: imx: imx7_mipi_csis: Drop csi_state phy field
>   media: imx: imx7_mipi_csis: Rename mipi_sd to sd
>   media: imx: imx7_mipi_csis: Rename csi_state flag field to state
>   media: imx: imx7_mipi_csis: Turn csi_state irq field into local
>     variable
>   media: imx: imx7_mipi_csis: Don't pass pdev to mipi_csis_parse_dt()
>   media: imx: imx7_mipi_csis: Pass csi_state to mipi_csis_subdev_init()
>   media: imx: imx7_mipi_csis: Drop csi_state pdev field
>   media: imx: imx7_mipi_csis: Make csi_state num_clocks field unsigned
>   media: imx: imx7_mipi_csis: Reorganize csi_state structure
>   media: imx: imx7_mipi_csis: Reorganize mipi_csis_probe()
>   media: imx: imx7_mipi_csis: Reject invalid data-lanes settings
>   dt-bindings: media: nxp,imx7-mipi-csi2: Add i.MX8MM support
>   media: imx: imx7_mipi_csis: Add i.MX8MM support
>
>  .../bindings/media/nxp,imx7-mipi-csi2.yaml    | 108 +-
>  drivers/staging/media/imx/imx7-mipi-csis.c    | 943 ++++++++++--------
>  2 files changed, 622 insertions(+), 429 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
>

Laurent,

Thank you for your work on this!

I have an IMX8MM board supporting CSI and a couple of devices to test with:
- Sony IMX477 12.3MP sensor (do not see any mainline support but there
are some hits on the net as this is a RPi camera)
- Sony IMX219 8MP sensor (should be supported by drivers/media/i2c/imx219.c)
- Auvidea B10x HDMI to CSI-2 bridge (Toshiba TC358743XBG HDMI to CSI-2
(MIPI)- 2D+C) (should be supported by drivers/media/i2c/tc358743.c)

Can you summarize the state of IMX8MM CSI capture in mainline? I
suppose the MIPI power domain is still an issue? Anything else that
would keep me from testing the above devices?

Best regards,

Tim

Frieder Schrempf April 26, 2021, 10:35 a.m. UTC | #4

Hi Tim,

On 21.04.21 17:27, Tim Harvey wrote:
> On Mon, Apr 12, 2021 at 7:31 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> Hello,
>>
>> This patch series adds support for the CSIS found in the NXP i.MX8MM SoC
>> to the imx7-mipi-csis driver.
>>
>> The CSIS is an IP core from Samsung, integrated in different NXP SoCs.
>> The driver currently supports v3.3 of the CSIS, found in SoCs from the
>> i.MX6 and i.MX7 families. This series extends the driver to support
>> v3.6.3 of the IP, found in i.MX8MM and other members of the i.MX8
>> family.
>>
>> The first 21 patches are miscellaneous cleanups and improvements. Please
>> see individual patches for details.
>>
>> Patch 22/23 extends the imx7-mipi-csis DT bindings with i.MX8MM support.
>> Support for other members of the i.MX8 family will come later, and for
>> SoCs including an ISI IP core (such as the i.MX8MP) this will require
>> more work to handle additional glue logic.
>>
>> Patch 23/23 finaly extends the imx7-mipi-csis driver accordingly.
>>
>> The changes in the integration of the CSIS between i.MX7 and i.MX8, as
>> described in the DT bindings, have been found through reading of
>> reference manuals and BSP source code, with different sources of
>> information contradicting each other. A confirmation from NXP would be
>> nice (in particular regarding the clocks).
>>
>> Laurent Pinchart (23):
>>    media: imx: imx7_mipi_csis: Fix logging of only error event counters
>>    media: imx: imx7_mipi_csis: Count the CSI-2 debug interrupts
>>    media: imx: imx7_mipi_csis: Update ISP_CONFIG macros for quad pixel
>>      mode
>>    media: imx: imx7_mipi_csis: Move static data to top of
>>      mipi_csis_dump_regs()
>>    media: imx: imx7_mipi_csis: Minimize locking in get/set format
>>    media: imx: imx7_mipi_csis: Don't set subdev data
>>    media: imx: imx7-mipi-csis: Reorganize code in sections
>>    media: imx: imx7_mipi_csis: Set the CLKSETTLE register field
>>    media: imx: imx7_mipi_csis: Drop unused csis_hw_reset structure
>>    media: imx: imx7_mipi_csis: Store CSI-2 data type in format structure
>>    media: imx: imx7_mipi_csis: Drop csi_state phy field
>>    media: imx: imx7_mipi_csis: Rename mipi_sd to sd
>>    media: imx: imx7_mipi_csis: Rename csi_state flag field to state
>>    media: imx: imx7_mipi_csis: Turn csi_state irq field into local
>>      variable
>>    media: imx: imx7_mipi_csis: Don't pass pdev to mipi_csis_parse_dt()
>>    media: imx: imx7_mipi_csis: Pass csi_state to mipi_csis_subdev_init()
>>    media: imx: imx7_mipi_csis: Drop csi_state pdev field
>>    media: imx: imx7_mipi_csis: Make csi_state num_clocks field unsigned
>>    media: imx: imx7_mipi_csis: Reorganize csi_state structure
>>    media: imx: imx7_mipi_csis: Reorganize mipi_csis_probe()
>>    media: imx: imx7_mipi_csis: Reject invalid data-lanes settings
>>    dt-bindings: media: nxp,imx7-mipi-csi2: Add i.MX8MM support
>>    media: imx: imx7_mipi_csis: Add i.MX8MM support
>>
>>   .../bindings/media/nxp,imx7-mipi-csi2.yaml    | 108 +-
>>   drivers/staging/media/imx/imx7-mipi-csis.c    | 943 ++++++++++--------
>>   2 files changed, 622 insertions(+), 429 deletions(-)
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>>
> 
> Laurent,
> 
> Thank you for your work on this!
> 
> I have an IMX8MM board supporting CSI and a couple of devices to test with:
> - Sony IMX477 12.3MP sensor (do not see any mainline support but there
> are some hits on the net as this is a RPi camera)
> - Sony IMX219 8MP sensor (should be supported by drivers/media/i2c/imx219.c)
> - Auvidea B10x HDMI to CSI-2 bridge (Toshiba TC358743XBG HDMI to CSI-2
> (MIPI)- 2D+C) (should be supported by drivers/media/i2c/tc358743.c)
> 
> Can you summarize the state of IMX8MM CSI capture in mainline? I
> suppose the MIPI power domain is still an issue? Anything else that
> would keep me from testing the above devices?
> 

Just in case it might help you: I tested the previous version of 
Laurent's patches (not on the mailing list) against mainline v5.10 with 
Lucas' power-domain patches.

It should work fine in general. Here are some notes about the problems I 
encountered: 
https://patchwork.kernel.org/project/linux-media/cover/20210215042741.28850-1-laurent.pinchart@ideasonboard.com/

Best regards
Frieder

Marco Felsch April 27, 2021, 11 a.m. UTC | #5

Hi,

On 21-04-26 12:35, Frieder Schrempf wrote:
> Hi Tim,
> 
> On 21.04.21 17:27, Tim Harvey wrote:
> > On Mon, Apr 12, 2021 at 7:31 PM Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > > 
> > > Hello,
> > > 
> > > This patch series adds support for the CSIS found in the NXP i.MX8MM SoC
> > > to the imx7-mipi-csis driver.
> > > 
> > > The CSIS is an IP core from Samsung, integrated in different NXP SoCs.
> > > The driver currently supports v3.3 of the CSIS, found in SoCs from the
> > > i.MX6 and i.MX7 families. This series extends the driver to support
> > > v3.6.3 of the IP, found in i.MX8MM and other members of the i.MX8
> > > family.
> > > 
> > > The first 21 patches are miscellaneous cleanups and improvements. Please
> > > see individual patches for details.
> > > 
> > > Patch 22/23 extends the imx7-mipi-csis DT bindings with i.MX8MM support.
> > > Support for other members of the i.MX8 family will come later, and for
> > > SoCs including an ISI IP core (such as the i.MX8MP) this will require
> > > more work to handle additional glue logic.
> > > 
> > > Patch 23/23 finaly extends the imx7-mipi-csis driver accordingly.
> > > 
> > > The changes in the integration of the CSIS between i.MX7 and i.MX8, as
> > > described in the DT bindings, have been found through reading of
> > > reference manuals and BSP source code, with different sources of
> > > information contradicting each other. A confirmation from NXP would be
> > > nice (in particular regarding the clocks).
> > > 
> > > Laurent Pinchart (23):
> > >    media: imx: imx7_mipi_csis: Fix logging of only error event counters
> > >    media: imx: imx7_mipi_csis: Count the CSI-2 debug interrupts
> > >    media: imx: imx7_mipi_csis: Update ISP_CONFIG macros for quad pixel
> > >      mode
> > >    media: imx: imx7_mipi_csis: Move static data to top of
> > >      mipi_csis_dump_regs()
> > >    media: imx: imx7_mipi_csis: Minimize locking in get/set format
> > >    media: imx: imx7_mipi_csis: Don't set subdev data
> > >    media: imx: imx7-mipi-csis: Reorganize code in sections
> > >    media: imx: imx7_mipi_csis: Set the CLKSETTLE register field
> > >    media: imx: imx7_mipi_csis: Drop unused csis_hw_reset structure
> > >    media: imx: imx7_mipi_csis: Store CSI-2 data type in format structure
> > >    media: imx: imx7_mipi_csis: Drop csi_state phy field
> > >    media: imx: imx7_mipi_csis: Rename mipi_sd to sd
> > >    media: imx: imx7_mipi_csis: Rename csi_state flag field to state
> > >    media: imx: imx7_mipi_csis: Turn csi_state irq field into local
> > >      variable
> > >    media: imx: imx7_mipi_csis: Don't pass pdev to mipi_csis_parse_dt()
> > >    media: imx: imx7_mipi_csis: Pass csi_state to mipi_csis_subdev_init()
> > >    media: imx: imx7_mipi_csis: Drop csi_state pdev field
> > >    media: imx: imx7_mipi_csis: Make csi_state num_clocks field unsigned
> > >    media: imx: imx7_mipi_csis: Reorganize csi_state structure
> > >    media: imx: imx7_mipi_csis: Reorganize mipi_csis_probe()
> > >    media: imx: imx7_mipi_csis: Reject invalid data-lanes settings
> > >    dt-bindings: media: nxp,imx7-mipi-csi2: Add i.MX8MM support
> > >    media: imx: imx7_mipi_csis: Add i.MX8MM support
> > > 
> > >   .../bindings/media/nxp,imx7-mipi-csi2.yaml    | 108 +-
> > >   drivers/staging/media/imx/imx7-mipi-csis.c    | 943 ++++++++++--------
> > >   2 files changed, 622 insertions(+), 429 deletions(-)
> > > 
> > > --
> > > Regards,
> > > 
> > > Laurent Pinchart
> > > 
> > 
> > Laurent,
> > 
> > Thank you for your work on this!
> > 
> > I have an IMX8MM board supporting CSI and a couple of devices to test with:
> > - Sony IMX477 12.3MP sensor (do not see any mainline support but there
> > are some hits on the net as this is a RPi camera)
> > - Sony IMX219 8MP sensor (should be supported by drivers/media/i2c/imx219.c)
> > - Auvidea B10x HDMI to CSI-2 bridge (Toshiba TC358743XBG HDMI to CSI-2
> > (MIPI)- 2D+C) (should be supported by drivers/media/i2c/tc358743.c)
> > 
> > Can you summarize the state of IMX8MM CSI capture in mainline? I
> > suppose the MIPI power domain is still an issue? Anything else that
> > would keep me from testing the above devices?
> > 
> 
> Just in case it might help you: I tested the previous version of Laurent's
> patches (not on the mailing list) against mainline v5.10 with Lucas'
> power-domain patches.
> 
> It should work fine in general. Here are some notes about the problems I
> encountered: https://patchwork.kernel.org/project/linux-media/cover/20210215042741.28850-1-laurent.pinchart@ideasonboard.com/

Is there also any ongoing work for the DPHY driver?

Regards,
  Marco

Martin Kepplinger May 4, 2021, 3:59 p.m. UTC | #6

hi Laurent, again thanks a lot for posting this series! I can't fully test
it, but base my work for imx8mq on it now. imx8mq includes
yet another mipi phy version than this and below is some very rough testing
code. it's not at all something I sign-off on but my following problem is based on it.

 * configured to use both staging csi drivers
 * the csi bridge driver at least streams frames together with the nxp "yav" mipi driver

media-ctl -p now says the output below, so one link from mipi to csi is missing.

Note that

media-ctl --set-v4l2 "'csi':0 [fmt:SBGGR10/640x480]"
works in that it changes the configured format below, but

media-ctl -l "'imx7-mipi-csis.0':1" -> "'csi':0[1]"
doesn't create said missing link.

Do I maybe use that wrongly? If now, does anything come to mind that would
be missing specifically?

When trying to stream anyway (if that makes sense), I get the following:

[ 2008.377470] capture_start_streaming: starting
[ 2008.381883] capture_find_format: calling imx_media_find_mbus_format with code 0x2006
[ 2008.389671] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt: capture_find_format err
[ 2008.397794] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt: capture_find_format found colorspace 0x1 != 0x0
[ 2008.407999] imx7-csi 30a90000.csi1_bridge: capture format not valid: -32

and if I ignore that (because I'm not yet sure whether that is specific to
platforms including an IPU), I get a WARN_ON from vb2_start_streaming()

again, it's great to see your updates!



Media device information
------------------------
driver          imx7-csi
model           imx-media
serial          
bus info        
hw revision     0x0
driver version  5.12.1

Device topology
- entity 1: csi (2 pads, 1 link)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev0
	pad0: Sink
		[fmt:UYVY8_2X8/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
	pad1: Source
		[fmt:UYVY8_2X8/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
		-> "csi capture":0 [ENABLED,IMMUTABLE]

- entity 4: csi capture (1 pad, 1 link)
            type Node subtype V4L flags 0
            device node name /dev/video1
	pad0: Sink
		<- "csi":1 [ENABLED,IMMUTABLE]

- entity 10: imx7-mipi-csis.0 (2 pads, 1 link)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev1
	pad0: Sink
		[fmt:UYVY8_1X16/640x480 field:none colorspace:smpte170m xfer:709 ycbcr:601 quantization:lim-range]
		<- "hi846 2-0020":0 []
	pad1: Source
		[fmt:UYVY8_1X16/640x480 field:none colorspace:smpte170m xfer:709 ycbcr:601 quantization:lim-range]

- entity 13: hi846 2-0020 (1 pad, 1 link)
             type V4L2 subdev subtype Sensor flags 0
             device node name /dev/v4l-subdev2
	pad0: Source
		[fmt:SGBRG10_1X10/640x480@1/120 field:none colorspace:unknown]
		-> "imx7-mipi-csis.0":0 []




---
 drivers/staging/media/imx/imx7-mipi-csis.c | 265 ++++++++++++++++++++-
 1 file changed, 252 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 0444b784c1ec..18e777d5a696 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -1,8 +1,11 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * Freescale i.MX7 SoC series MIPI-CSI V3.3 receiver driver
+ * Freescale i.MX SoC series MIPI-CSI V3.3 and V3.6 receiver driver
  *
+ * Copyright (C) 2021 Purism SPC
+ * Copyright (C) 2021 Laurent Pinchard
  * Copyright (C) 2019 Linaro Ltd
+ * Copyright (C) 2017 NXP
  * Copyright (C) 2015-2016 Freescale Semiconductor, Inc. All Rights Reserved.
  * Copyright (C) 2011 - 2013 Samsung Electronics Co., Ltd.
  *
@@ -21,6 +24,8 @@
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/spinlock.h>
@@ -237,12 +242,51 @@
 #define MIPI_CSI2_DATA_TYPE_RAW14		0x2d
 #define MIPI_CSI2_DATA_TYPE_USER(x)		(0x30 + (x))
 
+/* i.MX8MQ CSI-2 controller CSR */
+/* TODO 0x100, to dts? */
+#define CSI2RX_CFG_NUM_LANES			0x100
+#define CSI2RX_CFG_DISABLE_DATA_LANES		0x104
+#define CSI2RX_BIT_ERR				0x108
+#define CSI2RX_IRQ_STATUS			0x10C
+#define CSI2RX_IRQ_MASK				0x110
+#define CSI2RX_ULPS_STATUS			0x114
+#define CSI2RX_PPI_ERRSOT_HS			0x118
+#define CSI2RX_PPI_ERRSOTSYNC_HS		0x11C
+#define CSI2RX_PPI_ERRESC	 		0x120
+#define CSI2RX_PPI_ERRSYNCESC			0x124
+#define CSI2RX_PPI_ERRCONTROL			0x128
+#define CSI2RX_CFG_DISABLE_PAYLOAD_0		0x12C
+#define CSI2RX_CFG_DISABLE_PAYLOAD_1		0x130
+
+#if 0
+/* TODO leftover from yav. not used */
+struct mxc_mipi_csi2_dev {
+	struct v4l2_device		v4l2_dev;
+
+	struct v4l2_ctrl_handler ctrl_handler;
+
+	struct v4l2_async_subdev	asd;
+	struct v4l2_async_subdev	*async_subdevs[2];
+
+
+};
+#endif
+
 enum {
 	ST_POWERED	= 1,
 	ST_STREAMING	= 2,
 	ST_SUSPENDED	= 4,
 };
 
+#if 0
+imx8mq yav enum names:
+enum mxc_mipi_csi2_pm_state {
+	MXC_MIPI_CSI2_PM_POWERED	= 0x1,
+	MXC_MIPI_CSI2_PM_SUSPENDED	= 0x2,
+	MXC_MIPI_CSI2_RUNTIME_SUSPENDED	= 0x4,
+};
+#endif
+
 struct mipi_csis_event {
 	bool debug;
 	u32 mask;
@@ -297,15 +341,42 @@ static const char * const mipi_csis_clk_id[] = {
 enum mipi_csis_version {
 	MIPI_CSIS_V3_3,
 	MIPI_CSIS_V3_6_3,
+	MIPI_CSIS_V3_6_6, /* NXPs' "yet another version" */
 };
 
 struct mipi_csis_info {
 	enum mipi_csis_version version;
 };
 
+/* start imx8mq only */
+struct csis_imx8mq_hw_reset {
+	struct regmap *src;
+	u8 req_src;
+	u8 rst_val;
+};
+
+struct csis_imx8mq_phy_gpr {
+	struct regmap *gpr;
+	u8 req_src;
+};
+
+#define	GPR_CSI2_1_RX_ENABLE		BIT(13)
+#define	GPR_CSI2_1_VID_INTFC_ENB	BIT(12)
+#define	GPR_CSI2_1_HSEL			BIT(10)
+#define	GPR_CSI2_1_CONT_CLK_MODE 	BIT(8)
+#define	GPR_CSI2_1_S_PRG_RXHS_SETTLE(x)	(((x) & 0x3F) << 2)
+/*
+ * rxhs_settle[0] ... <720x480
+ * rxhs_settle[1] ... >720*480
+ *
+ * https://community.nxp.com/t5/i-MX-Processors/Explenation-for-HS-SETTLE-parameter-in-MIPI-CSI-D-PHY-registers/m-p/764275/highlight/true#M118744
+ */
+static u8 rxhs_settle[2] = { 0x14, 0x9 };
+/* end imx8mq only */
+
 struct csi_state {
 	struct device *dev;
-	void __iomem *regs;
+	void __iomem *regs; /* TODO yav name: base_regs */
 	unsigned int num_clks;
 	struct clk_bulk_data *clks;
 	struct reset_control *mrst;
@@ -315,23 +386,27 @@ struct csi_state {
 
 	struct v4l2_subdev sd;
 	struct media_pad pads[CSIS_PADS_NUM];
-	struct v4l2_async_notifier notifier;
-	struct v4l2_subdev *src_sd;
+	struct v4l2_async_notifier notifier; /* TODO yav name: subdev_notifier */
+	struct v4l2_subdev *src_sd; /* TODO yav name: sensor_sd */
 
-	struct v4l2_fwnode_bus_mipi_csi2 bus;
+	struct v4l2_fwnode_bus_mipi_csi2 bus; /* TODO yav name: int num_lanes */
 	u32 clk_frequency;
 	u32 hs_settle;
 	u32 clk_settle;
 
 	struct mutex lock;	/* Protect csis_fmt, format_mbus and state */
 	const struct csis_pix_format *csis_fmt;
-	struct v4l2_mbus_framefmt format_mbus;
+	struct v4l2_mbus_framefmt format_mbus; /* TODO imx8mq yav name: format */
 	u32 state;
 
 	spinlock_t slock;	/* Protect events */
 	struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS];
 	struct dentry *debugfs_root;
 	bool debug;
+
+	struct csis_imx8mq_hw_reset hw_reset;
+	struct csis_imx8mq_phy_gpr phy_gpr;
+	u32 send_level;
 };
 
 /* -----------------------------------------------------------------------------
@@ -457,13 +532,21 @@ static inline void mipi_csis_write(struct csi_state *state, u32 reg, u32 val)
 
 static void mipi_csis_enable_interrupts(struct csi_state *state, bool on)
 {
+	if (state->info->version == MIPI_CSIS_V3_6_6)
+		return;
+
 	mipi_csis_write(state, MIPI_CSIS_INT_MSK, on ? 0xffffffff : 0);
 	mipi_csis_write(state, MIPI_CSIS_DBG_INTR_MSK, on ? 0xffffffff : 0);
 }
 
 static void mipi_csis_sw_reset(struct csi_state *state)
 {
-	u32 val = mipi_csis_read(state, MIPI_CSIS_CMN_CTRL);
+	u32 val;
+
+	if (state->info->version == MIPI_CSIS_V3_6_6)
+		return;
+
+	val = mipi_csis_read(state, MIPI_CSIS_CMN_CTRL);
 
 	mipi_csis_write(state, MIPI_CSIS_CMN_CTRL,
 			val | MIPI_CSIS_CMN_CTRL_RESET);
@@ -485,17 +568,116 @@ static int mipi_csis_phy_init(struct csi_state *state)
 
 static void mipi_csis_phy_reset(struct csi_state *state)
 {
+	struct device *dev = state->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *node;
+	phandle phandle;
+	u32 out_val[3];
+	int ret;
+
 	if (state->info->version == MIPI_CSIS_V3_3) {
 		reset_control_assert(state->mrst);
 		msleep(20);
 		reset_control_deassert(state->mrst);
 	}
+
+	if (state->info->version != MIPI_CSIS_V3_6_6)
+		return;
+
+	ret = of_property_read_u32_array(np, "csis-phy-reset", out_val, 3);
+	if (ret) {
+		dev_info(dev, "no csis-hw-reset property found: %d\n", ret);
+		return;
+	}
+
+	phandle = *out_val;
+
+	node = of_find_node_by_phandle(phandle);
+	if (!node) {
+		ret = PTR_ERR(node);
+		dev_dbg(dev, "not find src node by phandle: %d\n", ret);
+	}
+	state->hw_reset.src = syscon_node_to_regmap(node);
+	if (IS_ERR(state->hw_reset.src)) {
+		ret = PTR_ERR(state->hw_reset.src);
+		dev_err(dev, "failed to get src regmap: %d\n", ret);
+	}
+	of_node_put(node);
+	if (ret < 0)
+		return;
+
+	state->hw_reset.req_src = out_val[1];
+	state->hw_reset.rst_val = out_val[2];
+
+	/* reset imx8mq mipi phy */
+	regmap_update_bits(state->hw_reset.src,
+			   state->hw_reset.req_src,
+			   state->hw_reset.rst_val,
+			   state->hw_reset.rst_val);
+	msleep(20);
+}
+
+static int mipi_csis_phy_gpr(struct csi_state *state)
+{
+	struct device *dev = state->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *node;
+	phandle phandle;
+	u32 out_val[2];
+	int ret;
+
+	ret = of_property_read_u32_array(np, "phy-gpr", out_val, 2);
+	if (ret) {
+		dev_dbg(dev, "no phy-gpr property found\n");
+	} else {
+		phandle = *out_val;
+
+		node = of_find_node_by_phandle(phandle);
+		if (!node) {
+			dev_dbg(dev, "not find gpr node by phandle\n");
+			ret = PTR_ERR(node);
+		}
+		state->phy_gpr.gpr = syscon_node_to_regmap(node);
+		if (IS_ERR(state->phy_gpr.gpr)) {
+			dev_err(dev, "failed to get gpr regmap\n");
+			ret = PTR_ERR(state->phy_gpr.gpr);
+		}
+		of_node_put(node);
+		if (ret < 0)
+			return ret;
+
+		state->phy_gpr.req_src = out_val[1];
+
+		regmap_update_bits(state->phy_gpr.gpr,
+				   state->phy_gpr.req_src,
+				   0x3FFF,
+				   GPR_CSI2_1_RX_ENABLE |
+				   GPR_CSI2_1_VID_INTFC_ENB |
+				   GPR_CSI2_1_HSEL |
+				   GPR_CSI2_1_CONT_CLK_MODE |
+				   GPR_CSI2_1_S_PRG_RXHS_SETTLE(state->
+								hs_settle));
+	}
+
+	dev_dbg(dev, "%s: hs_settle: 0x%X\n", __func__, state->hs_settle);
+
+	return ret;
 }
 
 static void mipi_csis_system_enable(struct csi_state *state, int on)
 {
 	u32 val, mask;
 
+	if (state->info->version == MIPI_CSIS_V3_6_6) {
+		if (on) {
+			mipi_csis_phy_gpr(state);
+		} else {
+			mipi_csis_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, 0xf);
+		}
+
+		return;
+	}
+
 	val = mipi_csis_read(state, MIPI_CSIS_CMN_CTRL);
 	if (on)
 		val |= MIPI_CSIS_CMN_CTRL_ENABLE;
@@ -534,6 +716,12 @@ static int mipi_csis_calculate_params(struct csi_state *state)
 	s64 link_freq;
 	u32 lane_rate;
 
+	if (state->info->version == MIPI_CSIS_V3_6_6) {
+		state->hs_settle = rxhs_settle[0];
+
+		return 0;
+	}
+
 	/* Calculate the line rate from the pixel rate. */
 	link_freq = v4l2_get_link_freq(state->src_sd->ctrl_handler,
 				       state->csis_fmt->width,
@@ -570,6 +758,31 @@ static void mipi_csis_set_params(struct csi_state *state)
 {
 	int lanes = state->bus.num_data_lanes;
 	u32 val;
+	int i;
+
+	if (state->info->version == MIPI_CSIS_V3_6_6) {
+		/* Lanes */
+		mipi_csis_write(state, CSI2RX_CFG_NUM_LANES, lanes - 1);
+		dev_err(state->dev, "imx8mq: %d lanes\n", lanes);
+
+		for (i = 0; i < lanes; i++)
+			val |= (1 << i);
+
+		val = 0xF & ~val;
+		mipi_csis_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, val);
+		dev_err(state->dev, "imx8mq: CSI2RX_CFG_DISABLE_DATA_LANES: 0x%X\n", val);
+
+		/* Mask interrupt */
+		// Don't let ULPS (ultra-low power status) interrupts flood
+		mipi_csis_write(state, CSI2RX_IRQ_MASK, 0x1ff);
+
+		mipi_csis_write(state, 0x180, 1);
+		/* vid_vc */
+		mipi_csis_write(state, 0x184, 1);
+		mipi_csis_write(state, 0x188, state->send_level);
+
+		return;
+	}
 
 	val = mipi_csis_read(state, MIPI_CSIS_CMN_CTRL);
 	val &= ~MIPI_CSIS_CMN_CTRL_LANE_NR_MASK;
@@ -799,6 +1012,9 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
 	struct csi_state *state = mipi_sd_to_csis_state(sd);
 	int ret;
 
+	if (state->info->version == MIPI_CSIS_V3_6_6)
+		mipi_csis_write(state, CSI2RX_IRQ_MASK, 0x008);
+
 	if (enable) {
 		ret = mipi_csis_calculate_params(state);
 		if (ret < 0)
@@ -814,6 +1030,9 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
 		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
 		if (ret < 0 && ret != -ENOIOCTLCMD)
 			goto done;
+
+		if (state->info->version == MIPI_CSIS_V3_6_6)
+			mipi_csis_phy_reset(state);
 	}
 
 	mutex_lock(&state->lock);
@@ -1014,6 +1233,19 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
 
 	sdformat->format = *fmt;
 
+	if (state->info->version == MIPI_CSIS_V3_6_6) {
+		if (sdformat->format.width * sdformat->format.height > 720 * 480) {
+			state->hs_settle = rxhs_settle[1];
+		} else {
+			state->hs_settle = rxhs_settle[0];
+		}
+		state->send_level = 64;
+
+		dev_dbg(state->dev,
+			"%s: set send_level %d hs_settle 0x%X\n", __func__,
+			state->send_level, state->hs_settle);
+	}
+
 	/* Propagate the format from sink to source. */
 	fmt = mipi_csis_get_format(state, cfg, sdformat->which,
 				   CSIS_PAD_SOURCE);
@@ -1371,12 +1603,14 @@ static int mipi_csis_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	/* Now that the hardware is initialized, request the interrupt. */
-	ret = devm_request_irq(dev, irq, mipi_csis_irq_handler, 0,
-			       dev_name(dev), state);
-	if (ret) {
-		dev_err(dev, "Interrupt request failed\n");
-		goto disable_clock;
+	if (state->info->version != MIPI_CSIS_V3_6_6) {
+		/* Now that the hardware is initialized, request the interrupt. */
+		ret = devm_request_irq(dev, irq, mipi_csis_irq_handler, 0,
+				       dev_name(dev), state);
+		if (ret) {
+			dev_err(dev, "Interrupt request failed\n");
+			goto disable_clock;
+		}
 	}
 
 	/* Initialize and register the subdev. */
@@ -1453,6 +1687,11 @@ static const struct of_device_id mipi_csis_of_match[] = {
 		.data = &(const struct mipi_csis_info){
 			.version = MIPI_CSIS_V3_6_3,
 		},
+	}, {
+		.compatible = "fsl,imx8mq-mipi-csi2",
+		.data = &(const struct mipi_csis_info){
+			.version = MIPI_CSIS_V3_6_6,
+		},
 	},
 	{ /* sentinel */ },
 };

Laurent Pinchart May 15, 2021, 10:46 p.m. UTC | #7

Hello,

Sorry for the late reply.

On Tue, Apr 27, 2021 at 01:00:45PM +0200, Marco Felsch wrote:
> On 21-04-26 12:35, Frieder Schrempf wrote:
> > On 21.04.21 17:27, Tim Harvey wrote:
> > > On Mon, Apr 12, 2021 at 7:31 PM Laurent Pinchart wrote:
> > > > 
> > > > Hello,
> > > > 
> > > > This patch series adds support for the CSIS found in the NXP i.MX8MM SoC
> > > > to the imx7-mipi-csis driver.
> > > > 
> > > > The CSIS is an IP core from Samsung, integrated in different NXP SoCs.
> > > > The driver currently supports v3.3 of the CSIS, found in SoCs from the
> > > > i.MX6 and i.MX7 families. This series extends the driver to support
> > > > v3.6.3 of the IP, found in i.MX8MM and other members of the i.MX8
> > > > family.
> > > > 
> > > > The first 21 patches are miscellaneous cleanups and improvements. Please
> > > > see individual patches for details.
> > > > 
> > > > Patch 22/23 extends the imx7-mipi-csis DT bindings with i.MX8MM support.
> > > > Support for other members of the i.MX8 family will come later, and for
> > > > SoCs including an ISI IP core (such as the i.MX8MP) this will require
> > > > more work to handle additional glue logic.
> > > > 
> > > > Patch 23/23 finaly extends the imx7-mipi-csis driver accordingly.
> > > > 
> > > > The changes in the integration of the CSIS between i.MX7 and i.MX8, as
> > > > described in the DT bindings, have been found through reading of
> > > > reference manuals and BSP source code, with different sources of
> > > > information contradicting each other. A confirmation from NXP would be
> > > > nice (in particular regarding the clocks).
> > > > 
> > > > Laurent Pinchart (23):
> > > >    media: imx: imx7_mipi_csis: Fix logging of only error event counters
> > > >    media: imx: imx7_mipi_csis: Count the CSI-2 debug interrupts
> > > >    media: imx: imx7_mipi_csis: Update ISP_CONFIG macros for quad pixel
> > > >      mode
> > > >    media: imx: imx7_mipi_csis: Move static data to top of
> > > >      mipi_csis_dump_regs()
> > > >    media: imx: imx7_mipi_csis: Minimize locking in get/set format
> > > >    media: imx: imx7_mipi_csis: Don't set subdev data
> > > >    media: imx: imx7-mipi-csis: Reorganize code in sections
> > > >    media: imx: imx7_mipi_csis: Set the CLKSETTLE register field
> > > >    media: imx: imx7_mipi_csis: Drop unused csis_hw_reset structure
> > > >    media: imx: imx7_mipi_csis: Store CSI-2 data type in format structure
> > > >    media: imx: imx7_mipi_csis: Drop csi_state phy field
> > > >    media: imx: imx7_mipi_csis: Rename mipi_sd to sd
> > > >    media: imx: imx7_mipi_csis: Rename csi_state flag field to state
> > > >    media: imx: imx7_mipi_csis: Turn csi_state irq field into local
> > > >      variable
> > > >    media: imx: imx7_mipi_csis: Don't pass pdev to mipi_csis_parse_dt()
> > > >    media: imx: imx7_mipi_csis: Pass csi_state to mipi_csis_subdev_init()
> > > >    media: imx: imx7_mipi_csis: Drop csi_state pdev field
> > > >    media: imx: imx7_mipi_csis: Make csi_state num_clocks field unsigned
> > > >    media: imx: imx7_mipi_csis: Reorganize csi_state structure
> > > >    media: imx: imx7_mipi_csis: Reorganize mipi_csis_probe()
> > > >    media: imx: imx7_mipi_csis: Reject invalid data-lanes settings
> > > >    dt-bindings: media: nxp,imx7-mipi-csi2: Add i.MX8MM support
> > > >    media: imx: imx7_mipi_csis: Add i.MX8MM support
> > > > 
> > > >   .../bindings/media/nxp,imx7-mipi-csi2.yaml    | 108 +-
> > > >   drivers/staging/media/imx/imx7-mipi-csis.c    | 943 ++++++++++--------
> > > >   2 files changed, 622 insertions(+), 429 deletions(-)
> > > 
> > > Laurent,
> > > 
> > > Thank you for your work on this!
> > > 
> > > I have an IMX8MM board supporting CSI and a couple of devices to test with:
> > > - Sony IMX477 12.3MP sensor (do not see any mainline support but there
> > > are some hits on the net as this is a RPi camera)
> > > - Sony IMX219 8MP sensor (should be supported by drivers/media/i2c/imx219.c)
> > > - Auvidea B10x HDMI to CSI-2 bridge (Toshiba TC358743XBG HDMI to CSI-2
> > > (MIPI)- 2D+C) (should be supported by drivers/media/i2c/tc358743.c)
> > > 
> > > Can you summarize the state of IMX8MM CSI capture in mainline? I
> > > suppose the MIPI power domain is still an issue? Anything else that
> > > would keep me from testing the above devices?
> > 
> > Just in case it might help you: I tested the previous version of Laurent's
> > patches (not on the mailing list) against mainline v5.10 with Lucas'
> > power-domain patches.
> > 
> > It should work fine in general. Here are some notes about the problems I
> > encountered: https://patchwork.kernel.org/project/linux-media/cover/20210215042741.28850-1-laurent.pinchart@ideasonboard.com/
> 
> Is there also any ongoing work for the DPHY driver?

I'm still working on the CSI bridge side. Aside from power domains,
there's no big blocker. I managed to get it working fine with a few
different sensors. It still fails with an MT9M114 for a reason I don't
understand at this point, but I don't think that's an upstreaming
blocker.

I'll try to post patches in the not too distant future.

Laurent Pinchart May 15, 2021, 10:55 p.m. UTC | #8

Hi Martin,

Sorry for the late reply.

On Tue, May 04, 2021 at 05:59:39PM +0200, Martin Kepplinger wrote:
> hi Laurent, again thanks a lot for posting this series! I can't fully test
> it, but base my work for imx8mq on it now. imx8mq includes
> yet another mipi phy version than this and below is some very rough testing
> code. it's not at all something I sign-off on but my following problem is based on it.

Unless I'm mistaken, the CSI-2 receiver in the i.MX8MQ is a completely
different device. I wouldn't try to support it in the imx7-mipi-csis
driver, but in a separate driver.

>  * configured to use both staging csi drivers
>  * the csi bridge driver at least streams frames together with the nxp "yav" mipi driver
> 
> media-ctl -p now says the output below, so one link from mipi to csi is missing.
> 
> Note that
> 
> media-ctl --set-v4l2 "'csi':0 [fmt:SBGGR10/640x480]"
> works in that it changes the configured format below, but
> 
> media-ctl -l "'imx7-mipi-csis.0':1" -> "'csi':0[1]"
> doesn't create said missing link.

media-ctl can't create links, it can only enable or disable them. Link
creation is the prerogative of drivers.

> Do I maybe use that wrongly? If now, does anything come to mind that would
> be missing specifically?

The link should be created by the call to media_create_pad_link() in
imx_media_capture_device_register(). You'll need to figure out if the
function is called and returns an error early, or if it doesn't get
called at all, and why.

> When trying to stream anyway (if that makes sense), I get the following:
> 
> [ 2008.377470] capture_start_streaming: starting
> [ 2008.381883] capture_find_format: calling imx_media_find_mbus_format with code 0x2006
> [ 2008.389671] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt: capture_find_format err
> [ 2008.397794] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt: capture_find_format found colorspace 0x1 != 0x0
> [ 2008.407999] imx7-csi 30a90000.csi1_bridge: capture format not valid: -32
> 
> and if I ignore that (because I'm not yet sure whether that is specific to
> platforms including an IPU), I get a WARN_ON from vb2_start_streaming()

That I have a fix for, I'll post it as part of an imx7-media-csi series.

> again, it's great to see your updates!
> 
> 
> 
> Media device information
> ------------------------
> driver          imx7-csi
> model           imx-media
> serial          
> bus info        
> hw revision     0x0
> driver version  5.12.1
> 
> Device topology
> - entity 1: csi (2 pads, 1 link)
>             type V4L2 subdev subtype Unknown flags 0
>             device node name /dev/v4l-subdev0
> 	pad0: Sink
> 		[fmt:UYVY8_2X8/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
> 	pad1: Source
> 		[fmt:UYVY8_2X8/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
> 		-> "csi capture":0 [ENABLED,IMMUTABLE]
> 
> - entity 4: csi capture (1 pad, 1 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video1
> 	pad0: Sink
> 		<- "csi":1 [ENABLED,IMMUTABLE]
> 
> - entity 10: imx7-mipi-csis.0 (2 pads, 1 link)
>              type V4L2 subdev subtype Unknown flags 0
>              device node name /dev/v4l-subdev1
> 	pad0: Sink
> 		[fmt:UYVY8_1X16/640x480 field:none colorspace:smpte170m xfer:709 ycbcr:601 quantization:lim-range]
> 		<- "hi846 2-0020":0 []
> 	pad1: Source
> 		[fmt:UYVY8_1X16/640x480 field:none colorspace:smpte170m xfer:709 ycbcr:601 quantization:lim-range]
> 
> - entity 13: hi846 2-0020 (1 pad, 1 link)
>              type V4L2 subdev subtype Sensor flags 0
>              device node name /dev/v4l-subdev2
> 	pad0: Source
> 		[fmt:SGBRG10_1X10/640x480@1/120 field:none colorspace:unknown]
> 		-> "imx7-mipi-csis.0":0 []
> 
> 
> 
> 
> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 265 ++++++++++++++++++++-
>  1 file changed, 252 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index 0444b784c1ec..18e777d5a696 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -1,8 +1,11 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * Freescale i.MX7 SoC series MIPI-CSI V3.3 receiver driver
> + * Freescale i.MX SoC series MIPI-CSI V3.3 and V3.6 receiver driver
>   *
> + * Copyright (C) 2021 Purism SPC
> + * Copyright (C) 2021 Laurent Pinchard
>   * Copyright (C) 2019 Linaro Ltd
> + * Copyright (C) 2017 NXP
>   * Copyright (C) 2015-2016 Freescale Semiconductor, Inc. All Rights Reserved.
>   * Copyright (C) 2011 - 2013 Samsung Electronics Co., Ltd.
>   *
> @@ -21,6 +24,8 @@
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
>  #include <linux/spinlock.h>
> @@ -237,12 +242,51 @@
>  #define MIPI_CSI2_DATA_TYPE_RAW14		0x2d
>  #define MIPI_CSI2_DATA_TYPE_USER(x)		(0x30 + (x))
>  
> +/* i.MX8MQ CSI-2 controller CSR */
> +/* TODO 0x100, to dts? */
> +#define CSI2RX_CFG_NUM_LANES			0x100
> +#define CSI2RX_CFG_DISABLE_DATA_LANES		0x104
> +#define CSI2RX_BIT_ERR				0x108
> +#define CSI2RX_IRQ_STATUS			0x10C
> +#define CSI2RX_IRQ_MASK				0x110
> +#define CSI2RX_ULPS_STATUS			0x114
> +#define CSI2RX_PPI_ERRSOT_HS			0x118
> +#define CSI2RX_PPI_ERRSOTSYNC_HS		0x11C
> +#define CSI2RX_PPI_ERRESC	 		0x120
> +#define CSI2RX_PPI_ERRSYNCESC			0x124
> +#define CSI2RX_PPI_ERRCONTROL			0x128
> +#define CSI2RX_CFG_DISABLE_PAYLOAD_0		0x12C
> +#define CSI2RX_CFG_DISABLE_PAYLOAD_1		0x130
> +
> +#if 0
> +/* TODO leftover from yav. not used */
> +struct mxc_mipi_csi2_dev {
> +	struct v4l2_device		v4l2_dev;
> +
> +	struct v4l2_ctrl_handler ctrl_handler;
> +
> +	struct v4l2_async_subdev	asd;
> +	struct v4l2_async_subdev	*async_subdevs[2];
> +
> +
> +};
> +#endif
> +
>  enum {
>  	ST_POWERED	= 1,
>  	ST_STREAMING	= 2,
>  	ST_SUSPENDED	= 4,
>  };
>  
> +#if 0
> +imx8mq yav enum names:
> +enum mxc_mipi_csi2_pm_state {
> +	MXC_MIPI_CSI2_PM_POWERED	= 0x1,
> +	MXC_MIPI_CSI2_PM_SUSPENDED	= 0x2,
> +	MXC_MIPI_CSI2_RUNTIME_SUSPENDED	= 0x4,
> +};
> +#endif
> +
>  struct mipi_csis_event {
>  	bool debug;
>  	u32 mask;
> @@ -297,15 +341,42 @@ static const char * const mipi_csis_clk_id[] = {
>  enum mipi_csis_version {
>  	MIPI_CSIS_V3_3,
>  	MIPI_CSIS_V3_6_3,
> +	MIPI_CSIS_V3_6_6, /* NXPs' "yet another version" */
>  };
>  
>  struct mipi_csis_info {
>  	enum mipi_csis_version version;
>  };
>  
> +/* start imx8mq only */
> +struct csis_imx8mq_hw_reset {
> +	struct regmap *src;
> +	u8 req_src;
> +	u8 rst_val;
> +};
> +
> +struct csis_imx8mq_phy_gpr {
> +	struct regmap *gpr;
> +	u8 req_src;
> +};
> +
> +#define	GPR_CSI2_1_RX_ENABLE		BIT(13)
> +#define	GPR_CSI2_1_VID_INTFC_ENB	BIT(12)
> +#define	GPR_CSI2_1_HSEL			BIT(10)
> +#define	GPR_CSI2_1_CONT_CLK_MODE 	BIT(8)
> +#define	GPR_CSI2_1_S_PRG_RXHS_SETTLE(x)	(((x) & 0x3F) << 2)
> +/*
> + * rxhs_settle[0] ... <720x480
> + * rxhs_settle[1] ... >720*480
> + *
> + * https://community.nxp.com/t5/i-MX-Processors/Explenation-for-HS-SETTLE-parameter-in-MIPI-CSI-D-PHY-registers/m-p/764275/highlight/true#M118744
> + */
> +static u8 rxhs_settle[2] = { 0x14, 0x9 };
> +/* end imx8mq only */
> +
>  struct csi_state {
>  	struct device *dev;
> -	void __iomem *regs;
> +	void __iomem *regs; /* TODO yav name: base_regs */
>  	unsigned int num_clks;
>  	struct clk_bulk_data *clks;
>  	struct reset_control *mrst;
> @@ -315,23 +386,27 @@ struct csi_state {
>  
>  	struct v4l2_subdev sd;
>  	struct media_pad pads[CSIS_PADS_NUM];
> -	struct v4l2_async_notifier notifier;
> -	struct v4l2_subdev *src_sd;
> +	struct v4l2_async_notifier notifier; /* TODO yav name: subdev_notifier */
> +	struct v4l2_subdev *src_sd; /* TODO yav name: sensor_sd */
>  
> -	struct v4l2_fwnode_bus_mipi_csi2 bus;
> +	struct v4l2_fwnode_bus_mipi_csi2 bus; /* TODO yav name: int num_lanes */
>  	u32 clk_frequency;
>  	u32 hs_settle;
>  	u32 clk_settle;
>  
>  	struct mutex lock;	/* Protect csis_fmt, format_mbus and state */
>  	const struct csis_pix_format *csis_fmt;
> -	struct v4l2_mbus_framefmt format_mbus;
> +	struct v4l2_mbus_framefmt format_mbus; /* TODO imx8mq yav name: format */
>  	u32 state;
>  
>  	spinlock_t slock;	/* Protect events */
>  	struct mipi_csis_event events[MIPI_CSIS_NUM_EVENTS];
>  	struct dentry *debugfs_root;
>  	bool debug;
> +
> +	struct csis_imx8mq_hw_reset hw_reset;
> +	struct csis_imx8mq_phy_gpr phy_gpr;
> +	u32 send_level;
>  };
>  
>  /* -----------------------------------------------------------------------------
> @@ -457,13 +532,21 @@ static inline void mipi_csis_write(struct csi_state *state, u32 reg, u32 val)
>  
>  static void mipi_csis_enable_interrupts(struct csi_state *state, bool on)
>  {
> +	if (state->info->version == MIPI_CSIS_V3_6_6)
> +		return;
> +
>  	mipi_csis_write(state, MIPI_CSIS_INT_MSK, on ? 0xffffffff : 0);
>  	mipi_csis_write(state, MIPI_CSIS_DBG_INTR_MSK, on ? 0xffffffff : 0);
>  }
>  
>  static void mipi_csis_sw_reset(struct csi_state *state)
>  {
> -	u32 val = mipi_csis_read(state, MIPI_CSIS_CMN_CTRL);
> +	u32 val;
> +
> +	if (state->info->version == MIPI_CSIS_V3_6_6)
> +		return;
> +
> +	val = mipi_csis_read(state, MIPI_CSIS_CMN_CTRL);
>  
>  	mipi_csis_write(state, MIPI_CSIS_CMN_CTRL,
>  			val | MIPI_CSIS_CMN_CTRL_RESET);
> @@ -485,17 +568,116 @@ static int mipi_csis_phy_init(struct csi_state *state)
>  
>  static void mipi_csis_phy_reset(struct csi_state *state)
>  {
> +	struct device *dev = state->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *node;
> +	phandle phandle;
> +	u32 out_val[3];
> +	int ret;
> +
>  	if (state->info->version == MIPI_CSIS_V3_3) {
>  		reset_control_assert(state->mrst);
>  		msleep(20);
>  		reset_control_deassert(state->mrst);
>  	}
> +
> +	if (state->info->version != MIPI_CSIS_V3_6_6)
> +		return;
> +
> +	ret = of_property_read_u32_array(np, "csis-phy-reset", out_val, 3);
> +	if (ret) {
> +		dev_info(dev, "no csis-hw-reset property found: %d\n", ret);
> +		return;
> +	}
> +
> +	phandle = *out_val;
> +
> +	node = of_find_node_by_phandle(phandle);
> +	if (!node) {
> +		ret = PTR_ERR(node);
> +		dev_dbg(dev, "not find src node by phandle: %d\n", ret);
> +	}
> +	state->hw_reset.src = syscon_node_to_regmap(node);
> +	if (IS_ERR(state->hw_reset.src)) {
> +		ret = PTR_ERR(state->hw_reset.src);
> +		dev_err(dev, "failed to get src regmap: %d\n", ret);
> +	}
> +	of_node_put(node);
> +	if (ret < 0)
> +		return;
> +
> +	state->hw_reset.req_src = out_val[1];
> +	state->hw_reset.rst_val = out_val[2];
> +
> +	/* reset imx8mq mipi phy */
> +	regmap_update_bits(state->hw_reset.src,
> +			   state->hw_reset.req_src,
> +			   state->hw_reset.rst_val,
> +			   state->hw_reset.rst_val);
> +	msleep(20);
> +}
> +
> +static int mipi_csis_phy_gpr(struct csi_state *state)
> +{
> +	struct device *dev = state->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *node;
> +	phandle phandle;
> +	u32 out_val[2];
> +	int ret;
> +
> +	ret = of_property_read_u32_array(np, "phy-gpr", out_val, 2);
> +	if (ret) {
> +		dev_dbg(dev, "no phy-gpr property found\n");
> +	} else {
> +		phandle = *out_val;
> +
> +		node = of_find_node_by_phandle(phandle);
> +		if (!node) {
> +			dev_dbg(dev, "not find gpr node by phandle\n");
> +			ret = PTR_ERR(node);
> +		}
> +		state->phy_gpr.gpr = syscon_node_to_regmap(node);
> +		if (IS_ERR(state->phy_gpr.gpr)) {
> +			dev_err(dev, "failed to get gpr regmap\n");
> +			ret = PTR_ERR(state->phy_gpr.gpr);
> +		}
> +		of_node_put(node);
> +		if (ret < 0)
> +			return ret;
> +
> +		state->phy_gpr.req_src = out_val[1];
> +
> +		regmap_update_bits(state->phy_gpr.gpr,
> +				   state->phy_gpr.req_src,
> +				   0x3FFF,
> +				   GPR_CSI2_1_RX_ENABLE |
> +				   GPR_CSI2_1_VID_INTFC_ENB |
> +				   GPR_CSI2_1_HSEL |
> +				   GPR_CSI2_1_CONT_CLK_MODE |
> +				   GPR_CSI2_1_S_PRG_RXHS_SETTLE(state->
> +								hs_settle));
> +	}
> +
> +	dev_dbg(dev, "%s: hs_settle: 0x%X\n", __func__, state->hs_settle);
> +
> +	return ret;
>  }
>  
>  static void mipi_csis_system_enable(struct csi_state *state, int on)
>  {
>  	u32 val, mask;
>  
> +	if (state->info->version == MIPI_CSIS_V3_6_6) {
> +		if (on) {
> +			mipi_csis_phy_gpr(state);
> +		} else {
> +			mipi_csis_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, 0xf);
> +		}
> +
> +		return;
> +	}
> +
>  	val = mipi_csis_read(state, MIPI_CSIS_CMN_CTRL);
>  	if (on)
>  		val |= MIPI_CSIS_CMN_CTRL_ENABLE;
> @@ -534,6 +716,12 @@ static int mipi_csis_calculate_params(struct csi_state *state)
>  	s64 link_freq;
>  	u32 lane_rate;
>  
> +	if (state->info->version == MIPI_CSIS_V3_6_6) {
> +		state->hs_settle = rxhs_settle[0];
> +
> +		return 0;
> +	}
> +
>  	/* Calculate the line rate from the pixel rate. */
>  	link_freq = v4l2_get_link_freq(state->src_sd->ctrl_handler,
>  				       state->csis_fmt->width,
> @@ -570,6 +758,31 @@ static void mipi_csis_set_params(struct csi_state *state)
>  {
>  	int lanes = state->bus.num_data_lanes;
>  	u32 val;
> +	int i;
> +
> +	if (state->info->version == MIPI_CSIS_V3_6_6) {
> +		/* Lanes */
> +		mipi_csis_write(state, CSI2RX_CFG_NUM_LANES, lanes - 1);
> +		dev_err(state->dev, "imx8mq: %d lanes\n", lanes);
> +
> +		for (i = 0; i < lanes; i++)
> +			val |= (1 << i);
> +
> +		val = 0xF & ~val;
> +		mipi_csis_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, val);
> +		dev_err(state->dev, "imx8mq: CSI2RX_CFG_DISABLE_DATA_LANES: 0x%X\n", val);
> +
> +		/* Mask interrupt */
> +		// Don't let ULPS (ultra-low power status) interrupts flood
> +		mipi_csis_write(state, CSI2RX_IRQ_MASK, 0x1ff);
> +
> +		mipi_csis_write(state, 0x180, 1);
> +		/* vid_vc */
> +		mipi_csis_write(state, 0x184, 1);
> +		mipi_csis_write(state, 0x188, state->send_level);
> +
> +		return;
> +	}
>  
>  	val = mipi_csis_read(state, MIPI_CSIS_CMN_CTRL);
>  	val &= ~MIPI_CSIS_CMN_CTRL_LANE_NR_MASK;
> @@ -799,6 +1012,9 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>  	struct csi_state *state = mipi_sd_to_csis_state(sd);
>  	int ret;
>  
> +	if (state->info->version == MIPI_CSIS_V3_6_6)
> +		mipi_csis_write(state, CSI2RX_IRQ_MASK, 0x008);
> +
>  	if (enable) {
>  		ret = mipi_csis_calculate_params(state);
>  		if (ret < 0)
> @@ -814,6 +1030,9 @@ static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
>  		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
>  		if (ret < 0 && ret != -ENOIOCTLCMD)
>  			goto done;
> +
> +		if (state->info->version == MIPI_CSIS_V3_6_6)
> +			mipi_csis_phy_reset(state);
>  	}
>  
>  	mutex_lock(&state->lock);
> @@ -1014,6 +1233,19 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
>  
>  	sdformat->format = *fmt;
>  
> +	if (state->info->version == MIPI_CSIS_V3_6_6) {
> +		if (sdformat->format.width * sdformat->format.height > 720 * 480) {
> +			state->hs_settle = rxhs_settle[1];
> +		} else {
> +			state->hs_settle = rxhs_settle[0];
> +		}
> +		state->send_level = 64;
> +
> +		dev_dbg(state->dev,
> +			"%s: set send_level %d hs_settle 0x%X\n", __func__,
> +			state->send_level, state->hs_settle);
> +	}
> +
>  	/* Propagate the format from sink to source. */
>  	fmt = mipi_csis_get_format(state, cfg, sdformat->which,
>  				   CSIS_PAD_SOURCE);
> @@ -1371,12 +1603,14 @@ static int mipi_csis_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	/* Now that the hardware is initialized, request the interrupt. */
> -	ret = devm_request_irq(dev, irq, mipi_csis_irq_handler, 0,
> -			       dev_name(dev), state);
> -	if (ret) {
> -		dev_err(dev, "Interrupt request failed\n");
> -		goto disable_clock;
> +	if (state->info->version != MIPI_CSIS_V3_6_6) {
> +		/* Now that the hardware is initialized, request the interrupt. */
> +		ret = devm_request_irq(dev, irq, mipi_csis_irq_handler, 0,
> +				       dev_name(dev), state);
> +		if (ret) {
> +			dev_err(dev, "Interrupt request failed\n");
> +			goto disable_clock;
> +		}
>  	}
>  
>  	/* Initialize and register the subdev. */
> @@ -1453,6 +1687,11 @@ static const struct of_device_id mipi_csis_of_match[] = {
>  		.data = &(const struct mipi_csis_info){
>  			.version = MIPI_CSIS_V3_6_3,
>  		},
> +	}, {
> +		.compatible = "fsl,imx8mq-mipi-csi2",
> +		.data = &(const struct mipi_csis_info){
> +			.version = MIPI_CSIS_V3_6_6,
> +		},
>  	},
>  	{ /* sentinel */ },
>  };

Martin Kepplinger May 18, 2021, 2:39 p.m. UTC | #9

Am Sonntag, dem 16.05.2021 um 01:55 +0300 schrieb Laurent Pinchart:
> Hi Martin,
> 
> Sorry for the late reply.
> 
> On Tue, May 04, 2021 at 05:59:39PM +0200, Martin Kepplinger wrote:
> > hi Laurent, again thanks a lot for posting this series! I can't
> > fully test
> > it, but base my work for imx8mq on it now. imx8mq includes
> > yet another mipi phy version than this and below is some very rough
> > testing
> > code. it's not at all something I sign-off on but my following
> > problem is based on it.
> 
> Unless I'm mistaken, the CSI-2 receiver in the i.MX8MQ is a
> completely
> different device. I wouldn't try to support it in the imx7-mipi-csis
> driver, but in a separate driver.
> 
> >  * configured to use both staging csi drivers
> >  * the csi bridge driver at least streams frames together with the
> > nxp "yav" mipi driver
> > 
> > media-ctl -p now says the output below, so one link from mipi to
> > csi is missing.
> > 
> > Note that
> > 
> > media-ctl --set-v4l2 "'csi':0 [fmt:SBGGR10/640x480]"
> > works in that it changes the configured format below, but
> > 
> > media-ctl -l "'imx7-mipi-csis.0':1" -> "'csi':0[1]"
> > doesn't create said missing link.
> 
> media-ctl can't create links, it can only enable or disable them.
> Link
> creation is the prerogative of drivers.
> 
> > Do I maybe use that wrongly? If now, does anything come to mind
> > that would
> > be missing specifically?
> 
> The link should be created by the call to media_create_pad_link() in
> imx_media_capture_device_register(). You'll need to figure out if the
> function is called and returns an error early, or if it doesn't get
> called at all, and why.
> 
> > When trying to stream anyway (if that makes sense), I get the
> > following:
> > 
> > [ 2008.377470] capture_start_streaming: starting
> > [ 2008.381883] capture_find_format: calling
> > imx_media_find_mbus_format with code 0x2006
> > [ 2008.389671] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt:
> > capture_find_format err
> > [ 2008.397794] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt:
> > capture_find_format found colorspace 0x1 != 0x0
> > [ 2008.407999] imx7-csi 30a90000.csi1_bridge: capture format not
> > valid: -32
> > 
> > and if I ignore that (because I'm not yet sure whether that is
> > specific to
> > platforms including an IPU), I get a WARN_ON from
> > vb2_start_streaming()
> 
> That I have a fix for, I'll post it as part of an imx7-media-csi
> series.
> 
> 

Hi Laurent,

You haven't posted that fix you're talking about, right? The below
driver (attached; I'll send it as patches after I successfully tested
myself, and cleanup and fixes obviously) results in the same situation
I described above:

* missing link from mipi (entity 10) -> csi (entity 1):
------------------------------------------------------

Device topology
- entity 1: csi (2 pads, 1 link)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev0
	pad0: Sink
		[fmt:UYVY8_2X8/640x480 field:none colorspace:srgb
xfer:srgb ycbcr:601 quantization:lim-range]
	pad1: Source
		[fmt:UYVY8_2X8/640x480 field:none colorspace:srgb
xfer:srgb ycbcr:601 quantization:lim-range]
		-> "csi capture":0 [ENABLED,IMMUTABLE]

- entity 4: csi capture (1 pad, 1 link)
            type Node subtype V4L flags 0
            device node name /dev/video0
	pad0: Sink
		<- "csi":1 [ENABLED,IMMUTABLE]

- entity 10: imx8mq-mipi-csis.0 (2 pads, 1 link)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev1
	pad0: Sink
		<- "hi846 2-0020":0 []
	pad1: Source

- entity 13: hi846 2-0020 (1 pad, 1 link)
             type V4L2 subdev subtype Sensor flags 0
             device node name /dev/v4l-subdev2
	pad0: Source
		[fmt:SGBRG10_1X10/640x480 field:none colorspace:raw]
		-> "imx8mq-mipi-csis.0":0 []


* and the mentioned vb2 WARN_ON:
--------------------------------

[   56.120834] imx7-csi 30a90000.csi1_bridge: begin graph walk at 'csi
capture'
[   56.120859] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi' on
stack
[   56.120865] imx7-csi 30a90000.csi1_bridge: walk: skipping entity
'csi capture' (already seen)
[   56.120871] imx7-csi 30a90000.csi1_bridge: walk: returning entity
'csi'
[   56.120877] imx7-csi 30a90000.csi1_bridge: walk: returning entity
'csi capture'
[   56.127415] vb2_common_vm_open: 000000006622b5ef, refcount: 1, vma:
ffffabe0b000-ffffabea1000
[   56.127438] vb2_dc_mmap: mapped dma addr 0xe8100000 at
0xffffabe0b000, size 614400
[   56.127480] vb2_common_vm_open: 00000000e689fd4f, refcount: 1, vma:
ffffabd75000-ffffabe0b000
[   56.127488] vb2_dc_mmap: mapped dma addr 0xe8200000 at
0xffffabd75000, size 614400
[   56.127501] vb2_common_vm_open: 00000000485fa30a, refcount: 1, vma:
ffffabcdf000-ffffabd75000
[   56.127509] vb2_dc_mmap: mapped dma addr 0xe8300000 at
0xffffabcdf000, size 614400
[   56.127522] vb2_common_vm_open: 0000000092607c6a, refcount: 1, vma:
ffffabc49000-ffffabcdf000
[   56.127529] vb2_dc_mmap: mapped dma addr 0xe8400000 at
0xffffabc49000, size 614400
[   56.127579] imx7-csi 30a90000.csi1_bridge: begin graph walk at 'csi'
[   56.127587] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi
capture' on stack
[   56.127593] imx7-csi 30a90000.csi1_bridge: walk: skipping entity
'csi' (already seen)
[   56.127599] imx7-csi 30a90000.csi1_bridge: walk: returning entity
'csi capture'
[   56.127604] imx7-csi 30a90000.csi1_bridge: walk: returning entity
'csi'
[   56.128102] imx7-csi 30a90000.csi1_bridge: begin graph walk at 'csi'
[   56.128111] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi
capture' on stack
[   56.128117] imx7-csi 30a90000.csi1_bridge: walk: skipping entity
'csi' (already seen)
[   56.128122] imx7-csi 30a90000.csi1_bridge: walk: returning entity
'csi capture'
[   56.128127] imx7-csi 30a90000.csi1_bridge: walk: returning entity
'csi'
[   56.128133] imx7-csi 30a90000.csi1_bridge: pipeline start failed
with -19
[   56.135091] ------------[ cut here ]------------
[   56.135102] WARNING: CPU: 3 PID: 1984 at
drivers/media/common/videobuf2/videobuf2-core.c:1568
vb2_start_streaming+0xe4/0x160 [videobuf2_common]
[   56.135151] Modules linked in: aes_ce_ccm exfat rfcomm algif_hash
algif_skcipher af_alg bnep qmi_wwan cdc_wdm option usbnet usb_wwan
usbserial mii ofpart mousedev spi_nor caam_jr mtd caamhash_desc
caamalg_desc crypto_engine uas redpine_sdio usb_storage redpine_91x
bluetooth mac80211 aes_ce_blk crypto_simd crct10dif_ce ghash_ce
cfg80211 sha2_ce sha1_ce st_lsm6dsx_spi bq25890_charger pwm_vibra
snd_soc_gtm601 snd_soc_simple_card snd_soc_simple_card_utils hi846
s5k3l6xx edt_ft5x06 snd_soc_wm8962 mx6s_capture imx7_media_csi(C)
imx_media_common(C) videobuf2_dma_contig imx8mq_mipi_csis(C)
mxc_mipi_csi2_yav videobuf2_memops videobuf2_v4l2 tps6598x
videobuf2_common vcnl4000 v4l2_fwnode typec
industrialio_triggered_buffer leds_lm3560 videodev mc st_lsm6dsx_i2c
st_lsm6dsx kfifo_buf gnss_mtk gnss_serial gnss snd_soc_fsl_sai imx_sdma
snvs_pwrkey imx_pcm_dma virt_dma snd_soc_core imx2_wdt watchdog
snd_pcm_dmaengine snd_pcm snd_timer snd caam soundcore error rfkill_hks
rfkill ledtrig_timer usb_f_acm
[   56.135494]  u_serial usb_f_rndis g_multi usb_f_mass_storage u_ether
libcomposite ledtrig_pattern fuse ip_tables x_tables ipv6 xhci_plat_hcd
xhci_hcd usbcore imx_dcss clk_bd718x7 cdns_mhdp_imx cdns_mhdp_drmcore
dwc3 ulpi udc_core roles phy_fsl_imx8mq_usb usb_common
[   56.135596] CPU: 3 PID: 1984 Comm: v4l2-ctl Tainted: G         C   
5.12.2-librem5-00049-g99f86eccfeae #335
[   56.135607] Hardware name: Purism Librem 5r4 (DT)
[   56.135613] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
[   56.135623] pc : vb2_start_streaming+0xe4/0x160 [videobuf2_common]
[   56.135653] lr : vb2_start_streaming+0x74/0x160 [videobuf2_common]
[   56.135682] sp : ffff8000148bbba0
[   56.135686] x29: ffff8000148bbba0 x28: ffff00001e833f00 
[   56.135700] x27: 0000000040045612 x26: ffff800008f406a0 
[   56.135713] x25: 0000000000000000 x24: ffff8000148bbd58 
[   56.135725] x23: ffff0000be730138 x22: ffff00000230ab00 
[   56.135738] x21: ffff0000be730330 x20: ffff0000be730348 
[   56.135751] x19: 00000000ffffffed x18: 0000000000000000 
[   56.135763] x17: 0000000000000000 x16: 0000000000000000 
[   56.135776] x15: 0000000000000030 x14: ffffffffffffffff 
[   56.135788] x13: ffff8000948bb737 x12: ffff8000148bb73f 
[   56.135801] x11: ffff80001152a7a0 x10: 00000000ffffe000 
[   56.135813] x9 : ffff800008f3c900 x8 : ffff80001147a7a0 
[   56.135826] x7 : ffff80001152a7a0 x6 : 0000000000000000 
[   56.135838] x5 : 0000000000000000 x4 : 0000000000000000 
[   56.135850] x3 : ffff0000be730344 x2 : 0000000000000000 
[   56.135863] x1 : ffff800008fe4000 x0 : ffff0000253d29f0 
[   56.135877] Call trace:
[   56.135882]  vb2_start_streaming+0xe4/0x160 [videobuf2_common]
[   56.135912]  vb2_core_streamon+0x9c/0x1a0 [videobuf2_common]
[   56.135940]  vb2_ioctl_streamon+0x68/0xbc [videobuf2_v4l2]
[   56.135964]  v4l_streamon+0x30/0x40 [videodev]
[   56.136063]  __video_do_ioctl+0x194/0x3f4 [videodev]
[   56.136145]  video_usercopy+0x1a4/0x770 [videodev]
[   56.136226]  video_ioctl2+0x24/0x40 [videodev]
[   56.136305]  v4l2_ioctl+0x4c/0x70 [videodev]
[   56.136385]  __arm64_sys_ioctl+0xb4/0xfc
[   56.136401]  el0_svc_common.constprop.0+0x68/0x130
[   56.136416]  do_el0_svc+0x28/0x34
[   56.136426]  el0_svc+0x2c/0x54
[   56.136438]  el0_sync_handler+0x1a4/0x1b0
[   56.136449]  el0_sync+0x174/0x180
[   56.136459] ---[ end trace 122c8abc5f14e4e5 ]---


thank you very much for your help,

                           martin

Laurent Pinchart May 19, 2021, 1:14 a.m. UTC | #10

Hi Martin,

On Tue, May 18, 2021 at 04:39:00PM +0200, Martin Kepplinger wrote:
> Am Sonntag, dem 16.05.2021 um 01:55 +0300 schrieb Laurent Pinchart:
> > On Tue, May 04, 2021 at 05:59:39PM +0200, Martin Kepplinger wrote:
> > > hi Laurent, again thanks a lot for posting this series! I can't fully test
> > > it, but base my work for imx8mq on it now. imx8mq includes
> > > yet another mipi phy version than this and below is some very rough testing
> > > code. it's not at all something I sign-off on but my following
> > > problem is based on it.
> > 
> > Unless I'm mistaken, the CSI-2 receiver in the i.MX8MQ is a completely
> > different device. I wouldn't try to support it in the imx7-mipi-csis
> > driver, but in a separate driver.
> > 
> > >  * configured to use both staging csi drivers
> > >  * the csi bridge driver at least streams frames together with the
> > > nxp "yav" mipi driver
> > > 
> > > media-ctl -p now says the output below, so one link from mipi to
> > > csi is missing.
> > > 
> > > Note that
> > > 
> > > media-ctl --set-v4l2 "'csi':0 [fmt:SBGGR10/640x480]"
> > > works in that it changes the configured format below, but
> > > 
> > > media-ctl -l "'imx7-mipi-csis.0':1" -> "'csi':0[1]"
> > > doesn't create said missing link.
> > 
> > media-ctl can't create links, it can only enable or disable them. Link
> > creation is the prerogative of drivers.
> > 
> > > Do I maybe use that wrongly? If now, does anything come to mind that would
> > > be missing specifically?
> > 
> > The link should be created by the call to media_create_pad_link() in
> > imx_media_capture_device_register(). You'll need to figure out if the
> > function is called and returns an error early, or if it doesn't get
> > called at all, and why.
> > 
> > > When trying to stream anyway (if that makes sense), I get the
> > > following:
> > > 
> > > [ 2008.377470] capture_start_streaming: starting
> > > [ 2008.381883] capture_find_format: calling imx_media_find_mbus_format with code 0x2006
> > > [ 2008.389671] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt: capture_find_format err
> > > [ 2008.397794] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt: capture_find_format found colorspace 0x1 != 0x0
> > > [ 2008.407999] imx7-csi 30a90000.csi1_bridge: capture format not valid: -32
> > > 
> > > and if I ignore that (because I'm not yet sure whether that is specific to
> > > platforms including an IPU), I get a WARN_ON from vb2_start_streaming()
> > 
> > That I have a fix for, I'll post it as part of an imx7-media-csi
> > series.
> 
> Hi Laurent,
> 
> You haven't posted that fix you're talking about, right?

Correct. It's now fixed (see "[PATCH] media: imx: imx7-media-csi: Fix
buffer return upon stream start failure", I've CC'ed you).

> The below
> driver (attached; I'll send it as patches after I successfully tested
> myself, and cleanup and fixes obviously)

Don't forget the DT bindings at that point :-)

> results in the same situation I described above:
> 
> * missing link from mipi (entity 10) -> csi (entity 1):

The link is supposed to be created by v4l2_create_fwnode_links_to_pad(),
called from imx7_csi_notify_bound(). Could you trace the calls and
figure out what goes wrong ?

> ------------------------------------------------------
> 
> Device topology
> - entity 1: csi (2 pads, 1 link)
>             type V4L2 subdev subtype Unknown flags 0
>             device node name /dev/v4l-subdev0
> 	pad0: Sink
> 		[fmt:UYVY8_2X8/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
> 	pad1: Source
> 		[fmt:UYVY8_2X8/640x480 field:none colorspace:srgb xfer:srgb ycbcr:601 quantization:lim-range]
> 		-> "csi capture":0 [ENABLED,IMMUTABLE]
> 
> - entity 4: csi capture (1 pad, 1 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video0
> 	pad0: Sink
> 		<- "csi":1 [ENABLED,IMMUTABLE]
> 
> - entity 10: imx8mq-mipi-csis.0 (2 pads, 1 link)
>              type V4L2 subdev subtype Unknown flags 0
>              device node name /dev/v4l-subdev1
> 	pad0: Sink
> 		<- "hi846 2-0020":0 []
> 	pad1: Source
> 
> - entity 13: hi846 2-0020 (1 pad, 1 link)
>              type V4L2 subdev subtype Sensor flags 0
>              device node name /dev/v4l-subdev2
> 	pad0: Source
> 		[fmt:SGBRG10_1X10/640x480 field:none colorspace:raw]
> 		-> "imx8mq-mipi-csis.0":0 []
> 
> 
> * and the mentioned vb2 WARN_ON:
> --------------------------------
> 
> [   56.120834] imx7-csi 30a90000.csi1_bridge: begin graph walk at 'csi capture'
> [   56.120859] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi' on stack
> [   56.120865] imx7-csi 30a90000.csi1_bridge: walk: skipping entity 'csi capture' (already seen)
> [   56.120871] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi'
> [   56.120877] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi capture'
> [   56.127415] vb2_common_vm_open: 000000006622b5ef, refcount: 1, vma: ffffabe0b000-ffffabea1000
> [   56.127438] vb2_dc_mmap: mapped dma addr 0xe8100000 at 0xffffabe0b000, size 614400
> [   56.127480] vb2_common_vm_open: 00000000e689fd4f, refcount: 1, vma: ffffabd75000-ffffabe0b000
> [   56.127488] vb2_dc_mmap: mapped dma addr 0xe8200000 at 0xffffabd75000, size 614400
> [   56.127501] vb2_common_vm_open: 00000000485fa30a, refcount: 1, vma: ffffabcdf000-ffffabd75000
> [   56.127509] vb2_dc_mmap: mapped dma addr 0xe8300000 at 0xffffabcdf000, size 614400
> [   56.127522] vb2_common_vm_open: 0000000092607c6a, refcount: 1, vma: ffffabc49000-ffffabcdf000
> [   56.127529] vb2_dc_mmap: mapped dma addr 0xe8400000 at 0xffffabc49000, size 614400
> [   56.127579] imx7-csi 30a90000.csi1_bridge: begin graph walk at 'csi'
> [   56.127587] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi capture' on stack
> [   56.127593] imx7-csi 30a90000.csi1_bridge: walk: skipping entity 'csi' (already seen)
> [   56.127599] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi capture'
> [   56.127604] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi'
> [   56.128102] imx7-csi 30a90000.csi1_bridge: begin graph walk at 'csi'
> [   56.128111] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi capture' on stack
> [   56.128117] imx7-csi 30a90000.csi1_bridge: walk: skipping entity 'csi' (already seen)
> [   56.128122] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi capture'
> [   56.128127] imx7-csi 30a90000.csi1_bridge: walk: returning entity 'csi'
> [   56.128133] imx7-csi 30a90000.csi1_bridge: pipeline start failed with -19
> [   56.135091] ------------[ cut here ]------------
> [   56.135102] WARNING: CPU: 3 PID: 1984 at drivers/media/common/videobuf2/videobuf2-core.c:1568 vb2_start_streaming+0xe4/0x160 [videobuf2_common]
> [   56.135151] Modules linked in: aes_ce_ccm exfat rfcomm algif_hash algif_skcipher af_alg bnep qmi_wwan cdc_wdm option usbnet usb_wwan usbserial mii ofpart mousedev spi_nor caam_jr mtd caamhash_desc caamalg_desc crypto_engine uas redpine_sdio usb_storage redpine_91x bluetooth mac80211 aes_ce_blk crypto_simd crct10dif_ce ghash_ce cfg80211 sha2_ce sha1_ce st_lsm6dsx_spi bq25890_charger pwm_vibra snd_soc_gtm601 snd_soc_simple_card snd_soc_simple_card_utils hi846 s5k3l6xx edt_ft5x06 snd_soc_wm8962 mx6s_capture imx7_media_csi(C) imx_media_common(C) videobuf2_dma_contig imx8mq_mipi_csis(C) mxc_mipi_csi2_yav videobuf2_memops videobuf2_v4l2 tps6598x videobuf2_common vcnl4000 v4l2_fwnode typec industrialio_triggered_buffer leds_lm3560 videodev mc st_lsm6dsx_i2c st_lsm6dsx kfifo_buf gnss_mtk gnss_serial gnss snd_soc_fsl_sai imx_sdma snvs_pwrkey imx_pcm_dma virt_dma snd_soc_core imx2_wdt watchdog snd_pcm_dmaengine snd_pcm snd_timer snd caam soundcore error rfkill_hks rfkill ledtrig_timer usb_f_acm
> [   56.135494]  u_serial usb_f_rndis g_multi usb_f_mass_storage u_ether libcomposite ledtrig_pattern fuse ip_tables x_tables ipv6 xhci_plat_hcd xhci_hcd usbcore imx_dcss clk_bd718x7 cdns_mhdp_imx cdns_mhdp_drmcore dwc3 ulpi udc_core roles phy_fsl_imx8mq_usb usb_common
> [   56.135596] CPU: 3 PID: 1984 Comm: v4l2-ctl Tainted: G         C   5.12.2-librem5-00049-g99f86eccfeae #335
> [   56.135607] Hardware name: Purism Librem 5r4 (DT)
> [   56.135613] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> [   56.135623] pc : vb2_start_streaming+0xe4/0x160 [videobuf2_common]
> [   56.135653] lr : vb2_start_streaming+0x74/0x160 [videobuf2_common]
> [   56.135682] sp : ffff8000148bbba0
> [   56.135686] x29: ffff8000148bbba0 x28: ffff00001e833f00 
> [   56.135700] x27: 0000000040045612 x26: ffff800008f406a0 
> [   56.135713] x25: 0000000000000000 x24: ffff8000148bbd58 
> [   56.135725] x23: ffff0000be730138 x22: ffff00000230ab00 
> [   56.135738] x21: ffff0000be730330 x20: ffff0000be730348 
> [   56.135751] x19: 00000000ffffffed x18: 0000000000000000 
> [   56.135763] x17: 0000000000000000 x16: 0000000000000000 
> [   56.135776] x15: 0000000000000030 x14: ffffffffffffffff 
> [   56.135788] x13: ffff8000948bb737 x12: ffff8000148bb73f 
> [   56.135801] x11: ffff80001152a7a0 x10: 00000000ffffe000 
> [   56.135813] x9 : ffff800008f3c900 x8 : ffff80001147a7a0 
> [   56.135826] x7 : ffff80001152a7a0 x6 : 0000000000000000 
> [   56.135838] x5 : 0000000000000000 x4 : 0000000000000000 
> [   56.135850] x3 : ffff0000be730344 x2 : 0000000000000000 
> [   56.135863] x1 : ffff800008fe4000 x0 : ffff0000253d29f0 
> [   56.135877] Call trace:
> [   56.135882]  vb2_start_streaming+0xe4/0x160 [videobuf2_common]
> [   56.135912]  vb2_core_streamon+0x9c/0x1a0 [videobuf2_common]
> [   56.135940]  vb2_ioctl_streamon+0x68/0xbc [videobuf2_v4l2]
> [   56.135964]  v4l_streamon+0x30/0x40 [videodev]
> [   56.136063]  __video_do_ioctl+0x194/0x3f4 [videodev]
> [   56.136145]  video_usercopy+0x1a4/0x770 [videodev]
> [   56.136226]  video_ioctl2+0x24/0x40 [videodev]
> [   56.136305]  v4l2_ioctl+0x4c/0x70 [videodev]
> [   56.136385]  __arm64_sys_ioctl+0xb4/0xfc
> [   56.136401]  el0_svc_common.constprop.0+0x68/0x130
> [   56.136416]  do_el0_svc+0x28/0x34
> [   56.136426]  el0_svc+0x2c/0x54
> [   56.136438]  el0_sync_handler+0x1a4/0x1b0
> [   56.136449]  el0_sync+0x174/0x180
> [   56.136459] ---[ end trace 122c8abc5f14e4e5 ]---

Hopefully the patch mentioned above will fix this.

> // SPDX-License-Identifier: GPL-2.0
> /*
>  * Freescale i.MX8MQ SoC series MIPI-CSI receiver driver
>  *
>  * Copyright (C) 2021 Purism SPC
>  * Copyright (C) 2019 Linaro Ltd
>  * Copyright (C) 2015-2016 Freescale Semiconductor, Inc. All Rights Reserved.
>  * Copyright (C) 2011 - 2013 Samsung Electronics Co., Ltd.
>  *
>  */
> 
> #include <linux/clk.h>
> #include <linux/debugfs.h>
> #include <linux/delay.h>
> #include <linux/errno.h>
> #include <linux/interrupt.h>
> #include <linux/io.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
> #include <linux/regmap.h>
> #include <linux/mfd/syscon.h>
> #include <linux/regulator/consumer.h>
> #include <linux/reset.h>
> #include <linux/spinlock.h>
> 
> #include <media/v4l2-common.h>
> #include <media/v4l2-device.h>
> #include <media/v4l2-fwnode.h>
> #include <media/v4l2-mc.h>
> #include <media/v4l2-subdev.h>
> 
> #define CSIS_DRIVER_NAME			"imx8mq-mipi-csis"
> #define CSIS_SUBDEV_NAME			CSIS_DRIVER_NAME
> 
> #define CSIS_PAD_SINK				0
> #define CSIS_PAD_SOURCE				1
> #define CSIS_PADS_NUM				2
> 
> #define MIPI_CSIS_DEF_PIX_WIDTH			640
> #define MIPI_CSIS_DEF_PIX_HEIGHT		480
> 
> /* Register map definition */
> 
> /* i.MX8MQ CSI-2 controller CSR */
> /* TODO 0x100, to dts? */
> #define CSI2RX_CFG_NUM_LANES			0x100
> #define CSI2RX_CFG_DISABLE_DATA_LANES		0x104
> #define CSI2RX_BIT_ERR				0x108
> #define CSI2RX_IRQ_STATUS			0x10C
> #define CSI2RX_IRQ_MASK				0x110
> #define CSI2RX_ULPS_STATUS			0x114
> #define CSI2RX_PPI_ERRSOT_HS			0x118
> #define CSI2RX_PPI_ERRSOTSYNC_HS		0x11C
> #define CSI2RX_PPI_ERRESC	 		0x120
> #define CSI2RX_PPI_ERRSYNCESC			0x124
> #define CSI2RX_PPI_ERRCONTROL			0x128
> #define CSI2RX_CFG_DISABLE_PAYLOAD_0		0x12C
> #define CSI2RX_CFG_DISABLE_PAYLOAD_1		0x130
> 
> enum {
> 	ST_POWERED	= 1,
> 	ST_STREAMING	= 2,
> 	ST_SUSPENDED	= 4,
> };
> 
> static const char * const mipi_csis_clk_id[] = {
> 	"clk_core",
> 	"clk_esc",
> 	"clk_pxl",
> 	"clk_clko2",
> };
> 
> struct csis_imx8mq_hw_reset {
> 	struct regmap *src;
> 	u8 req_src;
> 	u8 rst_val;
> };
> 
> struct csis_imx8mq_phy_gpr {
> 	struct regmap *gpr;
> 	u8 req_src;
> };
> 
> #define	GPR_CSI2_1_RX_ENABLE		BIT(13)
> #define	GPR_CSI2_1_VID_INTFC_ENB	BIT(12)
> #define	GPR_CSI2_1_HSEL			BIT(10)
> #define	GPR_CSI2_1_CONT_CLK_MODE 	BIT(8)
> #define	GPR_CSI2_1_S_PRG_RXHS_SETTLE(x)	(((x) & 0x3F) << 2)
> /*
>  * rxhs_settle[0] ... <720x480
>  * rxhs_settle[1] ... >720*480
>  *
>  * https://community.nxp.com/t5/i-MX-Processors/Explenation-for-HS-SETTLE-parameter-in-MIPI-CSI-D-PHY-registers/m-p/764275/highlight/true#M118744
>  */
> static u8 rxhs_settle[2] = { 0x14, 0x9 };
> 
> struct csi_state {
> 	struct device *dev;
> 	void __iomem *regs;
> 	struct clk_bulk_data *clks;
> 	struct reset_control *mrst;
> 	struct regulator *mipi_phy_regulator;
> 	u8 index;
> 
> 	struct v4l2_subdev sd;
> 	struct media_pad pads[CSIS_PADS_NUM];
> 	struct v4l2_async_notifier notifier;
> 	struct v4l2_subdev *src_sd;
> 
> 	struct v4l2_fwnode_bus_mipi_csi2 bus;
> 	u32 hs_settle;
> 	u32 clk_settle;
> 
> 	struct mutex lock;	/* Protect csis_fmt, format_mbus and state */
> 	u32 state;
> 
> 	struct dentry *debugfs_root;
> 	bool debug;
> 
> 	struct csis_imx8mq_hw_reset hw_reset;
> 	struct csis_imx8mq_phy_gpr phy_gpr;
> 	u32 send_level;
> };
> 
> /* -----------------------------------------------------------------------------
>  * Format helpers
>  */
> 
> /* -----------------------------------------------------------------------------
>  * Hardware configuration
>  */
> 
> static inline u32 mipi_csis_read(struct csi_state *state, u32 reg)
> {
> 	return readl(state->regs + reg);
> }
> 
> static inline void mipi_csis_write(struct csi_state *state, u32 reg, u32 val)
> {
> 	writel(val, state->regs + reg);
> }
> 
> static void mipi_csis_enable_interrupts(struct csi_state *state, bool on)
> {
> 	return;
> }
> 
> static void mipi_csis_sw_reset(struct csi_state *state)
> {
> 	/* TODO yav: mxc_mipi_csi1_phy_reset */
> 
> 	struct device *dev = state->dev;
> 	struct device_node *np = dev->of_node;
> 	struct device_node *node;
> 	phandle phandle;
> 	u32 out_val[3];
> 	int ret;
> 
> 	dev_dbg(dev, "%s: starting\n", __func__);
> 
> 	ret = of_property_read_u32_array(np, "csis-phy-reset", out_val, 3);
> 	if (ret) {
> 		dev_info(dev, "no csis-hw-reset property found: %d\n", ret);
> 		return;
> 	}
> 
> 	phandle = *out_val;
> 
> 	node = of_find_node_by_phandle(phandle);
> 	if (!node) {
> 		ret = PTR_ERR(node);
> 		dev_dbg(dev, "not find src node by phandle: %d\n", ret);
> 	}
> 	state->hw_reset.src = syscon_node_to_regmap(node);
> 	if (IS_ERR(state->hw_reset.src)) {
> 		ret = PTR_ERR(state->hw_reset.src);
> 		dev_err(dev, "failed to get src regmap: %d\n", ret);
> 	}
> 	of_node_put(node);
> 	if (ret < 0)
> 		return;
> 
> 	state->hw_reset.req_src = out_val[1];
> 	state->hw_reset.rst_val = out_val[2];
> 
> 	/* reset imx8mq mipi phy */
> 	regmap_update_bits(state->hw_reset.src,
> 			   state->hw_reset.req_src,
> 			   state->hw_reset.rst_val,
> 			   state->hw_reset.rst_val);
> 	msleep(20);
> 
> 	dev_dbg(dev, "%s: done\n", __func__);
> 
> 	return;
> }
> 
> static void mipi_csis_system_enable(struct csi_state *state, int on)
> {
> 	struct device *dev = state->dev;
> 	struct device_node *np = dev->of_node;
> 	struct device_node *node;
> 	phandle phandle;
> 	u32 out_val[2];
> 	int ret;
> 
> 	if (!on) {
> 		/* Disable Data lanes */
> 		mipi_csis_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, 0xf);
> 		return;
> 	}
> 
> 	ret = of_property_read_u32_array(np, "phy-gpr", out_val, 2);
> 	if (ret) {
> 		dev_info(dev, "no phy-gpr property found\n");
> 		return;
> 	}
> 
> 	phandle = *out_val;
> 
> 	node = of_find_node_by_phandle(phandle);
> 	if (!node) {
> 		dev_dbg(dev, "not find gpr node by phandle\n");
> 		ret = PTR_ERR(node);
> 	}
> 	state->phy_gpr.gpr = syscon_node_to_regmap(node);
> 	if (IS_ERR(state->phy_gpr.gpr)) {
> 		dev_err(dev, "failed to get gpr regmap\n");
> 		ret = PTR_ERR(state->phy_gpr.gpr);
> 	}
> 	of_node_put(node);
> 	if (ret < 0)
> 		return;
> 
> 	state->phy_gpr.req_src = out_val[1];
> 
> 	regmap_update_bits(state->phy_gpr.gpr,
> 			   state->phy_gpr.req_src,
> 			   0x3FFF,
> 			   GPR_CSI2_1_RX_ENABLE |
> 			   GPR_CSI2_1_VID_INTFC_ENB |
> 			   GPR_CSI2_1_HSEL |
> 			   GPR_CSI2_1_CONT_CLK_MODE |
> 			   GPR_CSI2_1_S_PRG_RXHS_SETTLE(state->
> 							hs_settle));
> 
> 	dev_dbg(dev, "%s: hs_settle: 0x%X\n", __func__, state->hs_settle);
> 
> 	return;
> }
> 
> static int mipi_csis_calculate_params(struct csi_state *state)
> {
> 	s64 link_freq;
> 	u32 lane_rate;
> 
> 	state->hs_settle = rxhs_settle[0];
> #if 0
> 	/* Calculate the line rate from the pixel rate. */
> 	link_freq = v4l2_get_link_freq(state->src_sd->ctrl_handler,
> 				       state->csis_fmt->width,
> 				       state->bus.num_data_lanes * 2);
> 	if (link_freq < 0) {
> 		dev_err(state->dev, "Unable to obtain link frequency: %d\n",
> 			(int)link_freq);
> 		return link_freq;
> 	}
> 
> 	lane_rate = link_freq * 2;
> 
> 	if (lane_rate < 80000000 || lane_rate > 1500000000) {
> 		dev_dbg(state->dev, "Out-of-bound lane rate %u\n", lane_rate);
> 		return -EINVAL;
> 	}
> 
> 	/*
> 	 * The HSSETTLE counter value is document in a table, but can also
> 	 * easily be calculated. Hardcode the CLKSETTLE value to 0 for now
> 	 * (which is documented as corresponding to CSI-2 v0.87 to v1.00) until
> 	 * we figure out how to compute it correctly.
> 	 */
> 	state->hs_settle = (lane_rate - 5000000) / 45000000;
> 	state->clk_settle = 0;
> 
> 	dev_dbg(state->dev, "lane rate %u, Tclk_settle %u, Ths_settle %u\n",
> 		lane_rate, state->clk_settle, state->hs_settle);
> #endif
> 	return 0;
> }
> 
> static void mipi_csis_set_params(struct csi_state *state)
> {
> 	int lanes = state->bus.num_data_lanes;
> 	u32 val = 0;
> 	int i;
> 
> 	/* Lanes */
> 	mipi_csis_write(state, CSI2RX_CFG_NUM_LANES, lanes - 1);
> 
> dev_err(state->dev, "imx8mq: %d lanes\n", lanes);
> 
> 	for (i = 0; i < lanes; i++)
> 		val |= (1 << i);
> 
> 	val = 0xF & ~val;
> 	mipi_csis_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, val);
> 
> dev_err(state->dev, "imx8mq: CSI2RX_CFG_DISABLE_DATA_LANES: 0x%X\n", val);
> 
> 	/* Mask interrupt */
> 	// Don't let ULPS (ultra-low power status) interrupts flood
> 	mipi_csis_write(state, CSI2RX_IRQ_MASK, 0x1ff);
> 
> 	mipi_csis_write(state, 0x180, 1);
> 	/* vid_vc */
> 	mipi_csis_write(state, 0x184, 1);
> 	mipi_csis_write(state, 0x188, state->send_level);
> }
> 
> static int mipi_csis_clk_enable(struct csi_state *state)
> {
> 	return clk_bulk_prepare_enable(ARRAY_SIZE(mipi_csis_clk_id), state->clks);
> }
> 
> static void mipi_csis_clk_disable(struct csi_state *state)
> {
> 	clk_bulk_disable_unprepare(ARRAY_SIZE(mipi_csis_clk_id), state->clks);
> }
> 
> static int mipi_csis_clk_get(struct csi_state *state)
> {
> 	unsigned int i;
> 	int ret;
> 
> 	state->clks = devm_kcalloc(state->dev, ARRAY_SIZE(mipi_csis_clk_id),
> 				   sizeof(*state->clks), GFP_KERNEL);
> 
> 	if (!state->clks)
> 		return -ENOMEM;
> 
> 	for (i = 0; i < ARRAY_SIZE(mipi_csis_clk_id); i++)
> 		state->clks[i].id = mipi_csis_clk_id[i];
> 
> 	ret = devm_clk_bulk_get(state->dev, ARRAY_SIZE(mipi_csis_clk_id),
> 				state->clks);
> 	return ret;
> }
> 
> static void mipi_csis_start_stream(struct csi_state *state)
> {
> 	mipi_csis_sw_reset(state);
> 	mipi_csis_set_params(state);
> 	mipi_csis_system_enable(state, true);
> 	mipi_csis_enable_interrupts(state, true);
> }
> 
> static void mipi_csis_stop_stream(struct csi_state *state)
> {
> 	mipi_csis_enable_interrupts(state, false);
> 	mipi_csis_system_enable(state, false);
> }
> 
> /* -----------------------------------------------------------------------------
>  * PHY regulator and reset
>  */
> 
> static int mipi_csis_phy_enable(struct csi_state *state)
> {
> 	return 0;
> }
> 
> static int mipi_csis_phy_disable(struct csi_state *state)
> {
> 	return 0;
> }
> 
> static void mipi_csis_phy_reset(struct csi_state *state)
> {
> 	return;
> }
> 
> static int mipi_csis_phy_init(struct csi_state *state)
> {
> 	return 0;
> }
> 
> /* -----------------------------------------------------------------------------
>  * Debug
>  */
> 
> static void mipi_csis_clear_counters(struct csi_state *state)
> {
> 	return;
> }
> 
> static void mipi_csis_log_counters(struct csi_state *state, bool non_errors)
> {
> 	return;
> }
> 
> static int mipi_csis_dump_regs(struct csi_state *state)
> {
> 	return 0;
> }
> 
> static int mipi_csis_dump_regs_show(struct seq_file *m, void *private)
> {
> 	struct csi_state *state = m->private;
> 
> 	return mipi_csis_dump_regs(state);
> }
> DEFINE_SHOW_ATTRIBUTE(mipi_csis_dump_regs);
> 
> static void mipi_csis_debugfs_init(struct csi_state *state)
> {
> 	state->debugfs_root = debugfs_create_dir(dev_name(state->dev), NULL);
> 
> 	debugfs_create_bool("debug_enable", 0600, state->debugfs_root,
> 			    &state->debug);
> 	debugfs_create_file("dump_regs", 0600, state->debugfs_root, state,
> 			    &mipi_csis_dump_regs_fops);
> }
> 
> static void mipi_csis_debugfs_exit(struct csi_state *state)
> {
> 	debugfs_remove_recursive(state->debugfs_root);
> }
> 
> /* -----------------------------------------------------------------------------
>  * V4L2 subdev operations
>  */
> 
> static struct csi_state *mipi_sd_to_csis_state(struct v4l2_subdev *sdev)
> {
> 	return container_of(sdev, struct csi_state, sd);
> }
> 
> static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> {
> 	struct csi_state *state = mipi_sd_to_csis_state(sd);
> 	int ret;
> 
> 	mipi_csis_write(state, CSI2RX_IRQ_MASK, 0x008);
> 
> 	dev_dbg(state->dev, "%s: enable: %d\n", __func__, enable);
> 
> 	if (enable) {
> 		ret = mipi_csis_calculate_params(state);
> 		if (ret < 0)
> 			return ret;
> 
> 		mipi_csis_clear_counters(state);
> 
> 		ret = pm_runtime_get_sync(state->dev);
> 		if (ret < 0) {
> 			pm_runtime_put_noidle(state->dev);
> 			return ret;
> 		}
> 		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
> 		if (ret < 0 && ret != -ENOIOCTLCMD)
> 			goto done;
> 	}
> 
> 	mutex_lock(&state->lock);
> 
> 	if (enable) {
> 		if (state->state & ST_SUSPENDED) {
> 			ret = -EBUSY;
> 			goto unlock;
> 		}
> 
> 		mipi_csis_start_stream(state);
> 		ret = v4l2_subdev_call(state->src_sd, video, s_stream, 1);
> 		if (ret < 0)
> 			goto unlock;
> 
> 		mipi_csis_log_counters(state, true);
> 
> 		state->state |= ST_STREAMING;
> 	} else {
> 		v4l2_subdev_call(state->src_sd, video, s_stream, 0);
> 		ret = v4l2_subdev_call(state->src_sd, core, s_power, 0);
> 		if (ret == -ENOIOCTLCMD)
> 			ret = 0;
> 		mipi_csis_stop_stream(state);
> 		state->state &= ~ST_STREAMING;
> 		if (state->debug)
> 			mipi_csis_log_counters(state, true);
> 	}
> 
> unlock:
> 	mutex_unlock(&state->lock);
> 
> done:
> 	if (!enable || ret < 0)
> 		pm_runtime_put(state->dev);
> 
> 	return ret;
> }
> 
> static int mipi_csis_get_fmt(struct v4l2_subdev *sd,
> 			     struct v4l2_subdev_pad_config *cfg,
> 			     struct v4l2_subdev_format *sdformat)
> {
> 	struct csi_state *state = mipi_sd_to_csis_state(sd);
> 
> 	return v4l2_subdev_call(state->src_sd, pad, get_fmt, NULL, sdformat);
> }
> 
> static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
> 				    struct v4l2_subdev_pad_config *cfg,
> 				    struct v4l2_subdev_mbus_code_enum *code)
> {
> 	struct csi_state *state = mipi_sd_to_csis_state(sd);
> 
> 	return v4l2_subdev_call(state->src_sd, pad, enum_mbus_code, NULL, code);
> }
> 
> static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
> 			     struct v4l2_subdev_pad_config *cfg,
> 			     struct v4l2_subdev_format *sdformat)
> {
> 	struct csi_state *state = mipi_sd_to_csis_state(sd);
> 
> 	/*
> 	 * The CSIS can't transcode in any way, the source format can't be
> 	 * modified.
> 	 */
> 	if (sdformat->pad == CSIS_PAD_SOURCE)
> 		return mipi_csis_get_fmt(sd, cfg, sdformat);
> 
> 	if (sdformat->pad != CSIS_PAD_SINK)
> 		return -EINVAL;
> 
> 	if (sdformat->format.width * sdformat->format.height > 720 * 480) {
> 		state->hs_settle = rxhs_settle[1];
> 	} else {
> 		state->hs_settle = rxhs_settle[0];
> 	}
> 	state->send_level = 64;
> 
> 	dev_dbg(state->dev,
> 		"%s: format %dx%d send_level %d hs_settle 0x%X\n", __func__,
> 		sdformat->format.width, sdformat->format.height,
> 		state->send_level, state->hs_settle);
> 
> 	return v4l2_subdev_call(state->src_sd, pad, set_fmt, NULL, sdformat);
> }
> 
> static int mipi_csis_log_status(struct v4l2_subdev *sd)
> {
> 	struct csi_state *state = mipi_sd_to_csis_state(sd);
> 
> 	mutex_lock(&state->lock);
> 	mipi_csis_log_counters(state, true);
> 	if (state->debug && (state->state & ST_POWERED))
> 		mipi_csis_dump_regs(state);
> 	mutex_unlock(&state->lock);
> 
> 	return 0;
> }
> 
> static const struct v4l2_subdev_core_ops mipi_csis_core_ops = {
> 	.log_status	= mipi_csis_log_status,
> };
> 
> static const struct v4l2_subdev_video_ops mipi_csis_video_ops = {
> 	.s_stream	= mipi_csis_s_stream,
> };
> 
> static const struct v4l2_subdev_pad_ops mipi_csis_pad_ops = {
> 	.enum_mbus_code		= mipi_csis_enum_mbus_code,
> 	.get_fmt		= mipi_csis_get_fmt,
> 	.set_fmt		= mipi_csis_set_fmt,
> };
> 
> static const struct v4l2_subdev_ops mipi_csis_subdev_ops = {
> 	.core	= &mipi_csis_core_ops,
> 	.video	= &mipi_csis_video_ops,
> 	.pad	= &mipi_csis_pad_ops,
> };
> 
> /* -----------------------------------------------------------------------------
>  * Media entity operations
>  */
> 
> static int mipi_csis_link_setup(struct media_entity *entity,
> 				const struct media_pad *local_pad,
> 				const struct media_pad *remote_pad, u32 flags)
> {
> 	struct v4l2_subdev *sd = media_entity_to_v4l2_subdev(entity);
> 	struct csi_state *state = mipi_sd_to_csis_state(sd);
> 	struct v4l2_subdev *remote_sd;
> 
> 	dev_dbg(state->dev, "link setup %s -> %s", remote_pad->entity->name,
> 		local_pad->entity->name);
> 
> 	/* We only care about the link to the source. */
> 	if (!(local_pad->flags & MEDIA_PAD_FL_SINK))
> 		return 0;
> 
> 	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
> 
> 	if (flags & MEDIA_LNK_FL_ENABLED) {
> 		if (state->src_sd)
> 			return -EBUSY;
> 
> 		state->src_sd = remote_sd;
> 	} else {
> 		state->src_sd = NULL;
> 	}
> 
> 	return 0;
> }
> 
> static const struct media_entity_operations mipi_csis_entity_ops = {
> 	.link_setup	= mipi_csis_link_setup,
> 	.link_validate	= v4l2_subdev_link_validate,
> 	.get_fwnode_pad = v4l2_subdev_get_fwnode_pad_1_to_1,
> };
> 
> /* -----------------------------------------------------------------------------
>  * Async subdev notifier
>  */
> 
> static struct csi_state *
> mipi_notifier_to_csis_state(struct v4l2_async_notifier *n)
> {
> 	return container_of(n, struct csi_state, notifier);
> }
> 
> static int mipi_csis_notify_bound(struct v4l2_async_notifier *notifier,
> 				  struct v4l2_subdev *sd,
> 				  struct v4l2_async_subdev *asd)
> {
> 	struct csi_state *state = mipi_notifier_to_csis_state(notifier);
> 	struct media_pad *sink = &state->sd.entity.pads[CSIS_PAD_SINK];
> 
> 	return v4l2_create_fwnode_links_to_pad(sd, sink, 0);
> }
> 
> static const struct v4l2_async_notifier_operations mipi_csis_notify_ops = {
> 	.bound = mipi_csis_notify_bound,
> };
> 
> static int mipi_csis_async_register(struct csi_state *state)
> {
> 	struct v4l2_fwnode_endpoint vep = {
> 		.bus_type = V4L2_MBUS_CSI2_DPHY,
> 	};
> 	struct v4l2_async_subdev *asd;
> 	struct fwnode_handle *ep;
> 	unsigned int i;
> 	int ret;
> 
> 	v4l2_async_notifier_init(&state->notifier);
> 
> 	ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(state->dev), 0, 0,
> 					     FWNODE_GRAPH_ENDPOINT_NEXT);
> 	if (!ep)
> 		return -ENOTCONN;
> 
> 	ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> 	if (ret)
> 		goto err_parse;
> 
> 	for (i = 0; i < vep.bus.mipi_csi2.num_data_lanes; ++i) {
> 		if (vep.bus.mipi_csi2.data_lanes[i] != i + 1) {
> 			dev_err(state->dev,
> 				"data lanes reordering is not supported");
> 			goto err_parse;
> 		}
> 	}
> 
> 	state->bus = vep.bus.mipi_csi2;
> 
> 	dev_dbg(state->dev, "data lanes: %d\n", state->bus.num_data_lanes);
> 	dev_dbg(state->dev, "flags: 0x%08x\n", state->bus.flags);
> 
> 	asd = v4l2_async_notifier_add_fwnode_remote_subdev(
> 		&state->notifier, ep, struct v4l2_async_subdev);
> 	if (IS_ERR(asd)) {
> 		ret = PTR_ERR(asd);
> 		goto err_parse;
> 	}
> 
> 	fwnode_handle_put(ep);
> 
> 	state->notifier.ops = &mipi_csis_notify_ops;
> 
> 	ret = v4l2_async_subdev_notifier_register(&state->sd, &state->notifier);
> 	if (ret)
> 		return ret;
> 
> 	return v4l2_async_register_subdev(&state->sd);
> 
> err_parse:
> 	fwnode_handle_put(ep);
> 
> 	return ret;
> }
> 
> /* -----------------------------------------------------------------------------
>  * Suspend/resume
>  */
> 
> static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
> {
> 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> 	struct csi_state *state = mipi_sd_to_csis_state(sd);
> 	int ret = 0;
> 
> 	mutex_lock(&state->lock);
> 	if (state->state & ST_POWERED) {
> 		mipi_csis_stop_stream(state);
> 		ret = mipi_csis_phy_disable(state);
> 		if (ret)
> 			goto unlock;
> 		mipi_csis_clk_disable(state);
> 		state->state &= ~ST_POWERED;
> 		if (!runtime)
> 			state->state |= ST_SUSPENDED;
> 	}
> 
> unlock:
> 	mutex_unlock(&state->lock);
> 
> 	return ret ? -EAGAIN : 0;
> }
> 
> static int mipi_csis_pm_resume(struct device *dev, bool runtime)
> {
> 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> 	struct csi_state *state = mipi_sd_to_csis_state(sd);
> 	int ret = 0;
> 
> 	mutex_lock(&state->lock);
> 	if (!runtime && !(state->state & ST_SUSPENDED))
> 		goto unlock;
> 
> 	if (!(state->state & ST_POWERED)) {
> 		ret = mipi_csis_phy_enable(state);
> 		if (ret)
> 			goto unlock;
> 
> 		state->state |= ST_POWERED;
> 		mipi_csis_clk_enable(state);
> 	}
> 	if (state->state & ST_STREAMING)
> 		mipi_csis_start_stream(state);
> 
> 	state->state &= ~ST_SUSPENDED;
> 
> unlock:
> 	mutex_unlock(&state->lock);
> 
> 	return ret ? -EAGAIN : 0;
> }
> 
> static int __maybe_unused mipi_csis_suspend(struct device *dev)
> {
> 	return mipi_csis_pm_suspend(dev, false);
> }
> 
> static int __maybe_unused mipi_csis_resume(struct device *dev)
> {
> 	return mipi_csis_pm_resume(dev, false);
> }
> 
> static int __maybe_unused mipi_csis_runtime_suspend(struct device *dev)
> {
> 	return mipi_csis_pm_suspend(dev, true);
> }
> 
> static int __maybe_unused mipi_csis_runtime_resume(struct device *dev)
> {
> 	return mipi_csis_pm_resume(dev, true);
> }
> 
> static const struct dev_pm_ops mipi_csis_pm_ops = {
> 	SET_RUNTIME_PM_OPS(mipi_csis_runtime_suspend, mipi_csis_runtime_resume,
> 			   NULL)
> 	SET_SYSTEM_SLEEP_PM_OPS(mipi_csis_suspend, mipi_csis_resume)
> };
> 
> /* -----------------------------------------------------------------------------
>  * Probe/remove & platform driver
>  */
> 
> static int mipi_csis_subdev_init(struct csi_state *state)
> {
> 	struct v4l2_subdev *sd = &state->sd;
> 
> 	v4l2_subdev_init(sd, &mipi_csis_subdev_ops);
> 	sd->owner = THIS_MODULE;
> 	snprintf(sd->name, sizeof(sd->name), "%s.%d",
> 		 CSIS_SUBDEV_NAME, state->index);
> 
> 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> 	sd->ctrl_handler = NULL;
> 
> 	sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> 	sd->entity.ops = &mipi_csis_entity_ops;
> 
> 	sd->dev = state->dev;
> 
> 	state->pads[CSIS_PAD_SINK].flags = MEDIA_PAD_FL_SINK
> 					 | MEDIA_PAD_FL_MUST_CONNECT;
> 	state->pads[CSIS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE
> 					   | MEDIA_PAD_FL_MUST_CONNECT;
> 	return media_entity_pads_init(&sd->entity, CSIS_PADS_NUM,
> 				      state->pads);
> }
> 
> static int mipi_csis_parse_dt(struct csi_state *state)
> {
> 	struct device_node *node = state->dev->of_node;
> 
> 	return 0;
> }
> 
> static int mipi_csis_probe(struct platform_device *pdev)
> {
> 	struct device *dev = &pdev->dev;
> 	struct csi_state *state;
> 	int ret;
> 
> 	state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> 	if (!state)
> 		return -ENOMEM;
> 
> 	mutex_init(&state->lock);
> 
> 	state->dev = dev;
> 
> 	/* Parse DT properties. */
> 	ret = mipi_csis_parse_dt(state);
> 	if (ret < 0) {
> 		dev_err(dev, "Failed to parse device tree: %d\n", ret);
> 		return ret;
> 	}
> 
> 	/* Acquire resources. */
> 	state->regs = devm_platform_ioremap_resource(pdev, 0);
> 	if (IS_ERR(state->regs))
> 		return PTR_ERR(state->regs);
> 
> 	ret = mipi_csis_phy_init(state);
> 	if (ret < 0)
> 		return ret;
> 
> 	ret = mipi_csis_clk_get(state);
> 	if (ret < 0)
> 		return ret;
> 
> 	/* Reset PHY and enable the clocks. */
> 	mipi_csis_phy_reset(state);
> 
> 	ret = mipi_csis_clk_enable(state);
> 	if (ret < 0) {
> 		dev_err(state->dev, "failed to enable clocks: %d\n", ret);
> 		return ret;
> 	}
> 
> 	/* Initialize and register the subdev. */
> 	ret = mipi_csis_subdev_init(state);
> 	if (ret < 0)
> 		goto disable_clock;
> 
> 	platform_set_drvdata(pdev, &state->sd);
> 
> 	ret = mipi_csis_async_register(state);
> 	if (ret < 0) {
> 		dev_err(dev, "async register failed: %d\n", ret);
> 		goto cleanup;
> 	}
> 
> 	/* Initialize debugfs. */
> 	mipi_csis_debugfs_init(state);
> 
> 	/* Enable runtime PM. */
> 	pm_runtime_enable(dev);
> 	if (!pm_runtime_enabled(dev)) {
> 		ret = mipi_csis_pm_resume(dev, true);
> 		if (ret < 0)
> 			goto unregister_all;
> 	}
> 
> 	dev_info(dev, "lanes: %d\n",
> 		 state->bus.num_data_lanes);
> 
> 	return 0;
> 
> unregister_all:
> 	mipi_csis_debugfs_exit(state);
> cleanup:
> 	media_entity_cleanup(&state->sd.entity);
> 	v4l2_async_notifier_unregister(&state->notifier);
> 	v4l2_async_notifier_cleanup(&state->notifier);
> 	v4l2_async_unregister_subdev(&state->sd);
> disable_clock:
> 	mipi_csis_clk_disable(state);
> 	mutex_destroy(&state->lock);
> 
> 	return ret;
> }
> 
> static int mipi_csis_remove(struct platform_device *pdev)
> {
> 	struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> 	struct csi_state *state = mipi_sd_to_csis_state(sd);
> 
> 	mipi_csis_debugfs_exit(state);
> 	v4l2_async_notifier_unregister(&state->notifier);
> 	v4l2_async_notifier_cleanup(&state->notifier);
> 	v4l2_async_unregister_subdev(&state->sd);
> 
> 	pm_runtime_disable(&pdev->dev);
> 	mipi_csis_pm_suspend(&pdev->dev, true);
> 	mipi_csis_clk_disable(state);
> 	media_entity_cleanup(&state->sd.entity);
> 	mutex_destroy(&state->lock);
> 	pm_runtime_set_suspended(&pdev->dev);
> 
> 	return 0;
> }
> 
> static const struct of_device_id mipi_csis_of_match[] = {
> 	{ .compatible = "fsl,imx8mq-mipi-csi2",},
> 	{ /* sentinel */ },
> };
> MODULE_DEVICE_TABLE(of, mipi_csis_of_match);
> 
> static struct platform_driver mipi_csis_driver = {
> 	.probe		= mipi_csis_probe,
> 	.remove		= mipi_csis_remove,
> 	.driver		= {
> 		.of_match_table = mipi_csis_of_match,
> 		.name		= CSIS_DRIVER_NAME,
> 		.pm		= &mipi_csis_pm_ops,
> 	},
> };
> 
> module_platform_driver(mipi_csis_driver);
> 
> MODULE_DESCRIPTION("i.MX8MQ MIPI CSI-2 receiver driver");
> MODULE_LICENSE("GPL v2");
> MODULE_ALIAS("platform:imx8mq-mipi-csi2");

Martin Kepplinger May 19, 2021, 9:33 a.m. UTC | #11

Am Mittwoch, dem 19.05.2021 um 04:14 +0300 schrieb Laurent Pinchart:
> Hi Martin,
> 
> On Tue, May 18, 2021 at 04:39:00PM +0200, Martin Kepplinger wrote:
> > Am Sonntag, dem 16.05.2021 um 01:55 +0300 schrieb Laurent Pinchart:
> > > On Tue, May 04, 2021 at 05:59:39PM +0200, Martin Kepplinger
> > > wrote:
> > > > hi Laurent, again thanks a lot for posting this series! I can't
> > > > fully test
> > > > it, but base my work for imx8mq on it now. imx8mq includes
> > > > yet another mipi phy version than this and below is some very
> > > > rough testing
> > > > code. it's not at all something I sign-off on but my following
> > > > problem is based on it.
> > > 
> > > Unless I'm mistaken, the CSI-2 receiver in the i.MX8MQ is a
> > > completely
> > > different device. I wouldn't try to support it in the imx7-mipi-
> > > csis
> > > driver, but in a separate driver.
> > > 
> > > >  * configured to use both staging csi drivers
> > > >  * the csi bridge driver at least streams frames together with
> > > > the
> > > > nxp "yav" mipi driver
> > > > 
> > > > media-ctl -p now says the output below, so one link from mipi
> > > > to
> > > > csi is missing.
> > > > 
> > > > Note that
> > > > 
> > > > media-ctl --set-v4l2 "'csi':0 [fmt:SBGGR10/640x480]"
> > > > works in that it changes the configured format below, but
> > > > 
> > > > media-ctl -l "'imx7-mipi-csis.0':1" -> "'csi':0[1]"
> > > > doesn't create said missing link.
> > > 
> > > media-ctl can't create links, it can only enable or disable them.
> > > Link
> > > creation is the prerogative of drivers.
> > > 
> > > > Do I maybe use that wrongly? If now, does anything come to mind
> > > > that would
> > > > be missing specifically?
> > > 
> > > The link should be created by the call to media_create_pad_link()
> > > in
> > > imx_media_capture_device_register(). You'll need to figure out if
> > > the
> > > function is called and returns an error early, or if it doesn't
> > > get
> > > called at all, and why.
> > > 
> > > > When trying to stream anyway (if that makes sense), I get the
> > > > following:
> > > > 
> > > > [ 2008.377470] capture_start_streaming: starting
> > > > [ 2008.381883] capture_find_format: calling
> > > > imx_media_find_mbus_format with code 0x2006
> > > > [ 2008.389671] imx7-csi 30a90000.csi1_bridge:
> > > > capture_validate_fmt: capture_find_format err
> > > > [ 2008.397794] imx7-csi 30a90000.csi1_bridge:
> > > > capture_validate_fmt: capture_find_format found colorspace 0x1
> > > > != 0x0
> > > > [ 2008.407999] imx7-csi 30a90000.csi1_bridge: capture format
> > > > not valid: -32
> > > > 
> > > > and if I ignore that (because I'm not yet sure whether that is
> > > > specific to
> > > > platforms including an IPU), I get a WARN_ON from
> > > > vb2_start_streaming()
> > > 
> > > That I have a fix for, I'll post it as part of an imx7-media-csi
> > > series.
> > 
> > Hi Laurent,
> > 
> > You haven't posted that fix you're talking about, right?
> 
> Correct. It's now fixed (see "[PATCH] media: imx: imx7-media-csi: Fix
> buffer return upon stream start failure", I've CC'ed you).
> 
> > The below
> > driver (attached; I'll send it as patches after I successfully
> > tested
> > myself, and cleanup and fixes obviously)
> 
> Don't forget the DT bindings at that point :-)

I won't forget it and try to avoid them after testing some
functionality.

> 
> > results in the same situation I described above:
> > 
> > * missing link from mipi (entity 10) -> csi (entity 1):
> 
> The link is supposed to be created by
> v4l2_create_fwnode_links_to_pad(),
> called from imx7_csi_notify_bound(). Could you trace the calls and
> figure out what goes wrong ?

will do.

> 
> > ------------------------------------------------------
> > 
> > Device topology
> > - entity 1: csi (2 pads, 1 link)
> >             type V4L2 subdev subtype Unknown flags 0
> >             device node name /dev/v4l-subdev0
> >         pad0: Sink
> >                 [fmt:UYVY8_2X8/640x480 field:none colorspace:srgb
> > xfer:srgb ycbcr:601 quantization:lim-range]
> >         pad1: Source
> >                 [fmt:UYVY8_2X8/640x480 field:none colorspace:srgb
> > xfer:srgb ycbcr:601 quantization:lim-range]
> >                 -> "csi capture":0 [ENABLED,IMMUTABLE]
> > 
> > - entity 4: csi capture (1 pad, 1 link)
> >             type Node subtype V4L flags 0
> >             device node name /dev/video0
> >         pad0: Sink
> >                 <- "csi":1 [ENABLED,IMMUTABLE]
> > 
> > - entity 10: imx8mq-mipi-csis.0 (2 pads, 1 link)
> >              type V4L2 subdev subtype Unknown flags 0
> >              device node name /dev/v4l-subdev1
> >         pad0: Sink
> >                 <- "hi846 2-0020":0 []
> >         pad1: Source
> > 
> > - entity 13: hi846 2-0020 (1 pad, 1 link)
> >              type V4L2 subdev subtype Sensor flags 0
> >              device node name /dev/v4l-subdev2
> >         pad0: Source
> >                 [fmt:SGBRG10_1X10/640x480 field:none
> > colorspace:raw]
> >                 -> "imx8mq-mipi-csis.0":0 []
> > 
> > 
> > * and the mentioned vb2 WARN_ON:
> > --------------------------------
> > 
> > [   56.120834] imx7-csi 30a90000.csi1_bridge: begin graph walk at
> > 'csi capture'
> > [   56.120859] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi'
> > on stack
> > [   56.120865] imx7-csi 30a90000.csi1_bridge: walk: skipping entity
> > 'csi capture' (already seen)
> > [   56.120871] imx7-csi 30a90000.csi1_bridge: walk: returning
> > entity 'csi'
> > [   56.120877] imx7-csi 30a90000.csi1_bridge: walk: returning
> > entity 'csi capture'
> > [   56.127415] vb2_common_vm_open: 000000006622b5ef, refcount: 1,
> > vma: ffffabe0b000-ffffabea1000
> > [   56.127438] vb2_dc_mmap: mapped dma addr 0xe8100000 at
> > 0xffffabe0b000, size 614400
> > [   56.127480] vb2_common_vm_open: 00000000e689fd4f, refcount: 1,
> > vma: ffffabd75000-ffffabe0b000
> > [   56.127488] vb2_dc_mmap: mapped dma addr 0xe8200000 at
> > 0xffffabd75000, size 614400
> > [   56.127501] vb2_common_vm_open: 00000000485fa30a, refcount: 1,
> > vma: ffffabcdf000-ffffabd75000
> > [   56.127509] vb2_dc_mmap: mapped dma addr 0xe8300000 at
> > 0xffffabcdf000, size 614400
> > [   56.127522] vb2_common_vm_open: 0000000092607c6a, refcount: 1,
> > vma: ffffabc49000-ffffabcdf000
> > [   56.127529] vb2_dc_mmap: mapped dma addr 0xe8400000 at
> > 0xffffabc49000, size 614400
> > [   56.127579] imx7-csi 30a90000.csi1_bridge: begin graph walk at
> > 'csi'
> > [   56.127587] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi
> > capture' on stack
> > [   56.127593] imx7-csi 30a90000.csi1_bridge: walk: skipping entity
> > 'csi' (already seen)
> > [   56.127599] imx7-csi 30a90000.csi1_bridge: walk: returning
> > entity 'csi capture'
> > [   56.127604] imx7-csi 30a90000.csi1_bridge: walk: returning
> > entity 'csi'
> > [   56.128102] imx7-csi 30a90000.csi1_bridge: begin graph walk at
> > 'csi'
> > [   56.128111] imx7-csi 30a90000.csi1_bridge: walk: pushing 'csi
> > capture' on stack
> > [   56.128117] imx7-csi 30a90000.csi1_bridge: walk: skipping entity
> > 'csi' (already seen)
> > [   56.128122] imx7-csi 30a90000.csi1_bridge: walk: returning
> > entity 'csi capture'
> > [   56.128127] imx7-csi 30a90000.csi1_bridge: walk: returning
> > entity 'csi'
> > [   56.128133] imx7-csi 30a90000.csi1_bridge: pipeline start failed
> > with -19
> > [   56.135091] ------------[ cut here ]------------
> > [   56.135102] WARNING: CPU: 3 PID: 1984 at
> > drivers/media/common/videobuf2/videobuf2-core.c:1568
> > vb2_start_streaming+0xe4/0x160 [videobuf2_common]
> > [   56.135151] Modules linked in: aes_ce_ccm exfat rfcomm
> > algif_hash algif_skcipher af_alg bnep qmi_wwan cdc_wdm option
> > usbnet usb_wwan usbserial mii ofpart mousedev spi_nor caam_jr mtd
> > caamhash_desc caamalg_desc crypto_engine uas redpine_sdio
> > usb_storage redpine_91x bluetooth mac80211 aes_ce_blk crypto_simd
> > crct10dif_ce ghash_ce cfg80211 sha2_ce sha1_ce st_lsm6dsx_spi
> > bq25890_charger pwm_vibra snd_soc_gtm601 snd_soc_simple_card
> > snd_soc_simple_card_utils hi846 s5k3l6xx edt_ft5x06 snd_soc_wm8962
> > mx6s_capture imx7_media_csi(C) imx_media_common(C)
> > videobuf2_dma_contig imx8mq_mipi_csis(C) mxc_mipi_csi2_yav
> > videobuf2_memops videobuf2_v4l2 tps6598x videobuf2_common vcnl4000
> > v4l2_fwnode typec industrialio_triggered_buffer leds_lm3560
> > videodev mc st_lsm6dsx_i2c st_lsm6dsx kfifo_buf gnss_mtk
> > gnss_serial gnss snd_soc_fsl_sai imx_sdma snvs_pwrkey imx_pcm_dma
> > virt_dma snd_soc_core imx2_wdt watchdog snd_pcm_dmaengine snd_pcm
> > snd_timer snd caam soundcore error rfkill_hks rfkill ledtrig_timer
> > usb_f_acm
> > [   56.135494]  u_serial usb_f_rndis g_multi usb_f_mass_storage
> > u_ether libcomposite ledtrig_pattern fuse ip_tables x_tables ipv6
> > xhci_plat_hcd xhci_hcd usbcore imx_dcss clk_bd718x7 cdns_mhdp_imx
> > cdns_mhdp_drmcore dwc3 ulpi udc_core roles phy_fsl_imx8mq_usb
> > usb_common
> > [   56.135596] CPU: 3 PID: 1984 Comm: v4l2-ctl Tainted: G        
> > C   5.12.2-librem5-00049-g99f86eccfeae #335
> > [   56.135607] Hardware name: Purism Librem 5r4 (DT)
> > [   56.135613] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO BTYPE=--)
> > [   56.135623] pc : vb2_start_streaming+0xe4/0x160
> > [videobuf2_common]
> > [   56.135653] lr : vb2_start_streaming+0x74/0x160
> > [videobuf2_common]
> > [   56.135682] sp : ffff8000148bbba0
> > [   56.135686] x29: ffff8000148bbba0 x28: ffff00001e833f00 
> > [   56.135700] x27: 0000000040045612 x26: ffff800008f406a0 
> > [   56.135713] x25: 0000000000000000 x24: ffff8000148bbd58 
> > [   56.135725] x23: ffff0000be730138 x22: ffff00000230ab00 
> > [   56.135738] x21: ffff0000be730330 x20: ffff0000be730348 
> > [   56.135751] x19: 00000000ffffffed x18: 0000000000000000 
> > [   56.135763] x17: 0000000000000000 x16: 0000000000000000 
> > [   56.135776] x15: 0000000000000030 x14: ffffffffffffffff 
> > [   56.135788] x13: ffff8000948bb737 x12: ffff8000148bb73f 
> > [   56.135801] x11: ffff80001152a7a0 x10: 00000000ffffe000 
> > [   56.135813] x9 : ffff800008f3c900 x8 : ffff80001147a7a0 
> > [   56.135826] x7 : ffff80001152a7a0 x6 : 0000000000000000 
> > [   56.135838] x5 : 0000000000000000 x4 : 0000000000000000 
> > [   56.135850] x3 : ffff0000be730344 x2 : 0000000000000000 
> > [   56.135863] x1 : ffff800008fe4000 x0 : ffff0000253d29f0 
> > [   56.135877] Call trace:
> > [   56.135882]  vb2_start_streaming+0xe4/0x160 [videobuf2_common]
> > [   56.135912]  vb2_core_streamon+0x9c/0x1a0 [videobuf2_common]
> > [   56.135940]  vb2_ioctl_streamon+0x68/0xbc [videobuf2_v4l2]
> > [   56.135964]  v4l_streamon+0x30/0x40 [videodev]
> > [   56.136063]  __video_do_ioctl+0x194/0x3f4 [videodev]
> > [   56.136145]  video_usercopy+0x1a4/0x770 [videodev]
> > [   56.136226]  video_ioctl2+0x24/0x40 [videodev]
> > [   56.136305]  v4l2_ioctl+0x4c/0x70 [videodev]
> > [   56.136385]  __arm64_sys_ioctl+0xb4/0xfc
> > [   56.136401]  el0_svc_common.constprop.0+0x68/0x130
> > [   56.136416]  do_el0_svc+0x28/0x34
> > [   56.136426]  el0_svc+0x2c/0x54
> > [   56.136438]  el0_sync_handler+0x1a4/0x1b0
> > [   56.136449]  el0_sync+0x174/0x180
> > [   56.136459] ---[ end trace 122c8abc5f14e4e5 ]---
> 
> Hopefully the patch mentioned above will fix this.

it does. no WARN_ON after "pipeline start failed with -19"

thank you!

> 
> > // SPDX-License-Identifier: GPL-2.0
> > /*
> >  * Freescale i.MX8MQ SoC series MIPI-CSI receiver driver
> >  *
> >  * Copyright (C) 2021 Purism SPC
> >  * Copyright (C) 2019 Linaro Ltd
> >  * Copyright (C) 2015-2016 Freescale Semiconductor, Inc. All Rights
> > Reserved.
> >  * Copyright (C) 2011 - 2013 Samsung Electronics Co., Ltd.
> >  *
> >  */
> > 
> > #include <linux/clk.h>
> > #include <linux/debugfs.h>
> > #include <linux/delay.h>
> > #include <linux/errno.h>
> > #include <linux/interrupt.h>
> > #include <linux/io.h>
> > #include <linux/kernel.h>
> > #include <linux/module.h>
> > #include <linux/mutex.h>
> > #include <linux/of.h>
> > #include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > #include <linux/pm_runtime.h>
> > #include <linux/regmap.h>
> > #include <linux/mfd/syscon.h>
> > #include <linux/regulator/consumer.h>
> > #include <linux/reset.h>
> > #include <linux/spinlock.h>
> > 
> > #include <media/v4l2-common.h>
> > #include <media/v4l2-device.h>
> > #include <media/v4l2-fwnode.h>
> > #include <media/v4l2-mc.h>
> > #include <media/v4l2-subdev.h>
> > 
> > #define CSIS_DRIVER_NAME                        "imx8mq-mipi-csis"
> > #define CSIS_SUBDEV_NAME                        CSIS_DRIVER_NAME
> > 
> > #define CSIS_PAD_SINK                           0
> > #define CSIS_PAD_SOURCE                         1
> > #define CSIS_PADS_NUM                           2
> > 
> > #define MIPI_CSIS_DEF_PIX_WIDTH                 640
> > #define MIPI_CSIS_DEF_PIX_HEIGHT                480
> > 
> > /* Register map definition */
> > 
> > /* i.MX8MQ CSI-2 controller CSR */
> > /* TODO 0x100, to dts? */
> > #define CSI2RX_CFG_NUM_LANES                    0x100
> > #define CSI2RX_CFG_DISABLE_DATA_LANES           0x104
> > #define CSI2RX_BIT_ERR                          0x108
> > #define CSI2RX_IRQ_STATUS                       0x10C
> > #define CSI2RX_IRQ_MASK                         0x110
> > #define CSI2RX_ULPS_STATUS                      0x114
> > #define CSI2RX_PPI_ERRSOT_HS                    0x118
> > #define CSI2RX_PPI_ERRSOTSYNC_HS                0x11C
> > #define CSI2RX_PPI_ERRESC                       0x120
> > #define CSI2RX_PPI_ERRSYNCESC                   0x124
> > #define CSI2RX_PPI_ERRCONTROL                   0x128
> > #define CSI2RX_CFG_DISABLE_PAYLOAD_0            0x12C
> > #define CSI2RX_CFG_DISABLE_PAYLOAD_1            0x130
> > 
> > enum {
> >         ST_POWERED      = 1,
> >         ST_STREAMING    = 2,
> >         ST_SUSPENDED    = 4,
> > };
> > 
> > static const char * const mipi_csis_clk_id[] = {
> >         "clk_core",
> >         "clk_esc",
> >         "clk_pxl",
> >         "clk_clko2",
> > };
> > 
> > struct csis_imx8mq_hw_reset {
> >         struct regmap *src;
> >         u8 req_src;
> >         u8 rst_val;
> > };
> > 
> > struct csis_imx8mq_phy_gpr {
> >         struct regmap *gpr;
> >         u8 req_src;
> > };
> > 
> > #define GPR_CSI2_1_RX_ENABLE            BIT(13)
> > #define GPR_CSI2_1_VID_INTFC_ENB        BIT(12)
> > #define GPR_CSI2_1_HSEL                 BIT(10)
> > #define GPR_CSI2_1_CONT_CLK_MODE        BIT(8)
> > #define GPR_CSI2_1_S_PRG_RXHS_SETTLE(x) (((x) & 0x3F) << 2)
> > /*
> >  * rxhs_settle[0] ... <720x480
> >  * rxhs_settle[1] ... >720*480
> >  *
> >  * 
> > https://community.nxp.com/t5/i-MX-Processors/Explenation-for-HS-SETTLE-parameter-in-MIPI-CSI-D-PHY-registers/m-p/764275/highlight/true#M118744
> >  */
> > static u8 rxhs_settle[2] = { 0x14, 0x9 };
> > 
> > struct csi_state {
> >         struct device *dev;
> >         void __iomem *regs;
> >         struct clk_bulk_data *clks;
> >         struct reset_control *mrst;
> >         struct regulator *mipi_phy_regulator;
> >         u8 index;
> > 
> >         struct v4l2_subdev sd;
> >         struct media_pad pads[CSIS_PADS_NUM];
> >         struct v4l2_async_notifier notifier;
> >         struct v4l2_subdev *src_sd;
> > 
> >         struct v4l2_fwnode_bus_mipi_csi2 bus;
> >         u32 hs_settle;
> >         u32 clk_settle;
> > 
> >         struct mutex lock;      /* Protect csis_fmt, format_mbus
> > and state */
> >         u32 state;
> > 
> >         struct dentry *debugfs_root;
> >         bool debug;
> > 
> >         struct csis_imx8mq_hw_reset hw_reset;
> >         struct csis_imx8mq_phy_gpr phy_gpr;
> >         u32 send_level;
> > };
> > 
> > /* ----------------------------------------------------------------
> > -------------
> >  * Format helpers
> >  */
> > 
> > /* ----------------------------------------------------------------
> > -------------
> >  * Hardware configuration
> >  */
> > 
> > static inline u32 mipi_csis_read(struct csi_state *state, u32 reg)
> > {
> >         return readl(state->regs + reg);
> > }
> > 
> > static inline void mipi_csis_write(struct csi_state *state, u32
> > reg, u32 val)
> > {
> >         writel(val, state->regs + reg);
> > }
> > 
> > static void mipi_csis_enable_interrupts(struct csi_state *state,
> > bool on)
> > {
> >         return;
> > }
> > 
> > static void mipi_csis_sw_reset(struct csi_state *state)
> > {
> >         /* TODO yav: mxc_mipi_csi1_phy_reset */
> > 
> >         struct device *dev = state->dev;
> >         struct device_node *np = dev->of_node;
> >         struct device_node *node;
> >         phandle phandle;
> >         u32 out_val[3];
> >         int ret;
> > 
> >         dev_dbg(dev, "%s: starting\n", __func__);
> > 
> >         ret = of_property_read_u32_array(np, "csis-phy-reset",
> > out_val, 3);
> >         if (ret) {
> >                 dev_info(dev, "no csis-hw-reset property found:
> > %d\n", ret);
> >                 return;
> >         }
> > 
> >         phandle = *out_val;
> > 
> >         node = of_find_node_by_phandle(phandle);
> >         if (!node) {
> >                 ret = PTR_ERR(node);
> >                 dev_dbg(dev, "not find src node by phandle: %d\n",
> > ret);
> >         }
> >         state->hw_reset.src = syscon_node_to_regmap(node);
> >         if (IS_ERR(state->hw_reset.src)) {
> >                 ret = PTR_ERR(state->hw_reset.src);
> >                 dev_err(dev, "failed to get src regmap: %d\n",
> > ret);
> >         }
> >         of_node_put(node);
> >         if (ret < 0)
> >                 return;
> > 
> >         state->hw_reset.req_src = out_val[1];
> >         state->hw_reset.rst_val = out_val[2];
> > 
> >         /* reset imx8mq mipi phy */
> >         regmap_update_bits(state->hw_reset.src,
> >                            state->hw_reset.req_src,
> >                            state->hw_reset.rst_val,
> >                            state->hw_reset.rst_val);
> >         msleep(20);
> > 
> >         dev_dbg(dev, "%s: done\n", __func__);
> > 
> >         return;
> > }
> > 
> > static void mipi_csis_system_enable(struct csi_state *state, int
> > on)
> > {
> >         struct device *dev = state->dev;
> >         struct device_node *np = dev->of_node;
> >         struct device_node *node;
> >         phandle phandle;
> >         u32 out_val[2];
> >         int ret;
> > 
> >         if (!on) {
> >                 /* Disable Data lanes */
> >                 mipi_csis_write(state,
> > CSI2RX_CFG_DISABLE_DATA_LANES, 0xf);
> >                 return;
> >         }
> > 
> >         ret = of_property_read_u32_array(np, "phy-gpr", out_val,
> > 2);
> >         if (ret) {
> >                 dev_info(dev, "no phy-gpr property found\n");
> >                 return;
> >         }
> > 
> >         phandle = *out_val;
> > 
> >         node = of_find_node_by_phandle(phandle);
> >         if (!node) {
> >                 dev_dbg(dev, "not find gpr node by phandle\n");
> >                 ret = PTR_ERR(node);
> >         }
> >         state->phy_gpr.gpr = syscon_node_to_regmap(node);
> >         if (IS_ERR(state->phy_gpr.gpr)) {
> >                 dev_err(dev, "failed to get gpr regmap\n");
> >                 ret = PTR_ERR(state->phy_gpr.gpr);
> >         }
> >         of_node_put(node);
> >         if (ret < 0)
> >                 return;
> > 
> >         state->phy_gpr.req_src = out_val[1];
> > 
> >         regmap_update_bits(state->phy_gpr.gpr,
> >                            state->phy_gpr.req_src,
> >                            0x3FFF,
> >                            GPR_CSI2_1_RX_ENABLE |
> >                            GPR_CSI2_1_VID_INTFC_ENB |
> >                            GPR_CSI2_1_HSEL |
> >                            GPR_CSI2_1_CONT_CLK_MODE |
> >                            GPR_CSI2_1_S_PRG_RXHS_SETTLE(state->
> >                                                         hs_settle))
> > ;
> > 
> >         dev_dbg(dev, "%s: hs_settle: 0x%X\n", __func__, state-
> > >hs_settle);
> > 
> >         return;
> > }
> > 
> > static int mipi_csis_calculate_params(struct csi_state *state)
> > {
> >         s64 link_freq;
> >         u32 lane_rate;
> > 
> >         state->hs_settle = rxhs_settle[0];
> > #if 0
> >         /* Calculate the line rate from the pixel rate. */
> >         link_freq = v4l2_get_link_freq(state->src_sd->ctrl_handler,
> >                                        state->csis_fmt->width,
> >                                        state->bus.num_data_lanes *
> > 2);
> >         if (link_freq < 0) {
> >                 dev_err(state->dev, "Unable to obtain link
> > frequency: %d\n",
> >                         (int)link_freq);
> >                 return link_freq;
> >         }
> > 
> >         lane_rate = link_freq * 2;
> > 
> >         if (lane_rate < 80000000 || lane_rate > 1500000000) {
> >                 dev_dbg(state->dev, "Out-of-bound lane rate %u\n",
> > lane_rate);
> >                 return -EINVAL;
> >         }
> > 
> >         /*
> >          * The HSSETTLE counter value is document in a table, but
> > can also
> >          * easily be calculated. Hardcode the CLKSETTLE value to 0
> > for now
> >          * (which is documented as corresponding to CSI-2 v0.87 to
> > v1.00) until
> >          * we figure out how to compute it correctly.
> >          */
> >         state->hs_settle = (lane_rate - 5000000) / 45000000;
> >         state->clk_settle = 0;
> > 
> >         dev_dbg(state->dev, "lane rate %u, Tclk_settle %u,
> > Ths_settle %u\n",
> >                 lane_rate, state->clk_settle, state->hs_settle);
> > #endif
> >         return 0;
> > }
> > 
> > static void mipi_csis_set_params(struct csi_state *state)
> > {
> >         int lanes = state->bus.num_data_lanes;
> >         u32 val = 0;
> >         int i;
> > 
> >         /* Lanes */
> >         mipi_csis_write(state, CSI2RX_CFG_NUM_LANES, lanes - 1);
> > 
> > dev_err(state->dev, "imx8mq: %d lanes\n", lanes);
> > 
> >         for (i = 0; i < lanes; i++)
> >                 val |= (1 << i);
> > 
> >         val = 0xF & ~val;
> >         mipi_csis_write(state, CSI2RX_CFG_DISABLE_DATA_LANES, val);
> > 
> > dev_err(state->dev, "imx8mq: CSI2RX_CFG_DISABLE_DATA_LANES:
> > 0x%X\n", val);
> > 
> >         /* Mask interrupt */
> >         // Don't let ULPS (ultra-low power status) interrupts flood
> >         mipi_csis_write(state, CSI2RX_IRQ_MASK, 0x1ff);
> > 
> >         mipi_csis_write(state, 0x180, 1);
> >         /* vid_vc */
> >         mipi_csis_write(state, 0x184, 1);
> >         mipi_csis_write(state, 0x188, state->send_level);
> > }
> > 
> > static int mipi_csis_clk_enable(struct csi_state *state)
> > {
> >         return
> > clk_bulk_prepare_enable(ARRAY_SIZE(mipi_csis_clk_id), state->clks);
> > }
> > 
> > static void mipi_csis_clk_disable(struct csi_state *state)
> > {
> >         clk_bulk_disable_unprepare(ARRAY_SIZE(mipi_csis_clk_id),
> > state->clks);
> > }
> > 
> > static int mipi_csis_clk_get(struct csi_state *state)
> > {
> >         unsigned int i;
> >         int ret;
> > 
> >         state->clks = devm_kcalloc(state->dev,
> > ARRAY_SIZE(mipi_csis_clk_id),
> >                                    sizeof(*state->clks),
> > GFP_KERNEL);
> > 
> >         if (!state->clks)
> >                 return -ENOMEM;
> > 
> >         for (i = 0; i < ARRAY_SIZE(mipi_csis_clk_id); i++)
> >                 state->clks[i].id = mipi_csis_clk_id[i];
> > 
> >         ret = devm_clk_bulk_get(state->dev,
> > ARRAY_SIZE(mipi_csis_clk_id),
> >                                 state->clks);
> >         return ret;
> > }
> > 
> > static void mipi_csis_start_stream(struct csi_state *state)
> > {
> >         mipi_csis_sw_reset(state);
> >         mipi_csis_set_params(state);
> >         mipi_csis_system_enable(state, true);
> >         mipi_csis_enable_interrupts(state, true);
> > }
> > 
> > static void mipi_csis_stop_stream(struct csi_state *state)
> > {
> >         mipi_csis_enable_interrupts(state, false);
> >         mipi_csis_system_enable(state, false);
> > }
> > 
> > /* ----------------------------------------------------------------
> > -------------
> >  * PHY regulator and reset
> >  */
> > 
> > static int mipi_csis_phy_enable(struct csi_state *state)
> > {
> >         return 0;
> > }
> > 
> > static int mipi_csis_phy_disable(struct csi_state *state)
> > {
> >         return 0;
> > }
> > 
> > static void mipi_csis_phy_reset(struct csi_state *state)
> > {
> >         return;
> > }
> > 
> > static int mipi_csis_phy_init(struct csi_state *state)
> > {
> >         return 0;
> > }
> > 
> > /* ----------------------------------------------------------------
> > -------------
> >  * Debug
> >  */
> > 
> > static void mipi_csis_clear_counters(struct csi_state *state)
> > {
> >         return;
> > }
> > 
> > static void mipi_csis_log_counters(struct csi_state *state, bool
> > non_errors)
> > {
> >         return;
> > }
> > 
> > static int mipi_csis_dump_regs(struct csi_state *state)
> > {
> >         return 0;
> > }
> > 
> > static int mipi_csis_dump_regs_show(struct seq_file *m, void
> > *private)
> > {
> >         struct csi_state *state = m->private;
> > 
> >         return mipi_csis_dump_regs(state);
> > }
> > DEFINE_SHOW_ATTRIBUTE(mipi_csis_dump_regs);
> > 
> > static void mipi_csis_debugfs_init(struct csi_state *state)
> > {
> >         state->debugfs_root = debugfs_create_dir(dev_name(state-
> > >dev), NULL);
> > 
> >         debugfs_create_bool("debug_enable", 0600, state-
> > >debugfs_root,
> >                             &state->debug);
> >         debugfs_create_file("dump_regs", 0600, state->debugfs_root,
> > state,
> >                             &mipi_csis_dump_regs_fops);
> > }
> > 
> > static void mipi_csis_debugfs_exit(struct csi_state *state)
> > {
> >         debugfs_remove_recursive(state->debugfs_root);
> > }
> > 
> > /* ----------------------------------------------------------------
> > -------------
> >  * V4L2 subdev operations
> >  */
> > 
> > static struct csi_state *mipi_sd_to_csis_state(struct v4l2_subdev
> > *sdev)
> > {
> >         return container_of(sdev, struct csi_state, sd);
> > }
> > 
> > static int mipi_csis_s_stream(struct v4l2_subdev *sd, int enable)
> > {
> >         struct csi_state *state = mipi_sd_to_csis_state(sd);
> >         int ret;
> > 
> >         mipi_csis_write(state, CSI2RX_IRQ_MASK, 0x008);
> > 
> >         dev_dbg(state->dev, "%s: enable: %d\n", __func__, enable);
> > 
> >         if (enable) {
> >                 ret = mipi_csis_calculate_params(state);
> >                 if (ret < 0)
> >                         return ret;
> > 
> >                 mipi_csis_clear_counters(state);
> > 
> >                 ret = pm_runtime_get_sync(state->dev);
> >                 if (ret < 0) {
> >                         pm_runtime_put_noidle(state->dev);
> >                         return ret;
> >                 }
> >                 ret = v4l2_subdev_call(state->src_sd, core,
> > s_power, 1);
> >                 if (ret < 0 && ret != -ENOIOCTLCMD)
> >                         goto done;
> >         }
> > 
> >         mutex_lock(&state->lock);
> > 
> >         if (enable) {
> >                 if (state->state & ST_SUSPENDED) {
> >                         ret = -EBUSY;
> >                         goto unlock;
> >                 }
> > 
> >                 mipi_csis_start_stream(state);
> >                 ret = v4l2_subdev_call(state->src_sd, video,
> > s_stream, 1);
> >                 if (ret < 0)
> >                         goto unlock;
> > 
> >                 mipi_csis_log_counters(state, true);
> > 
> >                 state->state |= ST_STREAMING;
> >         } else {
> >                 v4l2_subdev_call(state->src_sd, video, s_stream,
> > 0);
> >                 ret = v4l2_subdev_call(state->src_sd, core,
> > s_power, 0);
> >                 if (ret == -ENOIOCTLCMD)
> >                         ret = 0;
> >                 mipi_csis_stop_stream(state);
> >                 state->state &= ~ST_STREAMING;
> >                 if (state->debug)
> >                         mipi_csis_log_counters(state, true);
> >         }
> > 
> > unlock:
> >         mutex_unlock(&state->lock);
> > 
> > done:
> >         if (!enable || ret < 0)
> >                 pm_runtime_put(state->dev);
> > 
> >         return ret;
> > }
> > 
> > static int mipi_csis_get_fmt(struct v4l2_subdev *sd,
> >                              struct v4l2_subdev_pad_config *cfg,
> >                              struct v4l2_subdev_format *sdformat)
> > {
> >         struct csi_state *state = mipi_sd_to_csis_state(sd);
> > 
> >         return v4l2_subdev_call(state->src_sd, pad, get_fmt, NULL,
> > sdformat);
> > }
> > 
> > static int mipi_csis_enum_mbus_code(struct v4l2_subdev *sd,
> >                                     struct v4l2_subdev_pad_config
> > *cfg,
> >                                     struct
> > v4l2_subdev_mbus_code_enum *code)
> > {
> >         struct csi_state *state = mipi_sd_to_csis_state(sd);
> > 
> >         return v4l2_subdev_call(state->src_sd, pad, enum_mbus_code,
> > NULL, code);
> > }
> > 
> > static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
> >                              struct v4l2_subdev_pad_config *cfg,
> >                              struct v4l2_subdev_format *sdformat)
> > {
> >         struct csi_state *state = mipi_sd_to_csis_state(sd);
> > 
> >         /*
> >          * The CSIS can't transcode in any way, the source format
> > can't be
> >          * modified.
> >          */
> >         if (sdformat->pad == CSIS_PAD_SOURCE)
> >                 return mipi_csis_get_fmt(sd, cfg, sdformat);
> > 
> >         if (sdformat->pad != CSIS_PAD_SINK)
> >                 return -EINVAL;
> > 
> >         if (sdformat->format.width * sdformat->format.height > 720
> > * 480) {
> >                 state->hs_settle = rxhs_settle[1];
> >         } else {
> >                 state->hs_settle = rxhs_settle[0];
> >         }
> >         state->send_level = 64;
> > 
> >         dev_dbg(state->dev,
> >                 "%s: format %dx%d send_level %d hs_settle 0x%X\n",
> > __func__,
> >                 sdformat->format.width, sdformat->format.height,
> >                 state->send_level, state->hs_settle);
> > 
> >         return v4l2_subdev_call(state->src_sd, pad, set_fmt, NULL,
> > sdformat);
> > }
> > 
> > static int mipi_csis_log_status(struct v4l2_subdev *sd)
> > {
> >         struct csi_state *state = mipi_sd_to_csis_state(sd);
> > 
> >         mutex_lock(&state->lock);
> >         mipi_csis_log_counters(state, true);
> >         if (state->debug && (state->state & ST_POWERED))
> >                 mipi_csis_dump_regs(state);
> >         mutex_unlock(&state->lock);
> > 
> >         return 0;
> > }
> > 
> > static const struct v4l2_subdev_core_ops mipi_csis_core_ops = {
> >         .log_status     = mipi_csis_log_status,
> > };
> > 
> > static const struct v4l2_subdev_video_ops mipi_csis_video_ops = {
> >         .s_stream       = mipi_csis_s_stream,
> > };
> > 
> > static const struct v4l2_subdev_pad_ops mipi_csis_pad_ops = {
> >         .enum_mbus_code         = mipi_csis_enum_mbus_code,
> >         .get_fmt                = mipi_csis_get_fmt,
> >         .set_fmt                = mipi_csis_set_fmt,
> > };
> > 
> > static const struct v4l2_subdev_ops mipi_csis_subdev_ops = {
> >         .core   = &mipi_csis_core_ops,
> >         .video  = &mipi_csis_video_ops,
> >         .pad    = &mipi_csis_pad_ops,
> > };
> > 
> > /* ----------------------------------------------------------------
> > -------------
> >  * Media entity operations
> >  */
> > 
> > static int mipi_csis_link_setup(struct media_entity *entity,
> >                                 const struct media_pad *local_pad,
> >                                 const struct media_pad *remote_pad,
> > u32 flags)
> > {
> >         struct v4l2_subdev *sd =
> > media_entity_to_v4l2_subdev(entity);
> >         struct csi_state *state = mipi_sd_to_csis_state(sd);
> >         struct v4l2_subdev *remote_sd;
> > 
> >         dev_dbg(state->dev, "link setup %s -> %s", remote_pad-
> > >entity->name,
> >                 local_pad->entity->name);
> > 
> >         /* We only care about the link to the source. */
> >         if (!(local_pad->flags & MEDIA_PAD_FL_SINK))
> >                 return 0;
> > 
> >         remote_sd = media_entity_to_v4l2_subdev(remote_pad-
> > >entity);
> > 
> >         if (flags & MEDIA_LNK_FL_ENABLED) {
> >                 if (state->src_sd)
> >                         return -EBUSY;
> > 
> >                 state->src_sd = remote_sd;
> >         } else {
> >                 state->src_sd = NULL;
> >         }
> > 
> >         return 0;
> > }
> > 
> > static const struct media_entity_operations mipi_csis_entity_ops =
> > {
> >         .link_setup     = mipi_csis_link_setup,
> >         .link_validate  = v4l2_subdev_link_validate,
> >         .get_fwnode_pad = v4l2_subdev_get_fwnode_pad_1_to_1,
> > };
> > 
> > /* ----------------------------------------------------------------
> > -------------
> >  * Async subdev notifier
> >  */
> > 
> > static struct csi_state *
> > mipi_notifier_to_csis_state(struct v4l2_async_notifier *n)
> > {
> >         return container_of(n, struct csi_state, notifier);
> > }
> > 
> > static int mipi_csis_notify_bound(struct v4l2_async_notifier
> > *notifier,
> >                                   struct v4l2_subdev *sd,
> >                                   struct v4l2_async_subdev *asd)
> > {
> >         struct csi_state *state =
> > mipi_notifier_to_csis_state(notifier);
> >         struct media_pad *sink = &state-
> > >sd.entity.pads[CSIS_PAD_SINK];
> > 
> >         return v4l2_create_fwnode_links_to_pad(sd, sink, 0);
> > }
> > 
> > static const struct v4l2_async_notifier_operations
> > mipi_csis_notify_ops = {
> >         .bound = mipi_csis_notify_bound,
> > };
> > 
> > static int mipi_csis_async_register(struct csi_state *state)
> > {
> >         struct v4l2_fwnode_endpoint vep = {
> >                 .bus_type = V4L2_MBUS_CSI2_DPHY,
> >         };
> >         struct v4l2_async_subdev *asd;
> >         struct fwnode_handle *ep;
> >         unsigned int i;
> >         int ret;
> > 
> >         v4l2_async_notifier_init(&state->notifier);
> > 
> >         ep = fwnode_graph_get_endpoint_by_id(dev_fwnode(state-
> > >dev), 0, 0,
> >                                             
> > FWNODE_GRAPH_ENDPOINT_NEXT);
> >         if (!ep)
> >                 return -ENOTCONN;
> > 
> >         ret = v4l2_fwnode_endpoint_parse(ep, &vep);
> >         if (ret)
> >                 goto err_parse;
> > 
> >         for (i = 0; i < vep.bus.mipi_csi2.num_data_lanes; ++i) {
> >                 if (vep.bus.mipi_csi2.data_lanes[i] != i + 1) {
> >                         dev_err(state->dev,
> >                                 "data lanes reordering is not
> > supported");
> >                         goto err_parse;
> >                 }
> >         }
> > 
> >         state->bus = vep.bus.mipi_csi2;
> > 
> >         dev_dbg(state->dev, "data lanes: %d\n", state-
> > >bus.num_data_lanes);
> >         dev_dbg(state->dev, "flags: 0x%08x\n", state->bus.flags);
> > 
> >         asd = v4l2_async_notifier_add_fwnode_remote_subdev(
> >                 &state->notifier, ep, struct v4l2_async_subdev);
> >         if (IS_ERR(asd)) {
> >                 ret = PTR_ERR(asd);
> >                 goto err_parse;
> >         }
> > 
> >         fwnode_handle_put(ep);
> > 
> >         state->notifier.ops = &mipi_csis_notify_ops;
> > 
> >         ret = v4l2_async_subdev_notifier_register(&state->sd,
> > &state->notifier);
> >         if (ret)
> >                 return ret;
> > 
> >         return v4l2_async_register_subdev(&state->sd);
> > 
> > err_parse:
> >         fwnode_handle_put(ep);
> > 
> >         return ret;
> > }
> > 
> > /* ----------------------------------------------------------------
> > -------------
> >  * Suspend/resume
> >  */
> > 
> > static int mipi_csis_pm_suspend(struct device *dev, bool runtime)
> > {
> >         struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >         struct csi_state *state = mipi_sd_to_csis_state(sd);
> >         int ret = 0;
> > 
> >         mutex_lock(&state->lock);
> >         if (state->state & ST_POWERED) {
> >                 mipi_csis_stop_stream(state);
> >                 ret = mipi_csis_phy_disable(state);
> >                 if (ret)
> >                         goto unlock;
> >                 mipi_csis_clk_disable(state);
> >                 state->state &= ~ST_POWERED;
> >                 if (!runtime)
> >                         state->state |= ST_SUSPENDED;
> >         }
> > 
> > unlock:
> >         mutex_unlock(&state->lock);
> > 
> >         return ret ? -EAGAIN : 0;
> > }
> > 
> > static int mipi_csis_pm_resume(struct device *dev, bool runtime)
> > {
> >         struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >         struct csi_state *state = mipi_sd_to_csis_state(sd);
> >         int ret = 0;
> > 
> >         mutex_lock(&state->lock);
> >         if (!runtime && !(state->state & ST_SUSPENDED))
> >                 goto unlock;
> > 
> >         if (!(state->state & ST_POWERED)) {
> >                 ret = mipi_csis_phy_enable(state);
> >                 if (ret)
> >                         goto unlock;
> > 
> >                 state->state |= ST_POWERED;
> >                 mipi_csis_clk_enable(state);
> >         }
> >         if (state->state & ST_STREAMING)
> >                 mipi_csis_start_stream(state);
> > 
> >         state->state &= ~ST_SUSPENDED;
> > 
> > unlock:
> >         mutex_unlock(&state->lock);
> > 
> >         return ret ? -EAGAIN : 0;
> > }
> > 
> > static int __maybe_unused mipi_csis_suspend(struct device *dev)
> > {
> >         return mipi_csis_pm_suspend(dev, false);
> > }
> > 
> > static int __maybe_unused mipi_csis_resume(struct device *dev)
> > {
> >         return mipi_csis_pm_resume(dev, false);
> > }
> > 
> > static int __maybe_unused mipi_csis_runtime_suspend(struct device
> > *dev)
> > {
> >         return mipi_csis_pm_suspend(dev, true);
> > }
> > 
> > static int __maybe_unused mipi_csis_runtime_resume(struct device
> > *dev)
> > {
> >         return mipi_csis_pm_resume(dev, true);
> > }
> > 
> > static const struct dev_pm_ops mipi_csis_pm_ops = {
> >         SET_RUNTIME_PM_OPS(mipi_csis_runtime_suspend,
> > mipi_csis_runtime_resume,
> >                            NULL)
> >         SET_SYSTEM_SLEEP_PM_OPS(mipi_csis_suspend,
> > mipi_csis_resume)
> > };
> > 
> > /* ----------------------------------------------------------------
> > -------------
> >  * Probe/remove & platform driver
> >  */
> > 
> > static int mipi_csis_subdev_init(struct csi_state *state)
> > {
> >         struct v4l2_subdev *sd = &state->sd;
> > 
> >         v4l2_subdev_init(sd, &mipi_csis_subdev_ops);
> >         sd->owner = THIS_MODULE;
> >         snprintf(sd->name, sizeof(sd->name), "%s.%d",
> >                  CSIS_SUBDEV_NAME, state->index);
> > 
> >         sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >         sd->ctrl_handler = NULL;
> > 
> >         sd->entity.function = MEDIA_ENT_F_VID_IF_BRIDGE;
> >         sd->entity.ops = &mipi_csis_entity_ops;
> > 
> >         sd->dev = state->dev;
> > 
> >         state->pads[CSIS_PAD_SINK].flags = MEDIA_PAD_FL_SINK
> >                                          |
> > MEDIA_PAD_FL_MUST_CONNECT;
> >         state->pads[CSIS_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE
> >                                            |
> > MEDIA_PAD_FL_MUST_CONNECT;
> >         return media_entity_pads_init(&sd->entity, CSIS_PADS_NUM,
> >                                       state->pads);
> > }
> > 
> > static int mipi_csis_parse_dt(struct csi_state *state)
> > {
> >         struct device_node *node = state->dev->of_node;
> > 
> >         return 0;
> > }
> > 
> > static int mipi_csis_probe(struct platform_device *pdev)
> > {
> >         struct device *dev = &pdev->dev;
> >         struct csi_state *state;
> >         int ret;
> > 
> >         state = devm_kzalloc(dev, sizeof(*state), GFP_KERNEL);
> >         if (!state)
> >                 return -ENOMEM;
> > 
> >         mutex_init(&state->lock);
> > 
> >         state->dev = dev;
> > 
> >         /* Parse DT properties. */
> >         ret = mipi_csis_parse_dt(state);
> >         if (ret < 0) {
> >                 dev_err(dev, "Failed to parse device tree: %d\n",
> > ret);
> >                 return ret;
> >         }
> > 
> >         /* Acquire resources. */
> >         state->regs = devm_platform_ioremap_resource(pdev, 0);
> >         if (IS_ERR(state->regs))
> >                 return PTR_ERR(state->regs);
> > 
> >         ret = mipi_csis_phy_init(state);
> >         if (ret < 0)
> >                 return ret;
> > 
> >         ret = mipi_csis_clk_get(state);
> >         if (ret < 0)
> >                 return ret;
> > 
> >         /* Reset PHY and enable the clocks. */
> >         mipi_csis_phy_reset(state);
> > 
> >         ret = mipi_csis_clk_enable(state);
> >         if (ret < 0) {
> >                 dev_err(state->dev, "failed to enable clocks:
> > %d\n", ret);
> >                 return ret;
> >         }
> > 
> >         /* Initialize and register the subdev. */
> >         ret = mipi_csis_subdev_init(state);
> >         if (ret < 0)
> >                 goto disable_clock;
> > 
> >         platform_set_drvdata(pdev, &state->sd);
> > 
> >         ret = mipi_csis_async_register(state);
> >         if (ret < 0) {
> >                 dev_err(dev, "async register failed: %d\n", ret);
> >                 goto cleanup;
> >         }
> > 
> >         /* Initialize debugfs. */
> >         mipi_csis_debugfs_init(state);
> > 
> >         /* Enable runtime PM. */
> >         pm_runtime_enable(dev);
> >         if (!pm_runtime_enabled(dev)) {
> >                 ret = mipi_csis_pm_resume(dev, true);
> >                 if (ret < 0)
> >                         goto unregister_all;
> >         }
> > 
> >         dev_info(dev, "lanes: %d\n",
> >                  state->bus.num_data_lanes);
> > 
> >         return 0;
> > 
> > unregister_all:
> >         mipi_csis_debugfs_exit(state);
> > cleanup:
> >         media_entity_cleanup(&state->sd.entity);
> >         v4l2_async_notifier_unregister(&state->notifier);
> >         v4l2_async_notifier_cleanup(&state->notifier);
> >         v4l2_async_unregister_subdev(&state->sd);
> > disable_clock:
> >         mipi_csis_clk_disable(state);
> >         mutex_destroy(&state->lock);
> > 
> >         return ret;
> > }
> > 
> > static int mipi_csis_remove(struct platform_device *pdev)
> > {
> >         struct v4l2_subdev *sd = platform_get_drvdata(pdev);
> >         struct csi_state *state = mipi_sd_to_csis_state(sd);
> > 
> >         mipi_csis_debugfs_exit(state);
> >         v4l2_async_notifier_unregister(&state->notifier);
> >         v4l2_async_notifier_cleanup(&state->notifier);
> >         v4l2_async_unregister_subdev(&state->sd);
> > 
> >         pm_runtime_disable(&pdev->dev);
> >         mipi_csis_pm_suspend(&pdev->dev, true);
> >         mipi_csis_clk_disable(state);
> >         media_entity_cleanup(&state->sd.entity);
> >         mutex_destroy(&state->lock);
> >         pm_runtime_set_suspended(&pdev->dev);
> > 
> >         return 0;
> > }
> > 
> > static const struct of_device_id mipi_csis_of_match[] = {
> >         { .compatible = "fsl,imx8mq-mipi-csi2",},
> >         { /* sentinel */ },
> > };
> > MODULE_DEVICE_TABLE(of, mipi_csis_of_match);
> > 
> > static struct platform_driver mipi_csis_driver = {
> >         .probe          = mipi_csis_probe,
> >         .remove         = mipi_csis_remove,
> >         .driver         = {
> >                 .of_match_table = mipi_csis_of_match,
> >                 .name           = CSIS_DRIVER_NAME,
> >                 .pm             = &mipi_csis_pm_ops,
> >         },
> > };
> > 
> > module_platform_driver(mipi_csis_driver);
> > 
> > MODULE_DESCRIPTION("i.MX8MQ MIPI CSI-2 receiver driver");
> > MODULE_LICENSE("GPL v2");
> > MODULE_ALIAS("platform:imx8mq-mipi-csi2");
>

Martin Kepplinger May 19, 2021, 3:21 p.m. UTC | #12

Am Mittwoch, dem 19.05.2021 um 04:14 +0300 schrieb Laurent Pinchart:
> Hi Martin,
> 
> On Tue, May 18, 2021 at 04:39:00PM +0200, Martin Kepplinger wrote:
> > Am Sonntag, dem 16.05.2021 um 01:55 +0300 schrieb Laurent Pinchart:
> > > On Tue, May 04, 2021 at 05:59:39PM +0200, Martin Kepplinger
> > > wrote:
> > > > hi Laurent, again thanks a lot for posting this series! I can't
> > > > fully test
> > > > it, but base my work for imx8mq on it now. imx8mq includes
> > > > yet another mipi phy version than this and below is some very
> > > > rough testing
> > > > code. it's not at all something I sign-off on but my following
> > > > problem is based on it.
> > > 
> > > Unless I'm mistaken, the CSI-2 receiver in the i.MX8MQ is a
> > > completely
> > > different device. I wouldn't try to support it in the imx7-mipi-
> > > csis
> > > driver, but in a separate driver.
> > > 
> > > >  * configured to use both staging csi drivers
> > > >  * the csi bridge driver at least streams frames together with
> > > > the
> > > > nxp "yav" mipi driver
> > > > 
> > > > media-ctl -p now says the output below, so one link from mipi
> > > > to
> > > > csi is missing.
> > > > 
> > > > Note that
> > > > 
> > > > media-ctl --set-v4l2 "'csi':0 [fmt:SBGGR10/640x480]"
> > > > works in that it changes the configured format below, but
> > > > 
> > > > media-ctl -l "'imx7-mipi-csis.0':1" -> "'csi':0[1]"
> > > > doesn't create said missing link.
> > > 
> > > media-ctl can't create links, it can only enable or disable them.
> > > Link
> > > creation is the prerogative of drivers.
> > > 
> > > > Do I maybe use that wrongly? If now, does anything come to mind
> > > > that would
> > > > be missing specifically?
> > > 
> > > The link should be created by the call to media_create_pad_link()
> > > in
> > > imx_media_capture_device_register(). You'll need to figure out if
> > > the
> > > function is called and returns an error early, or if it doesn't
> > > get
> > > called at all, and why.
> > > 
> > > > When trying to stream anyway (if that makes sense), I get the
> > > > following:
> > > > 
> > > > [ 2008.377470] capture_start_streaming: starting
> > > > [ 2008.381883] capture_find_format: calling
> > > > imx_media_find_mbus_format with code 0x2006
> > > > [ 2008.389671] imx7-csi 30a90000.csi1_bridge:
> > > > capture_validate_fmt: capture_find_format err
> > > > [ 2008.397794] imx7-csi 30a90000.csi1_bridge:
> > > > capture_validate_fmt: capture_find_format found colorspace 0x1
> > > > != 0x0
> > > > [ 2008.407999] imx7-csi 30a90000.csi1_bridge: capture format
> > > > not valid: -32
> > > > 
> > > > and if I ignore that (because I'm not yet sure whether that is
> > > > specific to
> > > > platforms including an IPU), I get a WARN_ON from
> > > > vb2_start_streaming()
> > > 
> > > That I have a fix for, I'll post it as part of an imx7-media-csi
> > > series.
> > 
> > Hi Laurent,
> > 
> > You haven't posted that fix you're talking about, right?
> 
> Correct. It's now fixed (see "[PATCH] media: imx: imx7-media-csi: Fix
> buffer return upon stream start failure", I've CC'ed you).
> 
> > The below
> > driver (attached; I'll send it as patches after I successfully
> > tested
> > myself, and cleanup and fixes obviously)
> 
> Don't forget the DT bindings at that point :-)
> 
> > results in the same situation I described above:
> > 
> > * missing link from mipi (entity 10) -> csi (entity 1):
> 
> The link is supposed to be created by
> v4l2_create_fwnode_links_to_pad(),
> called from imx7_csi_notify_bound(). Could you trace the calls and
> figure out what goes wrong ?

that bound callback imx7_csi_notify_bound() is called only once during
probe: v4l2_create_fwnode_links_to_pad() returns 0 and
imx7_csi_async_register() returns success too.

(the imx8mq mipi driver probes successfully, independently, a few ms
before the above, the sensor driver a few ms after that)

So nothing obviously going wrong during probe(). that's the call trace
in imx7_csi_notify_bound() :

[    5.992126] Call trace:
[    5.992129]  dump_backtrace+0x0/0x1e4
[    5.992149]  show_stack+0x24/0x30
[    5.992155]  dump_stack+0xd0/0x12c
[    5.992163]  imx7_csi_notify_bound+0x78/0x8c [imx7_media_csi]
[    5.992182]  v4l2_async_match_notify+0x58/0x1b0 [videodev]
[    5.992262]  v4l2_async_notifier_try_all_subdevs+0x60/0xd0
[videodev]
[    5.992316]  v4l2_async_match_notify+0x130/0x1b0 [videodev]
[    5.992370]  v4l2_async_register_subdev+0x98/0x1f0 [videodev]
[    5.992424]  imx7_csi_probe+0x2c8/0x310 [imx7_media_csi]
[    5.992436]  platform_probe+0x74/0xe4
[    5.992446]  really_probe+0xf0/0x510
[    5.992453]  driver_probe_device+0xfc/0x170
[    5.992458]  device_driver_attach+0xcc/0xd4
[    5.992464]  __driver_attach+0xb0/0x17c
[    5.992469]  bus_for_each_dev+0x7c/0xe0
[    5.992478]  driver_attach+0x30/0x40
[    5.992483]  bus_add_driver+0x154/0x250
[    5.992490]  driver_register+0x84/0x140
[    5.992496]  __platform_driver_register+0x34/0x40
[    5.992502]  imx7_csi_driver_init+0x2c/0x1000 [imx7_media_csi]
[    5.992515]  do_one_initcall+0x50/0x2d0
[    5.992522]  do_init_module+0x60/0x274
[    5.992532]  load_module+0x2078/0x2450
[    5.992539]  __do_sys_finit_module+0xbc/0x130
[    5.992547]  __arm64_sys_finit_module+0x2c/0x3c
[    5.992555]  el0_svc_common.constprop.0+0x68/0x130
[    5.992565]  do_el0_svc+0x28/0x34
[    5.992571]  el0_svc+0x2c/0x54
[    5.992580]  el0_sync_handler+0x1a4/0x1b0
[    5.992587]  el0_sync+0x174/0x180


btw, my test is:

v4l2-ctl -d "/dev/v4l/by-path/platform-30a90000.csi1_bridge-video-
index0" --set-fmt-video=width=640,height=480 --stream-mmap --stream-
to=test.raw --stream-count=1

and that (probably because of the missing link) fails with

VIDIOC_STREAMON returned -1 (No such device)

which is in the kernel:

imx7-csi 30a90000.csi1_bridge: pipeline start failed with -19

Laurent Pinchart May 19, 2021, 3:46 p.m. UTC | #13

Hi Martin,

On Wed, May 19, 2021 at 05:21:11PM +0200, Martin Kepplinger wrote:
> Am Mittwoch, dem 19.05.2021 um 04:14 +0300 schrieb Laurent Pinchart:
> > On Tue, May 18, 2021 at 04:39:00PM +0200, Martin Kepplinger wrote:
> > > Am Sonntag, dem 16.05.2021 um 01:55 +0300 schrieb Laurent Pinchart:
> > > > On Tue, May 04, 2021 at 05:59:39PM +0200, Martin Kepplinger wrote:
> > > > > hi Laurent, again thanks a lot for posting this series! I can't fully test
> > > > > it, but base my work for imx8mq on it now. imx8mq includes
> > > > > yet another mipi phy version than this and below is some very rough testing
> > > > > code. it's not at all something I sign-off on but my following
> > > > > problem is based on it.
> > > > 
> > > > Unless I'm mistaken, the CSI-2 receiver in the i.MX8MQ is a completely
> > > > different device. I wouldn't try to support it in the imx7-mipi-csis
> > > > driver, but in a separate driver.
> > > > 
> > > > >  * configured to use both staging csi drivers
> > > > >  * the csi bridge driver at least streams frames together with the
> > > > > nxp "yav" mipi driver
> > > > > 
> > > > > media-ctl -p now says the output below, so one link from mipi to
> > > > > csi is missing.
> > > > > 
> > > > > Note that
> > > > > 
> > > > > media-ctl --set-v4l2 "'csi':0 [fmt:SBGGR10/640x480]"
> > > > > works in that it changes the configured format below, but
> > > > > 
> > > > > media-ctl -l "'imx7-mipi-csis.0':1" -> "'csi':0[1]"
> > > > > doesn't create said missing link.
> > > > 
> > > > media-ctl can't create links, it can only enable or disable them. Link
> > > > creation is the prerogative of drivers.
> > > > 
> > > > > Do I maybe use that wrongly? If now, does anything come to mind that would
> > > > > be missing specifically?
> > > > 
> > > > The link should be created by the call to media_create_pad_link() in
> > > > imx_media_capture_device_register(). You'll need to figure out if the
> > > > function is called and returns an error early, or if it doesn't get
> > > > called at all, and why.
> > > > 
> > > > > When trying to stream anyway (if that makes sense), I get the
> > > > > following:
> > > > > 
> > > > > [ 2008.377470] capture_start_streaming: starting
> > > > > [ 2008.381883] capture_find_format: calling imx_media_find_mbus_format with code 0x2006
> > > > > [ 2008.389671] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt: capture_find_format err
> > > > > [ 2008.397794] imx7-csi 30a90000.csi1_bridge: capture_validate_fmt: capture_find_format found colorspace 0x1 != 0x0
> > > > > [ 2008.407999] imx7-csi 30a90000.csi1_bridge: capture format not valid: -32
> > > > > 
> > > > > and if I ignore that (because I'm not yet sure whether that is specific to
> > > > > platforms including an IPU), I get a WARN_ON from vb2_start_streaming()
> > > > 
> > > > That I have a fix for, I'll post it as part of an imx7-media-csi
> > > > series.
> > > 
> > > Hi Laurent,
> > > 
> > > You haven't posted that fix you're talking about, right?
> > 
> > Correct. It's now fixed (see "[PATCH] media: imx: imx7-media-csi: Fix
> > buffer return upon stream start failure", I've CC'ed you).
> > 
> > > The below
> > > driver (attached; I'll send it as patches after I successfully tested
> > > myself, and cleanup and fixes obviously)
> > 
> > Don't forget the DT bindings at that point :-)
> > 
> > > results in the same situation I described above:
> > > 
> > > * missing link from mipi (entity 10) -> csi (entity 1):
> > 
> > The link is supposed to be created by
> > v4l2_create_fwnode_links_to_pad(),
> > called from imx7_csi_notify_bound(). Could you trace the calls and
> > figure out what goes wrong ?
> 
> that bound callback imx7_csi_notify_bound() is called only once during
> probe: v4l2_create_fwnode_links_to_pad() returns 0 and
> imx7_csi_async_register() returns success too.

What subdev is it called for (I assume the imx8mq-mipi-csis.0) ? Have
you traced inside the function to see why it doesn't create links ?

> (the imx8mq mipi driver probes successfully, independently, a few ms
> before the above, the sensor driver a few ms after that)
> 
> So nothing obviously going wrong during probe(). that's the call trace
> in imx7_csi_notify_bound() :
> 
> [    5.992126] Call trace:
> [    5.992129]  dump_backtrace+0x0/0x1e4
> [    5.992149]  show_stack+0x24/0x30
> [    5.992155]  dump_stack+0xd0/0x12c
> [    5.992163]  imx7_csi_notify_bound+0x78/0x8c [imx7_media_csi]
> [    5.992182]  v4l2_async_match_notify+0x58/0x1b0 [videodev]
> [    5.992262]  v4l2_async_notifier_try_all_subdevs+0x60/0xd0 [videodev]
> [    5.992316]  v4l2_async_match_notify+0x130/0x1b0 [videodev]
> [    5.992370]  v4l2_async_register_subdev+0x98/0x1f0 [videodev]
> [    5.992424]  imx7_csi_probe+0x2c8/0x310 [imx7_media_csi]
> [    5.992436]  platform_probe+0x74/0xe4
> [    5.992446]  really_probe+0xf0/0x510
> [    5.992453]  driver_probe_device+0xfc/0x170
> [    5.992458]  device_driver_attach+0xcc/0xd4
> [    5.992464]  __driver_attach+0xb0/0x17c
> [    5.992469]  bus_for_each_dev+0x7c/0xe0
> [    5.992478]  driver_attach+0x30/0x40
> [    5.992483]  bus_add_driver+0x154/0x250
> [    5.992490]  driver_register+0x84/0x140
> [    5.992496]  __platform_driver_register+0x34/0x40
> [    5.992502]  imx7_csi_driver_init+0x2c/0x1000 [imx7_media_csi]
> [    5.992515]  do_one_initcall+0x50/0x2d0
> [    5.992522]  do_init_module+0x60/0x274
> [    5.992532]  load_module+0x2078/0x2450
> [    5.992539]  __do_sys_finit_module+0xbc/0x130
> [    5.992547]  __arm64_sys_finit_module+0x2c/0x3c
> [    5.992555]  el0_svc_common.constprop.0+0x68/0x130
> [    5.992565]  do_el0_svc+0x28/0x34
> [    5.992571]  el0_svc+0x2c/0x54
> [    5.992580]  el0_sync_handler+0x1a4/0x1b0
> [    5.992587]  el0_sync+0x174/0x180
> 
> 
> btw, my test is:
> 
> v4l2-ctl -d "/dev/v4l/by-path/platform-30a90000.csi1_bridge-video-
> index0" --set-fmt-video=width=640,height=480 --stream-mmap --stream-
> to=test.raw --stream-count=1
> 
> and that (probably because of the missing link) fails with
> 
> VIDIOC_STREAMON returned -1 (No such device)
> 
> which is in the kernel:
> 
> imx7-csi 30a90000.csi1_bridge: pipeline start failed with -19

Let's fix the missing link first.

Martin Kepplinger May 20, 2021, 10:54 a.m. UTC | #14

Am Mittwoch, dem 19.05.2021 um 18:46 +0300 schrieb Laurent Pinchart:
> Hi Martin,
> 
> On Wed, May 19, 2021 at 05:21:11PM +0200, Martin Kepplinger wrote:
> > Am Mittwoch, dem 19.05.2021 um 04:14 +0300 schrieb Laurent
> > Pinchart:
> > > On Tue, May 18, 2021 at 04:39:00PM +0200, Martin Kepplinger
> > > wrote:
> > > > Am Sonntag, dem 16.05.2021 um 01:55 +0300 schrieb Laurent
> > > > Pinchart:
> > > > > On Tue, May 04, 2021 at 05:59:39PM +0200, Martin Kepplinger
> > > > > wrote:
> > > > > > hi Laurent, again thanks a lot for posting this series! I
> > > > > > can't fully test
> > > > > > it, but base my work for imx8mq on it now. imx8mq includes
> > > > > > yet another mipi phy version than this and below is some
> > > > > > very rough testing
> > > > > > code. it's not at all something I sign-off on but my
> > > > > > following
> > > > > > problem is based on it.
> > > > > 
> > > > > Unless I'm mistaken, the CSI-2 receiver in the i.MX8MQ is a
> > > > > completely
> > > > > different device. I wouldn't try to support it in the imx7-
> > > > > mipi-csis
> > > > > driver, but in a separate driver.
> > > > > 
> > > > > >  * configured to use both staging csi drivers
> > > > > >  * the csi bridge driver at least streams frames together
> > > > > > with the
> > > > > > nxp "yav" mipi driver
> > > > > > 
> > > > > > media-ctl -p now says the output below, so one link from
> > > > > > mipi to
> > > > > > csi is missing.
> > > > > > 
> > > > > > Note that
> > > > > > 
> > > > > > media-ctl --set-v4l2 "'csi':0 [fmt:SBGGR10/640x480]"
> > > > > > works in that it changes the configured format below, but
> > > > > > 
> > > > > > media-ctl -l "'imx7-mipi-csis.0':1" -> "'csi':0[1]"
> > > > > > doesn't create said missing link.
> > > > > 
> > > > > media-ctl can't create links, it can only enable or disable
> > > > > them. Link
> > > > > creation is the prerogative of drivers.
> > > > > 
> > > > > > Do I maybe use that wrongly? If now, does anything come to
> > > > > > mind that would
> > > > > > be missing specifically?
> > > > > 
> > > > > The link should be created by the call to
> > > > > media_create_pad_link() in
> > > > > imx_media_capture_device_register(). You'll need to figure
> > > > > out if the
> > > > > function is called and returns an error early, or if it
> > > > > doesn't get
> > > > > called at all, and why.
> > > > > 
> > > > > > When trying to stream anyway (if that makes sense), I get
> > > > > > the
> > > > > > following:
> > > > > > 
> > > > > > [ 2008.377470] capture_start_streaming: starting
> > > > > > [ 2008.381883] capture_find_format: calling
> > > > > > imx_media_find_mbus_format with code 0x2006
> > > > > > [ 2008.389671] imx7-csi 30a90000.csi1_bridge:
> > > > > > capture_validate_fmt: capture_find_format err
> > > > > > [ 2008.397794] imx7-csi 30a90000.csi1_bridge:
> > > > > > capture_validate_fmt: capture_find_format found colorspace
> > > > > > 0x1 != 0x0
> > > > > > [ 2008.407999] imx7-csi 30a90000.csi1_bridge: capture
> > > > > > format not valid: -32
> > > > > > 
> > > > > > and if I ignore that (because I'm not yet sure whether that
> > > > > > is specific to
> > > > > > platforms including an IPU), I get a WARN_ON from
> > > > > > vb2_start_streaming()
> > > > > 
> > > > > That I have a fix for, I'll post it as part of an imx7-media-
> > > > > csi
> > > > > series.
> > > > 
> > > > Hi Laurent,
> > > > 
> > > > You haven't posted that fix you're talking about, right?
> > > 
> > > Correct. It's now fixed (see "[PATCH] media: imx: imx7-media-csi:
> > > Fix
> > > buffer return upon stream start failure", I've CC'ed you).
> > > 
> > > > The below
> > > > driver (attached; I'll send it as patches after I successfully
> > > > tested
> > > > myself, and cleanup and fixes obviously)
> > > 
> > > Don't forget the DT bindings at that point :-)
> > > 
> > > > results in the same situation I described above:
> > > > 
> > > > * missing link from mipi (entity 10) -> csi (entity 1):
> > > 
> > > The link is supposed to be created by
> > > v4l2_create_fwnode_links_to_pad(),
> > > called from imx7_csi_notify_bound(). Could you trace the calls
> > > and
> > > figure out what goes wrong ?
> > 
> > that bound callback imx7_csi_notify_bound() is called only once
> > during
> > probe: v4l2_create_fwnode_links_to_pad() returns 0 and
> > imx7_csi_async_register() returns success too.
> 
> What subdev is it called for (I assume the imx8mq-mipi-csis.0) ? Have
> you traced inside the function to see why it doesn't create links ?

I fixed mipi -> csi link. I had the DT port descriptions for mipi csi
wrong.

now, just because I think it makes sense, I do:

media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480 colorspace:raw]"

which now prints:

Device topology
- entity 1: csi (2 pads, 2 links)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev0
	pad0: Sink
		[fmt:SGBRG10_1X10/640x480 field:none colorspace:raw
xfer:none ycbcr:601 quantization:full-range]
		<- "imx8mq-mipi-csis.0":1 [ENABLED,IMMUTABLE]
	pad1: Source
		[fmt:SGBRG10_1X10/640x480 field:none colorspace:raw
xfer:none ycbcr:601 quantization:full-range]
		-> "csi capture":0 [ENABLED,IMMUTABLE]

- entity 4: csi capture (1 pad, 1 link)
            type Node subtype V4L flags 0
            device node name /dev/video1
	pad0: Sink
		<- "csi":1 [ENABLED,IMMUTABLE]

- entity 10: imx8mq-mipi-csis.0 (2 pads, 2 links)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev1
	pad0: Sink
		<- "hi846 2-0020":0 []
	pad1: Source
		-> "csi":0 [ENABLED,IMMUTABLE]

- entity 15: hi846 2-0020 (1 pad, 1 link)
             type V4L2 subdev subtype Sensor flags 0
             device node name /dev/v4l-subdev2
	pad0: Source
		[fmt:SGBRG10_1X10/640x480 field:none colorspace:raw]
		-> "imx8mq-mipi-csis.0":0 []

> > 
> > btw, my test is:
> > 
> > v4l2-ctl -d "/dev/v4l/by-path/platform-30a90000.csi1_bridge-video-
> > index0" --set-fmt-video=width=640,height=480 --stream-mmap --
> > stream-
> > to=test.raw --stream-count=1
> > 
> > and that (probably because of the missing link) fails with
> > 
> > VIDIOC_STREAMON returned -1 (No such device)
> > 
> > which is in the kernel:
> > 
> > imx7-csi 30a90000.csi1_bridge: pipeline start failed with -19
> 
> Let's fix the missing link first.
> 

But now when trying to stream a frame, the error is:

Because of:

media bus code not compatible with the pixel format set on the video
node: 1 != 0

I get :

imx7-csi 30a90000.csi1_bridge: capture format not valid

which becomes for userspace:

VIDIOC_STREAMON returned -1 (Broken pipe)

Could that be a "user-problem" because "fmt" is not exactly the same
everywhere? Also, the sensor entity pad is not yet ENABLED...

(media-ctl is still very new to me, sorry if that's dumb questions now)

                          martin

Laurent Pinchart May 20, 2021, 12:37 p.m. UTC | #15

Hi Martin,

On Thu, May 20, 2021 at 12:54:27PM +0200, Martin Kepplinger wrote:
> Am Mittwoch, dem 19.05.2021 um 18:46 +0300 schrieb Laurent Pinchart:
> > On Wed, May 19, 2021 at 05:21:11PM +0200, Martin Kepplinger wrote:
> > > Am Mittwoch, dem 19.05.2021 um 04:14 +0300 schrieb Laurent Pinchart:
> > > > On Tue, May 18, 2021 at 04:39:00PM +0200, Martin Kepplinger wrote:
> > > > > Am Sonntag, dem 16.05.2021 um 01:55 +0300 schrieb Laurent Pinchart:
> > > > > > On Tue, May 04, 2021 at 05:59:39PM +0200, Martin Kepplinger wrote:

[snip]

> I fixed mipi -> csi link. I had the DT port descriptions for mipi csi
> wrong.

\o/

> now, just because I think it makes sense, I do:
> 
> media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480 colorspace:raw]"
> 
> which now prints:
> 
> Device topology
> - entity 1: csi (2 pads, 2 links)
>             type V4L2 subdev subtype Unknown flags 0
>             device node name /dev/v4l-subdev0
> 	pad0: Sink
> 		[fmt:SGBRG10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> 		<- "imx8mq-mipi-csis.0":1 [ENABLED,IMMUTABLE]
> 	pad1: Source
> 		[fmt:SGBRG10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> 		-> "csi capture":0 [ENABLED,IMMUTABLE]
> 
> - entity 4: csi capture (1 pad, 1 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video1
> 	pad0: Sink
> 		<- "csi":1 [ENABLED,IMMUTABLE]
> 
> - entity 10: imx8mq-mipi-csis.0 (2 pads, 2 links)
>              type V4L2 subdev subtype Unknown flags 0
>              device node name /dev/v4l-subdev1
> 	pad0: Sink
> 		<- "hi846 2-0020":0 []
> 	pad1: Source
> 		-> "csi":0 [ENABLED,IMMUTABLE]

This subdev doesn't seem to report formats on its sink and source pads,
which is weird. I've had a quick look at the .get_fmt() and .set_fmt()
implementations in the code you've posted, and they're wrong. They
shouldn't pass the calls to the source subdev with v4l2_subdev_call(),
they should instead implement get and set format on this subdev. You can
look at the imx7-mipi-csis driver to see how that's done. Once you'll
have fixed this, you'll have to set the format on each pad with
media-ctl to make sure formats through the pipeline match.

The only location where you imx8mq-mipi-csis driver should use
v4l2_subdev_call() is in .s_stream(), to propagate the operation to the
source.

By the way, I'd replace every occurence of "csis" with "csi2" in your
driver. The name "csis" in the i.MX7 driver comes from the CSI-2 RX IP
core that is named CSIS. That's not the case on the i.MX8QM.

> - entity 15: hi846 2-0020 (1 pad, 1 link)
>              type V4L2 subdev subtype Sensor flags 0
>              device node name /dev/v4l-subdev2
> 	pad0: Source
> 		[fmt:SGBRG10_1X10/640x480 field:none colorspace:raw]
> 		-> "imx8mq-mipi-csis.0":0 []

You need to enable this link, the following should do

media-ctl -l "'hi846 2-0020':0 -> 'imx8mq-mipi-csis.0':0 [1]"

> > > btw, my test is:
> > > 
> > > v4l2-ctl -d "/dev/v4l/by-path/platform-30a90000.csi1_bridge-video-index0"
> > > --set-fmt-video=width=640,height=480 --stream-mmap
> > > --stream-to=test.raw --stream-count=1
> > > 
> > > and that (probably because of the missing link) fails with
> > > 
> > > VIDIOC_STREAMON returned -1 (No such device)
> > > 
> > > which is in the kernel:
> > > 
> > > imx7-csi 30a90000.csi1_bridge: pipeline start failed with -19
> > 
> > Let's fix the missing link first.
> 
> But now when trying to stream a frame, the error is:
> 
> Because of:
> 
> media bus code not compatible with the pixel format set on the video
> node: 1 != 0
> 
> I get :
> 
> imx7-csi 30a90000.csi1_bridge: capture format not valid
> 
> which becomes for userspace:
> 
> VIDIOC_STREAMON returned -1 (Broken pipe)
> 
> Could that be a "user-problem" because "fmt" is not exactly the same
> everywhere? Also, the sensor entity pad is not yet ENABLED...

There's a combination of kernel-side issues (as detailed above) and
userspace issues (also as detailed above :-)).

> (media-ctl is still very new to me, sorry if that's dumb questions now)

It's not dumb at all.

Martin Kepplinger May 21, 2021, 9:25 a.m. UTC | #16

Am Donnerstag, dem 20.05.2021 um 15:37 +0300 schrieb Laurent Pinchart:
> Hi Martin,
> 
> On Thu, May 20, 2021 at 12:54:27PM +0200, Martin Kepplinger wrote:
> > Am Mittwoch, dem 19.05.2021 um 18:46 +0300 schrieb Laurent
> > Pinchart:
> > > On Wed, May 19, 2021 at 05:21:11PM +0200, Martin Kepplinger
> > > wrote:
> > > > Am Mittwoch, dem 19.05.2021 um 04:14 +0300 schrieb Laurent
> > > > Pinchart:
> > > > > On Tue, May 18, 2021 at 04:39:00PM +0200, Martin Kepplinger
> > > > > wrote:
> > > > > > Am Sonntag, dem 16.05.2021 um 01:55 +0300 schrieb Laurent
> > > > > > Pinchart:
> > > > > > > On Tue, May 04, 2021 at 05:59:39PM +0200, Martin
> > > > > > > Kepplinger wrote:
> 
> [snip]
> 
> > I fixed mipi -> csi link. I had the DT port descriptions for mipi
> > csi
> > wrong.
> 
> \o/
> 
> > now, just because I think it makes sense, I do:
> > 
> > media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480 colorspace:raw]"
> > 
> > which now prints:
> > 
> > Device topology
> > - entity 1: csi (2 pads, 2 links)
> >             type V4L2 subdev subtype Unknown flags 0
> >             device node name /dev/v4l-subdev0
> >         pad0: Sink
> >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw
> > xfer:none ycbcr:601 quantization:full-range]
> >                 <- "imx8mq-mipi-csis.0":1 [ENABLED,IMMUTABLE]
> >         pad1: Source
> >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw
> > xfer:none ycbcr:601 quantization:full-range]
> >                 -> "csi capture":0 [ENABLED,IMMUTABLE]
> > 
> > - entity 4: csi capture (1 pad, 1 link)
> >             type Node subtype V4L flags 0
> >             device node name /dev/video1
> >         pad0: Sink
> >                 <- "csi":1 [ENABLED,IMMUTABLE]
> > 
> > - entity 10: imx8mq-mipi-csis.0 (2 pads, 2 links)
> >              type V4L2 subdev subtype Unknown flags 0
> >              device node name /dev/v4l-subdev1
> >         pad0: Sink
> >                 <- "hi846 2-0020":0 []
> >         pad1: Source
> >                 -> "csi":0 [ENABLED,IMMUTABLE]
> 
> This subdev doesn't seem to report formats on its sink and source
> pads,
> which is weird. I've had a quick look at the .get_fmt() and
> .set_fmt()
> implementations in the code you've posted, and they're wrong. They
> shouldn't pass the calls to the source subdev with
> v4l2_subdev_call(),
> they should instead implement get and set format on this subdev. You
> can
> look at the imx7-mipi-csis driver to see how that's done. Once you'll
> have fixed this, you'll have to set the format on each pad with
> media-ctl to make sure formats through the pipeline match.
> 
> The only location where you imx8mq-mipi-csis driver should use
> v4l2_subdev_call() is in .s_stream(), to propagate the operation to
> the
> source.
> 
> By the way, I'd replace every occurence of "csis" with "csi2" in your
> driver. The name "csis" in the i.MX7 driver comes from the CSI-2 RX
> IP
> core that is named CSIS. That's not the case on the i.MX8QM.
> 
> > - entity 15: hi846 2-0020 (1 pad, 1 link)
> >              type V4L2 subdev subtype Sensor flags 0
> >              device node name /dev/v4l-subdev2
> >         pad0: Source
> >                 [fmt:SGBRG10_1X10/640x480 field:none
> > colorspace:raw]
> >                 -> "imx8mq-mipi-csis.0":0 []
> 
> You need to enable this link, the following should do
> 
> media-ctl -l "'hi846 2-0020':0 -> 'imx8mq-mipi-csis.0':0 [1]"
> 
> > > 

ok makes sense, even though I basically just allow a set of formats
without yet having to configure anything format-specific (I can at
least use bits-per-pixel later, so it makes sense to have them).
nevermind. I again append the current driver I use here.

then I do:

media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480 colorspace:raw]"
media-ctl --set-v4l2 "'imx8mq-mipi-csi2.0':0 [fmt:SGBRG10/640x480
colorspace:raw]"
media-ctl -l "'hi846 2-0020':0 -> 'imx8mq-mipi-csi2.0':0 [1]"

which gets me:

Device topology
- entity 1: csi (2 pads, 2 links)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev0
	pad0: Sink
		[fmt:SGBRG10_1X10/640x480 field:none colorspace:raw
xfer:none ycbcr:601 quantization:full-range]
		<- "imx8mq-mipi-csi2.0":1 [ENABLED,IMMUTABLE]
	pad1: Source
		[fmt:SGBRG10_1X10/640x480 field:none colorspace:raw
xfer:none ycbcr:601 quantization:full-range]
		-> "csi capture":0 [ENABLED,IMMUTABLE]

- entity 4: csi capture (1 pad, 1 link)
            type Node subtype V4L flags 0
            device node name /dev/video0
	pad0: Sink
		<- "csi":1 [ENABLED,IMMUTABLE]

- entity 10: imx8mq-mipi-csi2.0 (2 pads, 2 links)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev1
	pad0: Sink
		[fmt:SGBRG10_1X10/640x480]
		<- "hi846 2-0020":0 [ENABLED]
	pad1: Source
		[fmt:SGBRG10_1X10/640x480]
		-> "csi":0 [ENABLED,IMMUTABLE]

- entity 15: hi846 2-0020 (1 pad, 1 link)
             type V4L2 subdev subtype Sensor flags 0
             device node name /dev/v4l-subdev2
	pad0: Source
		[fmt:SGBRG10_1X10/640x480 field:none colorspace:raw]
		-> "imx8mq-mipi-csi2.0":0 [ENABLED]

but streaming still fails with:

[  352.255129] imx7-csi 30a90000.csi1_bridge: media bus code not
compatible with the pixel format set on the video node: 1 != 0
[  352.266439] imx7-csi 30a90000.csi1_bridge: capture format not valid

Laurent Pinchart May 21, 2021, 9:43 a.m. UTC | #17

Hi Martin,

On Fri, May 21, 2021 at 11:25:20AM +0200, Martin Kepplinger wrote:
> Am Donnerstag, dem 20.05.2021 um 15:37 +0300 schrieb Laurent Pinchart:
> > On Thu, May 20, 2021 at 12:54:27PM +0200, Martin Kepplinger wrote:
> > > Am Mittwoch, dem 19.05.2021 um 18:46 +0300 schrieb Laurent Pinchart:
> > > > On Wed, May 19, 2021 at 05:21:11PM +0200, Martin Kepplinger wrote:
> > > > > Am Mittwoch, dem 19.05.2021 um 04:14 +0300 schrieb Laurent Pinchart:
> > > > > > On Tue, May 18, 2021 at 04:39:00PM +0200, Martin Kepplinger wrote:
> > > > > > > Am Sonntag, dem 16.05.2021 um 01:55 +0300 schrieb Laurent Pinchart:
> > > > > > > > On Tue, May 04, 2021 at 05:59:39PM +0200, Martin Kepplinger wrote:
> > 
> > [snip]
> > 
> > > I fixed mipi -> csi link. I had the DT port descriptions for mipi
> > > csi wrong.
> > 
> > \o/
> > 
> > > now, just because I think it makes sense, I do:
> > > 
> > > media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480 colorspace:raw]"
> > > 
> > > which now prints:
> > > 
> > > Device topology
> > > - entity 1: csi (2 pads, 2 links)
> > >             type V4L2 subdev subtype Unknown flags 0
> > >             device node name /dev/v4l-subdev0
> > >         pad0: Sink
> > >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> > >                 <- "imx8mq-mipi-csis.0":1 [ENABLED,IMMUTABLE]
> > >         pad1: Source
> > >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> > >                 -> "csi capture":0 [ENABLED,IMMUTABLE]
> > > 
> > > - entity 4: csi capture (1 pad, 1 link)
> > >             type Node subtype V4L flags 0
> > >             device node name /dev/video1
> > >         pad0: Sink
> > >                 <- "csi":1 [ENABLED,IMMUTABLE]
> > > 
> > > - entity 10: imx8mq-mipi-csis.0 (2 pads, 2 links)
> > >              type V4L2 subdev subtype Unknown flags 0
> > >              device node name /dev/v4l-subdev1
> > >         pad0: Sink
> > >                 <- "hi846 2-0020":0 []
> > >         pad1: Source
> > >                 -> "csi":0 [ENABLED,IMMUTABLE]
> > 
> > This subdev doesn't seem to report formats on its sink and source pads,
> > which is weird. I've had a quick look at the .get_fmt() and .set_fmt()
> > implementations in the code you've posted, and they're wrong. They
> > shouldn't pass the calls to the source subdev with v4l2_subdev_call(),
> > they should instead implement get and set format on this subdev. You can
> > look at the imx7-mipi-csis driver to see how that's done. Once you'll
> > have fixed this, you'll have to set the format on each pad with
> > media-ctl to make sure formats through the pipeline match.
> > 
> > The only location where you imx8mq-mipi-csis driver should use
> > v4l2_subdev_call() is in .s_stream(), to propagate the operation to the
> > source.
> > 
> > By the way, I'd replace every occurence of "csis" with "csi2" in your
> > driver. The name "csis" in the i.MX7 driver comes from the CSI-2 RX IP
> > core that is named CSIS. That's not the case on the i.MX8QM.
> > 
> > > - entity 15: hi846 2-0020 (1 pad, 1 link)
> > >              type V4L2 subdev subtype Sensor flags 0
> > >              device node name /dev/v4l-subdev2
> > >         pad0: Source
> > >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw]
> > >                 -> "imx8mq-mipi-csis.0":0 []
> > 
> > You need to enable this link, the following should do
> > 
> > media-ctl -l "'hi846 2-0020':0 -> 'imx8mq-mipi-csis.0':0 [1]"
> 
> ok makes sense, even though I basically just allow a set of formats
> without yet having to configure anything format-specific (I can at
> least use bits-per-pixel later, so it makes sense to have them).
> nevermind. I again append the current driver I use here.
> 
> then I do:
> 
> media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480 colorspace:raw]"
> media-ctl --set-v4l2 "'imx8mq-mipi-csi2.0':0 [fmt:SGBRG10/640x480 colorspace:raw]"
> media-ctl -l "'hi846 2-0020':0 -> 'imx8mq-mipi-csi2.0':0 [1]"
> 
> which gets me:
> 
> Device topology
> - entity 1: csi (2 pads, 2 links)
>             type V4L2 subdev subtype Unknown flags 0
>             device node name /dev/v4l-subdev0
> 	pad0: Sink
> 		[fmt:SGBRG10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> 		<- "imx8mq-mipi-csi2.0":1 [ENABLED,IMMUTABLE]
> 	pad1: Source
> 		[fmt:SGBRG10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> 		-> "csi capture":0 [ENABLED,IMMUTABLE]
> 
> - entity 4: csi capture (1 pad, 1 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video0
> 	pad0: Sink
> 		<- "csi":1 [ENABLED,IMMUTABLE]
> 
> - entity 10: imx8mq-mipi-csi2.0 (2 pads, 2 links)
>              type V4L2 subdev subtype Unknown flags 0
>              device node name /dev/v4l-subdev1
> 	pad0: Sink
> 		[fmt:SGBRG10_1X10/640x480]
> 		<- "hi846 2-0020":0 [ENABLED]
> 	pad1: Source
> 		[fmt:SGBRG10_1X10/640x480]
> 		-> "csi":0 [ENABLED,IMMUTABLE]
> 
> - entity 15: hi846 2-0020 (1 pad, 1 link)
>              type V4L2 subdev subtype Sensor flags 0
>              device node name /dev/v4l-subdev2
> 	pad0: Source
> 		[fmt:SGBRG10_1X10/640x480 field:none colorspace:raw]
> 		-> "imx8mq-mipi-csi2.0":0 [ENABLED]

This looks better.

> but streaming still fails with:
> 
> [  352.255129] imx7-csi 30a90000.csi1_bridge: media bus code not compatible with the pixel format set on the video node: 1 != 0

What is the capture command line ? Can you trace this (I assume the
message is printed by capture_validate_fmt(), it's not present in
mainline so I don't know what 1 and 0 correspond to, even though I
suspect they would be IPUV3_COLORSPACE_* values) to see why it fails ?

> [  352.266439] imx7-csi 30a90000.csi1_bridge: capture format not valid

Martin Kepplinger May 21, 2021, 11:02 a.m. UTC | #18

Am Freitag, dem 21.05.2021 um 12:43 +0300 schrieb Laurent Pinchart:
> Hi Martin,
> 
> On Fri, May 21, 2021 at 11:25:20AM +0200, Martin Kepplinger wrote:
> > Am Donnerstag, dem 20.05.2021 um 15:37 +0300 schrieb Laurent
> > Pinchart:
> > > On Thu, May 20, 2021 at 12:54:27PM +0200, Martin Kepplinger
> > > wrote:
> > > > Am Mittwoch, dem 19.05.2021 um 18:46 +0300 schrieb Laurent
> > > > Pinchart:
> > > > > On Wed, May 19, 2021 at 05:21:11PM +0200, Martin Kepplinger
> > > > > wrote:
> > > > > > Am Mittwoch, dem 19.05.2021 um 04:14 +0300 schrieb Laurent
> > > > > > Pinchart:
> > > > > > > On Tue, May 18, 2021 at 04:39:00PM +0200, Martin
> > > > > > > Kepplinger wrote:
> > > > > > > > Am Sonntag, dem 16.05.2021 um 01:55 +0300 schrieb
> > > > > > > > Laurent Pinchart:
> > > > > > > > > On Tue, May 04, 2021 at 05:59:39PM +0200, Martin
> > > > > > > > > Kepplinger wrote:
> > > 
> > > [snip]
> > > 
> > > > I fixed mipi -> csi link. I had the DT port descriptions for
> > > > mipi
> > > > csi wrong.
> > > 
> > > \o/
> > > 
> > > > now, just because I think it makes sense, I do:
> > > > 
> > > > media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480
> > > > colorspace:raw]"
> > > > 
> > > > which now prints:
> > > > 
> > > > Device topology
> > > > - entity 1: csi (2 pads, 2 links)
> > > >             type V4L2 subdev subtype Unknown flags 0
> > > >             device node name /dev/v4l-subdev0
> > > >         pad0: Sink
> > > >                 [fmt:SGBRG10_1X10/640x480 field:none
> > > > colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> > > >                 <- "imx8mq-mipi-csis.0":1 [ENABLED,IMMUTABLE]
> > > >         pad1: Source
> > > >                 [fmt:SGBRG10_1X10/640x480 field:none
> > > > colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> > > >                 -> "csi capture":0 [ENABLED,IMMUTABLE]
> > > > 
> > > > - entity 4: csi capture (1 pad, 1 link)
> > > >             type Node subtype V4L flags 0
> > > >             device node name /dev/video1
> > > >         pad0: Sink
> > > >                 <- "csi":1 [ENABLED,IMMUTABLE]
> > > > 
> > > > - entity 10: imx8mq-mipi-csis.0 (2 pads, 2 links)
> > > >              type V4L2 subdev subtype Unknown flags 0
> > > >              device node name /dev/v4l-subdev1
> > > >         pad0: Sink
> > > >                 <- "hi846 2-0020":0 []
> > > >         pad1: Source
> > > >                 -> "csi":0 [ENABLED,IMMUTABLE]
> > > 
> > > This subdev doesn't seem to report formats on its sink and source
> > > pads,
> > > which is weird. I've had a quick look at the .get_fmt() and
> > > .set_fmt()
> > > implementations in the code you've posted, and they're wrong.
> > > They
> > > shouldn't pass the calls to the source subdev with
> > > v4l2_subdev_call(),
> > > they should instead implement get and set format on this subdev.
> > > You can
> > > look at the imx7-mipi-csis driver to see how that's done. Once
> > > you'll
> > > have fixed this, you'll have to set the format on each pad with
> > > media-ctl to make sure formats through the pipeline match.
> > > 
> > > The only location where you imx8mq-mipi-csis driver should use
> > > v4l2_subdev_call() is in .s_stream(), to propagate the operation
> > > to the
> > > source.
> > > 
> > > By the way, I'd replace every occurence of "csis" with "csi2" in
> > > your
> > > driver. The name "csis" in the i.MX7 driver comes from the CSI-2
> > > RX IP
> > > core that is named CSIS. That's not the case on the i.MX8QM.
> > > 
> > > > - entity 15: hi846 2-0020 (1 pad, 1 link)
> > > >              type V4L2 subdev subtype Sensor flags 0
> > > >              device node name /dev/v4l-subdev2
> > > >         pad0: Source
> > > >                 [fmt:SGBRG10_1X10/640x480 field:none
> > > > colorspace:raw]
> > > >                 -> "imx8mq-mipi-csis.0":0 []
> > > 
> > > You need to enable this link, the following should do
> > > 
> > > media-ctl -l "'hi846 2-0020':0 -> 'imx8mq-mipi-csis.0':0 [1]"
> > 
> > ok makes sense, even though I basically just allow a set of formats
> > without yet having to configure anything format-specific (I can at
> > least use bits-per-pixel later, so it makes sense to have them).
> > nevermind. I again append the current driver I use here.
> > 
> > then I do:
> > 
> > media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480 colorspace:raw]"
> > media-ctl --set-v4l2 "'imx8mq-mipi-csi2.0':0 [fmt:SGBRG10/640x480
> > colorspace:raw]"
> > media-ctl -l "'hi846 2-0020':0 -> 'imx8mq-mipi-csi2.0':0 [1]"
> > 
> > which gets me:
> > 
> > Device topology
> > - entity 1: csi (2 pads, 2 links)
> >             type V4L2 subdev subtype Unknown flags 0
> >             device node name /dev/v4l-subdev0
> >         pad0: Sink
> >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw
> > xfer:none ycbcr:601 quantization:full-range]
> >                 <- "imx8mq-mipi-csi2.0":1 [ENABLED,IMMUTABLE]
> >         pad1: Source
> >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw
> > xfer:none ycbcr:601 quantization:full-range]
> >                 -> "csi capture":0 [ENABLED,IMMUTABLE]
> > 
> > - entity 4: csi capture (1 pad, 1 link)
> >             type Node subtype V4L flags 0
> >             device node name /dev/video0
> >         pad0: Sink
> >                 <- "csi":1 [ENABLED,IMMUTABLE]
> > 
> > - entity 10: imx8mq-mipi-csi2.0 (2 pads, 2 links)
> >              type V4L2 subdev subtype Unknown flags 0
> >              device node name /dev/v4l-subdev1
> >         pad0: Sink
> >                 [fmt:SGBRG10_1X10/640x480]
> >                 <- "hi846 2-0020":0 [ENABLED]
> >         pad1: Source
> >                 [fmt:SGBRG10_1X10/640x480]
> >                 -> "csi":0 [ENABLED,IMMUTABLE]
> > 
> > - entity 15: hi846 2-0020 (1 pad, 1 link)
> >              type V4L2 subdev subtype Sensor flags 0
> >              device node name /dev/v4l-subdev2
> >         pad0: Source
> >                 [fmt:SGBRG10_1X10/640x480 field:none
> > colorspace:raw]
> >                 -> "imx8mq-mipi-csi2.0":0 [ENABLED]
> 
> This looks better.
> 
> > but streaming still fails with:
> > 
> > [  352.255129] imx7-csi 30a90000.csi1_bridge: media bus code not
> > compatible with the pixel format set on the video node: 1 != 0
> 
> What is the capture command line ? Can you trace this (I assume the
> message is printed by capture_validate_fmt(), it's not present in
> mainline so I don't know what 1 and 0 correspond to, even though I
> suspect they would be IPUV3_COLORSPACE_* values) to see why it fails
> ?

capture command:

v4l2-ctl -d "/dev/v4l/by-path/platform-30a90000.csi1_bridge-video-
index0" --set-fmt-video=width=640,height=480 --stream-mmap --stream-
to=test.raw --stream-count=1

I'll have to continue after the weekend, but let's share some logs.
Yes, "1 != 0" is from capture_validate_fmt():

priv->vdev.cc->cs != cc->cs

When I print the format the imx_media_find_mbus_format() finds and do:

media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480 colorspace:raw]"

I see:

[  184.251144] mc: media_release: Media Release
[  184.254397] selected specific mbus code 0 for list nr 0 (fourcc
0x59565955)
[  184.264564] selected specific mbus code 0 for list nr 0 (fourcc
0x59565955)
[  184.274763] selected specific mbus code 0 for list nr 21 (fourcc
0x36314247)
[  184.285102] selected specific mbus code 0 for list nr 21 (fourcc
0x36314247)
[  184.295383] selected specific mbus code 0 for list nr 21 (fourcc
0x36314247)
[  184.305752] selected specific mbus code 0 for list nr 21 (fourcc
0x36314247)

21 is the correct bayer format I want, but there's 0, so
"MEDIA_BUS_FMT_UYVY8_2X8" found the first 2 times. That is
IPUV3_COLORSPACE_YUV (1) while the correct Bayer format 21 is
IPUV3_COLORSPACE_RGB (0).

so some format settings not yet correct.

Laurent Pinchart May 21, 2021, 1:36 p.m. UTC | #19

Hi Martin,

On Fri, May 21, 2021 at 01:02:30PM +0200, Martin Kepplinger wrote:
> Am Freitag, dem 21.05.2021 um 12:43 +0300 schrieb Laurent Pinchart:
> > On Fri, May 21, 2021 at 11:25:20AM +0200, Martin Kepplinger wrote:
> > > Am Donnerstag, dem 20.05.2021 um 15:37 +0300 schrieb Laurent Pinchart:
> > > > On Thu, May 20, 2021 at 12:54:27PM +0200, Martin Kepplinger wrote:
> > > > > Am Mittwoch, dem 19.05.2021 um 18:46 +0300 schrieb Laurent Pinchart:
> > > > > > On Wed, May 19, 2021 at 05:21:11PM +0200, Martin Kepplinger wrote:
> > > > > > > Am Mittwoch, dem 19.05.2021 um 04:14 +0300 schrieb Laurent Pinchart:
> > > > > > > > On Tue, May 18, 2021 at 04:39:00PM +0200, Martin Kepplinger wrote:
> > > > > > > > > Am Sonntag, dem 16.05.2021 um 01:55 +0300 schrieb Laurent Pinchart:
> > > > > > > > > > On Tue, May 04, 2021 at 05:59:39PM +0200, Martin Kepplinger wrote:
> > > > 
> > > > [snip]
> > > > 
> > > > > I fixed mipi -> csi link. I had the DT port descriptions for
> > > > > mipi csi wrong.
> > > > 
> > > > \o/
> > > > 
> > > > > now, just because I think it makes sense, I do:
> > > > > 
> > > > > media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480
> > > > > colorspace:raw]"
> > > > > 
> > > > > which now prints:
> > > > > 
> > > > > Device topology
> > > > > - entity 1: csi (2 pads, 2 links)
> > > > >             type V4L2 subdev subtype Unknown flags 0
> > > > >             device node name /dev/v4l-subdev0
> > > > >         pad0: Sink
> > > > >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> > > > >                 <- "imx8mq-mipi-csis.0":1 [ENABLED,IMMUTABLE]
> > > > >         pad1: Source
> > > > >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> > > > >                 -> "csi capture":0 [ENABLED,IMMUTABLE]
> > > > > 
> > > > > - entity 4: csi capture (1 pad, 1 link)
> > > > >             type Node subtype V4L flags 0
> > > > >             device node name /dev/video1
> > > > >         pad0: Sink
> > > > >                 <- "csi":1 [ENABLED,IMMUTABLE]
> > > > > 
> > > > > - entity 10: imx8mq-mipi-csis.0 (2 pads, 2 links)
> > > > >              type V4L2 subdev subtype Unknown flags 0
> > > > >              device node name /dev/v4l-subdev1
> > > > >         pad0: Sink
> > > > >                 <- "hi846 2-0020":0 []
> > > > >         pad1: Source
> > > > >                 -> "csi":0 [ENABLED,IMMUTABLE]
> > > > 
> > > > This subdev doesn't seem to report formats on its sink and source pads,
> > > > which is weird. I've had a quick look at the .get_fmt() and .set_fmt()
> > > > implementations in the code you've posted, and they're wrong. They
> > > > shouldn't pass the calls to the source subdev with v4l2_subdev_call(),
> > > > they should instead implement get and set format on this subdev. You can
> > > > look at the imx7-mipi-csis driver to see how that's done. Once you'll
> > > > have fixed this, you'll have to set the format on each pad with
> > > > media-ctl to make sure formats through the pipeline match.
> > > > 
> > > > The only location where you imx8mq-mipi-csis driver should use
> > > > v4l2_subdev_call() is in .s_stream(), to propagate the operation to the
> > > > source.
> > > > 
> > > > By the way, I'd replace every occurence of "csis" with "csi2" in your
> > > > driver. The name "csis" in the i.MX7 driver comes from the CSI-2 RX IP
> > > > core that is named CSIS. That's not the case on the i.MX8QM.
> > > > 
> > > > > - entity 15: hi846 2-0020 (1 pad, 1 link)
> > > > >              type V4L2 subdev subtype Sensor flags 0
> > > > >              device node name /dev/v4l-subdev2
> > > > >         pad0: Source
> > > > >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw]
> > > > >                 -> "imx8mq-mipi-csis.0":0 []
> > > > 
> > > > You need to enable this link, the following should do
> > > > 
> > > > media-ctl -l "'hi846 2-0020':0 -> 'imx8mq-mipi-csis.0':0 [1]"
> > > 
> > > ok makes sense, even though I basically just allow a set of formats
> > > without yet having to configure anything format-specific (I can at
> > > least use bits-per-pixel later, so it makes sense to have them).
> > > nevermind. I again append the current driver I use here.
> > > 
> > > then I do:
> > > 
> > > media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480 colorspace:raw]"
> > > media-ctl --set-v4l2 "'imx8mq-mipi-csi2.0':0 [fmt:SGBRG10/640x480
> > > colorspace:raw]"
> > > media-ctl -l "'hi846 2-0020':0 -> 'imx8mq-mipi-csi2.0':0 [1]"
> > > 
> > > which gets me:
> > > 
> > > Device topology
> > > - entity 1: csi (2 pads, 2 links)
> > >             type V4L2 subdev subtype Unknown flags 0
> > >             device node name /dev/v4l-subdev0
> > >         pad0: Sink
> > >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> > >                 <- "imx8mq-mipi-csi2.0":1 [ENABLED,IMMUTABLE]
> > >         pad1: Source
> > >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> > >                 -> "csi capture":0 [ENABLED,IMMUTABLE]
> > > 
> > > - entity 4: csi capture (1 pad, 1 link)
> > >             type Node subtype V4L flags 0
> > >             device node name /dev/video0
> > >         pad0: Sink
> > >                 <- "csi":1 [ENABLED,IMMUTABLE]
> > > 
> > > - entity 10: imx8mq-mipi-csi2.0 (2 pads, 2 links)
> > >              type V4L2 subdev subtype Unknown flags 0
> > >              device node name /dev/v4l-subdev1
> > >         pad0: Sink
> > >                 [fmt:SGBRG10_1X10/640x480]
> > >                 <- "hi846 2-0020":0 [ENABLED]
> > >         pad1: Source
> > >                 [fmt:SGBRG10_1X10/640x480]
> > >                 -> "csi":0 [ENABLED,IMMUTABLE]
> > > 
> > > - entity 15: hi846 2-0020 (1 pad, 1 link)
> > >              type V4L2 subdev subtype Sensor flags 0
> > >              device node name /dev/v4l-subdev2
> > >         pad0: Source
> > >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw]
> > >                 -> "imx8mq-mipi-csi2.0":0 [ENABLED]
> > 
> > This looks better.
> > 
> > > but streaming still fails with:
> > > 
> > > [  352.255129] imx7-csi 30a90000.csi1_bridge: media bus code not
> > > compatible with the pixel format set on the video node: 1 != 0
> > 
> > What is the capture command line ? Can you trace this (I assume the
> > message is printed by capture_validate_fmt(), it's not present in
> > mainline so I don't know what 1 and 0 correspond to, even though I
> > suspect they would be IPUV3_COLORSPACE_* values) to see why it fails
> > ?
> 
> capture command:
> 
> v4l2-ctl -d "/dev/v4l/by-path/platform-30a90000.csi1_bridge-video-
> index0" --set-fmt-video=width=640,height=480 --stream-mmap --stream-
> to=test.raw --stream-count=1
> 
> I'll have to continue after the weekend, but let's share some logs.
> Yes, "1 != 0" is from capture_validate_fmt():
> 
> priv->vdev.cc->cs != cc->cs
> 
> When I print the format the imx_media_find_mbus_format() finds and do:
> 
> media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480 colorspace:raw]"
> 
> I see:
> 
> [  184.251144] mc: media_release: Media Release
> [  184.254397] selected specific mbus code 0 for list nr 0 (fourcc 0x59565955)
> [  184.264564] selected specific mbus code 0 for list nr 0 (fourcc 0x59565955)
> [  184.274763] selected specific mbus code 0 for list nr 21 (fourcc 0x36314247)
> [  184.285102] selected specific mbus code 0 for list nr 21 (fourcc 0x36314247)
> [  184.295383] selected specific mbus code 0 for list nr 21 (fourcc 0x36314247)
> [  184.305752] selected specific mbus code 0 for list nr 21 (fourcc 0x36314247)
> 
> 21 is the correct bayer format I want, but there's 0, so
> "MEDIA_BUS_FMT_UYVY8_2X8" found the first 2 times. That is
> IPUV3_COLORSPACE_YUV (1) while the correct Bayer format 21 is
> IPUV3_COLORSPACE_RGB (0).
> 
> so some format settings not yet correct.

You need to specify the capture pixel format to v4l2-ctl. The driver
defaults to YUYV (I think) otherwise. The CSI bridge will pad data with
0's on the right, so you need SGBRG16 (if I recall correctly, try
SGRBG10 if it doesn't work).

Martin Kepplinger May 25, 2021, 7:32 a.m. UTC | #20

Am Freitag, dem 21.05.2021 um 16:36 +0300 schrieb Laurent Pinchart:
> Hi Martin,
> 
> On Fri, May 21, 2021 at 01:02:30PM +0200, Martin Kepplinger wrote:
> > Am Freitag, dem 21.05.2021 um 12:43 +0300 schrieb Laurent Pinchart:
> > > On Fri, May 21, 2021 at 11:25:20AM +0200, Martin Kepplinger
> > > wrote:
> > > > Am Donnerstag, dem 20.05.2021 um 15:37 +0300 schrieb Laurent
> > > > Pinchart:
> > > > > On Thu, May 20, 2021 at 12:54:27PM +0200, Martin Kepplinger
> > > > > wrote:
> > > > > > Am Mittwoch, dem 19.05.2021 um 18:46 +0300 schrieb Laurent
> > > > > > Pinchart:
> > > > > > > On Wed, May 19, 2021 at 05:21:11PM +0200, Martin
> > > > > > > Kepplinger wrote:
> > > > > > > > Am Mittwoch, dem 19.05.2021 um 04:14 +0300 schrieb
> > > > > > > > Laurent Pinchart:
> > > > > > > > > On Tue, May 18, 2021 at 04:39:00PM +0200, Martin
> > > > > > > > > Kepplinger wrote:
> > > > > > > > > > Am Sonntag, dem 16.05.2021 um 01:55 +0300 schrieb
> > > > > > > > > > Laurent Pinchart:
> > > > > > > > > > > On Tue, May 04, 2021 at 05:59:39PM +0200, Martin
> > > > > > > > > > > Kepplinger wrote:
> > > > > 
> > > > > [snip]
> > > > > 
> > > > > > I fixed mipi -> csi link. I had the DT port descriptions
> > > > > > for
> > > > > > mipi csi wrong.
> > > > > 
> > > > > \o/
> > > > > 
> > > > > > now, just because I think it makes sense, I do:
> > > > > > 
> > > > > > media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480
> > > > > > colorspace:raw]"
> > > > > > 
> > > > > > which now prints:
> > > > > > 
> > > > > > Device topology
> > > > > > - entity 1: csi (2 pads, 2 links)
> > > > > >             type V4L2 subdev subtype Unknown flags 0
> > > > > >             device node name /dev/v4l-subdev0
> > > > > >         pad0: Sink
> > > > > >                 [fmt:SGBRG10_1X10/640x480 field:none
> > > > > > colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> > > > > >                 <- "imx8mq-mipi-csis.0":1
> > > > > > [ENABLED,IMMUTABLE]
> > > > > >         pad1: Source
> > > > > >                 [fmt:SGBRG10_1X10/640x480 field:none
> > > > > > colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> > > > > >                 -> "csi capture":0 [ENABLED,IMMUTABLE]
> > > > > > 
> > > > > > - entity 4: csi capture (1 pad, 1 link)
> > > > > >             type Node subtype V4L flags 0
> > > > > >             device node name /dev/video1
> > > > > >         pad0: Sink
> > > > > >                 <- "csi":1 [ENABLED,IMMUTABLE]
> > > > > > 
> > > > > > - entity 10: imx8mq-mipi-csis.0 (2 pads, 2 links)
> > > > > >              type V4L2 subdev subtype Unknown flags 0
> > > > > >              device node name /dev/v4l-subdev1
> > > > > >         pad0: Sink
> > > > > >                 <- "hi846 2-0020":0 []
> > > > > >         pad1: Source
> > > > > >                 -> "csi":0 [ENABLED,IMMUTABLE]
> > > > > 
> > > > > This subdev doesn't seem to report formats on its sink and
> > > > > source pads,
> > > > > which is weird. I've had a quick look at the .get_fmt() and
> > > > > .set_fmt()
> > > > > implementations in the code you've posted, and they're wrong.
> > > > > They
> > > > > shouldn't pass the calls to the source subdev with
> > > > > v4l2_subdev_call(),
> > > > > they should instead implement get and set format on this
> > > > > subdev. You can
> > > > > look at the imx7-mipi-csis driver to see how that's done.
> > > > > Once you'll
> > > > > have fixed this, you'll have to set the format on each pad
> > > > > with
> > > > > media-ctl to make sure formats through the pipeline match.
> > > > > 
> > > > > The only location where you imx8mq-mipi-csis driver should
> > > > > use
> > > > > v4l2_subdev_call() is in .s_stream(), to propagate the
> > > > > operation to the
> > > > > source.
> > > > > 
> > > > > By the way, I'd replace every occurence of "csis" with "csi2"
> > > > > in your
> > > > > driver. The name "csis" in the i.MX7 driver comes from the
> > > > > CSI-2 RX IP
> > > > > core that is named CSIS. That's not the case on the i.MX8QM.
> > > > > 
> > > > > > - entity 15: hi846 2-0020 (1 pad, 1 link)
> > > > > >              type V4L2 subdev subtype Sensor flags 0
> > > > > >              device node name /dev/v4l-subdev2
> > > > > >         pad0: Source
> > > > > >                 [fmt:SGBRG10_1X10/640x480 field:none
> > > > > > colorspace:raw]
> > > > > >                 -> "imx8mq-mipi-csis.0":0 []
> > > > > 
> > > > > You need to enable this link, the following should do
> > > > > 
> > > > > media-ctl -l "'hi846 2-0020':0 -> 'imx8mq-mipi-csis.0':0 [1]"
> > > > 
> > > > ok makes sense, even though I basically just allow a set of
> > > > formats
> > > > without yet having to configure anything format-specific (I can
> > > > at
> > > > least use bits-per-pixel later, so it makes sense to have
> > > > them).
> > > > nevermind. I again append the current driver I use here.
> > > > 
> > > > then I do:
> > > > 
> > > > media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480
> > > > colorspace:raw]"
> > > > media-ctl --set-v4l2 "'imx8mq-mipi-csi2.0':0
> > > > [fmt:SGBRG10/640x480
> > > > colorspace:raw]"
> > > > media-ctl -l "'hi846 2-0020':0 -> 'imx8mq-mipi-csi2.0':0 [1]"
> > > > 
> > > > which gets me:
> > > > 
> > > > Device topology
> > > > - entity 1: csi (2 pads, 2 links)
> > > >             type V4L2 subdev subtype Unknown flags 0
> > > >             device node name /dev/v4l-subdev0
> > > >         pad0: Sink
> > > >                 [fmt:SGBRG10_1X10/640x480 field:none
> > > > colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> > > >                 <- "imx8mq-mipi-csi2.0":1 [ENABLED,IMMUTABLE]
> > > >         pad1: Source
> > > >                 [fmt:SGBRG10_1X10/640x480 field:none
> > > > colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> > > >                 -> "csi capture":0 [ENABLED,IMMUTABLE]
> > > > 
> > > > - entity 4: csi capture (1 pad, 1 link)
> > > >             type Node subtype V4L flags 0
> > > >             device node name /dev/video0
> > > >         pad0: Sink
> > > >                 <- "csi":1 [ENABLED,IMMUTABLE]
> > > > 
> > > > - entity 10: imx8mq-mipi-csi2.0 (2 pads, 2 links)
> > > >              type V4L2 subdev subtype Unknown flags 0
> > > >              device node name /dev/v4l-subdev1
> > > >         pad0: Sink
> > > >                 [fmt:SGBRG10_1X10/640x480]
> > > >                 <- "hi846 2-0020":0 [ENABLED]
> > > >         pad1: Source
> > > >                 [fmt:SGBRG10_1X10/640x480]
> > > >                 -> "csi":0 [ENABLED,IMMUTABLE]
> > > > 
> > > > - entity 15: hi846 2-0020 (1 pad, 1 link)
> > > >              type V4L2 subdev subtype Sensor flags 0
> > > >              device node name /dev/v4l-subdev2
> > > >         pad0: Source
> > > >                 [fmt:SGBRG10_1X10/640x480 field:none
> > > > colorspace:raw]
> > > >                 -> "imx8mq-mipi-csi2.0":0 [ENABLED]
> > > 
> > > This looks better.
> > > 
> > > > but streaming still fails with:
> > > > 
> > > > [  352.255129] imx7-csi 30a90000.csi1_bridge: media bus code
> > > > not
> > > > compatible with the pixel format set on the video node: 1 != 0
> > > 
> > > What is the capture command line ? Can you trace this (I assume
> > > the
> > > message is printed by capture_validate_fmt(), it's not present in
> > > mainline so I don't know what 1 and 0 correspond to, even though
> > > I
> > > suspect they would be IPUV3_COLORSPACE_* values) to see why it
> > > fails
> > > ?
> > 
> > capture command:
> > 
> > v4l2-ctl -d "/dev/v4l/by-path/platform-30a90000.csi1_bridge-video-
> > index0" --set-fmt-video=width=640,height=480 --stream-mmap --
> > stream-
> > to=test.raw --stream-count=1
> > 
> > I'll have to continue after the weekend, but let's share some logs.
> > Yes, "1 != 0" is from capture_validate_fmt():
> > 
> > priv->vdev.cc->cs != cc->cs
> > 
> > When I print the format the imx_media_find_mbus_format() finds and
> > do:
> > 
> > media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480 colorspace:raw]"
> > 
> > I see:
> > 
> > [  184.251144] mc: media_release: Media Release
> > [  184.254397] selected specific mbus code 0 for list nr 0 (fourcc
> > 0x59565955)
> > [  184.264564] selected specific mbus code 0 for list nr 0 (fourcc
> > 0x59565955)
> > [  184.274763] selected specific mbus code 0 for list nr 21 (fourcc
> > 0x36314247)
> > [  184.285102] selected specific mbus code 0 for list nr 21 (fourcc
> > 0x36314247)
> > [  184.295383] selected specific mbus code 0 for list nr 21 (fourcc
> > 0x36314247)
> > [  184.305752] selected specific mbus code 0 for list nr 21 (fourcc
> > 0x36314247)
> > 
> > 21 is the correct bayer format I want, but there's 0, so
> > "MEDIA_BUS_FMT_UYVY8_2X8" found the first 2 times. That is
> > IPUV3_COLORSPACE_YUV (1) while the correct Bayer format 21 is
> > IPUV3_COLORSPACE_RGB (0).
> > 
> > so some format settings not yet correct.
> 
> You need to specify the capture pixel format to v4l2-ctl. The driver
> defaults to YUYV (I think) otherwise. The CSI bridge will pad data
> with
> 0's on the right, so you need SGBRG16 (if I recall correctly, try
> SGRBG10 if it doesn't work).
> 

right, now with:

v4l2-ctl -d "/dev/v4l/by-path/platform-30a90000.csi1_bridge-video-
index0" --set-fmt-video=width=640,height=480,pixelformat='GB16' --
stream-mmap --stream-to=test.raw --stream-count=1

indeed the colorspace mismatch is gone.

__media_pipeline_start returns broken pipe because of:
link validation failed for 'hi846 2-0020':0 -> 'imx8mq-mipi-csi2.0':0,
error -32:

imx7-csi 30a90000.csi1_bridge: walk: returning entity 'hi846 2-0020'  
imx7-csi 30a90000.csi1_bridge: walk: returning entity 'imx8mq-mipi-
csi2.0'      
hi846 2-0020: hi846: starting hi846_get_format                        
hi846 2-0020: Get format w=640 h=480 code=0x300e colorspace=0xb       
imx7-csi 30a90000.csi1_bridge: v4l2_subdev_link_validate_default: field
does not match (source 1, sink 0)
imx7-csi 30a90000.csi1_bridge: v4l2_subdev_link_validate_default: link
was "hi846 2-0020":0 -> "imx8mq-mipi-csi2.0":0
imx7-csi 30a90000.csi1_bridge: link validation failed for 'hi846 2-
0020':0 -> 'imx8mq-mipi-csi2.0':0, error -32
...
imx7-csi 30a90000.csi1_bridge: pipeline start failed with -32


then I added the init_cfg() callback (that sets sink field to NONE) and
I could stream frames. So the v4l2/mc side of things looks ok, despite
the one thing:

Also, now when saying "--list-formats-ext" on the video node, it lists
all formats that the mipi driver supports (since it doesn't ask the
sensor anymore about that). That seems to be wrong.

Also, the frames are all black, so I'll need to debug the driver side
of things.

But I again append the current driver in case you want to have a look.

thanks so much for walking through this with me :) I won't forget the
dt properties and upstreaming - after I get correct frames.

                          martin

Laurent Pinchart May 25, 2021, 11:25 a.m. UTC | #21

Hi Martin,

On Tue, May 25, 2021 at 09:32:54AM +0200, Martin Kepplinger wrote:
> Am Freitag, dem 21.05.2021 um 16:36 +0300 schrieb Laurent Pinchart:
> > On Fri, May 21, 2021 at 01:02:30PM +0200, Martin Kepplinger wrote:
> > > Am Freitag, dem 21.05.2021 um 12:43 +0300 schrieb Laurent Pinchart:
> > > > On Fri, May 21, 2021 at 11:25:20AM +0200, Martin Kepplinger wrote:

[snip]

> > > > > media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480 colorspace:raw]"
> > > > > media-ctl --set-v4l2 "'imx8mq-mipi-csi2.0':0 [fmt:SGBRG10/640x480 colorspace:raw]"
> > > > > media-ctl -l "'hi846 2-0020':0 -> 'imx8mq-mipi-csi2.0':0 [1]"
> > > > > 
> > > > > which gets me:
> > > > > 
> > > > > Device topology
> > > > > - entity 1: csi (2 pads, 2 links)
> > > > >             type V4L2 subdev subtype Unknown flags 0
> > > > >             device node name /dev/v4l-subdev0
> > > > >         pad0: Sink
> > > > >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> > > > >                 <- "imx8mq-mipi-csi2.0":1 [ENABLED,IMMUTABLE]
> > > > >         pad1: Source
> > > > >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
> > > > >                 -> "csi capture":0 [ENABLED,IMMUTABLE]
> > > > > 
> > > > > - entity 4: csi capture (1 pad, 1 link)
> > > > >             type Node subtype V4L flags 0
> > > > >             device node name /dev/video0
> > > > >         pad0: Sink
> > > > >                 <- "csi":1 [ENABLED,IMMUTABLE]
> > > > > 
> > > > > - entity 10: imx8mq-mipi-csi2.0 (2 pads, 2 links)
> > > > >              type V4L2 subdev subtype Unknown flags 0
> > > > >              device node name /dev/v4l-subdev1
> > > > >         pad0: Sink
> > > > >                 [fmt:SGBRG10_1X10/640x480]
> > > > >                 <- "hi846 2-0020":0 [ENABLED]
> > > > >         pad1: Source
> > > > >                 [fmt:SGBRG10_1X10/640x480]
> > > > >                 -> "csi":0 [ENABLED,IMMUTABLE]
> > > > > 
> > > > > - entity 15: hi846 2-0020 (1 pad, 1 link)
> > > > >              type V4L2 subdev subtype Sensor flags 0
> > > > >              device node name /dev/v4l-subdev2
> > > > >         pad0: Source
> > > > >                 [fmt:SGBRG10_1X10/640x480 field:none colorspace:raw]
> > > > >                 -> "imx8mq-mipi-csi2.0":0 [ENABLED]
> > > > 
> > > > This looks better.
> > > > 
> > > > > but streaming still fails with:
> > > > > 
> > > > > [  352.255129] imx7-csi 30a90000.csi1_bridge: media bus code not
> > > > > compatible with the pixel format set on the video node: 1 != 0
> > > > 
> > > > What is the capture command line ? Can you trace this (I assume the
> > > > message is printed by capture_validate_fmt(), it's not present in
> > > > mainline so I don't know what 1 and 0 correspond to, even though I
> > > > suspect they would be IPUV3_COLORSPACE_* values) to see why it fails
> > > > ?
> > > 
> > > capture command:
> > > 
> > > v4l2-ctl -d "/dev/v4l/by-path/platform-30a90000.csi1_bridge-video-
> > > index0" --set-fmt-video=width=640,height=480 --stream-mmap
> > > --stream-to=test.raw --stream-count=1
> > > 
> > > I'll have to continue after the weekend, but let's share some logs.
> > > Yes, "1 != 0" is from capture_validate_fmt():
> > > 
> > > priv->vdev.cc->cs != cc->cs
> > > 
> > > When I print the format the imx_media_find_mbus_format() finds and
> > > do:
> > > 
> > > media-ctl --set-v4l2 "'csi':0 [fmt:SGBRG10/640x480 colorspace:raw]"
> > > 
> > > I see:
> > > 
> > > [  184.251144] mc: media_release: Media Release
> > > [  184.254397] selected specific mbus code 0 for list nr 0 (fourcc 0x59565955)
> > > [  184.264564] selected specific mbus code 0 for list nr 0 (fourcc 0x59565955)
> > > [  184.274763] selected specific mbus code 0 for list nr 21 (fourcc 0x36314247)
> > > [  184.285102] selected specific mbus code 0 for list nr 21 (fourcc 0x36314247)
> > > [  184.295383] selected specific mbus code 0 for list nr 21 (fourcc 0x36314247)
> > > [  184.305752] selected specific mbus code 0 for list nr 21 (fourcc 0x36314247)
> > > 
> > > 21 is the correct bayer format I want, but there's 0, so
> > > "MEDIA_BUS_FMT_UYVY8_2X8" found the first 2 times. That is
> > > IPUV3_COLORSPACE_YUV (1) while the correct Bayer format 21 is
> > > IPUV3_COLORSPACE_RGB (0).
> > > 
> > > so some format settings not yet correct.
> > 
> > You need to specify the capture pixel format to v4l2-ctl. The driver
> > defaults to YUYV (I think) otherwise. The CSI bridge will pad data with
> > 0's on the right, so you need SGBRG16 (if I recall correctly, try
> > SGRBG10 if it doesn't work).
> 
> right, now with:
> 
> v4l2-ctl -d "/dev/v4l/by-path/platform-30a90000.csi1_bridge-video-
> index0" --set-fmt-video=width=640,height=480,pixelformat='GB16'
> --stream-mmap --stream-to=test.raw --stream-count=1
> 
> indeed the colorspace mismatch is gone.
> 
> __media_pipeline_start returns broken pipe because of:
> link validation failed for 'hi846 2-0020':0 -> 'imx8mq-mipi-csi2.0':0, error -32:
> 
> imx7-csi 30a90000.csi1_bridge: walk: returning entity 'hi846 2-0020'
> imx7-csi 30a90000.csi1_bridge: walk: returning entity 'imx8mq-mipi-csi2.0'
> hi846 2-0020: hi846: starting hi846_get_format
> hi846 2-0020: Get format w=640 h=480 code=0x300e colorspace=0xb
> imx7-csi 30a90000.csi1_bridge: v4l2_subdev_link_validate_default: field does not match (source 1, sink 0)
> imx7-csi 30a90000.csi1_bridge: v4l2_subdev_link_validate_default: link was "hi846 2-0020":0 -> "imx8mq-mipi-csi2.0":0
> imx7-csi 30a90000.csi1_bridge: link validation failed for 'hi846 2-0020':0 -> 'imx8mq-mipi-csi2.0':0, error -32
> ...
> imx7-csi 30a90000.csi1_bridge: pipeline start failed with -32
> 
> 
> then I added the init_cfg() callback (that sets sink field to NONE) and
> I could stream frames.

Nice :-)

> So the v4l2/mc side of things looks ok, despite the one thing:
> 
> Also, now when saying "--list-formats-ext" on the video node, it lists
> all formats that the mipi driver supports (since it doesn't ask the
> sensor anymore about that). That seems to be wrong.

That's expected. You can restrict the enumerated formats to the ones
matching a given media bus code by setting the mbus_code field of struct
v4l2_fmtdesc ([1]).

[1] https://linuxtv.org/downloads/v4l-dvb-apis/userspace-api/v4l/vidioc-enum-fmt.html#c.V4L.v4l2_fmtdesc

> Also, the frames are all black, so I'll need to debug the driver side
> of things.

Indeed :-)

> But I again append the current driver in case you want to have a look.

I'm afraid I don't have time right now, but I'll try to review it when
you'll post the patches.

It'd be curious to try the libcamera simple pipeline handler based on
that code.

> thanks so much for walking through this with me :) I won't forget the
> dt properties and upstreaming - after I get correct frames.

[00/23] media: imx: imx7-mipi-csis: Add i.MX8MM support

Message

Comments