Armada DRM: bridge with componentized devices

[Rafael J. Wysocki]

On Friday, January 18, 2019 1:57:47 PM CET Daniel Vetter wrote:
> On Fri, Jan 18, 2019 at 12:38 PM Rafael J. Wysocki <rafael at kernel.org> wrote:
> >
> > On Fri, Jan 18, 2019 at 12:17 PM Rafael J. Wysocki <rafael at kernel.org> wrote:
> > >
> > > On Fri, Jan 18, 2019 at 12:06 PM Daniel Vetter <daniel at ffwll.ch> wrote:
> > > >
> > > > On Fri, Jan 18, 2019 at 11:03 AM Rafael J. Wysocki <rafael at kernel.org> wrote:
> > > > >
> > > > > On Fri, Jan 18, 2019 at 10:37 AM Lucas Stach <l.stach at pengutronix.de> wrote:
> > > > > >
> > > > > > Am Donnerstag, den 17.01.2019, 13:20 +0100 schrieb Daniel Vetter:
> > > > > > [...]
> > > > > > >
> > > > > > > > I don't think it is addressed here.
> > > > > > > >
> > > > > > > > Can anyone please explain to me what happens in more detail?
> > > > > > >
> > > > > > > My understanding (and this might be wrong, Russell, Andrzej, ... pls
> > > > > > > correct) is that the following sequence goes wrong:
> > > > > > >
> > > > > > > - componentized driver (both producer and consumer) fully loaded&working
> > > > > > > - you unbind the producer/one of the components
> > > > > > > - component framework or driver core through device_link also unbinds
> > > > > > > the master/consumer
> > > > > > > - you reload/rebind the component
> > > > > > > - with the component framework this results in the master being
> > > > > > > rebound, and the overall driver working again. With device_links
> > > > > > > nothing happens.
> > > > > > >
> > > > > > > I think there was a patch floating around once to put drivers unbound
> > > > > > > due to device_links onto the deferred probe list, so that the next
> > > > > > > time something is bound they have a chance to rebind. But I can't find
> > > > > > > that patch anymore, maybe someone else has the link still?
> > > > > >
> > > > > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1348023.html
> > > > > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1355243.html
> > > >
> > > > Thanks for digging it out, those are the patches I had in mind.
> > > >
> > > > > > I can repost if this is finally deemed a good idea.
> > > > >
> > > > > I don't think this will help.
> > > > >
> > > > > Two things appear to be needed here: missing suppliers need to be
> > > > > probed automatically at a consumer probe time and "persistent" links
> > > > > created by ->probe() callbacks should not be reference-counted when
> > > > > those callbacks run again.  I'm going to cut a patch to do that (early
> > > > > next week if all goes well).
> > > >
> > > > I thought this should work, since it makes an unbind of a consumer act
> > > > the same way as if that consumer's bind function has hit an
> > > > EPROBE_DEFER. Example:
> > > >
> > > > 1. consumer's bind is called, realizes (through a subsystem function
> > > > like of_drm_find_panel) that the producer isn't there yet, fails with
> > > > EPROBE DEFER.
> > > >
> > > > 2. driver core puts the consumer onto the deferred probe list.
> > > >
> > > > 3. producer gets loaded, registers the panel
> > > >
> > > > 4. driver core re-runs the consumer's bind function
> > > >
> > > > 5. consumer's bind function finds the produced and sets up the device link.
> > > >
> > > > Now for the unbind case:
> > > >
> > > > 1. producer is unbound, driver core unbinds consumer due to the device_link
> > > >
> > > > 2. (Only with Lucas' patch) driver core puts the consumer onto the
> > > > deferred probe list.
> > > >
> > > > 3. Developer (this really is useful for development) rebinds/reloads
> > > > producer driver, which re-registers the panel
> > > >
> > > > 4 & 5 work exactly the same as with the initial load sequence.
> > >
> > > I misunderstood the Russell's description, sorry.
> > >
> > > My understanding was that the consumer driver would be rebound at this
> > > point and that the (missing) producer was expected to rebind as well
> > > in response.
> > >
> > > > With this initial loading and reloading would be very similar, and I
> > > > think that's a good thing. It also solves the device_link
> > > > lifetime/refcounting issue, since the device_link gets torn down
> > > > in/restored completely, and reloading isn't a special case like in
> > > > your proposed solution - there's no device_link left over from a
> > > > previous loading of the driver, the dependency is only handled by
> > > > putting the consumer (back)  onto the deferred probe list.
> > >
> > > My idea was based on incorrect understanding of the problem, so scratch it. :-)
> > >
> > > The Lukas' patch would indeed work here, but I'm not sure if doing
> > > that for all links unconditionally is a good idea.
> > >
> > > I'd rather have a link flag to indicate that this behavior is desirable.
> >
> > That said, the creation of a device link is a heavy-weight operation
> > in general as it triggers a list reordering that may turn out to be
> > recursive etc., so I'd rather avoid doing too much of that.
> >
> > Moreover, creating a link between two devices once should be
> > sufficient, so I'd prefer "persistent" links to be used in that case,
> > but they need to be fixed to avoid the extra reference counting.  That
> > and a new link flag to indicate that the consumer should be probed
> > automatically after binding the supplier driver.
> 
> Yeah if persistent links make more sense from an implementation pov
> then that's all fine, as long as it results in the same behaviour for
> from the pov of all the involved bind functions. So if the core
> handles all the refcount trickery, that's good.
> 
> Also an explicit flag sounds like a good idea, since for some other
> problems we want to make the device_link opt-in. There's otherwise
> some loops in some cases apparently.

OK, I think I know how to make device links support this use case, but
that will require some rework of the framework, mostly consisting of
fixes and cleanups.

I'll send the first part of it in a minute.

Cheers,
Rafael