| Message ID | 1490894749.2404.33.camel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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); > >
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.
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. >
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
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.
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
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
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
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
> 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
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
>> 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
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
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 --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);
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(+)