Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver

[Laurent Pinchart]

Hi Philipp,

On Tuesday 07 Feb 2017 11:41:30 Philipp Zabel wrote:
> On Tue, 2017-02-07 at 12:26 +0200, Laurent Pinchart wrote:
> > On Monday 06 Feb 2017 15:10:46 Steve Longerbeam wrote:
> >> On 02/06/2017 02:33 PM, Laurent Pinchart wrote:
> >>> On Monday 06 Feb 2017 10:50:22 Hans Verkuil wrote:
> >>>> On 02/05/2017 04:48 PM, Laurent Pinchart wrote:
> >>>>> On Tuesday 24 Jan 2017 18:07:55 Steve Longerbeam wrote:
> >>>>>> On 01/24/2017 04:02 AM, Philipp Zabel wrote:
> >>>>>>> On Fri, 2017-01-20 at 15:03 +0100, Hans Verkuil wrote:
> >>>>>>>>> +
> >>>>>>>>> +int vidsw_g_mbus_config(struct v4l2_subdev *sd, struct
> >>>>>>>>> v4l2_mbus_config *cfg)
> > 
> > [snip]
> > 
> >>>>>>>> I am not certain this op is needed at all. In the current kernel
> >>>>>>>> this op is only used by soc_camera, pxa_camera and omap3isp
> >>>>>>>> (somewhat dubious). Normally this information should come from the
> >>>>>>>> device tree and there should be no need for this op.
> >>>>>>>> 
> >>>>>>>> My (tentative) long-term plan was to get rid of this op.
> >>>>>>>> 
> >>>>>>>> If you don't need it, then I recommend it is removed.
> >>>>>> 
> >>>>>> Hi Hans, the imx-media driver was only calling g_mbus_config to the
> >>>>>> camera sensor, and it was doing that to determine the sensor's bus
> >>>>>> type. This info was already available from parsing a
> >>>>>> v4l2_of_endpoint from the sensor node. So it was simple to remove the
> >>>>>> g_mbus_config calls, and instead rely on the parsed sensor
> >>>>>> v4l2_of_endpoint.
> >>>>> 
> >>>>> That's not a good point.
> >>> 
> >>> (mea culpa, s/point/idea/)
> >>> 
> >>>>> The imx-media driver must not parse the sensor DT node as it is not
> >>>>> aware of what bindings the sensor is compatible with.
> >> 
> >> Hi Laurent,
> >> 
> >> I don't really understand this argument. The sensor node has been found
> >> by parsing the OF graph, so it is known to be a camera sensor node at
> >> that point.
> > 
> > All you know in the i.MX6 driver is that the remote node is a video
> > source. You can rely on the fact that it implements the OF graph bindings
> > to locate other ports in that DT node, but that's more or less it.
> > 
> > DT properties are defined by DT bindings and thus qualified by a
> > compatible string. Unless you match on sensor compat strings in the i.MX6
> > driver (which you shouldn't do, to keep the driver generic) you can't know
> > for certain how to parse the sensor node DT properties. For all you know,
> > the video source could be a bridge such as an HDMI to CSI-2 converter for
> > instance, so you can't even rely on the fact that it's a sensor.
> > 
> >>>>> Information must instead be queried from the sensor subdev at
> >>>>> runtime, through the g_mbus_config() operation.
> >>>>> 
> >>>>> Of course, if you can get the information from the imx-media DT
> >>>>> node, that's certainly an option. It's only information provided by
> >>>>> the sensor driver that you have no choice but query using a subdev
> >>>>> operation.
> >>>> 
> >>>> Shouldn't this come from the imx-media DT node? BTW, why is omap3isp
> >>>> using this?
> >>> 
> >>> It all depends on what type of information needs to be retrieved, and
> >>> whether it can change at runtime or is fixed. Adding properties to the
> >>> imx-media DT node is certainly fine as long as those properties
> >>> describe the i.MX side.
> >> 
> >> In this case the info needed is the media bus type. That info is most
> >> easily available by calling v4l2_of_parse_endpoint() on the sensor's
> >> endpoint node.
> > 
> > I haven't had time to check the code in details yet, so I can't really
> > comment on what you need and how it should be implemented exactly.
> > 
> >> The media bus type is not something that can be added to the
> >> imx-media node since it contains no endpoint nodes.
> > 
> > Agreed. You have endpoints in the CSI nodes though.
> > 
> >>> In the omap3isp case, we use the operation to query whether parallel
> >>> data contains embedded sync (BT.656) or uses separate h/v sync signals.
> >>> 
> >>>> The reason I am suspicious about this op is that it came from
> >>>> soc-camera and predates the DT. The contents of v4l2_mbus_config seems
> >>>> very much like a HW description to me, i.e. something that belongs in
> >>>> the DT.
> >>> 
> >>> Part of it is possibly outdated, but for buses that support multiple
> >>> modes of operation (such as the parallel bus case described above) we
> >>> need to make that information discoverable at runtime. Maybe this should
> >>> be considered as related to Sakari's efforts to support VC/DT for CSI-2,
> >>> and supported through the API he is working on.
> >>
> >> That sounds interesting, can you point me to some info on this effort?
> > 
> > Sure.
> > 
> > http://git.retiisi.org.uk/?p=~sailus/linux.git;a=shortlog;h=refs/heads/vc
> > 
> >> I've been thinking the DT should contain virtual channel info for CSI-2
> >> buses.
> > 
> > I don't think it should. CSI-2 virtual channels and data types should be
> > handled as a software concept, and thus supported through driver code
> > without involving DT.
> 
> I agree. The CSI2IPU gasket is a bit special in that it distributes its
> input data to four different parallel buses depending on the input's VC,
> but upstream of the MIPI CSI-2 receiver, any virtual channel information
> is purely a matter of the data sent over the CSI-2 link, and not board
> specific hardware description.

If the CSI2IPU gasket has four physical output buses then it can be modelled 
as an entity with four source pads. Only when VC/DT are multiplexed on the 
same physical bus do I think they should be handled without involving the 
device tree and the media controller graph.

-- 
Regards,

Laurent Pinchart