Re: [PATCH 1/9] media: v4l: Add v4l2_acpi_parse_sensor_gpios() helper function

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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