Hi Hans, On Fri, May 19, 2023 at 04:39:29PM +0200, Hans de Goede wrote: > 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. Thanks, looking forward to see v2! -- Kind regards, Sakari Ailus
