[PATCH v4 2/2] drm/panel: Add device_link from panel device to drm device

Wed May 23 08:29:10 UTC 2018

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 at ti.com>
>>>>>>>>>>> Reviewed-by: Eric Anholt <eric at 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.

Are not all dsi panels client on a bus, and therefore managed by a
dsi host? 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;
}