media_device.c question: can this workaround be removed?

media_device.c question: can this workaround be removed?

[Hans Verkuil]

The function media_device_enum_entities() has this workaround:

        /*
         * Workaround for a bug at media-ctl <= v1.10 that makes it to
         * do the wrong thing if the entity function doesn't belong to
         * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
         * Ranges.
         *
         * Non-subdevices are expected to be at the MEDIA_ENT_F_OLD_BASE,
         * or, otherwise, will be silently ignored by media-ctl when
         * printing the graphviz diagram. So, map them into the devnode
         * old range.
         */
        if (ent->function < MEDIA_ENT_F_OLD_BASE ||
            ent->function > MEDIA_ENT_F_TUNER) {
                if (is_media_entity_v4l2_subdev(ent))
                        entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
                else if (ent->function != MEDIA_ENT_F_IO_V4L)
                        entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
        }

But this means that the entity type returned by ENUM_ENTITIES just overwrites
perfectly fine types by bogus values and thus the returned value differs
from that returned by G_TOPOLOGY.

Can we please, please remove this workaround? I have no idea why a workaround
for media-ctl of all things ever made it in the kernel.

I'm adding media support to the vivid driver and because of this media-ctl -p
gives me this:

Device topology
- entity 1: vivid-000-vid-cap (1 pad, 0 link)
            type Node subtype V4L flags 0
            device node name /dev/video0
        pad0: Source

- entity 5: vivid-000-vid-out (1 pad, 0 link)
            type Node subtype V4L flags 0
            device node name /dev/video1
        pad0: Sink

- entity 9: vivid-000-vbi-cap (1 pad, 0 link)
            type Unknown subtype Unknown flags 0
        pad0: Source

- entity 13: vivid-000-vbi-out (1 pad, 0 link)
             type Unknown subtype Unknown flags 0
        pad0: Sink

- entity 17: vivid-000-sdr-cap (1 pad, 0 link)
             type Unknown subtype Unknown flags 0
        pad0: Source

So VBI and SDR report the 'Unknown' type whereas 'v4l2-ctl -D -d /dev/vbi0' (which
uses G_TOPOLOGY) gives me:

Interface Info:
        ID               : 0x0300000b
        Type             : V4L VBI
Entity Info:
        ID               : 0x00000009 (9)
        Name             : vivid-000-vbi-cap
        Function         : VBI I/O
        Pad 0x0100000a   : Source

That's how it should be.

<rant mode on>

Never again should we allow new userspace APIs without:

1) Proper compliance tests
2) Adding support for the new API to v4l2-ctl (or related v4l-utils apps)
3) If the new API replaces old defines with new defines (e.g.
   #define MEDIA_ENT_T_DEVNODE_V4L MEDIA_ENT_F_IO_V4L) then everything
   in v4l-utils that uses the old define should be updated to the new
   define.
4) If reasonable add support for the new API to at least one of the
   virtual drivers (vivid, vimc, vim2m) so this API can be tested without
   needing specialized hardware.

The MC API did none of this and how on earth are end-users able to work with
this if we have horribly inconsistent behavior like this?

BTW, uapi/linux/media.h is an utter mess. I'll see if I can make a patch to
make it more understandable. Right now it is extremely hard to tell which
define is legacy and which isn't.

<rant mode off>

Regards,

	Hans

Re: media_device.c question: can this workaround be removed?

[Sakari Ailus]

Hi Hans,

On Mon, Feb 05, 2018 at 11:26:47AM +0100, Hans Verkuil wrote:
> The function media_device_enum_entities() has this workaround:
> 
>         /*
>          * Workaround for a bug at media-ctl <= v1.10 that makes it to
>          * do the wrong thing if the entity function doesn't belong to
>          * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
>          * Ranges.
>          *
>          * Non-subdevices are expected to be at the MEDIA_ENT_F_OLD_BASE,
>          * or, otherwise, will be silently ignored by media-ctl when
>          * printing the graphviz diagram. So, map them into the devnode
>          * old range.
>          */
>         if (ent->function < MEDIA_ENT_F_OLD_BASE ||
>             ent->function > MEDIA_ENT_F_TUNER) {
>                 if (is_media_entity_v4l2_subdev(ent))
>                         entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
>                 else if (ent->function != MEDIA_ENT_F_IO_V4L)
>                         entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
>         }
> 
> But this means that the entity type returned by ENUM_ENTITIES just overwrites
> perfectly fine types by bogus values and thus the returned value differs
> from that returned by G_TOPOLOGY.
> 
> Can we please, please remove this workaround? I have no idea why a workaround
> for media-ctl of all things ever made it in the kernel.

The entity types were replaced by entity functions back in 2015 with the
big Media controller reshuffle. While I agree functions are better for
describing entities than types (and those types had Linux specific
interfaces in them), the new function-based API still may support a single
value, i.e. a single function per device.

This also created two different name spaces for describing entities: the
old types used by the MC API and the new functions used by MC v2 API.

This doesn't go well with the fact that, as you noticed, the pad
information is not available through MC v2 API. The pad information is
required by the user space so software continues to use the original MC
API.

I don't think there's a way to avoid maintaining two name spaces (types and
functions) without breaking at least one of the APIs.

> 
> I'm adding media support to the vivid driver and because of this media-ctl -p
> gives me this:
> 
> Device topology
> - entity 1: vivid-000-vid-cap (1 pad, 0 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video0
>         pad0: Source
> 
> - entity 5: vivid-000-vid-out (1 pad, 0 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video1
>         pad0: Sink
> 
> - entity 9: vivid-000-vbi-cap (1 pad, 0 link)
>             type Unknown subtype Unknown flags 0
>         pad0: Source
> 
> - entity 13: vivid-000-vbi-out (1 pad, 0 link)
>              type Unknown subtype Unknown flags 0
>         pad0: Sink
> 
> - entity 17: vivid-000-sdr-cap (1 pad, 0 link)
>              type Unknown subtype Unknown flags 0
>         pad0: Source

It may be that there's no corresponding type for certain functions.

> 
> So VBI and SDR report the 'Unknown' type whereas 'v4l2-ctl -D -d /dev/vbi0' (which
> uses G_TOPOLOGY) gives me:
> 
> Interface Info:
>         ID               : 0x0300000b
>         Type             : V4L VBI
> Entity Info:
>         ID               : 0x00000009 (9)
>         Name             : vivid-000-vbi-cap
>         Function         : VBI I/O
>         Pad 0x0100000a   : Source
> 
> That's how it should be.
> 
> <rant mode on>
> 
> Never again should we allow new userspace APIs without:
> 
> 1) Proper compliance tests
> 2) Adding support for the new API to v4l2-ctl (or related v4l-utils apps)
> 3) If the new API replaces old defines with new defines (e.g.
>    #define MEDIA_ENT_T_DEVNODE_V4L MEDIA_ENT_F_IO_V4L) then everything
>    in v4l-utils that uses the old define should be updated to the new
>    define.
> 4) If reasonable add support for the new API to at least one of the
>    virtual drivers (vivid, vimc, vim2m) so this API can be tested without
>    needing specialized hardware.
> 
> The MC API did none of this and how on earth are end-users able to work with
> this if we have horribly inconsistent behavior like this?
> 
> BTW, uapi/linux/media.h is an utter mess. I'll see if I can make a patch to
> make it more understandable. Right now it is extremely hard to tell which
> define is legacy and which isn't.
> 
> <rant mode off>
> 
> Regards,
> 
> 	Hans

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: media_device.c question: can this workaround be removed?

[Hans Verkuil]

On 02/05/2018 12:59 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Feb 05, 2018 at 11:26:47AM +0100, Hans Verkuil wrote:
>> The function media_device_enum_entities() has this workaround:
>>
>>         /*
>>          * Workaround for a bug at media-ctl <= v1.10 that makes it to
>>          * do the wrong thing if the entity function doesn't belong to
>>          * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
>>          * Ranges.
>>          *
>>          * Non-subdevices are expected to be at the MEDIA_ENT_F_OLD_BASE,
>>          * or, otherwise, will be silently ignored by media-ctl when
>>          * printing the graphviz diagram. So, map them into the devnode
>>          * old range.
>>          */
>>         if (ent->function < MEDIA_ENT_F_OLD_BASE ||
>>             ent->function > MEDIA_ENT_F_TUNER) {
>>                 if (is_media_entity_v4l2_subdev(ent))
>>                         entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
>>                 else if (ent->function != MEDIA_ENT_F_IO_V4L)
>>                         entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
>>         }
>>
>> But this means that the entity type returned by ENUM_ENTITIES just overwrites
>> perfectly fine types by bogus values and thus the returned value differs
>> from that returned by G_TOPOLOGY.
>>
>> Can we please, please remove this workaround? I have no idea why a workaround
>> for media-ctl of all things ever made it in the kernel.
> 
> The entity types were replaced by entity functions back in 2015 with the
> big Media controller reshuffle. While I agree functions are better for
> describing entities than types (and those types had Linux specific
> interfaces in them), the new function-based API still may support a single
> value, i.e. a single function per device.
> 
> This also created two different name spaces for describing entities: the
> old types used by the MC API and the new functions used by MC v2 API.
> 
> This doesn't go well with the fact that, as you noticed, the pad
> information is not available through MC v2 API. The pad information is
> required by the user space so software continues to use the original MC
> API.
> 
> I don't think there's a way to avoid maintaining two name spaces (types and
> functions) without breaking at least one of the APIs.

The comment specifically claims that this workaround is for media-ctl and
it suggests that it is fixed after v1.10. Is that comment bogus? I can't
really tell which commit fixed media-ctl. Does anyone know?

As far as I can tell the function defines have been chosen in such a way that
they will equally well work with the old name space. There should be no
problem there whatsoever and media-ctl should switch to use the new defines.

We now have a broken ENUM_ENTITIES ioctl (it rudely overwrites VBI/DVB/etc types)
and a broken G_TOPOLOGY ioctl (no pad index).

This sucks. Let's fix both so that they at least report consistent information.

> 
>>
>> I'm adding media support to the vivid driver and because of this media-ctl -p
>> gives me this:
>>
>> Device topology
>> - entity 1: vivid-000-vid-cap (1 pad, 0 link)
>>             type Node subtype V4L flags 0
>>             device node name /dev/video0
>>         pad0: Source
>>
>> - entity 5: vivid-000-vid-out (1 pad, 0 link)
>>             type Node subtype V4L flags 0
>>             device node name /dev/video1
>>         pad0: Sink
>>
>> - entity 9: vivid-000-vbi-cap (1 pad, 0 link)
>>             type Unknown subtype Unknown flags 0
>>         pad0: Source
>>
>> - entity 13: vivid-000-vbi-out (1 pad, 0 link)
>>              type Unknown subtype Unknown flags 0
>>         pad0: Sink
>>
>> - entity 17: vivid-000-sdr-cap (1 pad, 0 link)
>>              type Unknown subtype Unknown flags 0
>>         pad0: Source
> 
> It may be that there's no corresponding type for certain functions.

'type' can be interpreted as 'function'. All possible legacy type/subtype
combinations map to a unique function. It's how the spec defines this as well.
But it is subverted by this awful workaround.

Regards,

	Hans

Re: media_device.c question: can this workaround be removed?

[Sakari Ailus]

Hi Hans,

On Mon, Feb 05, 2018 at 01:30:04PM +0100, Hans Verkuil wrote:
> On 02/05/2018 12:59 PM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Mon, Feb 05, 2018 at 11:26:47AM +0100, Hans Verkuil wrote:
> >> The function media_device_enum_entities() has this workaround:
> >>
> >>         /*
> >>          * Workaround for a bug at media-ctl <= v1.10 that makes it to
> >>          * do the wrong thing if the entity function doesn't belong to
> >>          * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
> >>          * Ranges.
> >>          *
> >>          * Non-subdevices are expected to be at the MEDIA_ENT_F_OLD_BASE,
> >>          * or, otherwise, will be silently ignored by media-ctl when
> >>          * printing the graphviz diagram. So, map them into the devnode
> >>          * old range.
> >>          */
> >>         if (ent->function < MEDIA_ENT_F_OLD_BASE ||
> >>             ent->function > MEDIA_ENT_F_TUNER) {
> >>                 if (is_media_entity_v4l2_subdev(ent))
> >>                         entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
> >>                 else if (ent->function != MEDIA_ENT_F_IO_V4L)
> >>                         entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
> >>         }
> >>
> >> But this means that the entity type returned by ENUM_ENTITIES just overwrites
> >> perfectly fine types by bogus values and thus the returned value differs
> >> from that returned by G_TOPOLOGY.
> >>
> >> Can we please, please remove this workaround? I have no idea why a workaround
> >> for media-ctl of all things ever made it in the kernel.
> > 
> > The entity types were replaced by entity functions back in 2015 with the
> > big Media controller reshuffle. While I agree functions are better for
> > describing entities than types (and those types had Linux specific
> > interfaces in them), the new function-based API still may support a single
> > value, i.e. a single function per device.
> > 
> > This also created two different name spaces for describing entities: the
> > old types used by the MC API and the new functions used by MC v2 API.
> > 
> > This doesn't go well with the fact that, as you noticed, the pad
> > information is not available through MC v2 API. The pad information is
> > required by the user space so software continues to use the original MC
> > API.
> > 
> > I don't think there's a way to avoid maintaining two name spaces (types and
> > functions) without breaking at least one of the APIs.
> 
> The comment specifically claims that this workaround is for media-ctl and
> it suggests that it is fixed after v1.10. Is that comment bogus? I can't
> really tell which commit fixed media-ctl. Does anyone know?
> 
> As far as I can tell the function defines have been chosen in such a way that
> they will equally well work with the old name space. There should be no
> problem there whatsoever and media-ctl should switch to use the new defines.

The old interface (type) was centered around the uAPI for the entity.
That's no longer the case for functions. The entity type
(MEDIA_ENT_TYPE_MASK) tells the uAPI which affects the interpretation of
the dev union in struct media_entity_struct as well as the interface that
device node implements. With the new function field that's no longer the
case.

Also, the new MC v2 API makes a separation between the entity function and
the uAPI (interface) which was lacking in the old API.

> 
> We now have a broken ENUM_ENTITIES ioctl (it rudely overwrites VBI/DVB/etc types)
> and a broken G_TOPOLOGY ioctl (no pad index).
> 
> This sucks. Let's fix both so that they at least report consistent information.

The existing user space may assume that the type field of the entity
conveys that the entity does provide a V4L2 sub-device interface if that's
the case actually.

This is what media-ctl does and I presume if existing user space checks for
the type field, it may well have similar checks: it was how the API was
defined. Therefore it's not entirely accurate to say that only media-ctl
has this "bug", I'd generally assume programs that use MC (v1) API do this.

You could argue about the merits (or lack of them) of the old API, no
disagrement there.

> 
> > 
> >>
> >> I'm adding media support to the vivid driver and because of this media-ctl -p
> >> gives me this:
> >>
> >> Device topology
> >> - entity 1: vivid-000-vid-cap (1 pad, 0 link)
> >>             type Node subtype V4L flags 0
> >>             device node name /dev/video0
> >>         pad0: Source
> >>
> >> - entity 5: vivid-000-vid-out (1 pad, 0 link)
> >>             type Node subtype V4L flags 0
> >>             device node name /dev/video1
> >>         pad0: Sink
> >>
> >> - entity 9: vivid-000-vbi-cap (1 pad, 0 link)
> >>             type Unknown subtype Unknown flags 0
> >>         pad0: Source
> >>
> >> - entity 13: vivid-000-vbi-out (1 pad, 0 link)
> >>              type Unknown subtype Unknown flags 0
> >>         pad0: Sink
> >>
> >> - entity 17: vivid-000-sdr-cap (1 pad, 0 link)
> >>              type Unknown subtype Unknown flags 0
> >>         pad0: Source
> > 
> > It may be that there's no corresponding type for certain functions.
> 
> 'type' can be interpreted as 'function'. All possible legacy type/subtype
> combinations map to a unique function. It's how the spec defines this as well.
> But it is subverted by this awful workaround.
> 
> Regards,
> 
> 	Hans

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: media_device.c question: can this workaround be removed?

[Hans Verkuil]

On 02/05/2018 03:30 PM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Feb 05, 2018 at 01:30:04PM +0100, Hans Verkuil wrote:
>> On 02/05/2018 12:59 PM, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Mon, Feb 05, 2018 at 11:26:47AM +0100, Hans Verkuil wrote:
>>>> The function media_device_enum_entities() has this workaround:
>>>>
>>>>         /*
>>>>          * Workaround for a bug at media-ctl <= v1.10 that makes it to
>>>>          * do the wrong thing if the entity function doesn't belong to
>>>>          * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
>>>>          * Ranges.
>>>>          *
>>>>          * Non-subdevices are expected to be at the MEDIA_ENT_F_OLD_BASE,
>>>>          * or, otherwise, will be silently ignored by media-ctl when
>>>>          * printing the graphviz diagram. So, map them into the devnode
>>>>          * old range.
>>>>          */
>>>>         if (ent->function < MEDIA_ENT_F_OLD_BASE ||
>>>>             ent->function > MEDIA_ENT_F_TUNER) {
>>>>                 if (is_media_entity_v4l2_subdev(ent))
>>>>                         entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
>>>>                 else if (ent->function != MEDIA_ENT_F_IO_V4L)
>>>>                         entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
>>>>         }
>>>>
>>>> But this means that the entity type returned by ENUM_ENTITIES just overwrites
>>>> perfectly fine types by bogus values and thus the returned value differs
>>>> from that returned by G_TOPOLOGY.
>>>>
>>>> Can we please, please remove this workaround? I have no idea why a workaround
>>>> for media-ctl of all things ever made it in the kernel.
>>>
>>> The entity types were replaced by entity functions back in 2015 with the
>>> big Media controller reshuffle. While I agree functions are better for
>>> describing entities than types (and those types had Linux specific
>>> interfaces in them), the new function-based API still may support a single
>>> value, i.e. a single function per device.
>>>
>>> This also created two different name spaces for describing entities: the
>>> old types used by the MC API and the new functions used by MC v2 API.
>>>
>>> This doesn't go well with the fact that, as you noticed, the pad
>>> information is not available through MC v2 API. The pad information is
>>> required by the user space so software continues to use the original MC
>>> API.
>>>
>>> I don't think there's a way to avoid maintaining two name spaces (types and
>>> functions) without breaking at least one of the APIs.
>>
>> The comment specifically claims that this workaround is for media-ctl and
>> it suggests that it is fixed after v1.10. Is that comment bogus? I can't
>> really tell which commit fixed media-ctl. Does anyone know?
>>
>> As far as I can tell the function defines have been chosen in such a way that
>> they will equally well work with the old name space. There should be no
>> problem there whatsoever and media-ctl should switch to use the new defines.
> 
> The old interface (type) was centered around the uAPI for the entity.
> That's no longer the case for functions. The entity type
> (MEDIA_ENT_TYPE_MASK) tells the uAPI which affects the interpretation of
> the dev union in struct media_entity_struct as well as the interface that
> device node implements. With the new function field that's no longer the
> case.
> 
> Also, the new MC v2 API makes a separation between the entity function and
> the uAPI (interface) which was lacking in the old API.
> 
>>
>> We now have a broken ENUM_ENTITIES ioctl (it rudely overwrites VBI/DVB/etc types)
>> and a broken G_TOPOLOGY ioctl (no pad index).
>>
>> This sucks. Let's fix both so that they at least report consistent information.
> 
> The existing user space may assume that the type field of the entity
> conveys that the entity does provide a V4L2 sub-device interface if that's
> the case actually.
> 
> This is what media-ctl does and I presume if existing user space checks for
> the type field, it may well have similar checks: it was how the API was
> defined. Therefore it's not entirely accurate to say that only media-ctl
> has this "bug", I'd generally assume programs that use MC (v1) API do this.
> 
> You could argue about the merits (or lack of them) of the old API, no
> disagrement there.

