Hi, On 5/19/23 16:01, Sakari Ailus wrote: > Hi Hans, > > On Fri, May 19, 2023 at 03:08:02PM +0200, Hans de Goede wrote: >> Hi Sakari, >> >> On 5/19/23 14:16, Sakari Ailus wrote: >>> Hi Hans, >>> >>> On Fri, May 19, 2023 at 01:55:04PM +0200, Hans de Goede wrote: >>>> Hi Sakari, >>>> >>>> On 5/19/23 12:58, Sakari Ailus wrote: >>>>> Hi Hans, >>>>> >>>>> On Fri, May 19, 2023 at 10:55:42AM +0200, Hans de Goede wrote: >>>> >>>> <snip> >>>> >>>>>>> Although if the number of those drivers would be small, this could be just >>>>>>> undesirable but still somehow acceptable. And I wouldn't expect new sensors >>>>>>> to be paired with the IPU2 anymore. How many drivers there would be >>>>>>> roughly? I think I've seen ten-ish sensor drivers with the atomisp driver. >>>>>> >>>>>> About ten-ish drivers sounds right. >>>>>> >>>>>>> Isn't it possible to create a device for this purpose and use software >>>>>>> nodes for the GPIOs? I guess that would be a hack as well and you'd somehow >>>>>>> have to initialise this via other route than driver probe. >>>>>> >>>>>> This creates unsolvable probe-ordering problems. At a minimum we would >>>>>> need some check inside sensor drivers for them to return -EPROBE_DEFER >>>>>> until the GPIO mappings are added through some other means. Note that >>>>>> without the mappings gpiod_get will return -ENOENT, so we cannot just >>>>>> use its return value. >>>>> >>>>> They probably will already need this in order to make sure the atomisp >>>>> bridge has been initialized. >>>> >>>> The instantiation of the sensor i2c_clients and of the atomisp PCI device >>>> is independent of each other. In my other patch series I'm moving sensor >>>> registration for atomisp over to the v4l2-async framework like all other >>>> bridge/ISP drivers use so that atomisp no longer needs special sensor >>>> drivers. >>>> >>>> And AFAIK one of the reasons for having the v4l2-async framework is >>>> to avoid needing to have a specific probe order between bridge vs >>>> sensor drivers. >>>> >>>>> Could this code live in the atomisp bridge instead? >>>> >>>> That does not solve the probe ordering problem the sensor drivers >>>> need the GPIOs to enable the sensor and they all enable the sensor >>>> in their probe() to check the hw-id. The sensor's probe() function >>>> runs without any ordering guarantees vs the ISP/brige's probe() >>>> function. This is not an issue because at least during probe() >>>> the sensor drivers don't have any interactions with the bridge >>>> and any further access to the sensor-drivers is delayed until >>>> the v4l2-async notifier completion callback has run. >>>> >>>> The only way to work around the probe-ordering problem would >>>> be to delay checking the hw-id until the sensor gets linked >>>> to the bridge. Which would mean registering an async notifier >>>> from the sensors to catch binding from the sensor drivers >>>> and allowing the binding to fail. >>>> >>>> Delaying the hw-id check like this would be much more involved >>>> then the currently proposed solution and will likely cause >>>> other issues like the wrong driver binding when hw-vendors >>>> get the ACPI hw-id wrong (this is a known issue with audio-codecs, >>>> so chances are we will see this with sensors too). >>> >>> A simple question: how do you solve the probe ordering issue when it comes >>> to the atomisp bridge creating the graph endpoints needed by sensor >>> drivers? >>> >>> Or do you assume the sensor drivers will always use some static >>> configuration? >> >> This is all modeled after the IPU3 work done by Dan Scally and >> ATM the involved sensor drivers assume a static configuration >> wrt number of lanes + link frequency. > > In general the number of lanes used is dependent on a particular board, and > in many cases the same sensor is used on different systems with a different > lane configuration. This might not happen on sensors used with atomisp but > it probably already happens on those used by the CIO2 (ov8865). > > What the drivers should do and in most cases do is that they check the lane > configuration is valid for the device. > >> >> If / when we want to start supporting say both single + dual >> lane modes (with e.g. reduced framerate for high res in single lane) >> then AFAICT this will only influence things like e.g. subdev calls >> to get supported framerates and of course turning on streaming, >> all of which only ever happen after the async subdev registration >> of all involved subdevs is complete. > > On sensors the number of lanes is practically never selected dynamically, > it's simply a property of the board (the CSI-2 receiver and how many wires > happen to be connected, whichever has fewer lanes). Right, I understand / agree what I meant is that the driver does not really need to know the number of connected lanes during probe(). But yes if you want to check the number of lanes is valid in probe() then you do need to know it. >> So (again AFAICT) unlike the GPIOs there is no need to need >> to know the endpoint configuration at probe() time. But since >> we do want to turn the sensor on to check it is actually there >> and if it is the type of sensor we expect during probe() we >> do need the GPIOs beforehand. > > The other, somewhat cleaner in my opinion and clearly more future-proof, > would be to return -EPROBE_DEFER if there are no endpoints. Which is also > what the ov8865 driver does already. Ok that is a good suggestion, which I think should work. I'll give this a try sometime during the coming days. Regards, Hans
