| Message ID | 1519165310-28713-1-git-send-email-jsarha@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Feb 21, 2018 at 12:21:50AM +0200, Jyri Sarha wrote: > Currently the master drm driver is not protected against the attached > panel driver from becoming unavailable. Adding a device_link with > DL_FLAG_AUTOREMOVE flag unbinds the master drm device (the consumer) > when the panel device (the supplier) becomes unavailable. > > Signed-off-by: Jyri Sarha <jsarha@ti.com> > Suggested-by: Thierry Reding <thierry.reding@gmail.com> > cc: eric@anholt.net > cc: laurent.pinchart@ideasonboard.com > --- > It still annoys me that the unbound master drm device does not probe > again if the panel device becomes available again. If there is no > remedy to this, may be we should consider applying the module get/put > patch[1] too. > > Hmmm... there was an obvious reasons not to add module gets and puts > to drm_panel_attach/detach(), but is there any such reasons for not to > add and remove the device link there? Yeah that might be the better place for this. At least conceptually it's when we're creating the dependency link. > The bridge side look more complicated as there is no public > drm_bridge_detach(), and I am not sure if the drm_bridge_remove() should > remove the device link. I guess it should, if the link is there. drm_bridge_remove is meant to be called when unloading the driver, which is too late. We probably need a drm_bridge_detach (which the master drm_device driver would need to call). Since bridges are linked through drm_encoder/bridge already, the drm core could make that call (would avoid having to audit all the drivers I think). But imo we can postpone fixing the bridge side of things to a later time. > [1] https://lists.freedesktop.org/archives/dri-devel/2018-February/166350.html Afaiui the device_link will make sure that the main driver gets unbound before the panel driver. Since a module unload implies a driver unbind I think we're safe with this, and don't need any additional protection. Wrt reprobing the main drm_driver when you rebind/reload the panel driver: I guess poke it through the sysfs reprobe thing, or just reload the main driver too, that should work. It's still driver reloading we're talking about, as long as you can't make the kernel go boom I think it's all perfectly fine. -Daniel > > drivers/gpu/drm/bridge/panel.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > index 6d99d4a..d71ddd8 100644 > --- a/drivers/gpu/drm/bridge/panel.c > +++ b/drivers/gpu/drm/bridge/panel.c > @@ -22,6 +22,7 @@ struct panel_bridge { > struct drm_connector connector; > struct drm_panel *panel; > u32 connector_type; > + struct device_link *link; /* link to master drm dev */ > }; > > static inline struct panel_bridge * > @@ -57,6 +58,22 @@ static const struct drm_connector_funcs panel_bridge_connector_funcs = { > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > }; > > +static int panel_bridge_link_to_master(struct panel_bridge *panel_bridge) > +{ > + struct device *mdev = panel_bridge->bridge.dev->dev; > + struct device *pdev = panel_bridge->panel->dev; > + u32 flags = DL_FLAG_AUTOREMOVE; > + > + panel_bridge->link = device_link_add(mdev, pdev, flags); > + if (!panel_bridge->link) { > + dev_err(pdev, "failed to link panel %s to %s\n", > + dev_name(pdev), dev_name(mdev)); > + return -EINVAL; > + } > + > + return 0; > +} > + > static int panel_bridge_attach(struct drm_bridge *bridge) > { > struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > @@ -82,6 +99,9 @@ static int panel_bridge_attach(struct drm_bridge *bridge) > drm_mode_connector_attach_encoder(&panel_bridge->connector, > bridge->encoder); > > + if (panel_bridge_link_to_master(panel_bridge)) > + return -EINVAL; > + > ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector); > if (ret < 0) > return ret; > @@ -94,6 +114,8 @@ static void panel_bridge_detach(struct drm_bridge *bridge) > struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > > drm_panel_detach(panel_bridge->panel); > + > + device_link_del(panel_bridge->link); > } > > static void panel_bridge_pre_enable(struct drm_bridge *bridge) > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki >
On Wed, Feb 21, 2018 at 12:21:50AM +0200, Jyri Sarha wrote: > @@ -94,6 +114,8 @@ static void panel_bridge_detach(struct drm_bridge *bridge) > struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > > drm_panel_detach(panel_bridge->panel); > + > + device_link_del(panel_bridge->link); No, you've set the DL_FLAG_AUTOREMOVE flag, so you'll end up removing the link twice, which oopses. It's either DL_FLAG_AUTOREMOVE or device_link_del(), not both. > +static int panel_bridge_link_to_master(struct panel_bridge *panel_bridge) > +{ > + struct device *mdev = panel_bridge->bridge.dev->dev; > + struct device *pdev = panel_bridge->panel->dev; > + u32 flags = DL_FLAG_AUTOREMOVE; > + > + panel_bridge->link = device_link_add(mdev, pdev, flags); > + if (!panel_bridge->link) { > + dev_err(pdev, "failed to link panel %s to %s\n", > + dev_name(pdev), dev_name(mdev)); You're printing two instances of pdev's name in the log message, one should be sufficient. Also, you've mixed up the order: mdev is the consumer, pdev the supplier. (Bikeshed: The DL_FLAG_AUTOREMOVE would still fit within 80 chars on the line with device_link_add() and the flags variable wouldn't have to be declared then. Your call.) Thanks, Lukas
On 21/02/18 01:47, Daniel Vetter wrote: > On Wed, Feb 21, 2018 at 12:21:50AM +0200, Jyri Sarha wrote: >> Currently the master drm driver is not protected against the attached >> panel driver from becoming unavailable. Adding a device_link with >> DL_FLAG_AUTOREMOVE flag unbinds the master drm device (the consumer) >> when the panel device (the supplier) becomes unavailable. >> >> Signed-off-by: Jyri Sarha <jsarha@ti.com> >> Suggested-by: Thierry Reding <thierry.reding@gmail.com> >> cc: eric@anholt.net >> cc: laurent.pinchart@ideasonboard.com >> --- >> It still annoys me that the unbound master drm device does not probe >> again if the panel device becomes available again. If there is no >> remedy to this, may be we should consider applying the module get/put >> patch[1] too. >> >> Hmmm... there was an obvious reasons not to add module gets and puts >> to drm_panel_attach/detach(), but is there any such reasons for not to >> add and remove the device link there? > > Yeah that might be the better place for this. At least conceptually it's > when we're creating the dependency link. > >> The bridge side look more complicated as there is no public >> drm_bridge_detach(), and I am not sure if the drm_bridge_remove() should >> remove the device link. I guess it should, if the link is there. > > drm_bridge_remove is meant to be called when unloading the driver, which > is too late. We probably need a drm_bridge_detach (which the master > drm_device driver would need to call). Since bridges are linked through > drm_encoder/bridge already, the drm core could make that call (would avoid > having to audit all the drivers I think). > Actually, after experimenting with putting the link creation to drm_panel_attach() and removal to drm_panel_detach(), it looks to me that the approach taken in drm_bridge is correct and the one in drm_panel is wrong. The detach should never be called by the provider (panel or pridge driver), but only by the consumer (master drm device) when it does not need the provider any more. So if I put device link code to drm_panel_attach/detach() I need to remove the detach call from panel driver's remove call. BTW, what is the point of assigning the pointers in struct drm_panel to null just before it is going away in the first place? > But imo we can postpone fixing the bridge side of things to a later time. > >> [1] https://lists.freedesktop.org/archives/dri-devel/2018-February/166350.html > > Afaiui the device_link will make sure that the main driver gets unbound > before the panel driver. Since a module unload implies a driver unbind I > think we're safe with this, and don't need any additional protection. > > Wrt reprobing the main drm_driver when you rebind/reload the panel driver: > I guess poke it through the sysfs reprobe thing, or just reload the main > driver too, that should work. It's still driver reloading we're talking > about, as long as you can't make the kernel go boom I think it's all > perfectly fine. Yes, almost anything works. I just feel that the probe should happen automatically, but I'll see if Lukas' patch helps with it. Best regards, Jyri > -Daniel > >> >> drivers/gpu/drm/bridge/panel.c | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c >> index 6d99d4a..d71ddd8 100644 >> --- a/drivers/gpu/drm/bridge/panel.c >> +++ b/drivers/gpu/drm/bridge/panel.c >> @@ -22,6 +22,7 @@ struct panel_bridge { >> struct drm_connector connector; >> struct drm_panel *panel; >> u32 connector_type; >> + struct device_link *link; /* link to master drm dev */ >> }; >> >> static inline struct panel_bridge * >> @@ -57,6 +58,22 @@ static const struct drm_connector_funcs panel_bridge_connector_funcs = { >> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, >> }; >> >> +static int panel_bridge_link_to_master(struct panel_bridge *panel_bridge) >> +{ >> + struct device *mdev = panel_bridge->bridge.dev->dev; >> + struct device *pdev = panel_bridge->panel->dev; >> + u32 flags = DL_FLAG_AUTOREMOVE; >> + >> + panel_bridge->link = device_link_add(mdev, pdev, flags); >> + if (!panel_bridge->link) { >> + dev_err(pdev, "failed to link panel %s to %s\n", >> + dev_name(pdev), dev_name(mdev)); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> static int panel_bridge_attach(struct drm_bridge *bridge) >> { >> struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); >> @@ -82,6 +99,9 @@ static int panel_bridge_attach(struct drm_bridge *bridge) >> drm_mode_connector_attach_encoder(&panel_bridge->connector, >> bridge->encoder); >> >> + if (panel_bridge_link_to_master(panel_bridge)) >> + return -EINVAL; >> + >> ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector); >> if (ret < 0) >> return ret; >> @@ -94,6 +114,8 @@ static void panel_bridge_detach(struct drm_bridge *bridge) >> struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); >> >> drm_panel_detach(panel_bridge->panel); >> + >> + device_link_del(panel_bridge->link); >> } >> >> static void panel_bridge_pre_enable(struct drm_bridge *bridge) >> -- >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki >> >
On Wed, Feb 21, 2018 at 10:51:50AM +0200, Jyri Sarha wrote: > On 21/02/18 01:47, Daniel Vetter wrote: > > On Wed, Feb 21, 2018 at 12:21:50AM +0200, Jyri Sarha wrote: > >> Currently the master drm driver is not protected against the attached > >> panel driver from becoming unavailable. Adding a device_link with > >> DL_FLAG_AUTOREMOVE flag unbinds the master drm device (the consumer) > >> when the panel device (the supplier) becomes unavailable. > >> > >> Signed-off-by: Jyri Sarha <jsarha@ti.com> > >> Suggested-by: Thierry Reding <thierry.reding@gmail.com> > >> cc: eric@anholt.net > >> cc: laurent.pinchart@ideasonboard.com > >> --- > >> It still annoys me that the unbound master drm device does not probe > >> again if the panel device becomes available again. If there is no > >> remedy to this, may be we should consider applying the module get/put > >> patch[1] too. > >> > >> Hmmm... there was an obvious reasons not to add module gets and puts > >> to drm_panel_attach/detach(), but is there any such reasons for not to > >> add and remove the device link there? > > > > Yeah that might be the better place for this. At least conceptually it's > > when we're creating the dependency link. > > > >> The bridge side look more complicated as there is no public > >> drm_bridge_detach(), and I am not sure if the drm_bridge_remove() should > >> remove the device link. I guess it should, if the link is there. > > > > drm_bridge_remove is meant to be called when unloading the driver, which > > is too late. We probably need a drm_bridge_detach (which the master > > drm_device driver would need to call). Since bridges are linked through > > drm_encoder/bridge already, the drm core could make that call (would avoid > > having to audit all the drivers I think). > > > > Actually, after experimenting with putting the link creation to > drm_panel_attach() and removal to drm_panel_detach(), it looks to me > that the approach taken in drm_bridge is correct and the one in > drm_panel is wrong. The detach should never be called by the provider > (panel or pridge driver), but only by the consumer (master drm device) > when it does not need the provider any more. > > So if I put device link code to drm_panel_attach/detach() I need to > remove the detach call from panel driver's remove call. BTW, what is the > point of assigning the pointers in struct drm_panel to null just before > it is going away in the first place? Honestly I didn't check actual usage, my recommendation was based on the assumption that attach/detach is called by the consumer (drm device), not by the panel driver itself in it's unload code. For that I figured something like drm_bridge_remove (which removes the driver from of lookup tables and stuff like that) should be the only thing. With device_link ensure that remove never gets called while the panel is still attached to a drm_device. -Daniel > > > But imo we can postpone fixing the bridge side of things to a later time. > > > >> [1] https://lists.freedesktop.org/archives/dri-devel/2018-February/166350.html > > > > Afaiui the device_link will make sure that the main driver gets unbound > > before the panel driver. Since a module unload implies a driver unbind I > > think we're safe with this, and don't need any additional protection. > > > > Wrt reprobing the main drm_driver when you rebind/reload the panel driver: > > I guess poke it through the sysfs reprobe thing, or just reload the main > > driver too, that should work. It's still driver reloading we're talking > > about, as long as you can't make the kernel go boom I think it's all > > perfectly fine. > > Yes, almost anything works. I just feel that the probe should happen > automatically, but I'll see if Lukas' patch helps with it. > > Best regards, > Jyri > > > -Daniel > > > >> > >> drivers/gpu/drm/bridge/panel.c | 22 ++++++++++++++++++++++ > >> 1 file changed, 22 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c > >> index 6d99d4a..d71ddd8 100644 > >> --- a/drivers/gpu/drm/bridge/panel.c > >> +++ b/drivers/gpu/drm/bridge/panel.c > >> @@ -22,6 +22,7 @@ struct panel_bridge { > >> struct drm_connector connector; > >> struct drm_panel *panel; > >> u32 connector_type; > >> + struct device_link *link; /* link to master drm dev */ > >> }; > >> > >> static inline struct panel_bridge * > >> @@ -57,6 +58,22 @@ static const struct drm_connector_funcs panel_bridge_connector_funcs = { > >> .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > >> }; > >> > >> +static int panel_bridge_link_to_master(struct panel_bridge *panel_bridge) > >> +{ > >> + struct device *mdev = panel_bridge->bridge.dev->dev; > >> + struct device *pdev = panel_bridge->panel->dev; > >> + u32 flags = DL_FLAG_AUTOREMOVE; > >> + > >> + panel_bridge->link = device_link_add(mdev, pdev, flags); > >> + if (!panel_bridge->link) { > >> + dev_err(pdev, "failed to link panel %s to %s\n", > >> + dev_name(pdev), dev_name(mdev)); > >> + return -EINVAL; > >> + } > >> + > >> + return 0; > >> +} > >> + > >> static int panel_bridge_attach(struct drm_bridge *bridge) > >> { > >> struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > >> @@ -82,6 +99,9 @@ static int panel_bridge_attach(struct drm_bridge *bridge) > >> drm_mode_connector_attach_encoder(&panel_bridge->connector, > >> bridge->encoder); > >> > >> + if (panel_bridge_link_to_master(panel_bridge)) > >> + return -EINVAL; > >> + > >> ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector); > >> if (ret < 0) > >> return ret; > >> @@ -94,6 +114,8 @@ static void panel_bridge_detach(struct drm_bridge *bridge) > >> struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); > >> > >> drm_panel_detach(panel_bridge->panel); > >> + > >> + device_link_del(panel_bridge->link); > >> } > >> > >> static void panel_bridge_pre_enable(struct drm_bridge *bridge) > >> -- > >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > >> > > > > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c index 6d99d4a..d71ddd8 100644 --- a/drivers/gpu/drm/bridge/panel.c +++ b/drivers/gpu/drm/bridge/panel.c @@ -22,6 +22,7 @@ struct panel_bridge { struct drm_connector connector; struct drm_panel *panel; u32 connector_type; + struct device_link *link; /* link to master drm dev */ }; static inline struct panel_bridge * @@ -57,6 +58,22 @@ static const struct drm_connector_funcs panel_bridge_connector_funcs = { .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, }; +static int panel_bridge_link_to_master(struct panel_bridge *panel_bridge) +{ + struct device *mdev = panel_bridge->bridge.dev->dev; + struct device *pdev = panel_bridge->panel->dev; + u32 flags = DL_FLAG_AUTOREMOVE; + + panel_bridge->link = device_link_add(mdev, pdev, flags); + if (!panel_bridge->link) { + dev_err(pdev, "failed to link panel %s to %s\n", + dev_name(pdev), dev_name(mdev)); + return -EINVAL; + } + + return 0; +} + static int panel_bridge_attach(struct drm_bridge *bridge) { struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); @@ -82,6 +99,9 @@ static int panel_bridge_attach(struct drm_bridge *bridge) drm_mode_connector_attach_encoder(&panel_bridge->connector, bridge->encoder); + if (panel_bridge_link_to_master(panel_bridge)) + return -EINVAL; + ret = drm_panel_attach(panel_bridge->panel, &panel_bridge->connector); if (ret < 0) return ret; @@ -94,6 +114,8 @@ static void panel_bridge_detach(struct drm_bridge *bridge) struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge); drm_panel_detach(panel_bridge->panel); + + device_link_del(panel_bridge->link); } static void panel_bridge_pre_enable(struct drm_bridge *bridge)
Currently the master drm driver is not protected against the attached panel driver from becoming unavailable. Adding a device_link with DL_FLAG_AUTOREMOVE flag unbinds the master drm device (the consumer) when the panel device (the supplier) becomes unavailable. Signed-off-by: Jyri Sarha <jsarha@ti.com> Suggested-by: Thierry Reding <thierry.reding@gmail.com> cc: eric@anholt.net cc: laurent.pinchart@ideasonboard.com --- It still annoys me that the unbound master drm device does not probe again if the panel device becomes available again. If there is no remedy to this, may be we should consider applying the module get/put patch[1] too. Hmmm... there was an obvious reasons not to add module gets and puts to drm_panel_attach/detach(), but is there any such reasons for not to add and remove the device link there? The bridge side look more complicated as there is no public drm_bridge_detach(), and I am not sure if the drm_bridge_remove() should remove the device link. I guess it should, if the link is there. [1] https://lists.freedesktop.org/archives/dri-devel/2018-February/166350.html drivers/gpu/drm/bridge/panel.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)