* Configure video PAL decoder into media pipeline @ 2018-12-07 11:51 Jagan Teki 2018-12-07 12:11 ` Hans Verkuil 0 siblings, 1 reply; 13+ messages in thread From: Jagan Teki @ 2018-12-07 11:51 UTC (permalink / raw) To: Mauro Carvalho Chehab, linux-media, linux-kernel, Lars-Peter Clausen, Philipp Zabel Cc: Michael Trimarchi 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Configure video PAL decoder into media pipeline 2018-12-07 11:51 Configure video PAL decoder into media pipeline Jagan Teki @ 2018-12-07 12:11 ` Hans Verkuil 2018-12-08 11:48 ` Michael Nazzareno Trimarchi 0 siblings, 1 reply; 13+ messages in thread From: Hans Verkuil @ 2018-12-07 12:11 UTC (permalink / raw) To: Jagan Teki, Mauro Carvalho Chehab, linux-media, linux-kernel, Lars-Peter Clausen, Philipp Zabel Cc: Michael Trimarchi 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. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Configure video PAL decoder into media pipeline 2018-12-07 12:11 ` Hans Verkuil @ 2018-12-08 11:48 ` Michael Nazzareno Trimarchi 2018-12-08 17:07 ` Michael Nazzareno Trimarchi 0 siblings, 1 reply; 13+ messages in thread From: Michael Nazzareno Trimarchi @ 2018-12-08 11:48 UTC (permalink / raw) To: hverkuil Cc: Jagan Teki, mchehab, linux-media, LKML, Lars-Peter Clausen, Philipp Zabel 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 | ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Configure video PAL decoder into media pipeline 2018-12-08 11:48 ` Michael Nazzareno Trimarchi @ 2018-12-08 17:07 ` Michael Nazzareno Trimarchi 2018-12-09 19:39 ` jacopo mondi 0 siblings, 1 reply; 13+ messages in thread From: Michael Nazzareno Trimarchi @ 2018-12-08 17:07 UTC (permalink / raw) To: hverkuil Cc: Jagan Teki, mchehab, linux-media, LKML, Lars-Peter Clausen, Philipp Zabel 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. > > > > > > > > -- ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Configure video PAL decoder into media pipeline 2018-12-08 17:07 ` Michael Nazzareno Trimarchi @ 2018-12-09 19:39 ` jacopo mondi 2018-12-09 20:06 ` Michael Nazzareno Trimarchi 2018-12-10 21:45 ` Michael Nazzareno Trimarchi 0 siblings, 2 replies; 13+ messages in thread From: jacopo mondi @ 2018-12-09 19:39 UTC (permalink / raw) To: Michael Nazzareno Trimarchi Cc: hverkuil, Jagan Teki, mchehab, linux-media, LKML, Lars-Peter Clausen, Philipp Zabel [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Configure video PAL decoder into media pipeline 2018-12-09 19:39 ` jacopo mondi @ 2018-12-09 20:06 ` Michael Nazzareno Trimarchi 2018-12-10 21:45 ` Michael Nazzareno Trimarchi 1 sibling, 0 replies; 13+ messages in thread From: Michael Nazzareno Trimarchi @ 2018-12-09 20:06 UTC (permalink / raw) To: jacopo Cc: hverkuil, Jagan Teki, mchehab, linux-media, LKML, Lars-Peter Clausen, Philipp Zabel 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 | ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Configure video PAL decoder into media pipeline 2018-12-09 19:39 ` jacopo mondi 2018-12-09 20:06 ` Michael Nazzareno Trimarchi @ 2018-12-10 21:45 ` Michael Nazzareno Trimarchi 2018-12-11 11:39 ` jacopo mondi 1 sibling, 1 reply; 13+ messages in thread From: Michael Nazzareno Trimarchi @ 2018-12-10 21:45 UTC (permalink / raw) To: jacopo Cc: hverkuil, Jagan Teki, mchehab, linux-media, LKML, Lars-Peter Clausen, Philipp Zabel 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 | ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Configure video PAL decoder into media pipeline 2018-12-10 21:45 ` Michael Nazzareno Trimarchi @ 2018-12-11 11:39 ` jacopo mondi 2018-12-11 13:53 ` Michael Nazzareno Trimarchi 0 siblings, 1 reply; 13+ messages in thread From: jacopo mondi @ 2018-12-11 11:39 UTC (permalink / raw) To: Michael Nazzareno Trimarchi Cc: hverkuil, Jagan Teki, mchehab, linux-media, LKML, Lars-Peter Clausen, Philipp Zabel [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Configure video PAL decoder into media pipeline 2018-12-11 11:39 ` jacopo mondi @ 2018-12-11 13:53 ` Michael Nazzareno Trimarchi 2018-12-12 8:39 ` jacopo mondi 0 siblings, 1 reply; 13+ messages in thread From: Michael Nazzareno Trimarchi @ 2018-12-11 13:53 UTC (permalink / raw) To: jacopo Cc: hverkuil, Jagan Teki, mchehab, linux-media, LKML, Lars-Peter Clausen, Philipp Zabel 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 | ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Configure video PAL decoder into media pipeline 2018-12-11 13:53 ` Michael Nazzareno Trimarchi @ 2018-12-12 8:39 ` jacopo mondi 2018-12-12 8:43 ` Michael Nazzareno Trimarchi 0 siblings, 1 reply; 13+ messages in thread From: jacopo mondi @ 2018-12-12 8:39 UTC (permalink / raw) To: Michael Nazzareno Trimarchi Cc: hverkuil, Jagan Teki, mchehab, linux-media, LKML, Lars-Peter Clausen, Philipp Zabel [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Configure video PAL decoder into media pipeline 2018-12-12 8:39 ` jacopo mondi @ 2018-12-12 8:43 ` Michael Nazzareno Trimarchi 2018-12-12 8:54 ` jacopo mondi 0 siblings, 1 reply; 13+ messages in thread From: Michael Nazzareno Trimarchi @ 2018-12-12 8:43 UTC (permalink / raw) To: jacopo Cc: hverkuil, Jagan Teki, mchehab, linux-media, LKML, Lars-Peter Clausen, Philipp Zabel [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Configure video PAL decoder into media pipeline 2018-12-12 8:43 ` Michael Nazzareno Trimarchi @ 2018-12-12 8:54 ` jacopo mondi 2018-12-12 9:09 ` Michael Nazzareno Trimarchi 0 siblings, 1 reply; 13+ messages in thread From: jacopo mondi @ 2018-12-12 8:54 UTC (permalink / raw) To: Michael Nazzareno Trimarchi Cc: hverkuil, Jagan Teki, mchehab, linux-media, LKML, Lars-Peter Clausen, Philipp Zabel [-- 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 --] ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: Configure video PAL decoder into media pipeline 2018-12-12 8:54 ` jacopo mondi @ 2018-12-12 9:09 ` Michael Nazzareno Trimarchi 0 siblings, 0 replies; 13+ messages in thread From: Michael Nazzareno Trimarchi @ 2018-12-12 9:09 UTC (permalink / raw) To: jacopo Cc: hverkuil, Jagan Teki, mchehab, linux-media, LKML, Lars-Peter Clausen, Philipp Zabel 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 | ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-12-12 9:09 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-07 11:51 Configure video PAL decoder into media pipeline Jagan Teki 2018-12-07 12:11 ` Hans Verkuil 2018-12-08 11:48 ` Michael Nazzareno Trimarchi 2018-12-08 17:07 ` Michael Nazzareno Trimarchi 2018-12-09 19:39 ` jacopo mondi 2018-12-09 20:06 ` Michael Nazzareno Trimarchi 2018-12-10 21:45 ` Michael Nazzareno Trimarchi 2018-12-11 11:39 ` jacopo mondi 2018-12-11 13:53 ` Michael Nazzareno Trimarchi 2018-12-12 8:39 ` jacopo mondi 2018-12-12 8:43 ` Michael Nazzareno Trimarchi 2018-12-12 8:54 ` jacopo mondi 2018-12-12 9:09 ` Michael Nazzareno Trimarchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).