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

[Andrzej Hajda <a.hajda@samsung.com>]

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@ti.com>
>>>>> Reviewed-by: Eric Anholt <eric@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...
>
>>> The function device_is_bound seems like a good candidate for that
>>> check?
>>>
>>> Maybe device_link_add should automatically create a DL_FLAG_STATELESS link
>>> if either consumer or supplier has "proven" (with a successful probe) that
>>> they can exist without the other end? That would also work for us, since
>>> a DL_FLAG_STATELESS link does no harm in our case...
>> What will be benefit of having stateless device_links, in case of
>> panels/bridges?
> To have device suspend/resume happen in a predictable order? But maybe that
> has no value whatsoever?

At the moment panels do not have linux pm code, so I guess their pm is
done entirely by drm framework.
I am not sure about bridges.

Regards
Andrzej

>
> Cheers,
> Peter
>
>> Regards
>> Andrzej
>>
>>
>>> Cheers,
>>> Peter
>>>
>>>> The trick in case of exynos-dsi is to use mipi_dsi attach/detach
>>>> callbacks to get notifications about drm_panel's
>>>> appearance/disappearance, I guess ultimately such callbacks should be a
>>>> part of drm_panel
>>>> framework.
>>>>
>>>> Below description how all the code works:
>>>> Initialization:
>>>> 1. exynos_dsi (component of exynos_drm) at the end of bind callback
>>>> registers mipi_dsi bus.
>>>> 2. during mipi_dsi bus registration or later (if panel driver was not
>>>> yet registered), panel driver is bound ie - its probe callback is called.
>>>> 3. during panel's probe drm_panel_add is called, and later mipi_dsi_attach.
>>>> 4. mipi_dsi_attach calls dsi .attach callback of exynos_dsi.
>>>> 5. exynos_dsi.attach callback looks for drm_panel, calls
>>>> drm_panel_attach and changes connector's status to connected.
>>>> ....
>>>> Panel's unbind:
>>>> 1. device_release_driver calls panel's remove callback.
>>>> 2. panel's remove callback calls mipi_dsi_detach.
>>>> 3. mipi_dsi_detach calls exynos_dsi.detach callback.
>>>> 4. exynos_dsi.detach calls drm_panel_detach and changes connector's
>>>> state to disconnected.
>>>>
>>>> Panel's rebind is equivalent of steps 2-4 of Initialization.
>>>>
>>>> I have wrote the code few years ago, and it seemed to me the only safe
>>>> way to handle dynamic panel bind/unbind.
>>>> For example please note that step 5 of initialization and steps 2-4 of
>>>> unbind are executed always under panel's device_lock so it makes it
>>>> safe. Otherwise you risk that between panel lookup and panel attach
>>>> panel's driver can be unbind and consumer is left with dangling panel's
>>>> pointer, so even with your patch most of the drivers are still buggy.
>>>>
>>>> And few words about the bug(?) in device_link code: If you call
>>>> device_link_add from probe of the supplier(as it is in this case) its
>>>> status is set to DL_STATE_DORMANT, and after successful bind it is
>>>> changed to DL_STATE_AVAILABLE, which is incorrect (it should be
>>>> DL_STATE_ACTIVE), as a result on supplier's unbind consumer is not unbound.
>>>>
>>>> Regards
>>>> Andrzej
>>>>
>>>>> ---
>>>>>  drivers/gpu/drm/drm_panel.c | 10 ++++++++++
>>>>>  include/drm/drm_panel.h     |  1 +
>>>>>  2 files changed, 11 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
>>>>> index 71e4075..965530a 100644
>>>>> --- a/drivers/gpu/drm/drm_panel.c
>>>>> +++ b/drivers/gpu/drm/drm_panel.c
>>>>> @@ -24,6 +24,7 @@
>>>>>  #include <linux/err.h>
>>>>>  #include <linux/module.h>
>>>>>  
>>>>> +#include <drm/drm_device.h>
>>>>>  #include <drm/drm_crtc.h>
>>>>>  #include <drm/drm_panel.h>
>>>>>  
>>>>> @@ -104,6 +105,13 @@ int drm_panel_attach(struct drm_panel *panel, struct drm_connector *connector)
>>>>>  	if (panel->connector)
>>>>>  		return -EBUSY;
>>>>>  
>>>>> +	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;
>>>>>  
>>>>> @@ -125,6 +133,8 @@ EXPORT_SYMBOL(drm_panel_attach);
>>>>>   */
>>>>>  int drm_panel_detach(struct drm_panel *panel)
>>>>>  {
>>>>> +	device_link_del(panel->link);
>>>>> +
>>>>>  	panel->connector = NULL;
>>>>>  	panel->drm = NULL;
>>>>>  
>>>>> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
>>>>> index 14ac240..26a1b5f 100644
>>>>> --- a/include/drm/drm_panel.h
>>>>> +++ b/include/drm/drm_panel.h
>>>>> @@ -89,6 +89,7 @@ struct drm_panel {
>>>>>  	struct drm_device *drm;
>>>>>  	struct drm_connector *connector;
>>>>>  	struct device *dev;
>>>>> +	struct device_link *link;
>>>>>  
>>>>>  	const struct drm_panel_funcs *funcs;
>>>>>  
>>>
>>>
>
>
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel