[PATCH v3 0/5] Introduce ancillary links

[PATCH v3 0/5] 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. 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 v2:

  - Squashed #2 and #3

Series-level changes since v1:

	- New patch adding some documentation to the uAPI pages.

Dan


Daniel Scally (5):
  media: entity: Skip non-data links in graph iteration
  media: media.h: Add new media link type
  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            | 17 +++++--
 drivers/media/mc/mc-entity.c                  | 46 +++++++++++++++++--
 drivers/media/v4l2-core/v4l2-async.c          | 31 +++++++++++++
 include/media/media-entity.h                  | 19 ++++++++
 include/uapi/linux/media.h                    |  1 +
 6 files changed, 112 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH v3 1/5] 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 v2:

	- None

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 b411f9796191..d0563ee4b28b 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -295,6 +295,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 v3 2/5] 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.

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

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
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)

 .../media/mediactl/media-controller-model.rst   |  6 ++++++
 .../media/mediactl/media-types.rst              | 17 ++++++++++++-----
 include/uapi/linux/media.h                      |  1 +
 3 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/Documentation/userspace-api/media/mediactl/media-controller-model.rst b/Documentation/userspace-api/media/mediactl/media-controller-model.rst
index 222cb99debb5..78bfdfb2a322 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 denoting that two
+  entities form a single logical unit. 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..60747251d409 100644
--- a/Documentation/userspace-api/media/mediactl/media-types.rst
+++ b/Documentation/userspace-api/media/mediactl/media-types.rst
@@ -412,14 +412,21 @@ must be set for every pad.
 	  is set by drivers and is read-only for applications.
 
     *  -  ``MEDIA_LNK_FL_LINK_TYPE``
-       -  This is a bitmask that defines the type of the link. Currently,
-	  two types of links are supported:
+       -  This is a bitmask that defines the type of the link. The following
+	  link types are currently supported:
 
 	  .. _MEDIA-LNK-FL-DATA-LINK:
 
-	  ``MEDIA_LNK_FL_DATA_LINK`` if the link is between two pads
+	  ``MEDIA_LNK_FL_DATA_LINK`` for links that represent a data connection
+     between two pads.
 
 	  .. _MEDIA-LNK-FL-INTERFACE-LINK:
 
-	  ``MEDIA_LNK_FL_INTERFACE_LINK`` if the link is between an
-	  interface and an entity
+	  ``MEDIA_LNK_FL_INTERFACE_LINK`` for links that associate an entity to its
+     interface.
+
+	  .. _MEDIA-LNK-FL-ANCILLARY-LINK:
+
+	  ``MEDIA_LNK_FL_ANCILLARY_LINK`` for links that represent a physical
+     relationship between two entities. The link may or may not be ummutable, so
+     applications must not assume either case.
\ No newline at end of file
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

[PATCH v3 3/5] 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 v2:

	- None

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 d0563ee4b28b..1a7d0a4fb9e8 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -44,6 +44,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)
 {
@@ -89,9 +103,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 v3 4/5] 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.

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

Changes since v2:

	- Fixed some typos and comment phrasing (Laurent)
	- Changed the position of the new function to go after media_entity_call()
	(Laurent)

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 | 19 +++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index 1a7d0a4fb9e8..d7e2f78a83cc 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -1032,3 +1032,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..2a58defc4886 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -1108,4 +1108,23 @@ void media_remove_intf_links(struct media_interface *intf);
 	(((entity)->ops && (entity)->ops->operation) ?			\
 	 (entity)->ops->operation((entity) , ##args) : -ENOIOCTLCMD)
 
+/**
+ * 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 controller 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.
+ */
+struct media_link *
+media_create_ancillary_link(struct media_entity *primary,
+			    struct media_entity *ancillary);
+
 #endif
-- 
2.25.1

[PATCH v3 5/5] 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 v2:

	- Stopped checking the notifier entity's function when creating the new
	links, and just create them whenever the subdev entity's function is either
	a lens controller or a flash. (Sakari)

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 | 31 ++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 0404267f1ae4..436bd6900fd8 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -275,6 +275,24 @@ 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_links(struct v4l2_async_notifier *n,
+					     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 0;
+
+	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
+
+#endif
+
+	return IS_ERR(link) ? PTR_ERR(link) : 0;
+}
+
 static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
 				   struct v4l2_device *v4l2_dev,
 				   struct v4l2_subdev *sd,
@@ -293,6 +311,19 @@ 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_create_ancillary_links(notifier, sd);
+	if (ret) {
+		v4l2_async_nf_call_unbind(notifier, sd, asd);
+		v4l2_device_unregister_subdev(sd);
+		return ret;
+	}
+
 	/* Remove from the waiting list */
 	list_del(&asd->list);
 	sd->asd = asd;
-- 
2.25.1

Re: [PATCH v3 1/5] media: entity: Skip non-data links in graph iteration

[Jean-Michel Hautbois]

Hi !

Thanks for the patch !

On 02/03/2022 23:03, Daniel Scally wrote:
> 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>

Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

> ---
> 
> Changes since v2:
> 
> 	- None
> 
> 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 b411f9796191..d0563ee4b28b 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -295,6 +295,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;

Re: [PATCH v3 2/5] media: media.h: Add new media link type

[Jean-Michel Hautbois]

Hi !

Thanks for the patch !

On 02/03/2022 23:03, 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.
> 
> Add some elements to the uAPI documentation to explain the new link
> type, their purpose and some aspects of their current implementation.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>

Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.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)
> 
>   .../media/mediactl/media-controller-model.rst   |  6 ++++++
>   .../media/mediactl/media-types.rst              | 17 ++++++++++++-----
>   include/uapi/linux/media.h                      |  1 +
>   3 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/userspace-api/media/mediactl/media-controller-model.rst b/Documentation/userspace-api/media/mediactl/media-controller-model.rst
> index 222cb99debb5..78bfdfb2a322 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 denoting that two
> +  entities form a single logical unit. 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..60747251d409 100644
> --- a/Documentation/userspace-api/media/mediactl/media-types.rst
> +++ b/Documentation/userspace-api/media/mediactl/media-types.rst
> @@ -412,14 +412,21 @@ must be set for every pad.
>   	  is set by drivers and is read-only for applications.
>   
>       *  -  ``MEDIA_LNK_FL_LINK_TYPE``
> -       -  This is a bitmask that defines the type of the link. Currently,
> -	  two types of links are supported:
> +       -  This is a bitmask that defines the type of the link. The following
> +	  link types are currently supported:
>   
>   	  .. _MEDIA-LNK-FL-DATA-LINK:
>   
> -	  ``MEDIA_LNK_FL_DATA_LINK`` if the link is between two pads
> +	  ``MEDIA_LNK_FL_DATA_LINK`` for links that represent a data connection
> +     between two pads.
>   
>   	  .. _MEDIA-LNK-FL-INTERFACE-LINK:
>   
> -	  ``MEDIA_LNK_FL_INTERFACE_LINK`` if the link is between an
> -	  interface and an entity
> +	  ``MEDIA_LNK_FL_INTERFACE_LINK`` for links that associate an entity to its
> +     interface.
> +
> +	  .. _MEDIA-LNK-FL-ANCILLARY-LINK:
> +
> +	  ``MEDIA_LNK_FL_ANCILLARY_LINK`` for links that represent a physical
> +     relationship between two entities. The link may or may not be ummutable, so
> +     applications must not assume either case.
> \ No newline at end of file
> 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: [PATCH v3 3/5] media: entity: Add link_type_name() helper

[Jean-Michel Hautbois]

Hi !

Thanks for the patch !

On 02/03/2022 23:03, Daniel Scally wrote:
> 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>

Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

> ---
> 
> Changes since v2:
> 
> 	- None
> 
> 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 d0563ee4b28b..1a7d0a4fb9e8 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -44,6 +44,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)
>   {
> @@ -89,9 +103,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;

Re: [PATCH v3 4/5] media: entity: Add support for ancillary links

[Jean-Michel Hautbois]

Hi !

Thanks for the patch !

On 02/03/2022 23:03, Daniel Scally wrote:
> Add functions to create ancillary links, so that they don't need to
> be manually created by users.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Scally <djrscally@gmail.com>

Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

> ---
> 
> Changes since v2:
> 
> 	- Fixed some typos and comment phrasing (Laurent)
> 	- Changed the position of the new function to go after media_entity_call()
> 	(Laurent)
> 
> 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 | 19 +++++++++++++++++++
>   2 files changed, 41 insertions(+)
> 
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index 1a7d0a4fb9e8..d7e2f78a83cc 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -1032,3 +1032,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..2a58defc4886 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -1108,4 +1108,23 @@ void media_remove_intf_links(struct media_interface *intf);
>   	(((entity)->ops && (entity)->ops->operation) ?			\
>   	 (entity)->ops->operation((entity) , ##args) : -ENOIOCTLCMD)
>   
> +/**
> + * 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 controller 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.
> + */
> +struct media_link *
> +media_create_ancillary_link(struct media_entity *primary,
> +			    struct media_entity *ancillary);
> +
>   #endif

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

[Jean-Michel Hautbois]

Hi !

Thanks for the patch !

On 02/03/2022 23:03, 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>

Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

> ---
> 
> Changes since v2:
> 
> 	- Stopped checking the notifier entity's function when creating the new
> 	links, and just create them whenever the subdev entity's function is either
> 	a lens controller or a flash. (Sakari)
> 
> 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 | 31 ++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 0404267f1ae4..436bd6900fd8 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -275,6 +275,24 @@ 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_links(struct v4l2_async_notifier *n,
> +					     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 0;
> +
> +	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
> +
> +#endif
> +
> +	return IS_ERR(link) ? PTR_ERR(link) : 0;
> +}
> +
>   static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>   				   struct v4l2_device *v4l2_dev,
>   				   struct v4l2_subdev *sd,
> @@ -293,6 +311,19 @@ 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_create_ancillary_links(notifier, sd);
> +	if (ret) {
> +		v4l2_async_nf_call_unbind(notifier, sd, asd);
> +		v4l2_device_unregister_subdev(sd);
> +		return ret;
> +	}
> +
>   	/* Remove from the waiting list */
>   	list_del(&asd->list);
>   	sd->asd = asd;

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

[Laurent Pinchart]

Hi Dan,

Thank you for the patch.

On Wed, Mar 02, 2022 at 10:03:04PM +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>

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

> ---
> 
> Changes since v2:
> 
> 	- Stopped checking the notifier entity's function when creating the new
> 	links, and just create them whenever the subdev entity's function is either
> 	a lens controller or a flash. (Sakari)
> 
> 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 | 31 ++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 0404267f1ae4..436bd6900fd8 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -275,6 +275,24 @@ 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_links(struct v4l2_async_notifier *n,
> +					     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 0;
> +
> +	link = media_create_ancillary_link(&n->sd->entity, &sd->entity);
> +
> +#endif
> +
> +	return IS_ERR(link) ? PTR_ERR(link) : 0;
> +}
> +
>  static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier,
>  				   struct v4l2_device *v4l2_dev,
>  				   struct v4l2_subdev *sd,
> @@ -293,6 +311,19 @@ 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_create_ancillary_links(notifier, sd);
> +	if (ret) {
> +		v4l2_async_nf_call_unbind(notifier, sd, asd);
> +		v4l2_device_unregister_subdev(sd);
> +		return ret;
> +	}
> +
>  	/* Remove from the waiting list */
>  	list_del(&asd->list);
>  	sd->asd = asd;

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 0/5] Introduce ancillary links

[Daniel Scally]

Hello everyone


Any more comments on this series?


Thanks

Dan

On 02/03/2022 22:02, Daniel Scally wrote:
> 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. 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 v2:
>
>   - Squashed #2 and #3
>
> Series-level changes since v1:
>
> 	- New patch adding some documentation to the uAPI pages.
>
> Dan
>
>
> Daniel Scally (5):
>   media: entity: Skip non-data links in graph iteration
>   media: media.h: Add new media link type
>   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            | 17 +++++--
>  drivers/media/mc/mc-entity.c                  | 46 +++++++++++++++++--
>  drivers/media/v4l2-core/v4l2-async.c          | 31 +++++++++++++
>  include/media/media-entity.h                  | 19 ++++++++
>  include/uapi/linux/media.h                    |  1 +
>  6 files changed, 112 insertions(+), 8 deletions(-)
>

Re: [PATCH v3 0/5] Introduce ancillary links

[Sakari Ailus]

Hi Daniel,

On Tue, Mar 22, 2022 at 10:08:07PM +0000, Daniel Scally wrote:
> Hello everyone
> 
> 
> Any more comments on this series?

I hope people are happy with them. They've been out for review for quite
some time.

The patches are in my tree waiting for having rc1 in master.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v3 0/5] Introduce ancillary links

[Tomasz Figa]

Hi Sakari,

On Wed, Mar 23, 2022 at 7:13 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Daniel,
>
> On Tue, Mar 22, 2022 at 10:08:07PM +0000, Daniel Scally wrote:
> > Hello everyone
> >
> >
> > Any more comments on this series?
>
> I hope people are happy with them. They've been out for review for quite
> some time.
>
> The patches are in my tree waiting for having rc1 in master.

Thanks for picking this series. Do you think you could add your tree
to linux-next, so we could easily test changes that you pick?

Best regards,
Tomasz

>
> --
> Kind regards,
>
> Sakari Ailus