Hi Niklas, On Mon, Aug 27, 2018 at 03:23:45PM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > On 2018-08-27 11:49:56 +0200, Jacopo Mondi wrote: > > Hi Niklas, > > A few more talk on the adv748x link handling, please bear with me... > > > > On Sat, Aug 25, 2018 at 03:18:06PM +0200, jacopo mondi wrote: > > > Hi Laurent, Niklas, > > > > > > On Sat, Aug 25, 2018 at 08:37:15AM +0200, Niklas Söderlund wrote: > > > > Hi Laurent and Jacopo, > > > > > > > > On 2018-08-25 02:54:44 +0300, Laurent Pinchart wrote: > > > > > Hi Jacopo, > > > > > > > > > > On Monday, 20 August 2018 13:16:34 EEST Jacopo Mondi wrote: > > > > > > Hello renesas list, > > > > > > this series add supports for the HDMI and CVBS input to R-Car E3 R8A77990 > > > > > > Ebisu board. > > > > > > > > > > > > It's an RFT, as I don't have an Ebisu to test with :( > > > > > > > > > > > > The series adds supports for the following items: > > > > > > > > > > > > - PFC: add VIN groups and functions > > > > > > - R-Car VIN and R-Car CSI-2: add support for R8A77990 > > > > > > - R8A77990: Add I2C, VIN and CSI-2 nodes > > > > > > - Ebisu: describe HDMI and CVBS inputs > > > > > > > > > > > > Each patch, when relevant reports difference between the upported BSP patch > > > > > > and the proposed one. > > > > > > > > > > > > I know Laurent should receive an Ebisu sooner or later, maybe we can sync > > > > > > for testing :) > > > > > > > > > > I've given the series a first test, and I think a bit more work is needed :-) > > > > > > > > > > [ 1.455533] adv748x 0-0070: Endpoint /soc/i2c@e6500000/video-receiver@70/ > > > > > port@7/endpoint on port 7 > > > > > [ 1.464683] adv748x 0-0070: Endpoint /soc/i2c@e6500000/video-receiver@70/ > > > > > port@8/endpoint on port 8 > > > > > [ 1.473728] adv748x 0-0070: Endpoint /soc/i2c@e6500000/video-receiver@70/ > > > > > port@a/endpoint on port 10 > > > > > [ 1.484835] adv748x 0-0070: chip found @ 0xe0 revision 2143 > > > > > [ 1.639470] adv748x 0-0070: No endpoint found for txb > > > > > [ 1.644653] adv748x 0-0070: Failed to probe TXB > > > > > > > > I fear this is a design choice in the adv748x driver. Currently the > > > > driver requires both of its two CSI-2 transmitters to be connected/used > > > > else probe fails. Furthermore the HDMI capture is always routed to TXA > > > > while the analog capture is always routed to TXB. > > > > > > > > Now that we have a board where only TXA is connected but both HDMI and > > > > analog captures are used maybe it's time to do some more work on v4l2 > > > > and the adv748x driver ;-P What's missing: > > > > > > > > - Probe should be OK with either TXA or TXB connected and not bail if > > > > not both are used. > > > > > > I have three patches for this I hope to share as soon as I'll be able > > > to do some more testing > > > > > > > - The media_device_ops or at least the .link_notify() callback of that > > > > struct must be changed so not one driver in the media graph is > > > > responsible for all links. In this case rcar-vin provides the callback > > > > and rcar-vin should not judge which links between the adv748x > > > > subdevices are OK to enable/disable. Currently the links between the > > > > adv748x subdevices are immutably enabled to avoid this particular > > > > problem. > > > > > > Uh, I didn't get this :/ Care to elaborate more? > > > > > > > I'm thinking if it is not enough to just provide a .link_setup() > > callback to the (enabled) csi-2 sink pads of the adv748x and handle > > routing between the afe/hdmi sources and that sink, without the vin's > > registered link_notify playing any role in that. > > That is my point, the v4l2 framework needs work to allow all drivers to > provide a .link_setup() callback. And this as you describe will conflict > with the current solution where there is only one possible such > callback. So in addition to being able to have multiple such callbacks > the current drivers implementing one would need to be updated to ignore > links which it do not care about. > > In our case the .link_setup() in rcar-vin should not care about the > links between the adv748x internal subdevice. Of course the other way > around is also true, the .link_setup() in adv748x should not care about > the links handled by rcar-vin :-) > > As you discovers this is not possible today and might require some work > or maybe even a different design then the one outlined above. > Myabe I'm missing some piece for sure, but why do you think it is not possible? I've applied, for testing, the following patch: https://paste.debian.net/1039515/ And that's the output I get * At system boot, no link is enabled between the HDMI backend and the TXA output: [root@alarm ~]# media-ctl -p -d /dev/media2 .... - entity 7: adv748x 4-0070 txa (2 pads, 2 links) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev21 pad0: Sink [fmt:unknown/0x0] <- "adv748x 4-0070 hdmi":1 [] pad1: Source [fmt:unknown/0x0] -> "rcar_csi2 feaa0000.csi2":0 [ENABLED,IMMUTABLE] - entity 10: adv748x 4-0070 hdmi (2 pads, 1 link) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev20 pad0: Sink [dv.caps:BT.656/1120 min:640x480@13000000 max:1920x1200@162000000 stds:CEA-861,DMT caps:progressive] pad1: Source [fmt:RGB888_1X24/1280x720 field:none colorspace:srgb] [dv.caps:BT.656/1120 min:640x480@13000000 max:1920x1200@162000000 stds:CEA-861,DMT caps:progressive] [dv.query:no-link] [dv.current:BT.656/1120 1280x720p30 (3300x750) stds:CEA-861 flags:can-reduce-fps,CE-video,has-cea861-vic] -> "adv748x 4-0070 txa":0 [] ... * Enable the link, and see that .link_setup() has been called for the adv748x [root@alarm ~]# media-ctl -v -d /dev/media2 -l "'adv748x 4-0070 hdmi':1 -> 'adv748x 4-0070 txa':0 [1]" [root@alarm ~]# dmesg | tail ... [ 183.294541] rvin_group_link_notify:126 SOURCE adv748x 4-0070 hdmi -> SINK adv748x 4-0070 txa [ 183.304663] adv748x_link_setup:332 LOCAL adv748x 4-0070 hdmi -> REMOTE adv748x 4-0070 txa [ 183.314207] adv748x_link_setup:332 LOCAL adv748x 4-0070 txa -> REMOTE adv748x 4-0070 hdmi [ 183.323720] rvin_group_link_notify:126 SOURCE adv748x 4-0070 hdmi -> SINK adv748x 4-0070 txa [root@alarm ~]# media-ctl -p -d /dev/media2 .... - entity 7: adv748x 4-0070 txa (2 pads, 2 links) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev21 pad0: Sink [fmt:unknown/0x0] <- "adv748x 4-0070 hdmi":1 [ENABLED] pad1: Source [fmt:unknown/0x0] -> "rcar_csi2 feaa0000.csi2":0 [ENABLED,IMMUTABLE] - entity 10: adv748x 4-0070 hdmi (2 pads, 1 link) type V4L2 subdev subtype Unknown flags 0 device node name /dev/v4l-subdev20 pad0: Sink [dv.caps:BT.656/1120 min:640x480@13000000 max:1920x1200@162000000 stds:CEA-861,DMT caps:progressive] pad1: Source [fmt:RGB888_1X24/1280x720 field:none colorspace:srgb] [dv.caps:BT.656/1120 min:640x480@13000000 max:1920x1200@162000000 stds:CEA-861,DMT caps:progressive] [dv.query:no-link] [dv.current:BT.656/1120 1280x720p30 (3300x750) stds:CEA-861 flags:can-reduce-fps,CE-video,has-cea861-vic] -> "adv748x 4-0070 txa":0 [ENABLED] ... So, at link creation time, the adv748x receives a .link_setup() callback (as well as the vin driver does though...) Isn't this enough to dynamically set-up links created between the HDMI/CVBS inputs and one of the CSI-2 transmitters? Thanks j > > > > > What I was about to do, instead, given the fixed HDMI->TXA and AFE->TXB > > > routing in the adv748x driver was to insert a .link_validate() callback for > > > both the HDMI and AFE backends, that checks for the availability of > > > the corresponding output endpoint. This seems to me that this makes > > > easy when dynamic routing will be added to do the same on the > > > dynamically configured sink pad. > > > What do you think? > > > > On a second thought if a CSI-2 sink it's not enabled it is not part of > > the media graph neither, so it should not be possible to link it in any > > pipeline, so no link validation is required. The link simply can't > > exist. > > > > It seems to me that to support Ebisu-like designs is then enough to > > make the adv748x driver probe with a single csi-2 output enabled and > > handle power management accordingly. I will share patches for this > > briefly. > > > > > > > > Thanks > > > j > > > > > > > > > > > > > > > > > > PS: the list of upported patches will be sent separately. > > > > > > > > > > > > Jacopo Mondi (5): > > > > > > media: dt-bindings: media: rcar-vin: Add R8A77990 support > > > > > > media: rcar-vin: Add support for R-Car R8A77990 > > > > > > dt-bindings: media: rcar-csi2: Add R8A77990 > > > > > > media: rcar-csi2: Add R8A77990 support > > > > > > arm64: dts: renesas: ebisu: Add HDMI and CVBS input > > > > > > > > > > > > Koji Matsuoka (1): > > > > > > arm64: dts: r8a77990: Add VIN and CSI-2 device nodes > > > > > > > > > > > > Takeshi Kihara (2): > > > > > > pinctrl: sh-pfc: r8a77990: Add VIN pins, groups and functions > > > > > > arm64: dts: r8a77990: Add I2C device nodes > > > > > > > > > > > > .../devicetree/bindings/media/rcar_vin.txt | 1 + > > > > > > .../bindings/media/renesas,rcar-csi2.txt | 1 + > > > > > > arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts | 86 ++++ > > > > > > arch/arm64/boot/dts/renesas/r8a77990.dtsi | 202 +++++++++ > > > > > > drivers/media/platform/rcar-vin/rcar-core.c | 20 + > > > > > > drivers/media/platform/rcar-vin/rcar-csi2.c | 9 + > > > > > > drivers/pinctrl/sh-pfc/pfc-r8a77990.c | 504 ++++++++++++++++++ > > > > > > 7 files changed, 823 insertions(+) > > > > > > > > > > -- > > > > > Regards, > > > > > > > > > > Laurent Pinchart > > > > > > > > > > > > > > > > > > > > > > > -- > > > > Regards, > > > > Niklas Söderlund > > > > > > > > -- > Regards, > Niklas Söderlund
Attachment:
signature.asc
Description: PGP signature
