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.
>
> 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.
Regards
Andrzej
>
> Cheers,
> Peter
>
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
