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