MEDIA_IOC_G_TOPOLOGY and pad indices

MEDIA_IOC_G_TOPOLOGY and pad indices

[Hans Verkuil]

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

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

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 > 0x01000000. 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: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 > 0x01000000. 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

[Mauro Carvalho Chehab]

Hi Hans,

Em Sun, 4 Feb 2018 14:06:42 +0100
Hans Verkuil <hverkuil@xs4all.nl> 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 > 0x01000000. 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

[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 <hverkuil@xs4all.nl> 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 > 0x01000000. 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.

<snip>

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

[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 <hverkuil@xs4all.nl> 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 > 0x01000000. 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 number;

So should I also add a flag to signal that the pad index is set? Since
current and past kernels never set this. Obviously none of the current
applications using G_TOPOLOGY would use a pad index because there isn't
one. It would only be a problem for new applications that switch to
G_TOPOLOGY, use subdevs and assume that the kernel that is used supports
the pad index. I'm not sure if this warrants a new pad flag though.

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

struct media_v2_pad {
        __u32 id;
        __u32 entity_id;
        __u32 flags;
        __u32 reserved[5];
} __attribute__ ((packed));

This does not easily lend itself to a string. A pad type u16 would be easy
to add though. That said, adding a pad type is a completely separate issue
and needs an RFC or something to define this. If there such an RFC was posted
in the past, can you provide a link to refresh my memory?

Regards,

	Hans

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Mauro Carvalho Chehab]

Em Sun, 04 Feb 2018 15:20:55 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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 > 0x01000000. 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 the better
alternative. The main reason for not reaching a consensus is how
to map it to DT. So, it was decided to not expose connectors:

	93125094c07d ("[media] media.h: postpone connectors entities")

Once we reach an agreement, all we need is to revert the above
patch and adjust the drivers that use MEDIA_ENT_F_CONN_SVIDEO to
do the right thing. That won't affect the ioctl.

> and entities function vs. types that we have never solved.

Huh? This was solved. The legacy API was per type, while G_TOPOLOGY is 
per function.

What we don't have yet is a case where a single entity provide multiple
functions. Nobody ever submitted any patchset covering such scenario. 

What it was agreed is that, if we ever have have such kind of entities,
the other functions would be exposed via the properties API.

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

Thanks,
Mauro

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 12:55:21 +0100
Hans Verkuil <hverkuil@xs4all.nl> 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 <hverkuil@xs4all.nl> 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 > 0x01000000. 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 problem is not at S_FMT. It happens before that: How an userspace 
application will know what pad index or pad ID should be used to set the
pipeline?

I mean, how it will know that tuner #0 pad #0 should be connected to
entity #0 (an analog TV decoder) or to entity #2 (an audio decoder)?
It has to know if pad #0 contains analog TV signals or audio signals.
The API doesn't provide such information, and adding a pad index
won't solve it.

Ok, there is an obvious solution: it could hardcode pad indexes and pad IDs
per each hardware model it needs to work with (and the Kernel version, as
PAD order and even number of pads may change on future versions), but 
that defeats the purpose of having any topology API: if the application
already knows what to expect for a given hardware on a given Kernel version,
it could just setup the links without even querying about the topology.

But, if we want apps to rely on G_TOPOLOGY, it should describe the pads
in a way that apps can use the information provided there.

On devices with multiple inputs, if *all* inputs receive the same kind
of signal, just a pad index is enough. The same applies to devices
that have multiple outputs: if *all* outputs have the same type, a
pad index is enough to allow setting the pipelines via
MEDIA_IOC_SETUP_LINK (or a new PAD ID-based atomic setup links ioctl,
as discussed in the past).

However, on the general case, apps can only call MEDIA_IOC_SETUP_LINK
(or an equivalent pad-id based ioctl) when they know what signal types
each pad contains.

On other words, a pad index is one important information for devices 
with multiple pad inputs or outputs, but it works fine only if all 
inputs receive the same type of signals, and all pad outputs produce
the same type of signals.

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

Agreed.

> > 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;  
> 
> So should I also add a flag to signal that the pad index is set? 

A flag won't solve. If userspace doesn't know if it is running on
a new Kernel with a G_TOPOLOGY with new flags, it won't expect a
flags there.

IMO, the best thing to do here is to increment the API version reported
via MEDIA_IOC_DEVICE_INFO.

> Since
> current and past kernels never set this. Obviously none of the current
> applications using G_TOPOLOGY would use a pad index because there isn't
> one. It would only be a problem for new applications that switch to
> G_TOPOLOGY, use subdevs and assume that the kernel that is used supports
> the pad index. I'm not sure if this warrants a new pad flag though.

Perhaps it is time to write an ioctl that would allow setting the
pipelines via pad ID and allow setting multiple links at once on
an atomic way.

> 
> > 	- 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).  
> 
> struct media_v2_pad {
>         __u32 id;
>         __u32 entity_id;
>         __u32 flags;
>         __u32 reserved[5];
> } __attribute__ ((packed));
> 
> This does not easily lend itself to a string.

Yes. The original idea were to add it via properties API as an
string. That's why we didn't add extra space there to store strings.

We could get 32 bits or 64 bits there to be used as an index pointer to a
strings type, stored at the end of G_TOPOLOGY data output, but not sure 
if I like this idea.

> A pad type u16 would be easy to add though.

Due to alignment issues, I would prefer to make it u32, although
u16 is probably more than enough.

> That said, adding a pad type is a completely separate issue
> and needs an RFC or something to define this. If there such an RFC was posted
> in the past, can you provide a link to refresh my memory?

We had some discussions in the past, but we didn't come with a RFC.

For our current needs, I'd say we just need a handful set of types:

#define MEDIA_PAD_TYPE_DEFAULT		0
#define MEDIA_PAD_TYPE_ANALOG_IF	1
#define MEDIA_PAD_TYPE_AUDIO_IF		2
#define MEDIA_PAD_TYPE_DIGITAL_TV_IF	3
#define MEDIA_PAD_TYPE_DIGITAL_AUDIO	4

Values different than zero would be used only when not all inputs
or not all outputs would have the same type. Whey they're all the
same, it would use MEDIA_PAD_TYPE_DEFAULT.

I would use just a single PAD and a single type (MEDIA_PAD_TYPE_ANALOG_IF)
for the cases where a tuner provides Y and C signals, as they're always
grouped altogether when a tuner is connected to an analog TV demod.

It shouldn't be hard to add support for it at the existing subdevs.

Thanks,
Mauro

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 12:38:29 +0100
Hans Verkuil <hverkuil@xs4all.nl> 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 <hverkuil@xs4all.nl> 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 > 0x01000000. 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.
> 
> <snip>
> 
> >> 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

[Hans Verkuil]

On 02/05/2018 02:17 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 5 Feb 2018 12:55:21 +0100
> Hans Verkuil <hverkuil@xs4all.nl> 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 <hverkuil@xs4all.nl> 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 > 0x01000000. 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 problem is not at S_FMT. It happens before that: How an userspace 
> application will know what pad index or pad ID should be used to set the
> pipeline?
> 
> I mean, how it will know that tuner #0 pad #0 should be connected to
> entity #0 (an analog TV decoder) or to entity #2 (an audio decoder)?
> It has to know if pad #0 contains analog TV signals or audio signals.
> The API doesn't provide such information, and adding a pad index
> won't solve it.
> 
> Ok, there is an obvious solution: it could hardcode pad indexes and pad IDs
> per each hardware model it needs to work with (and the Kernel version, as
> PAD order and even number of pads may change on future versions), but 
> that defeats the purpose of having any topology API: if the application
> already knows what to expect for a given hardware on a given Kernel version,
> it could just setup the links without even querying about the topology.
> 
> But, if we want apps to rely on G_TOPOLOGY, it should describe the pads
> in a way that apps can use the information provided there.
> 
> On devices with multiple inputs, if *all* inputs receive the same kind
> of signal, just a pad index is enough. The same applies to devices
> that have multiple outputs: if *all* outputs have the same type, a
> pad index is enough to allow setting the pipelines via
> MEDIA_IOC_SETUP_LINK (or a new PAD ID-based atomic setup links ioctl,
> as discussed in the past).

And that is the case with the current set of drivers that use the MC
and subdevs. They all have the same pad type (i.e. a video stream).

> 
> However, on the general case, apps can only call MEDIA_IOC_SETUP_LINK
> (or an equivalent pad-id based ioctl) when they know what signal types
> each pad contains.
> 
> On other words, a pad index is one important information for devices 
> with multiple pad inputs or outputs, but it works fine only if all 
> inputs receive the same type of signals, and all pad outputs produce
> the same type of signals.
> 
>>
>> I actually like G_TOPOLOGY, it's nice. But it just does not give all the
>> information needed.
> 
> Agreed.
> 
>>> 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;  
>>
>> So should I also add a flag to signal that the pad index is set? 
> 
> A flag won't solve. If userspace doesn't know if it is running on
> a new Kernel with a G_TOPOLOGY with new flags, it won't expect a
> flags there.

???

Currently there is no pad index returned, so there are no applications
that try to use it :-)

Anyway, the idea of using media_version has merit. So instead of a flag
we can do something like this:

#define MEDIA_PAD_HAS_INDEX(media_version) ((media_version) >= KERNEL_VERSION(a, b, c))

where a,b,c is the kernel version that added the index field.

And the documentation will say that you need to check with this macro if the
index information is available.

It has the nice advantage of being able to bail out early on. And it works
with media_build.

> 
>> Since
>> current and past kernels never set this. Obviously none of the current
>> applications using G_TOPOLOGY would use a pad index because there isn't
>> one. It would only be a problem for new applications that switch to
>> G_TOPOLOGY, use subdevs and assume that the kernel that is used supports
>> the pad index. I'm not sure if this warrants a new pad flag though.
> 
> Perhaps it is time to write an ioctl that would allow setting the
> pipelines via pad ID and allow setting multiple links at once on
> an atomic way.

One thing at a time. Let's make the current API consistent first before
adding new stuff.

> 
>>
>>> 	- 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).  
>>
>> struct media_v2_pad {
>>         __u32 id;
>>         __u32 entity_id;
>>         __u32 flags;
>>         __u32 reserved[5];
>> } __attribute__ ((packed));
>>
>> This does not easily lend itself to a string.
> 
> Yes. The original idea were to add it via properties API as an
> string. That's why we didn't add extra space there to store strings.
> 
> We could get 32 bits or 64 bits there to be used as an index pointer to a
> strings type, stored at the end of G_TOPOLOGY data output, but not sure 
> if I like this idea.
> 
>> A pad type u16 would be easy to add though.
> 
> Due to alignment issues, I would prefer to make it u32, although
> u16 is probably more than enough.

Well, you'd get this:

	__u16 index;
	__u16 type;

That's why I used u16.

> 
>> That said, adding a pad type is a completely separate issue
>> and needs an RFC or something to define this. If there such an RFC was posted
>> in the past, can you provide a link to refresh my memory?
> 
> We had some discussions in the past, but we didn't come with a RFC.
> 
> For our current needs, I'd say we just need a handful set of types:
> 
> #define MEDIA_PAD_TYPE_DEFAULT		0
> #define MEDIA_PAD_TYPE_ANALOG_IF	1
> #define MEDIA_PAD_TYPE_AUDIO_IF		2
> #define MEDIA_PAD_TYPE_DIGITAL_TV_IF	3
> #define MEDIA_PAD_TYPE_DIGITAL_AUDIO	4
> 
> Values different than zero would be used only when not all inputs
> or not all outputs would have the same type. Whey they're all the
> same, it would use MEDIA_PAD_TYPE_DEFAULT.

A quick question: how is this done today in the absence of pad types? Hard-coded?
Or are we not using this at all?

I would also be inclined to use PAD_TYPE_MEDIA_BUS instead of DEFAULT.
All pads today used in V4L2 stream media bus data according to
include/uapi/linux/media-bus-format.h. This also nicely indicates that
the VIDIOC_SUBDEV_ENUM_MBUS_CODE ioctl must be present for that pad.

I also would start counting at 1, not 0. It makes it easier to detect
drivers that do not implement this correctly (well, it will probably be
set by the core as the default pad type, but still).

> I would use just a single PAD and a single type (MEDIA_PAD_TYPE_ANALOG_IF)
> for the cases where a tuner provides Y and C signals, as they're always
> grouped altogether when a tuner is connected to an analog TV demod.
> 
> It shouldn't be hard to add support for it at the existing subdevs.

Right.

	Hans

Re: MEDIA_IOC_G_TOPOLOGY and pad indices

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 14:47:51 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 02/05/2018 02:17 PM, Mauro Carvalho Chehab wrote:
> > Em Mon, 5 Feb 2018 12:55:21 +0100
> > Hans Verkuil <hverkuil@xs4all.nl> 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 <hverkuil@xs4all.nl> 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 > 0x01000000. 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 problem is not at S_FMT. It happens before that: How an userspace 
> > application will know what pad index or pad ID should be used to set the
> > pipeline?
> > 
> > I mean, how it will know that tuner #0 pad #0 should be connected to
> > entity #0 (an analog TV decoder) or to entity #2 (an audio decoder)?
> > It has to know if pad #0 contains analog TV signals or audio signals.
> > The API doesn't provide such information, and adding a pad index
> > won't solve it.
> > 
> > Ok, there is an obvious solution: it could hardcode pad indexes and pad IDs
> > per each hardware model it needs to work with (and the Kernel version, as
> > PAD order and even number of pads may change on future versions), but 
> > that defeats the purpose of having any topology API: if the application
> > already knows what to expect for a given hardware on a given Kernel version,
> > it could just setup the links without even querying about the topology.
> > 
> > But, if we want apps to rely on G_TOPOLOGY, it should describe the pads
> > in a way that apps can use the information provided there.
> > 
> > On devices with multiple inputs, if *all* inputs receive the same kind
> > of signal, just a pad index is enough. The same applies to devices
> > that have multiple outputs: if *all* outputs have the same type, a
> > pad index is enough to allow setting the pipelines via
> > MEDIA_IOC_SETUP_LINK (or a new PAD ID-based atomic setup links ioctl,
> > as discussed in the past).  
> 
> And that is the case with the current set of drivers that use the MC
> and subdevs. They all have the same pad type (i.e. a video stream).

On upstream, yes.

> > However, on the general case, apps can only call MEDIA_IOC_SETUP_LINK
> > (or an equivalent pad-id based ioctl) when they know what signal types
> > each pad contains.
> > 
> > On other words, a pad index is one important information for devices 
> > with multiple pad inputs or outputs, but it works fine only if all 
> > inputs receive the same type of signals, and all pad outputs produce
> > the same type of signals.
> >   
> >>
> >> I actually like G_TOPOLOGY, it's nice. But it just does not give all the
> >> information needed.  
> > 
> > Agreed.
> >   
> >>> 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;    
> >>
> >> So should I also add a flag to signal that the pad index is set?   
> > 
> > A flag won't solve. If userspace doesn't know if it is running on
> > a new Kernel with a G_TOPOLOGY with new flags, it won't expect a
> > flags there.  
> 
> ???
> 
> Currently there is no pad index returned, so there are no applications
> that try to use it :-)

Yes, but that's what your RFC patch is adding :-)

> Anyway, the idea of using media_version has merit. So instead of a flag
> we can do something like this:
> 
> #define MEDIA_PAD_HAS_INDEX(media_version) ((media_version) >= KERNEL_VERSION(a, b, c))
> 
> where a,b,c is the kernel version that added the index field.
> 
> And the documentation will say that you need to check with this macro if the
> index information is available.
> 
> It has the nice advantage of being able to bail out early on. And it works
> with media_build.

That works. Ack for that.

