| Message ID | 20180927124130.9102-1-linus.walleij@linaro.org |
|---|---|
| State | Accepted |
| Commit | d6a77ba0eb92d8ffa4b05a442fc20d0a9b11c4c4 |
| Headers | show |
| Series | Revert "drm/panel: Add device_link from panel device to DRM device" | expand |
[actually Cc Eric and Andrzej] On Thu, Sep 27, 2018 at 02:41:30PM +0200, Linus Walleij wrote: > This reverts commit 0c08754b59da5557532d946599854e6df28edc22. > > commit 0c08754b59da > ("drm/panel: Add device_link from panel device to DRM device") > creates a circular dependency under these circumstances: > > 1. The panel depends on dsi-host because it is MIPI-DSI child > device. > 2. dsi-host depends on the drm parent device (connector->dev->dev) > this should be allowed. > 3. drm parent dev (connector->dev->dev) depends on the panel > after this patch. > > This makes the dependency circular and while it appears it > does not affect any in-tree drivers (they do not seem to have > dsi hosts depending on the same parent device) this does not > seem right. Hey Linus, I'm trying to wrap my head around this :-) I just read through the original patch thread. It doesn't seem like this was introduced to fix a bug? Will reverting this cause regressions on in-tree drivers? What's different in your in-development driver that's causing you to hit this? Thanks! Sean > > As noted in a response from Andrzej Hajda, the intent is > likely to make the panel dependent on the DRM device > (connector->dev) not its parent. But we have no way of > doing that since the DRM device doesn't contain any > struct device on its own (arguably it should). > > Revert this until a proper approach is figured out. > > Cc: Jyri Sarha <jsarha@ti.com> > Cc: Eric Anholt <eric@anholt.net> > Cc: Andrzej Hajda <a.hajda@samsung.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/gpu/drm/drm_panel.c | 10 ---------- > include/drm/drm_panel.h | 1 - > 2 files changed, 11 deletions(-) > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > index b902361dee6e..1d9a9d2fe0e0 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c > @@ -24,7 +24,6 @@ > #include <linux/err.h> > #include <linux/module.h> > > -#include <drm/drm_device.h> > #include <drm/drm_crtc.h> > #include <drm/drm_panel.h> > > @@ -105,13 +104,6 @@ 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; > > @@ -133,8 +125,6 @@ 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 582a0ec0aa70..777814755fa6 100644 > --- a/include/drm/drm_panel.h > +++ b/include/drm/drm_panel.h > @@ -89,7 +89,6 @@ struct drm_panel { > struct drm_device *drm; > struct drm_connector *connector; > struct device *dev; > - struct device_link *link; > > const struct drm_panel_funcs *funcs; > > -- > 2.17.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 27.09.2018 15:50, Sean Paul wrote: > [actually Cc Eric and Andrzej] > > On Thu, Sep 27, 2018 at 02:41:30PM +0200, Linus Walleij wrote: >> This reverts commit 0c08754b59da5557532d946599854e6df28edc22. >> >> commit 0c08754b59da >> ("drm/panel: Add device_link from panel device to DRM device") >> creates a circular dependency under these circumstances: >> >> 1. The panel depends on dsi-host because it is MIPI-DSI child >> device. >> 2. dsi-host depends on the drm parent device (connector->dev->dev) >> this should be allowed. >> 3. drm parent dev (connector->dev->dev) depends on the panel >> after this patch. >> >> This makes the dependency circular and while it appears it >> does not affect any in-tree drivers (they do not seem to have >> dsi hosts depending on the same parent device) this does not >> seem right. > Hey Linus, > I'm trying to wrap my head around this :-) > > I just read through the original patch thread. It doesn't seem like this was > introduced to fix a bug? Will reverting this cause regressions on in-tree > drivers? As I wrote in original patch thread, the original patch breaks platforms with exynos-dsi encoder. Quite detailed explanation in the original thread [1], 1st response to the patch. Apparently my response was ignored :) [1]: https://patchwork.freedesktop.org/patch/218878/ Regards Andrzej > > What's different in your in-development driver that's causing you to hit > this? > > Thanks! > > Sean > >> As noted in a response from Andrzej Hajda, the intent is >> likely to make the panel dependent on the DRM device >> (connector->dev) not its parent. But we have no way of >> doing that since the DRM device doesn't contain any >> struct device on its own (arguably it should). >> >> Revert this until a proper approach is figured out. >> >> Cc: Jyri Sarha <jsarha@ti.com> >> Cc: Eric Anholt <eric@anholt.net> >> Cc: Andrzej Hajda <a.hajda@samsung.com> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> >> --- >> drivers/gpu/drm/drm_panel.c | 10 ---------- >> include/drm/drm_panel.h | 1 - >> 2 files changed, 11 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c >> index b902361dee6e..1d9a9d2fe0e0 100644 >> --- a/drivers/gpu/drm/drm_panel.c >> +++ b/drivers/gpu/drm/drm_panel.c >> @@ -24,7 +24,6 @@ >> #include <linux/err.h> >> #include <linux/module.h> >> >> -#include <drm/drm_device.h> >> #include <drm/drm_crtc.h> >> #include <drm/drm_panel.h> >> >> @@ -105,13 +104,6 @@ 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; >> >> @@ -133,8 +125,6 @@ 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 582a0ec0aa70..777814755fa6 100644 >> --- a/include/drm/drm_panel.h >> +++ b/include/drm/drm_panel.h >> @@ -89,7 +89,6 @@ struct drm_panel { >> struct drm_device *drm; >> struct drm_connector *connector; >> struct device *dev; >> - struct device_link *link; >> >> const struct drm_panel_funcs *funcs; >> >> -- >> 2.17.1 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Sep 27, 2018 at 04:25:56PM +0200, Andrzej Hajda wrote: > On 27.09.2018 15:50, Sean Paul wrote: > > [actually Cc Eric and Andrzej] > > > > On Thu, Sep 27, 2018 at 02:41:30PM +0200, Linus Walleij wrote: > >> This reverts commit 0c08754b59da5557532d946599854e6df28edc22. > >> > >> commit 0c08754b59da > >> ("drm/panel: Add device_link from panel device to DRM device") > >> creates a circular dependency under these circumstances: > >> > >> 1. The panel depends on dsi-host because it is MIPI-DSI child > >> device. > >> 2. dsi-host depends on the drm parent device (connector->dev->dev) > >> this should be allowed. > >> 3. drm parent dev (connector->dev->dev) depends on the panel > >> after this patch. > >> > >> This makes the dependency circular and while it appears it > >> does not affect any in-tree drivers (they do not seem to have > >> dsi hosts depending on the same parent device) this does not > >> seem right. > > Hey Linus, > > I'm trying to wrap my head around this :-) > > > > I just read through the original patch thread. It doesn't seem like this was > > introduced to fix a bug? Will reverting this cause regressions on in-tree > > drivers? > > As I wrote in original patch thread, the original patch breaks platforms > with exynos-dsi encoder. > Quite detailed explanation in the original thread [1], 1st response to > the patch. > Apparently my response was ignored :) Thanks Andrzej, I've pushed this to drm-misc-fixes. It'll go out this week or next. Thanks, Sean > > [1]: https://patchwork.freedesktop.org/patch/218878/ > > Regards > Andrzej > > > > > What's different in your in-development driver that's causing you to hit > > this? > > > > Thanks! > > > > Sean > > > >> As noted in a response from Andrzej Hajda, the intent is > >> likely to make the panel dependent on the DRM device > >> (connector->dev) not its parent. But we have no way of > >> doing that since the DRM device doesn't contain any > >> struct device on its own (arguably it should). > >> > >> Revert this until a proper approach is figured out. > >> > >> Cc: Jyri Sarha <jsarha@ti.com> > >> Cc: Eric Anholt <eric@anholt.net> > >> Cc: Andrzej Hajda <a.hajda@samsung.com> > >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > >> --- > >> drivers/gpu/drm/drm_panel.c | 10 ---------- > >> include/drm/drm_panel.h | 1 - > >> 2 files changed, 11 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > >> index b902361dee6e..1d9a9d2fe0e0 100644 > >> --- a/drivers/gpu/drm/drm_panel.c > >> +++ b/drivers/gpu/drm/drm_panel.c > >> @@ -24,7 +24,6 @@ > >> #include <linux/err.h> > >> #include <linux/module.h> > >> > >> -#include <drm/drm_device.h> > >> #include <drm/drm_crtc.h> > >> #include <drm/drm_panel.h> > >> > >> @@ -105,13 +104,6 @@ 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; > >> > >> @@ -133,8 +125,6 @@ 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 582a0ec0aa70..777814755fa6 100644 > >> --- a/include/drm/drm_panel.h > >> +++ b/include/drm/drm_panel.h > >> @@ -89,7 +89,6 @@ struct drm_panel { > >> struct drm_device *drm; > >> struct drm_connector *connector; > >> struct device *dev; > >> - struct device_link *link; > >> > >> const struct drm_panel_funcs *funcs; > >> > >> -- > >> 2.17.1 > >> > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel > >
On Thu, Sep 27, 2018 at 3:50 PM Sean Paul <sean@poorly.run> wrote: > What's different in your in-development driver that's causing you to hit > this? I think Andrzej answered it well, but I couldn't see what I did different from the Exynos driver, so I was puzzled that it would still work, but as it turns out that one breaks too. As far as I can tell, the proper fix is more complex and require embedding a struct device into struct drm_device like most subsystems do and create a link to that device instead of parent (dev->dev), because the parent device may be the parent of several subdevices, not just the drm_device, but also (in my case at least) the DSI devices. A typical case would be if the DSI host is in the same address range as the display controller (then it is usually represented by the same device). On other platforms the parent device may just be parent for the drm_device but we can't just assume that. Yours, Linus Walleij
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index b902361dee6e..1d9a9d2fe0e0 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -24,7 +24,6 @@ #include <linux/err.h> #include <linux/module.h> -#include <drm/drm_device.h> #include <drm/drm_crtc.h> #include <drm/drm_panel.h> @@ -105,13 +104,6 @@ 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; @@ -133,8 +125,6 @@ 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 582a0ec0aa70..777814755fa6 100644 --- a/include/drm/drm_panel.h +++ b/include/drm/drm_panel.h @@ -89,7 +89,6 @@ struct drm_panel { struct drm_device *drm; struct drm_connector *connector; struct device *dev; - struct device_link *link; const struct drm_panel_funcs *funcs;
This reverts commit 0c08754b59da5557532d946599854e6df28edc22. commit 0c08754b59da ("drm/panel: Add device_link from panel device to DRM device") creates a circular dependency under these circumstances: 1. The panel depends on dsi-host because it is MIPI-DSI child device. 2. dsi-host depends on the drm parent device (connector->dev->dev) this should be allowed. 3. drm parent dev (connector->dev->dev) depends on the panel after this patch. This makes the dependency circular and while it appears it does not affect any in-tree drivers (they do not seem to have dsi hosts depending on the same parent device) this does not seem right. As noted in a response from Andrzej Hajda, the intent is likely to make the panel dependent on the DRM device (connector->dev) not its parent. But we have no way of doing that since the DRM device doesn't contain any struct device on its own (arguably it should). Revert this until a proper approach is figured out. Cc: Jyri Sarha <jsarha@ti.com> Cc: Eric Anholt <eric@anholt.net> Cc: Andrzej Hajda <a.hajda@samsung.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/gpu/drm/drm_panel.c | 10 ---------- include/drm/drm_panel.h | 1 - 2 files changed, 11 deletions(-)