Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 14:47:51 +0100
Hans Verkuil  escreveu:

> On 02/05/2018 02:17 PM, Mauro Carvalho Chehab wrote:
> > Em Mon, 5 Feb 2018 12:55:21 +0100
> > Hans Verkuil  escreveu:
> >   
> >> On 02/05/2018 12:15 PM, Mauro Carvalho Chehab wrote:  
> >>> Hi Hans,
> >>>
> >>> Em Sun, 4 Feb 2018 14:06:42 +0100
> >>> Hans Verkuil  escreveu:
> >>> 
>  Hi Mauro,
> 
>  I'm working on adding proper compliance tests for the MC but I think 
>  something
>  is missing in the G_TOPOLOGY ioctl w.r.t. pads.
> 
>  In several v4l-subdev ioctls you need to pass the pad. There the pad is 
>  an index
>  for the corresponding entity. I.e. an entity has 3 pads, so the pad 
>  argument is
>  [0-2].
> 
>  The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't 
>  use that
>  in the v4l-subdev ioctls, so how do I translate that to a pad index in 
>  my application?
> 
>  It seems to be a missing feature in the API. I assume this information 
>  is available
>  in the core, so then I would add a field to struct media_v2_pad with the 
>  pad index
>  for the entity.
> >>>
> >>> No, this was designed this way by purpose, back in 2015, when this was
> >>> discussed at the first MC workshop. It was a consensus on that time that
> >>> parameters like PAD index, PAD name, etc should be implemented via the
> >>> properties API, as proposed by Sakari [1]. We also had a few discussions 
> >>> about that on both IRC and ML.
> >>>
> >>> [1] See: https://linuxtv.org/news.php?entry=2015-08-17.mchehab
> >>>
> >>>   3.3 Action items
> >>>   ...
> >>>   media_properties: RFC userspace API: Sakari
> >>>
> >>> Unfortunately, Sakari never found the time to send us a patch series
> >>> implementing a properties API, even as RFC.
> >>>
> >>> One of the other missing features is to add a new version for setting
> >>> links (I guess we called it as S_LINKS_V2 at the meetings there).
> >>> The hole idea is to provide a way to setup the entire pipeline using
> >>> a single ioctl, on an atomic way.
> >>>
> >>> The big problem with pad indexes happen on entities that have PADs with
> >>> different types of signal input or output, like for example a TV tuner.
> >>>
> >>> On almost all PC consumers hardware supported by the Kernel, the same
> >>> PCI/USB ID may come with different types of hardware. This may also
> >>> happen with embedded TV sets, as, depending on the market is is
> >>> sold, the same product may come with a different tuner.
> >>>
> >>> A "classic" TV tuner usually has those types of output signals:
> >>>
> >>>   - analog TV luminance IF;
> >>>   - analog TV chrominance IF [1];
> >>>   - digital TV OFDM IF;
> >>>   - audio IF;
> >>>
> >>> [1] As both luminance and chrominance should go to the same TV
> >>> demod, in practice, we can group both signals together on a
> >>> single pad.
> >>>
> >>> More modern tuners also have an audio demod integrated, meaning that
> >>> they also have another PAD:
> >>>   - digital mono or stereo audio.
> >>>
> >>> At the simplest possible scenario, let's say that we have a TV device
> >>> has those entities (among others), each with a single PAD input:
> >>>
> >>>   - entity #0: a TV tuner;
> >>>   - entity #1: an audio demod;
> >>>   - entity #2: an analog TV demod;
> >>>   - entity #3: a digital TV demod.
> >>>
> >>> And the TV tuner has 4 output pads:
> >>>
> >>>   - pad #0 -> analog TV luminance/chrominance;
> >>>   - pad #1 -> digital TV IF;
> >>>   - pad #2 -> audio IF;
> >>>   - pad #3 -> digital audio.
> >>>
> >>> There, pad #0 can only be connected to entity #2. You cannot
> >>> connect any other pad to entity #2. The same apply to every other
> >>> TV tuner output PAD.
> >>>
> >>> In this specific scenario, entity #1 can only be connected to pad #2,
> >>> and pad #3 can't be connected anywhere, as, on this model opted to
> >>> have a separate audio demod, and didn't wired the digital audio output.
> >>> Yet, the subdev has it.
> >>>
> >>> Another TV set may have different pad numbers - placing them even on a
> >>> different order, and opting to use the audio demod tuner, wiring the
> >>> digital audio output.
> >>>
> >>> Currently, there's no way for an userspace application to know what pads
> >>> can be linked to each entity, as there's no way to tell userspace what
> >>> kind of signal each pad supports.
> >>>
> >>> In any case, the Kernel should either detect the tuner model or know
> >>> it (via a different DT entry), but userspace need such information,
> >>> in order to be able to properly set the pipelines.
> >>>
> >>> So, what we really need here is passing an optional set of properties
> >>> to userspace in order to allow it to do the right thing.
> >>
> >> I fail to see the problem. Entities have pads. Pads have both a unique
> >> ID and an index within the entity pad list. I.e. the pad ID and the
> >> 

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Hans Verkuil]

