Re: [RFT 0/8] arm64: dts: renesas: Ebisu: Add HDMI and CVBS input

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Niklas,

On Monday, 27 August 2018 16:23:45 EEST Niklas Söderlund wrote:
> On 2018-08-27 11:49:56 +0200, Jacopo Mondi wrote:
> > On Sat, Aug 25, 2018 at 03:18:06PM +0200, jacopo mondi wrote:
> >> On Sat, Aug 25, 2018 at 08:37:15AM +0200, Niklas Söderlund wrote:
> >>> On 2018-08-25 02:54:44 +0300, Laurent Pinchart wrote:
> >>>> 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.

Isn't this already possible ? We have a single top-level .link_notify() 
operation at the media device level, but also .link_setup() operations for 
each entity.

> 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.
> 
> >> 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.

-- 
Regards,

Laurent Pinchart