Ping - any comments from anyone on this idea?
On Fri, Jul 06, 2018 at 01:43:05PM +0100, Russell King - ARM Linux wrote:
> On Fri, Jul 06, 2018 at 11:03:46AM +0100, Russell King - ARM Linux wrote:
> > On Wed, Apr 25, 2018 at 11:01:15PM +0300, Jyri Sarha wrote:
> > > Oh yes. But in this case the substandard solution is already there and
> > > it is already widely used, despite it being severely broken. I am merely
> > > trying to fix the existing substandard solution.
> > >
> > > I admit that a full integration with component helpers would probably be
> > > more elegant solution to the problem, but the amount of work is just too
> > > much. The change would impact the way all the master drm drivers pull
> > > them selves together. The drivers that already use the component helpers
> > > for some internal stuff will add their own challenge. Separate component
> > > matching implementations are needed for device-tree and ACPI (are ther
> > > more flavors?) etc. I just do not see this happening any time soon (am
> > > happy to be wrong about this).
> >
> > The issue is actually worse than that:
> >
> > - drivers that are already componentised can't use bridges
> > - drivers that use bridges can't use componentised stuff
> >
> > because bridges don't register themselves with the component helper,
> > and the helpers in drm_of.c assume that all graph nodes will be
> > components.
> >
> > The whole thing about whether stuff is componentised or bridge based
> > is really getting out of hand, and the push is towards bridge based
> > stuff even though that is technically inferior when it comes to being
> > able to develop and test (which involves being able to remove and
> > re-insert modules.)
> >
> > Consequently more and more code is being written for bridges, and
> > the component helper ignored, and the problems with bridges are
> > being ignored. This is not healthy.
> >
> > The problem is only going to get worse. Someone needs to bite the
> > bullet and fix bridges before the problem gets any more out of hand.
>
> This patch (which is actually two patches locally) allows the component
> helper to know what's going on inside the bridge code wrt bridge
> availability, and takes the appropriate action at the correct time.
> No need for device links or similar, or incompatibilities between
> bridges and components. The only requirement is that bridges set the
> "device" member of struct drm_bridge to opt-in to this.
>
> Tested with Armada converted to support bridges, TDA998x as a
> componentised bridge, and dumb-vga-dac as a non-componentised bridge:
>
> root@cubox:~# less /sys/kernel/debug/device_component/display-subsystem
> master name status
> -------------------------------------------------------------
> display-subsystem bound
>
> device name status
> -------------------------------------------------------------
> port registered
> port registered
> hdmi-encoder registered
> vga-bridge registered
> root@cubox:~# dmesg |grep bound
> [ 1.921798] armada-drm display-subsystem: bound f1820000.lcd-controller (ops
> armada_lcd_ops)
> [ 1.931014] armada-drm display-subsystem: bound f1810000.lcd-controller (ops
> armada_lcd_ops)
> [ 2.069231] armada-drm display-subsystem: bound 1-0070 (ops tda998x_ops)
> [ 2.076059] armada-drm display-subsystem: bound vga-bridge (ops dummy_ops)
>
> Without this, the same DT fails because "vga-bridge" is never added
> to the component helpers.
>
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index 8946dfee4768..b14b3a3655ea 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -602,4 +602,32 @@ void component_del(struct device *dev, const struct component_ops *ops)
> }
> EXPORT_SYMBOL_GPL(component_del);
>
> +static int component_dummy_bind(struct device *comp, struct device *master,
> + void *master_data)
> +{
> + return 0;
> +}
> +
> +static void component_dummy_unbind(struct device *comp, struct device *master,
> + void *master_data)
> +{
> +}
> +
> +static const struct component_ops dummy_ops = {
> + .bind = component_dummy_bind,
> + .unbind = component_dummy_unbind,
> +};
> +
> +int component_mark_available(struct device *dev)
> +{
> + return component_add(dev, &dummy_ops);
> +}
> +EXPORT_SYMBOL_GPL(component_mark_available);
> +
> +void component_mark_unavailable(struct device *dev)
> +{
> + component_del(dev, &dummy_ops);
> +}
> +EXPORT_SYMBOL_GPL(component_mark_unavailable);
> +
> MODULE_LICENSE("GPL v2");
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 1638bfe9627c..ce3ccd327916 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -21,6 +21,7 @@
> * DEALINGS IN THE SOFTWARE.
> */
>
> +#include <linux/component.h>
> #include <linux/err.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> @@ -73,6 +74,9 @@ void drm_bridge_add(struct drm_bridge *bridge)
> mutex_lock(&bridge_lock);
> list_add_tail(&bridge->list, &bridge_list);
> mutex_unlock(&bridge_lock);
> +
> + if (bridge->device)
> + WARN_ON(component_mark_available(bridge->device));
> }
> EXPORT_SYMBOL(drm_bridge_add);
>
> @@ -83,6 +87,9 @@ EXPORT_SYMBOL(drm_bridge_add);
> */
> void drm_bridge_remove(struct drm_bridge *bridge)
> {
> + if (bridge->device)
> + component_mark_unavailable(bridge->device);
> +
> mutex_lock(&bridge_lock);
> list_del_init(&bridge->list);
> mutex_unlock(&bridge_lock);
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 3270fec46979..e863da14d4d9 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -268,6 +268,7 @@ struct drm_bridge {
> struct drm_device *dev;
> struct drm_encoder *encoder;
> struct drm_bridge *next;
> + struct device *device;
> #ifdef CONFIG_OF
> struct device_node *of_node;
> #endif
> diff --git a/include/linux/component.h b/include/linux/component.h
> index e71fbbbc74e2..a1c824485f54 100644
> --- a/include/linux/component.h
> +++ b/include/linux/component.h
> @@ -16,6 +16,10 @@ struct component_ops {
> int component_add(struct device *, const struct component_ops *);
> void component_del(struct device *, const struct component_ops *);
>
> +/* For subsystems where drivers do not call component_add()/component_del() */
> +int component_mark_available(struct device *dev);
> +void component_mark_unavailable(struct device *dev);
> +
> int component_bind_all(struct device *master, void *master_data);
> void component_unbind_all(struct device *master, void *master_data);
>
>
>
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
> According to speedtest.net: 13Mbps down 490kbps up
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
