[PATCHv2 0/9] media/mc: fix inconsistencies

[PATCHv2 0/9] media/mc: fix inconsistencies

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This patch series is a follow-up to these two v1 series:

https://www.mail-archive.com/linux-media@vger.kernel.org/msg127943.html
https://www.mail-archive.com/linux-media@vger.kernel.org/msg127963.html

Some of those patches have been merged for 4.17, so this v2 contains
the remainder and has been updated/rebased for 4.18.

The first two patches add the missing hsv_enc to struct v4l2_mbus_framefmt.
The next patch removes the ugly and IMHO dangerous __NEED_MEDIA_LEGACY_API
define from media.h.

The remaining 6 patches add missing features to the 'old' and 'new' media
controller API. Afterwards the two APIs are the same, except that the new
API exposes interfaces (but that's reasonable since it is a superset of
the 'old' API).

While I am calling it 'old' and 'new' API, there is no reason why applications
can't just pick which API to use, just like applications can choose whether
to use QUERYCTRL or QUERY_EXT_CTRL. The latter ioctl is only required if you
need the new functionality that it gives you.

The one thing I did not add to the 'old' API is to expose the pad/link IDs.
While there is room for it in the structs, there is no API that uses those
IDs at the moment, and I think it would be confusing.

Link IDs would most likely be used with a future S_TOPOLOGY ioctl and not
with the old SETUP_LINK ioctl.

Regards,

	Hans

Hans Verkuil (9):
  v4l2-mediabus.h: add hsv_enc
  subdev-formats.rst: fix incorrect types
  media.h: remove __NEED_MEDIA_LEGACY_API
  media: add function field to struct media_entity_desc
  media-ioc-enum-entities.rst: document new 'function' field
  media: add 'index' to struct media_v2_pad
  media-ioc-g-topology.rst: document new 'index' field
  media: add flags field to struct media_v2_entity
  media-ioc-g-topology.rst: document new 'flags' field

 .../uapi/mediactl/media-ioc-enum-entities.rst      | 31 +++++++++++++++++-----
 .../media/uapi/mediactl/media-ioc-g-topology.rst   | 25 +++++++++++++++--
 Documentation/media/uapi/v4l/subdev-formats.rst    | 27 ++++++++++++++-----
 drivers/media/media-device.c                       | 16 ++++++++---
 include/uapi/linux/media.h                         | 23 +++++++++++++---
 include/uapi/linux/v4l2-mediabus.h                 |  8 +++++-
 6 files changed, 108 insertions(+), 22 deletions(-)

-- 
2.15.1

[PATCHv2 1/9] v4l2-mediabus.h: add hsv_enc

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Just like struct v4l2_pix_format add a hsv_enc field to describe
the HSV encoding. It is in a union with the ycbcr_enc, since it
is one or the other.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 include/uapi/linux/v4l2-mediabus.h | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h
index 123a231001a8..52fd6cc9d491 100644
--- a/include/uapi/linux/v4l2-mediabus.h
+++ b/include/uapi/linux/v4l2-mediabus.h
@@ -24,6 +24,7 @@
  * @field:	used interlacing type (from enum v4l2_field)
  * @colorspace:	colorspace of the data (from enum v4l2_colorspace)
  * @ycbcr_enc:	YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
+ * @hsv_enc:	HSV encoding of the data (from enum v4l2_hsv_encoding)
  * @quantization: quantization of the data (from enum v4l2_quantization)
  * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)
  */
@@ -33,7 +34,12 @@ struct v4l2_mbus_framefmt {
 	__u32			code;
 	__u32			field;
 	__u32			colorspace;
-	__u16			ycbcr_enc;
+	union {
+		/* enum v4l2_ycbcr_encoding */
+		__u16		ycbcr_enc;
+		/* enum v4l2_hsv_encoding */
+		__u16		hsv_enc;
+	};
 	__u16			quantization;
 	__u16			xfer_func;
 	__u16			reserved[11];
-- 
2.15.1

[PATCHv2 2/9] subdev-formats.rst: fix incorrect types

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

The ycbcr_enc, quantization and xfer_func fields are __u16 and not enums.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/media/uapi/v4l/subdev-formats.rst | 27 +++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/Documentation/media/uapi/v4l/subdev-formats.rst b/Documentation/media/uapi/v4l/subdev-formats.rst
index 9fcabe7f9367..a3f30853f8a8 100644
--- a/Documentation/media/uapi/v4l/subdev-formats.rst
+++ b/Documentation/media/uapi/v4l/subdev-formats.rst
@@ -37,19 +37,34 @@ Media Bus Formats
       - Image colorspace, from enum
 	:c:type:`v4l2_colorspace`. See
 	:ref:`colorspaces` for details.
-    * - enum :c:type:`v4l2_ycbcr_encoding`
+    * - union {
+      - (anonymous)
+      -
+    * - __u16
       - ``ycbcr_enc``
-      - This information supplements the ``colorspace`` and must be set by
+      - Y'CbCr encoding, from enum :c:type:`v4l2_ycbcr_encoding`.
+        This information supplements the ``colorspace`` and must be set by
+	the driver for capture streams and by the application for output
+	streams, see :ref:`colorspaces`.
+    * - __u16
+      - ``hsv_enc``
+      - HSV encoding, from enum :c:type:`v4l2_hsv_encoding`.
+        This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
 	streams, see :ref:`colorspaces`.
-    * - enum :c:type:`v4l2_quantization`
+    * - }
+      -
+      -
+    * - __u16
       - ``quantization``
-      - This information supplements the ``colorspace`` and must be set by
+      - Quantization range, from enum :c:type:`v4l2_quantization`.
+        This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
 	streams, see :ref:`colorspaces`.
-    * - enum :c:type:`v4l2_xfer_func`
+    * - __u16
       - ``xfer_func``
-      - This information supplements the ``colorspace`` and must be set by
+      - Transfer function, from enum :c:type:`v4l2_xfer_func`.
+        This information supplements the ``colorspace`` and must be set by
 	the driver for capture streams and by the application for output
 	streams, see :ref:`colorspaces`.
     * - __u16
-- 
2.15.1

[PATCHv2 3/9] media.h: remove __NEED_MEDIA_LEGACY_API

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

The __NEED_MEDIA_LEGACY_API define is 1) ugly and 2) dangerous
since it is all too easy for drivers to define it to get hold of
legacy defines. Instead just define what we need in media-device.c
which is the only place where we need the legacy define
(MEDIA_ENT_T_DEVNODE_UNKNOWN).

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 drivers/media/media-device.c | 13 ++++++++++---
 include/uapi/linux/media.h   |  2 +-
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 35e81f7c0d2f..7c3ab37c258a 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -16,9 +16,6 @@
  * GNU General Public License for more details.
  */
 
-/* We need to access legacy defines from linux/media.h */
-#define __NEED_MEDIA_LEGACY_API
-
 #include <linux/compat.h>
 #include <linux/export.h>
 #include <linux/idr.h>
@@ -35,6 +32,16 @@
 
 #ifdef CONFIG_MEDIA_CONTROLLER
 
+/*
+ * Legacy defines from linux/media.h. This is the only place we need this
+ * so we just define it here. The media.h header doesn't expose it to the
+ * kernel to prevent it from being used by drivers, but here (and only here!)
+ * we need it to handle the legacy behavior.
+ */
+#define MEDIA_ENT_SUBTYPE_MASK			0x0000ffff
+#define MEDIA_ENT_T_DEVNODE_UNKNOWN		(MEDIA_ENT_F_OLD_BASE | \
+						 MEDIA_ENT_SUBTYPE_MASK)
+
 /* -----------------------------------------------------------------------------
  * Userspace API
  */
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index c7e9a5cba24e..86c7dcc9cba3 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -348,7 +348,7 @@ struct media_v2_topology {
 #define MEDIA_IOC_SETUP_LINK	_IOWR('|', 0x03, struct media_link_desc)
 #define MEDIA_IOC_G_TOPOLOGY	_IOWR('|', 0x04, struct media_v2_topology)
 
-#if !defined(__KERNEL__) || defined(__NEED_MEDIA_LEGACY_API)
+#ifndef __KERNEL__
 
 /*
  * Legacy symbols used to avoid userspace compilation breakages.
-- 
2.15.1

[PATCHv2 4/9] media: add function field to struct media_entity_desc

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

This adds support for 'proper' functions to the existing API.
This information was before only available through the new v2
API, with this change it's available to both.

Yes, the plan is to allow entities to expose multiple functions for
multi-function devices, but we do not support it anywhere so this
is still vaporware.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 drivers/media/media-device.c | 1 +
 include/uapi/linux/media.h   | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 7c3ab37c258a..dca1e5a3e0f9 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -115,6 +115,7 @@ static long media_device_enum_entities(struct media_device *mdev,
 	if (ent->name)
 		strlcpy(entd->name, ent->name, sizeof(entd->name));
 	entd->type = ent->function;
+	entd->function = ent->function;
 	entd->revision = 0;		/* Unused */
 	entd->flags = ent->flags;
 	entd->group_id = 0;		/* Unused */
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 86c7dcc9cba3..ac08acffdb65 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -146,6 +146,10 @@ struct media_device_info {
 /* OR with the entity id value to find the next entity */
 #define MEDIA_ENT_ID_FLAG_NEXT			(1 << 31)
 
+/* Appeared in 4.18.0 */
+#define MEDIA_ENTITY_DESC_HAS_FUNCTION(media_version) \
+	((media_version) >= 0x00041200)
+
 struct media_entity_desc {
 	__u32 id;
 	char name[32];
@@ -155,8 +159,9 @@ struct media_entity_desc {
 	__u32 group_id;
 	__u16 pads;
 	__u16 links;
+	__u32 function;
 
-	__u32 reserved[4];
+	__u32 reserved[3];
 
 	union {
 		/* Node specifications */
-- 
2.15.1

[PATCHv2 5/9] media-ioc-enum-entities.rst: document new 'function' field

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Document the new struct media_entity_desc 'function' field.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 .../uapi/mediactl/media-ioc-enum-entities.rst      | 31 +++++++++++++++++-----
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst b/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst
index 582fda488810..6b0ab65288c6 100644
--- a/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst
+++ b/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst
@@ -90,6 +90,12 @@ id's until they get an error.
        -
        -
        -  Entity type, see :ref:`media-entity-functions` for details.
+          Deprecated. If possible, use the ``function`` field instead.
+	  For backwards compatibility this ``type`` field will only
+	  expose functions ``MEDIA_ENT_F_IO_V4L``, ``MEDIA_ENT_F_CAM_SENSOR``,
+	  ``MEDIA_ENT_F_FLASH``, ``MEDIA_ENT_F_LENS``, ``MEDIA_ENT_F_ATV_DECODER``
+	  and ``MEDIA_ENT_F_TUNER``. Other functions will be mapped to
+	  ``MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN`` or ``MEDIA_ENT_T_DEVNODE_UNKNOWN``.
 
     -  .. row 4
 
@@ -146,18 +152,31 @@ id's until they get an error.
 
        -  __u32
 
-       -  ``reserved[4]``
+       -  ``function``
+
+       -
+       -
+       -  Entity main function, see :ref:`media-entity-functions` for details.
+          Only valid if ``MEDIA_ENTITY_DESC_HAS_FUNCTION(media_version)``
+          returns true. The ``media_version`` is defined in struct
+          :c:type:`media_device_info`.
+
+    -  .. row 10
+
+       -  __u32
+
+       -  ``reserved[3]``
 
        -
        -
        -  Reserved for future extensions. Drivers and applications must set
           the array to zero.
 
-    -  .. row 10
+    -  .. row 11
 
        -  union
 
-    -  .. row 11
+    -  .. row 12
 
        -
        -  struct
@@ -167,7 +186,7 @@ id's until they get an error.
        -
        -  Valid for (sub-)devices that create a single device node.
 
-    -  .. row 12
+    -  .. row 13
 
        -
        -
@@ -177,7 +196,7 @@ id's until they get an error.
 
        -  Device node major number.
 
-    -  .. row 13
+    -  .. row 14
 
        -
        -
@@ -187,7 +206,7 @@ id's until they get an error.
 
        -  Device node minor number.
 
-    -  .. row 14
+    -  .. row 15
 
        -
        -  __u8
-- 
2.15.1

[PATCHv2 6/9] media: add 'index' to struct media_v2_pad

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

The v2 pad structure never exposed the pad index, which made it impossible
to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.

It is really trivial to just expose this information, so implement this.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 drivers/media/media-device.c | 1 +
 include/uapi/linux/media.h   | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index dca1e5a3e0f9..73ffea3e81c9 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -331,6 +331,7 @@ static long media_device_get_topology(struct media_device *mdev,
 		kpad.id = pad->graph_obj.id;
 		kpad.entity_id = pad->entity->graph_obj.id;
 		kpad.flags = pad->flags;
+		kpad.index = pad->index;
 
 		if (copy_to_user(upad, &kpad, sizeof(kpad)))
 			ret = -EFAULT;
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index ac08acffdb65..15f7f432f808 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -310,11 +310,16 @@ struct media_v2_interface {
 	};
 } __attribute__ ((packed));
 
+/* Appeared in 4.18.0 */
+#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
+	((media_version) >= 0x00041200)
+
 struct media_v2_pad {
 	__u32 id;
 	__u32 entity_id;
 	__u32 flags;
-	__u32 reserved[5];
+	__u32 index;
+	__u32 reserved[4];
 } __attribute__ ((packed));
 
 struct media_v2_link {
-- 
2.15.1

[PATCHv2 7/9] media-ioc-g-topology.rst: document new 'index' field

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Document the new struct media_v2_pad 'index' field.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/media/uapi/mediactl/media-ioc-g-topology.rst | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
index c4055ddf070a..459818c3490c 100644
--- a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
+++ b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
@@ -334,7 +334,17 @@ desired arrays with the media graph elements.
 
        -  __u32
 
-       -  ``reserved``\ [5]
+       -  ``index``
+
+       -  0-based pad index. Only valid if ``MEDIA_V2_PAD_HAS_INDEX(media_version)``
+          returns true. The ``media_version`` is defined in struct
+	  :c:type:`media_device_info`.
+
+    -  .. row 5
+
+       -  __u32
+
+       -  ``reserved``\ [4]
 
        -  Reserved for future extensions. Drivers and applications must set
 	  this array to zero.
-- 
2.15.1

[PATCHv2 8/9] media: add flags field to struct media_v2_entity

[Hans Verkuil]

From: Hans Verkuil <hansverk@cisco.com>

The v2 entity structure never exposed the entity flags, which made it
impossible to detect connector or default entities.

It is really trivial to just expose this information, so implement this.

Signed-off-by: Hans Verkuil <hansverk@cisco.com>
---
 drivers/media/media-device.c | 1 +
 include/uapi/linux/media.h   | 7 ++++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 73ffea3e81c9..00f7ce74e42a 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -266,6 +266,7 @@ static long media_device_get_topology(struct media_device *mdev,
 		memset(&kentity, 0, sizeof(kentity));
 		kentity.id = entity->graph_obj.id;
 		kentity.function = entity->function;
+		kentity.flags = entity->flags;
 		strlcpy(kentity.name, entity->name,
 			sizeof(kentity.name));
 
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 15f7f432f808..056100ef25cc 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -285,11 +285,16 @@ struct media_links_enum {
  * MC next gen API definitions
  */
 
+/* Appeared in 4.18.0 */
+#define MEDIA_V2_ENTITY_HAS_FLAGS(media_version) \
+	((media_version) >= 0x00041200)
+
 struct media_v2_entity {
 	__u32 id;
 	char name[64];
 	__u32 function;		/* Main function of the entity */
-	__u32 reserved[6];
+	__u32 flags;
+	__u32 reserved[5];
 } __attribute__ ((packed));
 
 /* Should match the specific fields at media_intf_devnode */
-- 
2.15.1

[PATCHv2 9/9] media-ioc-g-topology.rst: document new 'flags' field

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Document the new struct media_v2_entity 'flags' field.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 Documentation/media/uapi/mediactl/media-ioc-g-topology.rst | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
index 459818c3490c..6521ab7c9b58 100644
--- a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
+++ b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
@@ -211,7 +211,18 @@ desired arrays with the media graph elements.
 
        -  __u32
 
-       -  ``reserved``\ [6]
+       -  ``flags``
+
+       -  Entity flags, see :ref:`media-entity-flag` for details.
+          Only valid if ``MEDIA_V2_ENTITY_HAS_FLAGS(media_version)``
+          returns true. The ``media_version`` is defined in struct
+	  :c:type:`media_device_info`.
+
+    -  .. row 5
+
+       -  __u32
+
+       -  ``reserved``\ [5]
 
        -  Reserved for future extensions. Drivers and applications must set
 	  this array to zero.
-- 
2.15.1

Re: [PATCHv2 3/9] media.h: remove __NEED_MEDIA_LEGACY_API

[Mauro Carvalho Chehab]

Em Mon, 16 Apr 2018 15:21:15 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> From: Hans Verkuil <hansverk@cisco.com>
> 
> The __NEED_MEDIA_LEGACY_API define is 1) ugly and 2) dangerous
> since it is all too easy for drivers to define it to get hold of
> legacy defines. Instead just define what we need in media-device.c
> which is the only place where we need the legacy define
> (MEDIA_ENT_T_DEVNODE_UNKNOWN).
> 
> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> ---
>  drivers/media/media-device.c | 13 ++++++++++---
>  include/uapi/linux/media.h   |  2 +-
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 35e81f7c0d2f..7c3ab37c258a 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -16,9 +16,6 @@
>   * GNU General Public License for more details.
>   */
>  
> -/* We need to access legacy defines from linux/media.h */
> -#define __NEED_MEDIA_LEGACY_API
> -
>  #include <linux/compat.h>
>  #include <linux/export.h>
>  #include <linux/idr.h>
> @@ -35,6 +32,16 @@
>  
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  
> +/*
> + * Legacy defines from linux/media.h. This is the only place we need this
> + * so we just define it here. The media.h header doesn't expose it to the
> + * kernel to prevent it from being used by drivers, but here (and only here!)
> + * we need it to handle the legacy behavior.
> + */
> +#define MEDIA_ENT_SUBTYPE_MASK			0x0000ffff
> +#define MEDIA_ENT_T_DEVNODE_UNKNOWN		(MEDIA_ENT_F_OLD_BASE | \
> +						 MEDIA_ENT_SUBTYPE_MASK)

I don't like much the idea of duplicating defines at C code, but, 
in this specific case, I agree that this is better.

Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

> +
>  /* -----------------------------------------------------------------------------
>   * Userspace API
>   */
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index c7e9a5cba24e..86c7dcc9cba3 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -348,7 +348,7 @@ struct media_v2_topology {
>  #define MEDIA_IOC_SETUP_LINK	_IOWR('|', 0x03, struct media_link_desc)
>  #define MEDIA_IOC_G_TOPOLOGY	_IOWR('|', 0x04, struct media_v2_topology)
>  
> -#if !defined(__KERNEL__) || defined(__NEED_MEDIA_LEGACY_API)
> +#ifndef __KERNEL__
>  
>  /*
>   * Legacy symbols used to avoid userspace compilation breakages.



Thanks,
Mauro

Re: [PATCHv2 4/9] media: add function field to struct media_entity_desc

[Mauro Carvalho Chehab]

Em Mon, 16 Apr 2018 15:21:16 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> From: Hans Verkuil <hansverk@cisco.com>
> 
> This adds support for 'proper' functions to the existing API.
> This information was before only available through the new v2
> API, with this change it's available to both.
> 
> Yes, the plan is to allow entities to expose multiple functions for
> multi-function devices, but we do not support it anywhere so this
> is still vaporware.

I'm not convinced about that. I would, instead, just keep it as-is
and be sure that applications stop use the legacy calls.

> 
> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> ---
>  drivers/media/media-device.c | 1 +
>  include/uapi/linux/media.h   | 7 ++++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 7c3ab37c258a..dca1e5a3e0f9 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -115,6 +115,7 @@ static long media_device_enum_entities(struct media_device *mdev,
>  	if (ent->name)
>  		strlcpy(entd->name, ent->name, sizeof(entd->name));
>  	entd->type = ent->function;
> +	entd->function = ent->function;
>  	entd->revision = 0;		/* Unused */

I got confused here, until I went to the code and noticed that
entd->type is actually touched after this.

If we're willing to do that, you should add a comment there explaining
why we need to pass both type and function to userspace.

>  	entd->flags = ent->flags;
>  	entd->group_id = 0;		/* Unused */
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 86c7dcc9cba3..ac08acffdb65 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -146,6 +146,10 @@ struct media_device_info {
>  /* OR with the entity id value to find the next entity */
>  #define MEDIA_ENT_ID_FLAG_NEXT			(1 << 31)
>  
> +/* Appeared in 4.18.0 */
> +#define MEDIA_ENTITY_DESC_HAS_FUNCTION(media_version) \
> +	((media_version) >= 0x00041200)
> +
>  struct media_entity_desc {
>  	__u32 id;
>  	char name[32];
> @@ -155,8 +159,9 @@ struct media_entity_desc {
>  	__u32 group_id;
>  	__u16 pads;
>  	__u16 links;
> +	__u32 function;
>  
> -	__u32 reserved[4];
> +	__u32 reserved[3];
>  
>  	union {
>  		/* Node specifications */



Thanks,
Mauro

Re: [PATCHv2 5/9] media-ioc-enum-entities.rst: document new 'function' field

[Mauro Carvalho Chehab]

Em Mon, 16 Apr 2018 15:21:17 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Document the new struct media_entity_desc 'function' field.

See my comments to patch 4/9.

> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  .../uapi/mediactl/media-ioc-enum-entities.rst      | 31 +++++++++++++++++-----
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst b/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst
> index 582fda488810..6b0ab65288c6 100644
> --- a/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst
> +++ b/Documentation/media/uapi/mediactl/media-ioc-enum-entities.rst
> @@ -90,6 +90,12 @@ id's until they get an error.
>         -
>         -
>         -  Entity type, see :ref:`media-entity-functions` for details.
> +          Deprecated. If possible, use the ``function`` field instead.
> +	  For backwards compatibility this ``type`` field will only
> +	  expose functions ``MEDIA_ENT_F_IO_V4L``, ``MEDIA_ENT_F_CAM_SENSOR``,
> +	  ``MEDIA_ENT_F_FLASH``, ``MEDIA_ENT_F_LENS``, ``MEDIA_ENT_F_ATV_DECODER``
> +	  and ``MEDIA_ENT_F_TUNER``. Other functions will be mapped to
> +	  ``MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN`` or ``MEDIA_ENT_T_DEVNODE_UNKNOWN``.
>  
>      -  .. row 4
>  
> @@ -146,18 +152,31 @@ id's until they get an error.
>  
>         -  __u32
>  
> -       -  ``reserved[4]``
> +       -  ``function``
> +
> +       -
> +       -
> +       -  Entity main function, see :ref:`media-entity-functions` for details.
> +          Only valid if ``MEDIA_ENTITY_DESC_HAS_FUNCTION(media_version)``
> +          returns true. The ``media_version`` is defined in struct
> +          :c:type:`media_device_info`.
> +
> +    -  .. row 10
> +
> +       -  __u32
> +
> +       -  ``reserved[3]``
>  
>         -
>         -
>         -  Reserved for future extensions. Drivers and applications must set
>            the array to zero.
>  
> -    -  .. row 10
> +    -  .. row 11

Instead of keep incrementing the ".. row \d+" comment, better to
just remove them all from this rst file on a previous patch.

>  
>         -  union
>  
> -    -  .. row 11
> +    -  .. row 12
>  
>         -
>         -  struct
> @@ -167,7 +186,7 @@ id's until they get an error.
>         -
>         -  Valid for (sub-)devices that create a single device node.
>  
> -    -  .. row 12
> +    -  .. row 13
>  
>         -
>         -
> @@ -177,7 +196,7 @@ id's until they get an error.
>  
>         -  Device node major number.
>  
> -    -  .. row 13
> +    -  .. row 14
>  
>         -
>         -
> @@ -187,7 +206,7 @@ id's until they get an error.
>  
>         -  Device node minor number.
>  
> -    -  .. row 14
> +    -  .. row 15
>  
>         -
>         -  __u8



Thanks,
Mauro

Re: [PATCHv2 6/9] media: add 'index' to struct media_v2_pad

[Mauro Carvalho Chehab]

Em Mon, 16 Apr 2018 15:21:18 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> From: Hans Verkuil <hansverk@cisco.com>
> 
> The v2 pad structure never exposed the pad index, which made it impossible
> to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.
> 
> It is really trivial to just expose this information, so implement this.

Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> 
> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> ---
>  drivers/media/media-device.c | 1 +
>  include/uapi/linux/media.h   | 7 ++++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index dca1e5a3e0f9..73ffea3e81c9 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -331,6 +331,7 @@ static long media_device_get_topology(struct media_device *mdev,
>  		kpad.id = pad->graph_obj.id;
>  		kpad.entity_id = pad->entity->graph_obj.id;
>  		kpad.flags = pad->flags;
> +		kpad.index = pad->index;
>  
>  		if (copy_to_user(upad, &kpad, sizeof(kpad)))
>  			ret = -EFAULT;
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index ac08acffdb65..15f7f432f808 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -310,11 +310,16 @@ struct media_v2_interface {
>  	};
>  } __attribute__ ((packed));
>  
> +/* Appeared in 4.18.0 */
> +#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
> +	((media_version) >= 0x00041200)
> +
>  struct media_v2_pad {
>  	__u32 id;
>  	__u32 entity_id;
>  	__u32 flags;
> -	__u32 reserved[5];
> +	__u32 index;
> +	__u32 reserved[4];
>  } __attribute__ ((packed));
>  
>  struct media_v2_link {



Thanks,
Mauro

Re: [PATCHv2 7/9] media-ioc-g-topology.rst: document new 'index' field

[Mauro Carvalho Chehab]

Em Mon, 16 Apr 2018 15:21:19 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Document the new struct media_v2_pad 'index' field.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  Documentation/media/uapi/mediactl/media-ioc-g-topology.rst | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
> index c4055ddf070a..459818c3490c 100644
> --- a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
> +++ b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
> @@ -334,7 +334,17 @@ desired arrays with the media graph elements.
>  
>         -  __u32
>  
> -       -  ``reserved``\ [5]
> +       -  ``index``
> +
> +       -  0-based pad index. Only valid if ``MEDIA_V2_PAD_HAS_INDEX(media_version)``
> +          returns true. The ``media_version`` is defined in struct
> +	  :c:type:`media_device_info`.
> +
> +    -  .. row 5

Same comment as before: let's have just one patch removing those
script-generated comments from the doc files we touch.

> +
> +       -  __u32
> +
> +       -  ``reserved``\ [4]
>  
>         -  Reserved for future extensions. Drivers and applications must set
>  	  this array to zero.



Thanks,
Mauro

Re: [PATCHv2 6/9] media: add 'index' to struct media_v2_pad

[Mauro Carvalho Chehab]

Em Mon, 16 Apr 2018 15:03:35 -0300
Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:

> Em Mon, 16 Apr 2018 15:21:18 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
> > From: Hans Verkuil <hansverk@cisco.com>
> > 
> > The v2 pad structure never exposed the pad index, which made it impossible
> > to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.
> > 
> > It is really trivial to just expose this information, so implement this.  
> 
> Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>

Err... I looked on it too fast... See my comments below.

The same applies to patch 8/9.

> > 
> > Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> > ---
> >  drivers/media/media-device.c | 1 +
> >  include/uapi/linux/media.h   | 7 ++++++-
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> > index dca1e5a3e0f9..73ffea3e81c9 100644
> > --- a/drivers/media/media-device.c
> > +++ b/drivers/media/media-device.c
> > @@ -331,6 +331,7 @@ static long media_device_get_topology(struct media_device *mdev,
> >  		kpad.id = pad->graph_obj.id;
> >  		kpad.entity_id = pad->entity->graph_obj.id;
> >  		kpad.flags = pad->flags;
> > +		kpad.index = pad->index;
> >  
> >  		if (copy_to_user(upad, &kpad, sizeof(kpad)))
> >  			ret = -EFAULT;
> > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> > index ac08acffdb65..15f7f432f808 100644
> > --- a/include/uapi/linux/media.h
> > +++ b/include/uapi/linux/media.h
> > @@ -310,11 +310,16 @@ struct media_v2_interface {
> >  	};
> >  } __attribute__ ((packed));
> >  
> > +/* Appeared in 4.18.0 */
> > +#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
> > +	((media_version) >= 0x00041200)
> > +

I don't like this, for a couple of reasons:

1) it has a magic number on it, with is actually a parsed
   version of LINUX_VERSION() macro;

2) it sounds really weird to ship a header file with a new
   kernel version meant to provide backward compatibility with
   older versions;

3) this isn't any different than:

	#define MEDIA_V2_PAD_HAS_INDEX -1

I think we need to think a little bit more about that.


> >  struct media_v2_pad {
> >  	__u32 id;
> >  	__u32 entity_id;
> >  	__u32 flags;
> > -	__u32 reserved[5];
> > +	__u32 index;
> > +	__u32 reserved[4];
> >  } __attribute__ ((packed));
> >  
> >  struct media_v2_link {  
> 
> 
> 
> Thanks,
> Mauro



Thanks,
Mauro

Re: [PATCHv2 9/9] media-ioc-g-topology.rst: document new 'flags' field

[Mauro Carvalho Chehab]

Em Mon, 16 Apr 2018 15:21:21 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Document the new struct media_v2_entity 'flags' field.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  Documentation/media/uapi/mediactl/media-ioc-g-topology.rst | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
> index 459818c3490c..6521ab7c9b58 100644
> --- a/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
> +++ b/Documentation/media/uapi/mediactl/media-ioc-g-topology.rst
> @@ -211,7 +211,18 @@ desired arrays with the media graph elements.
>  
>         -  __u32
>  
> -       -  ``reserved``\ [6]
> +       -  ``flags``
> +
> +       -  Entity flags, see :ref:`media-entity-flag` for details.
> +          Only valid if ``MEDIA_V2_ENTITY_HAS_FLAGS(media_version)``
> +          returns true. The ``media_version`` is defined in struct
> +	  :c:type:`media_device_info`.
> +
> +    -  .. row 5

Same comment as before: let's not add new useless comments.

> +
> +       -  __u32
> +
> +       -  ``reserved``\ [5]
>  
>         -  Reserved for future extensions. Drivers and applications must set
>  	  this array to zero.



Thanks,
Mauro

Re: [PATCHv2 1/9] v4l2-mediabus.h: add hsv_enc

[Mauro Carvalho Chehab]

Em Mon, 16 Apr 2018 15:21:13 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Just like struct v4l2_pix_format add a hsv_enc field to describe
> the HSV encoding. It is in a union with the ycbcr_enc, since it
> is one or the other.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  include/uapi/linux/v4l2-mediabus.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/v4l2-mediabus.h b/include/uapi/linux/v4l2-mediabus.h
> index 123a231001a8..52fd6cc9d491 100644
> --- a/include/uapi/linux/v4l2-mediabus.h
> +++ b/include/uapi/linux/v4l2-mediabus.h
> @@ -24,6 +24,7 @@
>   * @field:	used interlacing type (from enum v4l2_field)
>   * @colorspace:	colorspace of the data (from enum v4l2_colorspace)
>   * @ycbcr_enc:	YCbCr encoding of the data (from enum v4l2_ycbcr_encoding)
> + * @hsv_enc:	HSV encoding of the data (from enum v4l2_hsv_encoding)
>   * @quantization: quantization of the data (from enum v4l2_quantization)
>   * @xfer_func:  transfer function of the data (from enum v4l2_xfer_func)
>   */
> @@ -33,7 +34,12 @@ struct v4l2_mbus_framefmt {
>  	__u32			code;
>  	__u32			field;
>  	__u32			colorspace;
> -	__u16			ycbcr_enc;
> +	union {
> +		/* enum v4l2_ycbcr_encoding */
> +		__u16		ycbcr_enc;
> +		/* enum v4l2_hsv_encoding */
> +		__u16		hsv_enc;
> +	};

While I'm OK with that, it currently doesn't make sense to apply
it, as no driver is currently using v4l2_mbus_framefmt.hsv_enc.

>  	__u16			quantization;
>  	__u16			xfer_func;
>  	__u16			reserved[11];



Thanks,
Mauro

Re: [PATCHv2 4/9] media: add function field to struct media_entity_desc

[Hans Verkuil]

On 04/16/2018 08:01 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 16 Apr 2018 15:21:16 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> From: Hans Verkuil <hansverk@cisco.com>
>>
>> This adds support for 'proper' functions to the existing API.
>> This information was before only available through the new v2
>> API, with this change it's available to both.
>>
>> Yes, the plan is to allow entities to expose multiple functions for
>> multi-function devices, but we do not support it anywhere so this
>> is still vaporware.
> 
> I'm not convinced about that. I would, instead, just keep it as-is
> and be sure that applications stop use the legacy calls.

You can't. First of all, since the new API does not provide the pad index
(fixed in patch 6/9) it is impossible to use the new API with any driver
that supports SETUP_LINK. So any such driver that uses any of the newer
subdevs with a function that is mapped to MEDIA_ENT_T_DEVNODE_UNKNOWN
is currently not reporting that correctly. A good example is the
imx driver. But also others if they are combined with such newer subdevs.

There is nothing wrong with the old API, except for not reporting the
proper function value in field 'type' due to historical concerns.

There is NO WAY we can suddenly prohibit applications from using the old
API since the new API was never usable. And besides that, we have no method
of knowing who uses the old API since such applications are likely custom
for specific hardware.

All that is really missing in the 'old' API (I hate the terms 'old' and
'new', they are misleading) is a proper 'function' field. Let's just add it
and make it consistent with the documentation about entity functions.

> 
>>
>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>> ---
>>  drivers/media/media-device.c | 1 +
>>  include/uapi/linux/media.h   | 7 ++++++-
>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index 7c3ab37c258a..dca1e5a3e0f9 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -115,6 +115,7 @@ static long media_device_enum_entities(struct media_device *mdev,
>>  	if (ent->name)
>>  		strlcpy(entd->name, ent->name, sizeof(entd->name));
>>  	entd->type = ent->function;
>> +	entd->function = ent->function;
>>  	entd->revision = 0;		/* Unused */
> 
> I got confused here, until I went to the code and noticed that
> entd->type is actually touched after this.
> 
> If we're willing to do that, you should add a comment there explaining
> why we need to pass both type and function to userspace.

True.

Regards,

	Hans

> 
>>  	entd->flags = ent->flags;
>>  	entd->group_id = 0;		/* Unused */
>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>> index 86c7dcc9cba3..ac08acffdb65 100644
>> --- a/include/uapi/linux/media.h
>> +++ b/include/uapi/linux/media.h
>> @@ -146,6 +146,10 @@ struct media_device_info {
>>  /* OR with the entity id value to find the next entity */
>>  #define MEDIA_ENT_ID_FLAG_NEXT			(1 << 31)
>>  
>> +/* Appeared in 4.18.0 */
>> +#define MEDIA_ENTITY_DESC_HAS_FUNCTION(media_version) \
>> +	((media_version) >= 0x00041200)
>> +
>>  struct media_entity_desc {
>>  	__u32 id;
>>  	char name[32];
>> @@ -155,8 +159,9 @@ struct media_entity_desc {
>>  	__u32 group_id;
>>  	__u16 pads;
>>  	__u16 links;
>> +	__u32 function;
>>  
>> -	__u32 reserved[4];
>> +	__u32 reserved[3];
>>  
>>  	union {
>>  		/* Node specifications */
> 
> 
> 
> Thanks,
> Mauro
> 

Re: [PATCHv2 4/9] media: add function field to struct media_entity_desc

[Mauro Carvalho Chehab]

Em Mon, 16 Apr 2018 21:27:01 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 04/16/2018 08:01 PM, Mauro Carvalho Chehab wrote:
> > Em Mon, 16 Apr 2018 15:21:16 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >   
> >> From: Hans Verkuil <hansverk@cisco.com>
> >>
> >> This adds support for 'proper' functions to the existing API.
> >> This information was before only available through the new v2
> >> API, with this change it's available to both.
> >>
> >> Yes, the plan is to allow entities to expose multiple functions for
> >> multi-function devices, but we do not support it anywhere so this
> >> is still vaporware.  
> > 
> > I'm not convinced about that. I would, instead, just keep it as-is
> > and be sure that applications stop use the legacy calls.  
> 
> You can't. First of all, since the new API does not provide the pad index
> (fixed in patch 6/9) it is impossible to use the new API with any driver
> that supports SETUP_LINK.

Yeah, unfortunately, the properties API was just an empty promise.

Anyway, as you said, patch 6/9 solves it.

> So any such driver that uses any of the newer
> subdevs with a function that is mapped to MEDIA_ENT_T_DEVNODE_UNKNOWN
> is currently not reporting that correctly. A good example is the
> imx driver. But also others if they are combined with such newer subdevs.

As far as I remember, other drivers also return MEDIA_ENT_F_UNKNOWN
(with also maps to MEDIA_ENT_T_DEVNODE_UNKNOWN) even via the new API, 
as the developer never cared to fill the entity function, even 
producing warnings.

> There is nothing wrong with the old API, except for not reporting the
> proper function value in field 'type' due to historical concerns.

There is. That's why we took about one year developing a new API.

> There is NO WAY we can suddenly prohibit applications from using the old
> API since the new API was never usable. And besides that, we have no method
> of knowing who uses the old API since such applications are likely custom
> for specific hardware.

Nobody is forbidding anything. We're just freezing it, as its
functionality was superseded.

> All that is really missing in the 'old' API (I hate the terms 'old' and
> 'new', they are misleading) is a proper 'function' field. Let's just add it
> and make it consistent with the documentation about entity functions.

It misses interfaces - with is needed to identify what interface controls
what.

> 
> >   
> >>
> >> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> >> ---
> >>  drivers/media/media-device.c | 1 +
> >>  include/uapi/linux/media.h   | 7 ++++++-
> >>  2 files changed, 7 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >> index 7c3ab37c258a..dca1e5a3e0f9 100644
> >> --- a/drivers/media/media-device.c
> >> +++ b/drivers/media/media-device.c
> >> @@ -115,6 +115,7 @@ static long media_device_enum_entities(struct media_device *mdev,
> >>  	if (ent->name)
> >>  		strlcpy(entd->name, ent->name, sizeof(entd->name));
> >>  	entd->type = ent->function;
> >> +	entd->function = ent->function;
> >>  	entd->revision = 0;		/* Unused */  
> > 
> > I got confused here, until I went to the code and noticed that
> > entd->type is actually touched after this.
> > 
> > If we're willing to do that, you should add a comment there explaining
> > why we need to pass both type and function to userspace.  
> 
> True.
> 
> Regards,
> 
> 	Hans
> 
> >   
> >>  	entd->flags = ent->flags;
> >>  	entd->group_id = 0;		/* Unused */
> >> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> >> index 86c7dcc9cba3..ac08acffdb65 100644
> >> --- a/include/uapi/linux/media.h
> >> +++ b/include/uapi/linux/media.h
> >> @@ -146,6 +146,10 @@ struct media_device_info {
> >>  /* OR with the entity id value to find the next entity */
> >>  #define MEDIA_ENT_ID_FLAG_NEXT			(1 << 31)
> >>  
> >> +/* Appeared in 4.18.0 */
> >> +#define MEDIA_ENTITY_DESC_HAS_FUNCTION(media_version) \
> >> +	((media_version) >= 0x00041200)
> >> +
> >>  struct media_entity_desc {
> >>  	__u32 id;
> >>  	char name[32];
> >> @@ -155,8 +159,9 @@ struct media_entity_desc {
> >>  	__u32 group_id;
> >>  	__u16 pads;
> >>  	__u16 links;
> >> +	__u32 function;
> >>  
> >> -	__u32 reserved[4];
> >> +	__u32 reserved[3];
> >>  
> >>  	union {
> >>  		/* Node specifications */  
> > 
> > 
> > 
> > Thanks,
> > Mauro
> >   
> 



Thanks,
Mauro

Re: [PATCHv2 6/9] media: add 'index' to struct media_v2_pad

[Hans Verkuil]

On 04/16/2018 08:09 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 16 Apr 2018 15:03:35 -0300
> Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:
> 
>> Em Mon, 16 Apr 2018 15:21:18 +0200
>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>
>>> From: Hans Verkuil <hansverk@cisco.com>
>>>
>>> The v2 pad structure never exposed the pad index, which made it impossible
>>> to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.
>>>
>>> It is really trivial to just expose this information, so implement this.  
>>
>> Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
> 
> Err... I looked on it too fast... See my comments below.
> 
> The same applies to patch 8/9.
> 
>>>
>>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>>> ---
>>>  drivers/media/media-device.c | 1 +
>>>  include/uapi/linux/media.h   | 7 ++++++-
>>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>> index dca1e5a3e0f9..73ffea3e81c9 100644
>>> --- a/drivers/media/media-device.c
>>> +++ b/drivers/media/media-device.c
>>> @@ -331,6 +331,7 @@ static long media_device_get_topology(struct media_device *mdev,
>>>  		kpad.id = pad->graph_obj.id;
>>>  		kpad.entity_id = pad->entity->graph_obj.id;
>>>  		kpad.flags = pad->flags;
>>> +		kpad.index = pad->index;
>>>  
>>>  		if (copy_to_user(upad, &kpad, sizeof(kpad)))
>>>  			ret = -EFAULT;
>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>>> index ac08acffdb65..15f7f432f808 100644
>>> --- a/include/uapi/linux/media.h
>>> +++ b/include/uapi/linux/media.h
>>> @@ -310,11 +310,16 @@ struct media_v2_interface {
>>>  	};
>>>  } __attribute__ ((packed));
>>>  
>>> +/* Appeared in 4.18.0 */
>>> +#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
>>> +	((media_version) >= 0x00041200)
>>> +
> 
> I don't like this, for a couple of reasons:
> 
> 1) it has a magic number on it, with is actually a parsed
>    version of LINUX_VERSION() macro;

I can/should change that to KERNEL_VERSION().

> 
> 2) it sounds really weird to ship a header file with a new
>    kernel version meant to provide backward compatibility with
>    older versions;
> 
> 3) this isn't any different than:
> 
> 	#define MEDIA_V2_PAD_HAS_INDEX -1
> 
> I think we need to think a little bit more about that.

What typically happens is that applications (like those in v4l-utils
for example) copy the headers locally. So they are compiled with the headers
of a specific kernel version, but they can run with very different kernels.

This is normal for distros where you can install different kernel versions
without needing to modify applications.

In fact, we (Cisco) use the latest v4l-utils code on kernels ranging between
2.6.39 to 4.10 (I think that's the latest one in use).

The media version tells you whether or not the kernel supports this feature.
I don't see another way of doing this.

In most other cases we can just say that if the field value is 0, then it
should not be used. Unfortunately, 0 is a valid value for the pad index, for
the entity flags and for the entity function (some drivers set it to
MEDIA_ENT_F_UNKNOWN, which has value 0). This last one is most unfortunate,
since this should never have happened and would have been detected if we had
proper compliance tools.

Regards,

	Hans

> 
> 
>>>  struct media_v2_pad {
>>>  	__u32 id;
>>>  	__u32 entity_id;
>>>  	__u32 flags;
>>> -	__u32 reserved[5];
>>> +	__u32 index;
>>> +	__u32 reserved[4];
>>>  } __attribute__ ((packed));
>>>  
>>>  struct media_v2_link {  
>>
>>
>>
>> Thanks,
>> Mauro
> 
> 
> 
> Thanks,
> Mauro
> 

Re: [PATCHv2 4/9] media: add function field to struct media_entity_desc

[Hans Verkuil]

On 04/16/2018 09:40 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 16 Apr 2018 21:27:01 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 04/16/2018 08:01 PM, Mauro Carvalho Chehab wrote:
>>> Em Mon, 16 Apr 2018 15:21:16 +0200
>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>   
>>>> From: Hans Verkuil <hansverk@cisco.com>
>>>>
>>>> This adds support for 'proper' functions to the existing API.
>>>> This information was before only available through the new v2
>>>> API, with this change it's available to both.
>>>>
>>>> Yes, the plan is to allow entities to expose multiple functions for
>>>> multi-function devices, but we do not support it anywhere so this
>>>> is still vaporware.  
>>>
>>> I'm not convinced about that. I would, instead, just keep it as-is
>>> and be sure that applications stop use the legacy calls.  
>>
>> You can't. First of all, since the new API does not provide the pad index
>> (fixed in patch 6/9) it is impossible to use the new API with any driver
>> that supports SETUP_LINK.
> 
> Yeah, unfortunately, the properties API was just an empty promise.
> 
> Anyway, as you said, patch 6/9 solves it.
> 
>> So any such driver that uses any of the newer
>> subdevs with a function that is mapped to MEDIA_ENT_T_DEVNODE_UNKNOWN
>> is currently not reporting that correctly. A good example is the
>> imx driver. But also others if they are combined with such newer subdevs.
> 
> As far as I remember, other drivers also return MEDIA_ENT_F_UNKNOWN
> (with also maps to MEDIA_ENT_T_DEVNODE_UNKNOWN) even via the new API, 
> as the developer never cared to fill the entity function, even 
> producing warnings.
> 
>> There is nothing wrong with the old API, except for not reporting the
>> proper function value in field 'type' due to historical concerns.
> 
> There is. That's why we took about one year developing a new API.

If you don't need the new functionality (like interfaces), then it is
perfectly fine. It's been in use for many years now.

> 
>> There is NO WAY we can suddenly prohibit applications from using the old
>> API since the new API was never usable. And besides that, we have no method
>> of knowing who uses the old API since such applications are likely custom
>> for specific hardware.
> 
> Nobody is forbidding anything. We're just freezing it, as its
> functionality was superseded.
> 
>> All that is really missing in the 'old' API (I hate the terms 'old' and
>> 'new', they are misleading) is a proper 'function' field. Let's just add it
>> and make it consistent with the documentation about entity functions.
> 
> It misses interfaces - with is needed to identify what interface controls
> what.

Sure, but for most use cases interfaces are not needed. But reporting the correct
function is very useful, makes the API consistent with the documentation (which
only talks about functions and no longer refers to types) and the new API and it
is trivial to add.

I'm not advocating any further chances, but while writing the compliance tests
for this it was incredibly ugly to have this mismatch between 'type' and 'function'.

The function of an entity is a critical piece of information, and having it
clamped to UNKNOWN for the newer functions is just wrong.

Regards,

	Hans

> 
>>
>>>   
>>>>
>>>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>>>> ---
>>>>  drivers/media/media-device.c | 1 +
>>>>  include/uapi/linux/media.h   | 7 ++++++-
>>>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>>> index 7c3ab37c258a..dca1e5a3e0f9 100644
>>>> --- a/drivers/media/media-device.c
>>>> +++ b/drivers/media/media-device.c
>>>> @@ -115,6 +115,7 @@ static long media_device_enum_entities(struct media_device *mdev,
>>>>  	if (ent->name)
>>>>  		strlcpy(entd->name, ent->name, sizeof(entd->name));
>>>>  	entd->type = ent->function;
>>>> +	entd->function = ent->function;
>>>>  	entd->revision = 0;		/* Unused */  
>>>
>>> I got confused here, until I went to the code and noticed that
>>> entd->type is actually touched after this.
>>>
>>> If we're willing to do that, you should add a comment there explaining
>>> why we need to pass both type and function to userspace.  
>>
>> True.
>>
>> Regards,
>>
>> 	Hans
>>
>>>   
>>>>  	entd->flags = ent->flags;
>>>>  	entd->group_id = 0;		/* Unused */
>>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>>>> index 86c7dcc9cba3..ac08acffdb65 100644
>>>> --- a/include/uapi/linux/media.h
>>>> +++ b/include/uapi/linux/media.h
>>>> @@ -146,6 +146,10 @@ struct media_device_info {
>>>>  /* OR with the entity id value to find the next entity */
>>>>  #define MEDIA_ENT_ID_FLAG_NEXT			(1 << 31)
>>>>  
>>>> +/* Appeared in 4.18.0 */
>>>> +#define MEDIA_ENTITY_DESC_HAS_FUNCTION(media_version) \
>>>> +	((media_version) >= 0x00041200)
>>>> +
>>>>  struct media_entity_desc {
>>>>  	__u32 id;
>>>>  	char name[32];
>>>> @@ -155,8 +159,9 @@ struct media_entity_desc {
>>>>  	__u32 group_id;
>>>>  	__u16 pads;
>>>>  	__u16 links;
>>>> +	__u32 function;
>>>>  
>>>> -	__u32 reserved[4];
>>>> +	__u32 reserved[3];
>>>>  
>>>>  	union {
>>>>  		/* Node specifications */  
>>>
>>>
>>>
>>> Thanks,
>>> Mauro
>>>   
>>
> 
> 
> 
> Thanks,
> Mauro
> 

Re: [PATCHv2 6/9] media: add 'index' to struct media_v2_pad

[Hans Verkuil]

On 04/16/18 21:41, Hans Verkuil wrote:
> On 04/16/2018 08:09 PM, Mauro Carvalho Chehab wrote:
>> Em Mon, 16 Apr 2018 15:03:35 -0300
>> Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:
>>
>>> Em Mon, 16 Apr 2018 15:21:18 +0200
>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>
>>>> From: Hans Verkuil <hansverk@cisco.com>
>>>>
>>>> The v2 pad structure never exposed the pad index, which made it impossible
>>>> to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.
>>>>
>>>> It is really trivial to just expose this information, so implement this.  
>>>
>>> Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>>
>> Err... I looked on it too fast... See my comments below.
>>
>> The same applies to patch 8/9.
>>
>>>>
>>>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>>>> ---
>>>>  drivers/media/media-device.c | 1 +
>>>>  include/uapi/linux/media.h   | 7 ++++++-
>>>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>>> index dca1e5a3e0f9..73ffea3e81c9 100644
>>>> --- a/drivers/media/media-device.c
>>>> +++ b/drivers/media/media-device.c
>>>> @@ -331,6 +331,7 @@ static long media_device_get_topology(struct media_device *mdev,
>>>>  		kpad.id = pad->graph_obj.id;
>>>>  		kpad.entity_id = pad->entity->graph_obj.id;
>>>>  		kpad.flags = pad->flags;
>>>> +		kpad.index = pad->index;
>>>>  
>>>>  		if (copy_to_user(upad, &kpad, sizeof(kpad)))
>>>>  			ret = -EFAULT;
>>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>>>> index ac08acffdb65..15f7f432f808 100644
>>>> --- a/include/uapi/linux/media.h
>>>> +++ b/include/uapi/linux/media.h
>>>> @@ -310,11 +310,16 @@ struct media_v2_interface {
>>>>  	};
>>>>  } __attribute__ ((packed));
>>>>  
>>>> +/* Appeared in 4.18.0 */
>>>> +#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
>>>> +	((media_version) >= 0x00041200)
>>>> +
>>
>> I don't like this, for a couple of reasons:
>>
>> 1) it has a magic number on it, with is actually a parsed
>>    version of LINUX_VERSION() macro;
> 
> I can/should change that to KERNEL_VERSION().
> 
>>
>> 2) it sounds really weird to ship a header file with a new
>>    kernel version meant to provide backward compatibility with
>>    older versions;
>>
>> 3) this isn't any different than:
>>
>> 	#define MEDIA_V2_PAD_HAS_INDEX -1
>>
>> I think we need to think a little bit more about that.
> 
> What typically happens is that applications (like those in v4l-utils
> for example) copy the headers locally. So they are compiled with the headers
> of a specific kernel version, but they can run with very different kernels.
> 
> This is normal for distros where you can install different kernel versions
> without needing to modify applications.
> 
> In fact, we (Cisco) use the latest v4l-utils code on kernels ranging between
> 2.6.39 to 4.10 (I think that's the latest one in use).
> 
> The media version tells you whether or not the kernel supports this feature.
> I don't see another way of doing this.
> 
> In most other cases we can just say that if the field value is 0, then it
> should not be used. Unfortunately, 0 is a valid value for the pad index, for
> the entity flags and for the entity function (some drivers set it to
> MEDIA_ENT_F_UNKNOWN, which has value 0). This last one is most unfortunate,
> since this should never have happened and would have been detected if we had
> proper compliance tools.

Actually, I think that if I first ensure that all drivers correctly set function
to a non-zero value, then there is no need for a test macro and I can just say
that if it is 0, then fall back to 'type'.

It requires some analysis, but it's doable.

For the index and flags field there is no alternative that I can think of, though.

Regards,

	Hans

> 
> Regards,
> 
> 	Hans
> 
>>
>>
>>>>  struct media_v2_pad {
>>>>  	__u32 id;
>>>>  	__u32 entity_id;
>>>>  	__u32 flags;
>>>> -	__u32 reserved[5];
>>>> +	__u32 index;
>>>> +	__u32 reserved[4];
>>>>  } __attribute__ ((packed));
>>>>  
>>>>  struct media_v2_link {  
>>>
>>>
>>>
>>> Thanks,
>>> Mauro
>>
>>
>>
>> Thanks,
>> Mauro
>>
> 

Re: [PATCHv2 6/9] media: add 'index' to struct media_v2_pad

[Mauro Carvalho Chehab]

Em Tue, 17 Apr 2018 11:59:40 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 04/16/18 21:41, Hans Verkuil wrote:
> > On 04/16/2018 08:09 PM, Mauro Carvalho Chehab wrote:  
> >> Em Mon, 16 Apr 2018 15:03:35 -0300
> >> Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:
> >>  
> >>> Em Mon, 16 Apr 2018 15:21:18 +0200
> >>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>>  
> >>>> From: Hans Verkuil <hansverk@cisco.com>
> >>>>
> >>>> The v2 pad structure never exposed the pad index, which made it impossible
> >>>> to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.
> >>>>
> >>>> It is really trivial to just expose this information, so implement this.    
> >>>
> >>> Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>  
> >>
> >> Err... I looked on it too fast... See my comments below.
> >>
> >> The same applies to patch 8/9.
> >>  
> >>>>
> >>>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> >>>> ---
> >>>>  drivers/media/media-device.c | 1 +
> >>>>  include/uapi/linux/media.h   | 7 ++++++-
> >>>>  2 files changed, 7 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >>>> index dca1e5a3e0f9..73ffea3e81c9 100644
> >>>> --- a/drivers/media/media-device.c
> >>>> +++ b/drivers/media/media-device.c
> >>>> @@ -331,6 +331,7 @@ static long media_device_get_topology(struct media_device *mdev,
> >>>>  		kpad.id = pad->graph_obj.id;
> >>>>  		kpad.entity_id = pad->entity->graph_obj.id;
> >>>>  		kpad.flags = pad->flags;
> >>>> +		kpad.index = pad->index;
> >>>>  
> >>>>  		if (copy_to_user(upad, &kpad, sizeof(kpad)))
> >>>>  			ret = -EFAULT;
> >>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> >>>> index ac08acffdb65..15f7f432f808 100644
> >>>> --- a/include/uapi/linux/media.h
> >>>> +++ b/include/uapi/linux/media.h
> >>>> @@ -310,11 +310,16 @@ struct media_v2_interface {
> >>>>  	};
> >>>>  } __attribute__ ((packed));
> >>>>  
> >>>> +/* Appeared in 4.18.0 */
> >>>> +#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
> >>>> +	((media_version) >= 0x00041200)
> >>>> +  
> >>
> >> I don't like this, for a couple of reasons:
> >>
> >> 1) it has a magic number on it, with is actually a parsed
> >>    version of LINUX_VERSION() macro;  
> > 
> > I can/should change that to KERNEL_VERSION().

I don't think so. The macro is not there at include/uapi.

> >   
> >>
> >> 2) it sounds really weird to ship a header file with a new
> >>    kernel version meant to provide backward compatibility with
> >>    older versions;
> >>
> >> 3) this isn't any different than:
> >>
> >> 	#define MEDIA_V2_PAD_HAS_INDEX -1
> >>
> >> I think we need to think a little bit more about that.  
> > 
> > What typically happens is that applications (like those in v4l-utils
> > for example) copy the headers locally. So they are compiled with the headers
> > of a specific kernel version, but they can run with very different kernels.
> > 
> > This is normal for distros where you can install different kernel versions
> > without needing to modify applications.
> > 
> > In fact, we (Cisco) use the latest v4l-utils code on kernels ranging between
> > 2.6.39 to 4.10 (I think that's the latest one in use).

Well, if you use a macro, the "compat" code at v4l-utils (or whatever other
app you use) will be assuming the specific Kernel version you used when you
built it, with is probably not what you want.

The way of checking if a feature is there or not is, instead, to ask for
the media version via MEDIA_IOC_DEVICE_INFO. It should provide the
media API version.

This is already filled with:
	info->media_version = LINUX_VERSION_CODE;

So, all we need to do is to document that the new fields are available only
for such version or above and add such check at v4l-utils.

> > 
> > The media version tells you whether or not the kernel supports this feature.
> > I don't see another way of doing this.
> > 
> > In most other cases we can just say that if the field value is 0, then it
> > should not be used. Unfortunately, 0 is a valid value for the pad index, for
> > the entity flags and for the entity function (some drivers set it to
> > MEDIA_ENT_F_UNKNOWN, which has value 0). This last one is most unfortunate,
> > since this should never have happened and would have been detected if we had
> > proper compliance tools.  
> 
> Actually, I think that if I first ensure that all drivers correctly set function
> to a non-zero value, then there is no need for a test macro and I can just say
> that if it is 0, then fall back to 'type'.
> 
> It requires some analysis, but it's doable.

That is something that we want to do anyway.
> 
> For the index and flags field there is no alternative that I can think of, though.
> 
> Regards,
> 
> 	Hans
> 
> > 
> > Regards,
> > 
> > 	Hans
> >   
> >>
> >>  
> >>>>  struct media_v2_pad {
> >>>>  	__u32 id;
> >>>>  	__u32 entity_id;
> >>>>  	__u32 flags;
> >>>> -	__u32 reserved[5];
> >>>> +	__u32 index;
> >>>> +	__u32 reserved[4];
> >>>>  } __attribute__ ((packed));
> >>>>  
> >>>>  struct media_v2_link {    
> >>>
> >>>
> >>>
> >>> Thanks,
> >>> Mauro  
> >>
> >>
> >>
> >> Thanks,
> >> Mauro
> >>  
> >   
> 



Thanks,
Mauro

Re: [PATCHv2 6/9] media: add 'index' to struct media_v2_pad

[Hans Verkuil]

On 04/17/18 13:55, Mauro Carvalho Chehab wrote:
> Em Tue, 17 Apr 2018 11:59:40 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 04/16/18 21:41, Hans Verkuil wrote:
>>> On 04/16/2018 08:09 PM, Mauro Carvalho Chehab wrote:  
>>>> Em Mon, 16 Apr 2018 15:03:35 -0300
>>>> Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:
>>>>  
>>>>> Em Mon, 16 Apr 2018 15:21:18 +0200
>>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>>>  
>>>>>> From: Hans Verkuil <hansverk@cisco.com>
>>>>>>
>>>>>> The v2 pad structure never exposed the pad index, which made it impossible
>>>>>> to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.
>>>>>>
>>>>>> It is really trivial to just expose this information, so implement this.    
>>>>>
>>>>> Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>  
>>>>
>>>> Err... I looked on it too fast... See my comments below.
>>>>
>>>> The same applies to patch 8/9.
>>>>  
>>>>>>
>>>>>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>>>>>> ---
>>>>>>  drivers/media/media-device.c | 1 +
>>>>>>  include/uapi/linux/media.h   | 7 ++++++-
>>>>>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>>>>> index dca1e5a3e0f9..73ffea3e81c9 100644
>>>>>> --- a/drivers/media/media-device.c
>>>>>> +++ b/drivers/media/media-device.c
>>>>>> @@ -331,6 +331,7 @@ static long media_device_get_topology(struct media_device *mdev,
>>>>>>  		kpad.id = pad->graph_obj.id;
>>>>>>  		kpad.entity_id = pad->entity->graph_obj.id;
>>>>>>  		kpad.flags = pad->flags;
>>>>>> +		kpad.index = pad->index;
>>>>>>  
>>>>>>  		if (copy_to_user(upad, &kpad, sizeof(kpad)))
>>>>>>  			ret = -EFAULT;
>>>>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>>>>>> index ac08acffdb65..15f7f432f808 100644
>>>>>> --- a/include/uapi/linux/media.h
>>>>>> +++ b/include/uapi/linux/media.h
>>>>>> @@ -310,11 +310,16 @@ struct media_v2_interface {
>>>>>>  	};
>>>>>>  } __attribute__ ((packed));
>>>>>>  
>>>>>> +/* Appeared in 4.18.0 */
>>>>>> +#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
>>>>>> +	((media_version) >= 0x00041200)
>>>>>> +  
>>>>
>>>> I don't like this, for a couple of reasons:
>>>>
>>>> 1) it has a magic number on it, with is actually a parsed
>>>>    version of LINUX_VERSION() macro;  
>>>
>>> I can/should change that to KERNEL_VERSION().
> 
> I don't think so. The macro is not there at include/uapi.
> 
>>>   
>>>>
>>>> 2) it sounds really weird to ship a header file with a new
>>>>    kernel version meant to provide backward compatibility with
>>>>    older versions;
>>>>
>>>> 3) this isn't any different than:
>>>>
>>>> 	#define MEDIA_V2_PAD_HAS_INDEX -1
>>>>
>>>> I think we need to think a little bit more about that.  
>>>
>>> What typically happens is that applications (like those in v4l-utils
>>> for example) copy the headers locally. So they are compiled with the headers
>>> of a specific kernel version, but they can run with very different kernels.
>>>
>>> This is normal for distros where you can install different kernel versions
>>> without needing to modify applications.
>>>
>>> In fact, we (Cisco) use the latest v4l-utils code on kernels ranging between
>>> 2.6.39 to 4.10 (I think that's the latest one in use).
> 
> Well, if you use a macro, the "compat" code at v4l-utils (or whatever other
> app you use) will be assuming the specific Kernel version you used when you
> built it, with is probably not what you want.
> 
> The way of checking if a feature is there or not is, instead, to ask for
> the media version via MEDIA_IOC_DEVICE_INFO. It should provide the
> media API version.
> 
> This is already filled with:
> 	info->media_version = LINUX_VERSION_CODE;
> 
> So, all we need to do is to document that the new fields are available only
> for such version or above and add such check at v4l-utils.

Yes, and that's what you stick in the macro argument:

	ioctl(fd, MEDIA_IOC_DEVICE_INFO, &info);
	if (MEDIA_V2_PAD_HAS_INDEX(info.media_version)) {
		// I can use the index field
	}

I think I did not document this clearly.

Regards,

	Hans

> 
>>>
>>> The media version tells you whether or not the kernel supports this feature.
>>> I don't see another way of doing this.
>>>
>>> In most other cases we can just say that if the field value is 0, then it
>>> should not be used. Unfortunately, 0 is a valid value for the pad index, for
>>> the entity flags and for the entity function (some drivers set it to
>>> MEDIA_ENT_F_UNKNOWN, which has value 0). This last one is most unfortunate,
>>> since this should never have happened and would have been detected if we had
>>> proper compliance tools.  
>>
>> Actually, I think that if I first ensure that all drivers correctly set function
>> to a non-zero value, then there is no need for a test macro and I can just say
>> that if it is 0, then fall back to 'type'.
>>
>> It requires some analysis, but it's doable.
> 
> That is something that we want to do anyway.
>>
>> For the index and flags field there is no alternative that I can think of, though.
>>
>> Regards,
>>
>> 	Hans
>>
>>>
>>> Regards,
>>>
>>> 	Hans
>>>   
>>>>
>>>>  
>>>>>>  struct media_v2_pad {
>>>>>>  	__u32 id;
>>>>>>  	__u32 entity_id;
>>>>>>  	__u32 flags;
>>>>>> -	__u32 reserved[5];
>>>>>> +	__u32 index;
>>>>>> +	__u32 reserved[4];
>>>>>>  } __attribute__ ((packed));
>>>>>>  
>>>>>>  struct media_v2_link {    
>>>>>
>>>>>
>>>>>
>>>>> Thanks,
>>>>> Mauro  
>>>>
>>>>
>>>>
>>>> Thanks,
>>>> Mauro
>>>>  
>>>   
>>
> 
> 
> 
> Thanks,
> Mauro
> 

Re: [PATCHv2 4/9] media: add function field to struct media_entity_desc

[Mauro Carvalho Chehab]

Em Mon, 16 Apr 2018 21:48:56 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 04/16/2018 09:40 PM, Mauro Carvalho Chehab wrote:
> > Em Mon, 16 Apr 2018 21:27:01 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >   
> >> On 04/16/2018 08:01 PM, Mauro Carvalho Chehab wrote:  
> >>> Em Mon, 16 Apr 2018 15:21:16 +0200
> >>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>>     
> >>>> From: Hans Verkuil <hansverk@cisco.com>
> >>>>
> >>>> This adds support for 'proper' functions to the existing API.
> >>>> This information was before only available through the new v2
> >>>> API, with this change it's available to both.
> >>>>
> >>>> Yes, the plan is to allow entities to expose multiple functions for
> >>>> multi-function devices, but we do not support it anywhere so this
> >>>> is still vaporware.    
> >>>
> >>> I'm not convinced about that. I would, instead, just keep it as-is
> >>> and be sure that applications stop use the legacy calls.    
> >>
> >> You can't. First of all, since the new API does not provide the pad index
> >> (fixed in patch 6/9) it is impossible to use the new API with any driver
> >> that supports SETUP_LINK.  
> > 
> > Yeah, unfortunately, the properties API was just an empty promise.
> > 
> > Anyway, as you said, patch 6/9 solves it.
> >   
> >> So any such driver that uses any of the newer
> >> subdevs with a function that is mapped to MEDIA_ENT_T_DEVNODE_UNKNOWN
> >> is currently not reporting that correctly. A good example is the
> >> imx driver. But also others if they are combined with such newer subdevs.  
> > 
> > As far as I remember, other drivers also return MEDIA_ENT_F_UNKNOWN
> > (with also maps to MEDIA_ENT_T_DEVNODE_UNKNOWN) even via the new API, 
> > as the developer never cared to fill the entity function, even 
> > producing warnings.
> >   
> >> There is nothing wrong with the old API, except for not reporting the
> >> proper function value in field 'type' due to historical concerns.  
> > 
> > There is. That's why we took about one year developing a new API.  
> 
> If you don't need the new functionality (like interfaces), then it is
> perfectly fine. It's been in use for many years now.
> 
> >   
> >> There is NO WAY we can suddenly prohibit applications from using the old
> >> API since the new API was never usable. And besides that, we have no method
> >> of knowing who uses the old API since such applications are likely custom
> >> for specific hardware.  
> > 
> > Nobody is forbidding anything. We're just freezing it, as its
> > functionality was superseded.
> >   
> >> All that is really missing in the 'old' API (I hate the terms 'old' and
> >> 'new', they are misleading) is a proper 'function' field. Let's just add it
> >> and make it consistent with the documentation about entity functions.  
> > 
> > It misses interfaces - with is needed to identify what interface controls
> > what.  
> 
> Sure, but for most use cases interfaces are not needed. But reporting the correct
> function is very useful, makes the API consistent with the documentation (which
> only talks about functions and no longer refers to types) and the new API and it
> is trivial to add.
> 
> I'm not advocating any further chances, but while writing the compliance tests
> for this it was incredibly ugly to have this mismatch between 'type' and 'function'.

I'm not a big fan of this patch. IMHO, we should really try to
not touch APIs that are replaced by a newer version, but let's
hear for other opinions about this particular change.

> 
> The function of an entity is a critical piece of information, and having it
> clamped to UNKNOWN for the newer functions is just wrong.

As I said before, just the changes on this patchset won't solve it, as
several drivers simply don't initialize the subdev type/function.

> Regards,
> 
> 	Hans
> 
> >   
> >>  
> >>>     
> >>>>
> >>>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> >>>> ---
> >>>>  drivers/media/media-device.c | 1 +
> >>>>  include/uapi/linux/media.h   | 7 ++++++-
> >>>>  2 files changed, 7 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >>>> index 7c3ab37c258a..dca1e5a3e0f9 100644
> >>>> --- a/drivers/media/media-device.c
> >>>> +++ b/drivers/media/media-device.c
> >>>> @@ -115,6 +115,7 @@ static long media_device_enum_entities(struct media_device *mdev,
> >>>>  	if (ent->name)
> >>>>  		strlcpy(entd->name, ent->name, sizeof(entd->name));
> >>>>  	entd->type = ent->function;
> >>>> +	entd->function = ent->function;
> >>>>  	entd->revision = 0;		/* Unused */    
> >>>
> >>> I got confused here, until I went to the code and noticed that
> >>> entd->type is actually touched after this.
> >>>
> >>> If we're willing to do that, you should add a comment there explaining
> >>> why we need to pass both type and function to userspace.    
> >>
> >> True.
> >>
> >> Regards,
> >>
> >> 	Hans
> >>  
> >>>     
> >>>>  	entd->flags = ent->flags;
> >>>>  	entd->group_id = 0;		/* Unused */
> >>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> >>>> index 86c7dcc9cba3..ac08acffdb65 100644
> >>>> --- a/include/uapi/linux/media.h
> >>>> +++ b/include/uapi/linux/media.h
> >>>> @@ -146,6 +146,10 @@ struct media_device_info {
> >>>>  /* OR with the entity id value to find the next entity */
> >>>>  #define MEDIA_ENT_ID_FLAG_NEXT			(1 << 31)
> >>>>  
> >>>> +/* Appeared in 4.18.0 */
> >>>> +#define MEDIA_ENTITY_DESC_HAS_FUNCTION(media_version) \
> >>>> +	((media_version) >= 0x00041200)
> >>>> +
> >>>>  struct media_entity_desc {
> >>>>  	__u32 id;
> >>>>  	char name[32];
> >>>> @@ -155,8 +159,9 @@ struct media_entity_desc {
> >>>>  	__u32 group_id;
> >>>>  	__u16 pads;
> >>>>  	__u16 links;
> >>>> +	__u32 function;
> >>>>  
> >>>> -	__u32 reserved[4];
> >>>> +	__u32 reserved[3];
> >>>>  
> >>>>  	union {
> >>>>  		/* Node specifications */    
> >>>
> >>>
> >>>
> >>> Thanks,
> >>> Mauro
> >>>     
> >>  
> > 
> > 
> > 
> > Thanks,
> > Mauro
> >   
> 



Thanks,
Mauro

Re: [PATCHv2 6/9] media: add 'index' to struct media_v2_pad

[Mauro Carvalho Chehab]

Em Tue, 17 Apr 2018 14:01:06 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> On 04/17/18 13:55, Mauro Carvalho Chehab wrote:
> > Em Tue, 17 Apr 2018 11:59:40 +0200
> > Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >   
> >> On 04/16/18 21:41, Hans Verkuil wrote:  
> >>> On 04/16/2018 08:09 PM, Mauro Carvalho Chehab wrote:    
> >>>> Em Mon, 16 Apr 2018 15:03:35 -0300
> >>>> Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:
> >>>>    
> >>>>> Em Mon, 16 Apr 2018 15:21:18 +0200
> >>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> >>>>>    
> >>>>>> From: Hans Verkuil <hansverk@cisco.com>
> >>>>>>
> >>>>>> The v2 pad structure never exposed the pad index, which made it impossible
> >>>>>> to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.
> >>>>>>
> >>>>>> It is really trivial to just expose this information, so implement this.      
> >>>>>
> >>>>> Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>    
> >>>>
> >>>> Err... I looked on it too fast... See my comments below.
> >>>>
> >>>> The same applies to patch 8/9.
> >>>>    
> >>>>>>
> >>>>>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
> >>>>>> ---
> >>>>>>  drivers/media/media-device.c | 1 +
> >>>>>>  include/uapi/linux/media.h   | 7 ++++++-
> >>>>>>  2 files changed, 7 insertions(+), 1 deletion(-)
> >>>>>>
> >>>>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> >>>>>> index dca1e5a3e0f9..73ffea3e81c9 100644
> >>>>>> --- a/drivers/media/media-device.c
> >>>>>> +++ b/drivers/media/media-device.c
> >>>>>> @@ -331,6 +331,7 @@ static long media_device_get_topology(struct media_device *mdev,
> >>>>>>  		kpad.id = pad->graph_obj.id;
> >>>>>>  		kpad.entity_id = pad->entity->graph_obj.id;
> >>>>>>  		kpad.flags = pad->flags;
> >>>>>> +		kpad.index = pad->index;
> >>>>>>  
> >>>>>>  		if (copy_to_user(upad, &kpad, sizeof(kpad)))
> >>>>>>  			ret = -EFAULT;
> >>>>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> >>>>>> index ac08acffdb65..15f7f432f808 100644
> >>>>>> --- a/include/uapi/linux/media.h
> >>>>>> +++ b/include/uapi/linux/media.h
> >>>>>> @@ -310,11 +310,16 @@ struct media_v2_interface {
> >>>>>>  	};
> >>>>>>  } __attribute__ ((packed));
> >>>>>>  
> >>>>>> +/* Appeared in 4.18.0 */
> >>>>>> +#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
> >>>>>> +	((media_version) >= 0x00041200)
> >>>>>> +    
> >>>>
> >>>> I don't like this, for a couple of reasons:
> >>>>
> >>>> 1) it has a magic number on it, with is actually a parsed
> >>>>    version of LINUX_VERSION() macro;    
> >>>
> >>> I can/should change that to KERNEL_VERSION().  
> > 
> > I don't think so. The macro is not there at include/uapi.
> >   
> >>>     
> >>>>
> >>>> 2) it sounds really weird to ship a header file with a new
> >>>>    kernel version meant to provide backward compatibility with
> >>>>    older versions;
> >>>>
> >>>> 3) this isn't any different than:
> >>>>
> >>>> 	#define MEDIA_V2_PAD_HAS_INDEX -1
> >>>>
> >>>> I think we need to think a little bit more about that.    
> >>>
> >>> What typically happens is that applications (like those in v4l-utils
> >>> for example) copy the headers locally. So they are compiled with the headers
> >>> of a specific kernel version, but they can run with very different kernels.
> >>>
> >>> This is normal for distros where you can install different kernel versions
> >>> without needing to modify applications.
> >>>
> >>> In fact, we (Cisco) use the latest v4l-utils code on kernels ranging between
> >>> 2.6.39 to 4.10 (I think that's the latest one in use).  
> > 
> > Well, if you use a macro, the "compat" code at v4l-utils (or whatever other
> > app you use) will be assuming the specific Kernel version you used when you
> > built it, with is probably not what you want.
> > 
> > The way of checking if a feature is there or not is, instead, to ask for
> > the media version via MEDIA_IOC_DEVICE_INFO. It should provide the
> > media API version.
> > 
> > This is already filled with:
> > 	info->media_version = LINUX_VERSION_CODE;
> > 
> > So, all we need to do is to document that the new fields are available only
> > for such version or above and add such check at v4l-utils.  
> 
> Yes, and that's what you stick in the macro argument:
> 
> 	ioctl(fd, MEDIA_IOC_DEVICE_INFO, &info);
> 	if (MEDIA_V2_PAD_HAS_INDEX(info.media_version)) {
> 		// I can use the index field
> 	}
> 
> I think I did not document this clearly.

Ok, makes sense. It should be better documented, IMO.

Still have an issue with KERNEL_VERSION: this macro doesn't exist
anymore on any Kernel header files. It is produced dynamically
at /Makefile:

	define filechk_version.h
		(echo \#define LINUX_VERSION_CODE $(shell                         \
		expr $(VERSION) \* 65536 + 0$(PATCHLEVEL) \* 256 + 0$(SUBLEVEL)); \
		echo '#define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))';)
	endef

Btw, this likely means that this is already broken:

include/uapi/linux/media.h:#define MEDIA_API_VERSION                    KERNEL_VERSION(0, 1, 0)

as userspace won't be able to evaluate it.

We could hardcode its value, as you proposed, but, IMHO, that sucks.

Thanks,
Mauro

Re: [PATCHv2 6/9] media: add 'index' to struct media_v2_pad

[Hans Verkuil]

On 04/17/18 14:16, Mauro Carvalho Chehab wrote:
> Em Tue, 17 Apr 2018 14:01:06 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> On 04/17/18 13:55, Mauro Carvalho Chehab wrote:
>>> Em Tue, 17 Apr 2018 11:59:40 +0200
>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>   
>>>> On 04/16/18 21:41, Hans Verkuil wrote:  
>>>>> On 04/16/2018 08:09 PM, Mauro Carvalho Chehab wrote:    
>>>>>> Em Mon, 16 Apr 2018 15:03:35 -0300
>>>>>> Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:
>>>>>>    
>>>>>>> Em Mon, 16 Apr 2018 15:21:18 +0200
>>>>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>>>>>    
>>>>>>>> From: Hans Verkuil <hansverk@cisco.com>
>>>>>>>>
>>>>>>>> The v2 pad structure never exposed the pad index, which made it impossible
>>>>>>>> to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.
>>>>>>>>
>>>>>>>> It is really trivial to just expose this information, so implement this.      
>>>>>>>
>>>>>>> Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>    
>>>>>>
>>>>>> Err... I looked on it too fast... See my comments below.
>>>>>>
>>>>>> The same applies to patch 8/9.
>>>>>>    
>>>>>>>>
>>>>>>>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>>>>>>>> ---
>>>>>>>>  drivers/media/media-device.c | 1 +
>>>>>>>>  include/uapi/linux/media.h   | 7 ++++++-
>>>>>>>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>>>>>>> index dca1e5a3e0f9..73ffea3e81c9 100644
>>>>>>>> --- a/drivers/media/media-device.c
>>>>>>>> +++ b/drivers/media/media-device.c
>>>>>>>> @@ -331,6 +331,7 @@ static long media_device_get_topology(struct media_device *mdev,
>>>>>>>>  		kpad.id = pad->graph_obj.id;
>>>>>>>>  		kpad.entity_id = pad->entity->graph_obj.id;
>>>>>>>>  		kpad.flags = pad->flags;
>>>>>>>> +		kpad.index = pad->index;
>>>>>>>>  
>>>>>>>>  		if (copy_to_user(upad, &kpad, sizeof(kpad)))
>>>>>>>>  			ret = -EFAULT;
>>>>>>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>>>>>>>> index ac08acffdb65..15f7f432f808 100644
>>>>>>>> --- a/include/uapi/linux/media.h
>>>>>>>> +++ b/include/uapi/linux/media.h
>>>>>>>> @@ -310,11 +310,16 @@ struct media_v2_interface {
>>>>>>>>  	};
>>>>>>>>  } __attribute__ ((packed));
>>>>>>>>  
>>>>>>>> +/* Appeared in 4.18.0 */
>>>>>>>> +#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
>>>>>>>> +	((media_version) >= 0x00041200)
>>>>>>>> +    
>>>>>>
>>>>>> I don't like this, for a couple of reasons:
>>>>>>
>>>>>> 1) it has a magic number on it, with is actually a parsed
>>>>>>    version of LINUX_VERSION() macro;    
>>>>>
>>>>> I can/should change that to KERNEL_VERSION().  
>>>
>>> I don't think so. The macro is not there at include/uapi.
>>>   
>>>>>     
>>>>>>
>>>>>> 2) it sounds really weird to ship a header file with a new
>>>>>>    kernel version meant to provide backward compatibility with
>>>>>>    older versions;
>>>>>>
>>>>>> 3) this isn't any different than:
>>>>>>
>>>>>> 	#define MEDIA_V2_PAD_HAS_INDEX -1
>>>>>>
>>>>>> I think we need to think a little bit more about that.    
>>>>>
>>>>> What typically happens is that applications (like those in v4l-utils
>>>>> for example) copy the headers locally. So they are compiled with the headers
>>>>> of a specific kernel version, but they can run with very different kernels.
>>>>>
>>>>> This is normal for distros where you can install different kernel versions
>>>>> without needing to modify applications.
>>>>>
>>>>> In fact, we (Cisco) use the latest v4l-utils code on kernels ranging between
>>>>> 2.6.39 to 4.10 (I think that's the latest one in use).  
>>>
>>> Well, if you use a macro, the "compat" code at v4l-utils (or whatever other
>>> app you use) will be assuming the specific Kernel version you used when you
>>> built it, with is probably not what you want.
>>>
>>> The way of checking if a feature is there or not is, instead, to ask for
>>> the media version via MEDIA_IOC_DEVICE_INFO. It should provide the
>>> media API version.
>>>
>>> This is already filled with:
>>> 	info->media_version = LINUX_VERSION_CODE;
>>>
>>> So, all we need to do is to document that the new fields are available only
>>> for such version or above and add such check at v4l-utils.  
>>
>> Yes, and that's what you stick in the macro argument:
>>
>> 	ioctl(fd, MEDIA_IOC_DEVICE_INFO, &info);
>> 	if (MEDIA_V2_PAD_HAS_INDEX(info.media_version)) {
>> 		// I can use the index field
>> 	}
>>
>> I think I did not document this clearly.
> 
> Ok, makes sense. It should be better documented, IMO.
> 
> Still have an issue with KERNEL_VERSION: this macro doesn't exist
> anymore on any Kernel header files. It is produced dynamically
> at /Makefile:
> 
> 	define filechk_version.h
> 		(echo \#define LINUX_VERSION_CODE $(shell                         \
> 		expr $(VERSION) \* 65536 + 0$(PATCHLEVEL) \* 256 + 0$(SUBLEVEL)); \
> 		echo '#define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))';)
> 	endef
> 
> Btw, this likely means that this is already broken:
> 
> include/uapi/linux/media.h:#define MEDIA_API_VERSION                    KERNEL_VERSION(0, 1, 0)
> 
> as userspace won't be able to evaluate it.
> 
> We could hardcode its value, as you proposed, but, IMHO, that sucks.

This might be why I hardcoded it :-)

I personally don't have a problem with hardcoding it. In the end, it's just a number.

Regards,

	Hans

Re: [PATCHv2 6/9] media: add 'index' to struct media_v2_pad

[Hans Verkuil]

On 17/04/18 14:19, Hans Verkuil wrote:
> On 04/17/18 14:16, Mauro Carvalho Chehab wrote:
>> Em Tue, 17 Apr 2018 14:01:06 +0200
>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>
>>> On 04/17/18 13:55, Mauro Carvalho Chehab wrote:
>>>> Em Tue, 17 Apr 2018 11:59:40 +0200
>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>>   
>>>>> On 04/16/18 21:41, Hans Verkuil wrote:  
>>>>>> On 04/16/2018 08:09 PM, Mauro Carvalho Chehab wrote:    
>>>>>>> Em Mon, 16 Apr 2018 15:03:35 -0300
>>>>>>> Mauro Carvalho Chehab <mchehab@s-opensource.com> escreveu:
>>>>>>>    
>>>>>>>> Em Mon, 16 Apr 2018 15:21:18 +0200
>>>>>>>> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
>>>>>>>>    
>>>>>>>>> From: Hans Verkuil <hansverk@cisco.com>
>>>>>>>>>
>>>>>>>>> The v2 pad structure never exposed the pad index, which made it impossible
>>>>>>>>> to call the MEDIA_IOC_SETUP_LINK ioctl, which needs that information.
>>>>>>>>>
>>>>>>>>> It is really trivial to just expose this information, so implement this.      
>>>>>>>>
>>>>>>>> Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>    
>>>>>>>
>>>>>>> Err... I looked on it too fast... See my comments below.
>>>>>>>
>>>>>>> The same applies to patch 8/9.
>>>>>>>    
>>>>>>>>>
>>>>>>>>> Signed-off-by: Hans Verkuil <hansverk@cisco.com>
>>>>>>>>> ---
>>>>>>>>>  drivers/media/media-device.c | 1 +
>>>>>>>>>  include/uapi/linux/media.h   | 7 ++++++-
>>>>>>>>>  2 files changed, 7 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>>>>>>>> index dca1e5a3e0f9..73ffea3e81c9 100644
>>>>>>>>> --- a/drivers/media/media-device.c
>>>>>>>>> +++ b/drivers/media/media-device.c
>>>>>>>>> @@ -331,6 +331,7 @@ static long media_device_get_topology(struct media_device *mdev,
>>>>>>>>>  		kpad.id = pad->graph_obj.id;
>>>>>>>>>  		kpad.entity_id = pad->entity->graph_obj.id;
>>>>>>>>>  		kpad.flags = pad->flags;
>>>>>>>>> +		kpad.index = pad->index;
>>>>>>>>>  
>>>>>>>>>  		if (copy_to_user(upad, &kpad, sizeof(kpad)))
>>>>>>>>>  			ret = -EFAULT;
>>>>>>>>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>>>>>>>>> index ac08acffdb65..15f7f432f808 100644
>>>>>>>>> --- a/include/uapi/linux/media.h
>>>>>>>>> +++ b/include/uapi/linux/media.h
>>>>>>>>> @@ -310,11 +310,16 @@ struct media_v2_interface {
>>>>>>>>>  	};
>>>>>>>>>  } __attribute__ ((packed));
>>>>>>>>>  
>>>>>>>>> +/* Appeared in 4.18.0 */
>>>>>>>>> +#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
>>>>>>>>> +	((media_version) >= 0x00041200)
>>>>>>>>> +    
>>>>>>>
>>>>>>> I don't like this, for a couple of reasons:
>>>>>>>
>>>>>>> 1) it has a magic number on it, with is actually a parsed
>>>>>>>    version of LINUX_VERSION() macro;    
>>>>>>
>>>>>> I can/should change that to KERNEL_VERSION().  
>>>>
>>>> I don't think so. The macro is not there at include/uapi.
>>>>   
>>>>>>     
>>>>>>>
>>>>>>> 2) it sounds really weird to ship a header file with a new
>>>>>>>    kernel version meant to provide backward compatibility with
>>>>>>>    older versions;
>>>>>>>
>>>>>>> 3) this isn't any different than:
>>>>>>>
>>>>>>> 	#define MEDIA_V2_PAD_HAS_INDEX -1
>>>>>>>
>>>>>>> I think we need to think a little bit more about that.    
>>>>>>
>>>>>> What typically happens is that applications (like those in v4l-utils
>>>>>> for example) copy the headers locally. So they are compiled with the headers
>>>>>> of a specific kernel version, but they can run with very different kernels.
>>>>>>
>>>>>> This is normal for distros where you can install different kernel versions
>>>>>> without needing to modify applications.
>>>>>>
>>>>>> In fact, we (Cisco) use the latest v4l-utils code on kernels ranging between
>>>>>> 2.6.39 to 4.10 (I think that's the latest one in use).  
>>>>
>>>> Well, if you use a macro, the "compat" code at v4l-utils (or whatever other
>>>> app you use) will be assuming the specific Kernel version you used when you
>>>> built it, with is probably not what you want.
>>>>
>>>> The way of checking if a feature is there or not is, instead, to ask for
>>>> the media version via MEDIA_IOC_DEVICE_INFO. It should provide the
>>>> media API version.
>>>>
>>>> This is already filled with:
>>>> 	info->media_version = LINUX_VERSION_CODE;
>>>>
>>>> So, all we need to do is to document that the new fields are available only
>>>> for such version or above and add such check at v4l-utils.  
>>>
>>> Yes, and that's what you stick in the macro argument:
>>>
>>> 	ioctl(fd, MEDIA_IOC_DEVICE_INFO, &info);
>>> 	if (MEDIA_V2_PAD_HAS_INDEX(info.media_version)) {
>>> 		// I can use the index field
>>> 	}
>>>
>>> I think I did not document this clearly.
>>
>> Ok, makes sense. It should be better documented, IMO.
>>
>> Still have an issue with KERNEL_VERSION: this macro doesn't exist
>> anymore on any Kernel header files. It is produced dynamically
>> at /Makefile:
>>
>> 	define filechk_version.h
>> 		(echo \#define LINUX_VERSION_CODE $(shell                         \
>> 		expr $(VERSION) \* 65536 + 0$(PATCHLEVEL) \* 256 + 0$(SUBLEVEL)); \
>> 		echo '#define KERNEL_VERSION(a,b,c) (((a) << 16) + ((b) << 8) + (c))';)
>> 	endef
>>
>> Btw, this likely means that this is already broken:
>>
>> include/uapi/linux/media.h:#define MEDIA_API_VERSION                    KERNEL_VERSION(0, 1, 0)
>>
>> as userspace won't be able to evaluate it.
>>
>> We could hardcode its value, as you proposed, but, IMHO, that sucks.
> 
> This might be why I hardcoded it :-)
> 
> I personally don't have a problem with hardcoding it. In the end, it's just a number.

How about this:

/* Appeared in 4.18.0 */
#define MEDIA_V2_PAD_HAS_INDEX(media_version) \
       ((media_version) >= ((4 << 16) | (18 << 8) | 0))

It's better than 0x00041200, and it works in the absence of KERNEL_VERSION.

Regards,

	Hans