The old API is already broken. E.g. using MEDIA_ENT_F_PROC_VIDEO_SCALER in
vimc/vimc-scaler.c (instead of the current - and bogus - MEDIA_ENT_F_ATV_DECODER)
gives me this with media-ctl:

- entity 21: Scaler (2 pads, 4 links)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev5
        pad0: Sink
                [fmt:RGB888_1X24/640x480 field:none]
                <- "Debayer A":1 [ENABLED]
                <- "Debayer B":1 []
                <- "RGB/YUV Input":0 []
        pad1: Source
                [fmt:RGB888_1X24/1920x1440 field:none]
                -> "RGB/YUV Capture":0 [ENABLED,IMMUTABLE]

Useless. We now have an old API that gives us pad indices but not the
function, and a new API that gives us the function but not the pad index.

How about adding a 'function' field to struct media_entity_desc
and fill that? Keep the type for backwards compatibility.

Then have a define like this:

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

that can be used to detect if the MC has function support.

Regards,

	Hans

Re: media_device.c question: can this workaround be removed?

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 16:19:28 +0100
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 02/05/2018 03:30 PM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Mon, Feb 05, 2018 at 01:30:04PM +0100, Hans Verkuil wrote:  
> >> On 02/05/2018 12:59 PM, Sakari Ailus wrote:  
> >>> Hi Hans,
> >>>
> >>> On Mon, Feb 05, 2018 at 11:26:47AM +0100, Hans Verkuil wrote:  
> >>>> The function media_device_enum_entities() has this workaround:
> >>>>
> >>>>         /*
> >>>>          * Workaround for a bug at media-ctl <= v1.10 that makes it to
> >>>>          * do the wrong thing if the entity function doesn't belong to
> >>>>          * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
> >>>>          * Ranges.
> >>>>          *
> >>>>          * Non-subdevices are expected to be at the MEDIA_ENT_F_OLD_BASE,
> >>>>          * or, otherwise, will be silently ignored by media-ctl when
> >>>>          * printing the graphviz diagram. So, map them into the devnode
> >>>>          * old range.
> >>>>          */
> >>>>         if (ent->function < MEDIA_ENT_F_OLD_BASE ||
> >>>>             ent->function > MEDIA_ENT_F_TUNER) {
> >>>>                 if (is_media_entity_v4l2_subdev(ent))
> >>>>                         entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
> >>>>                 else if (ent->function != MEDIA_ENT_F_IO_V4L)
> >>>>                         entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
> >>>>         }
> >>>>
> >>>> But this means that the entity type returned by ENUM_ENTITIES just overwrites
> >>>> perfectly fine types by bogus values and thus the returned value differs
> >>>> from that returned by G_TOPOLOGY.
> >>>>
> >>>> Can we please, please remove this workaround? I have no idea why a workaround
> >>>> for media-ctl of all things ever made it in the kernel.  
> >>>
> >>> The entity types were replaced by entity functions back in 2015 with the
> >>> big Media controller reshuffle. While I agree functions are better for
> >>> describing entities than types (and those types had Linux specific
> >>> interfaces in them), the new function-based API still may support a single
> >>> value, i.e. a single function per device.
> >>>
> >>> This also created two different name spaces for describing entities: the
> >>> old types used by the MC API and the new functions used by MC v2 API.
> >>>
> >>> This doesn't go well with the fact that, as you noticed, the pad
> >>> information is not available through MC v2 API. The pad information is
> >>> required by the user space so software continues to use the original MC
> >>> API.
> >>>
> >>> I don't think there's a way to avoid maintaining two name spaces (types and
> >>> functions) without breaking at least one of the APIs.  
> >>
> >> The comment specifically claims that this workaround is for media-ctl and
> >> it suggests that it is fixed after v1.10. Is that comment bogus? I can't
> >> really tell which commit fixed media-ctl. Does anyone know?
> >>
> >> As far as I can tell the function defines have been chosen in such a way that
> >> they will equally well work with the old name space. There should be no
> >> problem there whatsoever and media-ctl should switch to use the new defines.  
> > 
> > The old interface (type) was centered around the uAPI for the entity.
> > That's no longer the case for functions. The entity type
> > (MEDIA_ENT_TYPE_MASK) tells the uAPI which affects the interpretation of
> > the dev union in struct media_entity_struct as well as the interface that
> > device node implements. With the new function field that's no longer the
> > case.
> > 
> > Also, the new MC v2 API makes a separation between the entity function and
> > the uAPI (interface) which was lacking in the old API.
> >   
> >>
> >> We now have a broken ENUM_ENTITIES ioctl (it rudely overwrites VBI/DVB/etc types)
> >> and a broken G_TOPOLOGY ioctl (no pad index).
> >>
> >> This sucks. Let's fix both so that they at least report consistent information.  
> > 
> > The existing user space may assume that the type field of the entity
> > conveys that the entity does provide a V4L2 sub-device interface if that's
> > the case actually.
> > 
> > This is what media-ctl does and I presume if existing user space checks for
> > the type field, it may well have similar checks: it was how the API was
> > defined. Therefore it's not entirely accurate to say that only media-ctl
> > has this "bug", I'd generally assume programs that use MC (v1) API do this.
> > 
> > You could argue about the merits (or lack of them) of the old API, no
> > disagrement there.  
> 
> The old API is already broken. E.g. using MEDIA_ENT_F_PROC_VIDEO_SCALER in
> vimc/vimc-scaler.c (instead of the current - and bogus - MEDIA_ENT_F_ATV_DECODER)
> gives me this with media-ctl:
> 
> - entity 21: Scaler (2 pads, 4 links)
>              type V4L2 subdev subtype Unknown flags 0
>              device node name /dev/v4l-subdev5
>         pad0: Sink
>                 [fmt:RGB888_1X24/640x480 field:none]
>                 <- "Debayer A":1 [ENABLED]
>                 <- "Debayer B":1 []
>                 <- "RGB/YUV Input":0 []
>         pad1: Source
>                 [fmt:RGB888_1X24/1920x1440 field:none]
>                 -> "RGB/YUV Capture":0 [ENABLED,IMMUTABLE]  
> 
> Useless. We now have an old API that gives us pad indices but not the
> function, and a new API that gives us the function but not the pad index.

