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 <jsa...@ti.com>
>>>>>>>>>> Reviewed-by: Eric Anholt <e...@anholt.net>
>>>>>>>>> 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? 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.
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.
Cheers,
Peter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
