On Fri, May 18, 2018 at 1:52 PM Andrzej Hajda <a.hajda@xxxxxxxxxxx> 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?
I think all the device_link bugs have been fixed by now (lots of
discussion in some other thread around using them for bridges). Time
to resurrect/retest this patch and land it?
I still like the idea very much of making panel/bridges Just Work.
Cheers, Daniel
>
> 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.
>
> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
