[RFC PATCH 0/3] [media] tvp515p: Proposal for MC input connector support

[RFC PATCH 0/3] [media] tvp515p: Proposal for MC input connector support

[Javier Martinez Canillas]

Hello,

I was waiting for the MC input connector support discussion to settle before
attempting to propose another patch series for the tvp5150 video decoder but
IIUC you are going to continue the discussion at ELC so I'm posting a series
that I believe is aligned with the latest conversations.

This is of course a RFC and not meant to be merged but just to start looking
how the DT binding using OF graph for connectors could look like and to see
an implementation that uses a PAD (and thus link) per electrical signal (the
1:1 model mentioned by Laurent).

The mc_nextgen_test dot output after applying the series can be found at [0]
and the graph png generated using the dot tool at [1].

I've also uploaded the dot files and png when the Composite0 [2,3], Composite1
[4,5] and S-Video [6,7] links are enabled.

I tested these patches on an IGEPv2 by capturing using both Composite inputs,
unfortuantely S-Video using the two RCA connectors is not working, but seems
that is a regression with the tvp5150 driver not related with these patches.

[0]: http://hastebin.com/novoxezeko.tex
[1]: http://i.imgur.com/RWZEpMn.png
[2]: http://hastebin.com/asaduyetuf.tex
[3]: http://i.imgur.com/6y7d7AS.png
[4]: http://hastebin.com/dijowanuki.tex
[5]: http://i.imgur.com/Qr1F9dL.png
[6]: http://hastebin.com/zegiwisoli.tex
[7]: http://i.imgur.com/TdrVJ0R.png

Best regards,
Javier


Javier Martinez Canillas (3):
  [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum
  [media] tvp5150: Add input connectors DT bindings
  [media] tvp5150: Replace connector support according to DT binding

 .../devicetree/bindings/media/i2c/tvp5150.txt      |  59 +++++++
 drivers/media/i2c/tvp5150.c                        | 190 +++++++++++++++------
 include/media/v4l2-mc.h                            |   3 +-
 3 files changed, 203 insertions(+), 49 deletions(-)

-- 
2.5.0

[RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Javier Martinez Canillas]

The enum demod_pad_index list the PADs that an analog TV demod has but
in some decoders the S-Video Y (luminance) and C (chrominance) signals
are carried by different connectors. So a single DEMOD_PAD_IF_INPUT is
not enough and an additional PAD is needed in the case of S-Video for
the additional C signal.

Add a DEMOD_PAD_C_INPUT that can be used for this case and the existing
DEMOD_PAD_IF_INPUT can be used for either Composite or the Y signal.

Suggested-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---
Hello,

This change was suggested by Mauro in [0] although is still not clear
if this is the way forward since changing PAD indexes can break the
uAPI depending on how the PADs are looked up. Another alternative is
to have a PAD type as Mauro mentioned on the same email but since the
series are RFC, I'm making this change as an example and hopping that
the patches can help with the discussion.

[0]: http://www.spinics.net/lists/linux-media/msg98042.html

Best regards,
Javier

 include/media/v4l2-mc.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
index 98a938aabdfb..47c00c288a06 100644
--- a/include/media/v4l2-mc.h
+++ b/include/media/v4l2-mc.h
@@ -94,7 +94,8 @@ enum if_aud_dec_pad_index {
  * @DEMOD_NUM_PADS:	Maximum number of output pads.
  */
 enum demod_pad_index {
-	DEMOD_PAD_IF_INPUT,
+	DEMOD_PAD_IF_INPUT, /* S-Video Y input or Composite */
+	DEMOD_PAD_C_INPUT,  /* S-Video C input or Composite */
 	DEMOD_PAD_VID_OUT,
 	DEMOD_PAD_VBI_OUT,
 	DEMOD_PAD_AUDIO_OUT,
-- 
2.5.0

[RFC PATCH 2/3] [media] tvp5150: Add input connectors DT bindings

[Javier Martinez Canillas]

The tvp5150 and tvp5151 decoders support different video input source
connections to their AIP1A and AIP1B pins. Either two Composite input
signals or a S-Video (with separate Y and C signals) are supported.

The possible configurations are as follows:

- Analog Composite signal connected to AIP1A
- Analog Composite signal connected to AIP1B
- Analog S-Video Y (luminance) and C (chrominance)
  signals connected to AIP1A and AIP1B respectively.

Since the Composite signals shares the same pins than the S-Video ones,
some devices multiplex the physical connectors so the same connector
can be used for either Composite or a S-Video signal (Y or C). But from
the tvp5150 point of view, there are three different "connections", so
the DT binding models the hardware as three separate connectors instead.

Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---
Hello,

There are still some questions about how the pins / video signals are
going to be represented in DT (if represented at all?). In this case
I chose to not describe that information in DT because the driver is
able to figure out from the connectors what links have to be created
and on which PADs.

But I guess that won't be possible for more complex hardware without
having that information in the DT so that should be discussed as well.

There is also the questions about how the connector bindings will be
extended to support other attributes like color/position/group using
the properties API.

Best regards,
Javier

 .../devicetree/bindings/media/i2c/tvp5150.txt      | 59 ++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
index 8c0fc1a26bf0..df555650b0b4 100644
--- a/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
+++ b/Documentation/devicetree/bindings/media/i2c/tvp5150.txt
@@ -26,8 +26,46 @@ Required Endpoint Properties for parallel synchronization:
 If none of hsync-active, vsync-active and field-even-active is specified,
 the endpoint is assumed to use embedded BT.656 synchronization.
 
+-Optional nodes:
+- connectors: The list of tvp5150 input connectors available on a given
+  board. The node should contain a child 'port' node for each connector.
+
+  The tvp5150 has support for three possible connectors: 2 Composite and
+  1 S-video. The "reg" property is used to specify which input connector
+  is associated with each 'port', using the following possible values:
+
+  0: Composite0
+  1: Composite1
+  2: S-Video
+
+  The ports should have an endpoint subnode that is linked to a connector
+  node defined in Documentation/devicetree/bindings/display/connector/.
+  The linked connector compatible string should match the connector type.
+
 Example:
 
+composite0: connector@0 {
+	compatible = "composite-video-connector";
+	label = "Composite0";
+
+	port {
+		comp0_out: endpoint {
+			remote-endpoint = <&tvp5150_comp0_in>;
+		};
+	};
+};
+
+svideo: connector@1 {
+	compatible = "composite-video-connector";
+	label = "S-Video";
+
+	port {
+		svideo_out: endpoint {
+			remote-endpoint = <&tvp5150_svideo_in>;
+		};
+	};
+};
+
 &i2c2 {
 	...
 	tvp5150@5c {
@@ -36,6 +74,27 @@ Example:
 		pdn-gpios = <&gpio4 30 GPIO_ACTIVE_LOW>;
 		reset-gpios = <&gpio6 7 GPIO_ACTIVE_LOW>;
 
+		connectors {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			/* Composite0 input */
+			port@0 {
+				reg = <0>;
+				tvp5150_comp0_in: endpoint {
+					remote-endpoint = <&comp0_out>;
+				};
+			};
+
+			/* S-Video input */
+			port@2 {
+				reg = <2>;
+				tvp5150_svideo_in: endpoint {
+					remote-endpoint = <&svideo_out>;
+				};
+			};
+		};
+
 		port {
 			tvp5150_1: endpoint {
 				remote-endpoint = <&ccdc_ep>;
-- 
2.5.0

[RFC PATCH 3/3] [media] tvp5150: Replace connector support according to DT binding

[Javier Martinez Canillas]

The tvp5150 and tvp5151 decoders support different video input source
connections to their AIP1A and AIP1B pins. Either two Composite input
signals or a S-Video signal are supported.

The MC input connector support was added before, but the Device Tree
binding was found to be inadecuate so it was reverted. A new binding
documentation was added so this patch replaces the old logic with a
new one according to what is described in the latest DT binding doc.

There could be different connectors for each possible configuration
or physical connectors (i.e: two RCA) can be multiplexed to support
both the two Composite and a single S-Video input.

The DT binding only describes the type of the input connection and not
how the signals are routed to the chip, that information is inferred
by the driver that knows what are the possible input configurations.

Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>

---

 drivers/media/i2c/tvp5150.c | 190 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 142 insertions(+), 48 deletions(-)

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index ff18444e19e4..c6ea5581b7c7 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -41,7 +41,7 @@ struct tvp5150 {
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_pad pads[DEMOD_NUM_PADS];
 	struct media_entity input_ent[TVP5150_INPUT_NUM];
-	struct media_pad input_pad[TVP5150_INPUT_NUM];
+	struct media_pad input_pad[TVP5150_INPUT_NUM + 1]; /* 2 for S-Video */
 #endif
 	struct v4l2_ctrl_handler hdl;
 	struct v4l2_rect rect;
@@ -1176,6 +1176,8 @@ static int tvp5150_registered_async(struct v4l2_subdev *sd)
 {
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct tvp5150 *decoder = to_tvp5150(sd);
+	int num_pads;
+	u16 sink_pad;
 	int ret = 0;
 	int i;
 
@@ -1186,9 +1188,25 @@ static int tvp5150_registered_async(struct v4l2_subdev *sd)
 		if (!input->name)
 			continue;
 
+		switch (i) {
+		case TVP5150_COMPOSITE0:
+			sink_pad = DEMOD_PAD_IF_INPUT;
+			num_pads = 1;
+			break;
+		case TVP5150_COMPOSITE1:
+			sink_pad = DEMOD_PAD_C_INPUT;
+			num_pads = 1;
+			break;
+		case TVP5150_SVIDEO:
+			sink_pad = DEMOD_PAD_IF_INPUT;
+			num_pads = 2;
+			decoder->input_pad[i + 1].flags = MEDIA_PAD_FL_SOURCE;
+			break;
+		}
+
 		decoder->input_pad[i].flags = MEDIA_PAD_FL_SOURCE;
 
-		ret = media_entity_pads_init(input, 1, pad);
+		ret = media_entity_pads_init(input, num_pads, pad);
 		if (ret < 0)
 			return ret;
 
@@ -1196,12 +1214,20 @@ static int tvp5150_registered_async(struct v4l2_subdev *sd)
 		if (ret < 0)
 			return ret;
 
-		ret = media_create_pad_link(input, 0, &sd->entity,
-					    DEMOD_PAD_IF_INPUT, 0);
+		ret = media_create_pad_link(input, 0, &sd->entity, sink_pad, 0);
 		if (ret < 0) {
 			media_device_unregister_entity(input);
 			return ret;
 		}
+
+		if (input->function == MEDIA_ENT_F_CONN_SVIDEO) {
+			ret = media_create_pad_link(input, 1, &sd->entity,
+						    DEMOD_PAD_C_INPUT, 0);
+			if (ret < 0) {
+				media_device_unregister_entity(input);
+				return ret;
+			}
+		}
 	}
 #endif
 
@@ -1338,15 +1364,112 @@ static int tvp5150_init(struct i2c_client *c)
 	return 0;
 }
 
+#ifdef CONFIG_MEDIA_CONTROLLER
+static int tvp5150_parse_connector_node(struct tvp5150 *decoder,
+					struct device_node *port)
+{
+	struct v4l2_of_endpoint endpoint;
+	struct media_entity *input;
+	struct device_node *ep, *rem;
+	unsigned int input_type;
+	const char *name;
+	int ret;
+
+	/* tvp5150 connector ports can have only one endpoint. */
+	ep = of_get_next_child(port, NULL);
+	if (!ep) {
+		v4l2_err(&decoder->sd, "Endpoint not found for connector %s\n",
+			 port->full_name);
+		return -EINVAL;
+	}
+
+	ret = v4l2_of_parse_endpoint(ep, &endpoint);
+	if (ret) {
+		v4l2_err(&decoder->sd, "Connector %s parse endpoint failed %d\n",
+			 port->full_name, ret);
+		goto err_ep;
+	}
+
+	input_type = endpoint.base.port;
+
+	if (input_type >= TVP5150_INPUT_NUM) {
+		v4l2_err(&decoder->sd,
+			 "Connector %s port address %u is not a valid one\n",
+			 port->full_name, input_type);
+		ret = -EINVAL;
+		goto err_ep;
+	}
+
+	input = &decoder->input_ent[input_type];
+
+	/* Each input connector can only be defined once */
+	if (input->name) {
+		v4l2_err(&decoder->sd,
+			 "Connector %s with same type already exists\n",
+			 input->name);
+		ret = -EINVAL;
+		goto err_ep;
+	}
+
+	rem = of_graph_get_remote_port_parent(ep);
+	if (!rem) {
+		v4l2_err(&decoder->sd, "Port %s remote parent not found\n",
+			 ep->full_name);
+		ret = -EINVAL;
+		goto err_ep;
+	}
+
+	switch (input_type) {
+	case TVP5150_COMPOSITE0:
+	case TVP5150_COMPOSITE1:
+		if (!of_device_is_compatible(rem, "composite-video-connector")) {
+			v4l2_err(&decoder->sd, "Wrong compatible for port %s\n",
+				 rem->full_name);
+			ret = -EINVAL;
+			goto err_rem;
+		}
+
+		input->function = MEDIA_ENT_F_CONN_COMPOSITE;
+		break;
+	case TVP5150_SVIDEO:
+		if (!of_device_is_compatible(rem, "svideo-connector")) {
+			v4l2_err(&decoder->sd, "Wrong compatible for port %s\n",
+				 rem->full_name);
+			ret = -EINVAL;
+			goto err_rem;
+		}
+
+		input->function = MEDIA_ENT_F_CONN_SVIDEO;
+		break;
+	}
+
+	input->flags = MEDIA_ENT_FL_CONNECTOR;
+
+	ret = of_property_read_string(rem, "label", &name);
+	if (ret < 0) {
+		v4l2_err(&decoder->sd,
+			 "Missing label property in port %s\n",
+			 rem->full_name);
+		goto err_rem;
+	}
+
+	input->name = name;
+
+err_rem:
+	of_node_put(rem);
+err_ep:
+	of_node_put(ep);
+	return ret;
+
+}
+#endif
+
 static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
 {
 	struct v4l2_of_endpoint bus_cfg;
 	struct device_node *ep;
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct device_node *connectors, *child;
-	struct media_entity *input;
-	const char *name;
-	u32 input_type;
 #endif
 	unsigned int flags;
 	int ret = 0;
@@ -1377,52 +1500,22 @@ static int tvp5150_parse_dt(struct tvp5150 *decoder, struct device_node *np)
 	if (!connectors)
 		goto err;
 
+	if (of_get_child_count(connectors) > TVP5150_INPUT_NUM) {
+		v4l2_err(&decoder->sd,
+			 "Maximum number of connectors is %d\n",
+			 TVP5150_INPUT_NUM);
+		ret = -EINVAL;
+		goto err_connector;
+	}
+
 	for_each_available_child_of_node(connectors, child) {
-		ret = of_property_read_u32(child, "input", &input_type);
+		ret = tvp5150_parse_connector_node(decoder, child);
 		if (ret) {
 			v4l2_err(&decoder->sd,
-				 "missing type property in node %s\n",
-				 child->name);
-			goto err_connector;
-		}
-
-		if (input_type >= TVP5150_INPUT_NUM) {
-			ret = -EINVAL;
+				 "Connector %s parse failed\n", child->name);
+			of_node_put(child);
 			goto err_connector;
 		}
-
-		input = &decoder->input_ent[input_type];
-
-		/* Each input connector can only be defined once */
-		if (input->name) {
-			v4l2_err(&decoder->sd,
-				 "input %s with same type already exists\n",
-				 input->name);
-			ret = -EINVAL;
-			goto err_connector;
-		}
-
-		switch (input_type) {
-		case TVP5150_COMPOSITE0:
-		case TVP5150_COMPOSITE1:
-			input->function = MEDIA_ENT_F_CONN_COMPOSITE;
-			break;
-		case TVP5150_SVIDEO:
-			input->function = MEDIA_ENT_F_CONN_SVIDEO;
-			break;
-		}
-
-		input->flags = MEDIA_ENT_FL_CONNECTOR;
-
-		ret = of_property_read_string(child, "label", &name);
-		if (ret < 0) {
-			v4l2_err(&decoder->sd,
-				 "missing label property in node %s\n",
-				 child->name);
-			goto err_connector;
-		}
-
-		input->name = name;
 	}
 
 err_connector:
@@ -1477,6 +1570,7 @@ static int tvp5150_probe(struct i2c_client *c,
 
 #if defined(CONFIG_MEDIA_CONTROLLER)
 	core->pads[DEMOD_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
+	core->pads[DEMOD_PAD_C_INPUT].flags = MEDIA_PAD_FL_SINK;
 	core->pads[DEMOD_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
 	core->pads[DEMOD_PAD_VBI_OUT].flags = MEDIA_PAD_FL_SOURCE;
 
-- 
2.5.0

Re: [RFC PATCH 0/3] [media] tvp515p: Proposal for MC input connector support

[Javier Martinez Canillas]

Hello,

On 03/09/2016 04:09 PM, Javier Martinez Canillas wrote:
> 
> I was waiting for the MC input connector support discussion to settle before
> attempting to propose another patch series for the tvp5150 video decoder but
> IIUC you are going to continue the discussion at ELC so I'm posting a series
> that I believe is aligned with the latest conversations.
> 
> This is of course a RFC and not meant to be merged but just to start looking
> how the DT binding using OF graph for connectors could look like and to see
> an implementation that uses a PAD (and thus link) per electrical signal (the
> 1:1 model mentioned by Laurent).
>

Any comments about this series?
 
Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Hans Verkuil]

On 03/09/2016 08:09 PM, Javier Martinez Canillas wrote:
> The enum demod_pad_index list the PADs that an analog TV demod has but
> in some decoders the S-Video Y (luminance) and C (chrominance) signals
> are carried by different connectors. So a single DEMOD_PAD_IF_INPUT is
> not enough and an additional PAD is needed in the case of S-Video for
> the additional C signal.
> 
> Add a DEMOD_PAD_C_INPUT that can be used for this case and the existing
> DEMOD_PAD_IF_INPUT can be used for either Composite or the Y signal.
> 
> Suggested-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> 
> ---
> Hello,
> 
> This change was suggested by Mauro in [0] although is still not clear
> if this is the way forward since changing PAD indexes can break the
> uAPI depending on how the PADs are looked up. Another alternative is
> to have a PAD type as Mauro mentioned on the same email but since the
> series are RFC, I'm making this change as an example and hopping that
> the patches can help with the discussion.
> 
> [0]: http://www.spinics.net/lists/linux-media/msg98042.html
> 
> Best regards,
> Javier
> 
>  include/media/v4l2-mc.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
> index 98a938aabdfb..47c00c288a06 100644
> --- a/include/media/v4l2-mc.h
> +++ b/include/media/v4l2-mc.h
> @@ -94,7 +94,8 @@ enum if_aud_dec_pad_index {
>   * @DEMOD_NUM_PADS:	Maximum number of output pads.
>   */
>  enum demod_pad_index {
> -	DEMOD_PAD_IF_INPUT,
> +	DEMOD_PAD_IF_INPUT, /* S-Video Y input or Composite */
> +	DEMOD_PAD_C_INPUT,  /* S-Video C input or Composite */
>  	DEMOD_PAD_VID_OUT,
>  	DEMOD_PAD_VBI_OUT,
>  	DEMOD_PAD_AUDIO_OUT,
> 

These pad index enums are butt ugly and won't scale in the long run. An entity
should have just as many pads as it needs and no more.

If you want to have an heuristic so you can find which pad carries e.g.
composite video, then it is better to ask the subdev for that.

E.g.: err = v4l2_subdev_call(sd, pad, g_signal_pad, V4L2_PAD_Y_SIG_INPUT, &pad)

The subdev driver knows which pad has which signal, so this does not rely on
hardcoding all combinations of possible pad types and you can still heuristically
build a media graph for legacy drivers.

What we do now is reminiscent of the bad old days when the input numbers (as
returned by ENUMINPUT) where mapped to the i2c routing (basically pads). I worked
hard to get rid of that hardcoded relationship and I don't like to see it coming
back.

Actually, I am not sure if a new subdev op will work at all. This information
really belongs to the device tree or card info. Just as it is done today with
the audio and video s_routing op. The op basically sets up the routing (i.e.
pads). We didn't have pads (or an MC) when I made that op, but the purpose
is the same.

Although I guess that a g_signal_pad might be enough in many cases. I don't
know what is the best solution, to be honest. All I know is that the current
approach will end in tears.

Hmm, looking at saa7134-cards you have lists of inputs. You could add a pad_type
field there and use the g_signal_pad to obtain the corresponding pad of the
subdev entity. You'd have pad types TV, COMPOSITE[1-N], SVIDEO[1-N], etc.

Note that input 1 could map to pad type COMPOSITE3 since that all depends on
how the board is wired up.

But at least this approach doesn't force subdevs to have more pads than they
need.

Regards,

	Hans

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Hans Verkuil]

On 03/18/2016 04:45 PM, Hans Verkuil wrote:
> On 03/09/2016 08:09 PM, Javier Martinez Canillas wrote:
>> The enum demod_pad_index list the PADs that an analog TV demod has but
>> in some decoders the S-Video Y (luminance) and C (chrominance) signals
>> are carried by different connectors. So a single DEMOD_PAD_IF_INPUT is
>> not enough and an additional PAD is needed in the case of S-Video for
>> the additional C signal.
>>
>> Add a DEMOD_PAD_C_INPUT that can be used for this case and the existing
>> DEMOD_PAD_IF_INPUT can be used for either Composite or the Y signal.
>>
>> Suggested-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>
>> ---
>> Hello,
>>
>> This change was suggested by Mauro in [0] although is still not clear
>> if this is the way forward since changing PAD indexes can break the
>> uAPI depending on how the PADs are looked up. Another alternative is
>> to have a PAD type as Mauro mentioned on the same email but since the
>> series are RFC, I'm making this change as an example and hopping that
>> the patches can help with the discussion.
>>
>> [0]: http://www.spinics.net/lists/linux-media/msg98042.html
>>
>> Best regards,
>> Javier
>>
>>  include/media/v4l2-mc.h | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
>> index 98a938aabdfb..47c00c288a06 100644
>> --- a/include/media/v4l2-mc.h
>> +++ b/include/media/v4l2-mc.h
>> @@ -94,7 +94,8 @@ enum if_aud_dec_pad_index {
>>   * @DEMOD_NUM_PADS:	Maximum number of output pads.
>>   */
>>  enum demod_pad_index {
>> -	DEMOD_PAD_IF_INPUT,
>> +	DEMOD_PAD_IF_INPUT, /* S-Video Y input or Composite */
>> +	DEMOD_PAD_C_INPUT,  /* S-Video C input or Composite */
>>  	DEMOD_PAD_VID_OUT,
>>  	DEMOD_PAD_VBI_OUT,
>>  	DEMOD_PAD_AUDIO_OUT,
>>
> 
> These pad index enums are butt ugly and won't scale in the long run. An entity
> should have just as many pads as it needs and no more.
> 
> If you want to have an heuristic so you can find which pad carries e.g.
> composite video, then it is better to ask the subdev for that.
> 
> E.g.: err = v4l2_subdev_call(sd, pad, g_signal_pad, V4L2_PAD_Y_SIG_INPUT, &pad)
> 
> The subdev driver knows which pad has which signal, so this does not rely on
> hardcoding all combinations of possible pad types and you can still heuristically
> build a media graph for legacy drivers.
> 
> What we do now is reminiscent of the bad old days when the input numbers (as
> returned by ENUMINPUT) where mapped to the i2c routing (basically pads). I worked
> hard to get rid of that hardcoded relationship and I don't like to see it coming
> back.
> 
> Actually, I am not sure if a new subdev op will work at all. This information
> really belongs to the device tree or card info. Just as it is done today with
> the audio and video s_routing op. The op basically sets up the routing (i.e.
> pads). We didn't have pads (or an MC) when I made that op, but the purpose
> is the same.
> 
> Although I guess that a g_signal_pad might be enough in many cases. I don't
> know what is the best solution, to be honest. All I know is that the current
> approach will end in tears.
> 
> Hmm, looking at saa7134-cards you have lists of inputs. You could add a pad_type
> field there and use the g_signal_pad to obtain the corresponding pad of the
> subdev entity. You'd have pad types TV, COMPOSITE[1-N], SVIDEO[1-N], etc.
> 
> Note that input 1 could map to pad type COMPOSITE3 since that all depends on
> how the board is wired up.
> 
> But at least this approach doesn't force subdevs to have more pads than they
> need.

Actually, there is really no need for these 'pad types'. Why not just put the
actual pads in the card info? You know that anyway since you have to configure
the routing. So just stick it in the board info structs.

Why this rush to get all this in? Can we at least disable the media device
creation for these usb and pci devices until we are sure we got all the details
right? As long as we don't register the media device these pads aren't used and
we can still make changes.

Let's be honest: nobody is waiting for media devices for these chipsets. We want
it because we want to be consistent and it is great for prototyping, but other
than us no one cares.

This stuff is really hard to get right, and I feel these things are exposed too
soon. And once it is part of the public API it is very hard to revert.

Regards,

	Hans

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Mauro Carvalho Chehab]

Em Fri, 18 Mar 2016 18:33:39 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 03/18/2016 04:45 PM, Hans Verkuil wrote:
> > On 03/09/2016 08:09 PM, Javier Martinez Canillas wrote:  
> >> The enum demod_pad_index list the PADs that an analog TV demod has but
> >> in some decoders the S-Video Y (luminance) and C (chrominance) signals
> >> are carried by different connectors. So a single DEMOD_PAD_IF_INPUT is
> >> not enough and an additional PAD is needed in the case of S-Video for
> >> the additional C signal.
> >>
> >> Add a DEMOD_PAD_C_INPUT that can be used for this case and the existing
> >> DEMOD_PAD_IF_INPUT can be used for either Composite or the Y signal.
> >>
> >> Suggested-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> >> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
> >>
> >> ---
> >> Hello,
> >>
> >> This change was suggested by Mauro in [0] although is still not clear
> >> if this is the way forward since changing PAD indexes can break the
> >> uAPI depending on how the PADs are looked up. Another alternative is
> >> to have a PAD type as Mauro mentioned on the same email but since the
> >> series are RFC, I'm making this change as an example and hopping that
> >> the patches can help with the discussion.
> >>
> >> [0]: http://www.spinics.net/lists/linux-media/msg98042.html
> >>
> >> Best regards,
> >> Javier
> >>
> >>  include/media/v4l2-mc.h | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
> >> index 98a938aabdfb..47c00c288a06 100644
> >> --- a/include/media/v4l2-mc.h
> >> +++ b/include/media/v4l2-mc.h
> >> @@ -94,7 +94,8 @@ enum if_aud_dec_pad_index {
> >>   * @DEMOD_NUM_PADS:	Maximum number of output pads.
> >>   */
> >>  enum demod_pad_index {
> >> -	DEMOD_PAD_IF_INPUT,
> >> +	DEMOD_PAD_IF_INPUT, /* S-Video Y input or Composite */
> >> +	DEMOD_PAD_C_INPUT,  /* S-Video C input or Composite */
> >>  	DEMOD_PAD_VID_OUT,
> >>  	DEMOD_PAD_VBI_OUT,
> >>  	DEMOD_PAD_AUDIO_OUT,
> >>  
> > 
> > These pad index enums are butt ugly and won't scale in the long run. An entity
> > should have just as many pads as it needs and no more.
> > 
> > If you want to have an heuristic so you can find which pad carries e.g.
> > composite video, then it is better to ask the subdev for that.
> > 
> > E.g.: err = v4l2_subdev_call(sd, pad, g_signal_pad, V4L2_PAD_Y_SIG_INPUT, &pad)
> > 
> > The subdev driver knows which pad has which signal, so this does not rely on
> > hardcoding all combinations of possible pad types and you can still heuristically
> > build a media graph for legacy drivers.

Yes, accessing PADs via a hardcoded index is butt ugly.

For sure, we need a better strategy than that. This is one of the things
we need to discuss at the media summit.

> > What we do now is reminiscent of the bad old days when the input numbers (as
> > returned by ENUMINPUT) where mapped to the i2c routing (basically pads). I worked
> > hard to get rid of that hardcoded relationship and I don't like to see it coming
> > back.

No, this is completely unrelated with ENUMINPUT. 

With VIDIOC_*INPUT ioctls, a hardcoded list of inputs can happen only at
the Kernel side, as, userspace should not rely on the input index, but,
instead, should call VIDIOC_ENUMINPUT.

However, the media controller currently lacks an "ENUMPADS" ioctl that
would tell userspace what kind of data each PAD contains. Due to that,
on entities with more than one sink pad and/or more than one source
pad, the application should rely on the PAD index.

That also reflects on the Kernel side, that forces drivers to do
things like:

	struct tvp5150 *core = to_tvp5150(sd);
	int res;

	core->pads[DEMOD_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
	core->pads[DEMOD_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
	core->pads[DEMOD_PAD_VBI_OUT].flags = MEDIA_PAD_FL_SOURCE;

	res = media_entity_pads_init(&sd->entity, DEMOD_NUM_PADS, core->pads);

hardcoding the PAD indexes.

The enums that are right now at v4l2-mc.h just prevents the mess to
spread all over the drivers, while we don't have a better solution, as
at least it will prevent two different devices with the very same type
to have a completely different set of PADs, with would cause lots of
pain on drivers that work with a multiple set of entities of the same
type.

Please notice that this is not a problem with connectors. It is a
broader problem at the MC infra and API, with affects all subdevs.

> > Actually, I am not sure if a new subdev op will work at all. This information
> > really belongs to the device tree or card info. Just as it is done today with
> > the audio and video s_routing op. The op basically sets up the routing (i.e.
> > pads). We didn't have pads (or an MC) when I made that op, but the purpose
> > is the same.

I didn't think yet on the implementation, but it should be something that the
core should be able to get, if we want to avoid having to add a huge block
of MC-specific routines on each driver, with a really complex logic.

The problem is that the routines that builds the V4L2 graph and
enables/disables the PADs should know what PADs to use.

A subdev ops seem to be a good way for doing that, as it is the subdev
that will create the pads when running its .probe() function.

If we move the media-controller specific initialization out of .probe(), like
what I proposed at:
	https://patchwork.linuxtv.org/patch/33466/

Then, we can let the caller driver to tell the subdev-specific PAD init
code to create only the PADs that the driver will be using.

For example, on em28xx driver, devices with certain chips in the family
(like em2820 and em28xx) don't support VBI. So, the driver could ask
the demod to not create a VBI out PAD, creating only 2 pads.

Yet, it sounds painful to create a generic media_init() callback that
would allow to control the number and the meaning of the PADs that
would cover all cases.

> > Although I guess that a g_signal_pad might be enough in many cases. I don't
> > know what is the best solution, to be honest. All I know is that the current
> > approach will end in tears.
> > 
> > Hmm, looking at saa7134-cards you have lists of inputs. You could add a pad_type
> > field there and use the g_signal_pad to obtain the corresponding pad of the
> > subdev entity. You'd have pad types TV, COMPOSITE[1-N], SVIDEO[1-N], etc.
> > 
> > Note that input 1 could map to pad type COMPOSITE3 since that all depends on
> > how the board is wired up.
> > 
> > But at least this approach doesn't force subdevs to have more pads than they
> > need.  
> 
> Actually, there is really no need for these 'pad types'. Why not just put the
> actual pads in the card info? You know that anyway since you have to configure
> the routing. So just stick it in the board info structs.

If we hardcode the PAD indexes at the board info, that would mean that
we can't have a generic core routine to create the graph or enable/disable
the sources. We should get rid of hardcoding it, and not adding more
hardcoded values.

> Why this rush to get all this in? Can we at least disable the media device
> creation for these usb and pci devices until we are sure we got all the details
> right? As long as we don't register the media device these pads aren't used and
> we can still make changes.
> 
> Let's be honest: nobody is waiting for media devices for these chipsets. We want
> it because we want to be consistent and it is great for prototyping, but other
> than us no one cares.

No, we want this for two reasons:

1) To fix the locking troubles with analog, digital and audio parts of
the driver;

2) To report userspace apps what devnodes belong to each devices.

Both are longstanding bugs. (2) is partially solved via libmedia_dev, with
was actually added inside tvtime and xawtv, as we never agreed with the
library's API.

> This stuff is really hard to get right, and I feel these things are exposed too
> soon. And once it is part of the public API it is very hard to revert.

Well, this problem is there already, as userspace relies on
hardcoded index values since when the media controller got merged.

What we did with connectors was actually your suggestion: we removed
the definition of the connectors from the uAPI, with should give
us flexibility to change it as needed.

We could, instead, not be calling media_devnode_register() at the
new drivers and at PCI/USB drivers until we fix it, but we'll
still have to address backward compat with the legacy drivers.

Regards,
Mauro

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Hans Verkuil]

On 03/21/2016 03:40 PM, Mauro Carvalho Chehab wrote:
> Em Fri, 18 Mar 2016 18:33:39 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 03/18/2016 04:45 PM, Hans Verkuil wrote:
>>> On 03/09/2016 08:09 PM, Javier Martinez Canillas wrote:  
>>>> The enum demod_pad_index list the PADs that an analog TV demod has but
>>>> in some decoders the S-Video Y (luminance) and C (chrominance) signals
>>>> are carried by different connectors. So a single DEMOD_PAD_IF_INPUT is
>>>> not enough and an additional PAD is needed in the case of S-Video for
>>>> the additional C signal.
>>>>
>>>> Add a DEMOD_PAD_C_INPUT that can be used for this case and the existing
>>>> DEMOD_PAD_IF_INPUT can be used for either Composite or the Y signal.
>>>>
>>>> Suggested-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>>>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>>>
>>>> ---
>>>> Hello,
>>>>
>>>> This change was suggested by Mauro in [0] although is still not clear
>>>> if this is the way forward since changing PAD indexes can break the
>>>> uAPI depending on how the PADs are looked up. Another alternative is
>>>> to have a PAD type as Mauro mentioned on the same email but since the
>>>> series are RFC, I'm making this change as an example and hopping that
>>>> the patches can help with the discussion.
>>>>
>>>> [0]: http://www.spinics.net/lists/linux-media/msg98042.html
>>>>
>>>> Best regards,
>>>> Javier
>>>>
>>>>  include/media/v4l2-mc.h | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
>>>> index 98a938aabdfb..47c00c288a06 100644
>>>> --- a/include/media/v4l2-mc.h
>>>> +++ b/include/media/v4l2-mc.h
>>>> @@ -94,7 +94,8 @@ enum if_aud_dec_pad_index {
>>>>   * @DEMOD_NUM_PADS:	Maximum number of output pads.
>>>>   */
>>>>  enum demod_pad_index {
>>>> -	DEMOD_PAD_IF_INPUT,
>>>> +	DEMOD_PAD_IF_INPUT, /* S-Video Y input or Composite */
>>>> +	DEMOD_PAD_C_INPUT,  /* S-Video C input or Composite */
>>>>  	DEMOD_PAD_VID_OUT,
>>>>  	DEMOD_PAD_VBI_OUT,
>>>>  	DEMOD_PAD_AUDIO_OUT,
>>>>  
>>>
>>> These pad index enums are butt ugly and won't scale in the long run. An entity
>>> should have just as many pads as it needs and no more.
>>>
>>> If you want to have an heuristic so you can find which pad carries e.g.
>>> composite video, then it is better to ask the subdev for that.
>>>
>>> E.g.: err = v4l2_subdev_call(sd, pad, g_signal_pad, V4L2_PAD_Y_SIG_INPUT, &pad)
>>>
>>> The subdev driver knows which pad has which signal, so this does not rely on
>>> hardcoding all combinations of possible pad types and you can still heuristically
>>> build a media graph for legacy drivers.
> 
> Yes, accessing PADs via a hardcoded index is butt ugly.
> 
> For sure, we need a better strategy than that. This is one of the things
> we need to discuss at the media summit.
> 
>>> What we do now is reminiscent of the bad old days when the input numbers (as
>>> returned by ENUMINPUT) where mapped to the i2c routing (basically pads). I worked
>>> hard to get rid of that hardcoded relationship and I don't like to see it coming
>>> back.
> 
> No, this is completely unrelated with ENUMINPUT. 
> 
> With VIDIOC_*INPUT ioctls, a hardcoded list of inputs can happen only at
> the Kernel side, as, userspace should not rely on the input index, but,
> instead, should call VIDIOC_ENUMINPUT.
> 
> However, the media controller currently lacks an "ENUMPADS" ioctl that
> would tell userspace what kind of data each PAD contains. Due to that,
> on entities with more than one sink pad and/or more than one source
> pad, the application should rely on the PAD index.
> 
> That also reflects on the Kernel side, that forces drivers to do
> things like:
> 
> 	struct tvp5150 *core = to_tvp5150(sd);
> 	int res;
> 
> 	core->pads[DEMOD_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
> 	core->pads[DEMOD_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
> 	core->pads[DEMOD_PAD_VBI_OUT].flags = MEDIA_PAD_FL_SOURCE;
> 
> 	res = media_entity_pads_init(&sd->entity, DEMOD_NUM_PADS, core->pads);
> 
> hardcoding the PAD indexes.
> 
> The enums that are right now at v4l2-mc.h just prevents the mess to
> spread all over the drivers, while we don't have a better solution, as
> at least it will prevent two different devices with the very same type
> to have a completely different set of PADs, with would cause lots of
> pain on drivers that work with a multiple set of entities of the same
> type.

This is already device specific. The video and audio s_routing ops are there
precisely because the routing between devices is board specific. It links
entities with each other the way we had to before we had the media controller.

Subdev entities should *not* use these fake pads. It's going to be a nightmare.

A reasonable solution to simplify converting legacy drivers without creating
these global ugly pad indices is to add a new video (and probably audio) op
'g_pad_of_type(type)' where you ask the subdev entity to return which pad carries
signals of a certain type.

> Please notice that this is not a problem with connectors. It is a
> broader problem at the MC infra and API, with affects all subdevs.
> 
>>> Actually, I am not sure if a new subdev op will work at all. This information
>>> really belongs to the device tree or card info. Just as it is done today with
>>> the audio and video s_routing op. The op basically sets up the routing (i.e.
>>> pads). We didn't have pads (or an MC) when I made that op, but the purpose
>>> is the same.
> 
> I didn't think yet on the implementation, but it should be something that the
> core should be able to get, if we want to avoid having to add a huge block
> of MC-specific routines on each driver, with a really complex logic.
> 
> The problem is that the routines that builds the V4L2 graph and
> enables/disables the PADs should know what PADs to use.
> 
> A subdev ops seem to be a good way for doing that, as it is the subdev
> that will create the pads when running its .probe() function.
> 
> If we move the media-controller specific initialization out of .probe(), like
> what I proposed at:
> 	https://patchwork.linuxtv.org/patch/33466/
> 
> Then, we can let the caller driver to tell the subdev-specific PAD init
> code to create only the PADs that the driver will be using.
> 
> For example, on em28xx driver, devices with certain chips in the family
> (like em2820 and em28xx) don't support VBI. So, the driver could ask
> the demod to not create a VBI out PAD, creating only 2 pads.

No, subdev drivers should create the number of pads that corresponds to the
hardware. I think it makes life very difficult if you make this dynamic based
on what someone else wants.

What you want is to ask the subdev driver which pad carries a certain signal.
That can be done with a g_pad_of_type()-like op.

> 
> Yet, it sounds painful to create a generic media_init() callback that
> would allow to control the number and the meaning of the PADs that
> would cover all cases.
> 
>>> Although I guess that a g_signal_pad might be enough in many cases. I don't
>>> know what is the best solution, to be honest. All I know is that the current
>>> approach will end in tears.
>>>
>>> Hmm, looking at saa7134-cards you have lists of inputs. You could add a pad_type
>>> field there and use the g_signal_pad to obtain the corresponding pad of the
>>> subdev entity. You'd have pad types TV, COMPOSITE[1-N], SVIDEO[1-N], etc.
>>>
>>> Note that input 1 could map to pad type COMPOSITE3 since that all depends on
>>> how the board is wired up.
>>>
>>> But at least this approach doesn't force subdevs to have more pads than they
>>> need.  
>>
>> Actually, there is really no need for these 'pad types'. Why not just put the
>> actual pads in the card info? You know that anyway since you have to configure
>> the routing. So just stick it in the board info structs.
> 
> If we hardcode the PAD indexes at the board info, that would mean that
> we can't have a generic core routine to create the graph or enable/disable
> the sources. We should get rid of hardcoding it, and not adding more
> hardcoded values.

You can create something generic that covers 90%, but forget about 100%. Hardware
designer are crazy and you can never to everything automatic. So hardcoding will
always be part of life.

Try this with a complex driver like ivtv (or anything using saa7115 and/or msp34xx).

Drivers currently have to use s_routing with board specific arguments to set these
up, so those are nice difficult test cases.

>> Why this rush to get all this in? Can we at least disable the media device
>> creation for these usb and pci devices until we are sure we got all the details
>> right? As long as we don't register the media device these pads aren't used and
>> we can still make changes.
>>
>> Let's be honest: nobody is waiting for media devices for these chipsets. We want
>> it because we want to be consistent and it is great for prototyping, but other
>> than us no one cares.
> 
> No, we want this for two reasons:
> 
> 1) To fix the locking troubles with analog, digital and audio parts of
> the driver;
> 
> 2) To report userspace apps what devnodes belong to each devices.
> 
> Both are longstanding bugs. (2) is partially solved via libmedia_dev, with
> was actually added inside tvtime and xawtv, as we never agreed with the
> library's API.

I agree that the MC is the right place to fix it, and I really, really want it
too. But it has to be done right, and using DEMOD_PAD_* is really wrong.

> 
>> This stuff is really hard to get right, and I feel these things are exposed too
>> soon. And once it is part of the public API it is very hard to revert.
> 
> Well, this problem is there already, as userspace relies on
> hardcoded index values since when the media controller got merged.

Yes, but those index values map to real hardware properties. That's not the case
with these DEMOD_PAD_ indices. Those have no relationship with real hardware pins.

> What we did with connectors was actually your suggestion: we removed
> the definition of the connectors from the uAPI, with should give
> us flexibility to change it as needed.
> 
> We could, instead, not be calling media_devnode_register() at the
> new drivers and at PCI/USB drivers until we fix it, but we'll
> still have to address backward compat with the legacy drivers.

Sorry, I don't understand which 'backward compat with the legacy drivers' you
mean. That existing drivers like omap3 use fixed pads? That's actually the
way it should be since pads should map to hardware. Or do you mean something
else?

Regards,

	Hans

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Hans Verkuil]

On 03/21/2016 04:05 PM, Hans Verkuil wrote:
> On 03/21/2016 03:40 PM, Mauro Carvalho Chehab wrote:
>> Em Fri, 18 Mar 2016 18:33:39 +0100
>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>
>>> On 03/18/2016 04:45 PM, Hans Verkuil wrote:
>>>> On 03/09/2016 08:09 PM, Javier Martinez Canillas wrote:  
>>>>> The enum demod_pad_index list the PADs that an analog TV demod has but
>>>>> in some decoders the S-Video Y (luminance) and C (chrominance) signals
>>>>> are carried by different connectors. So a single DEMOD_PAD_IF_INPUT is
>>>>> not enough and an additional PAD is needed in the case of S-Video for
>>>>> the additional C signal.
>>>>>
>>>>> Add a DEMOD_PAD_C_INPUT that can be used for this case and the existing
>>>>> DEMOD_PAD_IF_INPUT can be used for either Composite or the Y signal.
>>>>>
>>>>> Suggested-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>>>>> Signed-off-by: Javier Martinez Canillas <javier@osg.samsung.com>
>>>>>
>>>>> ---
>>>>> Hello,
>>>>>
>>>>> This change was suggested by Mauro in [0] although is still not clear
>>>>> if this is the way forward since changing PAD indexes can break the
>>>>> uAPI depending on how the PADs are looked up. Another alternative is
>>>>> to have a PAD type as Mauro mentioned on the same email but since the
>>>>> series are RFC, I'm making this change as an example and hopping that
>>>>> the patches can help with the discussion.
>>>>>
>>>>> [0]: http://www.spinics.net/lists/linux-media/msg98042.html
>>>>>
>>>>> Best regards,
>>>>> Javier
>>>>>
>>>>>  include/media/v4l2-mc.h | 3 ++-
>>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
>>>>> index 98a938aabdfb..47c00c288a06 100644
>>>>> --- a/include/media/v4l2-mc.h
>>>>> +++ b/include/media/v4l2-mc.h
>>>>> @@ -94,7 +94,8 @@ enum if_aud_dec_pad_index {
>>>>>   * @DEMOD_NUM_PADS:	Maximum number of output pads.
>>>>>   */
>>>>>  enum demod_pad_index {
>>>>> -	DEMOD_PAD_IF_INPUT,
>>>>> +	DEMOD_PAD_IF_INPUT, /* S-Video Y input or Composite */
>>>>> +	DEMOD_PAD_C_INPUT,  /* S-Video C input or Composite */
>>>>>  	DEMOD_PAD_VID_OUT,
>>>>>  	DEMOD_PAD_VBI_OUT,
>>>>>  	DEMOD_PAD_AUDIO_OUT,
>>>>>  
>>>>
>>>> These pad index enums are butt ugly and won't scale in the long run. An entity
>>>> should have just as many pads as it needs and no more.
>>>>
>>>> If you want to have an heuristic so you can find which pad carries e.g.
>>>> composite video, then it is better to ask the subdev for that.
>>>>
>>>> E.g.: err = v4l2_subdev_call(sd, pad, g_signal_pad, V4L2_PAD_Y_SIG_INPUT, &pad)
>>>>
>>>> The subdev driver knows which pad has which signal, so this does not rely on
>>>> hardcoding all combinations of possible pad types and you can still heuristically
>>>> build a media graph for legacy drivers.
>>
>> Yes, accessing PADs via a hardcoded index is butt ugly.
>>
>> For sure, we need a better strategy than that. This is one of the things
>> we need to discuss at the media summit.
>>
>>>> What we do now is reminiscent of the bad old days when the input numbers (as
>>>> returned by ENUMINPUT) where mapped to the i2c routing (basically pads). I worked
>>>> hard to get rid of that hardcoded relationship and I don't like to see it coming
>>>> back.
>>
>> No, this is completely unrelated with ENUMINPUT. 
>>
>> With VIDIOC_*INPUT ioctls, a hardcoded list of inputs can happen only at
>> the Kernel side, as, userspace should not rely on the input index, but,
>> instead, should call VIDIOC_ENUMINPUT.
>>
>> However, the media controller currently lacks an "ENUMPADS" ioctl that
>> would tell userspace what kind of data each PAD contains. Due to that,
>> on entities with more than one sink pad and/or more than one source
>> pad, the application should rely on the PAD index.
>>
>> That also reflects on the Kernel side, that forces drivers to do
>> things like:
>>
>> 	struct tvp5150 *core = to_tvp5150(sd);
>> 	int res;
>>
>> 	core->pads[DEMOD_PAD_IF_INPUT].flags = MEDIA_PAD_FL_SINK;
>> 	core->pads[DEMOD_PAD_VID_OUT].flags = MEDIA_PAD_FL_SOURCE;
>> 	core->pads[DEMOD_PAD_VBI_OUT].flags = MEDIA_PAD_FL_SOURCE;
>>
>> 	res = media_entity_pads_init(&sd->entity, DEMOD_NUM_PADS, core->pads);
>>
>> hardcoding the PAD indexes.
>>
>> The enums that are right now at v4l2-mc.h just prevents the mess to
>> spread all over the drivers, while we don't have a better solution, as
>> at least it will prevent two different devices with the very same type
>> to have a completely different set of PADs, with would cause lots of
>> pain on drivers that work with a multiple set of entities of the same
>> type.
> 
> This is already device specific. The video and audio s_routing ops are there
> precisely because the routing between devices is board specific. It links
> entities with each other the way we had to before we had the media controller.
> 
> Subdev entities should *not* use these fake pads. It's going to be a nightmare.
> 
> A reasonable solution to simplify converting legacy drivers without creating
> these global ugly pad indices is to add a new video (and probably audio) op
> 'g_pad_of_type(type)' where you ask the subdev entity to return which pad carries
> signals of a certain type.

This basically puts a layer between the low-level pads as defined by the entity
and the 'meta-pads' that a generic MC link creator would need to handle legacy
drivers. The nice thing is that this is wholly inside the kernel so we can
modify it at will later without impacting userspace.

Regards,

	Hans

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Mauro Carvalho Chehab]

Hi Hans,

Em Mon, 21 Mar 2016 17:01:43 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> > A reasonable solution to simplify converting legacy drivers without creating
> > these global ugly pad indices is to add a new video (and probably audio) op
> > 'g_pad_of_type(type)' where you ask the subdev entity to return which pad carries
> > signals of a certain type.  
> 
> This basically puts a layer between the low-level pads as defined by the entity
> and the 'meta-pads' that a generic MC link creator would need to handle legacy
> drivers. The nice thing is that this is wholly inside the kernel so we can
> modify it at will later without impacting userspace.

I prepared a long answer to your email, but I guess we're not at the
same page.

Let be clear on my view. Please let me know where you disagree:

1) I'm not defending Javier's patchset. I have my restrictions to
it too. My understanding is that he sent this as a RFC for feeding
our discussions for the media summit.

Javier, please correct me if I'm wrong.

2) I don't understand what you're calling as "meta-pads". For me, a
PAD is a physical set of pins. 

3) IMO, the best is to have just one PAD for a decoder input. That makes
everything simple, yet functional.

In my view, the input PAD will be linked to several "input connections".
So, in the case of tvp5150, it will have:

	- composite 1
	- composite 2
	- s-video

4) On that view, the input PAD is actually a set of pins. In the
case of tvp5150, the pins that compose the input PADs are
AIP1A and AIP1B.

The output PAD is also a set of pins YOUT0 to YOUT7, plus some other
pins for sync. Yet, it should, IMHO, have just one output PAD at
the MC graph.

-- 
Thanks,
Mauro

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Hans Verkuil]

On 03/21/2016 06:50 PM, Mauro Carvalho Chehab wrote:
> Hi Hans,
> 
> Em Mon, 21 Mar 2016 17:01:43 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>>> A reasonable solution to simplify converting legacy drivers without creating
>>> these global ugly pad indices is to add a new video (and probably audio) op
>>> 'g_pad_of_type(type)' where you ask the subdev entity to return which pad carries
>>> signals of a certain type.  
>>
>> This basically puts a layer between the low-level pads as defined by the entity
>> and the 'meta-pads' that a generic MC link creator would need to handle legacy
>> drivers. The nice thing is that this is wholly inside the kernel so we can
>> modify it at will later without impacting userspace.
> 
> I prepared a long answer to your email, but I guess we're not at the
> same page.
> 
> Let be clear on my view. Please let me know where you disagree:
> 
> 1) I'm not defending Javier's patchset. I have my restrictions to
> it too. My understanding is that he sent this as a RFC for feeding
> our discussions for the media summit.
> 
> Javier, please correct me if I'm wrong.
> 
> 2) I don't understand what you're calling as "meta-pads". For me, a
> PAD is a physical set of pins. 

Poorly worded on my side. I'll elaborate below.

> 3) IMO, the best is to have just one PAD for a decoder input. That makes
> everything simple, yet functional.
> 
> In my view, the input PAD will be linked to several "input connections".
> So, in the case of tvp5150, it will have:
> 
> 	- composite 1
> 	- composite 2
> 	- s-video
> 
> 4) On that view, the input PAD is actually a set of pins. In the
> case of tvp5150, the pins that compose the input PADs are
> AIP1A and AIP1B.
> 
> The output PAD is also a set of pins YOUT0 to YOUT7, plus some other
> pins for sync. Yet, it should, IMHO, have just one output PAD at
> the MC graph.

Indeed. So a tvp5150 has three sink pads and one source pad (pixel port).
Other similar devices may have different numbers of sink pads (say four
composite sinks and no S-Video sinks). So the pads the entity creates
should match what the hardware supports.

So far, so good.

If we want to create code that can more-or-less automatically create a MC
topology for legacy drivers, then we would like to be able to map a high-level
description like 'the first S-Video sink pad' into the actual pad. So you'd
have a 'MAP_PAD_SVID_1' define that, when passed to the g_pad_of_type() op
would return the actual pad index for the first S-Video sink pad (or an error
if there isn't one). That's what I meant with 'meta-pad' (and let's just
forget about that name, poor choice from my side).

What I think Javier's patch did was to require subdevs that have an S-Video pad
to use the DEMOD_PAD_C_INPUT + IF_INPUT pad indices for that. That's really
wrong. The subdev driver decides how many pads there are and which pad is
assigned to which index. That shouldn't be forced on them from the outside
because that won't scale.

But you can make an op that asks 'which pad carries this signal?'. That's fine.

I hope this clarifies matters.

Regards,

	Hans

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Mauro Carvalho Chehab]

Em Mon, 21 Mar 2016 19:08:32 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 03/21/2016 06:50 PM, Mauro Carvalho Chehab wrote:
> > Hi Hans,
> > 
> > Em Mon, 21 Mar 2016 17:01:43 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >   
> >>> A reasonable solution to simplify converting legacy drivers without creating
> >>> these global ugly pad indices is to add a new video (and probably audio) op
> >>> 'g_pad_of_type(type)' where you ask the subdev entity to return which pad carries
> >>> signals of a certain type.    
> >>
> >> This basically puts a layer between the low-level pads as defined by the entity
> >> and the 'meta-pads' that a generic MC link creator would need to handle legacy
> >> drivers. The nice thing is that this is wholly inside the kernel so we can
> >> modify it at will later without impacting userspace.  
> > 
> > I prepared a long answer to your email, but I guess we're not at the
> > same page.
> > 
> > Let be clear on my view. Please let me know where you disagree:
> > 
> > 1) I'm not defending Javier's patchset. I have my restrictions to
> > it too. My understanding is that he sent this as a RFC for feeding
> > our discussions for the media summit.
> > 
> > Javier, please correct me if I'm wrong.
> > 
> > 2) I don't understand what you're calling as "meta-pads". For me, a
> > PAD is a physical set of pins.   
> 
> Poorly worded on my side. I'll elaborate below.
> 
> > 3) IMO, the best is to have just one PAD for a decoder input. That makes
> > everything simple, yet functional.
> > 
> > In my view, the input PAD will be linked to several "input connections".
> > So, in the case of tvp5150, it will have:
> > 
> > 	- composite 1
> > 	- composite 2
> > 	- s-video
> > 
> > 4) On that view, the input PAD is actually a set of pins. In the
> > case of tvp5150, the pins that compose the input PADs are
> > AIP1A and AIP1B.
> > 
> > The output PAD is also a set of pins YOUT0 to YOUT7, plus some other
> > pins for sync. Yet, it should, IMHO, have just one output PAD at
> > the MC graph.  
> 
> Indeed. So a tvp5150 has three sink pads and one source pad (pixel port).

I would actually map tvp5150 with just one sink pad, with 3 links
(one for each connector).

In other words, I'm mapping tvp5150 input mux via links, and the
output of its mux as the sink pad.

IMHO, this works a way better than one sink pad per input at its
internal mux.

Regards,
Mauro

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Javier Martinez Canillas]

Hello Hans,

On 03/21/2016 03:08 PM, Hans Verkuil wrote:
> On 03/21/2016 06:50 PM, Mauro Carvalho Chehab wrote:
>> Hi Hans,
>>
>> Em Mon, 21 Mar 2016 17:01:43 +0100
>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>
>>>> A reasonable solution to simplify converting legacy drivers without creating
>>>> these global ugly pad indices is to add a new video (and probably audio) op
>>>> 'g_pad_of_type(type)' where you ask the subdev entity to return which pad carries
>>>> signals of a certain type.  
>>>
>>> This basically puts a layer between the low-level pads as defined by the entity
>>> and the 'meta-pads' that a generic MC link creator would need to handle legacy
>>> drivers. The nice thing is that this is wholly inside the kernel so we can
>>> modify it at will later without impacting userspace.
>>
>> I prepared a long answer to your email, but I guess we're not at the
>> same page.
>>
>> Let be clear on my view. Please let me know where you disagree:
>>
>> 1) I'm not defending Javier's patchset. I have my restrictions to
>> it too. My understanding is that he sent this as a RFC for feeding
>> our discussions for the media summit.
>>
>> Javier, please correct me if I'm wrong.
>>

That's correct. I wanted to have some patches that were aligned to what
were discussed so far in order to have more examples to contribute in
the media summit discussion (since I won't be there).

The patches are RFC and not meant to upstream since there are too many
open questions. I just hoped that having more examples could help of
them. I was specially interested in the DT bindings using OF graph to
lookup the connectors and the level of detail there.

>> 2) I don't understand what you're calling as "meta-pads". For me, a
>> PAD is a physical set of pins. 
> 
> Poorly worded on my side. I'll elaborate below.
> 
>> 3) IMO, the best is to have just one PAD for a decoder input. That makes
>> everything simple, yet functional.
>>
>> In my view, the input PAD will be linked to several "input connections".
>> So, in the case of tvp5150, it will have:
>>
>> 	- composite 1
>> 	- composite 2
>> 	- s-video
>>
>> 4) On that view, the input PAD is actually a set of pins. In the
>> case of tvp5150, the pins that compose the input PADs are
>> AIP1A and AIP1B.
>>
>> The output PAD is also a set of pins YOUT0 to YOUT7, plus some other
>> pins for sync. Yet, it should, IMHO, have just one output PAD at
>> the MC graph.
> 
> Indeed. So a tvp5150 has three sink pads and one source pad (pixel port).

Why 3 sink pads? Are we going to model each possible connection as a PAD
instead of an entity or are you talking about physical pins? Because if
is the latter, then the tvp5150 has only 2 (Composite1 shares S-Video Y
and Composite2 shares C signal).

> Other similar devices may have different numbers of sink pads (say four
> composite sinks and no S-Video sinks). So the pads the entity creates
> should match what the hardware supports.
> 
> So far, so good.
>

I'm confused. I thought that the latest agreed approach was to model the
actual connection signals and input pins as PADs instead of a simplied
model that just each connection as a sink.
 
> If we want to create code that can more-or-less automatically create a MC
> topology for legacy drivers, then we would like to be able to map a high-level
> description like 'the first S-Video sink pad' into the actual pad. So you'd
> have a 'MAP_PAD_SVID_1' define that, when passed to the g_pad_of_type() op
> would return the actual pad index for the first S-Video sink pad (or an error
> if there isn't one). That's what I meant with 'meta-pad' (and let's just
> forget about that name, poor choice from my side).
>

Can you please provide an example of a media pipeline that user-space should
use with this approach? AFAICT whatever PADs are created when initiliazing
the PADs for an entity, will be exposed to user-space in the media graph.

So I'm not understading how it will be used in practice. I don't mean that
your approach is not correct, is just I'm not getting it :)

> What I think Javier's patch did was to require subdevs that have an S-Video pad
> to use the DEMOD_PAD_C_INPUT + IF_INPUT pad indices for that. That's really
> wrong. The subdev driver decides how many pads there are and which pad is
> assigned to which index. That shouldn't be forced on them from the outside
> because that won't scale.
>

Yes, that was something that Mauro suggested in [0] as a possible approach
but I also was not sure about it and mentioned in the patch comments.

> But you can make an op that asks 'which pad carries this signal?'. That's fine.
>
> I hope this clarifies matters.
> 
> Regards,
> 
> 	Hans
> 

[0]: http://www.spinics.net/lists/linux-media/msg98042.html

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Mauro Carvalho Chehab]

Em Mon, 21 Mar 2016 15:24:00 -0300
Javier Martinez Canillas <javier@osg.samsung.com> escreveu:

> Hello Hans,
> 
> On 03/21/2016 03:08 PM, Hans Verkuil wrote:
> > On 03/21/2016 06:50 PM, Mauro Carvalho Chehab wrote:
> >> Hi Hans,
> >>
> >> Em Mon, 21 Mar 2016 17:01:43 +0100
> >> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>
> >>>> A reasonable solution to simplify converting legacy drivers without creating
> >>>> these global ugly pad indices is to add a new video (and probably audio) op
> >>>> 'g_pad_of_type(type)' where you ask the subdev entity to return which pad carries
> >>>> signals of a certain type.  
> >>>
> >>> This basically puts a layer between the low-level pads as defined by the entity
> >>> and the 'meta-pads' that a generic MC link creator would need to handle legacy
> >>> drivers. The nice thing is that this is wholly inside the kernel so we can
> >>> modify it at will later without impacting userspace.
> >>
> >> I prepared a long answer to your email, but I guess we're not at the
> >> same page.
> >>
> >> Let be clear on my view. Please let me know where you disagree:
> >>
> >> 1) I'm not defending Javier's patchset. I have my restrictions to
> >> it too. My understanding is that he sent this as a RFC for feeding
> >> our discussions for the media summit.
> >>
> >> Javier, please correct me if I'm wrong.
> >>
> 
> That's correct. I wanted to have some patches that were aligned to what
> were discussed so far in order to have more examples to contribute in
> the media summit discussion (since I won't be there).
> 
> The patches are RFC and not meant to upstream since there are too many
> open questions. I just hoped that having more examples could help of
> them. I was specially interested in the DT bindings using OF graph to
> lookup the connectors and the level of detail there.
> 
> >> 2) I don't understand what you're calling as "meta-pads". For me, a
> >> PAD is a physical set of pins. 
> > 
> > Poorly worded on my side. I'll elaborate below.
> > 
> >> 3) IMO, the best is to have just one PAD for a decoder input. That makes
> >> everything simple, yet functional.
> >>
> >> In my view, the input PAD will be linked to several "input connections".
> >> So, in the case of tvp5150, it will have:
> >>
> >> 	- composite 1
> >> 	- composite 2
> >> 	- s-video
> >>
> >> 4) On that view, the input PAD is actually a set of pins. In the
> >> case of tvp5150, the pins that compose the input PADs are
> >> AIP1A and AIP1B.
> >>
> >> The output PAD is also a set of pins YOUT0 to YOUT7, plus some other
> >> pins for sync. Yet, it should, IMHO, have just one output PAD at
> >> the MC graph.
> > 
> > Indeed. So a tvp5150 has three sink pads and one source pad (pixel port).
> 
> Why 3 sink pads? Are we going to model each possible connection as a PAD
> instead of an entity or are you talking about physical pins? Because if
> is the latter, then the tvp5150 has only 2 (Composite1 shares S-Video Y
> and Composite2 shares C signal).
> 
> > Other similar devices may have different numbers of sink pads (say four
> > composite sinks and no S-Video sinks). So the pads the entity creates
> > should match what the hardware supports.
> > 
> > So far, so good.
> >
> 
> I'm confused. I thought that the latest agreed approach was to model the
> actual connection signals and input pins as PADs instead of a simplied
> model that just each connection as a sink.
>  
> > If we want to create code that can more-or-less automatically create a MC
> > topology for legacy drivers, then we would like to be able to map a high-level
> > description like 'the first S-Video sink pad' into the actual pad. So you'd
> > have a 'MAP_PAD_SVID_1' define that, when passed to the g_pad_of_type() op
> > would return the actual pad index for the first S-Video sink pad (or an error
> > if there isn't one). That's what I meant with 'meta-pad' (and let's just
> > forget about that name, poor choice from my side).
> >
> 
> Can you please provide an example of a media pipeline that user-space should
> use with this approach? AFAICT whatever PADs are created when initiliazing
> the PADs for an entity, will be exposed to user-space in the media graph.
> 
> So I'm not understading how it will be used in practice. I don't mean that
> your approach is not correct, is just I'm not getting it :)
> 
> > What I think Javier's patch did was to require subdevs that have an S-Video pad
> > to use the DEMOD_PAD_C_INPUT + IF_INPUT pad indices for that. That's really
> > wrong. The subdev driver decides how many pads there are and which pad is
> > assigned to which index. That shouldn't be forced on them from the outside
> > because that won't scale.
> >
> 
> Yes, that was something that Mauro suggested in [0] as a possible approach
> but I also was not sure about it and mentioned in the patch comments.
> 
> > But you can make an op that asks 'which pad carries this signal?'. That's fine.
> >
> > I hope this clarifies matters.
> > 
> > Regards,
> > 
> > 	Hans
> > 
> 
> [0]: http://www.spinics.net/lists/linux-media/msg98042.html

Yeah, I proposed that, but, after more thinking, it seems easier to
just use a single sink pad for all supported inputs, just like what's
there today.

We'll need to do something different for HDMI, as the HDMI input
may have signals like CEC that would be going through different
chips, but for TV decoders that have just composite/RF/s-video
inputs, I don't see any need to make the model complex.

Thanks,
Mauro

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Javier Martinez Canillas]

Hello Mauro,

On 03/21/2016 03:34 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 21 Mar 2016 15:24:00 -0300
> Javier Martinez Canillas <javier@osg.samsung.com> escreveu:
> 
>> Hello Hans,
>>
>> On 03/21/2016 03:08 PM, Hans Verkuil wrote:
>>> On 03/21/2016 06:50 PM, Mauro Carvalho Chehab wrote:
>>>> Hi Hans,
>>>>
>>>> Em Mon, 21 Mar 2016 17:01:43 +0100
>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>>
>>>>>> A reasonable solution to simplify converting legacy drivers without creating
>>>>>> these global ugly pad indices is to add a new video (and probably audio) op
>>>>>> 'g_pad_of_type(type)' where you ask the subdev entity to return which pad carries
>>>>>> signals of a certain type.  
>>>>>
>>>>> This basically puts a layer between the low-level pads as defined by the entity
>>>>> and the 'meta-pads' that a generic MC link creator would need to handle legacy
>>>>> drivers. The nice thing is that this is wholly inside the kernel so we can
>>>>> modify it at will later without impacting userspace.
>>>>
>>>> I prepared a long answer to your email, but I guess we're not at the
>>>> same page.
>>>>
>>>> Let be clear on my view. Please let me know where you disagree:
>>>>
>>>> 1) I'm not defending Javier's patchset. I have my restrictions to
>>>> it too. My understanding is that he sent this as a RFC for feeding
>>>> our discussions for the media summit.
>>>>
>>>> Javier, please correct me if I'm wrong.
>>>>
>>
>> That's correct. I wanted to have some patches that were aligned to what
>> were discussed so far in order to have more examples to contribute in
>> the media summit discussion (since I won't be there).
>>
>> The patches are RFC and not meant to upstream since there are too many
>> open questions. I just hoped that having more examples could help of
>> them. I was specially interested in the DT bindings using OF graph to
>> lookup the connectors and the level of detail there.
>>
>>>> 2) I don't understand what you're calling as "meta-pads". For me, a
>>>> PAD is a physical set of pins. 
>>>
>>> Poorly worded on my side. I'll elaborate below.
>>>
>>>> 3) IMO, the best is to have just one PAD for a decoder input. That makes
>>>> everything simple, yet functional.
>>>>
>>>> In my view, the input PAD will be linked to several "input connections".
>>>> So, in the case of tvp5150, it will have:
>>>>
>>>> 	- composite 1
>>>> 	- composite 2
>>>> 	- s-video
>>>>
>>>> 4) On that view, the input PAD is actually a set of pins. In the
>>>> case of tvp5150, the pins that compose the input PADs are
>>>> AIP1A and AIP1B.
>>>>
>>>> The output PAD is also a set of pins YOUT0 to YOUT7, plus some other
>>>> pins for sync. Yet, it should, IMHO, have just one output PAD at
>>>> the MC graph.
>>>
>>> Indeed. So a tvp5150 has three sink pads and one source pad (pixel port).
>>
>> Why 3 sink pads? Are we going to model each possible connection as a PAD
>> instead of an entity or are you talking about physical pins? Because if
>> is the latter, then the tvp5150 has only 2 (Composite1 shares S-Video Y
>> and Composite2 shares C signal).
>>
>>> Other similar devices may have different numbers of sink pads (say four
>>> composite sinks and no S-Video sinks). So the pads the entity creates
>>> should match what the hardware supports.
>>>
>>> So far, so good.
>>>
>>
>> I'm confused. I thought that the latest agreed approach was to model the
>> actual connection signals and input pins as PADs instead of a simplied
>> model that just each connection as a sink.
>>  
>>> If we want to create code that can more-or-less automatically create a MC
>>> topology for legacy drivers, then we would like to be able to map a high-level
>>> description like 'the first S-Video sink pad' into the actual pad. So you'd
>>> have a 'MAP_PAD_SVID_1' define that, when passed to the g_pad_of_type() op
>>> would return the actual pad index for the first S-Video sink pad (or an error
>>> if there isn't one). That's what I meant with 'meta-pad' (and let's just
>>> forget about that name, poor choice from my side).
>>>
>>
>> Can you please provide an example of a media pipeline that user-space should
>> use with this approach? AFAICT whatever PADs are created when initiliazing
>> the PADs for an entity, will be exposed to user-space in the media graph.
>>
>> So I'm not understading how it will be used in practice. I don't mean that
>> your approach is not correct, is just I'm not getting it :)
>>
>>> What I think Javier's patch did was to require subdevs that have an S-Video pad
>>> to use the DEMOD_PAD_C_INPUT + IF_INPUT pad indices for that. That's really
>>> wrong. The subdev driver decides how many pads there are and which pad is
>>> assigned to which index. That shouldn't be forced on them from the outside
>>> because that won't scale.
>>>
>>
>> Yes, that was something that Mauro suggested in [0] as a possible approach
>> but I also was not sure about it and mentioned in the patch comments.
>>
>>> But you can make an op that asks 'which pad carries this signal?'. That's fine.
>>>
>>> I hope this clarifies matters.
>>>
>>> Regards,
>>>
>>> 	Hans
>>>
>>
>> [0]: http://www.spinics.net/lists/linux-media/msg98042.html
> 
> Yeah, I proposed that, but, after more thinking, it seems easier to
> just use a single sink pad for all supported inputs, just like what's
> there today.
>

Indeed, that's actually how the driver works today since is the model I in
commit f7b4b54e6364 ("[media] tvp5150: add HW input connectors support").

But after the patches landed, I was told that the DT binding was
wrong because it didn't use the OF graph (so got reverted) and that
it didn't model the connectors correctly since there weren't three
different physical connectors but three different "connections"
that used two physical connectors.

That's why I thought the latest agreed approach was to model the
actual pins and signals as PAD instead of a simplified version.

It seems now that is agreed how the DT binding should look like
(basically use OF graph ports and endpoints) but we still are
discussing what the entities, PADs and links should model.

> We'll need to do something different for HDMI, as the HDMI input
> may have signals like CEC that would be going through different
> chips, but for TV decoders that have just composite/RF/s-video
> inputs, I don't see any need to make the model complex.
> 

I see, it would be nice though to think about that case too so
the DT bindings can be consistent for both analog and digital.

> Thanks,
> Mauro
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Hans Verkuil]

On 03/21/2016 07:23 PM, Mauro Carvalho Chehab wrote:
>> Indeed. So a tvp5150 has three sink pads and one source pad (pixel port).
> 
> I would actually map tvp5150 with just one sink pad, with 3 links
> (one for each connector).
> 
> In other words, I'm mapping tvp5150 input mux via links, and the
> output of its mux as the sink pad.
> 
> IMHO, this works a way better than one sink pad per input at its
> internal mux.

You're right, that would work better for this specific device.

Regards,

	Hans

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Hans Verkuil]

On 03/21/2016 07:34 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 21 Mar 2016 15:24:00 -0300
> Javier Martinez Canillas <javier@osg.samsung.com> escreveu:
> 
>> Hello Hans,
>>
>> On 03/21/2016 03:08 PM, Hans Verkuil wrote:
>>> On 03/21/2016 06:50 PM, Mauro Carvalho Chehab wrote:
>>>> Hi Hans,
>>>>
>>>> Em Mon, 21 Mar 2016 17:01:43 +0100
>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>>
>>>>>> A reasonable solution to simplify converting legacy drivers without creating
>>>>>> these global ugly pad indices is to add a new video (and probably audio) op
>>>>>> 'g_pad_of_type(type)' where you ask the subdev entity to return which pad carries
>>>>>> signals of a certain type.  
>>>>>
>>>>> This basically puts a layer between the low-level pads as defined by the entity
>>>>> and the 'meta-pads' that a generic MC link creator would need to handle legacy
>>>>> drivers. The nice thing is that this is wholly inside the kernel so we can
>>>>> modify it at will later without impacting userspace.
>>>>
>>>> I prepared a long answer to your email, but I guess we're not at the
>>>> same page.
>>>>
>>>> Let be clear on my view. Please let me know where you disagree:
>>>>
>>>> 1) I'm not defending Javier's patchset. I have my restrictions to
>>>> it too. My understanding is that he sent this as a RFC for feeding
>>>> our discussions for the media summit.
>>>>
>>>> Javier, please correct me if I'm wrong.
>>>>
>>
>> That's correct. I wanted to have some patches that were aligned to what
>> were discussed so far in order to have more examples to contribute in
>> the media summit discussion (since I won't be there).
>>
>> The patches are RFC and not meant to upstream since there are too many
>> open questions. I just hoped that having more examples could help of
>> them. I was specially interested in the DT bindings using OF graph to
>> lookup the connectors and the level of detail there.
>>
>>>> 2) I don't understand what you're calling as "meta-pads". For me, a
>>>> PAD is a physical set of pins. 
>>>
>>> Poorly worded on my side. I'll elaborate below.
>>>
>>>> 3) IMO, the best is to have just one PAD for a decoder input. That makes
>>>> everything simple, yet functional.
>>>>
>>>> In my view, the input PAD will be linked to several "input connections".
>>>> So, in the case of tvp5150, it will have:
>>>>
>>>> 	- composite 1
>>>> 	- composite 2
>>>> 	- s-video
>>>>
>>>> 4) On that view, the input PAD is actually a set of pins. In the
>>>> case of tvp5150, the pins that compose the input PADs are
>>>> AIP1A and AIP1B.
>>>>
>>>> The output PAD is also a set of pins YOUT0 to YOUT7, plus some other
>>>> pins for sync. Yet, it should, IMHO, have just one output PAD at
>>>> the MC graph.
>>>
>>> Indeed. So a tvp5150 has three sink pads and one source pad (pixel port).
>>
>> Why 3 sink pads? Are we going to model each possible connection as a PAD
>> instead of an entity or are you talking about physical pins? Because if
>> is the latter, then the tvp5150 has only 2 (Composite1 shares S-Video Y
>> and Composite2 shares C signal).
>>
>>> Other similar devices may have different numbers of sink pads (say four
>>> composite sinks and no S-Video sinks). So the pads the entity creates
>>> should match what the hardware supports.
>>>
>>> So far, so good.
>>>
>>
>> I'm confused. I thought that the latest agreed approach was to model the
>> actual connection signals and input pins as PADs instead of a simplied
>> model that just each connection as a sink.
>>  
>>> If we want to create code that can more-or-less automatically create a MC
>>> topology for legacy drivers, then we would like to be able to map a high-level
>>> description like 'the first S-Video sink pad' into the actual pad. So you'd
>>> have a 'MAP_PAD_SVID_1' define that, when passed to the g_pad_of_type() op
>>> would return the actual pad index for the first S-Video sink pad (or an error
>>> if there isn't one). That's what I meant with 'meta-pad' (and let's just
>>> forget about that name, poor choice from my side).
>>>
>>
>> Can you please provide an example of a media pipeline that user-space should
>> use with this approach? AFAICT whatever PADs are created when initiliazing
>> the PADs for an entity, will be exposed to user-space in the media graph.
>>
>> So I'm not understading how it will be used in practice. I don't mean that
>> your approach is not correct, is just I'm not getting it :)
>>
>>> What I think Javier's patch did was to require subdevs that have an S-Video pad
>>> to use the DEMOD_PAD_C_INPUT + IF_INPUT pad indices for that. That's really
>>> wrong. The subdev driver decides how many pads there are and which pad is
>>> assigned to which index. That shouldn't be forced on them from the outside
>>> because that won't scale.
>>>
>>
>> Yes, that was something that Mauro suggested in [0] as a possible approach
>> but I also was not sure about it and mentioned in the patch comments.
>>
>>> But you can make an op that asks 'which pad carries this signal?'. That's fine.
>>>
>>> I hope this clarifies matters.
>>>
>>> Regards,
>>>
>>> 	Hans
>>>
>>
>> [0]: http://www.spinics.net/lists/linux-media/msg98042.html
> 
> Yeah, I proposed that, but, after more thinking, it seems easier to
> just use a single sink pad for all supported inputs, just like what's
> there today.
> 
> We'll need to do something different for HDMI, as the HDMI input
> may have signals like CEC that would be going through different
> chips, but for TV decoders that have just composite/RF/s-video
> inputs, I don't see any need to make the model complex.

Also devices like the adv7604 have multiple HDMI inputs, and although only one
HDMI input will go out to the pixelport, the others are still used for EDID
and HDCP processing and hotplug detect, etc. So using a single pad won't
work there.

Regards,

	Hans

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Hans Verkuil]

On 03/21/2016 07:24 PM, Javier Martinez Canillas wrote:
> Hello Hans,
> 
> On 03/21/2016 03:08 PM, Hans Verkuil wrote:
>> On 03/21/2016 06:50 PM, Mauro Carvalho Chehab wrote:
>>> Hi Hans,
>>>
>>> Em Mon, 21 Mar 2016 17:01:43 +0100
>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>
>>>>> A reasonable solution to simplify converting legacy drivers without creating
>>>>> these global ugly pad indices is to add a new video (and probably audio) op
>>>>> 'g_pad_of_type(type)' where you ask the subdev entity to return which pad carries
>>>>> signals of a certain type.  
>>>>
>>>> This basically puts a layer between the low-level pads as defined by the entity
>>>> and the 'meta-pads' that a generic MC link creator would need to handle legacy
>>>> drivers. The nice thing is that this is wholly inside the kernel so we can
>>>> modify it at will later without impacting userspace.
>>>
>>> I prepared a long answer to your email, but I guess we're not at the
>>> same page.
>>>
>>> Let be clear on my view. Please let me know where you disagree:
>>>
>>> 1) I'm not defending Javier's patchset. I have my restrictions to
>>> it too. My understanding is that he sent this as a RFC for feeding
>>> our discussions for the media summit.
>>>
>>> Javier, please correct me if I'm wrong.
>>>
> 
> That's correct. I wanted to have some patches that were aligned to what
> were discussed so far in order to have more examples to contribute in
> the media summit discussion (since I won't be there).
> 
> The patches are RFC and not meant to upstream since there are too many
> open questions. I just hoped that having more examples could help of
> them. I was specially interested in the DT bindings using OF graph to
> lookup the connectors and the level of detail there.
> 
>>> 2) I don't understand what you're calling as "meta-pads". For me, a
>>> PAD is a physical set of pins. 
>>
>> Poorly worded on my side. I'll elaborate below.
>>
>>> 3) IMO, the best is to have just one PAD for a decoder input. That makes
>>> everything simple, yet functional.
>>>
>>> In my view, the input PAD will be linked to several "input connections".
>>> So, in the case of tvp5150, it will have:
>>>
>>> 	- composite 1
>>> 	- composite 2
>>> 	- s-video
>>>
>>> 4) On that view, the input PAD is actually a set of pins. In the
>>> case of tvp5150, the pins that compose the input PADs are
>>> AIP1A and AIP1B.
>>>
>>> The output PAD is also a set of pins YOUT0 to YOUT7, plus some other
>>> pins for sync. Yet, it should, IMHO, have just one output PAD at
>>> the MC graph.
>>
>> Indeed. So a tvp5150 has three sink pads and one source pad (pixel port).
> 
> Why 3 sink pads? Are we going to model each possible connection as a PAD
> instead of an entity or are you talking about physical pins? Because if
> is the latter, then the tvp5150 has only 2 (Composite1 shares S-Video Y
> and Composite2 shares C signal).

I'd go with Mauro's proposal of a single pad. And I didn't look into detail
in the hardware specs of the tvp5150, so that's why I got it wrong.

>> Other similar devices may have different numbers of sink pads (say four
>> composite sinks and no S-Video sinks). So the pads the entity creates
>> should match what the hardware supports.
>>
>> So far, so good.
>>
> 
> I'm confused. I thought that the latest agreed approach was to model the
> actual connection signals and input pins as PADs instead of a simplied
> model that just each connection as a sink.

My opinion is to just use the simple option (one pad) if you can get away
with it. I.e. in this case adding more sink pads doesn't add any useful
information. In the case of an adv7604 it does provide useful information since
you need to program the adv7604 based on how it is hooked up.

BTW, if the tvp5150 needs to know which composite connector is connected
to which hardware pin, then you still may need to be explicit w.r.t. the
number of pads. I just thought of that.

>> If we want to create code that can more-or-less automatically create a MC
>> topology for legacy drivers, then we would like to be able to map a high-level
>> description like 'the first S-Video sink pad' into the actual pad. So you'd
>> have a 'MAP_PAD_SVID_1' define that, when passed to the g_pad_of_type() op
>> would return the actual pad index for the first S-Video sink pad (or an error
>> if there isn't one). That's what I meant with 'meta-pad' (and let's just
>> forget about that name, poor choice from my side).
>>
> 
> Can you please provide an example of a media pipeline that user-space should
> use with this approach? AFAICT whatever PADs are created when initiliazing
> the PADs for an entity, will be exposed to user-space in the media graph.
> 
> So I'm not understading how it will be used in practice. I don't mean that
> your approach is not correct, is just I'm not getting it :)

Why would userspace need to use the pads? This is for legacy drivers (right?)
where the pipeline is fixed anyway.

To quote Mauro:

"we want this for two reasons:

1) To fix the locking troubles with analog, digital and audio parts of
the driver;

2) To report userspace apps what devnodes belong to each devices."

1) is internal to the kernel and doesn't involve userspace, 2) does involve
userspace, but you won't need to know about pads to handle that.

Regards,

	Hans

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Javier Martinez Canillas]

Hello Hans,

On 03/21/2016 04:06 PM, Hans Verkuil wrote:
> On 03/21/2016 07:24 PM, Javier Martinez Canillas wrote:
>> Hello Hans,
>>
>> On 03/21/2016 03:08 PM, Hans Verkuil wrote:
>>> On 03/21/2016 06:50 PM, Mauro Carvalho Chehab wrote:
>>>> Hi Hans,
>>>>
>>>> Em Mon, 21 Mar 2016 17:01:43 +0100
>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>>
>>>>>> A reasonable solution to simplify converting legacy drivers without creating
>>>>>> these global ugly pad indices is to add a new video (and probably audio) op
>>>>>> 'g_pad_of_type(type)' where you ask the subdev entity to return which pad carries
>>>>>> signals of a certain type.  
>>>>>
>>>>> This basically puts a layer between the low-level pads as defined by the entity
>>>>> and the 'meta-pads' that a generic MC link creator would need to handle legacy
>>>>> drivers. The nice thing is that this is wholly inside the kernel so we can
>>>>> modify it at will later without impacting userspace.
>>>>
>>>> I prepared a long answer to your email, but I guess we're not at the
>>>> same page.
>>>>
>>>> Let be clear on my view. Please let me know where you disagree:
>>>>
>>>> 1) I'm not defending Javier's patchset. I have my restrictions to
>>>> it too. My understanding is that he sent this as a RFC for feeding
>>>> our discussions for the media summit.
>>>>
>>>> Javier, please correct me if I'm wrong.
>>>>
>>
>> That's correct. I wanted to have some patches that were aligned to what
>> were discussed so far in order to have more examples to contribute in
>> the media summit discussion (since I won't be there).
>>
>> The patches are RFC and not meant to upstream since there are too many
>> open questions. I just hoped that having more examples could help of
>> them. I was specially interested in the DT bindings using OF graph to
>> lookup the connectors and the level of detail there.
>>
>>>> 2) I don't understand what you're calling as "meta-pads". For me, a
>>>> PAD is a physical set of pins. 
>>>
>>> Poorly worded on my side. I'll elaborate below.
>>>
>>>> 3) IMO, the best is to have just one PAD for a decoder input. That makes
>>>> everything simple, yet functional.
>>>>
>>>> In my view, the input PAD will be linked to several "input connections".
>>>> So, in the case of tvp5150, it will have:
>>>>
>>>> 	- composite 1
>>>> 	- composite 2
>>>> 	- s-video
>>>>
>>>> 4) On that view, the input PAD is actually a set of pins. In the
>>>> case of tvp5150, the pins that compose the input PADs are
>>>> AIP1A and AIP1B.
>>>>
>>>> The output PAD is also a set of pins YOUT0 to YOUT7, plus some other
>>>> pins for sync. Yet, it should, IMHO, have just one output PAD at
>>>> the MC graph.
>>>
>>> Indeed. So a tvp5150 has three sink pads and one source pad (pixel port).
>>
>> Why 3 sink pads? Are we going to model each possible connection as a PAD
>> instead of an entity or are you talking about physical pins? Because if
>> is the latter, then the tvp5150 has only 2 (Composite1 shares S-Video Y
>> and Composite2 shares C signal).
> 
> I'd go with Mauro's proposal of a single pad. And I didn't look into detail
> in the hardware specs of the tvp5150, so that's why I got it wrong.
> 

Ok.

>>> Other similar devices may have different numbers of sink pads (say four
>>> composite sinks and no S-Video sinks). So the pads the entity creates
>>> should match what the hardware supports.
>>>
>>> So far, so good.
>>>
>>
>> I'm confused. I thought that the latest agreed approach was to model the
>> actual connection signals and input pins as PADs instead of a simplied
>> model that just each connection as a sink.
> 
> My opinion is to just use the simple option (one pad) if you can get away
> with it. I.e. in this case adding more sink pads doesn't add any useful
> information. In the case of an adv7604 it does provide useful information since
> you need to program the adv7604 based on how it is hooked up.
>

Agreed.
 
> BTW, if the tvp5150 needs to know which composite connector is connected
> to which hardware pin, then you still may need to be explicit w.r.t. the
> number of pads. I just thought of that.
>

The tvp5150 doesn't care about that, as Mauro said is just a mux so you can
have logic in the .link_setup that does the mux depending on the remote
entity (that's in fact how I implemented and is currently in mainline).

Now, the user needs to know which entity is mapped to which input pin.
All its know from the HW documentation is that for example the left
RCA connector is AIP1A and the one inf the right is connected to AIP1B.

So there could be a convention that the composite connected to AIP1A pin
(the default) is Composite0 and the connected to AIP1B is Composite1.

>>> If we want to create code that can more-or-less automatically create a MC
>>> topology for legacy drivers, then we would like to be able to map a high-level
>>> description like 'the first S-Video sink pad' into the actual pad. So you'd
>>> have a 'MAP_PAD_SVID_1' define that, when passed to the g_pad_of_type() op
>>> would return the actual pad index for the first S-Video sink pad (or an error
>>> if there isn't one). That's what I meant with 'meta-pad' (and let's just
>>> forget about that name, poor choice from my side).
>>>
>>
>> Can you please provide an example of a media pipeline that user-space should
>> use with this approach? AFAICT whatever PADs are created when initiliazing
>> the PADs for an entity, will be exposed to user-space in the media graph.
>>
>> So I'm not understading how it will be used in practice. I don't mean that
>> your approach is not correct, is just I'm not getting it :)
> 
> Why would userspace need to use the pads? This is for legacy drivers (right?)
> where the pipeline is fixed anyway.
>

I asked because the user needs to setup the links in the media pipeline to
choose  which input connection will be linked to the tvp5150 decoder. But it
doesn't matter if we are going with the single sink pad approach since the
user will always do something like:

$ media-ctl -r -l '"Composite0":0->"tvp5150 1-005c":0[1]'

IOW, there will always choose the only connection source pad and tvp5150 sink.

There will be two source pads for the tvp5150 though, 1 for video and other
for VBI. But I guess this is not an issue since that's easier to standardize.
 
> To quote Mauro:
> 
> "we want this for two reasons:
> 
> 1) To fix the locking troubles with analog, digital and audio parts of
> the driver;
> 
> 2) To report userspace apps what devnodes belong to each devices."
> 
> 1) is internal to the kernel and doesn't involve userspace, 2) does involve
> userspace, but you won't need to know about pads to handle that.
> 
> Regards,
> 
> 	Hans
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Hans Verkuil]

