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.
>
> 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?
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
