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@xxxxxx> >> Suggested-by: Thierry Reding <thierry.reding@xxxxxxxxx> >> cc: eric@xxxxxxxxxx >> cc: laurent.pinchart@xxxxxxxxxxxxxxxx >> --- >> 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 >> > -- 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
