On 18/05/18 14: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?
>
Sounds good to me. Unfortunately I have no time to spend on this in the
coming two weeks. If you can come up with a fix, please send patch and
put me in cc.
I am also fine with reverting the patch.
> 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.
> 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.
>
That sounds like an ultimate solution, especially if there is a default
behaviour that unbinds the master drm device as the most devices do not
need anything else.
Best regards,
Jyri
> 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;
>>
>
>
--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
