On 23.05.2018 10:29, Peter Rosin wrote:
> On 2018-05-22 17:03, Peter Rosin wrote:
>> On 2018-05-22 11:45, Andrzej Hajda wrote:
>>> On 22.05.2018 09:36, Peter Rosin wrote:
>>>> On 2018-05-22 08:29, Andrzej Hajda wrote:
>>>>> On 21.05.2018 23:56, Peter Rosin wrote:
>>>>>> On 2018-05-21 11:21, Andrzej Hajda wrote:
>>>>>>> On 21.05.2018 10:53, Peter Rosin wrote:
>>>>>>>> On 2018-05-21 10:15, Andrzej Hajda wrote:
>>>>>>>>> On 19.05.2018 18:48, Peter Rosin wrote:
>>>>>>>>>> On 2018-05-18 13:51, Andrzej Hajda wrote:
>>>>>>>>>>> On 26.04.2018 10:07, Jyri Sarha wrote:
>>>>>>>>>>>> Add device_link from panel device (supplier) to drm device (consumer)
>>>>>>>>>>>> when drm_panel_attach() is called. This patch should protect the
>>>>>>>>>>>> master drm driver if an attached panel driver unbinds while it is in
>>>>>>>>>>>> use. The device_link should make sure the drm device is unbound before
>>>>>>>>>>>> the panel driver becomes unavailable.
>>>>>>>>>>>>
>>>>>>>>>>>> The device_link is removed when drm_panel_detach() is called. The
>>>>>>>>>>>> drm_panel_detach() should be called by the consumer DRM driver, not the
>>>>>>>>>>>> panel driver, otherwise both drivers are racing to delete the same link.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Jyri Sarha <jsarha@xxxxxx>
>>>>>>>>>>>> Reviewed-by: Eric Anholt <eric@xxxxxxxxxx>
>>>>>>>>>>> Hi Jyri,
>>>>>>>>>>>
>>>>>>>>>>> This patch breaks few platforms: tm2, tm2e, trats2 - ie all platforms
>>>>>>>>>>> using exynos_drm_dsi and dsi panels.
>>>>>>>>>>> Exynos-DSI properly handles panels unbind - ie references to panel are
>>>>>>>>>>> properly removed on panels removal and respective drm_connector enters
>>>>>>>>>>> disconnected state, without destroying whole drm device.
>>>>>>>>>>> And again on panel driver re-bind drm_panel is properly attached to
>>>>>>>>>>> exynos-dsi and panel pipeline is working again.
>>>>>>>>>>> This patch will break this behavior, ie it will destroy whole drm device.
>>>>>>>>>>>
>>>>>>>>>>> Making device_links for panels optional seems to me the best solution,
>>>>>>>>>>> what do you think?
>>>>>>>>>>>
>>>>>>>>>>> The funny thing is that due to bug in device link code, your patch has
>>>>>>>>>>> no effect on these platforms. So it does not hurt these platform yet,
>>>>>>>>>>> but the bug will be fixed sooner or later.
>>>>>>>>>> Ah, that's a pretty strong hint that we are doing something unusual. So,
>>>>>>>>>> I had a deeper look and I think that device-links (with state, i.e. not
>>>>>>>>>> DL_FLAG_STATELESS links) are assumed to be created by the .probe function
>>>>>>>>>> of either the consumer or the supplier. Then it seems to works as expected.
>>>>>>>>>> And that makes some sense too, because a link indicates that there exist
>>>>>>>>>> a dependency between the two and that the consumer cannot really exist
>>>>>>>>>> without the supplier. This is also what happens for the drm devices
>>>>>>>>>> (panel/bridge consumers) Jyri and I are working with; they call panel or
>>>>>>>>>> bridge attach during their probe. But this is not the case for exynos,
>>>>>>>>>> which calls panel/bridge attach at some later stage, and that obviously
>>>>>>>>>> violates the assumption that the device-link consumer cannot exist w/o
>>>>>>>>>> the supplier ("obviously" since the drm driver has managed to successfully
>>>>>>>>>> probe without the supplier).
>>>>>>>>>>
>>>>>>>>>> So, when the panel/bridge supplier is probed after the consumer is
>>>>>>>>>> bound, the link starts out as DL_STATE_DORMANT, and progresses to
>>>>>>>>>> DL_STATE_AVAILABLE once the panel/bridge has finished the probe. This is
>>>>>>>>>> not what *we* want.
>>>>>>>>> And this is also incorrect from Documentation PoV:
>>>>>>>>> * @DL_STATE_DORMANT: None of the supplier/consumer drivers is present.
>>>>>>>>> * @DL_STATE_AVAILABLE: The supplier driver is present, but the consumer
>>>>>>>>> is not.
>>>>>>>>>
>>>>>>>>>> So, one idea I have is to, on panel/bridge attach, verify if the
>>>>>>>>>> consumer is in its probe, and only create the link if that is the
>>>>>>>>>> case. So, the link would be optional, but it would all be automatic.
>>>>>>>>> Making it automatic looks tempting, but also error prone. In case of
>>>>>>>>> component framework bind callbacks can be called from probe of any
>>>>>>>>> component, so sometimes it can be called also from exynos-dsi probe,
>>>>>>>>> sometimes not (depending on order of probing, which we cannot rely on).
>>>>>>>>> So in some cases we will end-up with links, sometimes without. Ie
>>>>>>>>> following scenarios are possible in drm_panel_attach:
>>>>>>>>> 1. exynos-dsi bound, panel during probe.
>>>>>>>>> 2. exynos-dsi during probe, panel during probe.
>>>>>>>> 2. exynos-dsi during probe, panel bound? Or is this case 3, and 2 happens
>>>>>>>> when drivers probe in parallel?
>>>>>>> Panel is always probed not earlier than the end of exynos-dsi bind, so
>>>>>>> only scenarios 1 and 2 should be possible.
>>>>>>>
>>>>>>>> Whichever, you are right, I naively thought that the bind happened
>>>>>>>> after the probe of all devices, but naturally it happens as part of
>>>>>>>> the last device to register its component, and that normally happens
>>>>>>>> during its probe.
>>>>>>>>
>>>>>>>> Sigh. So, scratch that, I guess we need a flag...
>>>>>> I looked into that, and it seems very fragile to get the flag to be
>>>>>> correct for all cases. That road is bound to produce lots of bugs, and
>>>>>> it will be hard to get it right. In short, I would not descend into that
>>>>>> particular rabbit hole.
>>>>>>
>>>>>> Can it be assumed that the drm_device of the encoder in drm_bridge_attach
>>>>>> is a master component, if the drm "cluster" is componentized?
>>>>> I wouldn't call it assumption, I would say rather it is a common practice.
>>>>>
>>>>>> Are there
>>>>>> currently other ways of handling drm driver binding changes than
>>>>>> components?
>>>>> No, there are drivers which do not use components, but call
>>>>> drm_panel_attach:
>>>>> $ for d in drivers/gpu/drm/*/; do git grep -q 'struct component_ops' $d
>>>>> && continue; git grep -q drm_panel_attach $d && echo $d; done
>>>>> drivers/gpu/drm/bridge/
>>>>> drivers/gpu/drm/fsl-dcu/
>>>>> drivers/gpu/drm/mxsfb/
>>>>> drivers/gpu/drm/rcar-du/
>>>>> drivers/gpu/drm/tegra/
>>>>>
>>>>> Components are optional.
>>>> Yes, of course components are optional. The question was if there are
>>>> currently *other* ways (i.e. not the component framework, not device links)
>>>> of dealing with disappearing panels/bridges. However, see below, the
>>>> question is irrelevant with my below suggestion.
>>>>
>>>>>> If the answers are "yes" and "no", it might be possible to check if
>>>>>> encoder->dev is a master component and only establish the device link if
>>>>>> it isn't. All it would take is to add a component_device_is_master()
>>>>>> query function to drivers/base/component.c
>>>>>> Something like this (untested):
>>>>>>
>>>>>> bool component_device_is_master(struct device *dev)
>>>>>> {
>>>>>> struct master *m;
>>>>>>
>>>>>> mutex_lock(&component_mutex);
>>>>>> m = __master_find(dev, NULL);
>>>>>> mutex_unlock(&component_mutex);
>>>>>>
>>>>>> return !!m;
>>>>>> }
>>>>>> EXPORT_SYMBOL_GPL(component_device_is_master);
>>>>>>
>>>>>> And then check that before calling device_link_add().
>>>>> Why do not use simpler solution, just create function
>>>>> drm_panel_attach_without_link and call it explicitly from drivers, or
>>>>> add additional flag argument to either drm_panel_attach either to
>>>>> drm_panel structure? Maybe it will not be prettier but will be more
>>>>> explicit.
>>>> Because, if you take bridges into account as well and add a
>>>> drm_bridge_attach_without_link, which function do you call from e.g.
>>>> drm_simple_display_pipe_attach_bridge()? Sure, you can probably verify
>>>> the callers and come to the conclusion that you maybe always want the
>>>> link, currently. Same question for the tda998x driver, which function
>>>> to use for attaching the bridge would have to depend on how the driver
>>>> was bound (as a component or not, yes I know, currently tda998x only
>>>> does component, but the whole reason I'm involved is that I want it
>>>> to also register as a standalone drm_bridge). Not doing this with some
>>>> automatic check simply leads to combinatorial hell.
>>> I was focused on panels, which are managed not by drm core, but by
>>> upstream pipeline element (bridge/encoder). For them decision about
>>> using device links should be made by the manager not drm core, I guess.
>>> In case of bridges it is different, bridges are attached by upstream
>>> elements, but then they are tracked/managed/detached only by drm core
>>> (at least this is current practice). If somebody wants to implement
>>> dynamic bridges this pattern cannot be used, ie bridge should be
>>> attached/detached by upstream element, like in case of panels.
>>>
>>>> Maybe a better solution is for the drm driver to record whether it
>>>> wants links in its struct drm_device, before calling drm_dev_register?
>>>> That's also explicit. drm_panel_attach/drm_bridge_attach could then
>>>> easily determine if the link should be made. IMHO, it would also be
>>>> quite easy to set this correctly for any given drm_device.
>>> As I said earlier I think decision about link creation should be always
>>> up to element which performs attachment, It only knows what should be
>>> attached, so it is the best candidate to handle dynamic unbind and
>>> re-bind of the downstream element.
>> But does the attacher in fact know *what* should be attached? And
>> how? Take e.g. the drm_panel_attach in analogix_dp_bridge_attach, in
>> analogix_dp_core.c. Should that attach be with or without a
>> device-link?
analogix_dp_core.c is a 'library' used by exynos_dp and rockchip_dp,
neither of these drivers perform dynamic panel management so answer is
simple: device_link will protect them from panel unbind.
>> That function has no knowledge whatsoever about the
>> requirements for dp->plat_data->panel. That panel could e.g. come
>> from rockchip_dp_probe, in analogix_dp-rockchip.c, which can be
>> further traced back to drm_of_find_panel_or_bridge. But what kind of
>> requirements do that panel have? It might be a dsi panel, in which
>> case we had better not create a device-link, as you have shown
>> previously in the thread. Or it might be some little trivial panel,
>> like panel-lg-lg4573.c, in which case we *really* want the device-
>> link so that the panel doesn't disappear on us.
In general it is not a matter of panel, but of panel's consumer, ie
upstream pipe element, for example dsi host.
>>
>> Maybe it's easy to see this, if you know the ins and outs of the
>> code. But I don't see it. And I don't see how this path leads to
>> maintainable code. I still think the link-or-no-link decision needs
>> to be in a central place.
> Are not all dsi panels client on a bus, and therefore managed by a
> dsi host?
Panels are managed by dsi host only if dsi host implements it, and it is
up to dsi host author, it is not mandatory. The only case I am aware of
is exynos-dsi. I guess most developers are not aware of the problem, or
they do not care, or ... whatever. This patch shows it very clearly.
> And are not all dsi panels attached to a dsi connector?
>
> How about this:
>
> int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
> {
> int ret;
>
> if (panel->connector)
> return -EBUSY;
>
> if (connector->connector_type != DRM_MODE_CONNECTOR_DSI) {
> panel->link = device_link_add(connector->dev->dev,
> panel->dev, 0);
> if (!panel->link) {
> dev_err(panel->dev, "failed to link panel to %s\n",
> dev_name(connector->dev->dev));
> return -EINVAL;
> }
> }
>
> panel->connector = connector;
> panel->drm = connector->dev;
>
> return 0;
> }
With such approach you do not protect dsi hosts without dynamic panel
management support. As I said earlier: it is matter of panel consumers
(for example dsi host), ie if it implements dynamic panel management, it
should attach panel without creating device_links, as it take care of
disappearing panels. If it does not perform dynamic management it should
rely on default behavior - links should be created, to protect drm
device from using dangling pointers.
Regards
Andrzej
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
