[PATCH v2 45/49] drm/omap: Add support for drm_bridge

[Laurent Pinchart]

Hi Tomi,

On Fri, Jan 18, 2019 at 12:33:03PM +0200, Tomi Valkeinen wrote:
> On 11/01/19 05:51, Laurent Pinchart wrote:
> > Hook up drm_bridge support in the omapdrm driver. Despite the recent
> > extensive preparation work, this is a rather intrusive change, as the
> > management of outputs needs to be adapted through the driver to handle
> > both omap_dss_device and drm_bridge.
> > 
> > Connector creation is skipped when using a drm_bridge, as the bridge
> > creates the connector internally. This creates issues with systems that
> > split connector operations (such as modes retrieval and hot-plug
> > detection) across different bridges. These systems can't be supported
> > using drm_bridge for now (their support through the omap_dss_device
> > infrastructure is not affected), this will be fixed in subsequent
> > changes.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Sebastian Reichel <sebastian.reichel at collabora.com>
> 
> <snip>
> 
> > diff --git a/drivers/gpu/drm/omapdrm/dss/output.c b/drivers/gpu/drm/omapdrm/dss/output.c
> > index f25ecfd26534..2a53025c2fde 100644
> > --- a/drivers/gpu/drm/omapdrm/dss/output.c
> > +++ b/drivers/gpu/drm/omapdrm/dss/output.c
> > @@ -20,25 +20,34 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/of.h>
> > +#include <linux/of_graph.h>
> >  
> >  #include "dss.h"
> >  #include "omapdss.h"
> >  
> >  int omapdss_device_init_output(struct omap_dss_device *out)
> >  {
> > -	out->next = omapdss_of_find_connected_device(out->dev->of_node, 0);
> > -	if (IS_ERR(out->next)) {
> > -		if (PTR_ERR(out->next) != -EPROBE_DEFER)
> > -			dev_err(out->dev, "failed to find video sink\n");
> > -		return PTR_ERR(out->next);
> > +	struct device_node *remote_node;
> > +
> > +	remote_node = of_graph_get_remote_node(out->dev->of_node, 0, 0);
> > +	if (!remote_node) {
> > +		dev_dbg(out->dev, "failed to find video sink\n");
> > +		return 0;
> >  	}
> 
> This breaks boards that have displays/bridges connected to ports above
> 0. For example, DPI output has 3 ports on some SoCs.

Does it break them, or are they already broken ?
omapdss_of_find_connected_device() is defined as follows:

struct omap_dss_device *
omapdss_of_find_connected_device(struct device_node *node, unsigned int port)
{
        struct device_node *remote_node;
        struct omap_dss_device *dssdev;

        remote_node = of_graph_get_remote_node(node, port, 0);
        if (!remote_node)
                return NULL;

        dssdev = omapdss_find_device_by_node(remote_node);
        of_node_put(remote_node);

        return dssdev ? dssdev : ERR_PTR(-EPROBE_DEFER);
}

Before this patch is was called with port set to 0, so there's no change
here.

> I made a quick fix like so:
> 
> +       port_num = __ffs(out->of_ports);
> +
> +       remote_node = of_graph_get_remote_node(out->dev->of_node,
> port_num, 0);
> 
> Which I think works for all our outputs as we only set a single bit in
> of_ports for outputs. But I don't think that's quite correct. Should we
> have another field which tells which port is to be used? Then again,
> maybe __ffs() is good enough here, as we can guarantee that there's ever
> a single port in of_ports.

Down the road I think it would make sense to replace of_ports with
of_port. I thought we can't do so right now as we have three encoder
drivers that set two bits in of_ports, but now that I've double-checked,
of_ports isn't used. We could thus replace it, and only set it for the
internal DSS devices.

If my analysis is correct and the problem isn't introduced by this
patch, could it be fixed on top of the series ?

-- 
Regards,

Laurent Pinchart