diff mbox

[RFC,media] imx: assume MEDIA_ENT_F_ATV_DECODER entities output video on pad 1

Message ID 1490894749.2404.33.camel@pengutronix.de (mailing list archive)
State New, archived
Headers show

Commit Message

Philipp Zabel March 30, 2017, 5:25 p.m. UTC
The TVP5150 DT bindings specify a single output port (port 0) that
corresponds to the video output pad (pad 1, DEMOD_PAD_VID_OUT).

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
I'm trying to get this to work with a TVP5150 analog TV decoder, and the
first problem is that this device doesn't have pad 0 as its single
output pad. Instead, as a MEDIA_ENT_F_ATV_DECODER entity, it has for
pads (input, video out, vbi out, audio out), and video out is pad 1,
whereas the device tree only defines a single port (0).
---

 drivers/staging/media/imx/imx-media-dev.c | 9 +++++++++
 1 file changed, 9 insertions(+)

Comments

Steve Longerbeam April 4, 2017, 10:11 p.m. UTC | #1
On 03/30/2017 10:25 AM, Philipp Zabel wrote:
> The TVP5150 DT bindings specify a single output port (port 0) that
> corresponds to the video output pad (pad 1, DEMOD_PAD_VID_OUT).
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> I'm trying to get this to work with a TVP5150 analog TV decoder, and the
> first problem is that this device doesn't have pad 0 as its single
> output pad. Instead, as a MEDIA_ENT_F_ATV_DECODER entity, it has for
> pads (input, video out, vbi out, audio out), and video out is pad 1,
> whereas the device tree only defines a single port (0).

Shouldn't the DT bindings define ports for these other pads?
I haven't seen this documented anywhere, but shouldn't there
be a 1:1 correspondence between DT ports and media pads?

Steve


> ---
>
>  drivers/staging/media/imx/imx-media-dev.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
> index 17e2386a3ca3a..c52d6ca797965 100644
> --- a/drivers/staging/media/imx/imx-media-dev.c
> +++ b/drivers/staging/media/imx/imx-media-dev.c
> @@ -267,6 +267,15 @@ static int imx_media_create_link(struct imx_media_dev *imxmd,
>  	source_pad = link->local_pad;
>  	sink_pad = link->remote_pad;
>
> +	/*
> +	 * If the source subdev is an analog video decoder with a single source
> +	 * port, assume that this port 0 corresponds to the DEMOD_PAD_VID_OUT
> +	 * entity pad.
> +	 */
> +	if (source->entity.function == MEDIA_ENT_F_ATV_DECODER &&
> +	    local_sd->num_sink_pads == 0 && local_sd->num_src_pads == 1)
> +		source_pad = DEMOD_PAD_VID_OUT;
> +
>  	v4l2_info(&imxmd->v4l2_dev, "%s: %s:%d -> %s:%d\n", __func__,
>  		  source->name, source_pad, sink->name, sink_pad);
>
>
Russell King (Oracle) April 4, 2017, 11:10 p.m. UTC | #2
On Thu, Mar 30, 2017 at 07:25:49PM +0200, Philipp Zabel wrote:
> The TVP5150 DT bindings specify a single output port (port 0) that
> corresponds to the video output pad (pad 1, DEMOD_PAD_VID_OUT).
> 
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
> I'm trying to get this to work with a TVP5150 analog TV decoder, and the
> first problem is that this device doesn't have pad 0 as its single
> output pad. Instead, as a MEDIA_ENT_F_ATV_DECODER entity, it has for
> pads (input, video out, vbi out, audio out), and video out is pad 1,
> whereas the device tree only defines a single port (0).

Looking at the patch, it's highlighted another review point with
Steve's driver.

> diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
> index 17e2386a3ca3a..c52d6ca797965 100644
> --- a/drivers/staging/media/imx/imx-media-dev.c
> +++ b/drivers/staging/media/imx/imx-media-dev.c
> @@ -267,6 +267,15 @@ static int imx_media_create_link(struct imx_media_dev *imxmd,
>  	source_pad = link->local_pad;
>  	sink_pad = link->remote_pad;
>  
> +	/*
> +	 * If the source subdev is an analog video decoder with a single source
> +	 * port, assume that this port 0 corresponds to the DEMOD_PAD_VID_OUT
> +	 * entity pad.
> +	 */
> +	if (source->entity.function == MEDIA_ENT_F_ATV_DECODER &&
> +	    local_sd->num_sink_pads == 0 && local_sd->num_src_pads == 1)
> +		source_pad = DEMOD_PAD_VID_OUT;
> +
>  	v4l2_info(&imxmd->v4l2_dev, "%s: %s:%d -> %s:%d\n", __func__,
>  		  source->name, source_pad, sink->name, sink_pad);

What is "local" and what is "remote" here?  It seems that, in the case of
a link being created with the sensor(etc), it's all back to front.

Eg, I see locally:

imx-media: imx_media_create_link: imx219 0-0010:0 -> imx6-mipi-csi2:0

So here, "source" is the imx219 (the sensor), and sink is "imx6-mipi-csi2"
(part of the iMX6 capture.)  However, this makes "local_sd" the subdev of
the sensor, and "remote_sd" the subdev of the CSI2 interface - which is
totally back to front - this code is part of the iMX6 capture system,
so "local" implies that it should be part of that, and the "remote" thing
would be the sensor.

Hence, this seems completely confused.  I'd suggest that:

(a) the "pad->pad.flags & MEDIA_PAD_FL_SINK" test in imx_media_create_link()
    is moved into imx_media_create_links(), and placed here instead:

		for (j = 0; j < num_pads; j++) {
			pad = &local_sd->pad[j];

			if (pad->pad.flags & MEDIA_PAD_FL_SINK)
				continue;

			...
		}

    as the pad isn't going to spontaneously change this flag while we
    consider each individual link.  However, maybe the test should be:

			if (!(pad->pad.flags & MEDIA_PAD_FL_SOURCE))

    ?

(b) the terms "local" and "remote" in imx_media_create_link() are
    replaced with "source" and "sink" respectively, since this will
    now be called with a guaranteed source pad.

As for Philipp's solution, I'm not sure what the correct solution for
something like this is.  It depends how you view "hardware interface"
as defined by video-interfaces.txt, and whether the pads on the TVP5150
represent the hardware interfaces.  If we take the view that the pads
do represent hardware interfaces, then using the reg= property on the
port node will solve this problem.

If not, it would mean that we would have to have the iMX capture code
scan the pads on the source device, looking for outputs - but that
runs into a problem - if the source device has multiple outputs, does
the reg= property specify the output pad index or the pad number, and
what if one binding for a device specifies it one way and another
device's binding specifies it a different way.

There's lots of scope for making things really painful here, and
ending up with needing sensor-specific code in capture drivers to
work around different decisions on this.

I think someone needs to nail this down, if it's not already too late.
Steve Longerbeam April 5, 2017, 12:40 a.m. UTC | #3
On 04/04/2017 04:10 PM, Russell King - ARM Linux wrote:
> On Thu, Mar 30, 2017 at 07:25:49PM +0200, Philipp Zabel wrote:
>> The TVP5150 DT bindings specify a single output port (port 0) that
>> corresponds to the video output pad (pad 1, DEMOD_PAD_VID_OUT).
>>
>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> ---
>> I'm trying to get this to work with a TVP5150 analog TV decoder, and the
>> first problem is that this device doesn't have pad 0 as its single
>> output pad. Instead, as a MEDIA_ENT_F_ATV_DECODER entity, it has for
>> pads (input, video out, vbi out, audio out), and video out is pad 1,
>> whereas the device tree only defines a single port (0).
>
> Looking at the patch, it's highlighted another review point with
> Steve's driver.
>
>> diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
>> index 17e2386a3ca3a..c52d6ca797965 100644
>> --- a/drivers/staging/media/imx/imx-media-dev.c
>> +++ b/drivers/staging/media/imx/imx-media-dev.c
>> @@ -267,6 +267,15 @@ static int imx_media_create_link(struct imx_media_dev *imxmd,
>>  	source_pad = link->local_pad;
>>  	sink_pad = link->remote_pad;
>>
>> +	/*
>> +	 * If the source subdev is an analog video decoder with a single source
>> +	 * port, assume that this port 0 corresponds to the DEMOD_PAD_VID_OUT
>> +	 * entity pad.
>> +	 */
>> +	if (source->entity.function == MEDIA_ENT_F_ATV_DECODER &&
>> +	    local_sd->num_sink_pads == 0 && local_sd->num_src_pads == 1)
>> +		source_pad = DEMOD_PAD_VID_OUT;
>> +
>>  	v4l2_info(&imxmd->v4l2_dev, "%s: %s:%d -> %s:%d\n", __func__,
>>  		  source->name, source_pad, sink->name, sink_pad);
>
> What is "local" and what is "remote" here?  It seems that, in the case of
> a link being created with the sensor(etc), it's all back to front.
>
> Eg, I see locally:
>
> imx-media: imx_media_create_link: imx219 0-0010:0 -> imx6-mipi-csi2:0
>
> So here, "source" is the imx219 (the sensor), and sink is "imx6-mipi-csi2"
> (part of the iMX6 capture.)  However, this makes "local_sd" the subdev of
> the sensor, and "remote_sd" the subdev of the CSI2 interface - which is
> totally back to front - this code is part of the iMX6 capture system,
> so "local" implies that it should be part of that, and the "remote" thing
> would be the sensor.
>
> Hence, this seems completely confused.  I'd suggest that:
>
> (a) the "pad->pad.flags & MEDIA_PAD_FL_SINK" test in imx_media_create_link()
>     is moved into imx_media_create_links(), and placed here instead:
>
> 		for (j = 0; j < num_pads; j++) {
> 			pad = &local_sd->pad[j];
>
> 			if (pad->pad.flags & MEDIA_PAD_FL_SINK)
> 				continue;
>
> 			...
> 		}
>
>     as the pad isn't going to spontaneously change this flag while we
>     consider each individual link.


Sure, I can do that. It would avoid iterating unnecessarily through the
pad's links if the pad is a sink.


>  However, maybe the test should be:
>
> 			if (!(pad->pad.flags & MEDIA_PAD_FL_SOURCE))
>
>     ?
>

maybe that is more intuitive.


> (b) the terms "local" and "remote" in imx_media_create_link() are
>     replaced with "source" and "sink" respectively, since this will
>     now be called with a guaranteed source pad.

Agreed. I'll change the arg and local var names.

>
> As for Philipp's solution, I'm not sure what the correct solution for
> something like this is.  It depends how you view "hardware interface"
> as defined by video-interfaces.txt, and whether the pads on the TVP5150
> represent the hardware interfaces.  If we take the view that the pads
> do represent hardware interfaces, then using the reg= property on the
> port node will solve this problem.

And the missing port nodes would have to actually be defined first.
According to Philipp they aren't, only a single output port 0.


>
> If not, it would mean that we would have to have the iMX capture code
> scan the pads on the source device, looking for outputs - but that
> runs into a problem - if the source device has multiple outputs, does
> the reg= property specify the output pad index or the pad number,

And how do we even know the data direction of a DT port. Is it an input,
an output, bidirectional? The OF graph parsing in imx-media-of.c can't
determine a port's direction if it encounters a device it doesn't
recognize that has multiple ports. For now that is not really a problem
because upstream from the video mux and csi-2 receiver it's expected
there will only be original sources of video with only one source port.
But it can become a limitation later. For example a device that has
multiple output bus interfaces, which would require multiple output
ports.


Steve



> and what if one binding for a device specifies it one way and another
> device's binding specifies it a different way.
>
> There's lots of scope for making things really painful here, and
> ending up with needing sensor-specific code in capture drivers to
> work around different decisions on this.
>
> I think someone needs to nail this down, if it's not already too late.
>
Steve Longerbeam April 5, 2017, 12:44 a.m. UTC | #4
On 04/04/2017 05:40 PM, Steve Longerbeam wrote:
>
>
> On 04/04/2017 04:10 PM, Russell King - ARM Linux wrote:
>> On Thu, Mar 30, 2017 at 07:25:49PM +0200, Philipp Zabel wrote:
>>> The TVP5150 DT bindings specify a single output port (port 0) that
>>> corresponds to the video output pad (pad 1, DEMOD_PAD_VID_OUT).
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>> I'm trying to get this to work with a TVP5150 analog TV decoder, and the
>>> first problem is that this device doesn't have pad 0 as its single
>>> output pad. Instead, as a MEDIA_ENT_F_ATV_DECODER entity, it has for
>>> pads (input, video out, vbi out, audio out), and video out is pad 1,
>>> whereas the device tree only defines a single port (0).
>>
>> Looking at the patch, it's highlighted another review point with
>> Steve's driver.
>>
>>> diff --git a/drivers/staging/media/imx/imx-media-dev.c
>>> b/drivers/staging/media/imx/imx-media-dev.c
>>> index 17e2386a3ca3a..c52d6ca797965 100644
>>> --- a/drivers/staging/media/imx/imx-media-dev.c
>>> +++ b/drivers/staging/media/imx/imx-media-dev.c
>>> @@ -267,6 +267,15 @@ static int imx_media_create_link(struct
>>> imx_media_dev *imxmd,
>>>      source_pad = link->local_pad;
>>>      sink_pad = link->remote_pad;
>>>
>>> +    /*
>>> +     * If the source subdev is an analog video decoder with a single
>>> source
>>> +     * port, assume that this port 0 corresponds to the
>>> DEMOD_PAD_VID_OUT
>>> +     * entity pad.
>>> +     */
>>> +    if (source->entity.function == MEDIA_ENT_F_ATV_DECODER &&
>>> +        local_sd->num_sink_pads == 0 && local_sd->num_src_pads == 1)
>>> +        source_pad = DEMOD_PAD_VID_OUT;
>>> +
>>>      v4l2_info(&imxmd->v4l2_dev, "%s: %s:%d -> %s:%d\n", __func__,
>>>            source->name, source_pad, sink->name, sink_pad);
>>
>> What is "local" and what is "remote" here?  It seems that, in the case of
>> a link being created with the sensor(etc), it's all back to front.
>>
>> Eg, I see locally:
>>
>> imx-media: imx_media_create_link: imx219 0-0010:0 -> imx6-mipi-csi2:0
>>
>> So here, "source" is the imx219 (the sensor), and sink is
>> "imx6-mipi-csi2"
>> (part of the iMX6 capture.)  However, this makes "local_sd" the subdev of
>> the sensor, and "remote_sd" the subdev of the CSI2 interface - which is
>> totally back to front - this code is part of the iMX6 capture system,
>> so "local" implies that it should be part of that, and the "remote" thing
>> would be the sensor.
>>
>> Hence, this seems completely confused.  I'd suggest that:
>>
>> (a) the "pad->pad.flags & MEDIA_PAD_FL_SINK" test in
>> imx_media_create_link()
>>     is moved into imx_media_create_links(), and placed here instead:
>>
>>         for (j = 0; j < num_pads; j++) {
>>             pad = &local_sd->pad[j];
>>
>>             if (pad->pad.flags & MEDIA_PAD_FL_SINK)
>>                 continue;
>>
>>             ...
>>         }
>>
>>     as the pad isn't going to spontaneously change this flag while we
>>     consider each individual link.
>
>
> Sure, I can do that. It would avoid iterating unnecessarily through the
> pad's links if the pad is a sink.
>
>
>>  However, maybe the test should be:
>>
>>             if (!(pad->pad.flags & MEDIA_PAD_FL_SOURCE))
>>
>>     ?
>>
>
> maybe that is more intuitive.
>
>
>> (b) the terms "local" and "remote" in imx_media_create_link() are
>>     replaced with "source" and "sink" respectively, since this will
>>     now be called with a guaranteed source pad.
>
> Agreed. I'll change the arg and local var names.
>
>>
>> As for Philipp's solution, I'm not sure what the correct solution for
>> something like this is.  It depends how you view "hardware interface"
>> as defined by video-interfaces.txt, and whether the pads on the TVP5150
>> represent the hardware interfaces.  If we take the view that the pads
>> do represent hardware interfaces, then using the reg= property on the
>> port node will solve this problem.
>
> And the missing port nodes would have to actually be defined first.
> According to Philipp they aren't, only a single output port 0.
>
>
>>
>> If not, it would mean that we would have to have the iMX capture code
>> scan the pads on the source device, looking for outputs - but that
>> runs into a problem - if the source device has multiple outputs, does
>> the reg= property specify the output pad index or the pad number,
>
> And how do we even know the data direction of a DT port. Is it an input,
> an output, bidirectional? The OF graph parsing in imx-media-of.c can't
> determine a port's direction if it encounters a device it doesn't
> recognize that has multiple ports. For now that is not really a problem
> because upstream from the video mux and csi-2 receiver it's expected
> there will only be original sources of video with only one source port.
> But it can become a limitation later. For example a device that has
> multiple output bus interfaces, which would require multiple output
> ports.
>

Actually what was I thinking, the TVP5150 is already an example of
such a device.

All of this could be solved if there was some direction information
in port nodes.

Steve
Russell King (Oracle) April 5, 2017, 8:21 a.m. UTC | #5
On Tue, Apr 04, 2017 at 05:44:05PM -0700, Steve Longerbeam wrote:
> 
> 
> On 04/04/2017 05:40 PM, Steve Longerbeam wrote:
> >
> >
> >On 04/04/2017 04:10 PM, Russell King - ARM Linux wrote:
> >>On Thu, Mar 30, 2017 at 07:25:49PM +0200, Philipp Zabel wrote:
> >>>The TVP5150 DT bindings specify a single output port (port 0) that
> >>>corresponds to the video output pad (pad 1, DEMOD_PAD_VID_OUT).
> >>>
> >>>Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >>>---
> >>>I'm trying to get this to work with a TVP5150 analog TV decoder, and the
> >>>first problem is that this device doesn't have pad 0 as its single
> >>>output pad. Instead, as a MEDIA_ENT_F_ATV_DECODER entity, it has for
> >>>pads (input, video out, vbi out, audio out), and video out is pad 1,
> >>>whereas the device tree only defines a single port (0).
> >>
> >>Looking at the patch, it's highlighted another review point with
> >>Steve's driver.
> >>
> >>>diff --git a/drivers/staging/media/imx/imx-media-dev.c
> >>>b/drivers/staging/media/imx/imx-media-dev.c
> >>>index 17e2386a3ca3a..c52d6ca797965 100644
> >>>--- a/drivers/staging/media/imx/imx-media-dev.c
> >>>+++ b/drivers/staging/media/imx/imx-media-dev.c
> >>>@@ -267,6 +267,15 @@ static int imx_media_create_link(struct
> >>>imx_media_dev *imxmd,
> >>>     source_pad = link->local_pad;
> >>>     sink_pad = link->remote_pad;
> >>>
> >>>+    /*
> >>>+     * If the source subdev is an analog video decoder with a single
> >>>source
> >>>+     * port, assume that this port 0 corresponds to the
> >>>DEMOD_PAD_VID_OUT
> >>>+     * entity pad.
> >>>+     */
> >>>+    if (source->entity.function == MEDIA_ENT_F_ATV_DECODER &&
> >>>+        local_sd->num_sink_pads == 0 && local_sd->num_src_pads == 1)
> >>>+        source_pad = DEMOD_PAD_VID_OUT;
> >>>+
> >>>     v4l2_info(&imxmd->v4l2_dev, "%s: %s:%d -> %s:%d\n", __func__,
> >>>           source->name, source_pad, sink->name, sink_pad);
> >>
> >>What is "local" and what is "remote" here?  It seems that, in the case of
> >>a link being created with the sensor(etc), it's all back to front.
> >>
> >>Eg, I see locally:
> >>
> >>imx-media: imx_media_create_link: imx219 0-0010:0 -> imx6-mipi-csi2:0
> >>
> >>So here, "source" is the imx219 (the sensor), and sink is
> >>"imx6-mipi-csi2"
> >>(part of the iMX6 capture.)  However, this makes "local_sd" the subdev of
> >>the sensor, and "remote_sd" the subdev of the CSI2 interface - which is
> >>totally back to front - this code is part of the iMX6 capture system,
> >>so "local" implies that it should be part of that, and the "remote" thing
> >>would be the sensor.
> >>
> >>Hence, this seems completely confused.  I'd suggest that:
> >>
> >>(a) the "pad->pad.flags & MEDIA_PAD_FL_SINK" test in
> >>imx_media_create_link()
> >>    is moved into imx_media_create_links(), and placed here instead:
> >>
> >>        for (j = 0; j < num_pads; j++) {
> >>            pad = &local_sd->pad[j];
> >>
> >>            if (pad->pad.flags & MEDIA_PAD_FL_SINK)
> >>                continue;
> >>
> >>            ...
> >>        }
> >>
> >>    as the pad isn't going to spontaneously change this flag while we
> >>    consider each individual link.
> >
> >
> >Sure, I can do that. It would avoid iterating unnecessarily through the
> >pad's links if the pad is a sink.
> >
> >
> >> However, maybe the test should be:
> >>
> >>            if (!(pad->pad.flags & MEDIA_PAD_FL_SOURCE))
> >>
> >>    ?
> >>
> >
> >maybe that is more intuitive.
> >
> >
> >>(b) the terms "local" and "remote" in imx_media_create_link() are
> >>    replaced with "source" and "sink" respectively, since this will
> >>    now be called with a guaranteed source pad.
> >
> >Agreed. I'll change the arg and local var names.
> >
> >>
> >>As for Philipp's solution, I'm not sure what the correct solution for
> >>something like this is.  It depends how you view "hardware interface"
> >>as defined by video-interfaces.txt, and whether the pads on the TVP5150
> >>represent the hardware interfaces.  If we take the view that the pads
> >>do represent hardware interfaces, then using the reg= property on the
> >>port node will solve this problem.
> >
> >And the missing port nodes would have to actually be defined first.
> >According to Philipp they aren't, only a single output port 0.
> >
> >
> >>
> >>If not, it would mean that we would have to have the iMX capture code
> >>scan the pads on the source device, looking for outputs - but that
> >>runs into a problem - if the source device has multiple outputs, does
> >>the reg= property specify the output pad index or the pad number,
> >
> >And how do we even know the data direction of a DT port. Is it an input,
> >an output, bidirectional? The OF graph parsing in imx-media-of.c can't
> >determine a port's direction if it encounters a device it doesn't
> >recognize that has multiple ports. For now that is not really a problem
> >because upstream from the video mux and csi-2 receiver it's expected
> >there will only be original sources of video with only one source port.
> >But it can become a limitation later. For example a device that has
> >multiple output bus interfaces, which would require multiple output
> >ports.
> >
> 
> Actually what was I thinking, the TVP5150 is already an example of
> such a device.
> 
> All of this could be solved if there was some direction information
> in port nodes.

I disagree.

Philipp identified that the TVP5150 has four pads:

* Input pad
* Video output pad
* VBI output pad
* Audio output pad

So, it has one input and three outputs.  How does marking the direction
in the port node (which would indicate that there was a data flow out of
TVP5150 into the iMX6 capture) help identify which of those pads should
be used?

It would eliminate the input pad, but you still have three output pads
to choose from.

So no, your idea can't work.

As I already stated, I believe that this case is already covered by
video-interfaces.txt:

  If more than
  one port is present in a device node or there is more than one endpoint at a
  port, or port node needs to be associated with a selected hardware interface,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  a common scheme using '#address-cells', '#size-cells' and 'reg' properties is
  used.

So, according to that, you do not need to have more than one port node
to use the reg property - it's _either_ more than one port _or_ to
select the hardware interface.

It all hinges on whether you consider the video output, VBI output or
audio output to be separate hardware interfaces that the singular
specified "port" node needs to select between.

There's another reason why the TVP5150 binding looks wrong and broken,
however.  How does the audio output get routed to other parts of the
system if you're using the video output, and how is that relationship
defined?  It's a v4l2 subdev pad, so it would need to be part of the
v4l2 subdev graph.  It sounds to me like the binding was created with
a narrow focused "this is the board in front of me, it only has video
wired up, I'm not going to consider other use cases" blinkered view.
Philipp Zabel April 5, 2017, 9:34 a.m. UTC | #6
On Wed, 2017-04-05 at 09:21 +0100, Russell King - ARM Linux wrote:
[...]
> > Actually what was I thinking, the TVP5150 is already an example of
> > such a device.
> > 
> > All of this could be solved if there was some direction information
> > in port nodes.
> 
> I disagree.
>
> Philipp identified that the TVP5150 has four pads:
> 
> * Input pad
> * Video output pad
> * VBI output pad
> * Audio output pad

I didn't think hard enough about this earlier, but there are really only
two hardware interfaces on TVP5150. The ADC input, which can be
connected to either of two composite input pins (AIP1A, AIP1B), or use
both for s-video, and the digital output connected to pins (YOUT[7:0]).

VBI data can be transferred via the output pins during horizontal or
vertical blanking, if I understand correctly, or read from a FIFO via
I2C.

There is no apparent support for audio data whatsoever, and the only
mention of audio in the data manual is a vague reference about an "audio
interface available on the TVP5150" providing a clock to an audio
interface between an external audio decoder and the backend processor.

Further, commit 55606310e77f ("[media] tvp5150: create the expected
number of pads") creates DEMOD_NUM_PADS pads, but doesn't mention or
initialize the audio pad. It clearly expects the value of DEMOD_NUM_PADS
to be 3. And indeed the fourth pad was added later in commit
bddc418787cc ("[media] au0828: use standard demod pads struct").

So to me it looks like the VBI and audio pads should be removed from
TVP5150.

> So, it has one input and three outputs.  How does marking the direction
> in the port node (which would indicate that there was a data flow out of
> TVP5150 into the iMX6 capture) help identify which of those pads should
> be used?
>
> It would eliminate the input pad, but you still have three output pads
> to choose from.
> 
> So no, your idea can't work.

In this case, removal of the VBI and audio pads might make this work,
but in general this is true. In my opinion, to make this truly generic,
we need an interface to ask the driver which media entity pad a given
device tree port corresponds to, as there might not even be a single
media entity corresponding to all ports for more complex devices.

> As I already stated, I believe that this case is already covered by
> video-interfaces.txt:
> 
>   If more than
>   one port is present in a device node or there is more than one endpoint at a
>   port, or port node needs to be associated with a selected hardware interface,
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>   a common scheme using '#address-cells', '#size-cells' and 'reg' properties is
>   used.
> 
> So, according to that, you do not need to have more than one port node
> to use the reg property - it's _either_ more than one port _or_ to
> select the hardware interface.

I don't think enforcing a 1:1 correspondence between device tree node
and media entity and enforcing port reg property == entity pad index is
a good idea in the long run. Binding errors are going to be made that
will have to be worked around at the driver level, and more complex
devices might have to create multiple media entities / v4l2 subdevices
with external ports that interface with the of graph.

> It all hinges on whether you consider the video output, VBI output or
> audio output to be separate hardware interfaces that the singular
> specified "port" node needs to select between.
> 
> There's another reason why the TVP5150 binding looks wrong and broken,
> however.  How does the audio output get routed to other parts of the
> system if you're using the video output, and how is that relationship
> defined?  It's a v4l2 subdev pad, so it would need to be part of the
> v4l2 subdev graph.  It sounds to me like the binding was created with
> a narrow focused "this is the board in front of me, it only has video
> wired up, I'm not going to consider other use cases" blinkered view.

I think the output part is accurate, as the audio pad is an artifact of
an unrelated change. I'm not so sure about the VBI pad, but I think that
shouldn't exist either. The input pad, on the other hand, not having any
of graph representation in the device tree seems a bit strange. There
was a custom binding for the inputs, that got quickly reverted:
https://patchwork.kernel.org/patch/8395521/

regards
Philipp
Philipp Zabel April 5, 2017, 9:43 a.m. UTC | #7
On Tue, 2017-04-04 at 15:11 -0700, Steve Longerbeam wrote:
> 
> On 03/30/2017 10:25 AM, Philipp Zabel wrote:
> > The TVP5150 DT bindings specify a single output port (port 0) that
> > corresponds to the video output pad (pad 1, DEMOD_PAD_VID_OUT).
> >
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> > I'm trying to get this to work with a TVP5150 analog TV decoder, and the
> > first problem is that this device doesn't have pad 0 as its single
> > output pad. Instead, as a MEDIA_ENT_F_ATV_DECODER entity, it has for
> > pads (input, video out, vbi out, audio out), and video out is pad 1,
> > whereas the device tree only defines a single port (0).
> 
> Shouldn't the DT bindings define ports for these other pads?

In this case, probably yes for the input pad, certainly no for the
VBI/audio pads. See the other mail.

> I haven't seen this documented anywhere, but shouldn't there
> be a 1:1 correspondence between DT ports and media pads?

Not in general. But imagine a dual HDMI->CSI2 converter, for example. We
don't support this yet, but that might reasonably be described in the
device tree as a single device with two output ports. But the internal
representation would be two completely separate v4l2 subdevices.

regards
Philipp
Javier Martinez Canillas April 5, 2017, 1:55 p.m. UTC | #8
Hello Philipp,

On Wed, Apr 5, 2017 at 5:34 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> On Wed, 2017-04-05 at 09:21 +0100, Russell King - ARM Linux wrote:

[snip]

> I think the output part is accurate, as the audio pad is an artifact of
> an unrelated change. I'm not so sure about the VBI pad, but I think that
> shouldn't exist either. The input pad, on the other hand, not having any
> of graph representation in the device tree seems a bit strange. There

Agreed, there should be a OF graph representation (and also a MC
representation) of the input PADs.

The tvp5150 driver currently has hardcoded as input TVP5150_COMPOSITE1
(AIP1B), so it won't work for a board that has the composite connector
wired to TVP5150_COMPOSITE0 (AIP1A) neither will work for S-Video
(AIP1A + AIP1B).

> was a custom binding for the inputs, that got quickly reverted:
> https://patchwork.kernel.org/patch/8395521/
>

Yes, that was my first attempt to have input connector support for
tvp5150. The patches were merged without a detailed review of the DT
bindings and latter were found to be wrong. Instead of having a driver
specific DT binding, a generic binding that could be used by any
device should be defined.

The latest proposal was [0] but that was also found to be inadequate.
After a lot of discussion on #v4l, the best approach we could come
with was something like like [1]. But I was busy with other stuff and
never found time to do the driver changes.

By the way, only the DT bindings portion from the first approach got
reverted but the patch reverting the driver changes never made to
mainline. Could you please test if [2] doesn't cause any issues to you
so the left over can be finally removed?

> regards
> Philipp
>

[0]: https://lkml.org/lkml/2016/4/12/983
[1]: https://hastebin.com/kadagidino.diff
[2]: https://patchwork.kernel.org/patch/9472623/

Best regards,
Javier
Mauro Carvalho Chehab April 5, 2017, 2:53 p.m. UTC | #9
Em Wed, 05 Apr 2017 11:34:19 +0200
Philipp Zabel <p.zabel@pengutronix.de> escreveu:

> On Wed, 2017-04-05 at 09:21 +0100, Russell King - ARM Linux wrote:
> [...]
> > > Actually what was I thinking, the TVP5150 is already an example of
> > > such a device.
> > > 
> > > All of this could be solved if there was some direction information
> > > in port nodes.  
> > 
> > I disagree.
> >
> > Philipp identified that the TVP5150 has four pads:
> > 
> > * Input pad
> > * Video output pad
> > * VBI output pad
> > * Audio output pad  
> 
> I didn't think hard enough about this earlier, but there are really only
> two hardware interfaces on TVP5150. The ADC input, which can be
> connected to either of two composite input pins (AIP1A, AIP1B), or use
> both for s-video, and the digital output connected to pins (YOUT[7:0]).
> 
> VBI data can be transferred via the output pins during horizontal or
> vertical blanking, if I understand correctly, or read from a FIFO via
> I2C.
> 
> There is no apparent support for audio data whatsoever, and the only
> mention of audio in the data manual is a vague reference about an "audio
> interface available on the TVP5150" providing a clock to an audio
> interface between an external audio decoder and the backend processor.
> 
> Further, commit 55606310e77f ("[media] tvp5150: create the expected
> number of pads") creates DEMOD_NUM_PADS pads, but doesn't mention or
> initialize the audio pad. It clearly expects the value of DEMOD_NUM_PADS
> to be 3. And indeed the fourth pad was added later in commit
> bddc418787cc ("[media] au0828: use standard demod pads struct").
> 
> So to me it looks like the VBI and audio pads should be removed from
> TVP5150.

There are a number of drivers that can work with different
types of TV demodulators. Typical examples of such hardware can be
found at em28xx, saa7134, cx88 drivers (among lots of other drivers).
Those drivers don't use the subdev API. Instead, they use a generic
helper function with sets the pipelines, based on the pad number.

The problem here is that, currently, both MC API and MC core
lacks a way to identify PAD ports per type, as the only information
that a bridge driver has is a pad number. So, in order for a
generic helper function to work, we had to hardcode pad numbers,
in a way that it would work for all possible types of demods.

It shouldn't be hard to add a "pad_type" information at media_pad
struct, but passing such info to userspace requires a new API
(we're calling it as "properties API"). Sakari was meant to send
us an updated RFC for it[1] with a patchset, back in 2015, but
this never happened.

[1] https://linuxtv.org/news.php?entry=2015-08-17.mchehab

Each vendor chooses the demod using some random criteria 
(usually, they use whatever costs less by the time they create a
hardware model). Newer versions of the same hardware frequently
has a different model.

For example, in the case of em28xx driver, there are currently
several types of demods supported there, like (among other options):

- demods with tda9887, with is actually part of a tuner that has 
  an outputs for IF luminance, IF chrominance and IF audio;

- demods with xc3028, with is an integrated tuner + audio demod,
  with outputs both video as a baseband signal and audio data via IF,
  (or outputs a single IF signal, for Digital TV);

- demods with both xc3028 and msp3400. The last one transforms
  audio IF into I2S;

- demods with saa711x (supported by saa7115 driver), with may or may 
  not have raw VBI and/or sliced VBI, depending on the specific model,
  with is detected during driver's probe.

The generic function should be robust enough to handle all above
cases.

Without a way to report the PAD type to userspace, applications
that need to setup or recognize such pipelines need to hardcode the
pad numbers on userspace. That's, by the way, one of the issues why
it is currently impossible to write a full generic MC plugin at libv4l:
there's currently no way for userspace to recognize what type of
signals each PAD input or output carries.

So, in short, the tvp5150 demod doesn't decode audio, but there
are other demods that do it.

In the case of VBI, tvp5150 has actually two ways of reporting
it:

1) via YOUT[7:0] pins. VBI information is transmitted as a
   set of raw samples, via an ancillary data block, during
   vertical/horizontal blanking intervals. So, yes, it shares
   the same hardware output, although the VBI contents are
   actually multiplexed there. Please notice that not all
   video out PADS encapsulate raw VBI the same way as tvp5150
   (and some devices even don't support raw VBI, like saa7110 and
   some models supported by saa7115 driver).

2) via an interrupt that indicates that it decoded VBI data. The
   VBI information itself is there on FIFO, accessible via a set of
   registers (see "VBI Data processor" chapter at the datasheet).

Currently, the driver doesn't support (2), because, at the time
I wrote the driver, I didn't find a way to read the interrupts generated
by tvp5150 at em28xx[1], due to the lack of em28xx documentation,
but adding support for it shoudn't be hard. I may eventually do it
when I have some time to play with my ISEE hardware.

So, in the case of tvp5150 hardware, have those PADS:

- Input baseband;
- Video + raw VBI output;
- sliced VBI output.

Yet, we need an always unconnected audio output, in order to support
different demods out there.

[1] tvp5150 was written to support some em28xx-based devices

> 
> > So, it has one input and three outputs.  How does marking the direction
> > in the port node (which would indicate that there was a data flow out of
> > TVP5150 into the iMX6 capture) help identify which of those pads should
> > be used?
> >
> > It would eliminate the input pad, but you still have three output pads
> > to choose from.
> > 
> > So no, your idea can't work.  
> 
> In this case, removal of the VBI and audio pads might make this work,
> but in general this is true. In my opinion, to make this truly generic,
> we need an interface to ask the driver which media entity pad a given
> device tree port corresponds to, as there might not even be a single
> media entity corresponding to all ports for more complex devices.

Yes. We also need something like that at the userspace API.

Thanks,
Mauro
Devin Heitmueller April 5, 2017, 3:39 p.m. UTC | #10
> Currently, the driver doesn't support (2), because, at the time
> I wrote the driver, I didn't find a way to read the interrupts generated
> by tvp5150 at em28xx[1], due to the lack of em28xx documentation,
> but adding support for it shoudn't be hard. I may eventually do it
> when I have some time to play with my ISEE hardware.

For what it's worth, I doubt most of the em28xx designs have the
tvp5150 interrupt request line connected in any way.  You would likely
have to poll the FIFO status register via I2C, or use the feature to
embed the sliced data into as VANC data in the 656 output (as
described in sec 3.9 of the tvp5150am1 spec).

Devin
Mauro Carvalho Chehab April 5, 2017, 4:17 p.m. UTC | #11
Em Wed, 5 Apr 2017 11:39:06 -0400
Devin Heitmueller <dheitmueller@kernellabs.com> escreveu:

> > Currently, the driver doesn't support (2), because, at the time
> > I wrote the driver, I didn't find a way to read the interrupts generated
> > by tvp5150 at em28xx[1], due to the lack of em28xx documentation,
> > but adding support for it shoudn't be hard. I may eventually do it
> > when I have some time to play with my ISEE hardware.  
> 
> For what it's worth, I doubt most of the em28xx designs have the
> tvp5150 interrupt request line connected in any way.

True. But, on embedded hardware, such line may be connected into the
SoC. Actually, from the IGEPv3 expansion diagram:

	https://www.isee.biz/support/downloads/item/igepv2-expansion-rc-schematics

The INT line is connected to CAM_IRQ. That's connected to GPIO_154 pin
at OMAP3.

So, on a first glance, it seems possible to use it, instead of polling.

> You would likely
> have to poll the FIFO status register via I2C,

Yes, I considered this option when I wrote the driver. It could work, 
although it would likely have some performance drawback, as the driver
would need to poll it at least 60 times per second.

> or use the feature to
> embed the sliced data into as VANC data in the 656 output (as
> described in sec 3.9 of the tvp5150am1 spec).

True, but the bridge driver would need to handle such data. 

I remember I looked on this when I wrote the driver, but I was
unable to find a way for em28xx to parse (or forward) such
data packets.

Thanks,
Mauro
Devin Heitmueller April 5, 2017, 5:02 p.m. UTC | #12
>> For what it's worth, I doubt most of the em28xx designs have the
>> tvp5150 interrupt request line connected in any way.
>
> True. But, on embedded hardware, such line may be connected into the
> SoC. Actually, from the IGEPv3 expansion diagram:
>
>         https://www.isee.biz/support/downloads/item/igepv2-expansion-rc-schematics
>
> The INT line is connected to CAM_IRQ. That's connected to GPIO_154 pin
> at OMAP3.
>
> So, on a first glance, it seems possible to use it, instead of polling.

To be clear, I wasn't suggesting that the IRQ request line on the
tvp5150 couldn't be supported in general (for example, for those
embedded targets which have it wired up to a host processor).  I'm
just saying you shouldn't expect it to work on most (perhaps all)
em28xx designs which have the tvp5150.  In fact on some em28xx designs
the pin is used as a GPIO output tied to a mux to control input
selection.  Hence blindly enabling the interrupt request line by
default would do all sorts of bad things.

>> You would likely
>> have to poll the FIFO status register via I2C,
>
> Yes, I considered this option when I wrote the driver. It could work,
> although it would likely have some performance drawback, as the driver
> would need to poll it at least 60 times per second.
>
>> or use the feature to
>> embed the sliced data into as VANC data in the 656 output (as
>> described in sec 3.9 of the tvp5150am1 spec).
>
> True, but the bridge driver would need to handle such data.

Correct.

> I remember I looked on this when I wrote the driver, but I was
> unable to find a way for em28xx to parse (or forward) such
> data packets.

I'm pretty sure it's possible, but I haven't looked at the datasheets
in a number of years and don't recall the details.

Hardware VBI splicing is supported by a number of decoders but it's
rarely used on commodity PCs (the Conexant and NXP decoders support it
as well).  That said, I won't argue there might be some value on
really low end platforms.  All I would ask is that if you do introduce
any such functionality into the tvp5150 driver for some embedded
application that you please not break support for devices such as the
em28xx.

Thanks,

Devin
Mauro Carvalho Chehab April 5, 2017, 5:16 p.m. UTC | #13
Em Wed, 5 Apr 2017 13:02:52 -0400
Devin Heitmueller <dheitmueller@kernellabs.com> escreveu:

> > I remember I looked on this when I wrote the driver, but I was
> > unable to find a way for em28xx to parse (or forward) such
> > data packets.  
> 
> I'm pretty sure it's possible, but I haven't looked at the datasheets
> in a number of years and don't recall the details.
> 
> Hardware VBI splicing is supported by a number of decoders but it's
> rarely used on commodity PCs (the Conexant and NXP decoders support it
> as well).  That said, I won't argue there might be some value on
> really low end platforms.  All I would ask is that if you do introduce
> any such functionality into the tvp5150 driver for some embedded
> application that you please not break support for devices such as the
> em28xx.

Yeah, sure. If I write such patchset[1], it will be optional, and
will depend on a DT variable (or platform_data) setup that would
tell what GPIO pin should be used for interrupt.

Not providing it should either disable such feature or enable it
via polling.

[1] Please notice that I don't have any demand of doing it. Yet,
I may do it for fun on my spare time:-)

I added in the past an initial support for sliced VBI API on em28xx, 
with got reverted on changeset 1d179eeedc8cb48712bc236ec82ec6c63af42008, 
mainly due to the lack of such feature on tvp5150. So, such code was never
tested (and likely need fixes/updates), but if I end by adding sliced VBI
support on tvp5150 and on OMAP3, I may add support for it on em28xx too,
using I2C poll. On such case, I'll likely add a modprobe parameter to
enable such feature.

Thanks,
Mauro
Philipp Zabel April 6, 2017, 9:57 a.m. UTC | #14
On Wed, 2017-04-05 at 11:53 -0300, Mauro Carvalho Chehab wrote:
[...]
> There are a number of drivers that can work with different
> types of TV demodulators. Typical examples of such hardware can be
> found at em28xx, saa7134, cx88 drivers (among lots of other drivers).
> Those drivers don't use the subdev API. Instead, they use a generic
> helper function with sets the pipelines, based on the pad number.
> 
> The problem here is that, currently, both MC API and MC core
> lacks a way to identify PAD ports per type, as the only information
> that a bridge driver has is a pad number. So, in order for a
> generic helper function to work, we had to hardcode pad numbers,
> in a way that it would work for all possible types of demods.
> 
> It shouldn't be hard to add a "pad_type" information at media_pad
> struct, but passing such info to userspace requires a new API
> (we're calling it as "properties API"). Sakari was meant to send
> us an updated RFC for it[1] with a patchset, back in 2015, but
> this never happened.
> 
> [1] https://linuxtv.org/news.php?entry=2015-08-17.mchehab
[...]

That would be most useful.

> So, in short, the tvp5150 demod doesn't decode audio, but there
> are other demods that do it.
> 
> In the case of VBI, tvp5150 has actually two ways of reporting
> it:
> 
> 1) via YOUT[7:0] pins. VBI information is transmitted as a
>    set of raw samples, via an ancillary data block, during
>    vertical/horizontal blanking intervals. So, yes, it shares
>    the same hardware output, although the VBI contents are
>    actually multiplexed there. Please notice that not all
>    video out PADS encapsulate raw VBI the same way as tvp5150
>    (and some devices even don't support raw VBI, like saa7110 and
>    some models supported by saa7115 driver).

This is the physical interface that corresponds to the output port
(should be port@1) in the device tree. It should correspond to the video
output media entity pad.
What is unclear to is me whether the VBI media entity pad also should
correspond to the same physical interface / DT port.

> 2) via an interrupt that indicates that it decoded VBI data. The
>    VBI information itself is there on FIFO, accessible via a set of
>    registers (see "VBI Data processor" chapter at the datasheet).
>
> Currently, the driver doesn't support (2), because, at the time
> I wrote the driver, I didn't find a way to read the interrupts generated
> by tvp5150 at em28xx[1], due to the lack of em28xx documentation,
> but adding support for it shoudn't be hard. I may eventually do it
> when I have some time to play with my ISEE hardware.
> 
> So, in the case of tvp5150 hardware, have those PADS:
> 
> - Input baseband;
> - Video + raw VBI output;
> - sliced VBI output.

This DEMOD_PAD_VBI_OUT, does it correspond to 1) or 2) above?

> Yet, we need an always unconnected audio output, in order to support
> different demods out there.

Are you saying we have to keep pad[DEMOD_PAD_AUDIO_OUT] in tvp5150 even
though it doesn't exist because the framework can't cope with an
audio-less ATV_DECODER that only has three pads?

> [1] tvp5150 was written to support some em28xx-based devices
> 
> > 
> > > So, it has one input and three outputs.  How does marking the direction
> > > in the port node (which would indicate that there was a data flow out of
> > > TVP5150 into the iMX6 capture) help identify which of those pads should
> > > be used?
> > >
> > > It would eliminate the input pad, but you still have three output pads
> > > to choose from.
> > > 
> > > So no, your idea can't work.  
> > 
> > In this case, removal of the VBI and audio pads might make this work,
> > but in general this is true. In my opinion, to make this truly generic,
> > we need an interface to ask the driver which media entity pad a given
> > device tree port corresponds to, as there might not even be a single
> > media entity corresponding to all ports for more complex devices.
> 
> Yes. We also need something like that at the userspace API.

thanks
Philipp
diff mbox

Patch

diff --git a/drivers/staging/media/imx/imx-media-dev.c b/drivers/staging/media/imx/imx-media-dev.c
index 17e2386a3ca3a..c52d6ca797965 100644
--- a/drivers/staging/media/imx/imx-media-dev.c
+++ b/drivers/staging/media/imx/imx-media-dev.c
@@ -267,6 +267,15 @@  static int imx_media_create_link(struct imx_media_dev *imxmd,
 	source_pad = link->local_pad;
 	sink_pad = link->remote_pad;
 
+	/*
+	 * If the source subdev is an analog video decoder with a single source
+	 * port, assume that this port 0 corresponds to the DEMOD_PAD_VID_OUT
+	 * entity pad.
+	 */
+	if (source->entity.function == MEDIA_ENT_F_ATV_DECODER &&
+	    local_sd->num_sink_pads == 0 && local_sd->num_src_pads == 1)
+		source_pad = DEMOD_PAD_VID_OUT;
+
 	v4l2_info(&imxmd->v4l2_dev, "%s: %s:%d -> %s:%d\n", __func__,
 		  source->name, source_pad, sink->name, sink_pad);