Re: [PATCH v2 5/6] drm/vc4: Support the case where the DSI device is disabled

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3424 bytes --]

On Fri, May 04, 2018 at 03:49:06PM +0200, Boris Brezillon wrote:
> On Fri, 4 May 2018 15:29:15 +0200
> Thierry Reding <thierry.reding@gmail.com> wrote:
> 
> > On Fri, May 04, 2018 at 02:05:25PM +0200, Boris Brezillon wrote:
> > > On Fri, 4 May 2018 12:28:33 +0200
> > > Thierry Reding <thierry.reding@gmail.com> wrote:
> > >   
> > > > On Thu, May 03, 2018 at 06:40:08PM +0200, Boris Brezillon wrote:  
> > > > > Having a device with a status property != "okay" in the DT is a valid
> > > > > use case, and we should not prevent the registration of the DRM device
> > > > > when the DSI device connected to the DSI controller is disabled.
> > > > > 
> > > > > Consider the ENODEV return code as a valid result and do not expose the
> > > > > DSI encoder/connector when it happens.
> > > > > 
> > > > > Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > > > > ---
> > > > >  drivers/gpu/drm/vc4/vc4_dsi.c | 15 +++++++++++++--
> > > > >  1 file changed, 13 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
> > > > > index 8aa897835118..db2f137f8b7b 100644
> > > > > --- a/drivers/gpu/drm/vc4/vc4_dsi.c
> > > > > +++ b/drivers/gpu/drm/vc4/vc4_dsi.c
> > > > > @@ -1606,8 +1606,18 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
> > > > >  
> > > > >  	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0,
> > > > >  					  &panel, &dsi->bridge);
> > > > > -	if (ret)
> > > > > +	if (ret) {
> > > > > +		/* If the bridge or panel pointed by dev->of_node is not
> > > > > +		 * enabled, just return 0 here so that we don't prevent the DRM
> > > > > +		 * dev from being registered. Of course that means the DSI
> > > > > +		 * encoder won't be exposed, but that's not a problem since
> > > > > +		 * nothing is connected to it.
> > > > > +		 */
> > > > > +		if (ret == -ENODEV)
> > > > > +			return 0;
> > > > > +
> > > > >  		return ret;
> > > > > +	}
> > > > >  
> > > > >  	if (panel) {
> > > > >  		dsi->bridge = devm_drm_panel_bridge_add(dev, panel,
> > > > > @@ -1652,7 +1662,8 @@ static void vc4_dsi_unbind(struct device *dev, struct device *master,
> > > > >  	struct vc4_dev *vc4 = to_vc4_dev(drm);
> > > > >  	struct vc4_dsi *dsi = dev_get_drvdata(dev);
> > > > >  
> > > > > -	pm_runtime_disable(dev);
> > > > > +	if (dsi->bridge)
> > > > > +		pm_runtime_disable(dev);    
> > > > 
> > > > Is this safe? This uses component/master, so dsi->bridge is going to
> > > > remain valid until the driver's ->remove() is called. So technically you
> > > > could have a situation where drm_of_find_panel_or_bridge() returned some
> > > > error code that remains stored in dsi->bridge and cause the above
> > > > condition to be incorrectly true.  
> > > 
> > > No, because of_drm_find_bridge() (which is called from
> > > drm_of_find_panel_or_bridge() returns either NULL or a valid bridge
> > > pointer), so dsi->bridge either points to a valid bridge object or is
> > > NULL. Am I missing something?  
> > 
> > The return value of devm_drm_panel_bridge_add() is also assigned to
> > dsi->bridge later on (in the "if (panel)" conditional).
> 
> But then we return an error code if IS_ERR(dsi->bridge) [1], which
> should prevent the unbind function from being called, right?

Right, that should work. Seems safe, then.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel