Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1573899 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-02-05 16:50 +0100
    Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver Hans Verkuil <hverkuil@xs4all.nl> - 2017-02-06 11:00 +0100
      Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-02-06 23:40 +0100
        Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver Steve Longerbeam <slongerbeam@gmail.com> - 2017-02-07 00:20 +0100
          Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-02-07 11:30 +0100
            Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice  driver Philipp Zabel <p.zabel@pengutronix.de> - 2017-02-07 11:50 +0100
              Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-02-07 15:20 +0100
            Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver Benoit Parrot <bparrot@ti.com> - 2017-02-07 14:50 +0100
              Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver Sakari Ailus <sakari.ailus@iki.fi> - 2017-02-07 22:00 +0100
              Re: [PATCH v3 13/24] platform: add video-multiplexer subdevice driver Laurent Pinchart <laurent.pinchart@ideasonboard.com> - 2017-02-08 00:20 +0100

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

Hi Steve,

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)
> >>> +{
> >>> +	struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
> >>> +	struct media_pad *pad;
> >>> +	int ret;
> >>> +
> >>> +	if (vidsw->active == -1) {
> >>> +		dev_err(sd->dev, "no configuration for inactive mux\n");
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	/*
> >>> +	 * Retrieve media bus configuration from the entity connected to the
> >>> +	 * active input
> >>> +	 */
> >>> +	pad = media_entity_remote_pad(&vidsw->pads[vidsw->active]);
> >>> +	if (pad) {
> >>> +		sd = media_entity_to_v4l2_subdev(pad->entity);
> >>> +		ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
> >>> +		if (ret == -ENOIOCTLCMD)
> >>> +			pad = NULL;
> >>> +		else if (ret < 0) {
> >>> +			dev_err(sd->dev, "failed to get source 
configuration\n");
> >>> +			return ret;
> >>> +		}
> >>> +	}
> >>> +	if (!pad) {
> >>> +		/* Mirror the input side on the output side */
> >>> +		cfg->type = vidsw->endpoint[vidsw->active].bus_type;
> >>> +		if (cfg->type == V4L2_MBUS_PARALLEL ||
> >>> +		    cfg->type == V4L2_MBUS_BT656)
> >>> +			cfg->flags = vidsw->endpoint[vidsw-
>active].bus.parallel.flags;
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >> 
> >> 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. The imx-media driver must not parse the sensor DT 
node as it is not aware of what bindings the sensor is compatible with. 
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.

> > We currently use this to make the CSI capture interface understand
> > whether its source from the MIPI CSI-2 or from the parallel bus. That is
> > probably something that should be fixed, but I'm not quite sure how.
> > 
> > The Synopsys DesignWare MIPI CSI-2 reciever turns the incoming MIPI
> > CSI-2 signal into a 32-bit parallel pixel bus plus some signals for the
> > MIPI specific metadata (virtual channel, data type).
> > 
> > Then the CSI2IPU gasket turns this input bus into four separate parallel
> > 16-bit pixel buses plus an 8-bit "mct_di" bus for each of them, that
> > carries the MIPI metadata. The incoming data is split into the four
> > outputs according to the MIPI virtual channel.
> > 
> > Two of these 16-bit + 8-bit parallel buses are routed through a
> > multiplexer before finally arriving at the CSI on the other side.
> > 
> > We need to configure the CSI to either use or ignore the data from the
> > 8-bit mct_di bus depending on whether the source of the mux is
> > configured to the MIPI CSI-2 receiver / CSI2IPU gasket, or to a parallel
> > input.
> 
> Philipp, from my experience, the CSI_MIPI_DI register (configured
> by ipu_csi_set_mipi_datatype()) can only be given a virtual channel 0,
> otherwise no data is received from the MIPI CSI-2 sensor, regardless
> of the virtual channel the sensor is transmitting over.
> 
> So it seems the info on the 8-bit mct_di buses generated by the CSI2IPU
> gasket are ignored by the CSI's, at least the virtual channel number is
> ignored.
> 
> For example, if the sensor is transmitting on vc 1, the gasket routes
> the sensor data to the parallel bus to CSI1. But if CSI_MIPI_DI on CSI1
> is written with vc 1, no data is received.
> 
> Steve
> 
> > Currently we let g_mbus_config pretend that even the internal 32-bit +
> > metadata and 16-bit + 8-bit metadata parallel buses are of type
> > V4L2_MBUS_CSI so that the CSI can ask the mux, which propagates to the
> > CSI-2 receiver, if connected.
> > 
> > Without g_mbus_config we'd need to get that information from somewhere
> > else. One possibility would be to extend MEDIA_BUS formats to describe
> > these "parallelized MIPI data" buses separately.

-- 
Regards,

Laurent Pinchart

[toc] | [next] | [standalone]

#1574510

On 02/05/2017 04:48 PM, Laurent Pinchart wrote:
> Hi Steve,
> 
> 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)
>>>>> +{
>>>>> +	struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
>>>>> +	struct media_pad *pad;
>>>>> +	int ret;
>>>>> +
>>>>> +	if (vidsw->active == -1) {
>>>>> +		dev_err(sd->dev, "no configuration for inactive mux\n");
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	/*
>>>>> +	 * Retrieve media bus configuration from the entity connected to the
>>>>> +	 * active input
>>>>> +	 */
>>>>> +	pad = media_entity_remote_pad(&vidsw->pads[vidsw->active]);
>>>>> +	if (pad) {
>>>>> +		sd = media_entity_to_v4l2_subdev(pad->entity);
>>>>> +		ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
>>>>> +		if (ret == -ENOIOCTLCMD)
>>>>> +			pad = NULL;
>>>>> +		else if (ret < 0) {
>>>>> +			dev_err(sd->dev, "failed to get source 
> configuration\n");
>>>>> +			return ret;
>>>>> +		}
>>>>> +	}
>>>>> +	if (!pad) {
>>>>> +		/* Mirror the input side on the output side */
>>>>> +		cfg->type = vidsw->endpoint[vidsw->active].bus_type;
>>>>> +		if (cfg->type == V4L2_MBUS_PARALLEL ||
>>>>> +		    cfg->type == V4L2_MBUS_BT656)
>>>>> +			cfg->flags = vidsw->endpoint[vidsw-
>> active].bus.parallel.flags;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>
>>>> 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. The imx-media driver must not parse the sensor DT 
> node as it is not aware of what bindings the sensor is compatible with. 
> 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?

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.

Regards,

	Hans

[toc] | [prev] | [next] | [standalone]

#1575213

Hi Hans,

(CC'ing Sakari)

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)
> >>>>> +{
> >>>>> +	struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
> >>>>> +	struct media_pad *pad;
> >>>>> +	int ret;
> >>>>> +
> >>>>> +	if (vidsw->active == -1) {
> >>>>> +		dev_err(sd->dev, "no configuration for inactive 
mux\n");
> >>>>> +		return -EINVAL;
> >>>>> +	}
> >>>>> +
> >>>>> +	/*
> >>>>> +	 * Retrieve media bus configuration from the entity connected 
to the
> >>>>> +	 * active input
> >>>>> +	 */
> >>>>> +	pad = media_entity_remote_pad(&vidsw->pads[vidsw->active]);
> >>>>> +	if (pad) {
> >>>>> +		sd = media_entity_to_v4l2_subdev(pad->entity);
> >>>>> +		ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
> >>>>> +		if (ret == -ENOIOCTLCMD)
> >>>>> +			pad = NULL;
> >>>>> +		else if (ret < 0) {
> >>>>> +			dev_err(sd->dev, "failed to get source
> >>>>> configuration\n");
> >>>>> +			return ret;
> >>>>> +		}
> >>>>> +	}
> >>>>> +	if (!pad) {
> >>>>> +		/* Mirror the input side on the output side */
> >>>>> +		cfg->type = vidsw->endpoint[vidsw->active].bus_type;
> >>>>> +		if (cfg->type == V4L2_MBUS_PARALLEL ||
> >>>>> +		    cfg->type == V4L2_MBUS_BT656)
> >>>>> +			cfg->flags = vidsw->endpoint[vidsw-
> >>>>> active].bus.parallel.flags;
> >>>>> +	}
> >>>>> +
> >>>>> +	return 0;
> >>>>> +}
> >>>> 
> >>>> 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. 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 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.

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

#1575244

On 02/06/2017 02:33 PM, Laurent Pinchart wrote:
> Hi Hans,
>
> (CC'ing Sakari)
>
> 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)
>>>>>>> +{
>>>>>>> +	struct vidsw *vidsw = v4l2_subdev_to_vidsw(sd);
>>>>>>> +	struct media_pad *pad;
>>>>>>> +	int ret;
>>>>>>> +
>>>>>>> +	if (vidsw->active == -1) {
>>>>>>> +		dev_err(sd->dev, "no configuration for inactive
> mux\n");
>>>>>>> +		return -EINVAL;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	/*
>>>>>>> +	 * Retrieve media bus configuration from the entity connected
> to the
>>>>>>> +	 * active input
>>>>>>> +	 */
>>>>>>> +	pad = media_entity_remote_pad(&vidsw->pads[vidsw->active]);
>>>>>>> +	if (pad) {
>>>>>>> +		sd = media_entity_to_v4l2_subdev(pad->entity);
>>>>>>> +		ret = v4l2_subdev_call(sd, video, g_mbus_config, cfg);
>>>>>>> +		if (ret == -ENOIOCTLCMD)
>>>>>>> +			pad = NULL;
>>>>>>> +		else if (ret < 0) {
>>>>>>> +			dev_err(sd->dev, "failed to get source
>>>>>>> configuration\n");
>>>>>>> +			return ret;
>>>>>>> +		}
>>>>>>> +	}
>>>>>>> +	if (!pad) {
>>>>>>> +		/* Mirror the input side on the output side */
>>>>>>> +		cfg->type = vidsw->endpoint[vidsw->active].bus_type;
>>>>>>> +		if (cfg->type == V4L2_MBUS_PARALLEL ||
>>>>>>> +		    cfg->type == V4L2_MBUS_BT656)
>>>>>>> +			cfg->flags = vidsw->endpoint[vidsw-
>>>>>>> active].bus.parallel.flags;
>>>>>>> +	}
>>>>>>> +
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>> 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.


>>>   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.

The media bus type is not something that can be added to the
imx-media node since it contains no endpoint nodes.


> 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?
I've been thinking the DT should contain virtual channel info for CSI-2
buses.

Steve

>

[toc] | [prev] | [next] | [standalone]

#1575545

Hi Steve,

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.

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [next] | [standalone]

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

On Tue, 2017-02-07 at 12:26 +0200, Laurent Pinchart wrote:
> Hi Steve,
> 
> 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.

regards
Philipp

[toc] | [prev] | [next] | [standalone]

#1575761

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

[toc] | [prev] | [next] | [standalone]

#1575749

Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote on Tue [2017-Feb-07 12:26:32 +0200]:
> Hi Steve,
> 
> 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.

Laurent,

So when you have a CSI2 port aggregator for instance where traffic from up
to 4 CSI2 sources where each source is now assigned its own VC by the
aggregator and interleaved into a single CSI2 Receiver. I was hoping that
in this case the VC would be DT discoverable as a specicic source identifier.
So the CSI-RX side could associate a specific source and create its own
video device. I am guessing that no such thing exist today?

Benoit

> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

[toc] | [prev] | [next] | [standalone]

#1576059

Hi Benoit,

On Tue, Feb 07, 2017 at 07:36:48AM -0600, Benoit Parrot wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote on Tue [2017-Feb-07 12:26:32 +0200]:
> > Hi Steve,
> > 
> > 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.
> 
> Laurent,
> 
> So when you have a CSI2 port aggregator for instance where traffic from up
> to 4 CSI2 sources where each source is now assigned its own VC by the
> aggregator and interleaved into a single CSI2 Receiver. I was hoping that
> in this case the VC would be DT discoverable as a specicic source identifier.
> So the CSI-RX side could associate a specific source and create its own
> video device. I am guessing that no such thing exist today?

This should be configurable in software: the sensors connected to the
aggregator may also output multiple streams. The number of streams may also
depend on the user specified configuration over the MC / V4L2 sub-device
interfaces. Thus, this needs to be configurable in software.

We do need additional patches on top of the current mediatree.git master
branch though, some of which are not yet written...

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

[toc] | [prev] | [next] | [standalone]

#1576156

Hi Benoit,

On Tuesday 07 Feb 2017 07:36:48 Benoit Parrot wrote:
> Laurent Pinchart wrote on Tue [2017-Feb-07 12:26:32 +0200]:
> > 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.
> 
> Laurent,
> 
> So when you have a CSI2 port aggregator for instance where traffic from up
> to 4 CSI2 sources where each source is now assigned its own VC by the
> aggregator and interleaved into a single CSI2 Receiver. I was hoping that
> in this case the VC would be DT discoverable as a specicic source
> identifier. So the CSI-RX side could associate a specific source and create
> its own video device.

In this specific example, I believe the aggregator should be modelled with 4 
input ports and one output port in DT, and with 4 sink pads and one source pad 
in MC. Information about the VCs multiplexed over the aggregator's source link 
should not be part of the device tree, but should be discoverable at runtime 
through V4L2 subdev operations. This would include information about how the 4 
input streams are routed to VCs inside the aggregator.

> I am guessing that no such thing exist today?

There's very little (to not say nothing) in terms of VC support in V4L2 today.

-- 
Regards,

Laurent Pinchart

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web