On 03/21/2016 08:20 PM, Javier Martinez Canillas wrote:
> Hello Hans,
> 
> On 03/21/2016 04:06 PM, Hans Verkuil wrote:
>> On 03/21/2016 07:24 PM, Javier Martinez Canillas wrote:
>>> Hello Hans,
>>>
>>> On 03/21/2016 03:08 PM, Hans Verkuil wrote:
>>>> On 03/21/2016 06:50 PM, Mauro Carvalho Chehab wrote:
>>>>> Hi Hans,
>>>>>
>>>>> Em Mon, 21 Mar 2016 17:01:43 +0100
>>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>>>
>>>>>>> A reasonable solution to simplify converting legacy drivers without creating
>>>>>>> these global ugly pad indices is to add a new video (and probably audio) op
>>>>>>> 'g_pad_of_type(type)' where you ask the subdev entity to return which pad carries
>>>>>>> signals of a certain type.  
>>>>>>
>>>>>> This basically puts a layer between the low-level pads as defined by the entity
>>>>>> and the 'meta-pads' that a generic MC link creator would need to handle legacy
>>>>>> drivers. The nice thing is that this is wholly inside the kernel so we can
>>>>>> modify it at will later without impacting userspace.
>>>>>
>>>>> I prepared a long answer to your email, but I guess we're not at the
>>>>> same page.
>>>>>
>>>>> Let be clear on my view. Please let me know where you disagree:
>>>>>
>>>>> 1) I'm not defending Javier's patchset. I have my restrictions to
>>>>> it too. My understanding is that he sent this as a RFC for feeding
>>>>> our discussions for the media summit.
>>>>>
>>>>> Javier, please correct me if I'm wrong.
>>>>>
>>>
>>> That's correct. I wanted to have some patches that were aligned to what
>>> were discussed so far in order to have more examples to contribute in
>>> the media summit discussion (since I won't be there).
>>>
>>> The patches are RFC and not meant to upstream since there are too many
>>> open questions. I just hoped that having more examples could help of
>>> them. I was specially interested in the DT bindings using OF graph to
>>> lookup the connectors and the level of detail there.
>>>
>>>>> 2) I don't understand what you're calling as "meta-pads". For me, a
>>>>> PAD is a physical set of pins. 
>>>>
>>>> Poorly worded on my side. I'll elaborate below.
>>>>
>>>>> 3) IMO, the best is to have just one PAD for a decoder input. That makes
>>>>> everything simple, yet functional.
>>>>>
>>>>> In my view, the input PAD will be linked to several "input connections".
>>>>> So, in the case of tvp5150, it will have:
>>>>>
>>>>> 	- composite 1
>>>>> 	- composite 2
>>>>> 	- s-video
>>>>>
>>>>> 4) On that view, the input PAD is actually a set of pins. In the
>>>>> case of tvp5150, the pins that compose the input PADs are
>>>>> AIP1A and AIP1B.
>>>>>
>>>>> The output PAD is also a set of pins YOUT0 to YOUT7, plus some other
>>>>> pins for sync. Yet, it should, IMHO, have just one output PAD at
>>>>> the MC graph.
>>>>
>>>> Indeed. So a tvp5150 has three sink pads and one source pad (pixel port).
>>>
>>> Why 3 sink pads? Are we going to model each possible connection as a PAD
>>> instead of an entity or are you talking about physical pins? Because if
>>> is the latter, then the tvp5150 has only 2 (Composite1 shares S-Video Y
>>> and Composite2 shares C signal).
>>
>> I'd go with Mauro's proposal of a single pad. And I didn't look into detail
>> in the hardware specs of the tvp5150, so that's why I got it wrong.
>>
> 
> Ok.
> 
>>>> Other similar devices may have different numbers of sink pads (say four
>>>> composite sinks and no S-Video sinks). So the pads the entity creates
>>>> should match what the hardware supports.
>>>>
>>>> So far, so good.
>>>>
>>>
>>> I'm confused. I thought that the latest agreed approach was to model the
>>> actual connection signals and input pins as PADs instead of a simplied
>>> model that just each connection as a sink.
>>
>> My opinion is to just use the simple option (one pad) if you can get away
>> with it. I.e. in this case adding more sink pads doesn't add any useful
>> information. In the case of an adv7604 it does provide useful information since
>> you need to program the adv7604 based on how it is hooked up.
>>
> 
> Agreed.
>  
>> BTW, if the tvp5150 needs to know which composite connector is connected
>> to which hardware pin, then you still may need to be explicit w.r.t. the
>> number of pads. I just thought of that.
>>
> 
> The tvp5150 doesn't care about that, as Mauro said is just a mux so you can
> have logic in the .link_setup that does the mux depending on the remote
> entity (that's in fact how I implemented and is currently in mainline).
> 
> Now, the user needs to know which entity is mapped to which input pin.
> All its know from the HW documentation is that for example the left
> RCA connector is AIP1A and the one inf the right is connected to AIP1B.
> 
> So there could be a convention that the composite connected to AIP1A pin
> (the default) is Composite0 and the connected to AIP1B is Composite1.
> 
>>>> If we want to create code that can more-or-less automatically create a MC
>>>> topology for legacy drivers, then we would like to be able to map a high-level
>>>> description like 'the first S-Video sink pad' into the actual pad. So you'd
>>>> have a 'MAP_PAD_SVID_1' define that, when passed to the g_pad_of_type() op
>>>> would return the actual pad index for the first S-Video sink pad (or an error
>>>> if there isn't one). That's what I meant with 'meta-pad' (and let's just
>>>> forget about that name, poor choice from my side).
>>>>
>>>
>>> Can you please provide an example of a media pipeline that user-space should
>>> use with this approach? AFAICT whatever PADs are created when initiliazing
>>> the PADs for an entity, will be exposed to user-space in the media graph.
>>>
>>> So I'm not understading how it will be used in practice. I don't mean that
>>> your approach is not correct, is just I'm not getting it :)
>>
>> Why would userspace need to use the pads? This is for legacy drivers (right?)
>> where the pipeline is fixed anyway.
>>
> 
> I asked because the user needs to setup the links in the media pipeline to
> choose  which input connection will be linked to the tvp5150 decoder. But it
> doesn't matter if we are going with the single sink pad approach since the
> user will always do something like:

Why? The user will use an application that uses ENUM/S/G_INPUT for this. We're
talking legacy drivers ('interface centric drivers' would be a better description)
where we don't even expose the v4l-subdevX device nodes. Explicitly programming
a media pipeline is something you do for complex devices (embedded systems and
the like). Not for simple and generally fixed pipelines. Utterly pointless.

> 
> $ media-ctl -r -l '"Composite0":0->"tvp5150 1-005c":0[1]'
> 
> IOW, there will always choose the only connection source pad and tvp5150 sink.
> 
> There will be two source pads for the tvp5150 though, 1 for video and other
> for VBI. But I guess this is not an issue since that's easier to standardize.

Not all devices have VBI. Some devices may have *only* VBI (although the last
driver of that kind was removed from the kernel a long time ago), there may
be multiple video source pads, and when we add HDMI I can think of a lot more
complex scenarios. So source pads shouldn't have their pad indices imposed on
them by outside 'arrangements'. It is really the wrong approach, regardless of
whether we talk about sink or source pads.

Regards,

	Hans

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Javier Martinez Canillas]

Hello Hans,

On 03/21/2016 04:30 PM, Hans Verkuil wrote:

[snip]

>>>>
>>>> Can you please provide an example of a media pipeline that user-space should
>>>> use with this approach? AFAICT whatever PADs are created when initiliazing
>>>> the PADs for an entity, will be exposed to user-space in the media graph.
>>>>
>>>> So I'm not understading how it will be used in practice. I don't mean that
>>>> your approach is not correct, is just I'm not getting it :)
>>>
>>> Why would userspace need to use the pads? This is for legacy drivers (right?)
>>> where the pipeline is fixed anyway.
>>>
>>
>> I asked because the user needs to setup the links in the media pipeline to
>> choose  which input connection will be linked to the tvp5150 decoder. But it
>> doesn't matter if we are going with the single sink pad approach since the
>> user will always do something like:
> 
> Why? The user will use an application that uses ENUM/S/G_INPUT for this. We're
> talking legacy drivers ('interface centric drivers' would be a better description)
> where we don't even expose the v4l-subdevX device nodes. Explicitly programming
> a media pipeline is something you do for complex devices (embedded systems and
> the like). Not for simple and generally fixed pipelines. Utterly pointless.
>

Mauro was talking about legacy 'interface centric' PC-consumer's hardware but
my test system is an embedded board that also has a tvp5150 decoder. The
board has an OMAP3 and the tvp5150 is attached to the SoC ISP. Is this one:

https://www.isee.biz/products/igep-expansion-boards/igepv2-expansion

As you can see, the board has 2 RCA connectors and each one is routed a tvp5150
composite input and both connectors can be used for S-Video. So the user needs
to setup the pipeline manually to choose which input connection to use.

But on a second read of the thread, it seems that you were referring to the
meta-pads only for the 'interace centric' drivers so maybe I misunderstood you.

Sorry for the noise if that was the case.
 
>>
>> $ media-ctl -r -l '"Composite0":0->"tvp5150 1-005c":0[1]'
>>
>> IOW, there will always choose the only connection source pad and tvp5150 sink.
>>
>> There will be two source pads for the tvp5150 though, 1 for video and other
>> for VBI. But I guess this is not an issue since that's easier to standardize.
> 
> Not all devices have VBI. Some devices may have *only* VBI (although the last
> driver of that kind was removed from the kernel a long time ago), there may
> be multiple video source pads, and when we add HDMI I can think of a lot more
> complex scenarios. So source pads shouldn't have their pad indices imposed on
> them by outside 'arrangements'. It is really the wrong approach, regardless of
> whether we talk about sink or source pads.
>

Ok, thanks for the explanation.
 
> Regards,
> 
> 	Hans
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Mauro Carvalho Chehab]

Em Mon, 21 Mar 2016 16:48:12 -0300
Javier Martinez Canillas <javier@osg.samsung.com> escreveu:

> Hello Hans,
> 
> On 03/21/2016 04:30 PM, Hans Verkuil wrote:
> 
> [snip]
> 
> >>>>
> >>>> Can you please provide an example of a media pipeline that user-space should
> >>>> use with this approach? AFAICT whatever PADs are created when initiliazing
> >>>> the PADs for an entity, will be exposed to user-space in the media graph.
> >>>>
> >>>> So I'm not understading how it will be used in practice. I don't mean that
> >>>> your approach is not correct, is just I'm not getting it :)  
> >>>
> >>> Why would userspace need to use the pads? This is for legacy drivers (right?)
> >>> where the pipeline is fixed anyway.
> >>>  
> >>
> >> I asked because the user needs to setup the links in the media pipeline to
> >> choose  which input connection will be linked to the tvp5150 decoder. But it
> >> doesn't matter if we are going with the single sink pad approach since the
> >> user will always do something like:  
> > 
> > Why? The user will use an application that uses ENUM/S/G_INPUT for this. We're
> > talking legacy drivers ('interface centric drivers' would be a better description)
> > where we don't even expose the v4l-subdevX device nodes. Explicitly programming
> > a media pipeline is something you do for complex devices (embedded systems and
> > the like). Not for simple and generally fixed pipelines. Utterly pointless.
> >  
> 
> Mauro was talking about legacy 'interface centric' PC-consumer's hardware but
> my test system is an embedded board that also has a tvp5150 decoder. The
> board has an OMAP3 and the tvp5150 is attached to the SoC ISP. Is this one:
> 
> https://www.isee.biz/products/igep-expansion-boards/igepv2-expansion

Yeah, subdevs should be prepared to work with both "interface centric" and
"media controller centric" approaches.

Yet, I don't think using one sink pad for tvp5150 is a bad thing.

> As you can see, the board has 2 RCA connectors and each one is routed a tvp5150
> composite input and both connectors can be used for S-Video. So the user needs
> to setup the pipeline manually to choose which input connection to use.

Actually, on your tests, you were not able to make this work, nor
S-Video is officially supported by the manufacturer. So, in the
specific case of IGEPv2, I would not be adding a S-Video connector.

Btw, even on devices with an S-Video connector and tvp5150, at
least on my tests, the driver were not able to setup S-Video. It
seems that there's something more than just setting the mux.

- 
Thanks,
Mauro

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Mauro Carvalho Chehab]

Em Mon, 21 Mar 2016 16:20:09 -0300
Javier Martinez Canillas <javier@osg.samsung.com> escreveu:

> > BTW, if the tvp5150 needs to know which composite connector is connected
> > to which hardware pin, then you still may need to be explicit w.r.t. the
> > number of pads. I just thought of that.
> >  
> 
> The tvp5150 doesn't care about that, as Mauro said is just a mux so you can
> have logic in the .link_setup that does the mux depending on the remote
> entity (that's in fact how I implemented and is currently in mainline).
> 
> Now, the user needs to know which entity is mapped to which input pin.
> All its know from the HW documentation is that for example the left
> RCA connector is AIP1A and the one inf the right is connected to AIP1B.
> 
> So there could be a convention that the composite connected to AIP1A pin
> (the default) is Composite0 and the connected to AIP1B is Composite1.


We should avoid a convention here... instead, we should support
properties via the properties API to export enough info for userspace
to know what physical connectors correspond to each "connection" entity.

As we've discussed previously, such properties can be:
	- connector's name
	- color
	- position
etc...


Regards,
Mauro

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Javier Martinez Canillas]

Hello Mauro,

On 03/21/2016 06:20 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 21 Mar 2016 16:20:09 -0300
> Javier Martinez Canillas <javier@osg.samsung.com> escreveu:
> 
>>> BTW, if the tvp5150 needs to know which composite connector is connected
>>> to which hardware pin, then you still may need to be explicit w.r.t. the
>>> number of pads. I just thought of that.
>>>  
>>
>> The tvp5150 doesn't care about that, as Mauro said is just a mux so you can
>> have logic in the .link_setup that does the mux depending on the remote
>> entity (that's in fact how I implemented and is currently in mainline).
>>
>> Now, the user needs to know which entity is mapped to which input pin.
>> All its know from the HW documentation is that for example the left
>> RCA connector is AIP1A and the one inf the right is connected to AIP1B.
>>
>> So there could be a convention that the composite connected to AIP1A pin
>> (the default) is Composite0 and the connected to AIP1B is Composite1.
> 
> 
> We should avoid a convention here... instead, we should support
> properties via the properties API to export enough info for userspace
> to know what physical connectors correspond to each "connection" entity.
> 
> As we've discussed previously, such properties can be:
> 	- connector's name
> 	- color
> 	- position
> etc...
>

Right, I forgot about the properties API.
 
> 
> Regards,
> Mauro
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America

Re: [RFC PATCH 1/3] [media] v4l2-mc.h: Add a S-Video C input PAD to demod enum

[Javier Martinez Canillas]

Hello Mauro,

On 03/21/2016 06:15 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 21 Mar 2016 16:48:12 -0300
> Javier Martinez Canillas <javier@osg.samsung.com> escreveu:
> 
>> Hello Hans,
>>
>> On 03/21/2016 04:30 PM, Hans Verkuil wrote:
>>
>> [snip]
>>
>>>>>>
>>>>>> Can you please provide an example of a media pipeline that user-space should
>>>>>> use with this approach? AFAICT whatever PADs are created when initiliazing
>>>>>> the PADs for an entity, will be exposed to user-space in the media graph.
>>>>>>
>>>>>> So I'm not understading how it will be used in practice. I don't mean that
>>>>>> your approach is not correct, is just I'm not getting it :)  
>>>>>
>>>>> Why would userspace need to use the pads? This is for legacy drivers (right?)
>>>>> where the pipeline is fixed anyway.
>>>>>  
>>>>
>>>> I asked because the user needs to setup the links in the media pipeline to
>>>> choose  which input connection will be linked to the tvp5150 decoder. But it
>>>> doesn't matter if we are going with the single sink pad approach since the
>>>> user will always do something like:  
>>>
>>> Why? The user will use an application that uses ENUM/S/G_INPUT for this. We're
>>> talking legacy drivers ('interface centric drivers' would be a better description)
>>> where we don't even expose the v4l-subdevX device nodes. Explicitly programming
>>> a media pipeline is something you do for complex devices (embedded systems and
>>> the like). Not for simple and generally fixed pipelines. Utterly pointless.
>>>  
>>
>> Mauro was talking about legacy 'interface centric' PC-consumer's hardware but
>> my test system is an embedded board that also has a tvp5150 decoder. The
>> board has an OMAP3 and the tvp5150 is attached to the SoC ISP. Is this one:
>>
>> https://www.isee.biz/products/igep-expansion-boards/igepv2-expansion
> 
> Yeah, subdevs should be prepared to work with both "interface centric" and
> "media controller centric" approaches.
> 
> Yet, I don't think using one sink pad for tvp5150 is a bad thing.
> 
>> As you can see, the board has 2 RCA connectors and each one is routed a tvp5150
>> composite input and both connectors can be used for S-Video. So the user needs
>> to setup the pipeline manually to choose which input connection to use.
> 
> Actually, on your tests, you were not able to make this work, nor
> S-Video is officially supported by the manufacturer. So, in the
> specific case of IGEPv2, I would not be adding a S-Video connector.
>

Yes, we can leave that for now. As you said I was not able to make it
work and when contacted an engineer working for the manufacturer, he
told me that in theory it should work but they have never tested it.

> Btw, even on devices with an S-Video connector and tvp5150, at
> least on my tests, the driver were not able to setup S-Video. It
> seems that there's something more than just setting the mux.
>

That could explain why it was not working for me, but in any case I
can focus on the two composite inputs for now that I can test on my
board easily. The S-video input support can be added as follow up.

> - 
> Thanks,
> Mauro
> 

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America