[RFC PATCH 0/2] Introduce ancillary links

[RFC PATCH 0/2] Introduce ancillary links

[Daniel Scally]

Hello all

This series is not yet ready to merge, but I wanted to share it as I know some
other folks are working in similar areas at the moment (and I am including the
libcamera devel list for the same reason)

At present there's no means in the kernel of describing the supporting
relationship between subdevices that work together to form an effective single
unit - the type example in this case being a camera sensor and its
corresponding vcm. To attempt to solve that, this series adds a new type of
media link called MEDIA_LNK_FL_ANCILLARY_LINK, which connects two instances of
struct media_entity. Further work would be needed to document it properly, and
there may be ramifications within the v4l2-core which I have not yet discovered
(a lot of places seem to assume that media_entity->links means pad-2-pad links,
so some extra work might be needed to validate the link type before doing any
thing to them).

The mechanism of connection I have modelled as a notifier and async subdev,
which seemed the best route since sensor drivers already typically will call
v4l2_async_register_subdev_sensor() on probe, and that function already looks
for a reference to a firmware node with the reference named "lens-focus". To
avoid boilerplate in the sensor drivers, I added some new functions in
v4l2-async that are called in v4l2_async_match_notify() to create the ancillary
links - checking the entity.function of both notifier and subdev to make sure
that's appropriate. I haven't gone further than that yet, but I suspect we could
cut down on code elsewhere by, for example, also creating pad-to-pad links in
the same place.

Thoughts and comments very welcome :)

Dan

Daniel Scally (2):
  media: entity: Add support for ancillary links
  media: v4l2-async: Create links during v4l2_async_match_notify()

 drivers/media/mc/mc-entity.c         | 30 ++++++++++++++++
 drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++
 include/media/media-entity.h         | 30 ++++++++++++++++
 include/uapi/linux/media.h           |  1 +
 4 files changed, 112 insertions(+)

-- 
2.25.1

[RFC PATCH 1/2] media: entity: Add support for ancillary links

[Daniel Scally]

To describe in the kernel the connection between devices and their
supporting peripherals (for example, a camera sensor and the vcm
driving the focusing lens for it), add a new type of media link
which connects two instances of struct media_entity.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---

I was tempted to 'fix' the spaces between # and define in
include/uapi/linux/media.h but eventually decided they were probably deliberate
but if that's not true I'd fix those too.

 drivers/media/mc/mc-entity.c | 30 ++++++++++++++++++++++++++++++
 include/media/media-entity.h | 30 ++++++++++++++++++++++++++++++
 include/uapi/linux/media.h   |  1 +
 3 files changed, 61 insertions(+)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index f40f41977142..9c18b974e117 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -1032,3 +1032,33 @@ void media_remove_intf_links(struct media_interface *intf)
 	mutex_unlock(&mdev->graph_mutex);
 }
 EXPORT_SYMBOL_GPL(media_remove_intf_links);
+
+struct media_link *media_create_ancillary_link(struct media_entity *primary,
+					       struct media_entity *ancillary,
+					       u32 flags)
+{
+	struct media_link *link;
+
+	link = media_add_link(&primary->links);
+	if (!link)
+		return ERR_PTR(-ENOMEM);
+
+	link->primary = primary;
+	link->ancillary = ancillary;
+	link->flags = flags | MEDIA_LNK_FL_ANCILLARY_LINK;
+
+	/* Initialize graph object embedded at the new link */
+	media_gobj_create(primary->graph_obj.mdev, MEDIA_GRAPH_LINK,
+			  &link->graph_obj);
+
+	return link;
+}
+EXPORT_SYMBOL_GPL(media_create_ancillary_link);
+
+void media_remove_ancillary_link(struct media_link *link)
+{
+	list_del(&link->list);
+	media_gobj_destroy(&link->graph_obj);
+	kfree(link);
+}
+EXPORT_SYMBOL_GPL(media_remove_ancillary_link);
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index fea489f03d57..400b864857ee 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -119,12 +119,16 @@ struct media_pipeline {
  *		a pad. In that case, it represents the source pad.
  * @intf:	Part of a union. Used only if the first object (gobj0) is
  *		an interface.
+ * @primary:	Part of a union. Used only if the first object (gobj0) is
+ *		an entity and the link type is MEDIA_LNK_FL_ANCILLARY_LINK.
  * @gobj1:	Part of a union. Used to get the pointer for the second
  *		graph_object of the link.
  * @sink:	Part of a union. Used only if the second object (gobj1) is
  *		a pad. In that case, it represents the sink pad.
  * @entity:	Part of a union. Used only if the second object (gobj1) is
  *		an entity.
+ * @ancillary:	Part of a union. Used only if the second object (gobj1) is
+ *		an entity and the link type is MEDIA_LNK_FL_ANCILLARY_LINK.
  * @reverse:	Pointer to the link for the reverse direction of a pad to pad
  *		link.
  * @flags:	Link flags, as defined in uapi/media.h (MEDIA_LNK_FL_*)
@@ -137,11 +141,13 @@ struct media_link {
 		struct media_gobj *gobj0;
 		struct media_pad *source;
 		struct media_interface *intf;
+		struct media_entity *primary;
 	};
 	union {
 		struct media_gobj *gobj1;
 		struct media_pad *sink;
 		struct media_entity *entity;
+		struct media_entity *ancillary;
 	};
 	struct media_link *reverse;
 	unsigned long flags;
@@ -1104,6 +1110,30 @@ void media_remove_intf_links(struct media_interface *intf);
  * it will issue a call to @operation\(@entity, @args\).
  */
 
+/**
+ * media_create_ancillary_link() - creates a link between two entities
+ *
+ * @primary:	pointer to the primary %media_entity
+ * @ancillary:	pointer to the ancillary %media_entity
+ * @flags:	Link flags, as defined in
+ *		:ref:`include/uapi/linux/media.h <media_header>`
+ *		( seek for ``MEDIA_LNK_FL_*``)
+ *
+ *
+ * Valid values for flags:
+ *
+ * %MEDIA_LNK_FL_ENABLED
+ *   Indicates that the two entities are connected pieces of hardware that form
+ *   a single logical unit.
+ *
+ *   A typical example is a camera lens being linked to the sensor that it is
+ *   supporting.
+ */
+struct media_link *
+media_create_ancillary_link(struct media_entity *primary,
+			    struct media_entity *ancillary,
+			    u32 flags);
+
 #define media_entity_call(entity, operation, args...)			\
 	(((entity)->ops && (entity)->ops->operation) ?			\
 	 (entity)->ops->operation((entity) , ##args) : -ENOIOCTLCMD)
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index 200fa8462b90..afbae7213d35 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -226,6 +226,7 @@ struct media_pad_desc {
 #define MEDIA_LNK_FL_LINK_TYPE			(0xf << 28)
 #  define MEDIA_LNK_FL_DATA_LINK		(0 << 28)
 #  define MEDIA_LNK_FL_INTERFACE_LINK		(1 << 28)
+#  define MEDIA_LNK_FL_ANCILLARY_LINK		(2 << 28)
 
 struct media_link_desc {
 	struct media_pad_desc source;
-- 
2.25.1

[RFC PATCH 2/2] media: v4l2-async: Create links during v4l2_async_match_notify()

[Daniel Scally]

Upon an async fwnode match, there's some typical behaviour that the
notifier and matching subdev will want to do. For example, a notifier
representing a sensor matching to an async subdev representing its
VCM will want to create an ancillary link to expose that relationship
to userspace.

To avoid lots of code in individual drivers, try to build these links
within v4l2 core.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---

This is probably more complex than it needs to be at present, but the intent
would be to expand it with __v4l2_async_create_data_link() to be called where
the notifier entity's function was MEDIA_ENT_F_VID_IF_BRIDGE for example.

 drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 0404267f1ae4..6575b1cbe95f 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -275,6 +275,45 @@ v4l2_async_nf_try_complete(struct v4l2_async_notifier *notifier)
 static int
 v4l2_async_nf_try_all_subdevs(struct v4l2_async_notifier *notifier);
 
+static int
+__v4l2_async_create_ancillary_link(struct v4l2_async_notifier *notifier,
+				   struct v4l2_subdev *sd)
+{
+	struct media_link *link;
+
+	if (sd->entity.function != MEDIA_ENT_F_LENS &&
+	    sd->entity.function != MEDIA_ENT_F_FLASH)
+		return -EINVAL;
+
+	link = media_create_ancillary_link(&notifier->sd->entity, &sd->entity,
+					   MEDIA_LNK_FL_ENABLED |
+					   MEDIA_LNK_FL_IMMUTABLE);
+
+	return IS_ERR(link) ? PTR_ERR(link) : 0;
+}
+
+/*
+ * Setup links on behalf of the notifier and subdev, where it's obvious what
+ * should be done. At the moment, we only support cases where the notifier
+ * is a sensor and the subdev is a lens.
+ *
+ * TODO: Setup pad links if the notifier's function is MEDIA_ENT_F_VID_IF_BRIDGE
+ * and the subdev's is MEDIA_ENT_F_CAM_SENSOR
+ */
+static int v4l2_async_try_create_links(struct v4l2_async_notifier *notifier,
+				       struct v4l2_subdev *sd)
+{
+	if (!notifier->sd)
+		return 0;
+
+	switch (notifier->sd->entity.function) {
+	case MEDIA_ENT_F_CAM_SENSOR:
+		return __v4l2_async_create_ancillary_link(notifier, sd);
+	default:
+		return 0;
+	}
+}
+
 static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
 				   struct v4l2_device *v4l2_dev,
 				   struct v4l2_subdev *sd,
@@ -293,6 +332,18 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
 		return ret;
 	}
 
+	/*
+	 * Depending of the function of the entities involved, we may want to
+	 * create links between them (for example between a sensor and its lens
+	 * or between a sensor's source pad and the connected device's sink
+	 * pad)
+	 */
+	ret = v4l2_async_try_create_links(notifier, sd);
+	if (ret) {
+		v4l2_device_unregister_subdev(sd);
+		return ret;
+	}
+
 	/* Remove from the waiting list */
 	list_del(&asd->list);
 	sd->asd = asd;
-- 
2.25.1

Re: [RFC PATCH 1/2] media: entity: Add support for ancillary links

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]

Hi Daniel,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on media-tree/master]
[also build test WARNING on v5.16-rc2 next-20211125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Daniel-Scally/Introduce-ancillary-links/20211126-081931
base:   git://linuxtv.org/media_tree.git master
config: nds32-buildonly-randconfig-r002-20211125 (https://download.01.org/0day-ci/archive/20211126/202111261025.4FswT3HZ-lkp(a)intel.com/config)
compiler: nds32le-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/3770587c7546ff01080c99a552fa3fd486528b1d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Scally/Introduce-ancillary-links/20211126-081931
        git checkout 3770587c7546ff01080c99a552fa3fd486528b1d
        # save the config file to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=nds32 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/media/mc/mc-entity.c:1058:6: warning: no previous prototype for 'media_remove_ancillary_link' [-Wmissing-prototypes]
    1058 | void media_remove_ancillary_link(struct media_link *link)
         |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~


vim +/media_remove_ancillary_link +1058 drivers/media/mc/mc-entity.c

  1057	
> 1058	void media_remove_ancillary_link(struct media_link *link)

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [RFC PATCH 1/2] media: entity: Add support for ancillary links

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Fri, Nov 26, 2021 at 12:16:02AM +0000, Daniel Scally wrote:
> To describe in the kernel the connection between devices and their
> supporting peripherals (for example, a camera sensor and the vcm
> driving the focusing lens for it), add a new type of media link
> which connects two instances of struct media_entity.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> 
> I was tempted to 'fix' the spaces between # and define in
> include/uapi/linux/media.h but eventually decided they were probably deliberate
> but if that's not true I'd fix those too.
> 
>  drivers/media/mc/mc-entity.c | 30 ++++++++++++++++++++++++++++++
>  include/media/media-entity.h | 30 ++++++++++++++++++++++++++++++
>  include/uapi/linux/media.h   |  1 +
>  3 files changed, 61 insertions(+)
> 
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index f40f41977142..9c18b974e117 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -1032,3 +1032,33 @@ void media_remove_intf_links(struct media_interface *intf)
>  	mutex_unlock(&mdev->graph_mutex);
>  }
>  EXPORT_SYMBOL_GPL(media_remove_intf_links);
> +
> +struct media_link *media_create_ancillary_link(struct media_entity *primary,
> +					       struct media_entity *ancillary,
> +					       u32 flags)
> +{
> +	struct media_link *link;
> +
> +	link = media_add_link(&primary->links);
> +	if (!link)
> +		return ERR_PTR(-ENOMEM);
> +
> +	link->primary = primary;
> +	link->ancillary = ancillary;
> +	link->flags = flags | MEDIA_LNK_FL_ANCILLARY_LINK;
> +
> +	/* Initialize graph object embedded at the new link */
> +	media_gobj_create(primary->graph_obj.mdev, MEDIA_GRAPH_LINK,
> +			  &link->graph_obj);
> +
> +	return link;
> +}
> +EXPORT_SYMBOL_GPL(media_create_ancillary_link);
> +
> +void media_remove_ancillary_link(struct media_link *link)
> +{
> +	list_del(&link->list);
> +	media_gobj_destroy(&link->graph_obj);
> +	kfree(link);
> +}
> +EXPORT_SYMBOL_GPL(media_remove_ancillary_link);
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index fea489f03d57..400b864857ee 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -119,12 +119,16 @@ struct media_pipeline {
>   *		a pad. In that case, it represents the source pad.
>   * @intf:	Part of a union. Used only if the first object (gobj0) is
>   *		an interface.
> + * @primary:	Part of a union. Used only if the first object (gobj0) is
> + *		an entity and the link type is MEDIA_LNK_FL_ANCILLARY_LINK.
>   * @gobj1:	Part of a union. Used to get the pointer for the second
>   *		graph_object of the link.
>   * @sink:	Part of a union. Used only if the second object (gobj1) is
>   *		a pad. In that case, it represents the sink pad.
>   * @entity:	Part of a union. Used only if the second object (gobj1) is
>   *		an entity.
> + * @ancillary:	Part of a union. Used only if the second object (gobj1) is
> + *		an entity and the link type is MEDIA_LNK_FL_ANCILLARY_LINK.
>   * @reverse:	Pointer to the link for the reverse direction of a pad to pad
>   *		link.
>   * @flags:	Link flags, as defined in uapi/media.h (MEDIA_LNK_FL_*)
> @@ -137,11 +141,13 @@ struct media_link {
>  		struct media_gobj *gobj0;
>  		struct media_pad *source;
>  		struct media_interface *intf;
> +		struct media_entity *primary;
>  	};
>  	union {
>  		struct media_gobj *gobj1;
>  		struct media_pad *sink;
>  		struct media_entity *entity;
> +		struct media_entity *ancillary;
>  	};

Those unions are not very nice, but it's not your fault. I however
wonder if we couldn't just use the gobj0 and gobj1 fields, and avoid
adding primary and ancillary.

I'm sure we'll also nitpick on the names, especially when adding
documentation. The concept seems fine to me though.

>  	struct media_link *reverse;
>  	unsigned long flags;
> @@ -1104,6 +1110,30 @@ void media_remove_intf_links(struct media_interface *intf);
>   * it will issue a call to @operation\(@entity, @args\).
>   */
>  
> +/**
> + * media_create_ancillary_link() - creates a link between two entities
> + *
> + * @primary:	pointer to the primary %media_entity
> + * @ancillary:	pointer to the ancillary %media_entity
> + * @flags:	Link flags, as defined in
> + *		:ref:`include/uapi/linux/media.h <media_header>`
> + *		( seek for ``MEDIA_LNK_FL_*``)
> + *
> + *
> + * Valid values for flags:
> + *
> + * %MEDIA_LNK_FL_ENABLED
> + *   Indicates that the two entities are connected pieces of hardware that form
> + *   a single logical unit.
> + *
> + *   A typical example is a camera lens being linked to the sensor that it is
> + *   supporting.

Is there any use case for an ancillary link that wouldn't be enabled ?
Should the IMMUTABLE flag be set as well ?

> + */
> +struct media_link *
> +media_create_ancillary_link(struct media_entity *primary,
> +			    struct media_entity *ancillary,
> +			    u32 flags);
> +
>  #define media_entity_call(entity, operation, args...)			\
>  	(((entity)->ops && (entity)->ops->operation) ?			\
>  	 (entity)->ops->operation((entity) , ##args) : -ENOIOCTLCMD)
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 200fa8462b90..afbae7213d35 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -226,6 +226,7 @@ struct media_pad_desc {
>  #define MEDIA_LNK_FL_LINK_TYPE			(0xf << 28)
>  #  define MEDIA_LNK_FL_DATA_LINK		(0 << 28)
>  #  define MEDIA_LNK_FL_INTERFACE_LINK		(1 << 28)
> +#  define MEDIA_LNK_FL_ANCILLARY_LINK		(2 << 28)
>  
>  struct media_link_desc {
>  	struct media_pad_desc source;

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 0/2] Introduce ancillary links

[Laurent Pinchart]

On Fri, Nov 26, 2021 at 12:16:01AM +0000, Daniel Scally wrote:
> Hello all
> 
> This series is not yet ready to merge, but I wanted to share it as I know some
> other folks are working in similar areas at the moment (and I am including the
> libcamera devel list for the same reason)

Seems you forgot to CC libcamera-devel :-) Let's fix that on v2.

> At present there's no means in the kernel of describing the supporting
> relationship between subdevices that work together to form an effective single
> unit - the type example in this case being a camera sensor and its
> corresponding vcm. To attempt to solve that, this series adds a new type of
> media link called MEDIA_LNK_FL_ANCILLARY_LINK, which connects two instances of
> struct media_entity. Further work would be needed to document it properly, and
> there may be ramifications within the v4l2-core which I have not yet discovered
> (a lot of places seem to assume that media_entity->links means pad-2-pad links,
> so some extra work might be needed to validate the link type before doing any
> thing to them).
> 
> The mechanism of connection I have modelled as a notifier and async subdev,
> which seemed the best route since sensor drivers already typically will call
> v4l2_async_register_subdev_sensor() on probe, and that function already looks
> for a reference to a firmware node with the reference named "lens-focus". To
> avoid boilerplate in the sensor drivers, I added some new functions in
> v4l2-async that are called in v4l2_async_match_notify() to create the ancillary
> links - checking the entity.function of both notifier and subdev to make sure
> that's appropriate. I haven't gone further than that yet, but I suspect we could
> cut down on code elsewhere by, for example, also creating pad-to-pad links in
> the same place.
> 
> Thoughts and comments very welcome :)
> 
> Dan
> 
> Daniel Scally (2):
>   media: entity: Add support for ancillary links
>   media: v4l2-async: Create links during v4l2_async_match_notify()
> 
>  drivers/media/mc/mc-entity.c         | 30 ++++++++++++++++
>  drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++
>  include/media/media-entity.h         | 30 ++++++++++++++++
>  include/uapi/linux/media.h           |  1 +
>  4 files changed, 112 insertions(+)
> 

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH 1/2] media: entity: Add support for ancillary links

[kernel test robot]

Hi Daniel,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on media-tree/master]
[also build test WARNING on v5.16-rc2 next-20211125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Daniel-Scally/Introduce-ancillary-links/20211126-081931
base:   git://linuxtv.org/media_tree.git master
config: mips-buildonly-randconfig-r003-20211125 (https://download.01.org/0day-ci/archive/20211126/202111261133.XqFnu5wF-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 0332d105b9ad7f1f0ffca7e78b71de8b3a48f158)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/0day-ci/linux/commit/3770587c7546ff01080c99a552fa3fd486528b1d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Scally/Introduce-ancillary-links/20211126-081931
        git checkout 3770587c7546ff01080c99a552fa3fd486528b1d
        # save the config file to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/media/mc/mc-entity.c:1058:6: warning: no previous prototype for function 'media_remove_ancillary_link' [-Wmissing-prototypes]
   void media_remove_ancillary_link(struct media_link *link)
        ^
   drivers/media/mc/mc-entity.c:1058:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void media_remove_ancillary_link(struct media_link *link)
   ^
   static 
   drivers/media/mc/mc-entity.c:17:27: warning: unused function 'gobj_type' [-Wunused-function]
   static inline const char *gobj_type(enum media_gobj_type type)
                             ^
   2 warnings generated.


vim +/media_remove_ancillary_link +1058 drivers/media/mc/mc-entity.c

  1057	
> 1058	void media_remove_ancillary_link(struct media_link *link)

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [RFC PATCH 1/2] media: entity: Add support for ancillary links

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2600 bytes --]

Hi Daniel,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on media-tree/master]
[also build test WARNING on v5.16-rc2 next-20211125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Daniel-Scally/Introduce-ancillary-links/20211126-081931
base:   git://linuxtv.org/media_tree.git master
config: mips-buildonly-randconfig-r003-20211125 (https://download.01.org/0day-ci/archive/20211126/202111261133.XqFnu5wF-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 0332d105b9ad7f1f0ffca7e78b71de8b3a48f158)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install mips cross compiling tool for clang build
        # apt-get install binutils-mips-linux-gnu
        # https://github.com/0day-ci/linux/commit/3770587c7546ff01080c99a552fa3fd486528b1d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Scally/Introduce-ancillary-links/20211126-081931
        git checkout 3770587c7546ff01080c99a552fa3fd486528b1d
        # save the config file to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/media/mc/mc-entity.c:1058:6: warning: no previous prototype for function 'media_remove_ancillary_link' [-Wmissing-prototypes]
   void media_remove_ancillary_link(struct media_link *link)
        ^
   drivers/media/mc/mc-entity.c:1058:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   void media_remove_ancillary_link(struct media_link *link)
   ^
   static 
   drivers/media/mc/mc-entity.c:17:27: warning: unused function 'gobj_type' [-Wunused-function]
   static inline const char *gobj_type(enum media_gobj_type type)
                             ^
   2 warnings generated.


vim +/media_remove_ancillary_link +1058 drivers/media/mc/mc-entity.c

  1057	
> 1058	void media_remove_ancillary_link(struct media_link *link)

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

Re: [RFC PATCH 1/2] media: entity: Add support for ancillary links

[Daniel Scally]

Hi Laurent

On 26/11/2021 02:56, Laurent Pinchart wrote:
> Hi Dan,
>
> Thank you for the patch.
>
> On Fri, Nov 26, 2021 at 12:16:02AM +0000, Daniel Scally wrote:
>> To describe in the kernel the connection between devices and their
>> supporting peripherals (for example, a camera sensor and the vcm
>> driving the focusing lens for it), add a new type of media link
>> which connects two instances of struct media_entity.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>>
>> I was tempted to 'fix' the spaces between # and define in
>> include/uapi/linux/media.h but eventually decided they were probably deliberate
>> but if that's not true I'd fix those too.
>>
>>  drivers/media/mc/mc-entity.c | 30 ++++++++++++++++++++++++++++++
>>  include/media/media-entity.h | 30 ++++++++++++++++++++++++++++++
>>  include/uapi/linux/media.h   |  1 +
>>  3 files changed, 61 insertions(+)
>>
>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
>> index f40f41977142..9c18b974e117 100644
>> --- a/drivers/media/mc/mc-entity.c
>> +++ b/drivers/media/mc/mc-entity.c
>> @@ -1032,3 +1032,33 @@ void media_remove_intf_links(struct media_interface *intf)
>>  	mutex_unlock(&mdev->graph_mutex);
>>  }
>>  EXPORT_SYMBOL_GPL(media_remove_intf_links);
>> +
>> +struct media_link *media_create_ancillary_link(struct media_entity *primary,
>> +					       struct media_entity *ancillary,
>> +					       u32 flags)
>> +{
>> +	struct media_link *link;
>> +
>> +	link = media_add_link(&primary->links);
>> +	if (!link)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	link->primary = primary;
>> +	link->ancillary = ancillary;
>> +	link->flags = flags | MEDIA_LNK_FL_ANCILLARY_LINK;
>> +
>> +	/* Initialize graph object embedded at the new link */
>> +	media_gobj_create(primary->graph_obj.mdev, MEDIA_GRAPH_LINK,
>> +			  &link->graph_obj);
>> +
>> +	return link;
>> +}
>> +EXPORT_SYMBOL_GPL(media_create_ancillary_link);
>> +
>> +void media_remove_ancillary_link(struct media_link *link)
>> +{
>> +	list_del(&link->list);
>> +	media_gobj_destroy(&link->graph_obj);
>> +	kfree(link);
>> +}
>> +EXPORT_SYMBOL_GPL(media_remove_ancillary_link);
>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>> index fea489f03d57..400b864857ee 100644
>> --- a/include/media/media-entity.h
>> +++ b/include/media/media-entity.h
>> @@ -119,12 +119,16 @@ struct media_pipeline {
>>   *		a pad. In that case, it represents the source pad.
>>   * @intf:	Part of a union. Used only if the first object (gobj0) is
>>   *		an interface.
>> + * @primary:	Part of a union. Used only if the first object (gobj0) is
>> + *		an entity and the link type is MEDIA_LNK_FL_ANCILLARY_LINK.
>>   * @gobj1:	Part of a union. Used to get the pointer for the second
>>   *		graph_object of the link.
>>   * @sink:	Part of a union. Used only if the second object (gobj1) is
>>   *		a pad. In that case, it represents the sink pad.
>>   * @entity:	Part of a union. Used only if the second object (gobj1) is
>>   *		an entity.
>> + * @ancillary:	Part of a union. Used only if the second object (gobj1) is
>> + *		an entity and the link type is MEDIA_LNK_FL_ANCILLARY_LINK.
>>   * @reverse:	Pointer to the link for the reverse direction of a pad to pad
>>   *		link.
>>   * @flags:	Link flags, as defined in uapi/media.h (MEDIA_LNK_FL_*)
>> @@ -137,11 +141,13 @@ struct media_link {
>>  		struct media_gobj *gobj0;
>>  		struct media_pad *source;
>>  		struct media_interface *intf;
>> +		struct media_entity *primary;
>>  	};
>>  	union {
>>  		struct media_gobj *gobj1;
>>  		struct media_pad *sink;
>>  		struct media_entity *entity;
>> +		struct media_entity *ancillary;
>>  	};
> Those unions are not very nice, but it's not your fault. I however
> wonder if we couldn't just use the gobj0 and gobj1 fields, and avoid
> adding primary and ancillary.


Maybe; I'll investigate doing it that way


> I'm sure we'll also nitpick on the names, especially when adding
> documentation. The concept seems fine to me though.


Hah; that's fine, naming things was never my strong suit.

>>  	struct media_link *reverse;
>>  	unsigned long flags;
>> @@ -1104,6 +1110,30 @@ void media_remove_intf_links(struct media_interface *intf);
>>   * it will issue a call to @operation\(@entity, @args\).
>>   */
>>  
>> +/**
>> + * media_create_ancillary_link() - creates a link between two entities
>> + *
>> + * @primary:	pointer to the primary %media_entity
>> + * @ancillary:	pointer to the ancillary %media_entity
>> + * @flags:	Link flags, as defined in
>> + *		:ref:`include/uapi/linux/media.h <media_header>`
>> + *		( seek for ``MEDIA_LNK_FL_*``)
>> + *
>> + *
>> + * Valid values for flags:
>> + *
>> + * %MEDIA_LNK_FL_ENABLED
>> + *   Indicates that the two entities are connected pieces of hardware that form
>> + *   a single logical unit.
>> + *
>> + *   A typical example is a camera lens being linked to the sensor that it is
>> + *   supporting.
> Is there any use case for an ancillary link that wouldn't be enabled ?


I couldn't think of one really.

> Should the IMMUTABLE flag be set as well ?


Yes; I'll add that here too, thanks

>
>> + */
>> +struct media_link *
>> +media_create_ancillary_link(struct media_entity *primary,
>> +			    struct media_entity *ancillary,
>> +			    u32 flags);
>> +
>>  #define media_entity_call(entity, operation, args...)			\
>>  	(((entity)->ops && (entity)->ops->operation) ?			\
>>  	 (entity)->ops->operation((entity) , ##args) : -ENOIOCTLCMD)
>> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
>> index 200fa8462b90..afbae7213d35 100644
>> --- a/include/uapi/linux/media.h
>> +++ b/include/uapi/linux/media.h
>> @@ -226,6 +226,7 @@ struct media_pad_desc {
>>  #define MEDIA_LNK_FL_LINK_TYPE			(0xf << 28)
>>  #  define MEDIA_LNK_FL_DATA_LINK		(0 << 28)
>>  #  define MEDIA_LNK_FL_INTERFACE_LINK		(1 << 28)
>> +#  define MEDIA_LNK_FL_ANCILLARY_LINK		(2 << 28)
>>  
>>  struct media_link_desc {
>>  	struct media_pad_desc source;

Re: [RFC PATCH 0/2] Introduce ancillary links

[Daniel Scally]

On 26/11/2021 02:59, Laurent Pinchart wrote:
> On Fri, Nov 26, 2021 at 12:16:01AM +0000, Daniel Scally wrote:
>> Hello all
>>
>> This series is not yet ready to merge, but I wanted to share it as I know some
>> other folks are working in similar areas at the moment (and I am including the
>> libcamera devel list for the same reason)
> Seems you forgot to CC libcamera-devel :-) Let's fix that on v2.


Argh! Sorry, will do

>> At present there's no means in the kernel of describing the supporting
>> relationship between subdevices that work together to form an effective single
>> unit - the type example in this case being a camera sensor and its
>> corresponding vcm. To attempt to solve that, this series adds a new type of
>> media link called MEDIA_LNK_FL_ANCILLARY_LINK, which connects two instances of
>> struct media_entity. Further work would be needed to document it properly, and
>> there may be ramifications within the v4l2-core which I have not yet discovered
>> (a lot of places seem to assume that media_entity->links means pad-2-pad links,
>> so some extra work might be needed to validate the link type before doing any
>> thing to them).
>>
>> The mechanism of connection I have modelled as a notifier and async subdev,
>> which seemed the best route since sensor drivers already typically will call
>> v4l2_async_register_subdev_sensor() on probe, and that function already looks
>> for a reference to a firmware node with the reference named "lens-focus". To
>> avoid boilerplate in the sensor drivers, I added some new functions in
>> v4l2-async that are called in v4l2_async_match_notify() to create the ancillary
>> links - checking the entity.function of both notifier and subdev to make sure
>> that's appropriate. I haven't gone further than that yet, but I suspect we could
>> cut down on code elsewhere by, for example, also creating pad-to-pad links in
>> the same place.
>>
>> Thoughts and comments very welcome :)
>>
>> Dan
>>
>> Daniel Scally (2):
>>   media: entity: Add support for ancillary links
>>   media: v4l2-async: Create links during v4l2_async_match_notify()
>>
>>  drivers/media/mc/mc-entity.c         | 30 ++++++++++++++++
>>  drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++
>>  include/media/media-entity.h         | 30 ++++++++++++++++
>>  include/uapi/linux/media.h           |  1 +
>>  4 files changed, 112 insertions(+)
>>

Re: [RFC PATCH 0/2] Introduce ancillary links

[Hans de Goede]

Hi Daniel,

On 11/26/21 08:58, Daniel Scally wrote:
> 
> On 26/11/2021 02:59, Laurent Pinchart wrote:
>> On Fri, Nov 26, 2021 at 12:16:01AM +0000, Daniel Scally wrote:
>>> Hello all
>>>
>>> This series is not yet ready to merge, but I wanted to share it as I know some
>>> other folks are working in similar areas at the moment (and I am including the
>>> libcamera devel list for the same reason)
>> Seems you forgot to CC libcamera-devel :-) Let's fix that on v2.
> 
> 
> Argh! Sorry, will do

First of all, thank you very much for this RFC series as well
as for the matching libcamera series.

For v2 of the series can you please also add Kate Hsuan to the
Cc (I've added there to the To of this email). Kate is a colleague
of me working on adding auto-focus support for IPU3 based setups
to libcamera.

Regards,

Hans

Re: [RFC PATCH 0/2] Introduce ancillary links

[Daniel Scally]

Hi Hans

On 26/11/2021 09:41, Hans de Goede wrote:
> Hi Daniel,
>
> On 11/26/21 08:58, Daniel Scally wrote:
>> On 26/11/2021 02:59, Laurent Pinchart wrote:
>>> On Fri, Nov 26, 2021 at 12:16:01AM +0000, Daniel Scally wrote:
>>>> Hello all
>>>>
>>>> This series is not yet ready to merge, but I wanted to share it as I know some
>>>> other folks are working in similar areas at the moment (and I am including the
>>>> libcamera devel list for the same reason)
>>> Seems you forgot to CC libcamera-devel :-) Let's fix that on v2.
>>
>> Argh! Sorry, will do
> First of all, thank you very much for this RFC series as well
> as for the matching libcamera series.


My pleasure

> For v2 of the series can you please also add Kate Hsuan to the
> Cc (I've added there to the To of this email). Kate is a colleague
> of me working on adding auto-focus support for IPU3 based setups
> to libcamera.


Oops again; I had intended to do that as well...sorry Kate! I'll be more
careful with the CC list next time

>
> Regards,
>
> Hans
>

Re: [RFC PATCH 0/2] Introduce ancillary links

[Kate Hsuan]

On Fri, Nov 26, 2021 at 5:46 PM Daniel Scally <djrscally@gmail.com> wrote:
>
> Hi Hans
>
> On 26/11/2021 09:41, Hans de Goede wrote:
> > Hi Daniel,
> >
> > On 11/26/21 08:58, Daniel Scally wrote:
> >> On 26/11/2021 02:59, Laurent Pinchart wrote:
> >>> On Fri, Nov 26, 2021 at 12:16:01AM +0000, Daniel Scally wrote:
> >>>> Hello all
> >>>>
> >>>> This series is not yet ready to merge, but I wanted to share it as I know some
> >>>> other folks are working in similar areas at the moment (and I am including the
> >>>> libcamera devel list for the same reason)
> >>> Seems you forgot to CC libcamera-devel :-) Let's fix that on v2.
> >>
> >> Argh! Sorry, will do
> > First of all, thank you very much for this RFC series as well
> > as for the matching libcamera series.
>
>
> My pleasure
>
> > For v2 of the series can you please also add Kate Hsuan to the
> > Cc (I've added there to the To of this email). Kate is a colleague
> > of me working on adding auto-focus support for IPU3 based setups
> > to libcamera.
>
>
> Oops again; I had intended to do that as well...sorry Kate! I'll be more
> careful with the CC list next time

It's OK. Thank you :)

>
> >
> > Regards,
> >
> > Hans
> >
>


-- 
BR,
Kate

Re: [RFC PATCH 2/2] media: v4l2-async: Create links during v4l2_async_match_notify()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3856 bytes --]

Hi Daniel,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on media-tree/master]
[also build test ERROR on v5.16-rc2 next-20211126]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Daniel-Scally/Introduce-ancillary-links/20211126-081931
base:   git://linuxtv.org/media_tree.git master
config: nios2-buildonly-randconfig-r001-20211126 (https://download.01.org/0day-ci/archive/20211126/202111262100.6gHZZCcB-lkp(a)intel.com/config)
compiler: nios2-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/1682c9da549082ef83e79db3659b255fe1370307
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Scally/Introduce-ancillary-links/20211126-081931
        git checkout 1682c9da549082ef83e79db3659b255fe1370307
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=nios2 SHELL=/bin/bash drivers/media/v4l2-core/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/media/v4l2-core/v4l2-async.c: In function '__v4l2_async_create_ancillary_link':
>> drivers/media/v4l2-core/v4l2-async.c:284:15: error: 'struct v4l2_subdev' has no member named 'entity'
     284 |         if (sd->entity.function != MEDIA_ENT_F_LENS &&
         |               ^~
   drivers/media/v4l2-core/v4l2-async.c:285:15: error: 'struct v4l2_subdev' has no member named 'entity'
     285 |             sd->entity.function != MEDIA_ENT_F_FLASH)
         |               ^~
   drivers/media/v4l2-core/v4l2-async.c:288:57: error: 'struct v4l2_subdev' has no member named 'entity'
     288 |         link = media_create_ancillary_link(&notifier->sd->entity, &sd->entity,
         |                                                         ^~
   drivers/media/v4l2-core/v4l2-async.c:288:70: error: 'struct v4l2_subdev' has no member named 'entity'
     288 |         link = media_create_ancillary_link(&notifier->sd->entity, &sd->entity,
         |                                                                      ^~
   drivers/media/v4l2-core/v4l2-async.c: In function 'v4l2_async_try_create_links':
   drivers/media/v4l2-core/v4l2-async.c:309:29: error: 'struct v4l2_subdev' has no member named 'entity'
     309 |         switch (notifier->sd->entity.function) {
         |                             ^~
   drivers/media/v4l2-core/v4l2-async.c:315:1: error: control reaches end of non-void function [-Werror=return-type]
     315 | }
         | ^
   cc1: some warnings being treated as errors


vim +284 drivers/media/v4l2-core/v4l2-async.c

   277	
   278	static int
   279	__v4l2_async_create_ancillary_link(struct v4l2_async_notifier *notifier,
   280					   struct v4l2_subdev *sd)
   281	{
   282		struct media_link *link;
   283	
 > 284		if (sd->entity.function != MEDIA_ENT_F_LENS &&
   285		    sd->entity.function != MEDIA_ENT_F_FLASH)
   286			return -EINVAL;
   287	
   288		link = media_create_ancillary_link(&notifier->sd->entity, &sd->entity,
   289						   MEDIA_LNK_FL_ENABLED |
   290						   MEDIA_LNK_FL_IMMUTABLE);
   291	
   292		return IS_ERR(link) ? PTR_ERR(link) : 0;
   293	}
   294	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org