On 2018-05-22 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.
>
> 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.
While pursuing that idea, I found several examples of drivers that
deal with both component-parts not wanting the device-link and non-
component-parts wanting the device-link. So, a dead end.
Cheers,
Peter
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