On 02/05/2018 02:17 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 5 Feb 2018 12:55:21 +0100
> Hans Verkuil  escreveu:
> 
>> On 02/05/2018 12:15 PM, Mauro Carvalho Chehab wrote:
>>> Hi Hans,
>>>
>>> Em Sun, 4 Feb 2018 14:06:42 +0100
>>> Hans Verkuil  escreveu:
>>>   
 Hi Mauro,

 I'm working on adding proper compliance tests for the MC but I think 
 something
 is missing in the G_TOPOLOGY ioctl w.r.t. pads.

 In several v4l-subdev ioctls you need to pass the pad. There the pad is an 
 index
 for the corresponding entity. I.e. an entity has 3 pads, so the pad 
 argument is
 [0-2].

 The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use 
 that
 in the v4l-subdev ioctls, so how do I translate that to a pad index in my 
 application?

 It seems to be a missing feature in the API. I assume this information is 
 available
 in the core, so then I would add a field to struct media_v2_pad with the 
 pad index
 for the entity.  
>>>
>>> No, this was designed this way by purpose, back in 2015, when this was
>>> discussed at the first MC workshop. It was a consensus on that time that
>>> parameters like PAD index, PAD name, etc should be implemented via the
>>> properties API, as proposed by Sakari [1]. We also had a few discussions 
>>> about that on both IRC and ML.
>>>
>>> [1] See: https://linuxtv.org/news.php?entry=2015-08-17.mchehab
>>>
>>> 3.3 Action items
>>> ...
>>> media_properties: RFC userspace API: Sakari
>>>
>>> Unfortunately, Sakari never found the time to send us a patch series
>>> implementing a properties API, even as RFC.
>>>
>>> One of the other missing features is to add a new version for setting
>>> links (I guess we called it as S_LINKS_V2 at the meetings there).
>>> The hole idea is to provide a way to setup the entire pipeline using
>>> a single ioctl, on an atomic way.
>>>
>>> The big problem with pad indexes happen on entities that have PADs with
>>> different types of signal input or output, like for example a TV tuner.
>>>
>>> On almost all PC consumers hardware supported by the Kernel, the same
>>> PCI/USB ID may come with different types of hardware. This may also
>>> happen with embedded TV sets, as, depending on the market is is
>>> sold, the same product may come with a different tuner.
>>>
>>> A "classic" TV tuner usually has those types of output signals:
>>>
>>> - analog TV luminance IF;
>>> - analog TV chrominance IF [1];
>>> - digital TV OFDM IF;
>>> - audio IF;
>>>
>>> [1] As both luminance and chrominance should go to the same TV
>>> demod, in practice, we can group both signals together on a
>>> single pad.
>>>
>>> More modern tuners also have an audio demod integrated, meaning that
>>> they also have another PAD:
>>> - digital mono or stereo audio.
>>>
>>> At the simplest possible scenario, let's say that we have a TV device
>>> has those entities (among others), each with a single PAD input:
>>>
>>> - entity #0: a TV tuner;
>>> - entity #1: an audio demod;
>>> - entity #2: an analog TV demod;
>>> - entity #3: a digital TV demod.
>>>
>>> And the TV tuner has 4 output pads:
>>>
>>> - pad #0 -> analog TV luminance/chrominance;
>>> - pad #1 -> digital TV IF;
>>> - pad #2 -> audio IF;
>>> - pad #3 -> digital audio.
>>>
>>> There, pad #0 can only be connected to entity #2. You cannot
>>> connect any other pad to entity #2. The same apply to every other
>>> TV tuner output PAD.
>>>
>>> In this specific scenario, entity #1 can only be connected to pad #2,
>>> and pad #3 can't be connected anywhere, as, on this model opted to
>>> have a separate audio demod, and didn't wired the digital audio output.
>>> Yet, the subdev has it.
>>>
>>> Another TV set may have different pad numbers - placing them even on a
>>> different order, and opting to use the audio demod tuner, wiring the
>>> digital audio output.
>>>
>>> Currently, there's no way for an userspace application to know what pads
>>> can be linked to each entity, as there's no way to tell userspace what
>>> kind of signal each pad supports.
>>>
>>> In any case, the Kernel should either detect the tuner model or know
>>> it (via a different DT entry), but userspace need such information,
>>> in order to be able to properly set the pipelines.
>>>
>>> So, what we really need here is passing an optional set of properties
>>> to userspace in order to allow it to do the right thing.  
>>
>> I fail to see the problem. Entities have pads. Pads have both a unique
>> ID and an index within the entity pad list. I.e. the pad ID and the
>> (entity ID + pad index) tuple both uniquely identify the pad.
>>
>> Whether a pad is connected to anything or what type it is is unrelated
>> to this. Passing the pad index of an unconnected pad to e.g. SUBDEV_S_FMT
>> will just result in an error. All I need is to be able to obtain the
>> pad 

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 12:38:29 +0100
Hans Verkuil  escreveu:

> On 02/05/2018 12:15 PM, Mauro Carvalho Chehab wrote:
> > Hi Hans,
> > 
> > Em Sun, 4 Feb 2018 14:06:42 +0100
> > Hans Verkuil  escreveu:
> >   
> >> Hi Mauro,
> >>
> >> I'm working on adding proper compliance tests for the MC but I think 
> >> something
> >> is missing in the G_TOPOLOGY ioctl w.r.t. pads.
> >>
> >> In several v4l-subdev ioctls you need to pass the pad. There the pad is an 
> >> index
> >> for the corresponding entity. I.e. an entity has 3 pads, so the pad 
> >> argument is
> >> [0-2].
> >>
> >> The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use 
> >> that
> >> in the v4l-subdev ioctls, so how do I translate that to a pad index in my 
> >> application?
> >>
> >> It seems to be a missing feature in the API. I assume this information is 
> >> available
> >> in the core, so then I would add a field to struct media_v2_pad with the 
> >> pad index
> >> for the entity.  
> > 
> > No, this was designed this way by purpose, back in 2015, when this was
> > discussed at the first MC workshop. It was a consensus on that time that
> > parameters like PAD index, PAD name, etc should be implemented via the
> > properties API, as proposed by Sakari [1]. We also had a few discussions 
> > about that on both IRC and ML.  
> 
> I'll read up on this and reply to this later.
> 
> 
> 
> >> Next time we add new public API features I want to see compliance tests 
> >> before
> >> accepting it. It's much too easy to overlook something, either in the 
> >> design or
> >> in a driver or in the documentation, so this is really, really needed 
> >> IMHO.  
> > 
> > We added a test tool for G_TOPOLOGY on that time.
> > 
> > I doubt that writing test/compliance tools in advance will solve all API
> > gaps. The thing is that new features will take some time to be used on
> > real apps. Some things are only visible when real apps start using the
> > new API features.  
> 
> This is no excuse whatsoever NOT to write compliance tests. Sure, they will
> never find everything, but for sure they find a LOT! Especially all the
> little stupid things that can make something unusable for certain use-cases.

I agree that either a compliance or a test app is important.

> And equally important, driver developers can use it to check that they didn't
> forget anything.
> 
> A media-ctl/v4l2-ctl/whatever utility is no substitute for proper compliance
> testing. The media API is complex because video is complex. It is impossible
> to review code and catch all the little mistakes, but compliance tests can
> go far in verifying driver code and also catching core code regressions.
> 
> I have yet to see a new driver that didn't fail at least one v4l2-compliance
> test, and I very much doubt that existing subdev/media drivers would do any
> different.
> 
> It would be very interesting if you would test it as well on DVB devices.
> You should be able to run v4l2-compliance on the media device. There may
> well be bugs in the tests for DVB devices. But that might also be caused
> by incomplete documentation in the spec.

I can surely do some tests on DVB devices too. Please remind me after a
couple of weeks. I'm very busy this week, and usually the first week
after a merge window is also a busy one.


Regards,
Mauro

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 12:55:21 +0100
Hans Verkuil  escreveu:

