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@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? 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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
