On 2018-05-19 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.
>
> 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.
>
> 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
s/either consumer or supplier/the consumer/
The supplier should always be able to probe w/o the consumer being present.
> 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...
>
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
