| Message ID | E1Vypo6-0007FF-Lb@rmk-PC.arm.linux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jan 2, 2014 at 4:27 PM, Russell King <rmk+kernel@arm.linux.org.uk> 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>; > }; > Hi Russell, This looks really good. I'd definitely like to use it for the exynos drm driver both to bind the various IP blocks together, and also to pull in any attached bridges that are specified in the device tree. Along those lines, it might be worthwhile to pull some of the master bind functionality in your next patch into drm helpers so other drivers can use them, and so we have concrete bindings across drm. Make sense? Sean > The superdevice is declared into the component support, along with the > subcomponents. The superdevice receives callbacks to locate the > subcomponents, and identify when all components are present. At this > point, we bind the superdevice, which causes the appropriate subsystem > to be initialised in the conventional way. > > When any of the components or superdevice are removed from the system, > we unbind the superdevice, thereby taking the subsystem down. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > drivers/base/Makefile | 2 +- > drivers/base/component.c | 379 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/component.h | 31 ++++ > 3 files changed, 411 insertions(+), 1 deletions(-) > create mode 100644 drivers/base/component.c > create mode 100644 include/linux/component.h > > diff --git a/drivers/base/Makefile b/drivers/base/Makefile > index 94e8a80e87f8..870ecfd503af 100644 > --- a/drivers/base/Makefile > +++ b/drivers/base/Makefile > @@ -1,6 +1,6 @@ > # Makefile for the Linux device tree > > -obj-y := core.o bus.o dd.o syscore.o \ > +obj-y := component.o core.o bus.o dd.o syscore.o \ > driver.o class.o platform.o \ > cpu.o firmware.o init.o map.o devres.o \ > attribute_container.o transport_class.o \ > diff --git a/drivers/base/component.c b/drivers/base/component.c > new file mode 100644 > index 000000000000..5492cd8d2247 > --- /dev/null > +++ b/drivers/base/component.c > @@ -0,0 +1,379 @@ > +/* > + * Componentized device handling. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This is work in progress. We gather up the component devices into a list, > + * and bind them when instructed. At the moment, we're specific to the DRM > + * subsystem, and only handles one master device, but this doesn't have to be > + * the case. > + */ > +#include <linux/component.h> > +#include <linux/device.h> > +#include <linux/kref.h> > +#include <linux/list.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/slab.h> > + > +struct master { > + struct list_head node; > + struct list_head components; > + bool bound; > + > + const struct component_master_ops *ops; > + struct device *dev; > +}; > + > +struct component { > + struct list_head node; > + struct list_head master_node; > + struct master *master; > + bool bound; > + > + const struct component_ops *ops; > + struct device *dev; > +}; > + > +static DEFINE_MUTEX(component_mutex); > +static LIST_HEAD(component_list); > +static LIST_HEAD(masters); > + > +static struct master *__master_find(struct device *dev, const struct component_master_ops *ops) > +{ > + struct master *m; > + > + list_for_each_entry(m, &masters, node) > + if (m->dev == dev && (!ops || m->ops == ops)) > + return m; > + > + return NULL; > +} > + > +/* Attach an unattached component to a master. */ > +static void component_attach_master(struct master *master, struct component *c) > +{ > + c->master = master; > + > + list_add_tail(&c->master_node, &master->components); > +} > + > +/* Detach a component from a master. */ > +static void component_detach_master(struct master *master, struct component *c) > +{ > + list_del(&c->master_node); > + > + c->master = NULL; > +} > + > +int component_master_add_child(struct master *master, > + int (*compare)(struct device *, void *), void *compare_data) > +{ > + struct component *c; > + int ret = -ENXIO; > + > + list_for_each_entry(c, &component_list, node) { > + if (c->master) > + continue; > + > + if (compare(c->dev, compare_data)) { > + component_attach_master(master, c); > + ret = 0; > + break; > + } > + } > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(component_master_add_child); > + > +/* Detach all attached components from this master */ > +static void master_remove_components(struct master *master) > +{ > + while (!list_empty(&master->components)) { > + struct component *c = list_first_entry(&master->components, > + struct component, master_node); > + > + WARN_ON(c->master != master); > + > + component_detach_master(master, c); > + } > +} > + > +/* > + * Try to bring up a master. If component is NULL, we're interested in > + * this master, otherwise it's a component which must be present to try > + * and bring up the master. > + * > + * Returns 1 for successful bringup, 0 if not ready, or -ve errno. > + */ > +static int try_to_bring_up_master(struct master *master, > + struct component *component) > +{ > + int ret = 0; > + > + if (!master->bound) { > + /* > + * Search the list of components, looking for components that > + * belong to this master, and attach them to the master. > + */ > + if (master->ops->add_components(master->dev, master)) { > + /* Failed to find all components */ > + master_remove_components(master); > + ret = 0; > + goto out; > + } > + > + if (component && component->master != master) { > + master_remove_components(master); > + ret = 0; > + goto out; > + } > + > + /* Found all components */ > + ret = master->ops->bind(master->dev); > + if (ret < 0) { > + master_remove_components(master); > + goto out; > + } > + > + master->bound = true; > + ret = 1; > + } > +out: > + > + return ret; > +} > + > +static int try_to_bring_up_masters(struct component *component) > +{ > + struct master *m; > + int ret = 0; > + > + list_for_each_entry(m, &masters, node) { > + ret = try_to_bring_up_master(m, component); > + if (ret != 0) > + break; > + } > + > + return ret; > +} > + > +static void take_down_master(struct master *master) > +{ > + if (master->bound) { > + master->ops->unbind(master->dev); > + master->bound = false; > + } > + > + master_remove_components(master); > +} > + > +int component_master_add(struct device *dev, const struct component_master_ops *ops) > +{ > + struct master *master; > + int ret; > + > + master = kzalloc(sizeof(*master), GFP_KERNEL); > + if (!master) > + return -ENOMEM; > + > + master->dev = dev; > + master->ops = ops; > + INIT_LIST_HEAD(&master->components); > + > + /* Add to the list of available masters. */ > + mutex_lock(&component_mutex); > + list_add(&master->node, &masters); > + > + ret = try_to_bring_up_master(master, NULL); > + > + if (ret < 0) { > + /* Delete off the list if we weren't successful */ > + list_del(&master->node); > + kfree(master); > + } > + mutex_unlock(&component_mutex); > + > + return ret < 0 ? ret : 0; > +} > +EXPORT_SYMBOL_GPL(component_master_add); > + > +void component_master_del(struct device *dev, const struct component_master_ops *ops) > +{ > + struct master *master; > + > + mutex_lock(&component_mutex); > + master = __master_find(dev, ops); > + if (master) { > + take_down_master(master); > + > + list_del(&master->node); > + kfree(master); > + } > + mutex_unlock(&component_mutex); > +} > +EXPORT_SYMBOL_GPL(component_master_del); > + > +static void component_unbind(struct component *component, > + struct master *master, void *data) > +{ > + WARN_ON(!component->bound); > + > + component->ops->unbind(component->dev, master->dev, data); > + component->bound = false; > + > + /* Release all resources claimed in the binding of this component */ > + devres_release_group(component->dev, component); > +} > + > +void component_unbind_all(struct device *master_dev, void *data) > +{ > + struct master *master; > + struct component *c; > + > + WARN_ON(!mutex_is_locked(&component_mutex)); > + > + master = __master_find(master_dev, NULL); > + if (!master) > + return; > + > + list_for_each_entry_reverse(c, &master->components, master_node) > + component_unbind(c, master, data); > +} > +EXPORT_SYMBOL_GPL(component_unbind_all); > + > +static int component_bind(struct component *component, struct master *master, > + void *data) > +{ > + int ret; > + > + /* > + * Each component initialises inside its own devres group. > + * This allows us to roll-back a failed component without > + * affecting anything else. > + */ > + if (!devres_open_group(master->dev, NULL, GFP_KERNEL)) > + return -ENOMEM; > + > + /* > + * Also open a group for the device itself: this allows us > + * to release the resources claimed against the sub-device > + * at the appropriate moment. > + */ > + if (!devres_open_group(component->dev, component, GFP_KERNEL)) { > + devres_release_group(master->dev, NULL); > + return -ENOMEM; > + } > + > + dev_dbg(master->dev, "binding %s (ops %ps)\n", > + dev_name(component->dev), component->ops); > + > + ret = component->ops->bind(component->dev, master->dev, data); > + if (!ret) { > + component->bound = true; > + > + /* > + * Close the component device's group so that resources > + * allocated in the binding are encapsulated for removal > + * at unbind. Remove the group on the DRM device as we > + * can clean those resources up independently. > + */ > + devres_close_group(component->dev, NULL); > + devres_remove_group(master->dev, NULL); > + > + dev_info(master->dev, "bound %s (ops %ps)\n", > + dev_name(component->dev), component->ops); > + } else { > + devres_release_group(component->dev, NULL); > + devres_release_group(master->dev, NULL); > + > + dev_err(master->dev, "failed to bind %s (ops %ps): %d\n", > + dev_name(component->dev), component->ops, ret); > + } > + > + return ret; > +} > + > +int component_bind_all(struct device *master_dev, void *data) > +{ > + struct master *master; > + struct component *c; > + int ret = 0; > + > + WARN_ON(!mutex_is_locked(&component_mutex)); > + > + master = __master_find(master_dev, NULL); > + if (!master) > + return -EINVAL; > + > + list_for_each_entry(c, &master->components, master_node) { > + ret = component_bind(c, master, data); > + if (ret) > + break; > + } > + > + if (ret != 0) { > + list_for_each_entry_continue_reverse(c, &master->components, > + master_node) > + component_unbind(c, master, data); > + } > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(component_bind_all); > + > +int component_add(struct device *dev, const struct component_ops *ops) > +{ > + struct component *component; > + int ret; > + > + component = kzalloc(sizeof(*component), GFP_KERNEL); > + if (!component) > + return -ENOMEM; > + > + component->ops = ops; > + component->dev = dev; > + > + dev_dbg(dev, "adding component (ops %ps)\n", ops); > + > + mutex_lock(&component_mutex); > + list_add_tail(&component->node, &component_list); > + > + ret = try_to_bring_up_masters(component); > + if (ret < 0) { > + list_del(&component->node); > + > + kfree(component); > + } > + mutex_unlock(&component_mutex); > + > + return ret < 0 ? ret : 0; > +} > +EXPORT_SYMBOL_GPL(component_add); > + > +void component_del(struct device *dev, const struct component_ops *ops) > +{ > + struct component *c, *component = NULL; > + > + mutex_lock(&component_mutex); > + list_for_each_entry(c, &component_list, node) > + if (c->dev == dev && c->ops == ops) { > + list_del(&c->node); > + component = c; > + break; > + } > + > + if (component && component->master) > + take_down_master(component->master); > + > + mutex_unlock(&component_mutex); > + > + WARN_ON(!component); > + kfree(component); > +} > +EXPORT_SYMBOL_GPL(component_del); > + > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/component.h b/include/linux/component.h > new file mode 100644 > index 000000000000..73657636db0b > --- /dev/null > +++ b/include/linux/component.h > @@ -0,0 +1,31 @@ > +#ifndef COMPONENT_H > +#define COMPONENT_H > + > +struct device; > + > +struct component_ops { > + int (*bind)(struct device *, struct device *, void *); > + void (*unbind)(struct device *, struct device *, void *); > +}; > + > +int component_add(struct device *, const struct component_ops *); > +void component_del(struct device *, const struct component_ops *); > + > +int component_bind_all(struct device *, void *); > +void component_unbind_all(struct device *, void *); > + > +struct master; > + > +struct component_master_ops { > + int (*add_components)(struct device *, struct master *); > + int (*bind)(struct device *); > + void (*unbind)(struct device *); > +}; > + > +int component_master_add(struct device *, const struct component_master_ops *); > +void component_master_del(struct device *, const struct component_master_ops *); > + > +int component_master_add_child(struct master *master, > + int (*compare)(struct device *, void *), void *compare_data); > + > +#endif > -- > 1.7.4.4 > > _______________________________________________ > devel mailing list > devel@linuxdriverproject.org > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
On Tue, Jan 07, 2014 at 03:18:21PM -0500, Sean Paul wrote: > On Thu, Jan 2, 2014 at 4:27 PM, Russell King > <rmk+kernel@arm.linux.org.uk> 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>; > > }; > > > > Hi Russell, > This looks really good. I'd definitely like to use it for the exynos > drm driver both to bind the various IP blocks together, and also to > pull in any attached bridges that are specified in the device tree. > Along those lines, it might be worthwhile to pull some of the master > bind functionality in your next patch into drm helpers so other > drivers can use them, and so we have concrete bindings across drm. Which bits do you think would be useful to move into the core? imx_drm_add_components() is rather imx-drm specific, especially for the CRTCs - it carries the knowledge that the OF device to be matched can be found in the _parent_ device, rather than the device registered into the component helpers.
On Wed, Jan 8, 2014 at 4:36 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Jan 07, 2014 at 03:18:21PM -0500, Sean Paul wrote: >> On Thu, Jan 2, 2014 at 4:27 PM, Russell King >> <rmk+kernel@arm.linux.org.uk> 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>; >> > }; >> > >> >> Hi Russell, >> This looks really good. I'd definitely like to use it for the exynos >> drm driver both to bind the various IP blocks together, and also to >> pull in any attached bridges that are specified in the device tree. >> Along those lines, it might be worthwhile to pull some of the master >> bind functionality in your next patch into drm helpers so other >> drivers can use them, and so we have concrete bindings across drm. > > Which bits do you think would be useful to move into the core? > imx_drm_add_components() is rather imx-drm specific, especially for > the CRTCs - it carries the knowledge that the OF device to be matched > can be found in the _parent_ device, rather than the device registered > into the component helpers. > Yeah, I was thinking of imx_drm_add_components() actually. It probably doesn't make sense to enforce the parent relationship in a generic manner, but it would be nice to have a helper which added the various drm components (crtc/encoder/bridge/connector) with a consistent binding. We have 3 different exynos boards which would have the following superdevices (please forgive my hypothetical syntax/naming): Board 1: exynos-drm { compatible = "exynos,drm"; crtcs = <&fimd1>, <&mixer1>; encoders = <&dp1>, <&hdmi1>; bridges = <&ptn3460 &dp1>; connectors = <&ptn3460>, <&hdmi1>; }; Board 2: exynos-drm { compatible = "exynos,drm"; crtcs = <&fimd1>, <&mixer1>; encoders = <&dp1>, <&hdmi1>; bridges = <&ps8622 &dp1>, <&anx7808 &hdmi1>; connectors = <&ps8622>, <&anx7808>; }; Board 3: exynos-drm { compatible = "exynos,drm"; crtcs = <&fimd1>, <&mixer1>; encoders = <&dp1>, <&hdmi1>; connectors = <&dp1>, <&hdmi1>; }; In addition to enforcing common bindings across drivers, we could also assign bridges to encoders from the dt. The bridge->encoder relationship is 1:1 at the moment, and the bridge driver can be a completely separate entity from the encoder. Allowing assignment from the dt means the encoder never needs to know whether a bridge is connected. Right now the encoder must enumerate all possible bridges to see if they are present (check out "[PATCH v3 31/32] drm/exynos: Move lvds bridge discovery into DP driver" for an example of what I mean). Sean > -- > FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation > in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. > Estimate before purchase was "up to 13.2Mbit".
On Wed, Jan 08, 2014 at 05:39:31PM -0500, Sean Paul wrote: > On Wed, Jan 8, 2014 at 4:36 PM, Russell King - ARM Linux > > Which bits do you think would be useful to move into the core? > > imx_drm_add_components() is rather imx-drm specific, especially for > > the CRTCs - it carries the knowledge that the OF device to be matched > > can be found in the _parent_ device, rather than the device registered > > into the component helpers. > > > > Yeah, I was thinking of imx_drm_add_components() actually. It probably > doesn't make sense to enforce the parent relationship in a generic > manner, but it would be nice to have a helper which added the various > drm components (crtc/encoder/bridge/connector) with a consistent > binding. > > We have 3 different exynos boards which would have the following > superdevices (please forgive my hypothetical syntax/naming): > > Board 1: > exynos-drm { > compatible = "exynos,drm"; > crtcs = <&fimd1>, <&mixer1>; > encoders = <&dp1>, <&hdmi1>; > bridges = <&ptn3460 &dp1>; > connectors = <&ptn3460>, <&hdmi1>; > }; Can we have an example with a different number of encoders/connectors/crtcs, like: exynos-drm { compatible = "exynos,drm"; crtcs = <&fimd1>; encoders = <&dp1>, <&hdmi1>, <&lvds1>; connectors = <&ptn3460>, <&hdmi1>; }; Otherwise I get the impression that there is some topology of the components or at least relationship between the components encoded into the binding. Sascha
On Thu, Jan 02, 2014 at 07:10:55PM -0800, Greg Kroah-Hartman wrote: > On Thu, Jan 02, 2014 at 09:27:58PM +0000, 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>; > > }; > > > > The superdevice is declared into the component support, along with the > > subcomponents. The superdevice receives callbacks to locate the > > subcomponents, and identify when all components are present. At this > > point, we bind the superdevice, which causes the appropriate subsystem > > to be initialised in the conventional way. > > > > When any of the components or superdevice are removed from the system, > > we unbind the superdevice, thereby taking the subsystem down. > > This sounds a lot like the "containers" code that Rafael just submitted > and I acked for 3.14. Look at the lkml post: > Subject: [PATCH 2/2] ACPI / hotplug / driver core: Handle containers in a special way > Message-ID: <1991202.gilW172FBV@vostro.rjw.lan> > > And see if that could possibly be used instead? Greg, Not sure if you saw the outcome to your comment above. My conclusion was: "Yes, I'm coming to that conclusion as well. It looks like your "containers" aren't about collecting up several individual component devices into one super-device and probing the appropriate subsystem when all components are known. "Confused why Greg is pointing me at your patches." Does this mean you're happy with the patch? Thanks.
On Fri, Jan 10, 2014 at 02:54:44PM +0000, Russell King - ARM Linux wrote: > On Thu, Jan 02, 2014 at 07:10:55PM -0800, Greg Kroah-Hartman wrote: > > On Thu, Jan 02, 2014 at 09:27:58PM +0000, 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>; > > > }; > > > > > > The superdevice is declared into the component support, along with the > > > subcomponents. The superdevice receives callbacks to locate the > > > subcomponents, and identify when all components are present. At this > > > point, we bind the superdevice, which causes the appropriate subsystem > > > to be initialised in the conventional way. > > > > > > When any of the components or superdevice are removed from the system, > > > we unbind the superdevice, thereby taking the subsystem down. > > > > This sounds a lot like the "containers" code that Rafael just submitted > > and I acked for 3.14. Look at the lkml post: > > Subject: [PATCH 2/2] ACPI / hotplug / driver core: Handle containers in a special way > > Message-ID: <1991202.gilW172FBV@vostro.rjw.lan> > > > > And see if that could possibly be used instead? > > Greg, > > Not sure if you saw the outcome to your comment above. My conclusion > was: > > "Yes, I'm coming to that conclusion as well. It looks like your "containers" > aren't about collecting up several individual component devices into one > super-device and probing the appropriate subsystem when all components are > known. > > "Confused why Greg is pointing me at your patches." Ah, sorry, I missed that in my "catch up on 2 weeks of email" flood. > Does this mean you're happy with the patch? Well, I will not object to it on the grounds of it being a duplicate of Rafael's work now :) I'll be glad to consider it on its own, when you feel the series is ready to be submitted. thanks, greg k-h
On Fri, Jan 10, 2014 at 07:07:02AM -0800, Greg Kroah-Hartman wrote: > On Fri, Jan 10, 2014 at 02:54:44PM +0000, Russell King - ARM Linux wrote: > > Greg, > > > > Not sure if you saw the outcome to your comment above. My conclusion > > was: > > > > "Yes, I'm coming to that conclusion as well. It looks like your "containers" > > aren't about collecting up several individual component devices into one > > super-device and probing the appropriate subsystem when all components are > > known. > > > > "Confused why Greg is pointing me at your patches." > > Ah, sorry, I missed that in my "catch up on 2 weeks of email" flood. > > > Does this mean you're happy with the patch? > > Well, I will not object to it on the grounds of it being a duplicate of > Rafael's work now :) > > I'll be glad to consider it on its own, when you feel the series is > ready to be submitted. I'll sort out a new set of patches today, along with a branch to pull if you wish to take them that way. Thanks.
On Fri, Jan 10, 2014 at 03:11:23PM +0000, Russell King - ARM Linux wrote: > On Fri, Jan 10, 2014 at 07:07:02AM -0800, Greg Kroah-Hartman wrote: > > On Fri, Jan 10, 2014 at 02:54:44PM +0000, Russell King - ARM Linux wrote: > > > Greg, > > > > > > Not sure if you saw the outcome to your comment above. My conclusion > > > was: > > > > > > "Yes, I'm coming to that conclusion as well. It looks like your "containers" > > > aren't about collecting up several individual component devices into one > > > super-device and probing the appropriate subsystem when all components are > > > known. > > > > > > "Confused why Greg is pointing me at your patches." > > > > Ah, sorry, I missed that in my "catch up on 2 weeks of email" flood. > > > > > Does this mean you're happy with the patch? > > > > Well, I will not object to it on the grounds of it being a duplicate of > > Rafael's work now :) > > > > I'll be glad to consider it on its own, when you feel the series is > > ready to be submitted. > > I'll sort out a new set of patches today, along with a branch to pull if > you wish to take them that way. It's too late for 3.14, as my tree is now closed for that because 3.13 should be out this weekend. But I'll be glad to queue them up after 3.14-rc1 is out. thanks, greg k-h
On Fri, Jan 10, 2014 at 07:35:30AM -0800, Greg Kroah-Hartman wrote: > On Fri, Jan 10, 2014 at 03:11:23PM +0000, Russell King - ARM Linux wrote: > > On Fri, Jan 10, 2014 at 07:07:02AM -0800, Greg Kroah-Hartman wrote: > > > On Fri, Jan 10, 2014 at 02:54:44PM +0000, Russell King - ARM Linux wrote: > > > > Greg, > > > > > > > > Not sure if you saw the outcome to your comment above. My conclusion > > > > was: > > > > > > > > "Yes, I'm coming to that conclusion as well. It looks like your "containers" > > > > aren't about collecting up several individual component devices into one > > > > super-device and probing the appropriate subsystem when all components are > > > > known. > > > > > > > > "Confused why Greg is pointing me at your patches." > > > > > > Ah, sorry, I missed that in my "catch up on 2 weeks of email" flood. > > > > > > > Does this mean you're happy with the patch? > > > > > > Well, I will not object to it on the grounds of it being a duplicate of > > > Rafael's work now :) > > > > > > I'll be glad to consider it on its own, when you feel the series is > > > ready to be submitted. > > > > I'll sort out a new set of patches today, along with a branch to pull if > > you wish to take them that way. > > It's too late for 3.14, as my tree is now closed for that because 3.13 > should be out this weekend. But I'll be glad to queue them up after > 3.14-rc1 is out. That's rather annoying... I'll not put effort into this for a few months then.
On Fri, Jan 10, 2014 at 07:35:30AM -0800, Greg Kroah-Hartman wrote: > > I'll sort out a new set of patches today, along with a branch to pull if > > you wish to take them that way. > > It's too late for 3.14, as my tree is now closed for that because 3.13 > should be out this weekend. But I'll be glad to queue them up after > 3.14-rc1 is out. Didnt' Linus say there would be an -rc8? However - there is a devicetree issue to be discussed anyway (see Sascha's comment). Although we'd personally like to see this series being merged sooner than later, it should be sorted out first. Devicetree issues are ugly to change once they are in mainline. rsc
On Fri, Jan 10, 2014 at 07:30:21PM +0100, Robert Schwebel wrote: > On Fri, Jan 10, 2014 at 07:35:30AM -0800, Greg Kroah-Hartman wrote: > > > I'll sort out a new set of patches today, along with a branch to pull if > > > you wish to take them that way. > > > > It's too late for 3.14, as my tree is now closed for that because 3.13 > > should be out this weekend. But I'll be glad to queue them up after > > 3.14-rc1 is out. > > Didnt' Linus say there would be an -rc8? Ah, you are right, nevermind then :) Russell, if you want to send me just the driver core patch, I can take that now and get that into 3.14 so that you don't have any cross-tree dependancies on the rest of this patch series. thanks, greg k-h
On Fri, Jan 10, 2014 at 11:23:37PM +0000, Russell King - ARM Linux wrote: > We do this in DT by providing a "superdevice" node which specifies > the components, eg: > > imx-drm { > compatible = "fsl,drm"; > crtcs = <&ipu1>; > connectors = <&hdmi>; > }; Saschas comment from 20140109074030.GN6750@pengutronix.de isn't addressed yet: Sascha Hauer wrote: > Can we have an example with a different number of > encoders/connectors/crtcs, like: > > exynos-drm { > compatible = "exynos,drm"; > crtcs = <&fimd1>; > encoders = <&dp1>, <&hdmi1>, <&lvds1>; > connectors = <&ptn3460>, <&hdmi1>; > }; > > Otherwise I get the impression that there is some topology of the > components or at least relationship between the components encoded > into the binding. If I remember correctly, Sascha+Philipp+Lucas still had issues with the bindings, but I'm not sure if they have been already addressed. It's no good timing to finalize this during the weekend, where most people are not in the office :) rsc
On Sat, Jan 11, 2014 at 12:31:19PM +0100, Robert Schwebel wrote: > On Fri, Jan 10, 2014 at 11:23:37PM +0000, Russell King - ARM Linux wrote: > > We do this in DT by providing a "superdevice" node which specifies > > the components, eg: > > > > imx-drm { > > compatible = "fsl,drm"; > > crtcs = <&ipu1>; > > connectors = <&hdmi>; > > }; > > Saschas comment from 20140109074030.GN6750@pengutronix.de isn't > addressed yet: This is not a comment against _this_ patch. It was a question for Sean Paul. You can tell this by the contents of the To: and Cc: headers on Sascha's email. If that's not what was intended, then the email headers are misleading. > Sascha Hauer wrote: > > Can we have an example with a different number of > > encoders/connectors/crtcs, like: > > > > exynos-drm { > > compatible = "exynos,drm"; > > crtcs = <&fimd1>; > > encoders = <&dp1>, <&hdmi1>, <&lvds1>; > > connectors = <&ptn3460>, <&hdmi1>; > > }; > > > > Otherwise I get the impression that there is some topology of the > > components or at least relationship between the components encoded > > into the binding. > > If I remember correctly, Sascha+Philipp+Lucas still had issues with the > bindings, but I'm not sure if they have been already addressed. The bindings are not part of this patch. This patch doesn't even care about DT one bit - that's part of the design goal of it. It doesn't care about platform devices either. All it cares about is maintaining lists of struct device pointers, asking the master device(s) whether they have all their components, and binding/unbinding the components at the appropriate moment. It's completely up to the master device operations to decide how to make these decisions, and how those decisions are made, whether it be by looking up in DT, or ACPI, or whatever.
Am Samstag, den 11.01.2014, 11:40 +0000 schrieb Russell King - ARM Linux: > On Sat, Jan 11, 2014 at 12:31:19PM +0100, Robert Schwebel wrote: > > On Fri, Jan 10, 2014 at 11:23:37PM +0000, Russell King - ARM Linux wrote: > > > We do this in DT by providing a "superdevice" node which specifies > > > the components, eg: > > > > > > imx-drm { > > > compatible = "fsl,drm"; > > > crtcs = <&ipu1>; > > > connectors = <&hdmi>; > > > }; > > > > Saschas comment from 20140109074030.GN6750@pengutronix.de isn't > > addressed yet: > > This is not a comment against _this_ patch. It was a question for Sean > Paul. You can tell this by the contents of the To: and Cc: headers on > Sascha's email. If that's not what was intended, then the email headers > are misleading. > > > Sascha Hauer wrote: > > > Can we have an example with a different number of > > > encoders/connectors/crtcs, like: > > > > > > exynos-drm { > > > compatible = "exynos,drm"; > > > crtcs = <&fimd1>; > > > encoders = <&dp1>, <&hdmi1>, <&lvds1>; > > > connectors = <&ptn3460>, <&hdmi1>; > > > }; > > > > > > Otherwise I get the impression that there is some topology of the > > > components or at least relationship between the components encoded > > > into the binding. > > > > If I remember correctly, Sascha+Philipp+Lucas still had issues with the > > bindings, but I'm not sure if they have been already addressed. > > The bindings are not part of this patch. This patch doesn't even care > about DT one bit - that's part of the design goal of it. It doesn't > care about platform devices either. Yes, I think the device tree bindings are in need of discussion, but this is a separate issue. I'd be happy to hear your opinion on the "imx-drm dt bindings" patches. > All it cares about is maintaining lists of struct device pointers, > asking the master device(s) whether they have all their components, and > binding/unbinding the components at the appropriate moment. It's > completely up to the master device operations to decide how to make > these decisions, and how those decisions are made, whether it be by > looking up in DT, or ACPI, or whatever. I'm very much in favor of this particular patch being merged as soon as sensible. regards Philipp
I've chatted a bit with Hans Verkuil about this topic at fosdem and apparently both v4l and alsa have something like this already in their helper libraries. Adding more people as fyi in case they want to switch to the new driver core stuff from Russell. -Daniel On Thu, Jan 2, 2014 at 10:27 PM, Russell King <rmk+kernel@arm.linux.org.uk> 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>; > }; > > The superdevice is declared into the component support, along with the > subcomponents. The superdevice receives callbacks to locate the > subcomponents, and identify when all components are present. At this > point, we bind the superdevice, which causes the appropriate subsystem > to be initialised in the conventional way. > > When any of the components or superdevice are removed from the system, > we unbind the superdevice, thereby taking the subsystem down. > > Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> > --- > drivers/base/Makefile | 2 +- > drivers/base/component.c | 379 +++++++++++++++++++++++++++++++++++++++++++++ > include/linux/component.h | 31 ++++ > 3 files changed, 411 insertions(+), 1 deletions(-) > create mode 100644 drivers/base/component.c > create mode 100644 include/linux/component.h > > diff --git a/drivers/base/Makefile b/drivers/base/Makefile > index 94e8a80e87f8..870ecfd503af 100644 > --- a/drivers/base/Makefile > +++ b/drivers/base/Makefile > @@ -1,6 +1,6 @@ > # Makefile for the Linux device tree > > -obj-y := core.o bus.o dd.o syscore.o \ > +obj-y := component.o core.o bus.o dd.o syscore.o \ > driver.o class.o platform.o \ > cpu.o firmware.o init.o map.o devres.o \ > attribute_container.o transport_class.o \ > diff --git a/drivers/base/component.c b/drivers/base/component.c > new file mode 100644 > index 000000000000..5492cd8d2247 > --- /dev/null > +++ b/drivers/base/component.c > @@ -0,0 +1,379 @@ > +/* > + * Componentized device handling. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This is work in progress. We gather up the component devices into a list, > + * and bind them when instructed. At the moment, we're specific to the DRM > + * subsystem, and only handles one master device, but this doesn't have to be > + * the case. > + */ > +#include <linux/component.h> > +#include <linux/device.h> > +#include <linux/kref.h> > +#include <linux/list.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/slab.h> > + > +struct master { > + struct list_head node; > + struct list_head components; > + bool bound; > + > + const struct component_master_ops *ops; > + struct device *dev; > +}; > + > +struct component { > + struct list_head node; > + struct list_head master_node; > + struct master *master; > + bool bound; > + > + const struct component_ops *ops; > + struct device *dev; > +}; > + > +static DEFINE_MUTEX(component_mutex); > +static LIST_HEAD(component_list); > +static LIST_HEAD(masters); > + > +static struct master *__master_find(struct device *dev, const struct component_master_ops *ops) > +{ > + struct master *m; > + > + list_for_each_entry(m, &masters, node) > + if (m->dev == dev && (!ops || m->ops == ops)) > + return m; > + > + return NULL; > +} > + > +/* Attach an unattached component to a master. */ > +static void component_attach_master(struct master *master, struct component *c) > +{ > + c->master = master; > + > + list_add_tail(&c->master_node, &master->components); > +} > + > +/* Detach a component from a master. */ > +static void component_detach_master(struct master *master, struct component *c) > +{ > + list_del(&c->master_node); > + > + c->master = NULL; > +} > + > +int component_master_add_child(struct master *master, > + int (*compare)(struct device *, void *), void *compare_data) > +{ > + struct component *c; > + int ret = -ENXIO; > + > + list_for_each_entry(c, &component_list, node) { > + if (c->master) > + continue; > + > + if (compare(c->dev, compare_data)) { > + component_attach_master(master, c); > + ret = 0; > + break; > + } > + } > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(component_master_add_child); > + > +/* Detach all attached components from this master */ > +static void master_remove_components(struct master *master) > +{ > + while (!list_empty(&master->components)) { > + struct component *c = list_first_entry(&master->components, > + struct component, master_node); > + > + WARN_ON(c->master != master); > + > + component_detach_master(master, c); > + } > +} > + > +/* > + * Try to bring up a master. If component is NULL, we're interested in > + * this master, otherwise it's a component which must be present to try > + * and bring up the master. > + * > + * Returns 1 for successful bringup, 0 if not ready, or -ve errno. > + */ > +static int try_to_bring_up_master(struct master *master, > + struct component *component) > +{ > + int ret = 0; > + > + if (!master->bound) { > + /* > + * Search the list of components, looking for components that > + * belong to this master, and attach them to the master. > + */ > + if (master->ops->add_components(master->dev, master)) { > + /* Failed to find all components */ > + master_remove_components(master); > + ret = 0; > + goto out; > + } > + > + if (component && component->master != master) { > + master_remove_components(master); > + ret = 0; > + goto out; > + } > + > + /* Found all components */ > + ret = master->ops->bind(master->dev); > + if (ret < 0) { > + master_remove_components(master); > + goto out; > + } > + > + master->bound = true; > + ret = 1; > + } > +out: > + > + return ret; > +} > + > +static int try_to_bring_up_masters(struct component *component) > +{ > + struct master *m; > + int ret = 0; > + > + list_for_each_entry(m, &masters, node) { > + ret = try_to_bring_up_master(m, component); > + if (ret != 0) > + break; > + } > + > + return ret; > +} > + > +static void take_down_master(struct master *master) > +{ > + if (master->bound) { > + master->ops->unbind(master->dev); > + master->bound = false; > + } > + > + master_remove_components(master); > +} > + > +int component_master_add(struct device *dev, const struct component_master_ops *ops) > +{ > + struct master *master; > + int ret; > + > + master = kzalloc(sizeof(*master), GFP_KERNEL); > + if (!master) > + return -ENOMEM; > + > + master->dev = dev; > + master->ops = ops; > + INIT_LIST_HEAD(&master->components); > + > + /* Add to the list of available masters. */ > + mutex_lock(&component_mutex); > + list_add(&master->node, &masters); > + > + ret = try_to_bring_up_master(master, NULL); > + > + if (ret < 0) { > + /* Delete off the list if we weren't successful */ > + list_del(&master->node); > + kfree(master); > + } > + mutex_unlock(&component_mutex); > + > + return ret < 0 ? ret : 0; > +} > +EXPORT_SYMBOL_GPL(component_master_add); > + > +void component_master_del(struct device *dev, const struct component_master_ops *ops) > +{ > + struct master *master; > + > + mutex_lock(&component_mutex); > + master = __master_find(dev, ops); > + if (master) { > + take_down_master(master); > + > + list_del(&master->node); > + kfree(master); > + } > + mutex_unlock(&component_mutex); > +} > +EXPORT_SYMBOL_GPL(component_master_del); > + > +static void component_unbind(struct component *component, > + struct master *master, void *data) > +{ > + WARN_ON(!component->bound); > + > + component->ops->unbind(component->dev, master->dev, data); > + component->bound = false; > + > + /* Release all resources claimed in the binding of this component */ > + devres_release_group(component->dev, component); > +} > + > +void component_unbind_all(struct device *master_dev, void *data) > +{ > + struct master *master; > + struct component *c; > + > + WARN_ON(!mutex_is_locked(&component_mutex)); > + > + master = __master_find(master_dev, NULL); > + if (!master) > + return; > + > + list_for_each_entry_reverse(c, &master->components, master_node) > + component_unbind(c, master, data); > +} > +EXPORT_SYMBOL_GPL(component_unbind_all); > + > +static int component_bind(struct component *component, struct master *master, > + void *data) > +{ > + int ret; > + > + /* > + * Each component initialises inside its own devres group. > + * This allows us to roll-back a failed component without > + * affecting anything else. > + */ > + if (!devres_open_group(master->dev, NULL, GFP_KERNEL)) > + return -ENOMEM; > + > + /* > + * Also open a group for the device itself: this allows us > + * to release the resources claimed against the sub-device > + * at the appropriate moment. > + */ > + if (!devres_open_group(component->dev, component, GFP_KERNEL)) { > + devres_release_group(master->dev, NULL); > + return -ENOMEM; > + } > + > + dev_dbg(master->dev, "binding %s (ops %ps)\n", > + dev_name(component->dev), component->ops); > + > + ret = component->ops->bind(component->dev, master->dev, data); > + if (!ret) { > + component->bound = true; > + > + /* > + * Close the component device's group so that resources > + * allocated in the binding are encapsulated for removal > + * at unbind. Remove the group on the DRM device as we > + * can clean those resources up independently. > + */ > + devres_close_group(component->dev, NULL); > + devres_remove_group(master->dev, NULL); > + > + dev_info(master->dev, "bound %s (ops %ps)\n", > + dev_name(component->dev), component->ops); > + } else { > + devres_release_group(component->dev, NULL); > + devres_release_group(master->dev, NULL); > + > + dev_err(master->dev, "failed to bind %s (ops %ps): %d\n", > + dev_name(component->dev), component->ops, ret); > + } > + > + return ret; > +} > + > +int component_bind_all(struct device *master_dev, void *data) > +{ > + struct master *master; > + struct component *c; > + int ret = 0; > + > + WARN_ON(!mutex_is_locked(&component_mutex)); > + > + master = __master_find(master_dev, NULL); > + if (!master) > + return -EINVAL; > + > + list_for_each_entry(c, &master->components, master_node) { > + ret = component_bind(c, master, data); > + if (ret) > + break; > + } > + > + if (ret != 0) { > + list_for_each_entry_continue_reverse(c, &master->components, > + master_node) > + component_unbind(c, master, data); > + } > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(component_bind_all); > + > +int component_add(struct device *dev, const struct component_ops *ops) > +{ > + struct component *component; > + int ret; > + > + component = kzalloc(sizeof(*component), GFP_KERNEL); > + if (!component) > + return -ENOMEM; > + > + component->ops = ops; > + component->dev = dev; > + > + dev_dbg(dev, "adding component (ops %ps)\n", ops); > + > + mutex_lock(&component_mutex); > + list_add_tail(&component->node, &component_list); > + > + ret = try_to_bring_up_masters(component); > + if (ret < 0) { > + list_del(&component->node); > + > + kfree(component); > + } > + mutex_unlock(&component_mutex); > + > + return ret < 0 ? ret : 0; > +} > +EXPORT_SYMBOL_GPL(component_add); > + > +void component_del(struct device *dev, const struct component_ops *ops) > +{ > + struct component *c, *component = NULL; > + > + mutex_lock(&component_mutex); > + list_for_each_entry(c, &component_list, node) > + if (c->dev == dev && c->ops == ops) { > + list_del(&c->node); > + component = c; > + break; > + } > + > + if (component && component->master) > + take_down_master(component->master); > + > + mutex_unlock(&component_mutex); > + > + WARN_ON(!component); > + kfree(component); > +} > +EXPORT_SYMBOL_GPL(component_del); > + > +MODULE_LICENSE("GPL v2"); > diff --git a/include/linux/component.h b/include/linux/component.h > new file mode 100644 > index 000000000000..73657636db0b > --- /dev/null > +++ b/include/linux/component.h > @@ -0,0 +1,31 @@ > +#ifndef COMPONENT_H > +#define COMPONENT_H > + > +struct device; > + > +struct component_ops { > + int (*bind)(struct device *, struct device *, void *); > + void (*unbind)(struct device *, struct device *, void *); > +}; > + > +int component_add(struct device *, const struct component_ops *); > +void component_del(struct device *, const struct component_ops *); > + > +int component_bind_all(struct device *, void *); > +void component_unbind_all(struct device *, void *); > + > +struct master; > + > +struct component_master_ops { > + int (*add_components)(struct device *, struct master *); > + int (*bind)(struct device *); > + void (*unbind)(struct device *); > +}; > + > +int component_master_add(struct device *, const struct component_master_ops *); > +void component_master_del(struct device *, const struct component_master_ops *); > + > +int component_master_add_child(struct master *master, > + int (*compare)(struct device *, void *), void *compare_data); > + > +#endif > -- > 1.7.4.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Feb 07, 2014 at 10:04:30AM +0100, Daniel Vetter wrote: > I've chatted a bit with Hans Verkuil about this topic at fosdem and > apparently both v4l and alsa have something like this already in their > helper libraries. Adding more people as fyi in case they want to > switch to the new driver core stuff from Russell. It's not ALSA, but ASoC which has this. Mark is already aware of this and will be looking at it from an ASoC perspective.
On Fri, 7 Feb 2014 09:46:56 +0000 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Fri, Feb 07, 2014 at 10:04:30AM +0100, Daniel Vetter wrote: > > I've chatted a bit with Hans Verkuil about this topic at fosdem and > > apparently both v4l and alsa have something like this already in their > > helper libraries. Adding more people as fyi in case they want to > > switch to the new driver core stuff from Russell. > > It's not ALSA, but ASoC which has this. Mark is already aware of this > and will be looking at it from an ASoC perspective. Russell, I started to use your code (which works fine, thanks), and it avoids a lot of problems, especially, about probe_defer in a DT context. I was wondering if your componentised mechanism could be extended to the devices defined by DT. In the DT, when a device_node is a phandle, this means it is referenced by some other device(s), and these device(s) will not start until the phandle device is registered. Then, the idea is to do a component_add() for such phandle devices in device_add() (device_register). Pratically, - the component_add() call in device_register would not include any bind/unbind callback function, so, this should be tested in component_bind/unbind(), - component_add would not be called if the device being added already called component_add in its probe function. A simple flag in the struct device_node should solve this problem. What do you think about this?
On Fri, Feb 07, 2014 at 12:57:21PM +0100, Jean-Francois Moine wrote: > I started to use your code (which works fine, thanks), and it avoids a > lot of problems, especially, about probe_defer in a DT context. > > I was wondering if your componentised mechanism could be extended to the > devices defined by DT. It was developed against imx-drm, which is purely DT based. I already have a solution for the cubox armada DRM.
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. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
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. 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.
Hi Russell, Sorry for a long delay. On Wed, 26 Feb 2014, Russell King - ARM Linux wrote: [snip] > 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. Yes, I'm aware of that patch series, and I do look at the discussion from time to time, unfortunately I don't have too much time for it now. But in any case if this work is going to be used with imx-drm too, that should be a good direction to take, I think. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/
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.
Hi Russell, (CC'ing Philipp Zabel who might be able to provide feedback as a user of the component framework) Could you please have a look at the questions below and provide an answer when you'll have time ? I'd like to bridge the gap between the component and the V4L2 asynchronous registration implementations. On Friday 07 March 2014 00:24:33 Laurent Pinchart wrote: > 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.
On Wed, Mar 19, 2014 at 06:22:14PM +0100, Laurent Pinchart wrote: > Hi Russell, > > (CC'ing Philipp Zabel who might be able to provide feedback as a user of the > component framework) > > Could you please have a look at the questions below and provide an answer when > you'll have time ? I'd like to bridge the gap between the component and the > V4L2 asynchronous registration implementations. I have a reply partly prepared, but I'm snowed under by the L2 cache stuff at the moment, sorry.
On Fri, Mar 07, 2014 at 12:24:33AM +0100, Laurent Pinchart wrote: > 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. There's a lot of people who would have liked to be on the Cc, but there's two problems: 1. the Cc list would be too big for mailing lists to accept the message, and 2. finding everyone who should be on the Cc list is quite an impossible task. > 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. Yes, I am aware of CDF. However, the annoying thing is that it's another case of the bigger picture not being looked at - which is that we don't need yet another subsystem specific solution to a problem which is not subsystem specific. The fact of the matter is that /anyone/ has the opportunity to come up with a generic solution to this problem, and no one did... instead, more solutions were generated - the proof is "we solved this in CDF with a CDF specific solution". :p > 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. What would have happened is that CDF would have been raised, and there would be a big long discussion with no real resolution. The problem would not have been solved (even partially). We'd be sitting here right now still without any kind of solution that anyone can use. Instead, what we have right now is the opportunity for people to start making use of this and solving the real problems they have with driver initialisation. For example, the IPU on iMX locks up after a number of mode changes, and it's useful to be able to unload the driver, poke about in the hardware, and reload it. Without this problem fixed, that's impossible without rebooting the kernel, because removing the driver oopses the kernel due to the broken work-arounds that it has to do - and it has to do those because this problem has not been solved, despite it being known about for /years/. > 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. There's lots of things in the kernel which you just have to read the code for - and this is one of them at the moment. :) (Another is PM domains...) What I know is that this will not satisfy all your requirements - I don't want it to initially satisfy everyone's requirements, because that's just far too big a job, but it satisfies the common problem that most people are suffering from and have already implemented many badly written driver specific solutions. In other words - this is designed to _improve_ the current situation where we have lots of buggy implementions trying to work around this problem, factor that code out, and fix up those problems. Briefly, the idea is: - there is a master device - lots of these subsystems doing this already have that, whether that be ALSA or DRM based stuff. - then there are the individual component devices and their drivers. Subsystems like ALSA and DRM are not component based subsystems. These subsystems have two states - either they're initialised and the entire "card system" is known about, or they're not initialised. There is no possibility of a piecemeal approach, where they partially come up and additional stuff gets added later. With DRM, that's especially true because of how the userspace API works - to change that probably means changing stuff all the way through things like the X server and its xrandr application interface. This is probably the reason why David said at KS that DRM isn't going to do the hotplugging of components. The master device has a privileged position - it gets to make the decision about which component devices are relevant to it, and when the "card system" is fully known. As far as DT goes, we've had a long discussion about this approach in the past, and we've accepted this approach - we have the "sound" node which doesn't actually refer to any hardware block, it's a node which describes how the hardware blocks are connected together, which gets translated into a platform device. When a master device gets added, it gets added to the list of master devices, and then it's asked whether all the components that it needs are present. Since we don't want to fixate on any one particular method to make that decision, it calls back via a method to the master driver for that. The master then determines which components should be present, and asks the component helpers to check whether they are. The master does this in the order which the components are to be later bound (there's subsystems which have requirements on the ordering of individual components appearing - for example, with DRM the CRTCs need to be available before the encoders so that you know the CRTC indexes to bind the encoders to.) Each component that is found is added to a master-specific list, making it unavailable to other masters (any particular component can't be owned by two masters.) If a component isn't found, then this process is unwound, and we wait for more components to show up. When a component is added, it is added to a list of all components. All unbound masters are polled to see whether they require this component in the same way as above. When a master indicates that it is complete, the master is bound, which kicks off what would be the standard driver probe on non-componentised systems. Since we now know that we have all the components associated with this master, this is now safe to do, and the master driver gets to decide at what point during subsystem initialisation the components should be bound. This will normally be when the DRM device private structures or ALSA card has been allocated, and are available for the components to bind themselves to. Should any component or master return a deferred probe, that error code (or in fact any error code) is propagated all the way back through the chain, unwinding the effects as we go - including devres allocated resources. Eventually, we hit the caller to component_add() or component_master_add(), which would be a driver probe function, and this propagates it back to the driver model. So, the last driver (master or component) gets to eat any errors from the bring up - that's reasonable, because we can't return an error from an already completed probe function. The side effect of this is very important though - it gets us deferred probing. That failed driver probe with -EPROBE_DEFER will be retried later, hopefully when the resources that any one of those components needed is now available. Now, that's only half the story - the other half is far more important, because it's the one which gives the most problems with the current driver specific implementations, and is the one which leads to kernel oopses. That is, what happens when a bound component or master goes away. Remember that ALSA and DRM can't cope with that - the only way they can cope is by having the entire "card system" removed. It's an all or nothing case. With DRM, you wouldn't be able to remove a CRTC for example, because that would break the CRTC numbering that userspace sees via the kernel API, and also the CRTC numbering which xrandr Xorg applications see. So, when any component/master of a subsystem is removed, the whole setup is unbound: this starts with the master being unbound, and the master controls the point at which the components are unbound. Again, normal devres actions happen so drivers see proper devres behaviour. > 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. Right - I didn't want to modify existing structures like struct device, because they're already bloated enough. Given that this currently affects a small number of devices, it is unfair to impose this overhead on all device structures in the kernel. > 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. So, what happens after all components have been bound (and the complete callback has been called), and then one component is removed from the system? From your description, it sounds like nothing apart from the "unbind" callback for that component. Moreover, what happens if that component is then added back later? Do you get another "bind" followed by "complete"? How would a subsystem like, DRM or ALSA, be able to fit into this model when these subsystems can't tolerate any component going away or being introduced after the initial bring-up? > 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. This is something we could add to the component model. However, one of the target design goals is that the core should be free of the matching method, so that we don't end up with something that is specific only to existing ARM systems. However, one of the issues here is how to pass this information in - it can't be a static array (see below.) > 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. The more interesting question is what happens when a component is removed. This is what kills a lot of bad implementations of this right now. It's basically impossible to remove any component without oopsing the kernel. > 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) ? This doesn't work everywhere. Take ALSA as an example. There is struct snd_card. This is created by snd_card_create() and allocates your driver private data structure for you in addition to the snd_card. When you want to tear down the snd_card, this includes freeing all memory associated with it, including the driver private data structure. If a component helper data structure were to be embedded in the driver private data structure, it would also be freed - while still being registered into component stuff. Conversely, if you created the snd_card outside of the component stuff, then you have no real way to remove the components when they go away. Of course, the driver could allocate its own struct just for this, but then where does it store a pointer to that? In the driver data? What if the subsystem (and some do) insist on putting their own data in the struct device driver data pointer? DRM used to do exactly that (it's changed very recently to allow drivers to do their own thing wrt driver_data - but this wasn't the case when the component stuff was written, which was shortly after kernel summit.) So, that's why I made the decision to have it as a separate structure. Using the solution you're suggesting would result in unnecessary restrictions on how drivers and subsystems should behave. I don't think any of us want to go through changing the way lots of subsystems and possibly hundreds of drivers work in this regard. Hence, while it may not appear the simplest solution, in the bigger picture, it's better not to impose any kind of avoidable requirements on drivers. > - 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 supply an array of match function, match data, but the issue here is that the match data generally isn't constant - the driver would have to allocate an array of {function, data} and register that array into the component helper. Not much problem with that, except it results in more complexity. > 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. This I don't like, because it's making assumptions about the struct device, encoding subsystem and firmware specific knowledge. It's fine to do that as a match helper, but not as something core. > - 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 ? As you can see from the above explanation, this is intentional in this iteration, as this is the common requirement for these subsystems that it's trying to solve the problem for. That's not to say that it can't be done - at the moment I have some concerns from the point of view of these other subsystems which don't support partial bind, and ensuring that partial bind doesn't get mis-used there. In any case, there is scope for the way the master discovers its components to be changed, and I feel that would be a pre-requisit to partial bind support. Philipp's work on imx-drm for example, has ended up allocating an ordered list of devices, though there's patches around to remove that again. That was necessary due to a work-around for the brokenness in existing imx-drm for the CRTC nodes (which prevents a successful match against a previously found component) which will now be removed.
diff --git a/drivers/base/Makefile b/drivers/base/Makefile index 94e8a80e87f8..870ecfd503af 100644 --- a/drivers/base/Makefile +++ b/drivers/base/Makefile @@ -1,6 +1,6 @@ # Makefile for the Linux device tree -obj-y := core.o bus.o dd.o syscore.o \ +obj-y := component.o core.o bus.o dd.o syscore.o \ driver.o class.o platform.o \ cpu.o firmware.o init.o map.o devres.o \ attribute_container.o transport_class.o \ diff --git a/drivers/base/component.c b/drivers/base/component.c new file mode 100644 index 000000000000..5492cd8d2247 --- /dev/null +++ b/drivers/base/component.c @@ -0,0 +1,379 @@ +/* + * Componentized device handling. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This is work in progress. We gather up the component devices into a list, + * and bind them when instructed. At the moment, we're specific to the DRM + * subsystem, and only handles one master device, but this doesn't have to be + * the case. + */ +#include <linux/component.h> +#include <linux/device.h> +#include <linux/kref.h> +#include <linux/list.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/slab.h> + +struct master { + struct list_head node; + struct list_head components; + bool bound; + + const struct component_master_ops *ops; + struct device *dev; +}; + +struct component { + struct list_head node; + struct list_head master_node; + struct master *master; + bool bound; + + const struct component_ops *ops; + struct device *dev; +}; + +static DEFINE_MUTEX(component_mutex); +static LIST_HEAD(component_list); +static LIST_HEAD(masters); + +static struct master *__master_find(struct device *dev, const struct component_master_ops *ops) +{ + struct master *m; + + list_for_each_entry(m, &masters, node) + if (m->dev == dev && (!ops || m->ops == ops)) + return m; + + return NULL; +} + +/* Attach an unattached component to a master. */ +static void component_attach_master(struct master *master, struct component *c) +{ + c->master = master; + + list_add_tail(&c->master_node, &master->components); +} + +/* Detach a component from a master. */ +static void component_detach_master(struct master *master, struct component *c) +{ + list_del(&c->master_node); + + c->master = NULL; +} + +int component_master_add_child(struct master *master, + int (*compare)(struct device *, void *), void *compare_data) +{ + struct component *c; + int ret = -ENXIO; + + list_for_each_entry(c, &component_list, node) { + if (c->master) + continue; + + if (compare(c->dev, compare_data)) { + component_attach_master(master, c); + ret = 0; + break; + } + } + + return ret; +} +EXPORT_SYMBOL_GPL(component_master_add_child); + +/* Detach all attached components from this master */ +static void master_remove_components(struct master *master) +{ + while (!list_empty(&master->components)) { + struct component *c = list_first_entry(&master->components, + struct component, master_node); + + WARN_ON(c->master != master); + + component_detach_master(master, c); + } +} + +/* + * Try to bring up a master. If component is NULL, we're interested in + * this master, otherwise it's a component which must be present to try + * and bring up the master. + * + * Returns 1 for successful bringup, 0 if not ready, or -ve errno. + */ +static int try_to_bring_up_master(struct master *master, + struct component *component) +{ + int ret = 0; + + if (!master->bound) { + /* + * Search the list of components, looking for components that + * belong to this master, and attach them to the master. + */ + if (master->ops->add_components(master->dev, master)) { + /* Failed to find all components */ + master_remove_components(master); + ret = 0; + goto out; + } + + if (component && component->master != master) { + master_remove_components(master); + ret = 0; + goto out; + } + + /* Found all components */ + ret = master->ops->bind(master->dev); + if (ret < 0) { + master_remove_components(master); + goto out; + } + + master->bound = true; + ret = 1; + } +out: + + return ret; +} + +static int try_to_bring_up_masters(struct component *component) +{ + struct master *m; + int ret = 0; + + list_for_each_entry(m, &masters, node) { + ret = try_to_bring_up_master(m, component); + if (ret != 0) + break; + } + + return ret; +} + +static void take_down_master(struct master *master) +{ + if (master->bound) { + master->ops->unbind(master->dev); + master->bound = false; + } + + master_remove_components(master); +} + +int component_master_add(struct device *dev, const struct component_master_ops *ops) +{ + struct master *master; + int ret; + + master = kzalloc(sizeof(*master), GFP_KERNEL); + if (!master) + return -ENOMEM; + + master->dev = dev; + master->ops = ops; + INIT_LIST_HEAD(&master->components); + + /* Add to the list of available masters. */ + mutex_lock(&component_mutex); + list_add(&master->node, &masters); + + ret = try_to_bring_up_master(master, NULL); + + if (ret < 0) { + /* Delete off the list if we weren't successful */ + list_del(&master->node); + kfree(master); + } + mutex_unlock(&component_mutex); + + return ret < 0 ? ret : 0; +} +EXPORT_SYMBOL_GPL(component_master_add); + +void component_master_del(struct device *dev, const struct component_master_ops *ops) +{ + struct master *master; + + mutex_lock(&component_mutex); + master = __master_find(dev, ops); + if (master) { + take_down_master(master); + + list_del(&master->node); + kfree(master); + } + mutex_unlock(&component_mutex); +} +EXPORT_SYMBOL_GPL(component_master_del); + +static void component_unbind(struct component *component, + struct master *master, void *data) +{ + WARN_ON(!component->bound); + + component->ops->unbind(component->dev, master->dev, data); + component->bound = false; + + /* Release all resources claimed in the binding of this component */ + devres_release_group(component->dev, component); +} + +void component_unbind_all(struct device *master_dev, void *data) +{ + struct master *master; + struct component *c; + + WARN_ON(!mutex_is_locked(&component_mutex)); + + master = __master_find(master_dev, NULL); + if (!master) + return; + + list_for_each_entry_reverse(c, &master->components, master_node) + component_unbind(c, master, data); +} +EXPORT_SYMBOL_GPL(component_unbind_all); + +static int component_bind(struct component *component, struct master *master, + void *data) +{ + int ret; + + /* + * Each component initialises inside its own devres group. + * This allows us to roll-back a failed component without + * affecting anything else. + */ + if (!devres_open_group(master->dev, NULL, GFP_KERNEL)) + return -ENOMEM; + + /* + * Also open a group for the device itself: this allows us + * to release the resources claimed against the sub-device + * at the appropriate moment. + */ + if (!devres_open_group(component->dev, component, GFP_KERNEL)) { + devres_release_group(master->dev, NULL); + return -ENOMEM; + } + + dev_dbg(master->dev, "binding %s (ops %ps)\n", + dev_name(component->dev), component->ops); + + ret = component->ops->bind(component->dev, master->dev, data); + if (!ret) { + component->bound = true; + + /* + * Close the component device's group so that resources + * allocated in the binding are encapsulated for removal + * at unbind. Remove the group on the DRM device as we + * can clean those resources up independently. + */ + devres_close_group(component->dev, NULL); + devres_remove_group(master->dev, NULL); + + dev_info(master->dev, "bound %s (ops %ps)\n", + dev_name(component->dev), component->ops); + } else { + devres_release_group(component->dev, NULL); + devres_release_group(master->dev, NULL); + + dev_err(master->dev, "failed to bind %s (ops %ps): %d\n", + dev_name(component->dev), component->ops, ret); + } + + return ret; +} + +int component_bind_all(struct device *master_dev, void *data) +{ + struct master *master; + struct component *c; + int ret = 0; + + WARN_ON(!mutex_is_locked(&component_mutex)); + + master = __master_find(master_dev, NULL); + if (!master) + return -EINVAL; + + list_for_each_entry(c, &master->components, master_node) { + ret = component_bind(c, master, data); + if (ret) + break; + } + + if (ret != 0) { + list_for_each_entry_continue_reverse(c, &master->components, + master_node) + component_unbind(c, master, data); + } + + return ret; +} +EXPORT_SYMBOL_GPL(component_bind_all); + +int component_add(struct device *dev, const struct component_ops *ops) +{ + struct component *component; + int ret; + + component = kzalloc(sizeof(*component), GFP_KERNEL); + if (!component) + return -ENOMEM; + + component->ops = ops; + component->dev = dev; + + dev_dbg(dev, "adding component (ops %ps)\n", ops); + + mutex_lock(&component_mutex); + list_add_tail(&component->node, &component_list); + + ret = try_to_bring_up_masters(component); + if (ret < 0) { + list_del(&component->node); + + kfree(component); + } + mutex_unlock(&component_mutex); + + return ret < 0 ? ret : 0; +} +EXPORT_SYMBOL_GPL(component_add); + +void component_del(struct device *dev, const struct component_ops *ops) +{ + struct component *c, *component = NULL; + + mutex_lock(&component_mutex); + list_for_each_entry(c, &component_list, node) + if (c->dev == dev && c->ops == ops) { + list_del(&c->node); + component = c; + break; + } + + if (component && component->master) + take_down_master(component->master); + + mutex_unlock(&component_mutex); + + WARN_ON(!component); + kfree(component); +} +EXPORT_SYMBOL_GPL(component_del); + +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/component.h b/include/linux/component.h new file mode 100644 index 000000000000..73657636db0b --- /dev/null +++ b/include/linux/component.h @@ -0,0 +1,31 @@ +#ifndef COMPONENT_H +#define COMPONENT_H + +struct device; + +struct component_ops { + int (*bind)(struct device *, struct device *, void *); + void (*unbind)(struct device *, struct device *, void *); +}; + +int component_add(struct device *, const struct component_ops *); +void component_del(struct device *, const struct component_ops *); + +int component_bind_all(struct device *, void *); +void component_unbind_all(struct device *, void *); + +struct master; + +struct component_master_ops { + int (*add_components)(struct device *, struct master *); + int (*bind)(struct device *); + void (*unbind)(struct device *); +}; + +int component_master_add(struct device *, const struct component_master_ops *); +void component_master_del(struct device *, const struct component_master_ops *); + +int component_master_add_child(struct master *master, + int (*compare)(struct device *, void *), void *compare_data); + +#endif
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>; }; The superdevice is declared into the component support, along with the subcomponents. The superdevice receives callbacks to locate the subcomponents, and identify when all components are present. At this point, we bind the superdevice, which causes the appropriate subsystem to be initialised in the conventional way. When any of the components or superdevice are removed from the system, we unbind the superdevice, thereby taking the subsystem down. Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk> --- drivers/base/Makefile | 2 +- drivers/base/component.c | 379 +++++++++++++++++++++++++++++++++++++++++++++ include/linux/component.h | 31 ++++ 3 files changed, 411 insertions(+), 1 deletions(-) create mode 100644 drivers/base/component.c create mode 100644 include/linux/component.h