On Fri, Jan 11, 2019 at 3:49 PM Russell King - ARM Linux <linux@xxxxxxxxxxxxxxx> wrote: > > On Fri, Jan 11, 2019 at 03:36:28PM +0100, Rafael J. Wysocki wrote: > > On Fri, Jan 11, 2019 at 3:32 PM Russell King - ARM Linux > > <linux@xxxxxxxxxxxxxxx> wrote: > > > > > > On Fri, Jan 11, 2019 at 03:26:44PM +0100, Rafael J. Wysocki wrote: > > > > On Fri, Jan 11, 2019 at 3:20 PM Daniel Vetter <daniel@xxxxxxxx> wrote: > > > > > > > > > > On Wed, Jan 9, 2019 at 10:31 AM Russell King - ARM Linux > > > > > <linux@xxxxxxxxxxxxxxx> wrote: > > > > > > On Wed, Jan 09, 2019 at 10:24:01AM +0100, Rafael J. Wysocki wrote: > > > > > > > On Wed, Jan 9, 2019 at 10:13 AM Andrzej Hajda <a.hajda@xxxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > +CC: Rafael J. Wysocki <rafael@xxxxxxxxxx> > > > > > > > > > > > > > > > > On 08.01.2019 16:07, Russell King - ARM Linux wrote: > > > > > > > > > On Tue, Jan 08, 2019 at 03:33:54PM +0100, Andrzej Hajda wrote: > > > > > > > > >> On 08.01.2019 14:21, Russell King - ARM Linux wrote: > > > > > > > > >>> On Tue, Jan 08, 2019 at 01:27:56PM +0100, Andrzej Hajda wrote: > > > > > > > > >>>> On 08.01.2019 12:38, Russell King - ARM Linux wrote: > > > > > > > > >>>>> On Tue, Jan 08, 2019 at 12:25:34PM +0100, Andrzej Hajda wrote: > > > > > > > > >>>>>> Issues with device links have nothing to do with hotplugging, they are > > > > > > > > >>>>>> generic - lifetime of the objects (drm_bridge, drm_panel) is just > > > > > > > > >>>>>> slightly different of lifetime of device links, and this is racy even if > > > > > > > > >>>>>> you do not want hotplugging. Moreover since drm_dev is not device (has > > > > > > > > >>>>>> no associated struct device) assuming we can reuse its parent to create > > > > > > > > >>>>>> device link results in circular dependencies. > > > > > > > > >>>>> How about having the device links created depending on whether the > > > > > > > > >>>>> main drm driver wants them or not - that would mean that Exynos > > > > > > > > >>>>> could continue avoiding them, but others that want them can have > > > > > > > > >>>>> the links? > > > > > > > > >>>> That should be OK for Exynos. But regardless of Exynos device_links at > > > > > > > > >>>> the current state will not work correctly with bridges/panels as I > > > > > > > > >>>> described earlier. > > > > > > > > >>> However, I'm not sure you're correct with your interpretation of the > > > > > > > > >>> documentation. Firstly, the documentation says: > > > > > > > > >>> > > > > > > > > >>> Another example for an inconsistent state would be a device link that > > > > > > > > >>> represents a driver presence dependency, yet is added from the consumer's > > > > > > > > >>> ->probe callback while the supplier hasn't probed yet: Had the driver core > > > > > > > > >>> known about the device link earlier, it wouldn't have probed the consumer > > > > > > > > >>> in the first place. The onus is thus on the consumer to check presence of > > > > > > > > >>> the supplier after adding the link, and defer probing on non-presence. > > > > > > > > >>> > > > > > > > > >>> This is fine - if we add the device link from of_drm_find_bridge(), we > > > > > > > > >>> will be in the consumer's ->probe function, and the supplier must have > > > > > > > > >>> been probed for us to find the struct drm_bridge. > > > > > > > > >> > > > > > > > > >> Supplier usually is registered in it's probe time, so there is short > > > > > > > > >> period of time when supplier is available, but the probe is not yet > > > > > > > > >> finished - quite unsafe, but not impossible, especially if there exists > > > > > > > > >> some kind of notifications about resource appearance (MIPI-DSI case). > > > > > > > > > At some point during the supplier probe, the resource becomes available > > > > > > > > > to consumers. At that point, device links can be setup before the > > > > > > > > > supplier has finished probing. So any driver that provides resources > > > > > > > > > to another driver will, at some point during the provider's probe, > > > > > > > > > make resources available, and therefore be a candidate for device links > > > > > > > > > to be created _before_ the probe function has returned. > > > > > > > > > > > > > > > > > > What is a problem is if the provider publishes resources, and then fails > > > > > > > > > its probe function, causing the resource to disappear. > > > > > > > > > > > > > > > > > > > > > > > > But creating link to not-probed provider is still incorrect usage from > > > > > > > > device_links framework PoV, and my tests showed indeed that device link > > > > > > > > created before end of provider's probe does not work at all - and since > > > > > > > > it is stated in the documentation I guess it is by design. > > > > > > > > > > > > > > Yes, it is. > > > > > > > > > > > > So is the regulator support and the use of it being proposed for the CCF > > > > > > all going against the design of device links? In both those cases, > > > > > > device links _can_ be created while the supplier is still in the probe > > > > > > function by a consumer finding the resource. > > > > > > > > > > > > This seems fragile by design. > > > > > > > > > > Rafael, can you confirm? > > > > > > > > Let me quote from > > > > https://www.kernel.org/doc/html/latest/driver-api/device_link.html : > > > > > > > > "When driver presence on the supplier is irrelevant and only correct > > > > suspend/resume and shutdown ordering is needed, the device link may > > > > simply be set up with the DL_FLAG_STATELESS flag. In other words, > > > > enforcing driver presence on the supplier is optional." > > > > > > > > Which is exactly the case at hand here AFAICS. > > > > > > That is not what we're discussing. We are discussing using device > > > links to solve the problem with DRM bridges which may be removed > > > today from DRM without the rest of the DRM system being aware. > > > > Still, I think, the DL_FLAG_STATELESS flag should work for you as it > > turns the state tracking off entirely. > > Let me try again. > > This thread is discussing how to deal with Armada DRM, its use of the > component helper, TDA998x's hybrid use of the component helper and > DRM bridges. > > Currently, DRM bridges register into the DRM system and are added to > a global list of bridges. When a DRM driver binds, it looks up the > DRM bridge and attaches to it. There is no way in generic DRM code > for the DRM driver to know if the DRM bridge is unbound from DRM, > consequently a DRM driver may continue trying to call functions in > the DRM bridge driver using memory that has already been freed. > > We had similar issues with imx-drm, and a number of years ago at a > kernel summit, this was discussed, and I proposed a system which is > now known as the component helper. This handles the problem of a > multi-component system. > > However, DRM bridge was already established, and there appears to be > no desire to convert DRM bridges and DRM drivers to use the component > helper. > > We are presently in the situation where Armada DRM is incompatible > with the DRM bridges way of doing things, and making it compatible > essentially means introducing potential use-after-free bugs into the > code. > > Device links in their stateful form appear to provide an alternative > to the component helper, in that a stateful device link will remove > consumers of a resource when the supplier is going away - which is > exactly the problem which the component helper is solving. The > difference is that device links look like being a cleaner solution. > > Just like the component helper, a stateful link would unbind the > consumer of a resource when the supplier goes away - which is exactly > the behaviour we're wanting. > > The problem is that the connection between various drivers is only > really known when the drivers obtain their resources, and the > following can happen: > > supplier consumer > > probe() > alloc > probe() > publish > obtain supplier's resource > return > > Where things go wrong is if a stateful link is created when the > consumer obtains the suppliers resource before the supplier has > finished probing - which from what's been said is illegal. It just doesn't work (which means "invalid" rather than "illegal" I guess, but whatever :-)). Admittedly, the original design didn't take this particular case into account and I'm not actually sure how it can be taken into account either. If the link is created by the consumer in the scenario above, its status will be updated twice in a row after the consumer probe returns and after the supplier probe returns. It looks like this update would need to work regardless of the order it which this happened which sounds somewhat challenging. I would need to think about that. Moreover, there seems to be is an additional complication here, which is that the supplier probe may finish and the status update for its links may run before the extra link is actually created by the consumer probe. Or is there any reason why that cannot happen in this particular case? > It seems that stateful device links can only be created by bus layer > code, which limits their usefulness - in the case we have here, DT > doesn't always know ahead of time about these links. That's fair enough. > It sounds from what you're saying that you don't want to entertain > any changes to device links Why not? If they can be made work for this use case, they would become more useful in general I suppose, but I'm just not sure how to do that ATM. > - in which case, DRM can't use them to > solve this problem, even though they would be an elegant solution. > So, I think we're going to have to find a way to retrofit component > stuff into DRM in a way that makes it optional for any DRM bridge > consumer. > > I hope this makes the discussion and what we're trying to achieve > a little clearer. Yes, it does, thank you! Cheers, Rafael _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
