[PATCH v2 0/6] Introduce ancillary links

[PATCH v2 0/6] Introduce ancillary links

[Daniel Scally]

Hello all

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.

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.

Series-level changes since v1:

	- New patch adding some documentation to the uAPI pages.

Dan

Daniel Scally (6):
  media: entity: Skip non-data links in graph iteration
  media: media.h: Add new media link type
  media: docs: Add entries documenting ancillary links
  media: entity: Add link_type_name() helper
  media: entity: Add support for ancillary links
  media: v4l2-async: Create links during v4l2_async_match_notify()

 .../media/mediactl/media-controller-model.rst |  6 ++
 .../media/mediactl/media-types.rst            |  9 ++-
 drivers/media/mc/mc-entity.c                  | 46 ++++++++++++++-
 drivers/media/v4l2-core/v4l2-async.c          | 56 +++++++++++++++++++
 include/media/media-entity.h                  | 21 +++++++
 include/uapi/linux/media.h                    |  1 +
 6 files changed, 135 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH v2 1/6] media: entity: Skip non-data links in graph iteration

[Daniel Scally]

When iterating over the media graph, don't follow links that are not
data links.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---

Changes since v1:

	- Moved to the head of the series
	- s/pad-to-pad/data (Sakari)
	- Dropped the debug message (Laurent)

Changes since the rfc:

	- new patch

 drivers/media/mc/mc-entity.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index f40f41977142..2a6e16fb0048 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -313,6 +313,12 @@ static void media_graph_walk_iter(struct media_graph *graph)
 
 	link = list_entry(link_top(graph), typeof(*link), list);
 
+	/* If the link is not a data link, don't follow it */
+	if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) != MEDIA_LNK_FL_DATA_LINK) {
+		link_top(graph) = link_top(graph)->next;
+		return;
+	}
+
 	/* The link is not enabled so we do not follow. */
 	if (!(link->flags & MEDIA_LNK_FL_ENABLED)) {
 		link_top(graph) = link_top(graph)->next;
-- 
2.25.1

[PATCH v2 2/6] media: media.h: Add new media link type

[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
to introduce the concept of these ancillary links.

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

	- None

changes since the rfc:

	- Split out into its own patch (mostly so it can be followed by patch
	#3, which corrects some media-core code that is otherwise broken by the
	new links)

 include/uapi/linux/media.h | 1 +
 1 file changed, 1 insertion(+)

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

Re: [PATCH v2 2/6] media: media.h: Add new media link type

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Sun, Jan 30, 2022 at 11:58:17PM +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
> to introduce the concept of these ancillary links.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes since v1:
> 
> 	- None
> 
> changes since the rfc:
> 
> 	- Split out into its own patch (mostly so it can be followed by patch
> 	#3, which corrects some media-core code that is otherwise broken by the
> 	new links)
> 
>  include/uapi/linux/media.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> 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)

I'd squash this with 3/6. Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  struct media_link_desc {
>  	struct media_pad_desc source;

-- 
Regards,

Laurent Pinchart

[PATCH v2 3/6] media: docs: Add entries documenting ancillary links

[Daniel Scally]

Add some elements to the uAPI documentation to explain the new link
type, their purpose and some aspects of their current implementation.

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

	- New patch

 .../media/mediactl/media-controller-model.rst            | 6 ++++++
 .../userspace-api/media/mediactl/media-types.rst         | 9 ++++++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/Documentation/userspace-api/media/mediactl/media-controller-model.rst b/Documentation/userspace-api/media/mediactl/media-controller-model.rst
index 222cb99debb5..f77cb9d952e5 100644
--- a/Documentation/userspace-api/media/mediactl/media-controller-model.rst
+++ b/Documentation/userspace-api/media/mediactl/media-controller-model.rst
@@ -33,3 +33,9 @@ are:
 
 -  An **interface link** is a point-to-point bidirectional control
    connection between a Linux Kernel interface and an entity.
+
+- An **ancillary link** is a point-to-point connection describing a physical
+  relationship between two entities. For example this could represent the
+  fact that a particular camera sensor and lens controller form a single
+  physical module, meaning this lens controller drives the lens for this
+  camera sensor.
\ No newline at end of file
diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst
index 0a26397bd01d..d69bae359e5b 100644
--- a/Documentation/userspace-api/media/mediactl/media-types.rst
+++ b/Documentation/userspace-api/media/mediactl/media-types.rst
@@ -413,7 +413,7 @@ must be set for every pad.
 
     *  -  ``MEDIA_LNK_FL_LINK_TYPE``
        -  This is a bitmask that defines the type of the link. Currently,
-	  two types of links are supported:
+	  three types of links are supported:
 
 	  .. _MEDIA-LNK-FL-DATA-LINK:
 
@@ -423,3 +423,10 @@ must be set for every pad.
 
 	  ``MEDIA_LNK_FL_INTERFACE_LINK`` if the link is between an
 	  interface and an entity
+
+	  .. _MEDIA-LNK-FL-ANCILLARY-LINK:
+
+	  ``MEDIA_LNK_FL_ANCILLARY_LINK`` if the link is between two
+	  different entities. This at present implies both MEDIA_LNK_FL_ENABLED
+	  and MEDIA_LNK_FL_IMMUTABLE, however applications should not rely on
+	  that being the case in the future.
-- 
2.25.1

Re: [PATCH v2 3/6] media: docs: Add entries documenting ancillary links

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Sun, Jan 30, 2022 at 11:58:18PM +0000, Daniel Scally wrote:
> Add some elements to the uAPI documentation to explain the new link
> type, their purpose and some aspects of their current implementation.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes since v1:
> 
> 	- New patch
> 
>  .../media/mediactl/media-controller-model.rst            | 6 ++++++
>  .../userspace-api/media/mediactl/media-types.rst         | 9 ++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/userspace-api/media/mediactl/media-controller-model.rst b/Documentation/userspace-api/media/mediactl/media-controller-model.rst
> index 222cb99debb5..f77cb9d952e5 100644
> --- a/Documentation/userspace-api/media/mediactl/media-controller-model.rst
> +++ b/Documentation/userspace-api/media/mediactl/media-controller-model.rst
> @@ -33,3 +33,9 @@ are:
>  
>  -  An **interface link** is a point-to-point bidirectional control
>     connection between a Linux Kernel interface and an entity.
> +
> +- An **ancillary link** is a point-to-point connection describing a physical
> +  relationship between two entities. For example this could represent the
> +  fact that a particular camera sensor and lens controller form a single
> +  physical module, meaning this lens controller drives the lens for this
> +  camera sensor.
> \ No newline at end of file
> diff --git a/Documentation/userspace-api/media/mediactl/media-types.rst b/Documentation/userspace-api/media/mediactl/media-types.rst
> index 0a26397bd01d..d69bae359e5b 100644
> --- a/Documentation/userspace-api/media/mediactl/media-types.rst
> +++ b/Documentation/userspace-api/media/mediactl/media-types.rst
> @@ -413,7 +413,7 @@ must be set for every pad.
>  
>      *  -  ``MEDIA_LNK_FL_LINK_TYPE``
>         -  This is a bitmask that defines the type of the link. Currently,
> -	  two types of links are supported:
> +	  three types of links are supported:

Let's avoid having to patch this every time:

       -  This is a bitmask that defines the type of the link. The following
	  types of links are supported:

>  
>  	  .. _MEDIA-LNK-FL-DATA-LINK:
>  
> @@ -423,3 +423,10 @@ must be set for every pad.
>  
>  	  ``MEDIA_LNK_FL_INTERFACE_LINK`` if the link is between an
>  	  interface and an entity
> +
> +	  .. _MEDIA-LNK-FL-ANCILLARY-LINK:
> +
> +	  ``MEDIA_LNK_FL_ANCILLARY_LINK`` if the link is between two
> +	  different entities. This at present implies both MEDIA_LNK_FL_ENABLED
> +	  and MEDIA_LNK_FL_IMMUTABLE, however applications should not rely on
> +	  that being the case in the future.

Let's describe what the link represents:

	  ``MEDIA_LNK_FL_ANCILLARY_LINK`` for links that represent a physical
	  relationship between two entities.

You could also update the previous items similarly:

	  ``MEDIA_LNK_FL_DATA_LINK`` for links thatrepresent a data connection
	  between two pads.

 	  ``MEDIA_LNK_FL_INTERFACE_LINK`` for links that associate an entity
	  and its interface.

Let's also not tell that it impllies ENABLED and IMMUTABLE if
applications shouldn't rely on this:

	  The link may or may not be immutable, applications must not assume
	  either case as always being true.

-- 
Regards,

Laurent Pinchart

[PATCH v2 4/6] media: entity: Add link_type_name() helper

[Daniel Scally]

Now we have three types of media link, printing the right name during
debug output is slightly more complicated. Add a helper function to
make it easier.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
Changes since v1:

	- renamed function to link_type_name() (Laurent)

Changes since the rfc:

	- new patch

 drivers/media/mc/mc-entity.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index 2a6e16fb0048..29d1285c805a 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -60,6 +60,20 @@ static inline const char *intf_type(struct media_interface *intf)
 	}
 };
 
+static inline const char *link_type_name(struct media_link *link)
+{
+	switch (link->flags & MEDIA_LNK_FL_LINK_TYPE) {
+	case MEDIA_LNK_FL_DATA_LINK:
+		return "data";
+	case MEDIA_LNK_FL_INTERFACE_LINK:
+		return "interface";
+	case MEDIA_LNK_FL_ANCILLARY_LINK:
+		return "ancillary";
+	default:
+		return "unknown";
+	}
+}
+
 __must_check int __media_entity_enum_init(struct media_entity_enum *ent_enum,
 					  int idx_max)
 {
@@ -107,9 +121,7 @@ static void dev_dbg_obj(const char *event_name,  struct media_gobj *gobj)
 
 		dev_dbg(gobj->mdev->dev,
 			"%s id %u: %s link id %u ==> id %u\n",
-			event_name, media_id(gobj),
-			media_type(link->gobj0) == MEDIA_GRAPH_PAD ?
-				"data" : "interface",
+			event_name, media_id(gobj), link_type_name(link),
 			media_id(link->gobj0),
 			media_id(link->gobj1));
 		break;
-- 
2.25.1

[PATCH v2 5/6] media: entity: Add support for ancillary links

[Daniel Scally]

Add functions to create ancillary links, so that they don't need to
be manually created by users.

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

Changes since v1:

	- Hardcoded MEDIA_LINK_FL_IMMUTABLE and MEDIA_LINK_FL_ENABLED (Laurent)

Changes since the rfc:

	- (Laurent) Set gobj0 and gobj1 directly instead of the other union
	members
	- (Laurent) Added MEDIA_LNK_FL_IMMUTABLE to the kerneldoc for the new
	create function

 drivers/media/mc/mc-entity.c | 22 ++++++++++++++++++++++
 include/media/media-entity.h | 21 +++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index 29d1285c805a..7bf2c73a3886 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -1050,3 +1050,25 @@ 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)
+{
+	struct media_link *link;
+
+	link = media_add_link(&primary->links);
+	if (!link)
+		return ERR_PTR(-ENOMEM);
+
+	link->gobj0 = &primary->graph_obj;
+	link->gobj1 = &ancillary->graph_obj;
+	link->flags = MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED |
+		      MEDIA_LNK_FL_ANCILLARY_LINK;
+
+	/* Initialize graph object embedded in 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);
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index fea489f03d57..afeda41ece4c 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -1104,6 +1104,27 @@ void media_remove_intf_links(struct media_interface *intf);
  * it will issue a call to @operation\(@entity, @args\).
  */
 
+/**
+ * media_create_ancillary_link() - create an ancillary link between two
+ *				   instances of &media_entity
+ *
+ * @primary:	pointer to the primary &media_entity
+ * @ancillary:	pointer to the ancillary &media_entity
+ *
+ * Create an ancillary link between two entities, indicating that they
+ * represent two 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.
+ *
+ * The function sets both MEDIA_LNK_FL_ENABLED and MEDIA_LNK_FL_IMMUTABLE for
+ * the new link. This behaviour may be subject to change in the future, so
+ * userspace applications using ancillary links should ensure that ancillary
+ * links are enabled when in use.
+ */
+struct media_link *
+media_create_ancillary_link(struct media_entity *primary,
+			    struct media_entity *ancillary);
+
 #define media_entity_call(entity, operation, args...)			\
 	(((entity)->ops && (entity)->ops->operation) ?			\
 	 (entity)->ops->operation((entity) , ##args) : -ENOIOCTLCMD)
-- 
2.25.1

Re: [PATCH v2 5/6] media: entity: Add support for ancillary links

[kernel test robot]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on media-tree/master]
[also build test WARNING on v5.17-rc2 next-20220131]
[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/20220131-080041
base:   git://linuxtv.org/media_tree.git master
reproduce: make htmldocs

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

>> include/media/media-entity.h:1126: warning: expecting prototype for media_entity_call(). Prototype was for media_create_ancillary_link() instead

vim +1126 include/media/media-entity.h

  1094	
  1095	/**
  1096	 * media_entity_call - Calls a struct media_entity_operations operation on
  1097	 *	an entity
  1098	 *
  1099	 * @entity: entity where the @operation will be called
  1100	 * @operation: type of the operation. Should be the name of a member of
  1101	 *	struct &media_entity_operations.
  1102	 *
  1103	 * This helper function will check if @operation is not %NULL. On such case,
  1104	 * it will issue a call to @operation\(@entity, @args\).
  1105	 */
  1106	
  1107	/**
  1108	 * media_create_ancillary_link() - create an ancillary link between two
  1109	 *				   instances of &media_entity
  1110	 *
  1111	 * @primary:	pointer to the primary &media_entity
  1112	 * @ancillary:	pointer to the ancillary &media_entity
  1113	 *
  1114	 * Create an ancillary link between two entities, indicating that they
  1115	 * represent two connected pieces of hardware that form a single logical unit.
  1116	 * A typical example is a camera lens being linked to the sensor that it is
  1117	 * supporting.
  1118	 *
  1119	 * The function sets both MEDIA_LNK_FL_ENABLED and MEDIA_LNK_FL_IMMUTABLE for
  1120	 * the new link. This behaviour may be subject to change in the future, so
  1121	 * userspace applications using ancillary links should ensure that ancillary
  1122	 * links are enabled when in use.
  1123	 */
  1124	struct media_link *
  1125	media_create_ancillary_link(struct media_entity *primary,
> 1126				    struct media_entity *ancillary);
  1127	

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

Re: [PATCH v2 5/6] media: entity: Add support for ancillary links

[kernel test robot]

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

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on media-tree/master]
[also build test WARNING on v5.17-rc2 next-20220131]
[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/20220131-080041
base:   git://linuxtv.org/media_tree.git master
reproduce: make htmldocs

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

>> include/media/media-entity.h:1126: warning: expecting prototype for media_entity_call(). Prototype was for media_create_ancillary_link() instead

vim +1126 include/media/media-entity.h

  1094	
  1095	/**
  1096	 * media_entity_call - Calls a struct media_entity_operations operation on
  1097	 *	an entity
  1098	 *
  1099	 * @entity: entity where the @operation will be called
  1100	 * @operation: type of the operation. Should be the name of a member of
  1101	 *	struct &media_entity_operations.
  1102	 *
  1103	 * This helper function will check if @operation is not %NULL. On such case,
  1104	 * it will issue a call to @operation\(@entity, @args\).
  1105	 */
  1106	
  1107	/**
  1108	 * media_create_ancillary_link() - create an ancillary link between two
  1109	 *				   instances of &media_entity
  1110	 *
  1111	 * @primary:	pointer to the primary &media_entity
  1112	 * @ancillary:	pointer to the ancillary &media_entity
  1113	 *
  1114	 * Create an ancillary link between two entities, indicating that they
  1115	 * represent two connected pieces of hardware that form a single logical unit.
  1116	 * A typical example is a camera lens being linked to the sensor that it is
  1117	 * supporting.
  1118	 *
  1119	 * The function sets both MEDIA_LNK_FL_ENABLED and MEDIA_LNK_FL_IMMUTABLE for
  1120	 * the new link. This behaviour may be subject to change in the future, so
  1121	 * userspace applications using ancillary links should ensure that ancillary
  1122	 * links are enabled when in use.
  1123	 */
  1124	struct media_link *
  1125	media_create_ancillary_link(struct media_entity *primary,
> 1126				    struct media_entity *ancillary);
  1127	

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

Re: [PATCH v2 5/6] media: entity: Add support for ancillary links

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Sun, Jan 30, 2022 at 11:58:20PM +0000, Daniel Scally wrote:
> Add functions to create ancillary links, so that they don't need to
> be manually created by users.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> 
> Changes since v1:
> 
> 	- Hardcoded MEDIA_LINK_FL_IMMUTABLE and MEDIA_LINK_FL_ENABLED (Laurent)
> 
> Changes since the rfc:
> 
> 	- (Laurent) Set gobj0 and gobj1 directly instead of the other union
> 	members
> 	- (Laurent) Added MEDIA_LNK_FL_IMMUTABLE to the kerneldoc for the new
> 	create function
> 
>  drivers/media/mc/mc-entity.c | 22 ++++++++++++++++++++++
>  include/media/media-entity.h | 21 +++++++++++++++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index 29d1285c805a..7bf2c73a3886 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -1050,3 +1050,25 @@ 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)
> +{
> +	struct media_link *link;
> +
> +	link = media_add_link(&primary->links);
> +	if (!link)
> +		return ERR_PTR(-ENOMEM);
> +
> +	link->gobj0 = &primary->graph_obj;
> +	link->gobj1 = &ancillary->graph_obj;
> +	link->flags = MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED |
> +		      MEDIA_LNK_FL_ANCILLARY_LINK;
> +
> +	/* Initialize graph object embedded in 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);
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index fea489f03d57..afeda41ece4c 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -1104,6 +1104,27 @@ void media_remove_intf_links(struct media_interface *intf);
>   * it will issue a call to @operation\(@entity, @args\).
>   */
>  
> +/**
> + * media_create_ancillary_link() - create an ancillary link between two
> + *				   instances of &media_entity
> + *
> + * @primary:	pointer to the primary &media_entity
> + * @ancillary:	pointer to the ancillary &media_entity
> + *
> + * Create an ancillary link between two entities, indicating that they
> + * represent two connected pieces of hardware that form a single logical unit.

Here you say logical unit, while in patch 3/6 you use the term "physical
relationship". I think I'd go for "logical" there too.

> + * A typical example is a camera lens being linked to the sensor that it is

s/lens/lens controller/

> + * supporting.
> + *
> + * The function sets both MEDIA_LNK_FL_ENABLED and MEDIA_LNK_FL_IMMUTABLE for
> + * the new link. This behaviour may be subject to change in the future, so
> + * userspace applications using ancillary links should ensure that ancillary
> + * links are enabled when in use.

I'd drop the last two lines as this is kernel documentation, not
userspace documentation.

> + */
> +struct media_link *
> +media_create_ancillary_link(struct media_entity *primary,
> +			    struct media_entity *ancillary);

As reported by the kernel buildbot, this should go after
media_entity_call().

> +
>  #define media_entity_call(entity, operation, args...)			\
>  	(((entity)->ops && (entity)->ops->operation) ?			\
>  	 (entity)->ops->operation((entity) , ##args) : -ENOIOCTLCMD)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/6] media: entity: Add support for ancillary links

[Laurent Pinchart]

On Thu, Feb 03, 2022 at 01:32:28AM +0200, Laurent Pinchart wrote:
> Hi Dan,
> 
> Thank you for the patch.
> 
> On Sun, Jan 30, 2022 at 11:58:20PM +0000, Daniel Scally wrote:
> > Add functions to create ancillary links, so that they don't need to
> > be manually created by users.
> > 
> > Signed-off-by: Daniel Scally <djrscally@gmail.com>
> > ---
> > 
> > Changes since v1:
> > 
> > 	- Hardcoded MEDIA_LINK_FL_IMMUTABLE and MEDIA_LINK_FL_ENABLED (Laurent)
> > 
> > Changes since the rfc:
> > 
> > 	- (Laurent) Set gobj0 and gobj1 directly instead of the other union
> > 	members
> > 	- (Laurent) Added MEDIA_LNK_FL_IMMUTABLE to the kerneldoc for the new
> > 	create function
> > 
> >  drivers/media/mc/mc-entity.c | 22 ++++++++++++++++++++++
> >  include/media/media-entity.h | 21 +++++++++++++++++++++
> >  2 files changed, 43 insertions(+)
> > 
> > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > index 29d1285c805a..7bf2c73a3886 100644
> > --- a/drivers/media/mc/mc-entity.c
> > +++ b/drivers/media/mc/mc-entity.c
> > @@ -1050,3 +1050,25 @@ 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)
> > +{
> > +	struct media_link *link;
> > +
> > +	link = media_add_link(&primary->links);
> > +	if (!link)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	link->gobj0 = &primary->graph_obj;
> > +	link->gobj1 = &ancillary->graph_obj;
> > +	link->flags = MEDIA_LNK_FL_IMMUTABLE | MEDIA_LNK_FL_ENABLED |
> > +		      MEDIA_LNK_FL_ANCILLARY_LINK;
> > +
> > +	/* Initialize graph object embedded in 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);
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index fea489f03d57..afeda41ece4c 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -1104,6 +1104,27 @@ void media_remove_intf_links(struct media_interface *intf);
> >   * it will issue a call to @operation\(@entity, @args\).
> >   */
> >  
> > +/**
> > + * media_create_ancillary_link() - create an ancillary link between two
> > + *				   instances of &media_entity
> > + *
> > + * @primary:	pointer to the primary &media_entity
> > + * @ancillary:	pointer to the ancillary &media_entity
> > + *
> > + * Create an ancillary link between two entities, indicating that they
> > + * represent two connected pieces of hardware that form a single logical unit.
> 
> Here you say logical unit, while in patch 3/6 you use the term "physical
> relationship". I think I'd go for "logical" there too.
> 
> > + * A typical example is a camera lens being linked to the sensor that it is
> 
> s/lens/lens controller/
> 
> > + * supporting.
> > + *
> > + * The function sets both MEDIA_LNK_FL_ENABLED and MEDIA_LNK_FL_IMMUTABLE for
> > + * the new link. This behaviour may be subject to change in the future, so
> > + * userspace applications using ancillary links should ensure that ancillary
> > + * links are enabled when in use.
> 
> I'd drop the last two lines as this is kernel documentation, not
> userspace documentation.
> 
> > + */
> > +struct media_link *
> > +media_create_ancillary_link(struct media_entity *primary,
> > +			    struct media_entity *ancillary);
> 
> As reported by the kernel buildbot, this should go after
> media_entity_call().

And I forgot to add

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > +
> >  #define media_entity_call(entity, operation, args...)			\
> >  	(((entity)->ops && (entity)->ops->operation) ?			\
> >  	 (entity)->ops->operation((entity) , ##args) : -ENOIOCTLCMD)

-- 
Regards,

Laurent Pinchart

[PATCH v2 6/6] 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>
---
Changes since v1:

	- Added #ifdef guards for CONFIG_MEDIA_CONTROLLER
	- Some spelling and nomenclature cleanup (Laurent)

Changes since the rfc:

	- None

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

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 0404267f1ae4..8980534e755e 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -275,6 +275,50 @@ 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 = NULL;
+
+#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
+
+	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);
+
+#endif
+
+	return IS_ERR(link) ? PTR_ERR(link) : 0;
+}
+
+/*
+ * Create 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 camera sensor and the subdev is a lens controller.
+ *
+ * TODO: Create data 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 IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
+
+	if (!notifier->sd)
+		return 0;
+
+	switch (notifier->sd->entity.function) {
+	case MEDIA_ENT_F_CAM_SENSOR:
+		return __v4l2_async_create_ancillary_link(notifier, sd);
+	}
+
+#endif
+	return 0;
+}
+
 static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
 				   struct v4l2_device *v4l2_dev,
 				   struct v4l2_subdev *sd,
@@ -293,6 +337,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: [PATCH v2 6/6] media: v4l2-async: Create links during v4l2_async_match_notify()

[Sakari Ailus]

Hi Daniel,

Thanks for the update.

On Sun, Jan 30, 2022 at 11:58:21PM +0000, Daniel Scally wrote:
> 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>
> ---
> Changes since v1:
> 
> 	- Added #ifdef guards for CONFIG_MEDIA_CONTROLLER
> 	- Some spelling and nomenclature cleanup (Laurent)
> 
> Changes since the rfc:
> 
> 	- None
> 
>  drivers/media/v4l2-core/v4l2-async.c | 56 ++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 0404267f1ae4..8980534e755e 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -275,6 +275,50 @@ 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 = NULL;
> +
> +#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
> +
> +	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);
> +
> +#endif
> +
> +	return IS_ERR(link) ? PTR_ERR(link) : 0;
> +}
> +
> +/*
> + * Create 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 camera sensor and the subdev is a lens controller.

I think I'd rather change this so that ancillary links are created for lens
and flash subdevs, independently of the function of the notifier subdev.

Are there cases where this would go wrong currently, or in the future? I
can't think of any right now at least. I guess we could add more
information in the future to convey here if needed.

> + *
> + * TODO: Create data 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)

How about calling this v4l2_async_create_ancillary_links()?

> +{
> +#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
> +
> +	if (!notifier->sd)
> +		return 0;
> +
> +	switch (notifier->sd->entity.function) {
> +	case MEDIA_ENT_F_CAM_SENSOR:
> +		return __v4l2_async_create_ancillary_link(notifier, sd);
> +	}
> +
> +#endif
> +	return 0;
> +}
> +
>  static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>  				   struct v4l2_device *v4l2_dev,
>  				   struct v4l2_subdev *sd,
> @@ -293,6 +337,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) {

The notifier's bound function has been called already. The unbound function
needs to be thus called here, too.

> +		v4l2_device_unregister_subdev(sd);
> +		return ret;
> +	}
> +
>  	/* Remove from the waiting list */
>  	list_del(&asd->list);
>  	sd->asd = asd;

-- 
Kind regards,

Sakari Ailus

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

[Daniel Scally]

Hi Sakari

On 02/02/2022 16:38, Sakari Ailus wrote:
> Hi Daniel,
>
> Thanks for the update.
>
> On Sun, Jan 30, 2022 at 11:58:21PM +0000, Daniel Scally wrote:
>> 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>
>> ---
>> Changes since v1:
>>
>> 	- Added #ifdef guards for CONFIG_MEDIA_CONTROLLER
>> 	- Some spelling and nomenclature cleanup (Laurent)
>>
>> Changes since the rfc:
>>
>> 	- None
>>
>>  drivers/media/v4l2-core/v4l2-async.c | 56 ++++++++++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>> index 0404267f1ae4..8980534e755e 100644
>> --- a/drivers/media/v4l2-core/v4l2-async.c
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -275,6 +275,50 @@ 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 = NULL;
>> +
>> +#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
>> +
>> +	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);
>> +
>> +#endif
>> +
>> +	return IS_ERR(link) ? PTR_ERR(link) : 0;
>> +}
>> +
>> +/*
>> + * Create 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 camera sensor and the subdev is a lens controller.
> I think I'd rather change this so that ancillary links are created for lens
> and flash subdevs, independently of the function of the notifier subdev.
>
> Are there cases where this would go wrong currently, or in the future? I
> can't think of any right now at least. I guess we could add more
> information in the future to convey here if needed.
I don't think doing that would go wrong anyhow...at least not that I
could think of at the minute. My plan was to add a new function like
__v4l2_async_create_data_links() and call that (from
v4l2_async_try_create_links()) where the function of the notifier subdev
was MEDIA_ENT_F_VID_IF_BRIDGE...you think we shouldn't be doing that? Or
just that it should be separate?
>> + *
>> + * TODO: Create data 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)
> How about calling this v4l2_async_create_ancillary_links()?


Sure

>
>> +{
>> +#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
>> +
>> +	if (!notifier->sd)
>> +		return 0;
>> +
>> +	switch (notifier->sd->entity.function) {
>> +	case MEDIA_ENT_F_CAM_SENSOR:
>> +		return __v4l2_async_create_ancillary_link(notifier, sd);
>> +	}
>> +
>> +#endif
>> +	return 0;
>> +}
>> +
>>  static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>>  				   struct v4l2_device *v4l2_dev,
>>  				   struct v4l2_subdev *sd,
>> @@ -293,6 +337,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) {
> The notifier's bound function has been called already. The unbound function
> needs to be thus called here, too.


Thank you, I'll do that in the next version

>
>> +		v4l2_device_unregister_subdev(sd);
>> +		return ret;
>> +	}
>> +
>>  	/* Remove from the waiting list */
>>  	list_del(&asd->list);
>>  	sd->asd = asd;

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

[Daniel Scally]

Hi Sakari

On 02/02/2022 21:48, Daniel Scally wrote:
> Hi Sakari
>
> On 02/02/2022 16:38, Sakari Ailus wrote:
>> Hi Daniel,
>>
>> Thanks for the update.
>>
>> On Sun, Jan 30, 2022 at 11:58:21PM +0000, Daniel Scally wrote:
>>> 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>
>>> ---
>>> Changes since v1:
>>>
>>> 	- Added #ifdef guards for CONFIG_MEDIA_CONTROLLER
>>> 	- Some spelling and nomenclature cleanup (Laurent)
>>>
>>> Changes since the rfc:
>>>
>>> 	- None
>>>
>>>  drivers/media/v4l2-core/v4l2-async.c | 56 ++++++++++++++++++++++++++++
>>>  1 file changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>>> index 0404267f1ae4..8980534e755e 100644
>>> --- a/drivers/media/v4l2-core/v4l2-async.c
>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>>> @@ -275,6 +275,50 @@ 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 = NULL;
>>> +
>>> +#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
>>> +
>>> +	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);
>>> +
>>> +#endif
>>> +
>>> +	return IS_ERR(link) ? PTR_ERR(link) : 0;
>>> +}
>>> +
>>> +/*
>>> + * Create 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 camera sensor and the subdev is a lens controller.
>> I think I'd rather change this so that ancillary links are created for lens
>> and flash subdevs, independently of the function of the notifier subdev.
>>
>> Are there cases where this would go wrong currently, or in the future? I
>> can't think of any right now at least. I guess we could add more
>> information in the future to convey here if needed.
> I don't think doing that would go wrong anyhow...at least not that I
> could think of at the minute. My plan was to add a new function like
> __v4l2_async_create_data_links() and call that (from
> v4l2_async_try_create_links()) where the function of the notifier subdev
> was MEDIA_ENT_F_VID_IF_BRIDGE...you think we shouldn't be doing that? Or
> just that it should be separate?


Gentle ping for this part :)

>>> + *
>>> + * TODO: Create data 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)
>> How about calling this v4l2_async_create_ancillary_links()?
>
> Sure
>
>>> +{
>>> +#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
>>> +
>>> +	if (!notifier->sd)
>>> +		return 0;
>>> +
>>> +	switch (notifier->sd->entity.function) {
>>> +	case MEDIA_ENT_F_CAM_SENSOR:
>>> +		return __v4l2_async_create_ancillary_link(notifier, sd);
>>> +	}
>>> +
>>> +#endif
>>> +	return 0;
>>> +}
>>> +
>>>  static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>>>  				   struct v4l2_device *v4l2_dev,
>>>  				   struct v4l2_subdev *sd,
>>> @@ -293,6 +337,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) {
>> The notifier's bound function has been called already. The unbound function
>> needs to be thus called here, too.
>
> Thank you, I'll do that in the next version
>
>>> +		v4l2_device_unregister_subdev(sd);
>>> +		return ret;
>>> +	}
>>> +
>>>  	/* Remove from the waiting list */
>>>  	list_del(&asd->list);
>>>  	sd->asd = asd;

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

[Sakari Ailus]

Hi Daniel,

Thanks for the ping.

On Wed, Feb 02, 2022 at 09:48:56PM +0000, Daniel Scally wrote:
> Hi Sakari
> 
> On 02/02/2022 16:38, Sakari Ailus wrote:
> > Hi Daniel,
> >
> > Thanks for the update.
> >
> > On Sun, Jan 30, 2022 at 11:58:21PM +0000, Daniel Scally wrote:
> >> 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>
> >> ---
> >> Changes since v1:
> >>
> >> 	- Added #ifdef guards for CONFIG_MEDIA_CONTROLLER
> >> 	- Some spelling and nomenclature cleanup (Laurent)
> >>
> >> Changes since the rfc:
> >>
> >> 	- None
> >>
> >>  drivers/media/v4l2-core/v4l2-async.c | 56 ++++++++++++++++++++++++++++
> >>  1 file changed, 56 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> >> index 0404267f1ae4..8980534e755e 100644
> >> --- a/drivers/media/v4l2-core/v4l2-async.c
> >> +++ b/drivers/media/v4l2-core/v4l2-async.c
> >> @@ -275,6 +275,50 @@ 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 = NULL;
> >> +
> >> +#if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
> >> +
> >> +	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);
> >> +
> >> +#endif
> >> +
> >> +	return IS_ERR(link) ? PTR_ERR(link) : 0;
> >> +}
> >> +
> >> +/*
> >> + * Create 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 camera sensor and the subdev is a lens controller.
> > I think I'd rather change this so that ancillary links are created for lens
> > and flash subdevs, independently of the function of the notifier subdev.
> >
> > Are there cases where this would go wrong currently, or in the future? I
> > can't think of any right now at least. I guess we could add more
> > information in the future to convey here if needed.
> I don't think doing that would go wrong anyhow...at least not that I
> could think of at the minute. My plan was to add a new function like
> __v4l2_async_create_data_links() and call that (from
> v4l2_async_try_create_links()) where the function of the notifier subdev
> was MEDIA_ENT_F_VID_IF_BRIDGE...you think we shouldn't be doing that? Or
> just that it should be separate?

I'm not sure the function of the subdev should be involved with this.

The function is mainly used by the user space and different drivers tend to
use different functions. Of course there could (and should) be alignment on
this, but as you can have only a single function, there are bound to be
cases where you have to pick one that fits the best but does not entirely
match what the device is.

I see no problem doing this automatically, as long as it does not clash
with what drivers create by themselves. The local pad where the data link
is connected often comes from the driver --- same for the flags btw. --- so
some way needs to be provided for the driver to provide this information.

There's a callback for connecting endpoint with a pad the transmitter
drivers use, maybe that could be helpful?

-- 
Kind regards,

Sakari Ailus

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

[kernel test robot]

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

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20220130235821.48076-7-djrscally@gmail.com>
References: <20220130235821.48076-7-djrscally@gmail.com>
TO: Daniel Scally <djrscally@gmail.com>
TO: linux-media(a)vger.kernel.org
TO: libcamera-devel(a)lists.libcamera.org
CC: sakari.ailus(a)linux.intel.com
CC: laurent.pinchart(a)ideasonboard.com
CC: hanlinchen(a)chromium.org
CC: tfiga(a)chromium.org
CC: hdegoede(a)redhat.com
CC: kieran.bingham(a)ideasonboard.com
CC: hpa(a)redhat.com

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on media-tree/master]
[also build test WARNING on v5.17-rc2 next-20220128]
[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/20220131-080041
base:   git://linuxtv.org/media_tree.git master
:::::: branch date: 5 hours ago
:::::: commit date: 5 hours ago
config: powerpc-randconfig-c003-20220131 (https://download.01.org/0day-ci/archive/20220131/202201311324.q8ec5kGJ-lkp(a)intel.com/config)
compiler: powerpc-linux-gcc (GCC) 11.2.0

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


cocci warnings: (new ones prefixed by >>)
>> drivers/media/v4l2-core/v4l2-async.c:294:23-30: ERROR: PTR_ERR applied after initialization to constant on line 282

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

2cab00bb076b9f Sakari Ailus  2017-09-24  277  
8dc719a0ecab0c Daniel Scally 2022-01-30  278  static int
8dc719a0ecab0c Daniel Scally 2022-01-30  279  __v4l2_async_create_ancillary_link(struct v4l2_async_notifier *notifier,
8dc719a0ecab0c Daniel Scally 2022-01-30  280  				   struct v4l2_subdev *sd)
8dc719a0ecab0c Daniel Scally 2022-01-30  281  {
8dc719a0ecab0c Daniel Scally 2022-01-30 @282  	struct media_link *link = NULL;
8dc719a0ecab0c Daniel Scally 2022-01-30  283  
8dc719a0ecab0c Daniel Scally 2022-01-30  284  #if IS_ENABLED(CONFIG_MEDIA_CONTROLLER)
8dc719a0ecab0c Daniel Scally 2022-01-30  285  
8dc719a0ecab0c Daniel Scally 2022-01-30  286  	if (sd->entity.function != MEDIA_ENT_F_LENS &&
8dc719a0ecab0c Daniel Scally 2022-01-30  287  	    sd->entity.function != MEDIA_ENT_F_FLASH)
8dc719a0ecab0c Daniel Scally 2022-01-30  288  		return -EINVAL;
8dc719a0ecab0c Daniel Scally 2022-01-30  289  
8dc719a0ecab0c Daniel Scally 2022-01-30  290  	link = media_create_ancillary_link(&notifier->sd->entity, &sd->entity);
8dc719a0ecab0c Daniel Scally 2022-01-30  291  
8dc719a0ecab0c Daniel Scally 2022-01-30  292  #endif
8dc719a0ecab0c Daniel Scally 2022-01-30  293  
8dc719a0ecab0c Daniel Scally 2022-01-30 @294  	return IS_ERR(link) ? PTR_ERR(link) : 0;
8dc719a0ecab0c Daniel Scally 2022-01-30  295  }
8dc719a0ecab0c Daniel Scally 2022-01-30  296  

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