[PATCH RFC] drm/bridge: panel: Add device_link between panel and master drm device

Wed Feb 21 18:33:23 UTC 2018

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 at ti.com>
> >> Suggested-by: Thierry Reding <thierry.reding at gmail.com>
> >> cc: eric at anholt.net
> >> cc: laurent.pinchart at 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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch