On 02/21/18 16:14, Sakari Ailus wrote:
> Hi Hans,
>
> On Wed, Feb 21, 2018 at 03:33:03PM +0100, Hans Verkuil wrote:
>> On 02/21/18 15:15, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> On Mon, Feb 19, 2018 at 11:38:06AM +0100, Hans Verkuil wrote:
>>>> The media.h public header is very messy. It mixes legacy and 'new' defines
>>>> and it is not easy to figure out what should and what shouldn't be used. It
>>>> also contains confusing comment that are either out of date or completely
>>>> uninteresting for anyone that needs to use this header.
>>>>
>>>> The patch groups all entity functions together, including the 'old' defines
>>>> based on the old range base. The reader just wants to know about the
>>>> available
>>>> functions and doesn't care about what range is used.
>>>>
>>>> All legacy defines are moved to the end of the header, so it is easier to
>>>> locate them and just ignore them.
>>>>
>>>> The legacy structs in the struct media_entity_desc are put under
>>>> #if !defined(__KERNEL__) to prevent the kernel from using them, and this is
>>>> also a much more effective signal to the reader that they shouldn't be used
>>>> compared to the old method of relying on '#if 1' followed by a comment.
>>>>
>>>> The unused MEDIA_INTF_T_ALSA_* defines are also moved to the end of the
>>>> header
>>>> in the legacy area. They are also dropped from intf_type() in
>>>> media-entity.c.
>>>>
>>>> All defines are also aligned at the same tab making the header easier to
>>>> read.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verk...@cisco.com>
>>>> ---
>>>> drivers/media/media-entity.c | 16 --
>>>> include/uapi/linux/media.h | 345
>>>> +++++++++++++++++++++----------------------
>>>> 2 files changed, 166 insertions(+), 195 deletions(-)
>>>>
>>
>> <snip>
>>
>>>> /*
>>>> - * I/O entities
>>>> + * Subdevs are initialized with MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN in order
>>>> + * to preserve backward compatibility. Drivers must change to the proper
>>>> + * subdev type before registering the entity.
>>>
>>> s/type/function/
>>
>> Good catch!
>>
>>>
>>> I wonder if "should" would actually be better; the current API assumes an
>>> entity has at most one function which is somehow defined by the API. I
>>> guess you could force everything into some kind of a group, as the current
>>> approach seems to be. This is also what the enumeration through
>>> MEDIA_IOC_ENUM_ENTITIES will yield to for the new functions anyway.
>>
>> I don't quite follow you. 'should' suggests that they don't have to, but
>> that's
>> not the case. They really must set function to something other than UNKNOWN.
>
> I'd like to reiterate that a single function cannot fully describe all
> entities, and not all of them fit well to existing categories. If an entity
> has multiple functions, which one do you pick?
>
> Say, a SoC camera that also controls the lens.
In that case I would pick CAM_SENSOR and once we have properties you can add
lens controller to the list of functions.
CAM_SENSOR is *far* better than just leaving it at UNKNOWN. Especially since
properties are still vaporware. I'm interested in working on that, but first
I want to get all these compliance issues sorted.
>
>>
>> The fact that newer functions are never actually exposed in the
>> MEDIA_IOC_ENUM_ENTITIES
>> ioctl since there only is a 'type' field and not a 'function' field is
>> another
>> matter altogether.
>
> I don't have a strong opinion on this, feel free to leave it as-is for now.
>
>>
>>>
>>>> */
>>>> -#define MEDIA_ENT_F_IO_DTV (MEDIA_ENT_F_BASE + 0x01001)
>>>> -#define MEDIA_ENT_F_IO_VBI (MEDIA_ENT_F_BASE + 0x01002)
>>>> -#define MEDIA_ENT_F_IO_SWRADIO (MEDIA_ENT_F_BASE + 0x01003)
>>>> +#define MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN
>>>> MEDIA_ENT_F_OLD_SUBDEV_BASE
>>>>
>>>> /*
>>>> - * Analog TV IF-PLL decoders
>>>> - *
>>>> - * It is a responsibility of the master/bridge drivers to create links
>>>> - * for MEDIA_ENT_F_IF_VID_DECODER and MEDIA_ENT_F_IF_AUD_DECODER.
>>>> + * DVB entity functions
>>>> */
>>>> -#define MEDIA_ENT_F_IF_VID_DECODER (MEDIA_ENT_F_BASE + 0x02001)
>>>> -#define MEDIA_ENT_F_IF_AUD_DECODER (MEDIA_ENT_F_BASE + 0x02002)
>>>> +#define MEDIA_ENT_F_DTV_DEMOD (MEDIA_ENT_F_BASE +
>>>> 0x00001)
>>>> +#define MEDIA_ENT_F_TS_DEMUX (MEDIA_ENT_F_BASE +
>>>> 0x00002)
>>>> +#define MEDIA_ENT_F_DTV_CA (MEDIA_ENT_F_BASE +
>>>> 0x00003)
>>>> +#define MEDIA_ENT_F_DTV_NET_DECAP (MEDIA_ENT_F_BASE + 0x00004)
>>>>
>>>> /*
>>>> - * Audio Entity Functions
>>>> + * I/O entity functions
>>>> */
>>>> -#define MEDIA_ENT_F_AUDIO_CAPTURE (MEDIA_ENT_F_BASE + 0x03001)
>>>> -#define MEDIA_ENT_F_AUDIO_PLAYBACK (MEDIA_ENT_F_BASE + 0x03002)
>>>> -#define MEDIA_ENT_F_AUDIO_MIXER (MEDIA_ENT_F_BASE + 0x03003)
>>>> +#define MEDIA_ENT_F_IO_V4L (MEDIA_ENT_F_OLD_BASE +
>>>> 1)
>>>> +#define MEDIA_ENT_F_IO_DTV (MEDIA_ENT_F_BASE +
>>>> 0x01001)
>>>> +#define MEDIA_ENT_F_IO_VBI (MEDIA_ENT_F_BASE +
>>>> 0x01002)
>>>> +#define MEDIA_ENT_F_IO_SWRADIO (MEDIA_ENT_F_BASE +
>>>> 0x01003)
>>>>
>>>> /*
>>>> - * Processing entities
>>>> + * Sensor functions
>>>> */
>>>> -#define MEDIA_ENT_F_PROC_VIDEO_COMPOSER (MEDIA_ENT_F_BASE +
>>>> 0x4001)
>>>> -#define MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER (MEDIA_ENT_F_BASE +
>>>> 0x4002)
>>>> -#define MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV (MEDIA_ENT_F_BASE +
>>>> 0x4003)
>>>> -#define MEDIA_ENT_F_PROC_VIDEO_LUT (MEDIA_ENT_F_BASE +
>>>> 0x4004)
>>>> -#define MEDIA_ENT_F_PROC_VIDEO_SCALER (MEDIA_ENT_F_BASE +
>>>> 0x4005)
>>>> -#define MEDIA_ENT_F_PROC_VIDEO_STATISTICS (MEDIA_ENT_F_BASE + 0x4006)
>>>> +#define MEDIA_ENT_F_CAM_SENSOR
>>>> (MEDIA_ENT_F_OLD_SUBDEV_BASE + 1)
>>>> +#define MEDIA_ENT_F_FLASH (MEDIA_ENT_F_OLD_SUBDEV_BASE +
>>>> 2)
>>>> +#define MEDIA_ENT_F_LENS (MEDIA_ENT_F_OLD_SUBDEV_BASE +
>>>> 3)
>>>>
>>>> /*
>>>> - * Switch and bridge entitites
>>>> + * Analog video decoder functions
>>>> */
>>>> -#define MEDIA_ENT_F_VID_MUX (MEDIA_ENT_F_BASE +
>>>> 0x5001)
>>>> -#define MEDIA_ENT_F_VID_IF_BRIDGE (MEDIA_ENT_F_BASE + 0x5002)
>>>> +#define MEDIA_ENT_F_ATV_DECODER
>>>> (MEDIA_ENT_F_OLD_SUBDEV_BASE + 4)
>>>>
>>>> /*
>>>> - * Connectors
>>>> - */
>>>> -/* It is a responsibility of the entity drivers to add connectors and
>>>> links */
>>>> -#ifdef __KERNEL__
>>>> - /*
>>>> - * For now, it should not be used in userspace, as some
>>>> - * definitions may change
>>>> - */
>>>> -
>>>> -#define MEDIA_ENT_F_CONN_RF (MEDIA_ENT_F_BASE + 0x30001)
>>>> -#define MEDIA_ENT_F_CONN_SVIDEO (MEDIA_ENT_F_BASE + 0x30002)
>>>> -#define MEDIA_ENT_F_CONN_COMPOSITE (MEDIA_ENT_F_BASE + 0x30003)
>>>> -
>>>> -#endif
>>>> -
>>>> -/*
>>>> - * Don't touch on those. The ranges MEDIA_ENT_F_OLD_BASE and
>>>> - * MEDIA_ENT_F_OLD_SUBDEV_BASE are kept to keep backward compatibility
>>>> - * with the legacy v1 API.The number range is out of range by purpose:
>>>> - * several previously reserved numbers got excluded from this range.
>>>> + * Digital TV, analog TV, radio and/or software defined radio tuner
>>>> functions.
>>>> *
>>>> - * Subdevs are initialized with MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN,
>>>> - * in order to preserve backward compatibility.
>>>> - * Drivers must change to the proper subdev type before
>>>> - * registering the entity.
>>>> - */
>>>> -
>>>> -#define MEDIA_ENT_F_IO_V4L (MEDIA_ENT_F_OLD_BASE + 1)
>>>> -
>>>> -#define MEDIA_ENT_F_CAM_SENSOR (MEDIA_ENT_F_OLD_SUBDEV_BASE +
>>>> 1)
>>>> -#define MEDIA_ENT_F_FLASH (MEDIA_ENT_F_OLD_SUBDEV_BASE + 2)
>>>> -#define MEDIA_ENT_F_LENS (MEDIA_ENT_F_OLD_SUBDEV_BASE + 3)
>>>> -#define MEDIA_ENT_F_ATV_DECODER (MEDIA_ENT_F_OLD_SUBDEV_BASE +
>>>> 4)
>>>> -/*
>>>> * It is a responsibility of the master/bridge drivers to add connectors
>>>> * and links for MEDIA_ENT_F_TUNER. Please notice that some old tuners
>>>> * may require the usage of separate I2C chips to decode analog TV
>>>> signals,
>>>> @@ -151,49 +104,46 @@ struct media_device_info {
>>>> * On such cases, the IF-PLL staging is mapped via one or two entities:
>>>> * MEDIA_ENT_F_IF_VID_DECODER and/or MEDIA_ENT_F_IF_AUD_DECODER.
>>>> */
>>>> -#define MEDIA_ENT_F_TUNER (MEDIA_ENT_F_OLD_SUBDEV_BASE + 5)
>>>> +#define MEDIA_ENT_F_TUNER (MEDIA_ENT_F_OLD_SUBDEV_BASE +
>>>> 5)
>>>>
>>>> -#define MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN MEDIA_ENT_F_OLD_SUBDEV_BASE
>>>> +/*
>>>> + * Analog TV IF-PLL decoder functions
>>>> + *
>>>> + * It is a responsibility of the master/bridge drivers to create links
>>>> + * for MEDIA_ENT_F_IF_VID_DECODER and MEDIA_ENT_F_IF_AUD_DECODER.
>>>> + */
>>>> +#define MEDIA_ENT_F_IF_VID_DECODER (MEDIA_ENT_F_BASE +
>>>> 0x02001)
>>>> +#define MEDIA_ENT_F_IF_AUD_DECODER (MEDIA_ENT_F_BASE +
>>>> 0x02002)
>>>>
>>>> -#if !defined(__KERNEL__) || defined(__NEED_MEDIA_LEGACY_API)
>>>> +/*
>>>> + * Audio entity functions
>>>> + */
>>>> +#define MEDIA_ENT_F_AUDIO_CAPTURE (MEDIA_ENT_F_BASE + 0x03001)
>>>> +#define MEDIA_ENT_F_AUDIO_PLAYBACK (MEDIA_ENT_F_BASE +
>>>> 0x03002)
>>>> +#define MEDIA_ENT_F_AUDIO_MIXER (MEDIA_ENT_F_BASE +
>>>> 0x03003)
>>>>
>>>> /*
>>>> - * Legacy symbols used to avoid userspace compilation breakages
>>>> - *
>>>> - * Those symbols map the entity function into types and should be
>>>> - * used only on legacy programs for legacy hardware. Don't rely
>>>> - * on those for MEDIA_IOC_G_TOPOLOGY.
>>>> + * Processing entity functions
>>>> */
>>>> -#define MEDIA_ENT_TYPE_SHIFT 16
>>>> -#define MEDIA_ENT_TYPE_MASK 0x00ff0000
>>>> -#define MEDIA_ENT_SUBTYPE_MASK 0x0000ffff
>>>> -
>>>> -/* End of the old subdev reserved numberspace */
>>>> -#define MEDIA_ENT_T_DEVNODE_UNKNOWN (MEDIA_ENT_T_DEVNODE | \
>>>> - MEDIA_ENT_SUBTYPE_MASK)
>>>> -
>>>> -#define MEDIA_ENT_T_DEVNODE MEDIA_ENT_F_OLD_BASE
>>>> -#define MEDIA_ENT_T_DEVNODE_V4L MEDIA_ENT_F_IO_V4L
>>>> -#define MEDIA_ENT_T_DEVNODE_FB (MEDIA_ENT_T_DEVNODE + 2)
>>>> -#define MEDIA_ENT_T_DEVNODE_ALSA (MEDIA_ENT_T_DEVNODE + 3)
>>>> -#define MEDIA_ENT_T_DEVNODE_DVB (MEDIA_ENT_T_DEVNODE + 4)
>>>> -
>>>> -#define MEDIA_ENT_T_UNKNOWN MEDIA_ENT_F_UNKNOWN
>>>> -#define MEDIA_ENT_T_V4L2_VIDEO MEDIA_ENT_F_IO_V4L
>>>> -#define MEDIA_ENT_T_V4L2_SUBDEV MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN
>>>> -#define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR MEDIA_ENT_F_CAM_SENSOR
>>>> -#define MEDIA_ENT_T_V4L2_SUBDEV_FLASH MEDIA_ENT_F_FLASH
>>>> -#define MEDIA_ENT_T_V4L2_SUBDEV_LENS MEDIA_ENT_F_LENS
>>>> -#define MEDIA_ENT_T_V4L2_SUBDEV_DECODER MEDIA_ENT_F_ATV_DECODER
>>>> -#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER MEDIA_ENT_F_TUNER
>>>> +#define MEDIA_ENT_F_PROC_VIDEO_COMPOSER (MEDIA_ENT_F_BASE +
>>>> 0x4001)
>>>> +#define MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER (MEDIA_ENT_F_BASE +
>>>> 0x4002)
>>>> +#define MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV (MEDIA_ENT_F_BASE +
>>>> 0x4003)
>>>> +#define MEDIA_ENT_F_PROC_VIDEO_LUT (MEDIA_ENT_F_BASE +
>>>> 0x4004)
>>>> +#define MEDIA_ENT_F_PROC_VIDEO_SCALER (MEDIA_ENT_F_BASE +
>>>> 0x4005)
>>>> +#define MEDIA_ENT_F_PROC_VIDEO_STATISTICS (MEDIA_ENT_F_BASE + 0x4006)
>>>>
>>>> -/* Obsolete symbol for media_version, no longer used in the kernel */
>>>> -#define MEDIA_API_VERSION KERNEL_VERSION(0, 1, 0)
>>>> -#endif
>>>> +/*
>>>> + * Switch and bridge entity functions
>>>> + */
>>>> +#define MEDIA_ENT_F_VID_MUX (MEDIA_ENT_F_BASE +
>>>> 0x5001)
>>>> +#define MEDIA_ENT_F_VID_IF_BRIDGE (MEDIA_ENT_F_BASE + 0x5002)
>>>>
>>>> /* Entity flags */
>>>> -#define MEDIA_ENT_FL_DEFAULT (1 << 0)
>>>> -#define MEDIA_ENT_FL_CONNECTOR (1 << 1)
>>>> +#define MEDIA_ENT_FL_DEFAULT (1 << 0)
>>>> +#define MEDIA_ENT_FL_CONNECTOR (1 << 1)
>>>> +
>>>> +/* OR with the entity id value to find the next entity */
>>>
>>> This comment is confusing, I'd prefer to either drop or improve it. This is
>>> well documented in uAPI documentation after all.
>>
>> Hmm. How about:
>>
>> /* Modifier flag for the media_entity_desc 'id' field */
>>
>> Or, alternatively: /* Entity ID flag */
>>
>> I'd like to have something, otherwise it is just a random define without a
>> comment that puts it into context.
>
> How about:
>
> /* Flags for struct media_entity_desc id field */
Sounds good. I'll take this.
>
> Pick the one you like the best.
>
> Acked-by: Sakari Ailus <sakari.ai...@linux.intel.com>
>
Regards,
Hans