> On 02/05/2018 12:15 PM, Mauro Carvalho Chehab wrote:
> > Hi Hans,
> > 
> > Em Sun, 4 Feb 2018 14:06:42 +0100
> > Hans Verkuil  escreveu:
> >   
> >> Hi Mauro,
> >>
> >> I'm working on adding proper compliance tests for the MC but I think 
> >> something
> >> is missing in the G_TOPOLOGY ioctl w.r.t. pads.
> >>
> >> In several v4l-subdev ioctls you need to pass the pad. There the pad is an 
> >> index
> >> for the corresponding entity. I.e. an entity has 3 pads, so the pad 
> >> argument is
> >> [0-2].
> >>
> >> The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use 
> >> that
> >> in the v4l-subdev ioctls, so how do I translate that to a pad index in my 
> >> application?
> >>
> >> It seems to be a missing feature in the API. I assume this information is 
> >> available
> >> in the core, so then I would add a field to struct media_v2_pad with the 
> >> pad index
> >> for the entity.  
> > 
> > No, this was designed this way by purpose, back in 2015, when this was
> > discussed at the first MC workshop. It was a consensus on that time that
> > parameters like PAD index, PAD name, etc should be implemented via the
> > properties API, as proposed by Sakari [1]. We also had a few discussions 
> > about that on both IRC and ML.
> > 
> > [1] See: https://linuxtv.org/news.php?entry=2015-08-17.mchehab
> > 
> > 3.3 Action items
> > ...
> > media_properties: RFC userspace API: Sakari
> > 
> > Unfortunately, Sakari never found the time to send us a patch series
> > implementing a properties API, even as RFC.
> > 
> > One of the other missing features is to add a new version for setting
> > links (I guess we called it as S_LINKS_V2 at the meetings there).
> > The hole idea is to provide a way to setup the entire pipeline using
> > a single ioctl, on an atomic way.
> > 
> > The big problem with pad indexes happen on entities that have PADs with
> > different types of signal input or output, like for example a TV tuner.
> > 
> > On almost all PC consumers hardware supported by the Kernel, the same
> > PCI/USB ID may come with different types of hardware. This may also
> > happen with embedded TV sets, as, depending on the market is is
> > sold, the same product may come with a different tuner.
> > 
> > A "classic" TV tuner usually has those types of output signals:
> > 
> > - analog TV luminance IF;
> > - analog TV chrominance IF [1];
> > - digital TV OFDM IF;
> > - audio IF;
> > 
> > [1] As both luminance and chrominance should go to the same TV
> > demod, in practice, we can group both signals together on a
> > single pad.
> > 
> > More modern tuners also have an audio demod integrated, meaning that
> > they also have another PAD:
> > - digital mono or stereo audio.
> > 
> > At the simplest possible scenario, let's say that we have a TV device
> > has those entities (among others), each with a single PAD input:
> > 
> > - entity #0: a TV tuner;
> > - entity #1: an audio demod;
> > - entity #2: an analog TV demod;
> > - entity #3: a digital TV demod.
> > 
> > And the TV tuner has 4 output pads:
> > 
> > - pad #0 -> analog TV luminance/chrominance;
> > - pad #1 -> digital TV IF;
> > - pad #2 -> audio IF;
> > - pad #3 -> digital audio.
> > 
> > There, pad #0 can only be connected to entity #2. You cannot
> > connect any other pad to entity #2. The same apply to every other
> > TV tuner output PAD.
> > 
> > In this specific scenario, entity #1 can only be connected to pad #2,
> > and pad #3 can't be connected anywhere, as, on this model opted to
> > have a separate audio demod, and didn't wired the digital audio output.
> > Yet, the subdev has it.
> > 
> > Another TV set may have different pad numbers - placing them even on a
> > different order, and opting to use the audio demod tuner, wiring the
> > digital audio output.
> > 
> > Currently, there's no way for an userspace application to know what pads
> > can be linked to each entity, as there's no way to tell userspace what
> > kind of signal each pad supports.
> > 
> > In any case, the Kernel should either detect the tuner model or know
> > it (via a different DT entry), but userspace need such information,
> > in order to be able to properly set the pipelines.
> > 
> > So, what we really need here is passing an optional set of properties
> > to userspace in order to allow it to do the right thing.  
> 
> I fail to see the problem. Entities have pads. Pads have both a unique
> ID and an index within the entity pad list. I.e. the pad ID and the
> (entity ID + pad index) tuple both uniquely identify the pad.
> 
> Whether a pad is connected to anything or what type it is is unrelated
> to this. Passing the pad index of an unconnected pad to e.g. SUBDEV_S_FMT
> will just result in an error. All I need is to be able to obtain the
> pad index so I can call SUBDEV_S_FMT at all!

The 

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Mauro Carvalho Chehab]

Em Sun, 04 Feb 2018 15:20:55 +0200
Laurent Pinchart  escreveu:

> Hi Hans,
> 
> On Sunday, 4 February 2018 15:16:26 EET Hans Verkuil wrote:
> > On 02/04/2018 02:13 PM, Laurent Pinchart wrote:  
> > > On Sunday, 4 February 2018 15:06:42 EET Hans Verkuil wrote:  
> > >> Hi Mauro,
> > >> 
> > >> I'm working on adding proper compliance tests for the MC but I think
> > >> something is missing in the G_TOPOLOGY ioctl w.r.t. pads.
> > >> 
> > >> In several v4l-subdev ioctls you need to pass the pad. There the pad is
> > >> an index for the corresponding entity. I.e. an entity has 3 pads, so the
> > >> pad argument is [0-2].
> > >> 
> > >> The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use
> > >> that in the v4l-subdev ioctls, so how do I translate that to a pad index
> > >> in my application?
> > >> 
> > >> It seems to be a missing feature in the API. I assume this information is
> > >> available in the core, so then I would add a field to struct media_v2_pad
> > >> with the pad index for the entity.
> > >> 
> > >> Next time we add new public API features I want to see compliance tests
> > >> before accepting it. It's much too easy to overlook something, either in
> > >> the design or in a driver or in the documentation, so this is really,
> > >> really needed IMHO.  
> > > 
> > > I agree with you, and would even like to go one step beyond by requiring
> > > an implementation in a real use case, not just in a compliance or test
> > > tool.