Adding pad index to new API is trivial, as your RFC patch pointed.

Changing media-ctl to fully use the new ioctl is not trivial, as it
was written on a non-portable way, very dependent on the kernel API
specifics[1]. I suspect that it is a lot more easier to add support
for MEDIA_IOC_SETUP_LINK to mc_nextgen_test and rename it to
media-ctl[2].

[1] I tried it before working at contrib/test/mc_nextgen_test.c. The
internal data model used by media-ctl library was just a clone of the
model returned by the ioctls. Even a minimal change on the way ioctls 
return things (even adding new entity types) is enough to break it.

[2] It would be possible, however, to make media-ctl to use 
G_TOPOLOGY, ignoring new features that can't be represented using
the old ioctls and providing fallback to entity functions in order
to represent media types. It won't be too much elegant though.

> How about adding a 'function' field to struct media_entity_desc
> and fill that? Keep the type for backwards compatibility.

Nah, Let's not touch the old ioctls. Instead, we should stick
with the new API and convert (or replace) existing applications to
use it, as the old ioctl set can't even represent the interfaces.

Thanks,
Mauro

Re: media_device.c question: can this workaround be removed?

[Sakari Ailus]

Hi Mauro and Hans,

On Mon, Feb 05, 2018 at 02:32:28PM -0200, Mauro Carvalho Chehab wrote:
> Em Mon, 5 Feb 2018 16:19:28 +0100
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > On 02/05/2018 03:30 PM, Sakari Ailus wrote:
> > > Hi Hans,
> > > 
> > > On Mon, Feb 05, 2018 at 01:30:04PM +0100, Hans Verkuil wrote:  
> > >> On 02/05/2018 12:59 PM, Sakari Ailus wrote:  
> > >>> Hi Hans,
> > >>>
> > >>> On Mon, Feb 05, 2018 at 11:26:47AM +0100, Hans Verkuil wrote:  
> > >>>> The function media_device_enum_entities() has this workaround:
> > >>>>
> > >>>>         /*
> > >>>>          * Workaround for a bug at media-ctl <= v1.10 that makes it to
> > >>>>          * do the wrong thing if the entity function doesn't belong to
> > >>>>          * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
> > >>>>          * Ranges.
> > >>>>          *
> > >>>>          * Non-subdevices are expected to be at the MEDIA_ENT_F_OLD_BASE,
> > >>>>          * or, otherwise, will be silently ignored by media-ctl when
> > >>>>          * printing the graphviz diagram. So, map them into the devnode
> > >>>>          * old range.
> > >>>>          */
> > >>>>         if (ent->function < MEDIA_ENT_F_OLD_BASE ||
> > >>>>             ent->function > MEDIA_ENT_F_TUNER) {
> > >>>>                 if (is_media_entity_v4l2_subdev(ent))
> > >>>>                         entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
> > >>>>                 else if (ent->function != MEDIA_ENT_F_IO_V4L)
> > >>>>                         entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
> > >>>>         }
> > >>>>
> > >>>> But this means that the entity type returned by ENUM_ENTITIES just overwrites
> > >>>> perfectly fine types by bogus values and thus the returned value differs
> > >>>> from that returned by G_TOPOLOGY.
> > >>>>
> > >>>> Can we please, please remove this workaround? I have no idea why a workaround
> > >>>> for media-ctl of all things ever made it in the kernel.  
> > >>>
> > >>> The entity types were replaced by entity functions back in 2015 with the
> > >>> big Media controller reshuffle. While I agree functions are better for
> > >>> describing entities than types (and those types had Linux specific
> > >>> interfaces in them), the new function-based API still may support a single
> > >>> value, i.e. a single function per device.
> > >>>
> > >>> This also created two different name spaces for describing entities: the
> > >>> old types used by the MC API and the new functions used by MC v2 API.
> > >>>
> > >>> This doesn't go well with the fact that, as you noticed, the pad
> > >>> information is not available through MC v2 API. The pad information is
> > >>> required by the user space so software continues to use the original MC
> > >>> API.
> > >>>
> > >>> I don't think there's a way to avoid maintaining two name spaces (types and
> > >>> functions) without breaking at least one of the APIs.  
> > >>
> > >> The comment specifically claims that this workaround is for media-ctl and
> > >> it suggests that it is fixed after v1.10. Is that comment bogus? I can't
> > >> really tell which commit fixed media-ctl. Does anyone know?
> > >>
> > >> As far as I can tell the function defines have been chosen in such a way that
> > >> they will equally well work with the old name space. There should be no
> > >> problem there whatsoever and media-ctl should switch to use the new defines.  
> > > 
> > > The old interface (type) was centered around the uAPI for the entity.
> > > That's no longer the case for functions. The entity type
> > > (MEDIA_ENT_TYPE_MASK) tells the uAPI which affects the interpretation of
> > > the dev union in struct media_entity_struct as well as the interface that
> > > device node implements. With the new function field that's no longer the
> > > case.
> > > 
> > > Also, the new MC v2 API makes a separation between the entity function and
> > > the uAPI (interface) which was lacking in the old API.
> > >   
> > >>
> > >> We now have a broken ENUM_ENTITIES ioctl (it rudely overwrites VBI/DVB/etc types)
> > >> and a broken G_TOPOLOGY ioctl (no pad index).
> > >>
> > >> This sucks. Let's fix both so that they at least report consistent information.  
> > > 
> > > The existing user space may assume that the type field of the entity
> > > conveys that the entity does provide a V4L2 sub-device interface if that's
> > > the case actually.
> > > 
> > > This is what media-ctl does and I presume if existing user space checks for
> > > the type field, it may well have similar checks: it was how the API was
> > > defined. Therefore it's not entirely accurate to say that only media-ctl
> > > has this "bug", I'd generally assume programs that use MC (v1) API do this.
> > > 
> > > You could argue about the merits (or lack of them) of the old API, no
> > > disagrement there.  
> > 
> > The old API is already broken. E.g. using MEDIA_ENT_F_PROC_VIDEO_SCALER in
> > vimc/vimc-scaler.c (instead of the current - and bogus - MEDIA_ENT_F_ATV_DECODER)
> > gives me this with media-ctl:
> > 
> > - entity 21: Scaler (2 pads, 4 links)
> >              type V4L2 subdev subtype Unknown flags 0
> >              device node name /dev/v4l-subdev5
> >         pad0: Sink
> >                 [fmt:RGB888_1X24/640x480 field:none]
> >                 <- "Debayer A":1 [ENABLED]
> >                 <- "Debayer B":1 []
> >                 <- "RGB/YUV Input":0 []
> >         pad1: Source
> >                 [fmt:RGB888_1X24/1920x1440 field:none]
> >                 -> "RGB/YUV Capture":0 [ENABLED,IMMUTABLE]  
> > 
> > Useless. We now have an old API that gives us pad indices but not the
> > function, and a new API that gives us the function but not the pad index.
> 
> Adding pad index to new API is trivial, as your RFC patch pointed.
> 
> Changing media-ctl to fully use the new ioctl is not trivial, as it
> was written on a non-portable way, very dependent on the kernel API
> specifics[1]. I suspect that it is a lot more easier to add support
> for MEDIA_IOC_SETUP_LINK to mc_nextgen_test and rename it to
> media-ctl[2].

libmediactl (which media-ctl uses) was intended to be the user space
library for MC. Perhaps analogous to libv4l on V4L2.

It builds an internal state from what it queries from the kernel, it
shouldn't be *that* difficult to change it. The API can be changed, too,
it's not exported as a library from v4l-utils.

> 
> [1] I tried it before working at contrib/test/mc_nextgen_test.c. The
> internal data model used by media-ctl library was just a clone of the
> model returned by the ioctls. Even a minimal change on the way ioctls 
> return things (even adding new entity types) is enough to break it.

The entity type is only used for informational purposes AFAIK. Otherwise
it'll just say "unknown".

Don't forget that media-ctl also supports most of V4L2 sub-device API.

> 
> [2] It would be possible, however, to make media-ctl to use 
> G_TOPOLOGY, ignoring new features that can't be represented using
> the old ioctls and providing fallback to entity functions in order
> to represent media types. It won't be too much elegant though.
> 
> > How about adding a 'function' field to struct media_entity_desc
> > and fill that? Keep the type for backwards compatibility.
> 
> Nah, Let's not touch the old ioctls. Instead, we should stick
> with the new API and convert (or replace) existing applications to
> use it, as the old ioctl set can't even represent the interfaces.

MC v2 would need better support for pad information, as well as flags. At
least. I haven't decided yet what to think of this.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: media_device.c question: can this workaround be removed?

[Mauro Carvalho Chehab]

Em Mon, 5 Feb 2018 23:48:38 +0200
Sakari Ailus <sakari.ailus@iki.fi> escreveu:

> Hi Mauro and Hans,

> > Adding pad index to new API is trivial, as your RFC patch pointed.
> > 
> > Changing media-ctl to fully use the new ioctl is not trivial, as it
> > was written on a non-portable way, very dependent on the kernel API
> > specifics[1]. I suspect that it is a lot more easier to add support
> > for MEDIA_IOC_SETUP_LINK to mc_nextgen_test and rename it to
> > media-ctl[2].  
> 
> libmediactl (which media-ctl uses) was intended to be the user space
> library for MC. Perhaps analogous to libv4l on V4L2.
> 
> It builds an internal state from what it queries from the kernel, it
> shouldn't be *that* difficult to change it. The API can be changed, too,
> it's not exported as a library from v4l-utils.

Yes, I saw that at the time I tried to add support for G_TOPOLOGY
on it. Have you ever seen the contrib/test/mc_nextgen_test.c code?

It was written in a way that its code could easily be changed into
a library to store/retrieve the topology.

It uses only 3 functions to get topology:

mc_open() - opens device and allocs an struct
media_get_topology() - implements support for G_TOPOLOGY
mc_close() - closes devide and frees allocated data

And provides other functions to show the retrieved data:

	media_show_graphviz()
	media_show_entities()
	media_show_interfaces()
	media_show_links()

It is not a library, but it shouldn't be hard to convert into one,
copying the relevant parts of mc_open(), media_get_topology() 
and mc_close() into libmediactl, and removing/changing a few
prints there in order to match the way libmediactl works.

The big challenge with libmediactl is that it is too bound to the
API v1. For example, extending the logic there to support links
between entities and interfaces would be a challenge, and won't
be too efficient.

So, IMHO, it is easier to get the data model and the retrieve function
from mc_nextgen_test and use it as basis for V2-compliant version of
libmediactl. All it needs (from MC API PoV) is to add support for
setting links. As it uses the data provided by G_TOPOLOGY, if we add
new fields there, it will automatically be stored inside its data
model, as there's no abstraction layer.

With regards to the subdev API part of libmediactl, that probably doesn't
need to be touched (or if it needs, it would likely be just binding stuff
with the new data model).

> 
> > 
> > [1] I tried it before working at contrib/test/mc_nextgen_test.c. The
> > internal data model used by media-ctl library was just a clone of the
> > model returned by the ioctls. Even a minimal change on the way ioctls 
> > return things (even adding new entity types) is enough to break it.  
> 
> The entity type is only used for informational purposes AFAIK. Otherwise
> it'll just say "unknown".
> 
> Don't forget that media-ctl also supports most of V4L2 sub-device API.

Yes, but that part likely doesn't require changes - of if it requires,
it would probably be minimal ones.

> > Nah, Let's not touch the old ioctls. Instead, we should stick
> > with the new API and convert (or replace) existing applications to
> > use it, as the old ioctl set can't even represent the interfaces.  
> 
> MC v2 would need better support for pad information, as well as flags. At
> least. I haven't decided yet what to think of this.

While we don't have a properties API, let's add what's needed for
media-ctl to work. If flags are needed, let's add it.

Thanks,
Mauro

Re: media_device.c question: can this workaround be removed?

[Hans Verkuil]

On 02/06/18 11:15, Mauro Carvalho Chehab wrote:
> Em Mon, 5 Feb 2018 23:48:38 +0200
> Sakari Ailus <sakari.ailus@iki.fi> escreveu:
> 
>> Hi Mauro and Hans,
> 
>>> Adding pad index to new API is trivial, as your RFC patch pointed.
>>>
>>> Changing media-ctl to fully use the new ioctl is not trivial, as it
>>> was written on a non-portable way, very dependent on the kernel API
>>> specifics[1]. I suspect that it is a lot more easier to add support
>>> for MEDIA_IOC_SETUP_LINK to mc_nextgen_test and rename it to
>>> media-ctl[2].  
>>
>> libmediactl (which media-ctl uses) was intended to be the user space
>> library for MC. Perhaps analogous to libv4l on V4L2.
>>
>> It builds an internal state from what it queries from the kernel, it
>> shouldn't be *that* difficult to change it. The API can be changed, too,
>> it's not exported as a library from v4l-utils.
> 
> Yes, I saw that at the time I tried to add support for G_TOPOLOGY
> on it. Have you ever seen the contrib/test/mc_nextgen_test.c code?
> 
> It was written in a way that its code could easily be changed into
> a library to store/retrieve the topology.
> 
> It uses only 3 functions to get topology:
> 
> mc_open() - opens device and allocs an struct
> media_get_topology() - implements support for G_TOPOLOGY
> mc_close() - closes devide and frees allocated data
> 
> And provides other functions to show the retrieved data:
> 
> 	media_show_graphviz()
> 	media_show_entities()
> 	media_show_interfaces()
> 	media_show_links()
> 
> It is not a library, but it shouldn't be hard to convert into one,
> copying the relevant parts of mc_open(), media_get_topology() 
> and mc_close() into libmediactl, and removing/changing a few
> prints there in order to match the way libmediactl works.
> 
> The big challenge with libmediactl is that it is too bound to the
> API v1. For example, extending the logic there to support links
> between entities and interfaces would be a challenge, and won't
> be too efficient.
> 
> So, IMHO, it is easier to get the data model and the retrieve function
> from mc_nextgen_test and use it as basis for V2-compliant version of
> libmediactl. All it needs (from MC API PoV) is to add support for
> setting links. As it uses the data provided by G_TOPOLOGY, if we add
> new fields there, it will automatically be stored inside its data
> model, as there's no abstraction layer.
> 
> With regards to the subdev API part of libmediactl, that probably doesn't
> need to be touched (or if it needs, it would likely be just binding stuff
> with the new data model).
> 
>>
>>>
>>> [1] I tried it before working at contrib/test/mc_nextgen_test.c. The
>>> internal data model used by media-ctl library was just a clone of the
>>> model returned by the ioctls. Even a minimal change on the way ioctls 
>>> return things (even adding new entity types) is enough to break it.  
>>
>> The entity type is only used for informational purposes AFAIK. Otherwise
>> it'll just say "unknown".
>>
>> Don't forget that media-ctl also supports most of V4L2 sub-device API.
> 
> Yes, but that part likely doesn't require changes - of if it requires,
> it would probably be minimal ones.
> 
>>> Nah, Let's not touch the old ioctls. Instead, we should stick
>>> with the new API and convert (or replace) existing applications to
>>> use it, as the old ioctl set can't even represent the interfaces.  
>>
>> MC v2 would need better support for pad information, as well as flags. At
>> least. I haven't decided yet what to think of this.
> 
> While we don't have a properties API, let's add what's needed for
> media-ctl to work. If flags are needed, let's add it.

Unless there are objections, then I would like to work on this.

I have this in mind:

Once the merge window is closed and we're open again for pull requests
I would like to first get the basic fixes (zeroing reserved fields, etc.)
and ideally the cleaned up media.h merged (the current media.h is unreadable!)

The next step would be to add pad index and type support to G_TOPOLOGY.
I still would like to add function support to the old API, it's trivial
and it really helps testing. We'll discuss that separately.

Then work on improving media-ctl. I think media-ctl and mc_nextgen_test
should be merged so we are left with a single media-ctl utility.

Three things are remaining after that: a S_TOPOLOGY (or however it will
be names) call, properties and connector support. But at least we then
have a solid base to continue working from. For the record, I have no
plans to work on S_TOPOLOGY or properties, but I might tackle connector
support. I want to add MC support to CEC and I probably will need this.

Regards,

	Hans

Re: media_device.c question: can this workaround be removed?

[Sakari Ailus]

Hi Hans,