> >   
> >> Since
> >> current and past kernels never set this. Obviously none of the current
> >> applications using G_TOPOLOGY would use a pad index because there isn't
> >> one. It would only be a problem for new applications that switch to
> >> G_TOPOLOGY, use subdevs and assume that the kernel that is used supports
> >> the pad index. I'm not sure if this warrants a new pad flag though.  
> > 
> > Perhaps it is time to write an ioctl that would allow setting the
> > pipelines via pad ID and allow setting multiple links at once on
> > an atomic way.  
> 
> One thing at a time. Let's make the current API consistent first before
> adding new stuff.

Ok.

> 
> >   
> >>  
> >>> 	- 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).    
> >>
> >> struct media_v2_pad {
> >>         __u32 id;
> >>         __u32 entity_id;
> >>         __u32 flags;
> >>         __u32 reserved[5];
> >> } __attribute__ ((packed));
> >>
> >> This does not easily lend itself to a string.  
> > 
> > Yes. The original idea were to add it via properties API as an
> > string. That's why we didn't add extra space there to store strings.
> > 
> > We could get 32 bits or 64 bits there to be used as an index pointer to a
> > strings type, stored at the end of G_TOPOLOGY data output, but not sure 
> > if I like this idea.
> >   
> >> A pad type u16 would be easy to add though.  
> > 
> > Due to alignment issues, I would prefer to make it u32, although
> > u16 is probably more than enough.  
> 
> Well, you'd get this:
> 
> 	__u16 index;
> 	__u16 type;
> 
> That's why I used u16.

Works for me.

> 
> >   
> >> That said, adding a pad type is a completely separate issue
> >> and needs an RFC or something to define this. If there such an RFC was posted
> >> in the past, can you provide a link to refresh my memory?  
> > 
> > We had some discussions in the past, but we didn't come with a RFC.
> > 
> > For our current needs, I'd say we just need a handful set of types:
> > 
> > #define MEDIA_PAD_TYPE_DEFAULT		0
> > #define MEDIA_PAD_TYPE_ANALOG_IF	1
> > #define MEDIA_PAD_TYPE_AUDIO_IF		2
> > #define MEDIA_PAD_TYPE_DIGITAL_TV_IF	3
> > #define MEDIA_PAD_TYPE_DIGITAL_AUDIO	4
> > 
> > Values different than zero would be used only when not all inputs
> > or not all outputs would have the same type. Whey they're all the
> > same, it would use MEDIA_PAD_TYPE_DEFAULT.  
> 
> A quick question: how is this done today in the absence of pad types? Hard-coded?
> Or are we not using this at all?

Something like the above is actually required currently, even inside
the Kernel. What we did so far was a very ugly hack at
include/media/v4l2-mc.h. For devices where different types were supported,
we added enums, like:

	enum tuner_pad_index {
		TUNER_PAD_RF_INPUT,
		TUNER_PAD_OUTPUT,
		TUNER_PAD_AUD_OUT,
		TUNER_NUM_PADS
	};

and doing some ugly hacks to ensure that pad[0] will always be
used for video RF output.

Once we have a per-pad type, we can get rid of all of them for good.

> I would also be inclined to use PAD_TYPE_MEDIA_BUS instead of DEFAULT.
> All pads today used in V4L2 stream media bus data according to
> include/uapi/linux/media-bus-format.h. This also nicely indicates that
> the VIDIOC_SUBDEV_ENUM_MBUS_CODE ioctl must be present for that pad.

PAD_TYPE_MEDIA_BUS sounds very nice to me.

> I also would start counting at 1, not 0. It makes it easier to detect
> drivers that do not implement this correctly (well, it will probably be
> set by the core as the default pad type, but still).

Works for me.

> > I would use just a single PAD and a single type (MEDIA_PAD_TYPE_ANALOG_IF)
> > for the cases where a tuner provides Y and C signals, as they're always
> > grouped altogether when a tuner is connected to an analog TV demod.
> > 
> > It shouldn't be hard to add support for it at the existing subdevs.  
> 
> Right.
> 
> 	Hans



Thanks,
Mauro