On Wed, Oct 21, 2015 at 10:04:15PM +0100, Russell King - ARM Linux wrote:
> On Wed, Oct 21, 2015 at 10:37:27PM +0200, Takashi Iwai wrote:
> > On Wed, 21 Oct 2015 20:19:38 +0200,
> > Russell King - ARM Linux wrote:
> > >
> > > On Wed, Oct 21, 2015 at 07:59:06PM +0200, Takashi Iwai wrote:
> > > > On Wed, 21 Oct 2015 19:34:37 +0200,
> > > > Russell King - ARM Linux wrote:
> > > > >
> > > > > On Wed, Oct 21, 2015 at 09:49:28PM +0530, Vinod Koul wrote:
> > > > > > On Wed, Oct 21, 2015 at 03:37:47PM +0100, Russell King - ARM Linux wrote:
> > > > > > > In any case, this doesn't (and can't) solve the CEC problem, so it's not
> > > > > > > a solution to the problem at hand.
> > > > > >
> > > > > > Sorry am not sure I follow the reasons for that, wouldn't CEC be another
> > > > > > slave in such an interface? I though component fwk did allow us to have
> > > > > > multiple slaves..
> > > > >
> > > > > Not with the way you're using the component helper here.
> > > > >
> > > > > I guess that not all my message is being read, because people keep
> > > > > replying half-way down my messages...
> > > > >
> > > > > You can only register a struct device _once_ as a slave device.
> > > > >
> > > > > With the way you're using it here for audio, you're registering the
> > > > > i915 DRM device as a slave component device, and the audio side as
> > > > > the master. That means the audio master can bind to the DRM slave
> > > > > component device.
> > > > >
> > > > > You can't then have a CEC master bind to the i915 DRM slave device
> > > > > (it's already bound to the audio master device), and you can't
> > > > > register the i915 DRM device as a second slave component device.
> > > > > It becomes indistinguishable from the first, and there's no way
> > > > > to tell which of the two different 'ops' structures should be used
> > > > > with which master.
> > > > >
> > > > > I said this in my message 20151021140307.GE32532@xxxxxxxxxxxxxxxxxxxxxx
> > > > > which was two of my replies ago in this sub-thread.
> > > >
> > > > Can't the limitation of single slave dev be extended simply? e.g. add
> > > > some matching semantics to component_master_add_child() like a shared
> > > > key in both master and slave, and let assign only the matched slave.
> > >
> > > I've already explained that in the email message I referred to by
> > > message ID above (which was a reply to one of your previous messages)
> > >
> > > This is why I find email such a poor communication medium - I'm quite
> > > sure most people only read half an email message before hitting reply,
> > > and then they stick their reply after where they stopped reading and
> > > never bother reading the rest of the message. Normally, at this point,
> > > I'll start getting grumpy and sending frustrated emails... which would
> > > eventually deride into flames, but let's _try_ to keep this civil.
> >
> > Thank you for patience!
> >
> > > Here's the relevant paragraph from the above referenced message, and
> > > to make the appropriate bit stand out, I've underlined it with ^.
> > >
> > > > However, there's a lurking problem: you can't register the same struct
> > > > device as a slave more than once into the component helpers - that's
> > > > because it's designed to look for devices. There's intentionally no
> > > ^^^^^^^^^^^^^^^^^^^^^^^^
> > > > linkage between the slave ops and master ops to allow for generic
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > slave drivers (like tda998x) to be used with different master drivers
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > > (armada, tilcdc, etc). In theory, you can register the master device
> > > ^^^^^^^^^^^^^^^^^^^^^
> > > > of one componentised system as a slave device of another, but that
> > > > requires a much more complicated locking setup in component.c (I have
> > > > patches for that, but I've resisted sending them because it makes the
> > > > locking very messy.)
> > >
> > > In a subsequent sentence, I also address the issue that would occur
> > > if any of the already componentised DRM drivers were to attempt what
> > > you're doing in i915 - although I say it's solvable, I've resisted
> > > that because it makes the locking in there _much_ more hairy, and I'm
> > > now not certain that my more complex locking implementation would
> > > even cope with that scenario.
> >
> > I understood that the original component design wasn't for cross
> > subsystems. OTOH I wondered why it can't be extended for wider use --
> > that was the simple question; I haven't seen so complex locking in
> > some upstream drm drivers code through a quick glance, so the mess
> > about locking you mentioned wasn't clear to me. Now point taken.
>
> You can find my patch for the "more complex locking" problem at the
> end of this mail. This may not apply to the current version of
> component.c, but illustrates at least some of the problem of getting
> rid of the global component_mutex lock, which would be necessary to
> allow a binding master to call back into the component helper to
> then register a slave. Today, that action would deadlock on
> component_mutex.
>
> I'm not actually convinced that even this patch is correct as it
> stands...
>
> > To be noted, a weak dependency is still necessary for i915 audio case,
> > and a simple notifier type registration won't work, unfortunately.
> > GPU power adjustment is mandatory even at probing the audio hardware
> > to judge whether HDMI codec is present or not. Meanwhile the
> > dependency to the graphics must not be tight, either. The very same
> > audio driver is for a controller without graphics, too. That's the
> > reason we took the component as an alternative implementation, which
> > is a bit cleaner than the direct symbol resolving in the earlier
> > code.
> >
> > So, if any better solution that covers a use case like i915/hda is
> > present, we're willing to switch. As repeatedly written, the current
> > implementation is a stop gap. Although this doesn't look too bad,
> > there are still some obvious pitfalls as you pointed out. We can
> > paper over them (maybe I'll do for 4.4), but a fundamentally better
> > solution would be more welcome, of course.
>
> Well, I guess it's fine as a temporary stop-gap, but what I don't want
> to see is this stop-gap becoming more common. It may work for your
> i915 case, but it won't work everywhere, so it can't become a generic
> solution to this problem.
My gut feeling is that component isn't the right framework for something
generic. It's really good to build something very specific (and i915.ko
binding to the i915-specific side of snd-hda is such a case I think). But
for a generic audio-over-hdmi framework I think we need to have proper
abstraction at that level, with a set of "give me the hdmi endpoint I
want" functions for the audio side, and corresponding register functions
for the gfx side. Similar to how you get your gpios, regulators and
whatever else.
-Daniel
>
>
> drivers/base/component.c | 142 ++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 116 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/base/component.c b/drivers/base/component.c
> index f748430bb654..d72fe9bc7033 100644
> --- a/drivers/base/component.c
> +++ b/drivers/base/component.c
> @@ -30,7 +30,10 @@ struct component_match {
> struct master {
> struct list_head node;
> struct list_head components;
> + struct kref kref;
> + struct mutex mutex;
> bool bound;
> + bool dead;
>
> const struct component_master_ops *ops;
> struct device *dev;
> @@ -49,8 +52,33 @@ struct component {
>
> static DEFINE_MUTEX(component_mutex);
> static LIST_HEAD(component_list);
> +static DEFINE_MUTEX(master_mutex);
> static LIST_HEAD(masters);
>
> +static void master_release(struct kref *kref) __releases(master_mutex)
> +{
> + struct master *master = container_of(kref, struct master, kref);
> +
> + list_del(&master->node);
> + mutex_unlock(&master_mutex);
> + WARN_ON(!list_empty(&master->components));
> + kfree(master);
> +}
> +
> +static void master_release_nolock(struct kref *kref)
> +{
> + struct master *master = container_of(kref, struct master, kref);
> +
> + list_del(&master->node);
> + WARN_ON(!list_empty(&master->components));
> + kfree(master);
> +}
> +
> +static void master_norelease(struct kref *kref)
> +{
> + WARN_ON(kref);
> +}
> +
> static struct master *__master_find(struct device *dev,
> const struct component_master_ops *ops)
> {
> @@ -63,9 +91,30 @@ static struct master *__master_find(struct device *dev,
> return NULL;
> }
>
> +/*
> + * Find the master structure for this device, and make sure that the
> + * per-master lock is held. This is used from within the master device
> + * drivers bind and unbind callbacks, which should only be called from
> + * paths where the per-master lock is already held.
> + */
> +static struct master *master_find_locked(struct device *dev,
> + const struct component_master_ops *ops)
> +{
> + struct master *master;
> +
> + mutex_lock(&master_mutex);
> + master = __master_find(dev, NULL);
> + WARN_ON(master && !mutex_is_locked(&master->mutex));
> + mutex_unlock(&master_mutex);
> +
> + return master;
> +}
> +
> /* Attach an unattached component to a master. */
> static void component_attach_master(struct master *master, struct component *c)
> {
> + kref_get(&master->kref);
> +
> c->master = master;
>
> list_add_tail(&c->master_node, &master->components);
> @@ -77,6 +126,9 @@ static void component_detach_master(struct master *master, struct component *c)
> list_del(&c->master_node);
>
> c->master = NULL;
> +
> + /* This kref_put should never release the master */
> + kref_put(&master->kref, master_norelease);
> }
>
> /*
> @@ -90,6 +142,7 @@ int component_master_add_child(struct master *master,
> struct component *c;
> int ret = -ENXIO;
>
> + mutex_lock(&component_mutex);
> list_for_each_entry(c, &component_list, node) {
> if (c->master && c->master != master)
> continue;
> @@ -101,6 +154,7 @@ int component_master_add_child(struct master *master,
> break;
> }
> }
> + mutex_unlock(&component_mutex);
>
> return ret;
> }
> @@ -137,6 +191,7 @@ static int find_components(struct master *master)
> /* Detach all attached components from this master */
> static void master_remove_components(struct master *master)
> {
> + mutex_lock(&component_mutex);
> while (!list_empty(&master->components)) {
> struct component *c = list_first_entry(&master->components,
> struct component, master_node);
> @@ -145,6 +200,7 @@ static void master_remove_components(struct master *master)
>
> component_detach_master(master, c);
> }
> + mutex_unlock(&component_mutex);
> }
>
> /*
> @@ -201,20 +257,43 @@ static int try_to_bring_up_master(struct master *master,
>
> static int try_to_bring_up_masters(struct component *component)
> {
> - struct master *m;
> + struct master *m, *last = NULL;
> int ret = 0;
>
> + mutex_lock(&master_mutex);
> list_for_each_entry(m, &masters, node) {
> + if (last) {
> + kref_put(&last->kref, master_release_nolock);
> + last = NULL;
> + }
> +
> + if (m->dead || m->bound)
> + continue;
> +
> + kref_get(&m->kref);
> + mutex_unlock(&master_mutex);
> +
> + mutex_lock(&m->mutex);
> ret = try_to_bring_up_master(m, component);
> + mutex_unlock(&m->mutex);
> +
> + mutex_lock(&master_mutex);
> + last = m;
> +
> if (ret != 0)
> break;
> }
>
> + if (last)
> + kref_put(&last->kref, master_release_nolock);
> + mutex_unlock(&master_mutex);
> +
> return ret;
> }
>
> static void take_down_master(struct master *master)
> {
> + mutex_lock(&master->mutex);
> if (master->bound) {
> master->ops->unbind(master->dev);
> devres_release_group(master->dev, NULL);
> @@ -222,6 +301,7 @@ static void take_down_master(struct master *master)
> }
>
> master_remove_components(master);
> + mutex_unlock(&master->mutex);
> }
>
> static size_t component_match_size(size_t num)
> @@ -307,20 +387,25 @@ int component_master_add_with_match(struct device *dev,
> master->dev = dev;
> master->ops = ops;
> master->match = match;
> + mutex_init(&master->mutex);
> INIT_LIST_HEAD(&master->components);
> + kref_init(&master->kref);
>
> - /* Add to the list of available masters. */
> - mutex_lock(&component_mutex);
> + /*
> + * Add to the list of available masters. This is so
> + * the component code below can find this master.
> + */
> + mutex_lock(&master_mutex);
> list_add(&master->node, &masters);
> + mutex_unlock(&master_mutex);
>
> + mutex_lock(&master->mutex);
> ret = try_to_bring_up_master(master, NULL);
> + mutex_unlock(&master->mutex);
>
> - if (ret < 0) {
> - /* Delete off the list if we weren't successful */
> - list_del(&master->node);
> - kfree(master);
> - }
> - mutex_unlock(&component_mutex);
> + /* Delete off the list if we weren't successful */
> + if (ret < 0)
> + kref_put_mutex(&master->kref, master_release, &master_mutex);
>
> return ret < 0 ? ret : 0;
> }
> @@ -338,15 +423,17 @@ void component_master_del(struct device *dev,
> {
> struct master *master;
>
> - mutex_lock(&component_mutex);
> + mutex_lock(&master_mutex);
> master = __master_find(dev, ops);
> + if (master)
> + master->dead = true;
> + mutex_unlock(&master_mutex);
> +
> if (master) {
> take_down_master(master);
>
> - list_del(&master->node);
> - kfree(master);
> + kref_put_mutex(&master->kref, master_release, &master_mutex);
> }
> - mutex_unlock(&component_mutex);
> }
> EXPORT_SYMBOL_GPL(component_master_del);
>
> @@ -364,12 +451,9 @@ static void component_unbind(struct component *component,
>
> void component_unbind_all(struct device *master_dev, void *data)
> {
> - struct master *master;
> + struct master *master = master_find_locked(master_dev, NULL);
> struct component *c;
>
> - WARN_ON(!mutex_is_locked(&component_mutex));
> -
> - master = __master_find(master_dev, NULL);
> if (!master)
> return;
>
> @@ -432,13 +516,10 @@ static int component_bind(struct component *component, struct master *master,
>
> int component_bind_all(struct device *master_dev, void *data)
> {
> - struct master *master;
> + struct master *master = master_find_locked(master_dev, NULL);
> struct component *c;
> int ret = 0;
>
> - WARN_ON(!mutex_is_locked(&component_mutex));
> -
> - master = __master_find(master_dev, NULL);
> if (!master)
> return -EINVAL;
>
> @@ -474,14 +555,16 @@ int component_add(struct device *dev, const struct component_ops *ops)
>
> mutex_lock(&component_mutex);
> list_add_tail(&component->node, &component_list);
> + mutex_unlock(&component_mutex);
>
> ret = try_to_bring_up_masters(component);
> if (ret < 0) {
> + mutex_lock(&component_mutex);
> list_del(&component->node);
> + mutex_unlock(&component_mutex);
>
> kfree(component);
> }
> - mutex_unlock(&component_mutex);
>
> return ret < 0 ? ret : 0;
> }
> @@ -490,21 +573,28 @@ EXPORT_SYMBOL_GPL(component_add);
> void component_del(struct device *dev, const struct component_ops *ops)
> {
> struct component *c, *component = NULL;
> + struct master *master = NULL;
>
> mutex_lock(&component_mutex);
> list_for_each_entry(c, &component_list, node)
> if (c->dev == dev && c->ops == ops) {
> + master = c->master;
> + if (master)
> + kref_get(&master->kref);
> list_del(&c->node);
> component = c;
> break;
> }
> + mutex_unlock(&component_mutex);
>
> - if (component && component->master)
> - take_down_master(component->master);
> + if (WARN_ON(!component))
> + return;
>
> - mutex_unlock(&component_mutex);
> + if (master) {
> + take_down_master(master);
> + kref_put_mutex(&master->kref, master_release, &master_mutex);
> + }
>
> - WARN_ON(!component);
> kfree(component);
> }
> EXPORT_SYMBOL_GPL(component_del);
>
>
> --
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