On Mon, Feb 05, 2018 at 04:19:28PM +0100, Hans Verkuil wrote:
> On 02/05/2018 03:30 PM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > On Mon, Feb 05, 2018 at 01:30:04PM +0100, Hans Verkuil wrote:
> >> On 02/05/2018 12:59 PM, Sakari Ailus wrote:
> >>> Hi Hans,
> >>>
> >>> On Mon, Feb 05, 2018 at 11:26:47AM +0100, Hans Verkuil wrote:
> >>>> The function media_device_enum_entities() has this workaround:
> >>>>
> >>>>         /*
> >>>>          * Workaround for a bug at media-ctl <= v1.10 that makes it to
> >>>>          * do the wrong thing if the entity function doesn't belong to
> >>>>          * either MEDIA_ENT_F_OLD_BASE or MEDIA_ENT_F_OLD_SUBDEV_BASE
> >>>>          * Ranges.
> >>>>          *
> >>>>          * Non-subdevices are expected to be at the MEDIA_ENT_F_OLD_BASE,
> >>>>          * or, otherwise, will be silently ignored by media-ctl when
> >>>>          * printing the graphviz diagram. So, map them into the devnode
> >>>>          * old range.
> >>>>          */
> >>>>         if (ent->function < MEDIA_ENT_F_OLD_BASE ||
> >>>>             ent->function > MEDIA_ENT_F_TUNER) {
> >>>>                 if (is_media_entity_v4l2_subdev(ent))
> >>>>                         entd->type = MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
> >>>>                 else if (ent->function != MEDIA_ENT_F_IO_V4L)
> >>>>                         entd->type = MEDIA_ENT_T_DEVNODE_UNKNOWN;
> >>>>         }
> >>>>
> >>>> But this means that the entity type returned by ENUM_ENTITIES just overwrites
> >>>> perfectly fine types by bogus values and thus the returned value differs
> >>>> from that returned by G_TOPOLOGY.
> >>>>
> >>>> Can we please, please remove this workaround? I have no idea why a workaround
> >>>> for media-ctl of all things ever made it in the kernel.
> >>>
> >>> The entity types were replaced by entity functions back in 2015 with the
> >>> big Media controller reshuffle. While I agree functions are better for
> >>> describing entities than types (and those types had Linux specific
> >>> interfaces in them), the new function-based API still may support a single
> >>> value, i.e. a single function per device.
> >>>
> >>> This also created two different name spaces for describing entities: the
> >>> old types used by the MC API and the new functions used by MC v2 API.
> >>>
> >>> This doesn't go well with the fact that, as you noticed, the pad
> >>> information is not available through MC v2 API. The pad information is
> >>> required by the user space so software continues to use the original MC
> >>> API.
> >>>
> >>> I don't think there's a way to avoid maintaining two name spaces (types and
> >>> functions) without breaking at least one of the APIs.
> >>
> >> The comment specifically claims that this workaround is for media-ctl and
> >> it suggests that it is fixed after v1.10. Is that comment bogus? I can't
> >> really tell which commit fixed media-ctl. Does anyone know?
> >>
> >> As far as I can tell the function defines have been chosen in such a way that
> >> they will equally well work with the old name space. There should be no
> >> problem there whatsoever and media-ctl should switch to use the new defines.
> > 
> > The old interface (type) was centered around the uAPI for the entity.
> > That's no longer the case for functions. The entity type
> > (MEDIA_ENT_TYPE_MASK) tells the uAPI which affects the interpretation of
> > the dev union in struct media_entity_struct as well as the interface that
> > device node implements. With the new function field that's no longer the
> > case.
> > 
> > Also, the new MC v2 API makes a separation between the entity function and
> > the uAPI (interface) which was lacking in the old API.
> > 
> >>
> >> We now have a broken ENUM_ENTITIES ioctl (it rudely overwrites VBI/DVB/etc types)
> >> and a broken G_TOPOLOGY ioctl (no pad index).
> >>
> >> This sucks. Let's fix both so that they at least report consistent information.
> > 
> > The existing user space may assume that the type field of the entity
> > conveys that the entity does provide a V4L2 sub-device interface if that's
> > the case actually.
> > 
> > This is what media-ctl does and I presume if existing user space checks for
> > the type field, it may well have similar checks: it was how the API was
> > defined. Therefore it's not entirely accurate to say that only media-ctl
> > has this "bug", I'd generally assume programs that use MC (v1) API do this.
> > 
> > You could argue about the merits (or lack of them) of the old API, no
> > disagrement there.
> 
> The old API is already broken. E.g. using MEDIA_ENT_F_PROC_VIDEO_SCALER in
> vimc/vimc-scaler.c (instead of the current - and bogus - MEDIA_ENT_F_ATV_DECODER)
> gives me this with media-ctl:
> 
> - entity 21: Scaler (2 pads, 4 links)
>              type V4L2 subdev subtype Unknown flags 0
>              device node name /dev/v4l-subdev5
>         pad0: Sink
>                 [fmt:RGB888_1X24/640x480 field:none]
>                 <- "Debayer A":1 [ENABLED]
>                 <- "Debayer B":1 []
>                 <- "RGB/YUV Input":0 []
>         pad1: Source
>                 [fmt:RGB888_1X24/1920x1440 field:none]
>                 -> "RGB/YUV Capture":0 [ENABLED,IMMUTABLE]
> 
> Useless. We now have an old API that gives us pad indices but not the
> function, and a new API that gives us the function but not the pad index.

Not really useless. What it tells is that the entity has a V4L2 sub-device
interface. If you remove the code snippet there, then the user space has no
knowledge of that anymore. I expect changing that to break stuff --- this
is how applications find their device nodes, after all.

> 
> How about adding a 'function' field to struct media_entity_desc
> and fill that? Keep the type for backwards compatibility.
> 
> Then have a define like this:
> 
> #define MEDIA_ENT_HAS_FUNCTION(media_version) ((media_version) >= KERNEL_VERSION(a, b, c))
> 
> that can be used to detect if the MC has function support.

I'm not really too worried that the exact device function isn't conveyed to
the user space. The interface the device provides is more important: when
the user space program is looking for a device with a particular name, it
already knows what kind of a device it is. But it might check that the
interface is what it expects, too.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi