Configure video PAL decoder into media pipeline

Configure video PAL decoder into media pipeline

[Jagan Teki]

Hi,

We have some unconventional setup for parallel CSI design where analog
input data is converted into to digital composite using PAL decoder
and it feed to adv7180, camera sensor.

Analog input =>  Video PAL Decoder => ADV7180 => IPU-CSI0

The PAL decoder is I2C based, tda9885 chip. We setup it up via dt
bindings and the chip is
detected fine.

But we need to know, is this to be part of media control subdev
pipeline? so-that we can configure pads, links like what we do on
conventional pipeline  or it should not to be part of media pipeline?

Please advise for best possible way to fit this into the design.

Another observation is since the IPU has more than one sink, source
pads, we source or sink the other components on the pipeline but look
like the same thing seems not possible with adv7180 since if has only
one pad. If it has only one pad sourcing to adv7180 from tda9885 seems
not possible, If I'm not mistaken.

I tried to look for similar design in mainline, but I couldn't find
it. is there any design similar to this in mainline?

Please let us know if anyone has any suggestions on this.

Jagan.

-- 
Jagan Teki
Senior Linux Kernel Engineer | Amarula Solutions
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

Re: Configure video PAL decoder into media pipeline

[Hans Verkuil]

On 12/07/2018 12:51 PM, Jagan Teki wrote:
> Hi,
> 
> We have some unconventional setup for parallel CSI design where analog
> input data is converted into to digital composite using PAL decoder
> and it feed to adv7180, camera sensor.
> 
> Analog input =>  Video PAL Decoder => ADV7180 => IPU-CSI0

Just PAL? No NTSC support?

> 
> The PAL decoder is I2C based, tda9885 chip. We setup it up via dt
> bindings and the chip is
> detected fine.
> 
> But we need to know, is this to be part of media control subdev
> pipeline? so-that we can configure pads, links like what we do on
> conventional pipeline  or it should not to be part of media pipeline?

Yes, I would say it should be part of the pipeline.

> 
> Please advise for best possible way to fit this into the design.
> 
> Another observation is since the IPU has more than one sink, source
> pads, we source or sink the other components on the pipeline but look
> like the same thing seems not possible with adv7180 since if has only
> one pad. If it has only one pad sourcing to adv7180 from tda9885 seems
> not possible, If I'm not mistaken.

Correct, in all cases where the adv7180 is used it is directly connected
to the video input connector on a board.

So to support this the adv7180 driver should be modified to add an input pad
so you can connect the decoder. It will be needed at some point anyway once
we add support for connector entities.

Regards,

	Hans

> 
> I tried to look for similar design in mainline, but I couldn't find
> it. is there any design similar to this in mainline?
> 
> Please let us know if anyone has any suggestions on this.
> 
> Jagan.
> 

Re: Configure video PAL decoder into media pipeline

[Michael Nazzareno Trimarchi]

Hi

On Fri, Dec 7, 2018 at 1:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 12/07/2018 12:51 PM, Jagan Teki wrote:
> > Hi,
> >
> > We have some unconventional setup for parallel CSI design where analog
> > input data is converted into to digital composite using PAL decoder
> > and it feed to adv7180, camera sensor.
> >
> > Analog input =>  Video PAL Decoder => ADV7180 => IPU-CSI0
>
> Just PAL? No NTSC support?
>
For now does not matter. I have registere the TUNER that support it
but seems that media-ctl is not suppose to work with the MEDIA_ENT_F_TUNER

Is this correct?

> >
> > The PAL decoder is I2C based, tda9885 chip. We setup it up via dt
> > bindings and the chip is
> > detected fine.
> >
> > But we need to know, is this to be part of media control subdev
> > pipeline? so-that we can configure pads, links like what we do on
> > conventional pipeline  or it should not to be part of media pipeline?
>
> Yes, I would say it should be part of the pipeline.
>

Ok I have created a draft patch to add the adv some new endpoint but
is sufficient to declare tuner type in media control?

Michael

> >
> > Please advise for best possible way to fit this into the design.
> >
> > Another observation is since the IPU has more than one sink, source
> > pads, we source or sink the other components on the pipeline but look
> > like the same thing seems not possible with adv7180 since if has only
> > one pad. If it has only one pad sourcing to adv7180 from tda9885 seems
> > not possible, If I'm not mistaken.
>
> Correct, in all cases where the adv7180 is used it is directly connected
> to the video input connector on a board.
>
> So to support this the adv7180 driver should be modified to add an input pad
> so you can connect the decoder. It will be needed at some point anyway once
> we add support for connector entities.
>
> Regards,
>
>         Hans
>
> >
> > I tried to look for similar design in mainline, but I couldn't find
> > it. is there any design similar to this in mainline?
> >
> > Please let us know if anyone has any suggestions on this.
> >
> > Jagan.
> >
>


-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Re: Configure video PAL decoder into media pipeline

[Michael Nazzareno Trimarchi]

Hi

Down you have my tentative of connection

I need to hack a bit to have tuner registered. I'm using imx-media

On Sat, Dec 8, 2018 at 12:48 PM Michael Nazzareno Trimarchi
<michael@amarulasolutions.com> wrote:
>
> Hi
>
> On Fri, Dec 7, 2018 at 1:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> >
> > On 12/07/2018 12:51 PM, Jagan Teki wrote:
> > > Hi,
> > >
> > > We have some unconventional setup for parallel CSI design where analog
> > > input data is converted into to digital composite using PAL decoder
> > > and it feed to adv7180, camera sensor.
> > >
> > > Analog input =>  Video PAL Decoder => ADV7180 => IPU-CSI0
> >
> > Just PAL? No NTSC support?
> >
> For now does not matter. I have registere the TUNER that support it
> but seems that media-ctl is not suppose to work with the MEDIA_ENT_F_TUNER
>
> Is this correct?
>
> > >
> > > The PAL decoder is I2C based, tda9885 chip. We setup it up via dt
> > > bindings and the chip is
> > > detected fine.
> > >
> > > But we need to know, is this to be part of media control subdev
> > > pipeline? so-that we can configure pads, links like what we do on
> > > conventional pipeline  or it should not to be part of media pipeline?
> >
> > Yes, I would say it should be part of the pipeline.
> >
>
> Ok I have created a draft patch to add the adv some new endpoint but
> is sufficient to declare tuner type in media control?
>
> Michael
>
> > >
> > > Please advise for best possible way to fit this into the design.
> > >
> > > Another observation is since the IPU has more than one sink, source
> > > pads, we source or sink the other components on the pipeline but look
> > > like the same thing seems not possible with adv7180 since if has only
> > > one pad. If it has only one pad sourcing to adv7180 from tda9885 seems
> > > not possible, If I'm not mistaken.
> >
> > Correct, in all cases where the adv7180 is used it is directly connected
> > to the video input connector on a board.
> >
> > So to support this the adv7180 driver should be modified to add an input pad
> > so you can connect the decoder. It will be needed at some point anyway once
> > we add support for connector entities.
> >
> > Regards,
> >
> >         Hans
> >
> > >
> > > I tried to look for similar design in mainline, but I couldn't find
> > > it. is there any design similar to this in mainline?
> > >
> > > Please let us know if anyone has any suggestions on this.
> > >

[    3.379129] imx-media: ipu1_vdic:2 -> ipu1_ic_prp:0
[    3.384262] imx-media: ipu2_vdic:2 -> ipu2_ic_prp:0
[    3.389217] imx-media: ipu1_ic_prp:1 -> ipu1_ic_prpenc:0
[    3.394616] imx-media: ipu1_ic_prp:2 -> ipu1_ic_prpvf:0
[    3.399867] imx-media: ipu2_ic_prp:1 -> ipu2_ic_prpenc:0
[    3.405289] imx-media: ipu2_ic_prp:2 -> ipu2_ic_prpvf:0
[    3.410552] imx-media: ipu1_csi0:1 -> ipu1_ic_prp:0
[    3.415502] imx-media: ipu1_csi0:1 -> ipu1_vdic:0
[    3.420305] imx-media: ipu1_csi0_mux:5 -> ipu1_csi0:0
[    3.425427] imx-media: ipu1_csi1:1 -> ipu1_ic_prp:0
[    3.430328] imx-media: ipu1_csi1:1 -> ipu1_vdic:0
[    3.435142] imx-media: ipu1_csi1_mux:5 -> ipu1_csi1:0
[    3.440321] imx-media: adv7180 2-0020:1 -> ipu1_csi0_mux:4

with
       tuner: tuner@43 {
                compatible = "tuner";
                reg = <0x43>;
                pinctrl-names = "default";
                pinctrl-0 = <&pinctrl_tuner>;

                ports {
                        #address-cells = <1>;
                        #size-cells = <0>;
                        port@1 {
                                reg = <1>;

                                tuner_in: endpoint {
                                        remote-endpoint = <&tuner_out>;
                                };
                        };
                };
        };

        adv7180: camera@20 {
                compatible = "adi,adv7180";
                reg = <0x20>;
                pinctrl-names = "default";
                pinctrl-0 = <&pinctrl_adv7180>;
                powerdown-gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; /* PDEC_PWRDN */

                ports {
                        #address-cells = <1>;
                        #size-cells = <0>;

                        port@1 {
                                reg = <1>;

                                adv7180_to_ipu1_csi0_mux: endpoint {
                                        remote-endpoint =
<&ipu1_csi0_mux_from_parallel_sensor>;
                                        bus-width = <8>;
                                };
                        };

                        port@0 {
                                reg = <0>;

                                tuner_out: endpoint {
                                        remote-endpoint = <&tuner_in>;
                                };
                        };
                };
        };

Any help is appreciate

Michael

> > > Jagan.
> > >
> >
>
>
> --

Re: Configure video PAL decoder into media pipeline

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 9711 bytes --]

Hi Michael, Jagan, Hans,

On Sat, Dec 08, 2018 at 06:07:04PM +0100, Michael Nazzareno Trimarchi wrote:
> Hi
>
> Down you have my tentative of connection
>
> I need to hack a bit to have tuner registered. I'm using imx-media
>
> On Sat, Dec 8, 2018 at 12:48 PM Michael Nazzareno Trimarchi
> <michael@amarulasolutions.com> wrote:
> >
> > Hi
> >
> > On Fri, Dec 7, 2018 at 1:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > >
> > > On 12/07/2018 12:51 PM, Jagan Teki wrote:
> > > > Hi,
> > > >
> > > > We have some unconventional setup for parallel CSI design where analog
> > > > input data is converted into to digital composite using PAL decoder
> > > > and it feed to adv7180, camera sensor.
> > > >
> > > > Analog input =>  Video PAL Decoder => ADV7180 => IPU-CSI0
> > >
> > > Just PAL? No NTSC support?
> > >
> > For now does not matter. I have registere the TUNER that support it
> > but seems that media-ctl is not suppose to work with the MEDIA_ENT_F_TUNER
> >
> > Is this correct?
> >

media-types.rst reports:

    *  -  ``MEDIA_ENT_F_IF_VID_DECODER``
       -  IF-PLL video decoder. It receives the IF from a PLL and decodes
	  the analog TV video signal. This is commonly found on some very
	  old analog tuners, like Philips MK3 designs. They all contain a
	  tda9887 (or some software compatible similar chip, like tda9885).
	  Those devices use a different I2C address than the tuner PLL.

Is this what you were looking for?

> > > >
> > > > The PAL decoder is I2C based, tda9885 chip. We setup it up via dt
> > > > bindings and the chip is
> > > > detected fine.
> > > >
> > > > But we need to know, is this to be part of media control subdev
> > > > pipeline? so-that we can configure pads, links like what we do on
> > > > conventional pipeline  or it should not to be part of media pipeline?
> > >
> > > Yes, I would say it should be part of the pipeline.
> > >
> >
> > Ok I have created a draft patch to add the adv some new endpoint but
> > is sufficient to declare tuner type in media control?
> >
> > Michael
> >
> > > >
> > > > Please advise for best possible way to fit this into the design.
> > > >
> > > > Another observation is since the IPU has more than one sink, source
> > > > pads, we source or sink the other components on the pipeline but look
> > > > like the same thing seems not possible with adv7180 since if has only
> > > > one pad. If it has only one pad sourcing to adv7180 from tda9885 seems
> > > > not possible, If I'm not mistaken.
> > >
> > > Correct, in all cases where the adv7180 is used it is directly connected
> > > to the video input connector on a board.
> > >
> > > So to support this the adv7180 driver should be modified to add an input pad
> > > so you can connect the decoder. It will be needed at some point anyway once
> > > we add support for connector entities.
> > >
> > > Regards,
> > >
> > >         Hans
> > >
> > > >
> > > > I tried to look for similar design in mainline, but I couldn't find
> > > > it. is there any design similar to this in mainline?
> > > >
> > > > Please let us know if anyone has any suggestions on this.
> > > >
>
> [    3.379129] imx-media: ipu1_vdic:2 -> ipu1_ic_prp:0
> [    3.384262] imx-media: ipu2_vdic:2 -> ipu2_ic_prp:0
> [    3.389217] imx-media: ipu1_ic_prp:1 -> ipu1_ic_prpenc:0
> [    3.394616] imx-media: ipu1_ic_prp:2 -> ipu1_ic_prpvf:0
> [    3.399867] imx-media: ipu2_ic_prp:1 -> ipu2_ic_prpenc:0
> [    3.405289] imx-media: ipu2_ic_prp:2 -> ipu2_ic_prpvf:0
> [    3.410552] imx-media: ipu1_csi0:1 -> ipu1_ic_prp:0
> [    3.415502] imx-media: ipu1_csi0:1 -> ipu1_vdic:0
> [    3.420305] imx-media: ipu1_csi0_mux:5 -> ipu1_csi0:0
> [    3.425427] imx-media: ipu1_csi1:1 -> ipu1_ic_prp:0
> [    3.430328] imx-media: ipu1_csi1:1 -> ipu1_vdic:0
> [    3.435142] imx-media: ipu1_csi1_mux:5 -> ipu1_csi1:0
> [    3.440321] imx-media: adv7180 2-0020:1 -> ipu1_csi0_mux:4
>
> with
>        tuner: tuner@43 {
>                 compatible = "tuner";
>                 reg = <0x43>;
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&pinctrl_tuner>;
>
>                 ports {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>                         port@1 {
>                                 reg = <1>;
>
>                                 tuner_in: endpoint {

Nit: This is the tuner output, I would call this "tuner_out"

>                                         remote-endpoint = <&tuner_out>;
>                                 };
>                         };
>                 };
>         };
>
>         adv7180: camera@20 {
>                 compatible = "adi,adv7180";

One minor thing first: look at the adv7180 bindings documentation, and
you'll find out that only devices compatible with "adv7180cp" and
"adv7180st" shall declare a 'ports' node. This is minor issues (also,
I don't see it enforced in driver's code, but still worth pointing it
out from the very beginning)

>                 reg = <0x20>;
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&pinctrl_adv7180>;
>                 powerdown-gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; /* PDEC_PWRDN */
>
>                 ports {
>                         #address-cells = <1>;
>                         #size-cells = <0>;
>
>                         port@1 {
>                                 reg = <1>;
>
>                                 adv7180_to_ipu1_csi0_mux: endpoint {
>                                         remote-endpoint =
> <&ipu1_csi0_mux_from_parallel_sensor>;
>                                         bus-width = <8>;
>                                 };
>                         };
>
>                         port@0 {
>                                 reg = <0>;
>
>                                 tuner_out: endpoint {

Nit: That's an adv7180 endpoint, I would call it 'adv7180_in'

>                                         remote-endpoint = <&tuner_in>;
>                                 };
>                         };
>                 };
>         };
>
> Any help is appreciate
>

The adv7180(cp|st) bindings says the device can declare one (or more)
input endpoints, but that's just to make possible to connect in device
tree the 7180's device node with the video input port. You can see an
example in arch/arm64/boot/dts/renesas/r8a77995-draak.dts which is
similar to what you've done here.

The video input port does not show up in the media graph, as it is
just a 'place holder' to describe an input port in DTs, the
adv7180 driver does not register any sink pad, where to connect any
video source to.

Your proposed bindings here look almost correct, but to have it
working for real you should add a sink pad to the adv7180 registered
media entity (possibly only conditionally to the presence of an input
endpoint in DTs...). You should then register a subdev-notifier, which
registers on an async-subdevice that uses the remote endpoint
connected to your newly registered input pad to find out which device
you're linked to; then at 'bound' (or possibly 'complete') time
register a link between the two entities, on which you can operate on
from userspace.

Your tuner driver for tda9885 (which I don't see in mainline, so I
assume it's downstream or custom code) should register an async subdevice,
so that the adv7180 registered subdev-notifier gets matched and your
callbacks invoked.

If I were you, I would:
1) Add dt-parsing routine to tda7180, to find out if any input
endpoint is registered in DT.
2) If it is, then register a SINK pad, along with the single SOURCE pad
which is registered today.
3) When parsing DT, for input endpoints, get a reference to the remote
endpoint connected to your input and register a subdev-notifier
4) Fill in the notifier 'bound' callback and register the link to
your remote device there.
5) Make sure your tuner driver registers its subdevice with
v4l2_async_register_subdev()

A good example on how to register subdev notifier is provided in the
rcsi2_parse_dt() function in rcar-csi2.c driver (I'm quite sure imx
now uses subdev notifiers from v4.19 on too if you want to have a look
there).

-- Entering slippery territory here: you might want more inputs on this

To make thing simpler&nicer (TM), if you blindly do what I've suggested
here, you're going to break all current adv7180 users in mainline :(

That's because the v4l2-async design 'completes' the root notifier,
only if all subdev-notifiers connected to it have completed first.
If you add a notifier for the adv7180 input ports unconditionally, and
to the input port is connected a plain simple "composite-video-connector",
as all DTs in mainline are doing right now, the newly registered
subdev-notifier will never complete, as the "composite-video-connector"
does not register any subdevice to match with. Bummer!

A quick look in the code base, returns me that, in example:
drivers/gpu/drm/meson/meson_drv.c filters on
"composite-video-connector" and a few other compatible values. You
might want to do the same, and register a notifier if and only if the
remote input endpoint is one of those known not to register a
subdevice. I'm sure there are other ways to deal with this issue, but
that's the best I can come up with...
---

Hope this is reasonably clear and that I'm not misleading you. I had to
use adv7180 recently, and its single pad design confused me a bit as well :)

Thanks
  j

> Michael
>
> > > > Jagan.
> > > >
> > >
> >
> >
> > --

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Configure video PAL decoder into media pipeline

[Michael Nazzareno Trimarchi]

Hi

On Sun, Dec 9, 2018 at 8:39 PM jacopo mondi <jacopo@jmondi.org> wrote:
>
> Hi Michael, Jagan, Hans,
>
> On Sat, Dec 08, 2018 at 06:07:04PM +0100, Michael Nazzareno Trimarchi wrote:
> > Hi
> >
> > Down you have my tentative of connection
> >
> > I need to hack a bit to have tuner registered. I'm using imx-media
> >
> > On Sat, Dec 8, 2018 at 12:48 PM Michael Nazzareno Trimarchi
> > <michael@amarulasolutions.com> wrote:
> > >
> > > Hi
> > >
> > > On Fri, Dec 7, 2018 at 1:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > >
> > > > On 12/07/2018 12:51 PM, Jagan Teki wrote:
> > > > > Hi,
> > > > >
> > > > > We have some unconventional setup for parallel CSI design where analog
> > > > > input data is converted into to digital composite using PAL decoder
> > > > > and it feed to adv7180, camera sensor.
> > > > >
> > > > > Analog input =>  Video PAL Decoder => ADV7180 => IPU-CSI0
> > > >
> > > > Just PAL? No NTSC support?
> > > >
> > > For now does not matter. I have registere the TUNER that support it
> > > but seems that media-ctl is not suppose to work with the MEDIA_ENT_F_TUNER
> > >
> > > Is this correct?
> > >
>
> media-types.rst reports:
>
>     *  -  ``MEDIA_ENT_F_IF_VID_DECODER``
>        -  IF-PLL video decoder. It receives the IF from a PLL and decodes
>           the analog TV video signal. This is commonly found on some very
>           old analog tuners, like Philips MK3 designs. They all contain a
>           tda9887 (or some software compatible similar chip, like tda9885).
>           Those devices use a different I2C address than the tuner PLL.
>
> Is this what you were looking for?
>
> > > > >
> > > > > The PAL decoder is I2C based, tda9885 chip. We setup it up via dt
> > > > > bindings and the chip is
> > > > > detected fine.
> > > > >
> > > > > But we need to know, is this to be part of media control subdev
> > > > > pipeline? so-that we can configure pads, links like what we do on
> > > > > conventional pipeline  or it should not to be part of media pipeline?
> > > >
> > > > Yes, I would say it should be part of the pipeline.
> > > >
> > >
> > > Ok I have created a draft patch to add the adv some new endpoint but
> > > is sufficient to declare tuner type in media control?
> > >
> > > Michael
> > >
> > > > >
> > > > > Please advise for best possible way to fit this into the design.
> > > > >
> > > > > Another observation is since the IPU has more than one sink, source
> > > > > pads, we source or sink the other components on the pipeline but look
> > > > > like the same thing seems not possible with adv7180 since if has only
> > > > > one pad. If it has only one pad sourcing to adv7180 from tda9885 seems
> > > > > not possible, If I'm not mistaken.
> > > >
> > > > Correct, in all cases where the adv7180 is used it is directly connected
> > > > to the video input connector on a board.
> > > >
> > > > So to support this the adv7180 driver should be modified to add an input pad
> > > > so you can connect the decoder. It will be needed at some point anyway once
> > > > we add support for connector entities.
> > > >
> > > > Regards,
> > > >
> > > >         Hans
> > > >
> > > > >
> > > > > I tried to look for similar design in mainline, but I couldn't find
> > > > > it. is there any design similar to this in mainline?
> > > > >
> > > > > Please let us know if anyone has any suggestions on this.
> > > > >
> >
> > [    3.379129] imx-media: ipu1_vdic:2 -> ipu1_ic_prp:0
> > [    3.384262] imx-media: ipu2_vdic:2 -> ipu2_ic_prp:0
> > [    3.389217] imx-media: ipu1_ic_prp:1 -> ipu1_ic_prpenc:0
> > [    3.394616] imx-media: ipu1_ic_prp:2 -> ipu1_ic_prpvf:0
> > [    3.399867] imx-media: ipu2_ic_prp:1 -> ipu2_ic_prpenc:0
> > [    3.405289] imx-media: ipu2_ic_prp:2 -> ipu2_ic_prpvf:0
> > [    3.410552] imx-media: ipu1_csi0:1 -> ipu1_ic_prp:0
> > [    3.415502] imx-media: ipu1_csi0:1 -> ipu1_vdic:0
> > [    3.420305] imx-media: ipu1_csi0_mux:5 -> ipu1_csi0:0
> > [    3.425427] imx-media: ipu1_csi1:1 -> ipu1_ic_prp:0
> > [    3.430328] imx-media: ipu1_csi1:1 -> ipu1_vdic:0
> > [    3.435142] imx-media: ipu1_csi1_mux:5 -> ipu1_csi1:0
> > [    3.440321] imx-media: adv7180 2-0020:1 -> ipu1_csi0_mux:4
> >
> > with
> >        tuner: tuner@43 {
> >                 compatible = "tuner";
> >                 reg = <0x43>;
> >                 pinctrl-names = "default";
> >                 pinctrl-0 = <&pinctrl_tuner>;
> >
> >                 ports {
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> >                         port@1 {
> >                                 reg = <1>;
> >
> >                                 tuner_in: endpoint {
>
> Nit: This is the tuner output, I would call this "tuner_out"
>
> >                                         remote-endpoint = <&tuner_out>;
> >                                 };
> >                         };
> >                 };
> >         };
> >
> >         adv7180: camera@20 {
> >                 compatible = "adi,adv7180";
>
> One minor thing first: look at the adv7180 bindings documentation, and
> you'll find out that only devices compatible with "adv7180cp" and
> "adv7180st" shall declare a 'ports' node. This is minor issues (also,
> I don't see it enforced in driver's code, but still worth pointing it
> out from the very beginning)

Ok

>
> >                 reg = <0x20>;
> >                 pinctrl-names = "default";
> >                 pinctrl-0 = <&pinctrl_adv7180>;
> >                 powerdown-gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; /* PDEC_PWRDN */
> >
> >                 ports {
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> >
> >                         port@1 {
> >                                 reg = <1>;
> >
> >                                 adv7180_to_ipu1_csi0_mux: endpoint {
> >                                         remote-endpoint =
> > <&ipu1_csi0_mux_from_parallel_sensor>;
> >                                         bus-width = <8>;
> >                                 };
> >                         };
> >
> >                         port@0 {
> >                                 reg = <0>;
> >
> >                                 tuner_out: endpoint {
>
> Nit: That's an adv7180 endpoint, I would call it 'adv7180_in'
>
> >                                         remote-endpoint = <&tuner_in>;
> >                                 };
> >                         };
> >                 };
> >         };
> >
> > Any help is appreciate
> >
>
> The adv7180(cp|st) bindings says the device can declare one (or more)
> input endpoints, but that's just to make possible to connect in device
> tree the 7180's device node with the video input port. You can see an
> example in arch/arm64/boot/dts/renesas/r8a77995-draak.dts which is
> similar to what you've done here.
>
> The video input port does not show up in the media graph, as it is
> just a 'place holder' to describe an input port in DTs, the
> adv7180 driver does not register any sink pad, where to connect any
> video source to.
>
> Your proposed bindings here look almost correct, but to have it
> working for real you should add a sink pad to the adv7180 registered
> media entity (possibly only conditionally to the presence of an input
> endpoint in DTs...). You should then register a subdev-notifier, which

--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -190,6 +190,12 @@ struct adv7180_state;
 #define ADV7180_FLAG_MIPI_CSI2         BIT(2)
 #define ADV7180_FLAG_I2P               BIT(3)

+enum adv7180_pads {
+       ADV7180_PAD_IF_INPUT,
+       ADV7180_PAD_VID_OUT,
+       ADV7180_NUM_PADS
+};
+
 struct adv7180_chip_info {
        unsigned int flags;
        unsigned int valid_input_mask;
@@ -201,7 +207,7 @@ struct adv7180_chip_info {
 struct adv7180_state {
        struct v4l2_ctrl_handler ctrl_hdl;
        struct v4l2_subdev      sd;
-       struct media_pad        pad;
+       struct media_pad        pad[ADV7180_NUM_PADS];
        struct mutex            mutex; /* mutual excl. when accessing chip */
        int                     irq;
        struct gpio_desc        *pwdn_gpio;
@@ -1360,9 +1366,12 @@ static int adv7180_probe(struct i2c_client *client,
        if (ret)
                goto err_unregister_vpp_client;

-       state->pad.flags = MEDIA_PAD_FL_SOURCE;
+       state->pad[ADV7180_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
+       state->pad[ADV7180_PAD_IF_INPUT].sig_type = PAD_SIGNAL_ANALOG;
+       state->pad[ADV7180_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
+       state->pad[ADV7180_PAD_VID_OUT].sig_type = PAD_SIGNAL_DV;
        sd->entity.function = MEDIA_ENT_F_ATV_DECODER;
-       ret = media_entity_pads_init(&sd->entity, 1, &state->pad);
+       ret = media_entity_pads_init(&sd->entity, ADV7180_NUM_PADS, state->pad);
        if (ret)
                goto err_free_ctrl;

> registers on an async-subdevice that uses the remote endpoint
> connected to your newly registered input pad to find out which device
> you're linked to; then at 'bound' (or possibly 'complete') time
> register a link between the two entities, on which you can operate on
> from userspace.
>
> Your tuner driver for tda9885 (which I don't see in mainline, so I
> assume it's downstream or custom code) should register an async subdevice,

tda988x is on mainline. Now I need to force somenthing in the config to have
registered as a subdev

Michael

> so that the adv7180 registered subdev-notifier gets matched and your
> callbacks invoked.
>
> If I were you, I would:
> 1) Add dt-parsing routine to tda7180, to find out if any input
> endpoint is registered in DT.
> 2) If it is, then register a SINK pad, along with the single SOURCE pad
> which is registered today.
> 3) When parsing DT, for input endpoints, get a reference to the remote
> endpoint connected to your input and register a subdev-notifier
> 4) Fill in the notifier 'bound' callback and register the link to
> your remote device there.
> 5) Make sure your tuner driver registers its subdevice with
> v4l2_async_register_subdev()
>
> A good example on how to register subdev notifier is provided in the
> rcsi2_parse_dt() function in rcar-csi2.c driver (I'm quite sure imx
> now uses subdev notifiers from v4.19 on too if you want to have a look
> there).
>
> -- Entering slippery territory here: you might want more inputs on this
>
> To make thing simpler&nicer (TM), if you blindly do what I've suggested
> here, you're going to break all current adv7180 users in mainline :(
>
> That's because the v4l2-async design 'completes' the root notifier,
> only if all subdev-notifiers connected to it have completed first.
> If you add a notifier for the adv7180 input ports unconditionally, and
> to the input port is connected a plain simple "composite-video-connector",
> as all DTs in mainline are doing right now, the newly registered
> subdev-notifier will never complete, as the "composite-video-connector"
> does not register any subdevice to match with. Bummer!
>
> A quick look in the code base, returns me that, in example:
> drivers/gpu/drm/meson/meson_drv.c filters on
> "composite-video-connector" and a few other compatible values. You
> might want to do the same, and register a notifier if and only if the
> remote input endpoint is one of those known not to register a
> subdevice. I'm sure there are other ways to deal with this issue, but
> that's the best I can come up with...
> ---
>
> Hope this is reasonably clear and that I'm not misleading you. I had to
> use adv7180 recently, and its single pad design confused me a bit as well :)
>
> Thanks
>   j
>
> > Michael
> >
> > > > > Jagan.
> > > > >
> > > >
> > >
> > >
> > > --



--
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Re: Configure video PAL decoder into media pipeline

[Michael Nazzareno Trimarchi]

Hi Jacopo

Let's see what I have done

On Sun, Dec 9, 2018 at 8:39 PM jacopo mondi <jacopo@jmondi.org> wrote:
>
> Hi Michael, Jagan, Hans,
>
> On Sat, Dec 08, 2018 at 06:07:04PM +0100, Michael Nazzareno Trimarchi wrote:
> > Hi
> >
> > Down you have my tentative of connection
> >
> > I need to hack a bit to have tuner registered. I'm using imx-media
> >
> > On Sat, Dec 8, 2018 at 12:48 PM Michael Nazzareno Trimarchi
> > <michael@amarulasolutions.com> wrote:
> > >
> > > Hi
> > >
> > > On Fri, Dec 7, 2018 at 1:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > >
> > > > On 12/07/2018 12:51 PM, Jagan Teki wrote:
> > > > > Hi,
> > > > >
> > > > > We have some unconventional setup for parallel CSI design where analog
> > > > > input data is converted into to digital composite using PAL decoder
> > > > > and it feed to adv7180, camera sensor.
> > > > >
> > > > > Analog input =>  Video PAL Decoder => ADV7180 => IPU-CSI0
> > > >
> > > > Just PAL? No NTSC support?
> > > >
> > > For now does not matter. I have registere the TUNER that support it
> > > but seems that media-ctl is not suppose to work with the MEDIA_ENT_F_TUNER
> > >
> > > Is this correct?
> > >
>
> media-types.rst reports:
>
>     *  -  ``MEDIA_ENT_F_IF_VID_DECODER``
>        -  IF-PLL video decoder. It receives the IF from a PLL and decodes
>           the analog TV video signal. This is commonly found on some very
>           old analog tuners, like Philips MK3 designs. They all contain a
>           tda9887 (or some software compatible similar chip, like tda9885).
>           Those devices use a different I2C address than the tuner PLL.
>
> Is this what you were looking for?
>
> > > > >
> > > > > The PAL decoder is I2C based, tda9885 chip. We setup it up via dt
> > > > > bindings and the chip is
> > > > > detected fine.
> > > > >
> > > > > But we need to know, is this to be part of media control subdev
> > > > > pipeline? so-that we can configure pads, links like what we do on
> > > > > conventional pipeline  or it should not to be part of media pipeline?
> > > >
> > > > Yes, I would say it should be part of the pipeline.
> > > >
> > >
> > > Ok I have created a draft patch to add the adv some new endpoint but
> > > is sufficient to declare tuner type in media control?
> > >
> > > Michael
> > >
> > > > >
> > > > > Please advise for best possible way to fit this into the design.
> > > > >
> > > > > Another observation is since the IPU has more than one sink, source
> > > > > pads, we source or sink the other components on the pipeline but look
> > > > > like the same thing seems not possible with adv7180 since if has only
> > > > > one pad. If it has only one pad sourcing to adv7180 from tda9885 seems
> > > > > not possible, If I'm not mistaken.
> > > >
> > > > Correct, in all cases where the adv7180 is used it is directly connected
> > > > to the video input connector on a board.
> > > >
> > > > So to support this the adv7180 driver should be modified to add an input pad
> > > > so you can connect the decoder. It will be needed at some point anyway once
> > > > we add support for connector entities.
> > > >
> > > > Regards,
> > > >
> > > >         Hans
> > > >
> > > > >
> > > > > I tried to look for similar design in mainline, but I couldn't find
> > > > > it. is there any design similar to this in mainline?
> > > > >
> > > > > Please let us know if anyone has any suggestions on this.
> > > > >
> >
> > [    3.379129] imx-media: ipu1_vdic:2 -> ipu1_ic_prp:0
> > [    3.384262] imx-media: ipu2_vdic:2 -> ipu2_ic_prp:0
> > [    3.389217] imx-media: ipu1_ic_prp:1 -> ipu1_ic_prpenc:0
> > [    3.394616] imx-media: ipu1_ic_prp:2 -> ipu1_ic_prpvf:0
> > [    3.399867] imx-media: ipu2_ic_prp:1 -> ipu2_ic_prpenc:0
> > [    3.405289] imx-media: ipu2_ic_prp:2 -> ipu2_ic_prpvf:0
> > [    3.410552] imx-media: ipu1_csi0:1 -> ipu1_ic_prp:0
> > [    3.415502] imx-media: ipu1_csi0:1 -> ipu1_vdic:0
> > [    3.420305] imx-media: ipu1_csi0_mux:5 -> ipu1_csi0:0
> > [    3.425427] imx-media: ipu1_csi1:1 -> ipu1_ic_prp:0
> > [    3.430328] imx-media: ipu1_csi1:1 -> ipu1_vdic:0
> > [    3.435142] imx-media: ipu1_csi1_mux:5 -> ipu1_csi1:0
> > [    3.440321] imx-media: adv7180 2-0020:1 -> ipu1_csi0_mux:4
> >
> > with
> >        tuner: tuner@43 {
> >                 compatible = "tuner";
> >                 reg = <0x43>;
> >                 pinctrl-names = "default";
> >                 pinctrl-0 = <&pinctrl_tuner>;
> >
> >                 ports {
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> >                         port@1 {
> >                                 reg = <1>;
> >
> >                                 tuner_in: endpoint {
>
> Nit: This is the tuner output, I would call this "tuner_out"
>

Done

> >                                         remote-endpoint = <&tuner_out>;
> >                                 };
> >                         };
> >                 };
> >         };
> >
> >         adv7180: camera@20 {
> >                 compatible = "adi,adv7180";
>
> One minor thing first: look at the adv7180 bindings documentation, and
> you'll find out that only devices compatible with "adv7180cp" and
> "adv7180st" shall declare a 'ports' node. This is minor issues (also,
> I don't see it enforced in driver's code, but still worth pointing it
> out from the very beginning)
>
> >                 reg = <0x20>;
> >                 pinctrl-names = "default";
> >                 pinctrl-0 = <&pinctrl_adv7180>;
> >                 powerdown-gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; /* PDEC_PWRDN */
> >
> >                 ports {
> >                         #address-cells = <1>;
> >                         #size-cells = <0>;
> >
> >                         port@1 {
> >                                 reg = <1>;
> >
> >                                 adv7180_to_ipu1_csi0_mux: endpoint {
> >                                         remote-endpoint =
> > <&ipu1_csi0_mux_from_parallel_sensor>;
> >                                         bus-width = <8>;
> >                                 };
> >                         };
> >
> >                         port@0 {
> >                                 reg = <0>;
> >
> >                                 tuner_out: endpoint {
>
> Nit: That's an adv7180 endpoint, I would call it 'adv7180_in'
>

Done

> >                                         remote-endpoint = <&tuner_in>;
> >                                 };
> >                         };
> >                 };
> >         };
> >
> > Any help is appreciate
> >
>
> The adv7180(cp|st) bindings says the device can declare one (or more)
> input endpoints, but that's just to make possible to connect in device
> tree the 7180's device node with the video input port. You can see an
> example in arch/arm64/boot/dts/renesas/r8a77995-draak.dts which is
> similar to what you've done here.
>
> The video input port does not show up in the media graph, as it is
> just a 'place holder' to describe an input port in DTs, the
> adv7180 driver does not register any sink pad, where to connect any
> video source to.
>
> Your proposed bindings here look almost correct, but to have it
> working for real you should add a sink pad to the adv7180 registered
> media entity (possibly only conditionally to the presence of an input
> endpoint in DTs...). You should then register a subdev-notifier, which
> registers on an async-subdevice that uses the remote endpoint
> connected to your newly registered input pad to find out which device
> you're linked to; then at 'bound' (or possibly 'complete') time
> register a link between the two entities, on which you can operate on
> from userspace.
>
> Your tuner driver for tda9885 (which I don't see in mainline, so I
> assume it's downstream or custom code) should register an async subdevice,
> so that the adv7180 registered subdev-notifier gets matched and your
> callbacks invoked.
>
> If I were you, I would:
> 1) Add dt-parsing routine to tda7180, to find out if any input
> endpoint is registered in DT.

Done

> 2) If it is, then register a SINK pad, along with the single SOURCE pad
> which is registered today.

Done

> 3) When parsing DT, for input endpoints, get a reference to the remote
> endpoint connected to your input and register a subdev-notifier

Done

> 4) Fill in the notifier 'bound' callback and register the link to
> your remote device there.

Both are subdevice that has not a v4l2_device, so bound is not called from two
sub-device. Is this expected?


> 5) Make sure your tuner driver registers its subdevice with
> v4l2_async_register_subdev()
>
> A good example on how to register subdev notifier is provided in the
> rcsi2_parse_dt() function in rcar-csi2.c driver (I'm quite sure imx
> now uses subdev notifiers from v4.19 on too if you want to have a look
> there).

Already seen it

>
> -- Entering slippery territory here: you might want more inputs on this
>
> To make thing simpler&nicer (TM), if you blindly do what I've suggested
> here, you're going to break all current adv7180 users in mainline :(
>
> That's because the v4l2-async design 'completes' the root notifier,
> only if all subdev-notifiers connected to it have completed first.
> If you add a notifier for the adv7180 input ports unconditionally, and

I don't get to complete. So let's proceed by step

Michael

> to the input port is connected a plain simple "composite-video-connector",
> as all DTs in mainline are doing right now, the newly registered
> subdev-notifier will never complete, as the "composite-video-connector"
> does not register any subdevice to match with. Bummer!
>
> A quick look in the code base, returns me that, in example:
> drivers/gpu/drm/meson/meson_drv.c filters on
> "composite-video-connector" and a few other compatible values. You
> might want to do the same, and register a notifier if and only if the
> remote input endpoint is one of those known not to register a
> subdevice. I'm sure there are other ways to deal with this issue, but
> that's the best I can come up with...
> ---
>
> Hope this is reasonably clear and that I'm not misleading you. I had to
> use adv7180 recently, and its single pad design confused me a bit as well :)
>
> Thanks
>   j
>
> > Michael
> >
> > > > > Jagan.
> > > > >
> > > >
> > >
> > >
> > > --



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Re: Configure video PAL decoder into media pipeline

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 11822 bytes --]

Hi Michael,

On Mon, Dec 10, 2018 at 10:45:02PM +0100, Michael Nazzareno Trimarchi wrote:
> Hi Jacopo
>
> Let's see what I have done
>
> On Sun, Dec 9, 2018 at 8:39 PM jacopo mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Michael, Jagan, Hans,
> >
> > On Sat, Dec 08, 2018 at 06:07:04PM +0100, Michael Nazzareno Trimarchi wrote:
> > > Hi
> > >
> > > Down you have my tentative of connection
> > >
> > > I need to hack a bit to have tuner registered. I'm using imx-media
> > >
> > > On Sat, Dec 8, 2018 at 12:48 PM Michael Nazzareno Trimarchi
> > > <michael@amarulasolutions.com> wrote:
> > > >
> > > > Hi
> > > >
> > > > On Fri, Dec 7, 2018 at 1:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > > >
> > > > > On 12/07/2018 12:51 PM, Jagan Teki wrote:
> > > > > > Hi,
> > > > > >
> > > > > > We have some unconventional setup for parallel CSI design where analog
> > > > > > input data is converted into to digital composite using PAL decoder
> > > > > > and it feed to adv7180, camera sensor.
> > > > > >
> > > > > > Analog input =>  Video PAL Decoder => ADV7180 => IPU-CSI0
> > > > >
> > > > > Just PAL? No NTSC support?
> > > > >
> > > > For now does not matter. I have registere the TUNER that support it
> > > > but seems that media-ctl is not suppose to work with the MEDIA_ENT_F_TUNER
> > > >
> > > > Is this correct?
> > > >
> >
> > media-types.rst reports:
> >
> >     *  -  ``MEDIA_ENT_F_IF_VID_DECODER``
> >        -  IF-PLL video decoder. It receives the IF from a PLL and decodes
> >           the analog TV video signal. This is commonly found on some very
> >           old analog tuners, like Philips MK3 designs. They all contain a
> >           tda9887 (or some software compatible similar chip, like tda9885).
> >           Those devices use a different I2C address than the tuner PLL.
> >
> > Is this what you were looking for?
> >
> > > > > >
> > > > > > The PAL decoder is I2C based, tda9885 chip. We setup it up via dt
> > > > > > bindings and the chip is
> > > > > > detected fine.
> > > > > >
> > > > > > But we need to know, is this to be part of media control subdev
> > > > > > pipeline? so-that we can configure pads, links like what we do on
> > > > > > conventional pipeline  or it should not to be part of media pipeline?
> > > > >
> > > > > Yes, I would say it should be part of the pipeline.
> > > > >
> > > >
> > > > Ok I have created a draft patch to add the adv some new endpoint but
> > > > is sufficient to declare tuner type in media control?
> > > >
> > > > Michael
> > > >
> > > > > >
> > > > > > Please advise for best possible way to fit this into the design.
> > > > > >
> > > > > > Another observation is since the IPU has more than one sink, source
> > > > > > pads, we source or sink the other components on the pipeline but look
> > > > > > like the same thing seems not possible with adv7180 since if has only
> > > > > > one pad. If it has only one pad sourcing to adv7180 from tda9885 seems
> > > > > > not possible, If I'm not mistaken.
> > > > >
> > > > > Correct, in all cases where the adv7180 is used it is directly connected
> > > > > to the video input connector on a board.
> > > > >
> > > > > So to support this the adv7180 driver should be modified to add an input pad
> > > > > so you can connect the decoder. It will be needed at some point anyway once
> > > > > we add support for connector entities.
> > > > >
> > > > > Regards,
> > > > >
> > > > >         Hans
> > > > >
> > > > > >
> > > > > > I tried to look for similar design in mainline, but I couldn't find
> > > > > > it. is there any design similar to this in mainline?
> > > > > >
> > > > > > Please let us know if anyone has any suggestions on this.
> > > > > >
> > >
> > > [    3.379129] imx-media: ipu1_vdic:2 -> ipu1_ic_prp:0
> > > [    3.384262] imx-media: ipu2_vdic:2 -> ipu2_ic_prp:0
> > > [    3.389217] imx-media: ipu1_ic_prp:1 -> ipu1_ic_prpenc:0
> > > [    3.394616] imx-media: ipu1_ic_prp:2 -> ipu1_ic_prpvf:0
> > > [    3.399867] imx-media: ipu2_ic_prp:1 -> ipu2_ic_prpenc:0
> > > [    3.405289] imx-media: ipu2_ic_prp:2 -> ipu2_ic_prpvf:0
> > > [    3.410552] imx-media: ipu1_csi0:1 -> ipu1_ic_prp:0
> > > [    3.415502] imx-media: ipu1_csi0:1 -> ipu1_vdic:0
> > > [    3.420305] imx-media: ipu1_csi0_mux:5 -> ipu1_csi0:0
> > > [    3.425427] imx-media: ipu1_csi1:1 -> ipu1_ic_prp:0
> > > [    3.430328] imx-media: ipu1_csi1:1 -> ipu1_vdic:0
> > > [    3.435142] imx-media: ipu1_csi1_mux:5 -> ipu1_csi1:0
> > > [    3.440321] imx-media: adv7180 2-0020:1 -> ipu1_csi0_mux:4
> > >
> > > with
> > >        tuner: tuner@43 {
> > >                 compatible = "tuner";
> > >                 reg = <0x43>;
> > >                 pinctrl-names = "default";
> > >                 pinctrl-0 = <&pinctrl_tuner>;
> > >
> > >                 ports {
> > >                         #address-cells = <1>;
> > >                         #size-cells = <0>;
> > >                         port@1 {
> > >                                 reg = <1>;
> > >
> > >                                 tuner_in: endpoint {
> >
> > Nit: This is the tuner output, I would call this "tuner_out"
> >
>
> Done
>
> > >                                         remote-endpoint = <&tuner_out>;
> > >                                 };
> > >                         };
> > >                 };
> > >         };
> > >
> > >         adv7180: camera@20 {
> > >                 compatible = "adi,adv7180";
> >
> > One minor thing first: look at the adv7180 bindings documentation, and
> > you'll find out that only devices compatible with "adv7180cp" and
> > "adv7180st" shall declare a 'ports' node. This is minor issues (also,
> > I don't see it enforced in driver's code, but still worth pointing it
> > out from the very beginning)
> >
> > >                 reg = <0x20>;
> > >                 pinctrl-names = "default";
> > >                 pinctrl-0 = <&pinctrl_adv7180>;
> > >                 powerdown-gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; /* PDEC_PWRDN */
> > >
> > >                 ports {
> > >                         #address-cells = <1>;
> > >                         #size-cells = <0>;
> > >
> > >                         port@1 {
> > >                                 reg = <1>;
> > >
> > >                                 adv7180_to_ipu1_csi0_mux: endpoint {
> > >                                         remote-endpoint =
> > > <&ipu1_csi0_mux_from_parallel_sensor>;
> > >                                         bus-width = <8>;
> > >                                 };
> > >                         };
> > >
> > >                         port@0 {
> > >                                 reg = <0>;
> > >
> > >                                 tuner_out: endpoint {
> >
> > Nit: That's an adv7180 endpoint, I would call it 'adv7180_in'
> >
>
> Done
>
> > >                                         remote-endpoint = <&tuner_in>;
> > >                                 };
> > >                         };
> > >                 };
> > >         };
> > >
> > > Any help is appreciate
> > >
> >
> > The adv7180(cp|st) bindings says the device can declare one (or more)
> > input endpoints, but that's just to make possible to connect in device
> > tree the 7180's device node with the video input port. You can see an
> > example in arch/arm64/boot/dts/renesas/r8a77995-draak.dts which is
> > similar to what you've done here.
> >
> > The video input port does not show up in the media graph, as it is
> > just a 'place holder' to describe an input port in DTs, the
> > adv7180 driver does not register any sink pad, where to connect any
> > video source to.
> >
> > Your proposed bindings here look almost correct, but to have it
> > working for real you should add a sink pad to the adv7180 registered
> > media entity (possibly only conditionally to the presence of an input
> > endpoint in DTs...). You should then register a subdev-notifier, which
> > registers on an async-subdevice that uses the remote endpoint
> > connected to your newly registered input pad to find out which device
> > you're linked to; then at 'bound' (or possibly 'complete') time
> > register a link between the two entities, on which you can operate on
> > from userspace.
> >
> > Your tuner driver for tda9885 (which I don't see in mainline, so I
> > assume it's downstream or custom code) should register an async subdevice,
> > so that the adv7180 registered subdev-notifier gets matched and your
> > callbacks invoked.
> >
> > If I were you, I would:
> > 1) Add dt-parsing routine to tda7180, to find out if any input
> > endpoint is registered in DT.
>
> Done
>
> > 2) If it is, then register a SINK pad, along with the single SOURCE pad
> > which is registered today.
>
> Done
>
> > 3) When parsing DT, for input endpoints, get a reference to the remote
> > endpoint connected to your input and register a subdev-notifier
>
> Done
>
> > 4) Fill in the notifier 'bound' callback and register the link to
> > your remote device there.
>
> Both are subdevice that has not a v4l2_device, so bound is not called from two
> sub-device. Is this expected?

That should not be an issue. As we discussed, I slightly misleaded
you, pointing you to rcar-csi2, which implements a 'custom' matching
logic, trying to match its remote on endpoints and not on device node.

	priv->asd.match.fwnode =
		fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));

I'm sorry about this.

You better use something like:

	asd->match.fwnode =
		fwnode_graph_get_remote_port_parent(endpoint);

or the recently introduced 'v4l2_async_notifier_parse_fwnode_endpoints_by_port()'
function, that does most of that for you.

Sorry about this.
Thanks
   j

>
>
> > 5) Make sure your tuner driver registers its subdevice with
> > v4l2_async_register_subdev()
> >
> > A good example on how to register subdev notifier is provided in the
> > rcsi2_parse_dt() function in rcar-csi2.c driver (I'm quite sure imx
> > now uses subdev notifiers from v4.19 on too if you want to have a look
> > there).
>
> Already seen it
>
> >
> > -- Entering slippery territory here: you might want more inputs on this
> >
> > To make thing simpler&nicer (TM), if you blindly do what I've suggested
> > here, you're going to break all current adv7180 users in mainline :(
> >
> > That's because the v4l2-async design 'completes' the root notifier,
> > only if all subdev-notifiers connected to it have completed first.
> > If you add a notifier for the adv7180 input ports unconditionally, and
>
> I don't get to complete. So let's proceed by step
>
> Michael
>
> > to the input port is connected a plain simple "composite-video-connector",
> > as all DTs in mainline are doing right now, the newly registered
> > subdev-notifier will never complete, as the "composite-video-connector"
> > does not register any subdevice to match with. Bummer!
> >
> > A quick look in the code base, returns me that, in example:
> > drivers/gpu/drm/meson/meson_drv.c filters on
> > "composite-video-connector" and a few other compatible values. You
> > might want to do the same, and register a notifier if and only if the
> > remote input endpoint is one of those known not to register a
> > subdevice. I'm sure there are other ways to deal with this issue, but
> > that's the best I can come up with...
> > ---
> >
> > Hope this is reasonably clear and that I'm not misleading you. I had to
> > use adv7180 recently, and its single pad design confused me a bit as well :)
> >
> > Thanks
> >   j
> >
> > > Michael
> > >
> > > > > > Jagan.
> > > > > >
> > > > >
> > > >
> > > >
> > > > --
>
>
>
> --
> | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> | COO  -  Founder                                      Cruquiuskade 47 |
> | +31(0)851119172                                 Amsterdam 1018 AM NL |
> |                  [`as] http://www.amarulasolutions.com               |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Configure video PAL decoder into media pipeline

[Michael Nazzareno Trimarchi]

Hi Jacopo

On Tue, Dec 11, 2018 at 12:39 PM jacopo mondi <jacopo@jmondi.org> wrote:
>
> Hi Michael,
>
> On Mon, Dec 10, 2018 at 10:45:02PM +0100, Michael Nazzareno Trimarchi wrote:
> > Hi Jacopo
> >
> > Let's see what I have done
> >
> > On Sun, Dec 9, 2018 at 8:39 PM jacopo mondi <jacopo@jmondi.org> wrote:
> > >
> > > Hi Michael, Jagan, Hans,
> > >
> > > On Sat, Dec 08, 2018 at 06:07:04PM +0100, Michael Nazzareno Trimarchi wrote:
> > > > Hi
> > > >
> > > > Down you have my tentative of connection
> > > >
> > > > I need to hack a bit to have tuner registered. I'm using imx-media
> > > >
> > > > On Sat, Dec 8, 2018 at 12:48 PM Michael Nazzareno Trimarchi
> > > > <michael@amarulasolutions.com> wrote:
> > > > >
> > > > > Hi
> > > > >
> > > > > On Fri, Dec 7, 2018 at 1:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > > > >
> > > > > > On 12/07/2018 12:51 PM, Jagan Teki wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > We have some unconventional setup for parallel CSI design where analog
> > > > > > > input data is converted into to digital composite using PAL decoder
> > > > > > > and it feed to adv7180, camera sensor.
> > > > > > >
> > > > > > > Analog input =>  Video PAL Decoder => ADV7180 => IPU-CSI0
> > > > > >
> > > > > > Just PAL? No NTSC support?
> > > > > >
> > > > > For now does not matter. I have registere the TUNER that support it
> > > > > but seems that media-ctl is not suppose to work with the MEDIA_ENT_F_TUNER
> > > > >
> > > > > Is this correct?
> > > > >
> > >
> > > media-types.rst reports:
> > >
> > >     *  -  ``MEDIA_ENT_F_IF_VID_DECODER``
> > >        -  IF-PLL video decoder. It receives the IF from a PLL and decodes
> > >           the analog TV video signal. This is commonly found on some very
> > >           old analog tuners, like Philips MK3 designs. They all contain a
> > >           tda9887 (or some software compatible similar chip, like tda9885).
> > >           Those devices use a different I2C address than the tuner PLL.
> > >
> > > Is this what you were looking for?
> > >
> > > > > > >
> > > > > > > The PAL decoder is I2C based, tda9885 chip. We setup it up via dt
> > > > > > > bindings and the chip is
> > > > > > > detected fine.
> > > > > > >
> > > > > > > But we need to know, is this to be part of media control subdev
> > > > > > > pipeline? so-that we can configure pads, links like what we do on
> > > > > > > conventional pipeline  or it should not to be part of media pipeline?
> > > > > >
> > > > > > Yes, I would say it should be part of the pipeline.
> > > > > >
> > > > >
> > > > > Ok I have created a draft patch to add the adv some new endpoint but
> > > > > is sufficient to declare tuner type in media control?
> > > > >
> > > > > Michael
> > > > >
> > > > > > >
> > > > > > > Please advise for best possible way to fit this into the design.
> > > > > > >
> > > > > > > Another observation is since the IPU has more than one sink, source
> > > > > > > pads, we source or sink the other components on the pipeline but look
> > > > > > > like the same thing seems not possible with adv7180 since if has only
> > > > > > > one pad. If it has only one pad sourcing to adv7180 from tda9885 seems
> > > > > > > not possible, If I'm not mistaken.
> > > > > >
> > > > > > Correct, in all cases where the adv7180 is used it is directly connected
> > > > > > to the video input connector on a board.
> > > > > >
> > > > > > So to support this the adv7180 driver should be modified to add an input pad
> > > > > > so you can connect the decoder. It will be needed at some point anyway once
> > > > > > we add support for connector entities.
> > > > > >
> > > > > > Regards,
> > > > > >
> > > > > >         Hans
> > > > > >
> > > > > > >
> > > > > > > I tried to look for similar design in mainline, but I couldn't find
> > > > > > > it. is there any design similar to this in mainline?
> > > > > > >
> > > > > > > Please let us know if anyone has any suggestions on this.
> > > > > > >
> > > >
> > > > [    3.379129] imx-media: ipu1_vdic:2 -> ipu1_ic_prp:0
> > > > [    3.384262] imx-media: ipu2_vdic:2 -> ipu2_ic_prp:0
> > > > [    3.389217] imx-media: ipu1_ic_prp:1 -> ipu1_ic_prpenc:0
> > > > [    3.394616] imx-media: ipu1_ic_prp:2 -> ipu1_ic_prpvf:0
> > > > [    3.399867] imx-media: ipu2_ic_prp:1 -> ipu2_ic_prpenc:0
> > > > [    3.405289] imx-media: ipu2_ic_prp:2 -> ipu2_ic_prpvf:0
> > > > [    3.410552] imx-media: ipu1_csi0:1 -> ipu1_ic_prp:0
> > > > [    3.415502] imx-media: ipu1_csi0:1 -> ipu1_vdic:0
> > > > [    3.420305] imx-media: ipu1_csi0_mux:5 -> ipu1_csi0:0
> > > > [    3.425427] imx-media: ipu1_csi1:1 -> ipu1_ic_prp:0
> > > > [    3.430328] imx-media: ipu1_csi1:1 -> ipu1_vdic:0
> > > > [    3.435142] imx-media: ipu1_csi1_mux:5 -> ipu1_csi1:0
> > > > [    3.440321] imx-media: adv7180 2-0020:1 -> ipu1_csi0_mux:4
> > > >
> > > > with
> > > >        tuner: tuner@43 {
> > > >                 compatible = "tuner";
> > > >                 reg = <0x43>;
> > > >                 pinctrl-names = "default";
> > > >                 pinctrl-0 = <&pinctrl_tuner>;
> > > >
> > > >                 ports {
> > > >                         #address-cells = <1>;
> > > >                         #size-cells = <0>;
> > > >                         port@1 {
> > > >                                 reg = <1>;
> > > >
> > > >                                 tuner_in: endpoint {
> > >
> > > Nit: This is the tuner output, I would call this "tuner_out"
> > >
> >
> > Done
> >
> > > >                                         remote-endpoint = <&tuner_out>;
> > > >                                 };
> > > >                         };
> > > >                 };
> > > >         };
> > > >
> > > >         adv7180: camera@20 {
> > > >                 compatible = "adi,adv7180";
> > >
> > > One minor thing first: look at the adv7180 bindings documentation, and
> > > you'll find out that only devices compatible with "adv7180cp" and
> > > "adv7180st" shall declare a 'ports' node. This is minor issues (also,
> > > I don't see it enforced in driver's code, but still worth pointing it
> > > out from the very beginning)
> > >
> > > >                 reg = <0x20>;
> > > >                 pinctrl-names = "default";
> > > >                 pinctrl-0 = <&pinctrl_adv7180>;
> > > >                 powerdown-gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; /* PDEC_PWRDN */
> > > >
> > > >                 ports {
> > > >                         #address-cells = <1>;
> > > >                         #size-cells = <0>;
> > > >
> > > >                         port@1 {
> > > >                                 reg = <1>;
> > > >
> > > >                                 adv7180_to_ipu1_csi0_mux: endpoint {
> > > >                                         remote-endpoint =
> > > > <&ipu1_csi0_mux_from_parallel_sensor>;
> > > >                                         bus-width = <8>;
> > > >                                 };
> > > >                         };
> > > >
> > > >                         port@0 {
> > > >                                 reg = <0>;
> > > >
> > > >                                 tuner_out: endpoint {
> > >
> > > Nit: That's an adv7180 endpoint, I would call it 'adv7180_in'
> > >
> >
> > Done
> >
> > > >                                         remote-endpoint = <&tuner_in>;
> > > >                                 };
> > > >                         };
> > > >                 };
> > > >         };
> > > >
> > > > Any help is appreciate
> > > >
> > >
> > > The adv7180(cp|st) bindings says the device can declare one (or more)
> > > input endpoints, but that's just to make possible to connect in device
> > > tree the 7180's device node with the video input port. You can see an
> > > example in arch/arm64/boot/dts/renesas/r8a77995-draak.dts which is
> > > similar to what you've done here.
> > >
> > > The video input port does not show up in the media graph, as it is
> > > just a 'place holder' to describe an input port in DTs, the
> > > adv7180 driver does not register any sink pad, where to connect any
> > > video source to.
> > >
> > > Your proposed bindings here look almost correct, but to have it
> > > working for real you should add a sink pad to the adv7180 registered
> > > media entity (possibly only conditionally to the presence of an input
> > > endpoint in DTs...). You should then register a subdev-notifier, which
> > > registers on an async-subdevice that uses the remote endpoint
> > > connected to your newly registered input pad to find out which device
> > > you're linked to; then at 'bound' (or possibly 'complete') time
> > > register a link between the two entities, on which you can operate on
> > > from userspace.
> > >
> > > Your tuner driver for tda9885 (which I don't see in mainline, so I
> > > assume it's downstream or custom code) should register an async subdevice,
> > > so that the adv7180 registered subdev-notifier gets matched and your
> > > callbacks invoked.
> > >
> > > If I were you, I would:
> > > 1) Add dt-parsing routine to tda7180, to find out if any input
> > > endpoint is registered in DT.
> >
> > Done
> >
> > > 2) If it is, then register a SINK pad, along with the single SOURCE pad
> > > which is registered today.
> >
> > Done
> >
> > > 3) When parsing DT, for input endpoints, get a reference to the remote
> > > endpoint connected to your input and register a subdev-notifier
> >
> > Done
> >
> > > 4) Fill in the notifier 'bound' callback and register the link to
> > > your remote device there.
> >
> > Both are subdevice that has not a v4l2_device, so bound is not called from two
> > sub-device. Is this expected?
>
> That should not be an issue. As we discussed, I slightly misleaded
> you, pointing you to rcar-csi2, which implements a 'custom' matching
> logic, trying to match its remote on endpoints and not on device node.
>
>         priv->asd.match.fwnode =
>                 fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
>
> I'm sorry about this.
>
> You better use something like:
>
>         asd->match.fwnode =
>                 fwnode_graph_get_remote_port_parent(endpoint);
>
> or the recently introduced 'v4l2_async_notifier_parse_fwnode_endpoints_by_port()'
> function, that does most of that for you.
>

- entity 80: adv7180 2-0020 (2 pads, 5 links)
             type V4L2 subdev subtype Decoder flags 0
             device node name /dev/v4l-subdev11
pad0: Sink
[fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
<- "ipu1_csi0_mux":4 []
-> "ipu1_csi0_mux":4 []
<- "tda9887":1 [ENABLED,IMMUTABLE]
pad1: Source
[fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
-> "tda9887":1 []
<- "tda9887":1 []

- entity 83: tda9887 (2 pads, 3 links)
             type V4L2 subdev subtype Unknown flags 0
pad0: Sink
pad1: Source
<- "adv7180 2-0020":1 []
-> "adv7180 2-0020":0 [ENABLED,IMMUTABLE]
-> "adv7180 2-0020":1 []


Now the only problem is that I have a link in the graph that I have
not defined that not le me to stream. Look and png file I can see an
hard link from tda9887 to csi. Do you know why is coming?

Michael

> Sorry about this.
> Thanks
>    j
>
> >
> >
> > > 5) Make sure your tuner driver registers its subdevice with
> > > v4l2_async_register_subdev()
> > >
> > > A good example on how to register subdev notifier is provided in the
> > > rcsi2_parse_dt() function in rcar-csi2.c driver (I'm quite sure imx
> > > now uses subdev notifiers from v4.19 on too if you want to have a look
> > > there).
> >
> > Already seen it
> >
> > >
> > > -- Entering slippery territory here: you might want more inputs on this
> > >
> > > To make thing simpler&nicer (TM), if you blindly do what I've suggested
> > > here, you're going to break all current adv7180 users in mainline :(
> > >
> > > That's because the v4l2-async design 'completes' the root notifier,
> > > only if all subdev-notifiers connected to it have completed first.
> > > If you add a notifier for the adv7180 input ports unconditionally, and
> >
> > I don't get to complete. So let's proceed by step
> >
> > Michael
> >
> > > to the input port is connected a plain simple "composite-video-connector",
> > > as all DTs in mainline are doing right now, the newly registered
> > > subdev-notifier will never complete, as the "composite-video-connector"
> > > does not register any subdevice to match with. Bummer!
> > >
> > > A quick look in the code base, returns me that, in example:
> > > drivers/gpu/drm/meson/meson_drv.c filters on
> > > "composite-video-connector" and a few other compatible values. You
> > > might want to do the same, and register a notifier if and only if the
> > > remote input endpoint is one of those known not to register a
> > > subdevice. I'm sure there are other ways to deal with this issue, but
> > > that's the best I can come up with...
> > > ---
> > >
> > > Hope this is reasonably clear and that I'm not misleading you. I had to
> > > use adv7180 recently, and its single pad design confused me a bit as well :)
> > >
> > > Thanks
> > >   j
> > >
> > > > Michael
> > > >
> > > > > > > Jagan.
> > > > > > >
> > > > > >
> > > > >
> > > > >
> > > > > --
> >
> >
> >
> > --
> > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > | COO  -  Founder                                      Cruquiuskade 47 |
> > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > |                  [`as] http://www.amarulasolutions.com               |



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

Re: Configure video PAL decoder into media pipeline

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 15683 bytes --]

Hi Michael,

On Tue, Dec 11, 2018 at 02:53:24PM +0100, Michael Nazzareno Trimarchi wrote:
> Hi Jacopo
>
> On Tue, Dec 11, 2018 at 12:39 PM jacopo mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Michael,
> >
> > On Mon, Dec 10, 2018 at 10:45:02PM +0100, Michael Nazzareno Trimarchi wrote:
> > > Hi Jacopo
> > >
> > > Let's see what I have done
> > >
> > > On Sun, Dec 9, 2018 at 8:39 PM jacopo mondi <jacopo@jmondi.org> wrote:
> > > >
> > > > Hi Michael, Jagan, Hans,
> > > >
> > > > On Sat, Dec 08, 2018 at 06:07:04PM +0100, Michael Nazzareno Trimarchi wrote:
> > > > > Hi
> > > > >
> > > > > Down you have my tentative of connection
> > > > >
> > > > > I need to hack a bit to have tuner registered. I'm using imx-media
> > > > >
> > > > > On Sat, Dec 8, 2018 at 12:48 PM Michael Nazzareno Trimarchi
> > > > > <michael@amarulasolutions.com> wrote:
> > > > > >
> > > > > > Hi
> > > > > >
> > > > > > On Fri, Dec 7, 2018 at 1:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > > > > >
> > > > > > > On 12/07/2018 12:51 PM, Jagan Teki wrote:
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > We have some unconventional setup for parallel CSI design where analog
> > > > > > > > input data is converted into to digital composite using PAL decoder
> > > > > > > > and it feed to adv7180, camera sensor.
> > > > > > > >
> > > > > > > > Analog input =>  Video PAL Decoder => ADV7180 => IPU-CSI0
> > > > > > >
> > > > > > > Just PAL? No NTSC support?
> > > > > > >
> > > > > > For now does not matter. I have registere the TUNER that support it
> > > > > > but seems that media-ctl is not suppose to work with the MEDIA_ENT_F_TUNER
> > > > > >
> > > > > > Is this correct?
> > > > > >
> > > >
> > > > media-types.rst reports:
> > > >
> > > >     *  -  ``MEDIA_ENT_F_IF_VID_DECODER``
> > > >        -  IF-PLL video decoder. It receives the IF from a PLL and decodes
> > > >           the analog TV video signal. This is commonly found on some very
> > > >           old analog tuners, like Philips MK3 designs. They all contain a
> > > >           tda9887 (or some software compatible similar chip, like tda9885).
> > > >           Those devices use a different I2C address than the tuner PLL.
> > > >
> > > > Is this what you were looking for?
> > > >
> > > > > > > >
> > > > > > > > The PAL decoder is I2C based, tda9885 chip. We setup it up via dt
> > > > > > > > bindings and the chip is
> > > > > > > > detected fine.
> > > > > > > >
> > > > > > > > But we need to know, is this to be part of media control subdev
> > > > > > > > pipeline? so-that we can configure pads, links like what we do on
> > > > > > > > conventional pipeline  or it should not to be part of media pipeline?
> > > > > > >
> > > > > > > Yes, I would say it should be part of the pipeline.
> > > > > > >
> > > > > >
> > > > > > Ok I have created a draft patch to add the adv some new endpoint but
> > > > > > is sufficient to declare tuner type in media control?
> > > > > >
> > > > > > Michael
> > > > > >
> > > > > > > >
> > > > > > > > Please advise for best possible way to fit this into the design.
> > > > > > > >
> > > > > > > > Another observation is since the IPU has more than one sink, source
> > > > > > > > pads, we source or sink the other components on the pipeline but look
> > > > > > > > like the same thing seems not possible with adv7180 since if has only
> > > > > > > > one pad. If it has only one pad sourcing to adv7180 from tda9885 seems
> > > > > > > > not possible, If I'm not mistaken.
> > > > > > >
> > > > > > > Correct, in all cases where the adv7180 is used it is directly connected
> > > > > > > to the video input connector on a board.
> > > > > > >
> > > > > > > So to support this the adv7180 driver should be modified to add an input pad
> > > > > > > so you can connect the decoder. It will be needed at some point anyway once
> > > > > > > we add support for connector entities.
> > > > > > >
> > > > > > > Regards,
> > > > > > >
> > > > > > >         Hans
> > > > > > >
> > > > > > > >
> > > > > > > > I tried to look for similar design in mainline, but I couldn't find
> > > > > > > > it. is there any design similar to this in mainline?
> > > > > > > >
> > > > > > > > Please let us know if anyone has any suggestions on this.
> > > > > > > >
> > > > >
> > > > > [    3.379129] imx-media: ipu1_vdic:2 -> ipu1_ic_prp:0
> > > > > [    3.384262] imx-media: ipu2_vdic:2 -> ipu2_ic_prp:0
> > > > > [    3.389217] imx-media: ipu1_ic_prp:1 -> ipu1_ic_prpenc:0
> > > > > [    3.394616] imx-media: ipu1_ic_prp:2 -> ipu1_ic_prpvf:0
> > > > > [    3.399867] imx-media: ipu2_ic_prp:1 -> ipu2_ic_prpenc:0
> > > > > [    3.405289] imx-media: ipu2_ic_prp:2 -> ipu2_ic_prpvf:0
> > > > > [    3.410552] imx-media: ipu1_csi0:1 -> ipu1_ic_prp:0
> > > > > [    3.415502] imx-media: ipu1_csi0:1 -> ipu1_vdic:0
> > > > > [    3.420305] imx-media: ipu1_csi0_mux:5 -> ipu1_csi0:0
> > > > > [    3.425427] imx-media: ipu1_csi1:1 -> ipu1_ic_prp:0
> > > > > [    3.430328] imx-media: ipu1_csi1:1 -> ipu1_vdic:0
> > > > > [    3.435142] imx-media: ipu1_csi1_mux:5 -> ipu1_csi1:0
> > > > > [    3.440321] imx-media: adv7180 2-0020:1 -> ipu1_csi0_mux:4
> > > > >
> > > > > with
> > > > >        tuner: tuner@43 {
> > > > >                 compatible = "tuner";
> > > > >                 reg = <0x43>;
> > > > >                 pinctrl-names = "default";
> > > > >                 pinctrl-0 = <&pinctrl_tuner>;
> > > > >
> > > > >                 ports {
> > > > >                         #address-cells = <1>;
> > > > >                         #size-cells = <0>;
> > > > >                         port@1 {
> > > > >                                 reg = <1>;
> > > > >
> > > > >                                 tuner_in: endpoint {
> > > >
> > > > Nit: This is the tuner output, I would call this "tuner_out"
> > > >
> > >
> > > Done
> > >
> > > > >                                         remote-endpoint = <&tuner_out>;
> > > > >                                 };
> > > > >                         };
> > > > >                 };
> > > > >         };
> > > > >
> > > > >         adv7180: camera@20 {
> > > > >                 compatible = "adi,adv7180";
> > > >
> > > > One minor thing first: look at the adv7180 bindings documentation, and
> > > > you'll find out that only devices compatible with "adv7180cp" and
> > > > "adv7180st" shall declare a 'ports' node. This is minor issues (also,
> > > > I don't see it enforced in driver's code, but still worth pointing it
> > > > out from the very beginning)
> > > >
> > > > >                 reg = <0x20>;
> > > > >                 pinctrl-names = "default";
> > > > >                 pinctrl-0 = <&pinctrl_adv7180>;
> > > > >                 powerdown-gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; /* PDEC_PWRDN */
> > > > >
> > > > >                 ports {
> > > > >                         #address-cells = <1>;
> > > > >                         #size-cells = <0>;
> > > > >
> > > > >                         port@1 {
> > > > >                                 reg = <1>;
> > > > >
> > > > >                                 adv7180_to_ipu1_csi0_mux: endpoint {
> > > > >                                         remote-endpoint =
> > > > > <&ipu1_csi0_mux_from_parallel_sensor>;
> > > > >                                         bus-width = <8>;
> > > > >                                 };
> > > > >                         };
> > > > >
> > > > >                         port@0 {
> > > > >                                 reg = <0>;
> > > > >
> > > > >                                 tuner_out: endpoint {
> > > >
> > > > Nit: That's an adv7180 endpoint, I would call it 'adv7180_in'
> > > >
> > >
> > > Done
> > >
> > > > >                                         remote-endpoint = <&tuner_in>;
> > > > >                                 };
> > > > >                         };
> > > > >                 };
> > > > >         };
> > > > >
> > > > > Any help is appreciate
> > > > >
> > > >
> > > > The adv7180(cp|st) bindings says the device can declare one (or more)
> > > > input endpoints, but that's just to make possible to connect in device
> > > > tree the 7180's device node with the video input port. You can see an
> > > > example in arch/arm64/boot/dts/renesas/r8a77995-draak.dts which is
> > > > similar to what you've done here.
> > > >
> > > > The video input port does not show up in the media graph, as it is
> > > > just a 'place holder' to describe an input port in DTs, the
> > > > adv7180 driver does not register any sink pad, where to connect any
> > > > video source to.
> > > >
> > > > Your proposed bindings here look almost correct, but to have it
> > > > working for real you should add a sink pad to the adv7180 registered
> > > > media entity (possibly only conditionally to the presence of an input
> > > > endpoint in DTs...). You should then register a subdev-notifier, which
> > > > registers on an async-subdevice that uses the remote endpoint
> > > > connected to your newly registered input pad to find out which device
> > > > you're linked to; then at 'bound' (or possibly 'complete') time
> > > > register a link between the two entities, on which you can operate on
> > > > from userspace.
> > > >
> > > > Your tuner driver for tda9885 (which I don't see in mainline, so I
> > > > assume it's downstream or custom code) should register an async subdevice,
> > > > so that the adv7180 registered subdev-notifier gets matched and your
> > > > callbacks invoked.
> > > >
> > > > If I were you, I would:
> > > > 1) Add dt-parsing routine to tda7180, to find out if any input
> > > > endpoint is registered in DT.
> > >
> > > Done
> > >
> > > > 2) If it is, then register a SINK pad, along with the single SOURCE pad
> > > > which is registered today.
> > >
> > > Done
> > >
> > > > 3) When parsing DT, for input endpoints, get a reference to the remote
> > > > endpoint connected to your input and register a subdev-notifier
> > >
> > > Done
> > >
> > > > 4) Fill in the notifier 'bound' callback and register the link to
> > > > your remote device there.
> > >
> > > Both are subdevice that has not a v4l2_device, so bound is not called from two
> > > sub-device. Is this expected?
> >
> > That should not be an issue. As we discussed, I slightly misleaded
> > you, pointing you to rcar-csi2, which implements a 'custom' matching
> > logic, trying to match its remote on endpoints and not on device node.
> >
> >         priv->asd.match.fwnode =
> >                 fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> >
> > I'm sorry about this.
> >
> > You better use something like:
> >
> >         asd->match.fwnode =
> >                 fwnode_graph_get_remote_port_parent(endpoint);
> >
> > or the recently introduced 'v4l2_async_notifier_parse_fwnode_endpoints_by_port()'
> > function, that does most of that for you.
> >
>
> - entity 80: adv7180 2-0020 (2 pads, 5 links)
>              type V4L2 subdev subtype Decoder flags 0
>              device node name /dev/v4l-subdev11
> pad0: Sink
> [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
> <- "ipu1_csi0_mux":4 []
> -> "ipu1_csi0_mux":4 []
> <- "tda9887":1 [ENABLED,IMMUTABLE]
> pad1: Source
> [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
> -> "tda9887":1 []
> <- "tda9887":1 []
>
> - entity 83: tda9887 (2 pads, 3 links)
>              type V4L2 subdev subtype Unknown flags 0
> pad0: Sink
> pad1: Source
> <- "adv7180 2-0020":1 []
> -> "adv7180 2-0020":0 [ENABLED,IMMUTABLE]
> -> "adv7180 2-0020":1 []
>
>
> Now the only problem is that I have a link in the graph that I have
> not defined that not le me to stream. Look and png file I can see an
> hard link from tda9887 to csi. Do you know why is coming?
>

I don't see any link between tda and csi in the snippet you pasted
above (nor I see a .png representing the media graph attached).

What I see is the link: '"adv7180 2-0020":0 -> "tda9887":1' which is
fine, but you're missing a link '"adv7180 2-0020":1 -> "ipu1_csi0_mux":4'

From what I see your DTS (or parsing routines, I can't tell without
the seeing the code) links  adv7180:1->tda9887:1 which is a
source->source link, and the same time creates an
adv7180:0->ipu1_csi0_mux:4 which is a sink->sink link.

If you fix that by creating instead a adv7180:1->ipu1_csi0_mux:4 you
should be fine (provided you keep the tda9887:1->adv7180:0 link you have
already).

If you send patches, we can comment further, otherwise it gets hard
without seeing what's happening for real.

Thanks
   j

> Michael
>
> > Sorry about this.
> > Thanks
> >    j
> >
> > >
> > >
> > > > 5) Make sure your tuner driver registers its subdevice with
> > > > v4l2_async_register_subdev()
> > > >
> > > > A good example on how to register subdev notifier is provided in the
> > > > rcsi2_parse_dt() function in rcar-csi2.c driver (I'm quite sure imx
> > > > now uses subdev notifiers from v4.19 on too if you want to have a look
> > > > there).
> > >
> > > Already seen it
> > >
> > > >
> > > > -- Entering slippery territory here: you might want more inputs on this
> > > >
> > > > To make thing simpler&nicer (TM), if you blindly do what I've suggested
> > > > here, you're going to break all current adv7180 users in mainline :(
> > > >
> > > > That's because the v4l2-async design 'completes' the root notifier,
> > > > only if all subdev-notifiers connected to it have completed first.
> > > > If you add a notifier for the adv7180 input ports unconditionally, and
> > >
> > > I don't get to complete. So let's proceed by step
> > >
> > > Michael
> > >
> > > > to the input port is connected a plain simple "composite-video-connector",
> > > > as all DTs in mainline are doing right now, the newly registered
> > > > subdev-notifier will never complete, as the "composite-video-connector"
> > > > does not register any subdevice to match with. Bummer!
> > > >
> > > > A quick look in the code base, returns me that, in example:
> > > > drivers/gpu/drm/meson/meson_drv.c filters on
> > > > "composite-video-connector" and a few other compatible values. You
> > > > might want to do the same, and register a notifier if and only if the
> > > > remote input endpoint is one of those known not to register a
> > > > subdevice. I'm sure there are other ways to deal with this issue, but
> > > > that's the best I can come up with...
> > > > ---
> > > >
> > > > Hope this is reasonably clear and that I'm not misleading you. I had to
> > > > use adv7180 recently, and its single pad design confused me a bit as well :)
> > > >
> > > > Thanks
> > > >   j
> > > >
> > > > > Michael
> > > > >
> > > > > > > > Jagan.
> > > > > > > >
> > > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > >
> > >
> > >
> > > --
> > > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > > | COO  -  Founder                                      Cruquiuskade 47 |
> > > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > > |                  [`as] http://www.amarulasolutions.com               |
>
>
>
> --
> | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> | COO  -  Founder                                      Cruquiuskade 47 |
> | +31(0)851119172                                 Amsterdam 1018 AM NL |
> |                  [`as] http://www.amarulasolutions.com               |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Configure video PAL decoder into media pipeline

[Michael Nazzareno Trimarchi]

[-- Attachment #1: Type: text/plain, Size: 16920 bytes --]

Hi

On Wed, Dec 12, 2018 at 9:39 AM jacopo mondi <jacopo@jmondi.org> wrote:
>
> Hi Michael,
>
> On Tue, Dec 11, 2018 at 02:53:24PM +0100, Michael Nazzareno Trimarchi wrote:
> > Hi Jacopo
> >
> > On Tue, Dec 11, 2018 at 12:39 PM jacopo mondi <jacopo@jmondi.org> wrote:
> > >
> > > Hi Michael,
> > >
> > > On Mon, Dec 10, 2018 at 10:45:02PM +0100, Michael Nazzareno Trimarchi wrote:
> > > > Hi Jacopo
> > > >
> > > > Let's see what I have done
> > > >
> > > > On Sun, Dec 9, 2018 at 8:39 PM jacopo mondi <jacopo@jmondi.org> wrote:
> > > > >
> > > > > Hi Michael, Jagan, Hans,
> > > > >

media-ctl --links "'tda9887':1->'adv7180 2-0020':1[1]"
media-ctl --links "'adv7180 2-0020':0->'ipu1_csi0_mux':4[1]"
media-ctl --links "'ipu1_csi0_mux':5->'ipu1_csi0':0[1]"
media-ctl --links "'ipu1_csi0':1->'ipu1_vdic':0[1]"
media-ctl --links "'ipu1_vdic':2->'ipu1_ic_prp':0[1]"
media-ctl -l "'ipu1_ic_prp':2 -> 'ipu1_ic_prpvf':0[1]"
media-ctl -l "'ipu1_ic_prpvf':1 -> 'ipu1_ic_prpvf capture':0[1]"

If I try to activate this one or any other go to the end, it's just dealock.

Michael

> > > > > On Sat, Dec 08, 2018 at 06:07:04PM +0100, Michael Nazzareno Trimarchi wrote:
> > > > > > Hi
> > > > > >
> > > > > > Down you have my tentative of connection
> > > > > >
> > > > > > I need to hack a bit to have tuner registered. I'm using imx-media
> > > > > >
> > > > > > On Sat, Dec 8, 2018 at 12:48 PM Michael Nazzareno Trimarchi
> > > > > > <michael@amarulasolutions.com> wrote:
> > > > > > >
> > > > > > > Hi
> > > > > > >
> > > > > > > On Fri, Dec 7, 2018 at 1:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > > > > > >
> > > > > > > > On 12/07/2018 12:51 PM, Jagan Teki wrote:
> > > > > > > > > Hi,
> > > > > > > > >
> > > > > > > > > We have some unconventional setup for parallel CSI design where analog
> > > > > > > > > input data is converted into to digital composite using PAL decoder
> > > > > > > > > and it feed to adv7180, camera sensor.
> > > > > > > > >
> > > > > > > > > Analog input =>  Video PAL Decoder => ADV7180 => IPU-CSI0
> > > > > > > >
> > > > > > > > Just PAL? No NTSC support?
> > > > > > > >
> > > > > > > For now does not matter. I have registere the TUNER that support it
> > > > > > > but seems that media-ctl is not suppose to work with the MEDIA_ENT_F_TUNER
> > > > > > >
> > > > > > > Is this correct?
> > > > > > >
> > > > >
> > > > > media-types.rst reports:
> > > > >
> > > > >     *  -  ``MEDIA_ENT_F_IF_VID_DECODER``
> > > > >        -  IF-PLL video decoder. It receives the IF from a PLL and decodes
> > > > >           the analog TV video signal. This is commonly found on some very
> > > > >           old analog tuners, like Philips MK3 designs. They all contain a
> > > > >           tda9887 (or some software compatible similar chip, like tda9885).
> > > > >           Those devices use a different I2C address than the tuner PLL.
> > > > >
> > > > > Is this what you were looking for?
> > > > >
> > > > > > > > >
> > > > > > > > > The PAL decoder is I2C based, tda9885 chip. We setup it up via dt
> > > > > > > > > bindings and the chip is
> > > > > > > > > detected fine.
> > > > > > > > >
> > > > > > > > > But we need to know, is this to be part of media control subdev
> > > > > > > > > pipeline? so-that we can configure pads, links like what we do on
> > > > > > > > > conventional pipeline  or it should not to be part of media pipeline?
> > > > > > > >
> > > > > > > > Yes, I would say it should be part of the pipeline.
> > > > > > > >
> > > > > > >
> > > > > > > Ok I have created a draft patch to add the adv some new endpoint but
> > > > > > > is sufficient to declare tuner type in media control?
> > > > > > >
> > > > > > > Michael
> > > > > > >
> > > > > > > > >
> > > > > > > > > Please advise for best possible way to fit this into the design.
> > > > > > > > >
> > > > > > > > > Another observation is since the IPU has more than one sink, source
> > > > > > > > > pads, we source or sink the other components on the pipeline but look
> > > > > > > > > like the same thing seems not possible with adv7180 since if has only
> > > > > > > > > one pad. If it has only one pad sourcing to adv7180 from tda9885 seems
> > > > > > > > > not possible, If I'm not mistaken.
> > > > > > > >
> > > > > > > > Correct, in all cases where the adv7180 is used it is directly connected
> > > > > > > > to the video input connector on a board.
> > > > > > > >
> > > > > > > > So to support this the adv7180 driver should be modified to add an input pad
> > > > > > > > so you can connect the decoder. It will be needed at some point anyway once
> > > > > > > > we add support for connector entities.
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > >
> > > > > > > >         Hans
> > > > > > > >
> > > > > > > > >
> > > > > > > > > I tried to look for similar design in mainline, but I couldn't find
> > > > > > > > > it. is there any design similar to this in mainline?
> > > > > > > > >
> > > > > > > > > Please let us know if anyone has any suggestions on this.
> > > > > > > > >
> > > > > >
> > > > > > [    3.379129] imx-media: ipu1_vdic:2 -> ipu1_ic_prp:0
> > > > > > [    3.384262] imx-media: ipu2_vdic:2 -> ipu2_ic_prp:0
> > > > > > [    3.389217] imx-media: ipu1_ic_prp:1 -> ipu1_ic_prpenc:0
> > > > > > [    3.394616] imx-media: ipu1_ic_prp:2 -> ipu1_ic_prpvf:0
> > > > > > [    3.399867] imx-media: ipu2_ic_prp:1 -> ipu2_ic_prpenc:0
> > > > > > [    3.405289] imx-media: ipu2_ic_prp:2 -> ipu2_ic_prpvf:0
> > > > > > [    3.410552] imx-media: ipu1_csi0:1 -> ipu1_ic_prp:0
> > > > > > [    3.415502] imx-media: ipu1_csi0:1 -> ipu1_vdic:0
> > > > > > [    3.420305] imx-media: ipu1_csi0_mux:5 -> ipu1_csi0:0
> > > > > > [    3.425427] imx-media: ipu1_csi1:1 -> ipu1_ic_prp:0
> > > > > > [    3.430328] imx-media: ipu1_csi1:1 -> ipu1_vdic:0
> > > > > > [    3.435142] imx-media: ipu1_csi1_mux:5 -> ipu1_csi1:0
> > > > > > [    3.440321] imx-media: adv7180 2-0020:1 -> ipu1_csi0_mux:4
> > > > > >
> > > > > > with
> > > > > >        tuner: tuner@43 {
> > > > > >                 compatible = "tuner";
> > > > > >                 reg = <0x43>;
> > > > > >                 pinctrl-names = "default";
> > > > > >                 pinctrl-0 = <&pinctrl_tuner>;
> > > > > >
> > > > > >                 ports {
> > > > > >                         #address-cells = <1>;
> > > > > >                         #size-cells = <0>;
> > > > > >                         port@1 {
> > > > > >                                 reg = <1>;
> > > > > >
> > > > > >                                 tuner_in: endpoint {
> > > > >
> > > > > Nit: This is the tuner output, I would call this "tuner_out"
> > > > >
> > > >
> > > > Done
> > > >
> > > > > >                                         remote-endpoint = <&tuner_out>;
> > > > > >                                 };
> > > > > >                         };
> > > > > >                 };
> > > > > >         };
> > > > > >
> > > > > >         adv7180: camera@20 {
> > > > > >                 compatible = "adi,adv7180";
> > > > >
> > > > > One minor thing first: look at the adv7180 bindings documentation, and
> > > > > you'll find out that only devices compatible with "adv7180cp" and
> > > > > "adv7180st" shall declare a 'ports' node. This is minor issues (also,
> > > > > I don't see it enforced in driver's code, but still worth pointing it
> > > > > out from the very beginning)
> > > > >
> > > > > >                 reg = <0x20>;
> > > > > >                 pinctrl-names = "default";
> > > > > >                 pinctrl-0 = <&pinctrl_adv7180>;
> > > > > >                 powerdown-gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; /* PDEC_PWRDN */
> > > > > >
> > > > > >                 ports {
> > > > > >                         #address-cells = <1>;
> > > > > >                         #size-cells = <0>;
> > > > > >
> > > > > >                         port@1 {
> > > > > >                                 reg = <1>;
> > > > > >
> > > > > >                                 adv7180_to_ipu1_csi0_mux: endpoint {
> > > > > >                                         remote-endpoint =
> > > > > > <&ipu1_csi0_mux_from_parallel_sensor>;
> > > > > >                                         bus-width = <8>;
> > > > > >                                 };
> > > > > >                         };
> > > > > >
> > > > > >                         port@0 {
> > > > > >                                 reg = <0>;
> > > > > >
> > > > > >                                 tuner_out: endpoint {
> > > > >
> > > > > Nit: That's an adv7180 endpoint, I would call it 'adv7180_in'
> > > > >
> > > >
> > > > Done
> > > >
> > > > > >                                         remote-endpoint = <&tuner_in>;
> > > > > >                                 };
> > > > > >                         };
> > > > > >                 };
> > > > > >         };
> > > > > >
> > > > > > Any help is appreciate
> > > > > >
> > > > >
> > > > > The adv7180(cp|st) bindings says the device can declare one (or more)
> > > > > input endpoints, but that's just to make possible to connect in device
> > > > > tree the 7180's device node with the video input port. You can see an
> > > > > example in arch/arm64/boot/dts/renesas/r8a77995-draak.dts which is
> > > > > similar to what you've done here.
> > > > >
> > > > > The video input port does not show up in the media graph, as it is
> > > > > just a 'place holder' to describe an input port in DTs, the
> > > > > adv7180 driver does not register any sink pad, where to connect any
> > > > > video source to.
> > > > >
> > > > > Your proposed bindings here look almost correct, but to have it
> > > > > working for real you should add a sink pad to the adv7180 registered
> > > > > media entity (possibly only conditionally to the presence of an input
> > > > > endpoint in DTs...). You should then register a subdev-notifier, which
> > > > > registers on an async-subdevice that uses the remote endpoint
> > > > > connected to your newly registered input pad to find out which device
> > > > > you're linked to; then at 'bound' (or possibly 'complete') time
> > > > > register a link between the two entities, on which you can operate on
> > > > > from userspace.
> > > > >
> > > > > Your tuner driver for tda9885 (which I don't see in mainline, so I
> > > > > assume it's downstream or custom code) should register an async subdevice,
> > > > > so that the adv7180 registered subdev-notifier gets matched and your
> > > > > callbacks invoked.
> > > > >
> > > > > If I were you, I would:
> > > > > 1) Add dt-parsing routine to tda7180, to find out if any input
> > > > > endpoint is registered in DT.
> > > >
> > > > Done
> > > >
> > > > > 2) If it is, then register a SINK pad, along with the single SOURCE pad
> > > > > which is registered today.
> > > >
> > > > Done
> > > >
> > > > > 3) When parsing DT, for input endpoints, get a reference to the remote
> > > > > endpoint connected to your input and register a subdev-notifier
> > > >
> > > > Done
> > > >
> > > > > 4) Fill in the notifier 'bound' callback and register the link to
> > > > > your remote device there.
> > > >
> > > > Both are subdevice that has not a v4l2_device, so bound is not called from two
> > > > sub-device. Is this expected?
> > >
> > > That should not be an issue. As we discussed, I slightly misleaded
> > > you, pointing you to rcar-csi2, which implements a 'custom' matching
> > > logic, trying to match its remote on endpoints and not on device node.
> > >
> > >         priv->asd.match.fwnode =
> > >                 fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> > >
> > > I'm sorry about this.
> > >
> > > You better use something like:
> > >
> > >         asd->match.fwnode =
> > >                 fwnode_graph_get_remote_port_parent(endpoint);
> > >
> > > or the recently introduced 'v4l2_async_notifier_parse_fwnode_endpoints_by_port()'
> > > function, that does most of that for you.
> > >
> >
> > - entity 80: adv7180 2-0020 (2 pads, 5 links)
> >              type V4L2 subdev subtype Decoder flags 0
> >              device node name /dev/v4l-subdev11
> > pad0: Sink
> > [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
> > <- "ipu1_csi0_mux":4 []
> > -> "ipu1_csi0_mux":4 []
> > <- "tda9887":1 [ENABLED,IMMUTABLE]
> > pad1: Source
> > [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
> > -> "tda9887":1 []
> > <- "tda9887":1 []
> >
> > - entity 83: tda9887 (2 pads, 3 links)
> >              type V4L2 subdev subtype Unknown flags 0
> > pad0: Sink
> > pad1: Source
> > <- "adv7180 2-0020":1 []
> > -> "adv7180 2-0020":0 [ENABLED,IMMUTABLE]
> > -> "adv7180 2-0020":1 []
> >
> >
> > Now the only problem is that I have a link in the graph that I have
> > not defined that not le me to stream. Look and png file I can see an
> > hard link from tda9887 to csi. Do you know why is coming?
> >
>
> I don't see any link between tda and csi in the snippet you pasted
> above (nor I see a .png representing the media graph attached).
>
> What I see is the link: '"adv7180 2-0020":0 -> "tda9887":1' which is
> fine, but you're missing a link '"adv7180 2-0020":1 -> "ipu1_csi0_mux":4'
>
> From what I see your DTS (or parsing routines, I can't tell without
> the seeing the code) links  adv7180:1->tda9887:1 which is a
> source->source link, and the same time creates an
> adv7180:0->ipu1_csi0_mux:4 which is a sink->sink link.
>
> If you fix that by creating instead a adv7180:1->ipu1_csi0_mux:4 you
> should be fine (provided you keep the tda9887:1->adv7180:0 link you have
> already).
>
> If you send patches, we can comment further, otherwise it gets hard
> without seeing what's happening for real.
>
> Thanks
>    j
>
> > Michael
> >
> > > Sorry about this.
> > > Thanks
> > >    j
> > >
> > > >
> > > >
> > > > > 5) Make sure your tuner driver registers its subdevice with
> > > > > v4l2_async_register_subdev()
> > > > >
> > > > > A good example on how to register subdev notifier is provided in the
> > > > > rcsi2_parse_dt() function in rcar-csi2.c driver (I'm quite sure imx
> > > > > now uses subdev notifiers from v4.19 on too if you want to have a look
> > > > > there).
> > > >
> > > > Already seen it
> > > >
> > > > >
> > > > > -- Entering slippery territory here: you might want more inputs on this
> > > > >
> > > > > To make thing simpler&nicer (TM), if you blindly do what I've suggested
> > > > > here, you're going to break all current adv7180 users in mainline :(
> > > > >
> > > > > That's because the v4l2-async design 'completes' the root notifier,
> > > > > only if all subdev-notifiers connected to it have completed first.
> > > > > If you add a notifier for the adv7180 input ports unconditionally, and
> > > >
> > > > I don't get to complete. So let's proceed by step
> > > >
> > > > Michael
> > > >
> > > > > to the input port is connected a plain simple "composite-video-connector",
> > > > > as all DTs in mainline are doing right now, the newly registered
> > > > > subdev-notifier will never complete, as the "composite-video-connector"
> > > > > does not register any subdevice to match with. Bummer!
> > > > >
> > > > > A quick look in the code base, returns me that, in example:
> > > > > drivers/gpu/drm/meson/meson_drv.c filters on
> > > > > "composite-video-connector" and a few other compatible values. You
> > > > > might want to do the same, and register a notifier if and only if the
> > > > > remote input endpoint is one of those known not to register a
> > > > > subdevice. I'm sure there are other ways to deal with this issue, but
> > > > > that's the best I can come up with...
> > > > > ---
> > > > >
> > > > > Hope this is reasonably clear and that I'm not misleading you. I had to
> > > > > use adv7180 recently, and its single pad design confused me a bit as well :)
> > > > >
> > > > > Thanks
> > > > >   j
> > > > >
> > > > > > Michael
> > > > > >
> > > > > > > > > Jagan.
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > >
> > > >
> > > >
> > > > --
> > > > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > > > | COO  -  Founder                                      Cruquiuskade 47 |
> > > > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > > > |                  [`as] http://www.amarulasolutions.com               |
> >
> >
> >
> > --
> > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > | COO  -  Founder                                      Cruquiuskade 47 |
> > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > |                  [`as] http://www.amarulasolutions.com               |



-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |

[-- Attachment #2: graph.png --]
[-- Type: image/png, Size: 69888 bytes --]

Re: Configure video PAL decoder into media pipeline

[jacopo mondi]

[-- Attachment #1: Type: text/plain, Size: 19599 bytes --]

On Wed, Dec 12, 2018 at 09:43:23AM +0100, Michael Nazzareno Trimarchi wrote:
> Hi
>
> On Wed, Dec 12, 2018 at 9:39 AM jacopo mondi <jacopo@jmondi.org> wrote:
> >
> > Hi Michael,
> >
> > On Tue, Dec 11, 2018 at 02:53:24PM +0100, Michael Nazzareno Trimarchi wrote:
> > > Hi Jacopo
> > >
> > > On Tue, Dec 11, 2018 at 12:39 PM jacopo mondi <jacopo@jmondi.org> wrote:
> > > >
> > > > Hi Michael,
> > > >
> > > > On Mon, Dec 10, 2018 at 10:45:02PM +0100, Michael Nazzareno Trimarchi wrote:
> > > > > Hi Jacopo
> > > > >
> > > > > Let's see what I have done
> > > > >
> > > > > On Sun, Dec 9, 2018 at 8:39 PM jacopo mondi <jacopo@jmondi.org> wrote:
> > > > > >
> > > > > > Hi Michael, Jagan, Hans,
> > > > > >
>
> media-ctl --links "'tda9887':1->'adv7180 2-0020':1[1]"

According to what you pasted below, this is a source->source link,
which you don't need as you already have
"'tda9887':1->'adv7180 2-0020':0"
enabled and immutable (I would question the immutable, but that's ok
for now)

 - entity 80: adv7180 2-0020 (2 pads, 5 links)
              type V4L2 subdev subtype Decoder flags 0
              device node name /dev/v4l-subdev11
 pad0: Sink
 [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
 <- "ipu1_csi0_mux":4 []                  <--- THIS shouldn't be here
 -> "ipu1_csi0_mux":4 []                  <--- THIS shouldn't be here
 <- "tda9887":1 [ENABLED,IMMUTABLE]
 pad1: Source
 [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
 -> "tda9887":1 []                  <--- THIS shouldn't be here
 <- "tda9887":1 []                  <--- THIS shouldn't be here

 - entity 83: tda9887 (2 pads, 3 links)
              type V4L2 subdev subtype Unknown flags 0
 pad0: Sink
 pad1: Source
 <- "adv7180 2-0020":1 []                  <--- THIS shouldn't be here
 -> "adv7180 2-0020":0 [ENABLED,IMMUTABLE]
 -> "adv7180 2-0020":1 []                  <--- THIS shouldn't be here

So fix your DTS, or your parsing routines, the media graph should look
like

 - entity 80: adv7180 2-0020 (2 pads, 5 links)
              type V4L2 subdev subtype Decoder flags 0
              device node name /dev/v4l-subdev11
 pad0: Sink
 [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
 <- "tda9887":1 [ENABLED,IMMUTABLE]
 pad1: Source
 [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
 -> "ipu1_csi0_mux":4 []

 - entity 83: tda9887 (2 pads, 3 links)
              type V4L2 subdev subtype Unknown flags 0
 pad0: Sink
 pad1: Source
 -> "adv7180 2-0020":0 [ENABLED,IMMUTABLE]

Thanks
   j

> media-ctl --links "'adv7180 2-0020':0->'ipu1_csi0_mux':4[1]"
> media-ctl --links "'ipu1_csi0_mux':5->'ipu1_csi0':0[1]"
> media-ctl --links "'ipu1_csi0':1->'ipu1_vdic':0[1]"
> media-ctl --links "'ipu1_vdic':2->'ipu1_ic_prp':0[1]"
> media-ctl -l "'ipu1_ic_prp':2 -> 'ipu1_ic_prpvf':0[1]"
> media-ctl -l "'ipu1_ic_prpvf':1 -> 'ipu1_ic_prpvf capture':0[1]"
>
> If I try to activate this one or any other go to the end, it's just dealock.
>
> Michael
>
> > > > > > On Sat, Dec 08, 2018 at 06:07:04PM +0100, Michael Nazzareno Trimarchi wrote:
> > > > > > > Hi
> > > > > > >
> > > > > > > Down you have my tentative of connection
> > > > > > >
> > > > > > > I need to hack a bit to have tuner registered. I'm using imx-media
> > > > > > >
> > > > > > > On Sat, Dec 8, 2018 at 12:48 PM Michael Nazzareno Trimarchi
> > > > > > > <michael@amarulasolutions.com> wrote:
> > > > > > > >
> > > > > > > > Hi
> > > > > > > >
> > > > > > > > On Fri, Dec 7, 2018 at 1:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > > > > > > >
> > > > > > > > > On 12/07/2018 12:51 PM, Jagan Teki wrote:
> > > > > > > > > > Hi,
> > > > > > > > > >
> > > > > > > > > > We have some unconventional setup for parallel CSI design where analog
> > > > > > > > > > input data is converted into to digital composite using PAL decoder
> > > > > > > > > > and it feed to adv7180, camera sensor.
> > > > > > > > > >
> > > > > > > > > > Analog input =>  Video PAL Decoder => ADV7180 => IPU-CSI0
> > > > > > > > >
> > > > > > > > > Just PAL? No NTSC support?
> > > > > > > > >
> > > > > > > > For now does not matter. I have registere the TUNER that support it
> > > > > > > > but seems that media-ctl is not suppose to work with the MEDIA_ENT_F_TUNER
> > > > > > > >
> > > > > > > > Is this correct?
> > > > > > > >
> > > > > >
> > > > > > media-types.rst reports:
> > > > > >
> > > > > >     *  -  ``MEDIA_ENT_F_IF_VID_DECODER``
> > > > > >        -  IF-PLL video decoder. It receives the IF from a PLL and decodes
> > > > > >           the analog TV video signal. This is commonly found on some very
> > > > > >           old analog tuners, like Philips MK3 designs. They all contain a
> > > > > >           tda9887 (or some software compatible similar chip, like tda9885).
> > > > > >           Those devices use a different I2C address than the tuner PLL.
> > > > > >
> > > > > > Is this what you were looking for?
> > > > > >
> > > > > > > > > >
> > > > > > > > > > The PAL decoder is I2C based, tda9885 chip. We setup it up via dt
> > > > > > > > > > bindings and the chip is
> > > > > > > > > > detected fine.
> > > > > > > > > >
> > > > > > > > > > But we need to know, is this to be part of media control subdev
> > > > > > > > > > pipeline? so-that we can configure pads, links like what we do on
> > > > > > > > > > conventional pipeline  or it should not to be part of media pipeline?
> > > > > > > > >
> > > > > > > > > Yes, I would say it should be part of the pipeline.
> > > > > > > > >
> > > > > > > >
> > > > > > > > Ok I have created a draft patch to add the adv some new endpoint but
> > > > > > > > is sufficient to declare tuner type in media control?
> > > > > > > >
> > > > > > > > Michael
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Please advise for best possible way to fit this into the design.
> > > > > > > > > >
> > > > > > > > > > Another observation is since the IPU has more than one sink, source
> > > > > > > > > > pads, we source or sink the other components on the pipeline but look
> > > > > > > > > > like the same thing seems not possible with adv7180 since if has only
> > > > > > > > > > one pad. If it has only one pad sourcing to adv7180 from tda9885 seems
> > > > > > > > > > not possible, If I'm not mistaken.
> > > > > > > > >
> > > > > > > > > Correct, in all cases where the adv7180 is used it is directly connected
> > > > > > > > > to the video input connector on a board.
> > > > > > > > >
> > > > > > > > > So to support this the adv7180 driver should be modified to add an input pad
> > > > > > > > > so you can connect the decoder. It will be needed at some point anyway once
> > > > > > > > > we add support for connector entities.
> > > > > > > > >
> > > > > > > > > Regards,
> > > > > > > > >
> > > > > > > > >         Hans
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > I tried to look for similar design in mainline, but I couldn't find
> > > > > > > > > > it. is there any design similar to this in mainline?
> > > > > > > > > >
> > > > > > > > > > Please let us know if anyone has any suggestions on this.
> > > > > > > > > >
> > > > > > >
> > > > > > > [    3.379129] imx-media: ipu1_vdic:2 -> ipu1_ic_prp:0
> > > > > > > [    3.384262] imx-media: ipu2_vdic:2 -> ipu2_ic_prp:0
> > > > > > > [    3.389217] imx-media: ipu1_ic_prp:1 -> ipu1_ic_prpenc:0
> > > > > > > [    3.394616] imx-media: ipu1_ic_prp:2 -> ipu1_ic_prpvf:0
> > > > > > > [    3.399867] imx-media: ipu2_ic_prp:1 -> ipu2_ic_prpenc:0
> > > > > > > [    3.405289] imx-media: ipu2_ic_prp:2 -> ipu2_ic_prpvf:0
> > > > > > > [    3.410552] imx-media: ipu1_csi0:1 -> ipu1_ic_prp:0
> > > > > > > [    3.415502] imx-media: ipu1_csi0:1 -> ipu1_vdic:0
> > > > > > > [    3.420305] imx-media: ipu1_csi0_mux:5 -> ipu1_csi0:0
> > > > > > > [    3.425427] imx-media: ipu1_csi1:1 -> ipu1_ic_prp:0
> > > > > > > [    3.430328] imx-media: ipu1_csi1:1 -> ipu1_vdic:0
> > > > > > > [    3.435142] imx-media: ipu1_csi1_mux:5 -> ipu1_csi1:0
> > > > > > > [    3.440321] imx-media: adv7180 2-0020:1 -> ipu1_csi0_mux:4
> > > > > > >
> > > > > > > with
> > > > > > >        tuner: tuner@43 {
> > > > > > >                 compatible = "tuner";
> > > > > > >                 reg = <0x43>;
> > > > > > >                 pinctrl-names = "default";
> > > > > > >                 pinctrl-0 = <&pinctrl_tuner>;
> > > > > > >
> > > > > > >                 ports {
> > > > > > >                         #address-cells = <1>;
> > > > > > >                         #size-cells = <0>;
> > > > > > >                         port@1 {
> > > > > > >                                 reg = <1>;
> > > > > > >
> > > > > > >                                 tuner_in: endpoint {
> > > > > >
> > > > > > Nit: This is the tuner output, I would call this "tuner_out"
> > > > > >
> > > > >
> > > > > Done
> > > > >
> > > > > > >                                         remote-endpoint = <&tuner_out>;
> > > > > > >                                 };
> > > > > > >                         };
> > > > > > >                 };
> > > > > > >         };
> > > > > > >
> > > > > > >         adv7180: camera@20 {
> > > > > > >                 compatible = "adi,adv7180";
> > > > > >
> > > > > > One minor thing first: look at the adv7180 bindings documentation, and
> > > > > > you'll find out that only devices compatible with "adv7180cp" and
> > > > > > "adv7180st" shall declare a 'ports' node. This is minor issues (also,
> > > > > > I don't see it enforced in driver's code, but still worth pointing it
> > > > > > out from the very beginning)
> > > > > >
> > > > > > >                 reg = <0x20>;
> > > > > > >                 pinctrl-names = "default";
> > > > > > >                 pinctrl-0 = <&pinctrl_adv7180>;
> > > > > > >                 powerdown-gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; /* PDEC_PWRDN */
> > > > > > >
> > > > > > >                 ports {
> > > > > > >                         #address-cells = <1>;
> > > > > > >                         #size-cells = <0>;
> > > > > > >
> > > > > > >                         port@1 {
> > > > > > >                                 reg = <1>;
> > > > > > >
> > > > > > >                                 adv7180_to_ipu1_csi0_mux: endpoint {
> > > > > > >                                         remote-endpoint =
> > > > > > > <&ipu1_csi0_mux_from_parallel_sensor>;
> > > > > > >                                         bus-width = <8>;
> > > > > > >                                 };
> > > > > > >                         };
> > > > > > >
> > > > > > >                         port@0 {
> > > > > > >                                 reg = <0>;
> > > > > > >
> > > > > > >                                 tuner_out: endpoint {
> > > > > >
> > > > > > Nit: That's an adv7180 endpoint, I would call it 'adv7180_in'
> > > > > >
> > > > >
> > > > > Done
> > > > >
> > > > > > >                                         remote-endpoint = <&tuner_in>;
> > > > > > >                                 };
> > > > > > >                         };
> > > > > > >                 };
> > > > > > >         };
> > > > > > >
> > > > > > > Any help is appreciate
> > > > > > >
> > > > > >
> > > > > > The adv7180(cp|st) bindings says the device can declare one (or more)
> > > > > > input endpoints, but that's just to make possible to connect in device
> > > > > > tree the 7180's device node with the video input port. You can see an
> > > > > > example in arch/arm64/boot/dts/renesas/r8a77995-draak.dts which is
> > > > > > similar to what you've done here.
> > > > > >
> > > > > > The video input port does not show up in the media graph, as it is
> > > > > > just a 'place holder' to describe an input port in DTs, the
> > > > > > adv7180 driver does not register any sink pad, where to connect any
> > > > > > video source to.
> > > > > >
> > > > > > Your proposed bindings here look almost correct, but to have it
> > > > > > working for real you should add a sink pad to the adv7180 registered
> > > > > > media entity (possibly only conditionally to the presence of an input
> > > > > > endpoint in DTs...). You should then register a subdev-notifier, which
> > > > > > registers on an async-subdevice that uses the remote endpoint
> > > > > > connected to your newly registered input pad to find out which device
> > > > > > you're linked to; then at 'bound' (or possibly 'complete') time
> > > > > > register a link between the two entities, on which you can operate on
> > > > > > from userspace.
> > > > > >
> > > > > > Your tuner driver for tda9885 (which I don't see in mainline, so I
> > > > > > assume it's downstream or custom code) should register an async subdevice,
> > > > > > so that the adv7180 registered subdev-notifier gets matched and your
> > > > > > callbacks invoked.
> > > > > >
> > > > > > If I were you, I would:
> > > > > > 1) Add dt-parsing routine to tda7180, to find out if any input
> > > > > > endpoint is registered in DT.
> > > > >
> > > > > Done
> > > > >
> > > > > > 2) If it is, then register a SINK pad, along with the single SOURCE pad
> > > > > > which is registered today.
> > > > >
> > > > > Done
> > > > >
> > > > > > 3) When parsing DT, for input endpoints, get a reference to the remote
> > > > > > endpoint connected to your input and register a subdev-notifier
> > > > >
> > > > > Done
> > > > >
> > > > > > 4) Fill in the notifier 'bound' callback and register the link to
> > > > > > your remote device there.
> > > > >
> > > > > Both are subdevice that has not a v4l2_device, so bound is not called from two
> > > > > sub-device. Is this expected?
> > > >
> > > > That should not be an issue. As we discussed, I slightly misleaded
> > > > you, pointing you to rcar-csi2, which implements a 'custom' matching
> > > > logic, trying to match its remote on endpoints and not on device node.
> > > >
> > > >         priv->asd.match.fwnode =
> > > >                 fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> > > >
> > > > I'm sorry about this.
> > > >
> > > > You better use something like:
> > > >
> > > >         asd->match.fwnode =
> > > >                 fwnode_graph_get_remote_port_parent(endpoint);
> > > >
> > > > or the recently introduced 'v4l2_async_notifier_parse_fwnode_endpoints_by_port()'
> > > > function, that does most of that for you.
> > > >
> > >
> > > - entity 80: adv7180 2-0020 (2 pads, 5 links)
> > >              type V4L2 subdev subtype Decoder flags 0
> > >              device node name /dev/v4l-subdev11
> > > pad0: Sink
> > > [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
> > > <- "ipu1_csi0_mux":4 []
> > > -> "ipu1_csi0_mux":4 []
> > > <- "tda9887":1 [ENABLED,IMMUTABLE]
> > > pad1: Source
> > > [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
> > > -> "tda9887":1 []
> > > <- "tda9887":1 []
> > >
> > > - entity 83: tda9887 (2 pads, 3 links)
> > >              type V4L2 subdev subtype Unknown flags 0
> > > pad0: Sink
> > > pad1: Source
> > > <- "adv7180 2-0020":1 []
> > > -> "adv7180 2-0020":0 [ENABLED,IMMUTABLE]
> > > -> "adv7180 2-0020":1 []
> > >
> > >
> > > Now the only problem is that I have a link in the graph that I have
> > > not defined that not le me to stream. Look and png file I can see an
> > > hard link from tda9887 to csi. Do you know why is coming?
> > >
> >
> > I don't see any link between tda and csi in the snippet you pasted
> > above (nor I see a .png representing the media graph attached).
> >
> > What I see is the link: '"adv7180 2-0020":0 -> "tda9887":1' which is
> > fine, but you're missing a link '"adv7180 2-0020":1 -> "ipu1_csi0_mux":4'
> >
> > From what I see your DTS (or parsing routines, I can't tell without
> > the seeing the code) links  adv7180:1->tda9887:1 which is a
> > source->source link, and the same time creates an
> > adv7180:0->ipu1_csi0_mux:4 which is a sink->sink link.
> >
> > If you fix that by creating instead a adv7180:1->ipu1_csi0_mux:4 you
> > should be fine (provided you keep the tda9887:1->adv7180:0 link you have
> > already).
> >
> > If you send patches, we can comment further, otherwise it gets hard
> > without seeing what's happening for real.
> >
> > Thanks
> >    j
> >
> > > Michael
> > >
> > > > Sorry about this.
> > > > Thanks
> > > >    j
> > > >
> > > > >
> > > > >
> > > > > > 5) Make sure your tuner driver registers its subdevice with
> > > > > > v4l2_async_register_subdev()
> > > > > >
> > > > > > A good example on how to register subdev notifier is provided in the
> > > > > > rcsi2_parse_dt() function in rcar-csi2.c driver (I'm quite sure imx
> > > > > > now uses subdev notifiers from v4.19 on too if you want to have a look
> > > > > > there).
> > > > >
> > > > > Already seen it
> > > > >
> > > > > >
> > > > > > -- Entering slippery territory here: you might want more inputs on this
> > > > > >
> > > > > > To make thing simpler&nicer (TM), if you blindly do what I've suggested
> > > > > > here, you're going to break all current adv7180 users in mainline :(
> > > > > >
> > > > > > That's because the v4l2-async design 'completes' the root notifier,
> > > > > > only if all subdev-notifiers connected to it have completed first.
> > > > > > If you add a notifier for the adv7180 input ports unconditionally, and
> > > > >
> > > > > I don't get to complete. So let's proceed by step
> > > > >
> > > > > Michael
> > > > >
> > > > > > to the input port is connected a plain simple "composite-video-connector",
> > > > > > as all DTs in mainline are doing right now, the newly registered
> > > > > > subdev-notifier will never complete, as the "composite-video-connector"
> > > > > > does not register any subdevice to match with. Bummer!
> > > > > >
> > > > > > A quick look in the code base, returns me that, in example:
> > > > > > drivers/gpu/drm/meson/meson_drv.c filters on
> > > > > > "composite-video-connector" and a few other compatible values. You
> > > > > > might want to do the same, and register a notifier if and only if the
> > > > > > remote input endpoint is one of those known not to register a
> > > > > > subdevice. I'm sure there are other ways to deal with this issue, but
> > > > > > that's the best I can come up with...
> > > > > > ---
> > > > > >
> > > > > > Hope this is reasonably clear and that I'm not misleading you. I had to
> > > > > > use adv7180 recently, and its single pad design confused me a bit as well :)
> > > > > >
> > > > > > Thanks
> > > > > >   j
> > > > > >
> > > > > > > Michael
> > > > > > >
> > > > > > > > > > Jagan.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > > > > | COO  -  Founder                                      Cruquiuskade 47 |
> > > > > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > > > > |                  [`as] http://www.amarulasolutions.com               |
> > >
> > >
> > >
> > > --
> > > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > > | COO  -  Founder                                      Cruquiuskade 47 |
> > > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > > |                  [`as] http://www.amarulasolutions.com               |
>
>
>
> --
> | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> | COO  -  Founder                                      Cruquiuskade 47 |
> | +31(0)851119172                                 Amsterdam 1018 AM NL |
> |                  [`as] http://www.amarulasolutions.com               |



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: Configure video PAL decoder into media pipeline

[Michael Nazzareno Trimarchi]

Hi

On Wed, Dec 12, 2018 at 9:55 AM jacopo mondi <jacopo@jmondi.org> wrote:
>
> On Wed, Dec 12, 2018 at 09:43:23AM +0100, Michael Nazzareno Trimarchi wrote:
> > Hi
> >
> > On Wed, Dec 12, 2018 at 9:39 AM jacopo mondi <jacopo@jmondi.org> wrote:
> > >
> > > Hi Michael,
> > >
> > > On Tue, Dec 11, 2018 at 02:53:24PM +0100, Michael Nazzareno Trimarchi wrote:
> > > > Hi Jacopo
> > > >
> > > > On Tue, Dec 11, 2018 at 12:39 PM jacopo mondi <jacopo@jmondi.org> wrote:
> > > > >
> > > > > Hi Michael,
> > > > >
> > > > > On Mon, Dec 10, 2018 at 10:45:02PM +0100, Michael Nazzareno Trimarchi wrote:
> > > > > > Hi Jacopo
> > > > > >
> > > > > > Let's see what I have done
> > > > > >
> > > > > > On Sun, Dec 9, 2018 at 8:39 PM jacopo mondi <jacopo@jmondi.org> wrote:
> > > > > > >
> > > > > > > Hi Michael, Jagan, Hans,
> > > > > > >
> >
> > media-ctl --links "'tda9887':1->'adv7180 2-0020':1[1]"
>
> According to what you pasted below, this is a source->source link,
> which you don't need as you already have
> "'tda9887':1->'adv7180 2-0020':0"
> enabled and immutable (I would question the immutable, but that's ok
> for now)
>
>  - entity 80: adv7180 2-0020 (2 pads, 5 links)
>               type V4L2 subdev subtype Decoder flags 0
>               device node name /dev/v4l-subdev11
>  pad0: Sink
>  [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
>  <- "ipu1_csi0_mux":4 []                  <--- THIS shouldn't be here
>  -> "ipu1_csi0_mux":4 []                  <--- THIS shouldn't be here
>  <- "tda9887":1 [ENABLED,IMMUTABLE]
>  pad1: Source
>  [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
>  -> "tda9887":1 []                  <--- THIS shouldn't be here
>  <- "tda9887":1 []                  <--- THIS shouldn't be here
>
>  - entity 83: tda9887 (2 pads, 3 links)
>               type V4L2 subdev subtype Unknown flags 0
>  pad0: Sink
>  pad1: Source
>  <- "adv7180 2-0020":1 []                  <--- THIS shouldn't be here
>  -> "adv7180 2-0020":0 [ENABLED,IMMUTABLE]
>  -> "adv7180 2-0020":1 []                  <--- THIS shouldn't be here
>
> So fix your DTS, or your parsing routines, the media graph should look
> like
>

I think last graph was already fine, now it looks correct. Have you
seen the attachment?

>  - entity 80: adv7180 2-0020 (2 pads, 5 links)
>               type V4L2 subdev subtype Decoder flags 0
>               device node name /dev/v4l-subdev11
>  pad0: Sink
>  [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
>  <- "tda9887":1 [ENABLED,IMMUTABLE]
>  pad1: Source
>  [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
>  -> "ipu1_csi0_mux":4 []
>

This was a problem of dangling code in the bind event during
connection. Should be
fine I can post patch shortly to give an overview. The fact is that
any time that a try to close
a link path I have a deadlock down there. Let me test the adv path
without the tda

Michael



>  - entity 83: tda9887 (2 pads, 3 links)
>               type V4L2 subdev subtype Unknown flags 0
>  pad0: Sink
>  pad1: Source
>  -> "adv7180 2-0020":0 [ENABLED,IMMUTABLE]
>
> Thanks
>    j
>
> > media-ctl --links "'adv7180 2-0020':0->'ipu1_csi0_mux':4[1]"
> > media-ctl --links "'ipu1_csi0_mux':5->'ipu1_csi0':0[1]"
> > media-ctl --links "'ipu1_csi0':1->'ipu1_vdic':0[1]"
> > media-ctl --links "'ipu1_vdic':2->'ipu1_ic_prp':0[1]"
> > media-ctl -l "'ipu1_ic_prp':2 -> 'ipu1_ic_prpvf':0[1]"
> > media-ctl -l "'ipu1_ic_prpvf':1 -> 'ipu1_ic_prpvf capture':0[1]"
> >
> > If I try to activate this one or any other go to the end, it's just dealock.
> >
> > Michael
> >
> > > > > > > On Sat, Dec 08, 2018 at 06:07:04PM +0100, Michael Nazzareno Trimarchi wrote:
> > > > > > > > Hi
> > > > > > > >
> > > > > > > > Down you have my tentative of connection
> > > > > > > >
> > > > > > > > I need to hack a bit to have tuner registered. I'm using imx-media
> > > > > > > >
> > > > > > > > On Sat, Dec 8, 2018 at 12:48 PM Michael Nazzareno Trimarchi
> > > > > > > > <michael@amarulasolutions.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi
> > > > > > > > >
> > > > > > > > > On Fri, Dec 7, 2018 at 1:11 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
> > > > > > > > > >
> > > > > > > > > > On 12/07/2018 12:51 PM, Jagan Teki wrote:
> > > > > > > > > > > Hi,
> > > > > > > > > > >
> > > > > > > > > > > We have some unconventional setup for parallel CSI design where analog
> > > > > > > > > > > input data is converted into to digital composite using PAL decoder
> > > > > > > > > > > and it feed to adv7180, camera sensor.
> > > > > > > > > > >
> > > > > > > > > > > Analog input =>  Video PAL Decoder => ADV7180 => IPU-CSI0
> > > > > > > > > >
> > > > > > > > > > Just PAL? No NTSC support?
> > > > > > > > > >
> > > > > > > > > For now does not matter. I have registere the TUNER that support it
> > > > > > > > > but seems that media-ctl is not suppose to work with the MEDIA_ENT_F_TUNER
> > > > > > > > >
> > > > > > > > > Is this correct?
> > > > > > > > >
> > > > > > >
> > > > > > > media-types.rst reports:
> > > > > > >
> > > > > > >     *  -  ``MEDIA_ENT_F_IF_VID_DECODER``
> > > > > > >        -  IF-PLL video decoder. It receives the IF from a PLL and decodes
> > > > > > >           the analog TV video signal. This is commonly found on some very
> > > > > > >           old analog tuners, like Philips MK3 designs. They all contain a
> > > > > > >           tda9887 (or some software compatible similar chip, like tda9885).
> > > > > > >           Those devices use a different I2C address than the tuner PLL.
> > > > > > >
> > > > > > > Is this what you were looking for?
> > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > The PAL decoder is I2C based, tda9885 chip. We setup it up via dt
> > > > > > > > > > > bindings and the chip is
> > > > > > > > > > > detected fine.
> > > > > > > > > > >
> > > > > > > > > > > But we need to know, is this to be part of media control subdev
> > > > > > > > > > > pipeline? so-that we can configure pads, links like what we do on
> > > > > > > > > > > conventional pipeline  or it should not to be part of media pipeline?
> > > > > > > > > >
> > > > > > > > > > Yes, I would say it should be part of the pipeline.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Ok I have created a draft patch to add the adv some new endpoint but
> > > > > > > > > is sufficient to declare tuner type in media control?
> > > > > > > > >
> > > > > > > > > Michael
> > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Please advise for best possible way to fit this into the design.
> > > > > > > > > > >
> > > > > > > > > > > Another observation is since the IPU has more than one sink, source
> > > > > > > > > > > pads, we source or sink the other components on the pipeline but look
> > > > > > > > > > > like the same thing seems not possible with adv7180 since if has only
> > > > > > > > > > > one pad. If it has only one pad sourcing to adv7180 from tda9885 seems
> > > > > > > > > > > not possible, If I'm not mistaken.
> > > > > > > > > >
> > > > > > > > > > Correct, in all cases where the adv7180 is used it is directly connected
> > > > > > > > > > to the video input connector on a board.
> > > > > > > > > >
> > > > > > > > > > So to support this the adv7180 driver should be modified to add an input pad
> > > > > > > > > > so you can connect the decoder. It will be needed at some point anyway once
> > > > > > > > > > we add support for connector entities.
> > > > > > > > > >
> > > > > > > > > > Regards,
> > > > > > > > > >
> > > > > > > > > >         Hans
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > I tried to look for similar design in mainline, but I couldn't find
> > > > > > > > > > > it. is there any design similar to this in mainline?
> > > > > > > > > > >
> > > > > > > > > > > Please let us know if anyone has any suggestions on this.
> > > > > > > > > > >
> > > > > > > >
> > > > > > > > [    3.379129] imx-media: ipu1_vdic:2 -> ipu1_ic_prp:0
> > > > > > > > [    3.384262] imx-media: ipu2_vdic:2 -> ipu2_ic_prp:0
> > > > > > > > [    3.389217] imx-media: ipu1_ic_prp:1 -> ipu1_ic_prpenc:0
> > > > > > > > [    3.394616] imx-media: ipu1_ic_prp:2 -> ipu1_ic_prpvf:0
> > > > > > > > [    3.399867] imx-media: ipu2_ic_prp:1 -> ipu2_ic_prpenc:0
> > > > > > > > [    3.405289] imx-media: ipu2_ic_prp:2 -> ipu2_ic_prpvf:0
> > > > > > > > [    3.410552] imx-media: ipu1_csi0:1 -> ipu1_ic_prp:0
> > > > > > > > [    3.415502] imx-media: ipu1_csi0:1 -> ipu1_vdic:0
> > > > > > > > [    3.420305] imx-media: ipu1_csi0_mux:5 -> ipu1_csi0:0
> > > > > > > > [    3.425427] imx-media: ipu1_csi1:1 -> ipu1_ic_prp:0
> > > > > > > > [    3.430328] imx-media: ipu1_csi1:1 -> ipu1_vdic:0
> > > > > > > > [    3.435142] imx-media: ipu1_csi1_mux:5 -> ipu1_csi1:0
> > > > > > > > [    3.440321] imx-media: adv7180 2-0020:1 -> ipu1_csi0_mux:4
> > > > > > > >
> > > > > > > > with
> > > > > > > >        tuner: tuner@43 {
> > > > > > > >                 compatible = "tuner";
> > > > > > > >                 reg = <0x43>;
> > > > > > > >                 pinctrl-names = "default";
> > > > > > > >                 pinctrl-0 = <&pinctrl_tuner>;
> > > > > > > >
> > > > > > > >                 ports {
> > > > > > > >                         #address-cells = <1>;
> > > > > > > >                         #size-cells = <0>;
> > > > > > > >                         port@1 {
> > > > > > > >                                 reg = <1>;
> > > > > > > >
> > > > > > > >                                 tuner_in: endpoint {
> > > > > > >
> > > > > > > Nit: This is the tuner output, I would call this "tuner_out"
> > > > > > >
> > > > > >
> > > > > > Done
> > > > > >
> > > > > > > >                                         remote-endpoint = <&tuner_out>;
> > > > > > > >                                 };
> > > > > > > >                         };
> > > > > > > >                 };
> > > > > > > >         };
> > > > > > > >
> > > > > > > >         adv7180: camera@20 {
> > > > > > > >                 compatible = "adi,adv7180";
> > > > > > >
> > > > > > > One minor thing first: look at the adv7180 bindings documentation, and
> > > > > > > you'll find out that only devices compatible with "adv7180cp" and
> > > > > > > "adv7180st" shall declare a 'ports' node. This is minor issues (also,
> > > > > > > I don't see it enforced in driver's code, but still worth pointing it
> > > > > > > out from the very beginning)
> > > > > > >
> > > > > > > >                 reg = <0x20>;
> > > > > > > >                 pinctrl-names = "default";
> > > > > > > >                 pinctrl-0 = <&pinctrl_adv7180>;
> > > > > > > >                 powerdown-gpios = <&gpio3 20 GPIO_ACTIVE_LOW>; /* PDEC_PWRDN */
> > > > > > > >
> > > > > > > >                 ports {
> > > > > > > >                         #address-cells = <1>;
> > > > > > > >                         #size-cells = <0>;
> > > > > > > >
> > > > > > > >                         port@1 {
> > > > > > > >                                 reg = <1>;
> > > > > > > >
> > > > > > > >                                 adv7180_to_ipu1_csi0_mux: endpoint {
> > > > > > > >                                         remote-endpoint =
> > > > > > > > <&ipu1_csi0_mux_from_parallel_sensor>;
> > > > > > > >                                         bus-width = <8>;
> > > > > > > >                                 };
> > > > > > > >                         };
> > > > > > > >
> > > > > > > >                         port@0 {
> > > > > > > >                                 reg = <0>;
> > > > > > > >
> > > > > > > >                                 tuner_out: endpoint {
> > > > > > >
> > > > > > > Nit: That's an adv7180 endpoint, I would call it 'adv7180_in'
> > > > > > >
> > > > > >
> > > > > > Done
> > > > > >
> > > > > > > >                                         remote-endpoint = <&tuner_in>;
> > > > > > > >                                 };
> > > > > > > >                         };
> > > > > > > >                 };
> > > > > > > >         };
> > > > > > > >
> > > > > > > > Any help is appreciate
> > > > > > > >
> > > > > > >
> > > > > > > The adv7180(cp|st) bindings says the device can declare one (or more)
> > > > > > > input endpoints, but that's just to make possible to connect in device
> > > > > > > tree the 7180's device node with the video input port. You can see an
> > > > > > > example in arch/arm64/boot/dts/renesas/r8a77995-draak.dts which is
> > > > > > > similar to what you've done here.
> > > > > > >
> > > > > > > The video input port does not show up in the media graph, as it is
> > > > > > > just a 'place holder' to describe an input port in DTs, the
> > > > > > > adv7180 driver does not register any sink pad, where to connect any
> > > > > > > video source to.
> > > > > > >
> > > > > > > Your proposed bindings here look almost correct, but to have it
> > > > > > > working for real you should add a sink pad to the adv7180 registered
> > > > > > > media entity (possibly only conditionally to the presence of an input
> > > > > > > endpoint in DTs...). You should then register a subdev-notifier, which
> > > > > > > registers on an async-subdevice that uses the remote endpoint
> > > > > > > connected to your newly registered input pad to find out which device
> > > > > > > you're linked to; then at 'bound' (or possibly 'complete') time
> > > > > > > register a link between the two entities, on which you can operate on
> > > > > > > from userspace.
> > > > > > >
> > > > > > > Your tuner driver for tda9885 (which I don't see in mainline, so I
> > > > > > > assume it's downstream or custom code) should register an async subdevice,
> > > > > > > so that the adv7180 registered subdev-notifier gets matched and your
> > > > > > > callbacks invoked.
> > > > > > >
> > > > > > > If I were you, I would:
> > > > > > > 1) Add dt-parsing routine to tda7180, to find out if any input
> > > > > > > endpoint is registered in DT.
> > > > > >
> > > > > > Done
> > > > > >
> > > > > > > 2) If it is, then register a SINK pad, along with the single SOURCE pad
> > > > > > > which is registered today.
> > > > > >
> > > > > > Done
> > > > > >
> > > > > > > 3) When parsing DT, for input endpoints, get a reference to the remote
> > > > > > > endpoint connected to your input and register a subdev-notifier
> > > > > >
> > > > > > Done
> > > > > >
> > > > > > > 4) Fill in the notifier 'bound' callback and register the link to
> > > > > > > your remote device there.
> > > > > >
> > > > > > Both are subdevice that has not a v4l2_device, so bound is not called from two
> > > > > > sub-device. Is this expected?
> > > > >
> > > > > That should not be an issue. As we discussed, I slightly misleaded
> > > > > you, pointing you to rcar-csi2, which implements a 'custom' matching
> > > > > logic, trying to match its remote on endpoints and not on device node.
> > > > >
> > > > >         priv->asd.match.fwnode =
> > > > >                 fwnode_graph_get_remote_endpoint(of_fwnode_handle(ep));
> > > > >
> > > > > I'm sorry about this.
> > > > >
> > > > > You better use something like:
> > > > >
> > > > >         asd->match.fwnode =
> > > > >                 fwnode_graph_get_remote_port_parent(endpoint);
> > > > >
> > > > > or the recently introduced 'v4l2_async_notifier_parse_fwnode_endpoints_by_port()'
> > > > > function, that does most of that for you.
> > > > >
> > > >
> > > > - entity 80: adv7180 2-0020 (2 pads, 5 links)
> > > >              type V4L2 subdev subtype Decoder flags 0
> > > >              device node name /dev/v4l-subdev11
> > > > pad0: Sink
> > > > [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
> > > > <- "ipu1_csi0_mux":4 []
> > > > -> "ipu1_csi0_mux":4 []
> > > > <- "tda9887":1 [ENABLED,IMMUTABLE]
> > > > pad1: Source
> > > > [fmt:UYVY8_2X8/720x240@1001/30000 field:alternate colorspace:smpte170m]
> > > > -> "tda9887":1 []
> > > > <- "tda9887":1 []
> > > >
> > > > - entity 83: tda9887 (2 pads, 3 links)
> > > >              type V4L2 subdev subtype Unknown flags 0
> > > > pad0: Sink
> > > > pad1: Source
> > > > <- "adv7180 2-0020":1 []
> > > > -> "adv7180 2-0020":0 [ENABLED,IMMUTABLE]
> > > > -> "adv7180 2-0020":1 []
> > > >
> > > >
> > > > Now the only problem is that I have a link in the graph that I have
> > > > not defined that not le me to stream. Look and png file I can see an
> > > > hard link from tda9887 to csi. Do you know why is coming?
> > > >
> > >
> > > I don't see any link between tda and csi in the snippet you pasted
> > > above (nor I see a .png representing the media graph attached).
> > >
> > > What I see is the link: '"adv7180 2-0020":0 -> "tda9887":1' which is
> > > fine, but you're missing a link '"adv7180 2-0020":1 -> "ipu1_csi0_mux":4'
> > >
> > > From what I see your DTS (or parsing routines, I can't tell without
> > > the seeing the code) links  adv7180:1->tda9887:1 which is a
> > > source->source link, and the same time creates an
> > > adv7180:0->ipu1_csi0_mux:4 which is a sink->sink link.
> > >
> > > If you fix that by creating instead a adv7180:1->ipu1_csi0_mux:4 you
> > > should be fine (provided you keep the tda9887:1->adv7180:0 link you have
> > > already).
> > >
> > > If you send patches, we can comment further, otherwise it gets hard
> > > without seeing what's happening for real.
> > >
> > > Thanks
> > >    j
> > >
> > > > Michael
> > > >
> > > > > Sorry about this.
> > > > > Thanks
> > > > >    j
> > > > >
> > > > > >
> > > > > >
> > > > > > > 5) Make sure your tuner driver registers its subdevice with
> > > > > > > v4l2_async_register_subdev()
> > > > > > >
> > > > > > > A good example on how to register subdev notifier is provided in the
> > > > > > > rcsi2_parse_dt() function in rcar-csi2.c driver (I'm quite sure imx
> > > > > > > now uses subdev notifiers from v4.19 on too if you want to have a look
> > > > > > > there).
> > > > > >
> > > > > > Already seen it
> > > > > >
> > > > > > >
> > > > > > > -- Entering slippery territory here: you might want more inputs on this
> > > > > > >
> > > > > > > To make thing simpler&nicer (TM), if you blindly do what I've suggested
> > > > > > > here, you're going to break all current adv7180 users in mainline :(
> > > > > > >
> > > > > > > That's because the v4l2-async design 'completes' the root notifier,
> > > > > > > only if all subdev-notifiers connected to it have completed first.
> > > > > > > If you add a notifier for the adv7180 input ports unconditionally, and
> > > > > >
> > > > > > I don't get to complete. So let's proceed by step
> > > > > >
> > > > > > Michael
> > > > > >
> > > > > > > to the input port is connected a plain simple "composite-video-connector",
> > > > > > > as all DTs in mainline are doing right now, the newly registered
> > > > > > > subdev-notifier will never complete, as the "composite-video-connector"
> > > > > > > does not register any subdevice to match with. Bummer!
> > > > > > >
> > > > > > > A quick look in the code base, returns me that, in example:
> > > > > > > drivers/gpu/drm/meson/meson_drv.c filters on
> > > > > > > "composite-video-connector" and a few other compatible values. You
> > > > > > > might want to do the same, and register a notifier if and only if the
> > > > > > > remote input endpoint is one of those known not to register a
> > > > > > > subdevice. I'm sure there are other ways to deal with this issue, but
> > > > > > > that's the best I can come up with...
> > > > > > > ---
> > > > > > >
> > > > > > > Hope this is reasonably clear and that I'm not misleading you. I had to
> > > > > > > use adv7180 recently, and its single pad design confused me a bit as well :)
> > > > > > >
> > > > > > > Thanks
> > > > > > >   j
> > > > > > >
> > > > > > > > Michael
> > > > > > > >
> > > > > > > > > > > Jagan.
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > > > > > | COO  -  Founder                                      Cruquiuskade 47 |
> > > > > > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > > > > > |                  [`as] http://www.amarulasolutions.com               |
> > > >
> > > >
> > > >
> > > > --
> > > > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > > > | COO  -  Founder                                      Cruquiuskade 47 |
> > > > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > > > |                  [`as] http://www.amarulasolutions.com               |
> >
> >
> >
> > --
> > | Michael Nazzareno Trimarchi                     Amarula Solutions BV |
> > | COO  -  Founder                                      Cruquiuskade 47 |
> > | +31(0)851119172                                 Amsterdam 1018 AM NL |
> > |                  [`as] http://www.amarulasolutions.com               |
>
>


-- 
| Michael Nazzareno Trimarchi                     Amarula Solutions BV |
| COO  -  Founder                                      Cruquiuskade 47 |
| +31(0)851119172                                 Amsterdam 1018 AM NL |
|                  [`as] http://www.amarulasolutions.com               |