On Fri, May 18, 2018 at 1:52 PM Andrzej Hajda <a.ha...@samsung.com> 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?
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@lists.freedesktop.org > 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel