Message-Id: <20140320170910.F0D75C4067A@trevor.secretlab.ca>
Date:	Thu, 20 Mar 2014 17:09:10 +0000
From:	Grant Likely <grant.likely@...aro.org>
To:	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Philipp Zabel <p.zabel@...gutronix.de>
Cc:	Tomi Valkeinen <tomi.valkeinen@...com>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Rob Herring <robherring2@...il.com>,
	Russell King - ARM Linux <linux@....linux.org.uk>,
	Mauro Carvalho Chehab <m.chehab@...sung.com>,
	Rob Herring <robh+dt@...nel.org>,
	Sylwester Nawrocki <s.nawrocki@...sung.com>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Philipp Zabel <philipp.zabel@...il.com>
Subject: Re: [RFC PATCH] [media]: of: move graph helpers from drivers/media/v4l2-core to drivers/of

On Tue, 11 Mar 2014 12:43:44 +0100, Laurent Pinchart <laurent.pinchart@...asonboard.com> wrote:
> Hi Philipp,
> 
> On Monday 10 March 2014 16:40:30 Philipp Zabel wrote:
> > Am Montag, den 10.03.2014, 16:15 +0100 schrieb Laurent Pinchart:
> > > On Monday 10 March 2014 14:58:15 Grant Likely wrote:
> > > > On Mon, 10 Mar 2014 14:52:53 +0100, Laurent Pinchart wrote:
> > > > > On Monday 10 March 2014 12:18:20 Tomi Valkeinen wrote:
> > > > > > On 08/03/14 13:41, Grant Likely wrote:
> > > > > > >> Ok. If we go for single directional link, the question is then:
> > > > > > >> which way? And is the direction different for display and camera,
> > > > > > >> which are kind of reflections of each other?
> > > > > > > 
> > > > > > > In general I would recommend choosing whichever device you would
> > > > > > > sensibly think of as a master. In the camera case I would choose
> > > > > > > the camera controller node instead of the camera itself, and in
> > > > > > > the display case I would choose the display controller instead of
> > > > > > > the panel. The binding author needs to choose what she things
> > > > > > > makes the most sense, but drivers can still use if it it turns out
> > > > > > > to be 'backwards'
> > > > > > 
> > > > > > I would perhaps choose the same approach, but at the same time I
> > > > > > think it's all but clear. The display controller doesn't control the
> > > > > > panel any more than a DMA controller controls, say, the display
> > > > > > controller.
> > > > > > 
> > > > > > In fact, in earlier versions of OMAP DSS DT support I had a simpler
> > > > > > port description, and in that I had the panel as the master (i.e.
> > > > > > link from panel to dispc) because the panel driver uses the display
> > > > > > controller's features to provide the panel device a data stream.
> > > > > > 
> > > > > > And even with the current OMAP DSS DT version, which uses the v4l2
> > > > > > style ports/endpoints, the driver model is still the same, and only
> > > > > > links towards upstream are used.
> > > > > > 
> > > > > > So one reason I'm happy with the dual-linking is that I can easily
> > > > > > follow the links from the downstream entities to upstream entities,
> > > > > > and other people, who have different driver model, can easily do the
> > > > > > opposite.
> > > > > > 
> > > > > > But I agree that single-linking is enough and this can be handled at
> > > > > > runtime, even if it makes the code more complex. And perhaps
> > > > > > requires extra data in the dts, to give the start points for the
> > > > > > graph.
> > > > > 
> > > > > In theory unidirectional links in DT are indeed enough. However, let's
> > > > > not forget the following.
> > > > > 
> > > > > - There's no such thing as single start points for graphs. Sure, in
> > > > > some simple cases the graph will have a single start point, but that's
> > > > > not a generic rule. For instance the camera graphs
> > > > > http://ideasonboard.org/media/omap3isp.ps and
> > > > > http://ideasonboard.org/media/eyecam.ps have two camera sensors, and
> > > > > thus two starting points from a data flow point of view. And if you
> > > > > want a better understanding of how complex media graphs can become,
> > > > > have a look at http://ideasonboard.org/media/vsp1.0.pdf (that's a real
> > > > > world example, albeit all connections are internal to the SoC in that
> > > > > particular case, and don't need to be described in DT).
> > > > > 
> > > > > - There's also no such thing as a master device that can just point to
> > > > > slave devices. Once again simple cases exist where that model could
> > > > > work, but real world examples exist of complex pipelines with dozens
> > > > > of elements all implemented by a separate IP core and handled by
> > > > > separate drivers, forming a graph with long chains and branches. We
> > > > > thus need real graph bindings.
> > > > > 
> > > > > - Finally, having no backlinks in DT would make the software
> > > > > implementation very complex. We need to be able to walk the graph in a
> > > > > generic way without having any of the IP core drivers loaded, and
> > > > > without any specific starting point. We would thus need to parse the
> > > > > complete DT tree, looking at all nodes and trying to find out whether
> > > > > they're part of the graph we're trying to walk. The complexity of the
> > > > > operation would be at best quadratic to the number of nodes in the
> > > > > whole DT and to the number of nodes in the graph.
> > > > 
> > > > Not really. To being with, you cannot determine any meaning of a node
> > > > across the tree (aside from it being an endpoint)
> > > 
> > > That's the important part. I can assume the target node of the
> > > remote-endpoint phandle to be an endpoint, and can thus assume that it
> > > implements the of-graph bindings. That's all I need to be able to walk
> > > the graph in a generic way.
> > >
> > > > without also understanding the binding that the node is a part of. That
> > > > means you need to have something matching against the compatible string
> > > > on both ends of the linkage. For instance:
> > > > 
> > > > panel {
> > > > 	compatible = "acme,lvds-panel";
> > > > 	lvds-port: port {
> > > > 	};
> > > > };
> > > > 
> > > > display-controller {
> > > > 	compatible = "encom,video";
> > > > 	port {
> > > > 		remote-endpoint = <&lvds-port>;
> > > > 	};
> > > > };
> > > > 
> > > > In the above example, the encom,video driver has absolutely zero
> > > > information about what the acme,lvds-panel binding actually implements.
> > > > There needs to be both a driver for the "acme,lvds-panel" binding and
> > > > one for the "encom,video" binding (even if the acme,lvds-panel binding
> > > > is very thin and defers the functionality to the video controller).
> > > 
> > > I absolutely agree with that. We need a driver for each device (in this
> > > case the acme panel and the encom display controller), and we need those
> > > drivers to register entities (in the generic sense of the term) for them
> > > to be able to communicate with each other. The display controller driver
> > > must not try to parse panel-specific properties from the panel node.
> > > However, as described above, I believe it can parse ports and endpoints
> > > to walk the graph.
> > >
> > > > What you want here is the drivers to register each side of the
> > > > connection. That could be modeled with something like the following
> > > > (pseudocode):
> > > > 
> > > > struct of_endpoint {
> > > > 	struct list_head list;
> > > > 	struct device_node *ep_node;
> > > > 	void *context;
> > > > 	void (*cb)(struct of_endpoint *ep, void *data);
> > > > }
> > > > 
> > > > int of_register_port(struct device *node, void (*cb)(struct of_endpoint
> > > > *ep, void *data), void *data) {
> > > > 	struct of_endpoint *ep = kzalloc(sizeof(*ep), GFP_KERNEL);
> > > > 	
> > > > 	ep->ep_node = node;
> > > > 	ep->data = data;
> > > > 	ep->callback = cb;
> > > > 	
> > > > 	/* store the endpoint to a list */
> > > > 	/* check if the endpoint has a remote-endpoint link */
> > > > 		/* If so, then link the two together and call the
> > > > 		 * callbacks */
> > > > }
> > > > 
> > > > That's neither expensive or complicated.
> > > > 
> > > > Originally I suggested walking the whole tree multiple times, but as
> > > > mentioned that doesn't scale, and as I thought about the above it isn't
> > > > even a valid thing to do. Everything has to be driven by drivers, so
> > > > even if the backlinks are there, nothing can be done with the link until
> > > > the other side goes through enumeration independently.
> > > 
> > > For such composite devices, what we need from a drivers point of view is a
> > > mechanism to wait for all components to be in place before proceeding.
> > > This isn't DT-related as such, but the graph is obviously described in DT
> > > for DT- based platforms.
> > > 
> > > There are at least two mainline implementation of such a mechanism. One of
> > > them can be found in drivers/media/v4l2-core/v4l2-async.c, another more
> > > recent one in drivers/base/component.c. Neither of them is DT-specific,
> > > and they don't try to parse DT content.
> > > 
> > > The main problem, from a DT point of view, is that we need to pick a
> > > master driver that will initiate the process of waiting for all components
> > > to be in place. This is usually the driver of the main component inside
> > > the SoC. For a camera capture pipeline the master is the SoC camera device
> > > driver that will create the V4L2 device node(s). For a display pipeline
> > > the master is the SoC display driver that will create the DRM/KMS
> > > devices.
> > > 
> > > The master device driver needs to create a list of all components it
> > > needs, and wait until all those components have been probed by their
> > > respective driver. Creating such a list requires walking the graph,
> > > starting at the master device (using a CPU-centric view as described by
> > > Russell). This is why we need the backlinks, as the master device can have
> > > inbound links.
> >
> > We could scan the whole tree for entities, ports and endpoints once, in
> > the base oftree code, and put that into a graph structure, adding the
> > backlinks.
> > The of_graph_* helpers could then use that graph instead of the device
> > tree.
> 
> That could work. The complexity would still be quadratic, but we would parse 
> the full device tree once only.
> 
> The runtime complexity would still be increased, as the graph helpers would 
> need to find the endpoint object in the parsed graph corresponding to the DT 
> node they get as an argument. That's proportional to the number of graph 
> elements, not the total number of DT nodes, so I suppose it's not too bad.
> 
> We also need to make sure this would work with insertion of DT fragments at 
> runtime. Probably not a big deal, but it has to be kept in mind.

That is also an important aspect. Another reason to be explicit about
what depends on what. It is a lot easier to handle if each driver knows
which links it has a dependency on, and if the dependency tree is
acyclic.

g.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists