Re: [PATCH 2/2] media: V4L2: support asynchronous subdevice registration

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

Hello,

On Monday 22 October 2012 17:22:16 Hans Verkuil wrote:
> On Mon October 22 2012 16:48:05 Guennadi Liakhovetski wrote:
> > On Mon, 22 Oct 2012, Hans Verkuil wrote:
> > > On Mon October 22 2012 14:50:14 Guennadi Liakhovetski wrote:
> > > > On Mon, 22 Oct 2012, Hans Verkuil wrote:
> > > > > On Mon October 22 2012 13:08:12 Guennadi Liakhovetski wrote:
> > > > > > On Mon, 22 Oct 2012, Hans Verkuil wrote:
> > > > > > > On Sat October 20 2012 00:20:24 Guennadi Liakhovetski wrote:
> > > > > > > > Currently bridge device drivers register devices for all
> > > > > > > > subdevices synchronously, tupically, during their probing.
> > > > > > > > E.g. if an I2C CMOS sensor is attached to a video bridge
> > > > > > > > device, the bridge driver will create an I2C device and wait
> > > > > > > > for the respective I2C driver to probe. This makes linking of
> > > > > > > > devices straight forward, but this approach cannot be used
> > > > > > > > with intrinsically asynchronous and unordered device
> > > > > > > > registration systems like the Flattened Device Tree. To
> > > > > > > > support such systems this patch adds an asynchronous subdevice
> > > > > > > > registration framework to V4L2. To use it respective (e.g.
> > > > > > > > I2C) subdevice drivers must request deferred probing as long
> > > > > > > > as their bridge driver hasn't probed. The bridge driver during
> > > > > > > > its probing submits a an arbitrary number of subdevice
> > > > > > > > descriptor groups to the framework to manage. After that it
> > > > > > > > can add callbacks to each of those groups to be called at
> > > > > > > > various stages during subdevice probing, e.g. after
> > > > > > > > completion. Then the bridge driver can request single groups
> > > > > > > > to be probed, finish its own probing and continue its video
> > > > > > > > subsystem configuration from its callbacks.
> > > > > > > 
> > > > > > > What is the purpose of allowing multiple groups?
> > > > > > 
> > > > > > To support, e.g. multiple sensors connected to a single bridge.
> > > > > 
> > > > > So, isn't that one group with two sensor subdevs?
> > > > 
> > > > No, one group consists of all subdevices, necessary to operate a
> > > > single video pipeline. A simple group only contains a sensor. More
> > > > complex groups can contain a CSI-2 interface, a line shifter, or
> > > > anything else.
> > > 
> > > Why? Why would you want to wait for completion of multiple groups? You
> > > need all subdevs to be registered. If you split them up in multiple
> > > groups, then you have to wait until all those groups have completed,
> > > which only makes the bridge driver more complex. It adds nothing to the
> > > problem that we're trying to solve.
> > 
> > I see it differently. Firstly, there's no waiting.
> 
> If they are independent, then that's true. But in almost all cases you need
> them all. Even in cases where theoretically you can 'activate' groups
> independently, it doesn't add anything. It's overengineering, trying to
> solve a problem that doesn't exist.
> 
> Just keep it simple, that's hard enough.

I quite agree here. Sure, in theory groups could be interesting, allowing you 
to start using part of the pipeline before everything is properly initialized, 
or if a sensor can't be probed for some reason. In practice, however, I don't 
think we'll get any substantial gain in real use cases. I propose dropping the 
groups for now, and adding them later if we need to.

> > Secondly, you don't need all of them. With groups as soon as one group is
> > complete you can start using it. If you require all your subdevices to
> > complete their probing before you can use anything. In fact, some
> > subdevices might never probe, but groups, that don't need them can be used
> > regardless.
> > 
> > > > > A bridge driver has a list of subdevs. There is no concept of
> > > > > 'groups'. Perhaps I misunderstand?
> > > > 
> > > > Well, we have a group ID, which can be used for what I'm proposing
> > > > groups for. At least on soc-camera we use the group ID exactly for
> > > > this purpose. We attach all subdevices to a V4L2 device, but assign
> > > > group IDs according to pipelines. Then subdevice operations only act
> > > > on members of one pipeline. I know that we currently don't specify
> > > > precisely what that group ID should be used for in general. So, this
> > > > my group concept is an extension of what we currently have in V4L2.
> > > 
> > > How the grp_id field is used is entirely up to the bridge driver. It may
> > > not be used at all, it may uniquely identify each subdev, it may put
> > > each subdev in a particular group and perhaps a single subdev might
> > > belong to multiple groups. There is no standard concept of a group.
> > > It's just a simple method (actually, more of a hack) of allowing bridge
> > > drivers to call ops for some subset of the sub-devices.
> > 
> > Yes, I know, at least it's something that loosely indicates a group
> > concept in the current code:-)
> > 
> > > Frankly, I wonder if most of the drivers that use grp_id actually need
> > > it at all.
> > > 
> > > Just drop the group concept, things can be simplified quite a bit
> > > without it.
> > 
> > So far I think we should keep it. Also think about our DT layout. A bridge
> > can have several ports each with multiple links (maybe it has already been
> > decided to change names, don't remember by heart, sorry). Each of them
> > would then start a group.
> 
> So? What does that gain you?
> 
> I don't have time today to go over the remainder of your reply, I'll try to
> answer that later in the week.

-- 
Regards,

Laurent Pinchart