Hi Russell, Time for me to jump in. The more, the merrier I suppose.
On Wednesday 26 February 2014 22:19:39 Russell King - ARM Linux wrote: > On Wed, Feb 26, 2014 at 10:00:25PM +0100, Guennadi Liakhovetski wrote: > > Hi Russell > > > > (I suspect this my email will be rejected by ALKML too like other my > > recent emails, but at least other MLs will pick it up and individual CCs > > too, so, if replying, maybe it would be good to keep my entire reply, all > > the more that it's going to be very short) > > > > On Thu, 2 Jan 2014, Russell King wrote: > > > Subsystems such as ALSA, DRM and others require a single card-level > > > device structure to represent a subsystem. However, firmware tends to > > > describe the individual devices and the connections between them. > > > > > > Therefore, we need a way to gather up the individual component devices > > > together, and indicate when we have all the component devices. > > > > > > We do this in DT by providing a "superdevice" node which specifies > > > the components, eg: > > > imx-drm { > > > compatible = "fsl,drm"; > > > crtcs = <&ipu1>; > > > connectors = <&hdmi>; > > > }; > > > > It is a pity linux-media wasn't CC'ed and apparently V4L developers didn't > > notice this and other related patches in a "clean up" series, and now this > > patch is already in the mainline. But at least I'd like to ask whether the > > bindings, defined in > > Documentation/devicetree/bindings/media/video-interfaces.txt and > > implemented in drivers/media/v4l2-core/v4l2-of.c have been considered for > > this job, and - if so - why have they been found unsuitable? Wouldn't it > > have been better to use and - if needed - extend them to cover any > > deficiencies? Even though the implementation is currently located under > > drivers/media/v4l2-code/ it's pretty generic and should be easily > > transferable to a more generic location. > > The component helpers have nothing to do with DT apart from solving > the problem of how to deal with subsystems which expect a single device, > but we have a group of devices and their individual drivers to cope with. > Subsystems like DRM and ALSA. (and V4L2) Point duly taken. First of all I want to mention that your proposal is greatly appreciated. This is a problem that crosses subsystem boundaries, and should thus be addressed centrally. However, we (as in the V4L2 community, and me personally) would have appreciated to be CC'ed on the proposal. As you might know we already had a solution for this problem, albeit V4L2-specific, in drivers/media/v4l2- core/v4l2-async.c. Whether or not this solution should have been made generic instead of coming up with a new separate implementation would of course have been debatable, but the most important point would have been to make sure that v4l2-async could easily be implemented on top of the common component architecture. The topic is particularly hot given that a similar solution was also proposed as part of the now defunct (or at least hibernating) common display framework. If I had replied to this mail thread without sleeping on it first I might not have known better and have got half-paranoid, wondereding whether there had been a deliberate attempt to fast-track this API before the V4L2 developers noticed. To be perfectly clear, there is *NO* implicit or explicit such accusation here, as I know better. Let's all take this as a positive opportunity to cooperate more closely, media devices still need a huge effort to be cleanly supported on modern hardware, and we'll need all the development power we can get. Accordingly, I would like to comment on the component API, despite the fact that it has been merged in mainline already. The first thing that I believe is missing is documentation. Do you have any pending patch for that, either as kerneldoc or as a text file for Documentation/ ? As I've read the code to understand it I might have missed so design goals, so please bear with the stupid questions that may follow. I'll first provide a brief comparison of the two models to make the rest of the comments easier to understand. v4l2-async calls the component master object v4l2_async_notifier. The base component child object is a v4l2_subdev instance instead of being a plain device. v4l2_subdev instances are stored in v4l2-async lists similarly to how the component framework stores objects, except that the list head is directly embedded inside the v4l2_subdev structure instead of being part of a separate structure allocated by the framework. The notifier has three callback functions, bound, complete and unbind. The bound function is called when one component has been bound to the master. Similarly the unbind function is called when one component is about to be unbound from the master. The complete function is called when all components have been bound, and is thus equivalent to the bind function of the component framework. Notifiers are registered along with a list of match entries. A match entry is roughly equivalent to the compare function passed to component_master_add_child, except that it includes built-in support for matching on an OF node, dev_name or I2C bus number and child address. Whenever a subdev (component child) is registered with v4l2_async_register_subdev (equivalent to component_add), the list of notifiers (masters) is walked and their match entries are processed. If a matching entry is found the subdev is bound to the notifier immediately, otherwise it is added to a list of unbound subdevices (component_list). Whenever a notifier (component master) is registered with v4l2_async_notifier_register (component_master_add) the list of unbound subdevs is walked and every match entry of the notifier is tested. If a matching entry is found the subdev is bound to the notifier. I've seen a couple of core differences in concept between your component model and the v4l2-async model: - The component framework uses private master and component structures. Wouldn't it simplify the code from a memory management point of view to expose the master structure (which would then be embedded in driver-specific structures) and the component structure (which would be embedded in struct device) ? The latter would be slightly more intrusive from a struct device point of view, so I don't have a strong opinion there yet, exposing the master structure only might be better. - The component framework requires the master to provide an add_components operation that will call the component_master_add_child function for every component it needs, with a compare function. The add child function is called when the master is registered, and then for every component added to the system. I'm not sure to understand the design decisions behind this, but these two levels of indirection appear pretty complex and confusing. Wouldn't it be simpler to pass an array of match entries to the master registration function instead and remove the add_components operation ? A match entry would basically be a structure with a compare function and a compare_data pointer. We could also extend the match entry with explicit support for OF node and I2C bus number + address matching as those are the most common cases, or at least provide a couple of standard compare functions for those cases. - The component framework doesn't provide partial bind support. Children are bound to the master only when all children are available. This makes it impossible in practice to implement v4l2-async on top of the component framework. What would you think about adding optional partial bind support ? The master operations would then have partial bind, complete bind, partial unbind and complete unbind functions. Drivers that only need full bind support could set the partial bind and unbind functions to NULL. > It is completely agnostic to whether you're using platform data, DT or > even ACPI - this code could *not* care less. None of that comes anywhere > near what this patch does. It merely provides a way to collect up > individual devices from co-operating drivers, and control their binding > such that a subsystem like DRM or ALSA can be presented with a "card" > level view of the hardware rather than a multi-device medusa with all > the buggy, racy, crap fsckage that people come up to make that kind of > thing work. > > Now, as for the binding above, first, what does "eg" mean... and > secondly, how would a binding which refers to crtcs and connectors > have anything to do with ALSA? Clearly this isn't an example of a > binding for an ALSA use, which was talked about in the very first > line of the above commit commentry. So it's quite clear that what is > given there is an example of how it /could/ be used. > > I suppose I could have instead turned imx-drm into a completely unusable > mess by not coming up with some kind of binding, and instead submitted > a whole pile of completely untested code. Alternatively, I could've > used the OF binding as you're suggesting, but that would mean radically > changing the /existing/ bindings for the IPU as a whole - something > which others are better suited at as they have a /much/ better > understanding of the complexities of this hardware than I. > > So, what I have done is implemented - for a driver in staging which is > still subject to ongoing development and non-stable DT bindings - > something which allows forward progress with a *minimum* of disruption > to the existing DT bindings for everyone, while still allowing forward > progress. > > Better bindings for imx-drm are currently being worked on. Philipp > Zabel of Pengutronix is currently looking at it, and has posted many > RFC patches on this very subject, including moving the V4L2 OF helpers > to a more suitable location. OF people have been involved in that > discussion over the preceding weeks, and there's a working implementation > of imx-drm using these helpers from v4l2. > > I'm finding people who are working in the same area and trying to get > everyone talking to each other so that we /do/ end up with a set of > bindings for the display stuff which are suitable for everyone. Tomi > from TI has already expressed his input to this ongoing discussion. > > You're welcome to get involved in those discussions too. > > I hope this makes it clear, and clears up the confusion. > > Thanks. -- Regards, Laurent Pinchart