We could require an open source real application and some hardware to
allow us to test new APIs before allowing adding them, but I suspect that
this will just stall the development, as, on most companies, one development
team work on writing a new Kernel feature. Once done, a separate team
starts to implement userspace tools. On embedded world, this doesn't even
envolve writing any open source apps.

> > > On the topic of the G_TOPOLOGY API, it's pretty clear it was merged too
> > > hastily. We could try to fix it, but given all the issues we haven't
> > > solved yet, I believe a new version of the API would be better.  
> > 
> > It's actually not too bad as an API speaking as an end-user. It's simple and
> > efficient. But this pad issue is a real problem.  
> 
> We have other issues such as connector support

The thing with connector is unrelated with the API that reports entities.
From API PoV, a connector is just a new entity type. 

The discussions around it were purely related about how to deal with the
case where a single physical connector carries multiple signals on it, 
but require different settings, depending on how this is physically wired.

This is a very common problem with S-Video connectors
(MEDIA_ENT_F_CONN_SVIDEO), as lots of boards are shipped with a cable to 
allow using a S-Video input for composite video.

At proprietary software that comes with those boards, it identifies a
single S-Video connector as if they were two different physical connectors
(e. g. it looks at the final connector after the cabling).

In other words S-Video input physical connectors at the board can be used
on two different scenarios:

1) using S-Video/S-Video cable, using 4 pins, 2 for chrominance, 2 for
   luminance. The end connector is a S-Video plug.

2) using a S-Video to composite video cable, using just 2 pins of the
   board input connector. The end connector is RCA composite plug.

There are even some scenarios (very common on Hauppauge devices) where
a single multiple pin proprietary connector has multiple physical
connectors at their ends for both Audio and Video. Among them, there
is a separate S-Video plug and a separate Composite RCA plug.
On such cables, the Composite connector is usually physically wired
to the S-Video Y signal, just like if it had a S-Video to composite
cable.

Depending if the physical connector is connected using a S-Video/S-Video
or a S-Video/Composite cable, the settings at the driver should be
different (not only enabling input pins but also changing some demod
parameters in order to provide enhanced quality for S-Video).

So, while physically there is just one connector at the board, logically, 
there are two different V4L2 device/subdev settings, depending on the type
of cable/signals connected to it.

Any way, such discussion is completely orthogonal to G_TOPOLOGY.
No matter what API we use, we should have a way to allow userspace
to select between "S-Video" and "composite" on devices that provide
S-Video physical connectors.

As discussed, the main alternatives are:

1) Have one pad for Y signal and one pad for C signal.

If both are linked to a S-Video connector, it is in S-Video mode.
If just Y signal is linked, it is in composite mode.

2) Have one pad for S-Video, one pad for Composite.

If composite pad is linked to a S-Video connector, it is composite;
if s-video pad is linked to a S-Video connector, it is S-Video.

We didn't reach a consensus if either (1) or (2) would be 

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Hans Verkuil]

On 02/05/2018 12:15 PM, Mauro Carvalho Chehab wrote:
> Hi Hans,
> 
> Em Sun, 4 Feb 2018 14:06:42 +0100
> Hans Verkuil  escreveu:
> 
>> Hi Mauro,
>>
>> I'm working on adding proper compliance tests for the MC but I think 
>> something
>> is missing in the G_TOPOLOGY ioctl w.r.t. pads.
>>
>> In several v4l-subdev ioctls you need to pass the pad. There the pad is an 
>> index
>> for the corresponding entity. I.e. an entity has 3 pads, so the pad argument 
>> is
>> [0-2].
>>
>> The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use 
>> that
>> in the v4l-subdev ioctls, so how do I translate that to a pad index in my 
>> application?
>>
>> It seems to be a missing feature in the API. I assume this information is 
>> available
>> in the core, so then I would add a field to struct media_v2_pad with the pad 
>> index
>> for the entity.
> 
> No, this was designed this way by purpose, back in 2015, when this was
> discussed at the first MC workshop. It was a consensus on that time that
> parameters like PAD index, PAD name, etc should be implemented via the
> properties API, as proposed by Sakari [1]. We also had a few discussions 
> about that on both IRC and ML.
> 
> [1] See: https://linuxtv.org/news.php?entry=2015-08-17.mchehab
> 
>   3.3 Action items
>   ...
>   media_properties: RFC userspace API: Sakari
> 
> Unfortunately, Sakari never found the time to send us a patch series
> implementing a properties API, even as RFC.
> 
> One of the other missing features is to add a new version for setting
> links (I guess we called it as S_LINKS_V2 at the meetings there).
> The hole idea is to provide a way to setup the entire pipeline using
> a single ioctl, on an atomic way.
> 
> The big problem with pad indexes happen on entities that have PADs with
> different types of signal input or output, like for example a TV tuner.
> 
> On almost all PC consumers hardware supported by the Kernel, the same
> PCI/USB ID may come with different types of hardware. This may also
> happen with embedded TV sets, as, depending on the market is is
> sold, the same product may come with a different tuner.
> 
> A "classic" TV tuner usually has those types of output signals:
> 
>   - analog TV luminance IF;
>   - analog TV chrominance IF [1];
>   - digital TV OFDM IF;
>   - audio IF;
> 
> [1] As both luminance and chrominance should go to the same TV
> demod, in practice, we can group both signals together on a
> single pad.
> 
> More modern tuners also have an audio demod integrated, meaning that
> they also have another PAD:
>   - digital mono or stereo audio.
> 
> At the simplest possible scenario, let's say that we have a TV device
> has those entities (among others), each with a single PAD input:
> 
>   - entity #0: a TV tuner;
>   - entity #1: an audio demod;
>   - entity #2: an analog TV demod;
>   - entity #3: a digital TV demod.
> 
> And the TV tuner has 4 output pads:
> 
>   - pad #0 -> analog TV luminance/chrominance;
>   - pad #1 -> digital TV IF;
>   - pad #2 -> audio IF;
>   - pad #3 -> digital audio.
> 
> There, pad #0 can only be connected to entity #2. You cannot
> connect any other pad to entity #2. The same apply to every other
> TV tuner output PAD.
> 
> In this specific scenario, entity #1 can only be connected to pad #2,
> and pad #3 can't be connected anywhere, as, on this model opted to
> have a separate audio demod, and didn't wired the digital audio output.
> Yet, the subdev has it.
> 
> Another TV set may have different pad numbers - placing them even on a
> different order, and opting to use the audio demod tuner, wiring the
> digital audio output.
> 
> Currently, there's no way for an userspace application to know what pads
> can be linked to each entity, as there's no way to tell userspace what
> kind of signal each pad supports.
> 
> In any case, the Kernel should either detect the tuner model or know
> it (via a different DT entry), but userspace need such information,
> in order to be able to properly set the pipelines.
> 
> So, what we really need here is passing an optional set of properties
> to userspace in order to allow it to do the right thing.

I fail to see the problem. Entities have pads. Pads have both a unique
ID and an index within the entity pad list. I.e. the pad ID and the
(entity ID + pad index) tuple both uniquely identify the pad.

Whether a pad is connected to anything or what type it is is unrelated
to this. Passing the pad index of an unconnected pad to e.g. SUBDEV_S_FMT
will just result in an error. All I need is to be able to obtain the
pad index so I can call SUBDEV_S_FMT at all!

I actually like G_TOPOLOGY, it's nice. But it just does not give all the
information needed.

> Yet, I agree with you: we should not wait anymore for a properties API,
> as it seems unlikely that we'll add support for it anytime soon.
> 
> So, let's add two fields there:
>   - pad index 

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Hans Verkuil]

On 02/05/2018 12:15 PM, Mauro Carvalho Chehab wrote:
> Hi Hans,
> 
> Em Sun, 4 Feb 2018 14:06:42 +0100
> Hans Verkuil  escreveu:
> 
>> Hi Mauro,
>>
>> I'm working on adding proper compliance tests for the MC but I think 
>> something
>> is missing in the G_TOPOLOGY ioctl w.r.t. pads.
>>
>> In several v4l-subdev ioctls you need to pass the pad. There the pad is an 
>> index
>> for the corresponding entity. I.e. an entity has 3 pads, so the pad argument 
>> is
>> [0-2].
>>
>> The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use 
>> that
>> in the v4l-subdev ioctls, so how do I translate that to a pad index in my 
>> application?
>>
>> It seems to be a missing feature in the API. I assume this information is 
>> available
>> in the core, so then I would add a field to struct media_v2_pad with the pad 
>> index
>> for the entity.
> 
> No, this was designed this way by purpose, back in 2015, when this was
> discussed at the first MC workshop. It was a consensus on that time that
> parameters like PAD index, PAD name, etc should be implemented via the
> properties API, as proposed by Sakari [1]. We also had a few discussions 
> about that on both IRC and ML.

I'll read up on this and reply to this later.



>> Next time we add new public API features I want to see compliance tests 
>> before
>> accepting it. It's much too easy to overlook something, either in the design 
>> or
>> in a driver or in the documentation, so this is really, really needed IMHO.
> 
> We added a test tool for G_TOPOLOGY on that time.
> 
> I doubt that writing test/compliance tools in advance will solve all API
> gaps. The thing is that new features will take some time to be used on
> real apps. Some things are only visible when real apps start using the
> new API features.

This is no excuse whatsoever NOT to write compliance tests. Sure, they will
never find everything, but for sure they find a LOT! Especially all the
little stupid things that can make something unusable for certain use-cases.

And equally important, driver developers can use it to check that they didn't
forget anything.

A media-ctl/v4l2-ctl/whatever utility is no substitute for proper compliance
testing. The media API is complex because video is complex. It is impossible
to review code and catch all the little mistakes, but compliance tests can
go far in verifying driver code and also catching core code regressions.

I have yet to see a new driver that didn't fail at least one v4l2-compliance
test, and I very much doubt that existing subdev/media drivers would do any
different.

It would be very interesting if you would test it as well on DVB devices.
You should be able to run v4l2-compliance on the media device. There may
well be bugs in the tests for DVB devices. But that might also be caused
by incomplete documentation in the spec.

Regards,

Hans

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Mauro Carvalho Chehab]

Hi Hans,

Em Sun, 4 Feb 2018 14:06:42 +0100
Hans Verkuil  escreveu:

> Hi Mauro,
> 
> I'm working on adding proper compliance tests for the MC but I think something
> is missing in the G_TOPOLOGY ioctl w.r.t. pads.
> 
> In several v4l-subdev ioctls you need to pass the pad. There the pad is an 
> index
> for the corresponding entity. I.e. an entity has 3 pads, so the pad argument 
> is
> [0-2].
> 
> The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use that
> in the v4l-subdev ioctls, so how do I translate that to a pad index in my 
> application?
> 
> It seems to be a missing feature in the API. I assume this information is 
> available
> in the core, so then I would add a field to struct media_v2_pad with the pad 
> index
> for the entity.

No, this was designed this way by purpose, back in 2015, when this was
discussed at the first MC workshop. It was a consensus on that time that
parameters like PAD index, PAD name, etc should be implemented via the
properties API, as proposed by Sakari [1]. We also had a few discussions 
about that on both IRC and ML.

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

3.3 Action items
...
media_properties: RFC userspace API: Sakari

Unfortunately, Sakari never found the time to send us a patch series
implementing a properties API, even as RFC.

One of the other missing features is to add a new version for setting
links (I guess we called it as S_LINKS_V2 at the meetings there).
The hole idea is to provide a way to setup the entire pipeline using
a single ioctl, on an atomic way.

The big problem with pad indexes happen on entities that have PADs with
different types of signal input or output, like for example a TV tuner.

On almost all PC consumers hardware supported by the Kernel, the same
PCI/USB ID may come with different types of hardware. This may also
happen with embedded TV sets, as, depending on the market is is
sold, the same product may come with a different tuner.

A "classic" TV tuner usually has those types of output signals:

- analog TV luminance IF;
- analog TV chrominance IF [1];
- digital TV OFDM IF;
- audio IF;

[1] As both luminance and chrominance should go to the same TV
demod, in practice, we can group both signals together on a
single pad.

More modern tuners also have an audio demod integrated, meaning that
they also have another PAD:
- digital mono or stereo audio.

At the simplest possible scenario, let's say that we have a TV device
has those entities (among others), each with a single PAD input:

- entity #0: a TV tuner;
- entity #1: an audio demod;
- entity #2: an analog TV demod;
- entity #3: a digital TV demod.

And the TV tuner has 4 output pads:

- pad #0 -> analog TV luminance/chrominance;
- pad #1 -> digital TV IF;
- pad #2 -> audio IF;
- pad #3 -> digital audio.

There, pad #0 can only be connected to entity #2. You cannot
connect any other pad to entity #2. The same apply to every other
TV tuner output PAD.

In this specific scenario, entity #1 can only be connected to pad #2,
and pad #3 can't be connected anywhere, as, on this model opted to
have a separate audio demod, and didn't wired the digital audio output.
Yet, the subdev has it.

Another TV set may have different pad numbers - placing them even on a
different order, and opting to use the audio demod tuner, wiring the
digital audio output.

Currently, there's no way for an userspace application to know what pads
can be linked to each entity, as there's no way to tell userspace what
kind of signal each pad supports.

In any case, the Kernel should either detect the tuner model or know
it (via a different DT entry), but userspace need such information,
in order to be able to properly set the pipelines.

So, what we really need here is passing an optional set of properties
to userspace in order to allow it to do the right thing.

Yet, I agree with you: we should not wait anymore for a properties API,
as it seems unlikely that we'll add support for it anytime soon.

So, let's add two fields there:
- pad index number;
- pad type.

In order to make things easy, I guess the best would be to use a string
for the pad type, and fill it only for entities where it is relevant
(like TV tuners and demods).

> Next time we add new public API features I want to see compliance tests before
> accepting it. It's much too easy to overlook something, either in the design 
> or
> in a driver or in the documentation, so this is really, really needed IMHO.

We added a test tool for G_TOPOLOGY on that time.

I doubt that writing test/compliance tools in advance will solve all API
gaps. The thing is that new features will take some time to be used on
real apps. Some things are only visible when real apps start using the
new API features.


Thanks,
Mauro

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Laurent Pinchart]

Hi Hans,

On Sunday, 4 February 2018 15:16:26 EET Hans Verkuil wrote:
> On 02/04/2018 02:13 PM, Laurent Pinchart wrote:
> > On Sunday, 4 February 2018 15:06:42 EET Hans Verkuil wrote:
> >> Hi Mauro,
> >> 
> >> I'm working on adding proper compliance tests for the MC but I think
> >> something is missing in the G_TOPOLOGY ioctl w.r.t. pads.
> >> 
> >> In several v4l-subdev ioctls you need to pass the pad. There the pad is
> >> an index for the corresponding entity. I.e. an entity has 3 pads, so the
> >> pad argument is [0-2].
> >> 
> >> The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use
> >> that in the v4l-subdev ioctls, so how do I translate that to a pad index
> >> in my application?
> >> 
> >> It seems to be a missing feature in the API. I assume this information is
> >> available in the core, so then I would add a field to struct media_v2_pad
> >> with the pad index for the entity.
> >> 
> >> Next time we add new public API features I want to see compliance tests
> >> before accepting it. It's much too easy to overlook something, either in
> >> the design or in a driver or in the documentation, so this is really,
> >> really needed IMHO.
> > 
> > I agree with you, and would even like to go one step beyond by requiring
> > an implementation in a real use case, not just in a compliance or test
> > tool.
> > 
> > On the topic of the G_TOPOLOGY API, it's pretty clear it was merged too
> > hastily. We could try to fix it, but given all the issues we haven't
> > solved yet, I believe a new version of the API would be better.
> 
> It's actually not too bad as an API speaking as an end-user. It's simple and
> efficient. But this pad issue is a real problem.

We have other issues such as connector support and entities function vs. types 
that we have never solved. The G_TOPOLOGY ioctl moves in the right direction 
but has clearly been merged too early. It might be possible to fix it, I 
haven't checked yet, but I really don't want to see this mistake being 
repeated in the future.

-- 
Regards,

Laurent Pinchart

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Hans Verkuil]

On 02/04/2018 02:13 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Sunday, 4 February 2018 15:06:42 EET Hans Verkuil wrote:
>> Hi Mauro,
>>
>> I'm working on adding proper compliance tests for the MC but I think
>> something is missing in the G_TOPOLOGY ioctl w.r.t. pads.
>>
>> In several v4l-subdev ioctls you need to pass the pad. There the pad is an
>> index for the corresponding entity. I.e. an entity has 3 pads, so the pad
>> argument is [0-2].
>>
>> The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use
>> that in the v4l-subdev ioctls, so how do I translate that to a pad index in
>> my application?
>>
>> It seems to be a missing feature in the API. I assume this information is
>> available in the core, so then I would add a field to struct media_v2_pad
>> with the pad index for the entity.
>>
>> Next time we add new public API features I want to see compliance tests
>> before accepting it. It's much too easy to overlook something, either in
>> the design or in a driver or in the documentation, so this is really,
>> really needed IMHO.
> 
> I agree with you, and would even like to go one step beyond by requiring an 
> implementation in a real use case, not just in a compliance or test tool.
> 
> On the topic of the G_TOPOLOGY API, it's pretty clear it was merged too 
> hastily. We could try to fix it, but given all the issues we haven't solved 
> yet, I believe a new version of the API would be better.
> 

It's actually not too bad as an API speaking as an end-user. It's simple and
efficient. But this pad issue is a real problem.

Regards,

Hans

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Laurent Pinchart]

Hi Hans,

On Sunday, 4 February 2018 15:06:42 EET Hans Verkuil wrote:
> Hi Mauro,
> 
> I'm working on adding proper compliance tests for the MC but I think
> something is missing in the G_TOPOLOGY ioctl w.r.t. pads.
> 
> In several v4l-subdev ioctls you need to pass the pad. There the pad is an
> index for the corresponding entity. I.e. an entity has 3 pads, so the pad
> argument is [0-2].
> 
> The G_TOPOLOGY ioctl returns a pad ID, which is > 0x0100. I can't use
> that in the v4l-subdev ioctls, so how do I translate that to a pad index in
> my application?
> 
> It seems to be a missing feature in the API. I assume this information is
> available in the core, so then I would add a field to struct media_v2_pad
> with the pad index for the entity.
> 
> Next time we add new public API features I want to see compliance tests
> before accepting it. It's much too easy to overlook something, either in
> the design or in a driver or in the documentation, so this is really,
> really needed IMHO.

I agree with you, and would even like to go one step beyond by requiring an 
implementation in a real use case, not just in a compliance or test tool.

On the topic of the G_TOPOLOGY API, it's pretty clear it was merged too 
hastily. We could try to fix it, but given all the issues we haven't solved 
yet, I believe a new version of the API would be better.

-- 
Regards,

Laurent Pinchart