[PATCH 0/5] Introduce ancillary links

[PATCH 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 - 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 (5):
  media: media.h: Add new media link type
  media: entity: Add link_type() helper
  media: entity: Skip non-data links in graph iteration
  media: entity: Add support for ancillary links
  media: v4l2-async: Create links during v4l2_async_match_notify()

 drivers/media/mc/mc-entity.c         | 56 ++++++++++++++++++++++++++--
 drivers/media/v4l2-core/v4l2-async.c | 51 +++++++++++++++++++++++++
 include/media/media-entity.h         | 29 ++++++++++++++
 include/uapi/linux/media.h           |  1 +
 4 files changed, 134 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH 1/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.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
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

[PATCH 2/5] media: entity: Add link_type() 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.

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
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 f40f41977142..d79eb88bc167 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(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(link),
 			media_id(link->gobj0),
 			media_id(link->gobj1));
 		break;
-- 
2.25.1

[PATCH 3/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
pad-to-pad links.

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

	- new patch

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

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index d79eb88bc167..aeddc3f6310e 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -325,6 +325,14 @@ 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 pad-to-pad 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;
+		dev_dbg(entity->graph_obj.mdev->dev, "walk: skipping %s link\n",
+			link_type(link));
+		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 4/5] media: entity: Add support for ancillary links

[Daniel Scally]

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

Signed-off-by: Daniel Scally <djrscally@gmail.com>
---
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 | 30 ++++++++++++++++++++++++++++++
 include/media/media-entity.h | 29 +++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
index aeddc3f6310e..4e39e100ea03 100644
--- a/drivers/media/mc/mc-entity.c
+++ b/drivers/media/mc/mc-entity.c
@@ -1052,3 +1052,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->gobj0 = &primary->graph_obj;
+	link->gobj1 = &ancillary->graph_obj;
+	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..f7b1738cef88 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -1104,6 +1104,35 @@ 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.
+ *
+ * %MEDIA_LNK_FL_IMMUTABLE
+ *   Indicates that the link enabled state can't be modified at runtime. If
+ *   %MEDIA_LNK_FL_IMMUTABLE is set, then %MEDIA_LNK_FL_ENABLED must also be
+ *   set, since an immutable link is always enabled.
+ */
+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)
-- 
2.25.1

[PATCH 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 the rfc:

	- None

 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: [PATCH 4/5] 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.16-rc5]
[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/20211214-073020
base:   git://linuxtv.org/media_tree.git master
config: i386-buildonly-randconfig-r003-20211213 (https://download.01.org/0day-ci/archive/20211214/202112141239.xdqTNOOD-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project b6a2ddb6c8ac29412b1361810972e15221fa021c)
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/627c8446267d301ed36953f7e4fa616ab6cb771a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Scally/Introduce-ancillary-links/20211214-073020
        git checkout 627c8446267d301ed36953f7e4fa616ab6cb771a
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/media/mc/

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:1062: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:1062: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 'intf_type' [-Wunused-function]
   static inline const char *intf_type(struct media_interface *intf)
                             ^
   2 warnings generated.


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

  1061	
> 1062	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: [PATCH 3/5] media: entity: Skip non-data links in graph iteration

[Sakari Ailus]

Hi Daniel,

On Mon, Dec 13, 2021 at 11:28:47PM +0000, Daniel Scally wrote:
> When iterating over the media graph, don't follow links that are not
> pad-to-pad links.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> Changes since the rfc:
> 
> 	- new patch
> 
>  drivers/media/mc/mc-entity.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index d79eb88bc167..aeddc3f6310e 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -325,6 +325,14 @@ 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 pad-to-pad link, don't follow it */

This comment should mention data links, not pad-to-pad links.

Seems fine apart from this.

> +	if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) != MEDIA_LNK_FL_DATA_LINK) {
> +		link_top(graph) = link_top(graph)->next;
> +		dev_dbg(entity->graph_obj.mdev->dev, "walk: skipping %s link\n",
> +			link_type(link));
> +		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;

-- 
Kind regards,

Sakari Ailus

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

[Daniel Scally]

Hi Sakari

On 14/12/2021 15:01, Sakari Ailus wrote:
> Hi Daniel,
>
> On Mon, Dec 13, 2021 at 11:28:47PM +0000, Daniel Scally wrote:
>> When iterating over the media graph, don't follow links that are not
>> pad-to-pad links.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>> Changes since the rfc:
>>
>> 	- new patch
>>
>>  drivers/media/mc/mc-entity.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
>> index d79eb88bc167..aeddc3f6310e 100644
>> --- a/drivers/media/mc/mc-entity.c
>> +++ b/drivers/media/mc/mc-entity.c
>> @@ -325,6 +325,14 @@ 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 pad-to-pad link, don't follow it */
> This comment should mention data links, not pad-to-pad links.


I wondered about the terminology of this actually...since we create
those links with media_create_pad_link(), and they're called pad-to-pad
links in the documentation [1], but in other cases called data links. Do
we need to fix those other references too?



[1] https://www.kernel.org/doc/html/v5.0/media/kapi/mc-core.html#links

>
> Seems fine apart from this.
>
>> +	if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) != MEDIA_LNK_FL_DATA_LINK) {
>> +		link_top(graph) = link_top(graph)->next;
>> +		dev_dbg(entity->graph_obj.mdev->dev, "walk: skipping %s link\n",
>> +			link_type(link));
>> +		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 3/5] media: entity: Skip non-data links in graph iteration

[Sakari Ailus]

Hi Daniel,

On Tue, Dec 14, 2021 at 04:14:21PM +0000, Daniel Scally wrote:
> Hi Sakari
> 
> On 14/12/2021 15:01, Sakari Ailus wrote:
> > Hi Daniel,
> >
> > On Mon, Dec 13, 2021 at 11:28:47PM +0000, Daniel Scally wrote:
> >> When iterating over the media graph, don't follow links that are not
> >> pad-to-pad links.
> >>
> >> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> >> ---
> >> Changes since the rfc:
> >>
> >> 	- new patch
> >>
> >>  drivers/media/mc/mc-entity.c | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >>
> >> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> >> index d79eb88bc167..aeddc3f6310e 100644
> >> --- a/drivers/media/mc/mc-entity.c
> >> +++ b/drivers/media/mc/mc-entity.c
> >> @@ -325,6 +325,14 @@ 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 pad-to-pad link, don't follow it */
> > This comment should mention data links, not pad-to-pad links.
> 
> 
> I wondered about the terminology of this actually...since we create
> those links with media_create_pad_link(), and they're called pad-to-pad
> links in the documentation [1], but in other cases called data links. Do
> we need to fix those other references too?
> 
> 
> 
> [1] https://www.kernel.org/doc/html/v5.0/media/kapi/mc-core.html#links

Good point.

There were only one type of links before the interface links were
introduced. Some of the documentation seems to discuss pad links whereas
the corresponding macro name is MEDIA_LNK_FL_DATA_LINK. What the links
really represent is flow of data.

It would be good to align this, although that should probably be done in a
different context from this patchset.

-- 
Kind regards,

Sakari Ailus

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

[Sakari Ailus]

Hi Daniel,

On Mon, Dec 13, 2021 at 11:28:48PM +0000, Daniel Scally wrote:
> Add functions to create and destroy ancillary links, so that they
> don't need to be manually created by users.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> 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 | 30 ++++++++++++++++++++++++++++++
>  include/media/media-entity.h | 29 +++++++++++++++++++++++++++++
>  2 files changed, 59 insertions(+)
> 
> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> index aeddc3f6310e..4e39e100ea03 100644
> --- a/drivers/media/mc/mc-entity.c
> +++ b/drivers/media/mc/mc-entity.c
> @@ -1052,3 +1052,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->gobj0 = &primary->graph_obj;
> +	link->gobj1 = &ancillary->graph_obj;
> +	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..f7b1738cef88 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -1104,6 +1104,35 @@ 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.
> + *
> + * %MEDIA_LNK_FL_IMMUTABLE
> + *   Indicates that the link enabled state can't be modified at runtime. If
> + *   %MEDIA_LNK_FL_IMMUTABLE is set, then %MEDIA_LNK_FL_ENABLED must also be
> + *   set, since an immutable link is always enabled.

What's the use case for both of the flags?

I know the flags are there but what will they mean in practice for
ancillary links?

> + */
> +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)

-- 
Kind regards,

Sakari Ailus

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

[Daniel Scally]

Hi Sakari

On 14/12/2021 21:22, Sakari Ailus wrote:
> Hi Daniel,
>
> On Tue, Dec 14, 2021 at 04:14:21PM +0000, Daniel Scally wrote:
>> Hi Sakari
>>
>> On 14/12/2021 15:01, Sakari Ailus wrote:
>>> Hi Daniel,
>>>
>>> On Mon, Dec 13, 2021 at 11:28:47PM +0000, Daniel Scally wrote:
>>>> When iterating over the media graph, don't follow links that are not
>>>> pad-to-pad links.
>>>>
>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>>> ---
>>>> Changes since the rfc:
>>>>
>>>> 	- new patch
>>>>
>>>>  drivers/media/mc/mc-entity.c | 8 ++++++++
>>>>  1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
>>>> index d79eb88bc167..aeddc3f6310e 100644
>>>> --- a/drivers/media/mc/mc-entity.c
>>>> +++ b/drivers/media/mc/mc-entity.c
>>>> @@ -325,6 +325,14 @@ 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 pad-to-pad link, don't follow it */
>>> This comment should mention data links, not pad-to-pad links.
>>
>> I wondered about the terminology of this actually...since we create
>> those links with media_create_pad_link(), and they're called pad-to-pad
>> links in the documentation [1], but in other cases called data links. Do
>> we need to fix those other references too?
>>
>>
>>
>> [1] https://www.kernel.org/doc/html/v5.0/media/kapi/mc-core.html#links
> Good point.
>
> There were only one type of links before the interface links were
> introduced. Some of the documentation seems to discuss pad links whereas
> the corresponding macro name is MEDIA_LNK_FL_DATA_LINK. What the links
> really represent is flow of data.
>
> It would be good to align this, although that should probably be done in a
> different context from this patchset.
>
Ack; I'll fix the comment as you suggested for now

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

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Mon, Dec 13, 2021 at 11:28:45PM +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 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)

This needs documentation in
Documentation/userspace-api/media/mediactl/media-types.rst and in
media-controller-model.rst. The latter should probably be a bit more
detailed to explain the use cases with at least one example.

>  
>  struct media_link_desc {
>  	struct media_pad_desc source;

-- 
Regards,

Laurent Pinchart

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

[Daniel Scally]

Hi Laurent

On 14/12/2021 21:50, Laurent Pinchart wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> On Mon, Dec 13, 2021 at 11:28:45PM +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 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)
> This needs documentation in
> Documentation/userspace-api/media/mediactl/media-types.rst and in
> media-controller-model.rst. The latter should probably be a bit more
> detailed to explain the use cases with at least one example.


Agreed - I'll add some for the v2

>
>>  
>>  struct media_link_desc {
>>  	struct media_pad_desc source;

Re: [PATCH 2/5] media: entity: Add link_type() helper

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Mon, Dec 13, 2021 at 11:28:46PM +0000, 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.
> 
> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> ---
> 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 f40f41977142..d79eb88bc167 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(struct media_link *link)

This could be named link_type_name() to avoid confusion with a function
that would return the link type.

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

> +{
> +	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(link),
>  			media_id(link->gobj0),
>  			media_id(link->gobj1));
>  		break;

-- 
Regards,

Laurent Pinchart

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

[Daniel Scally]

Hi Sakari

On 14/12/2021 21:25, Sakari Ailus wrote:
> Hi Daniel,
>
> On Mon, Dec 13, 2021 at 11:28:48PM +0000, Daniel Scally wrote:
>> Add functions to create and destroy ancillary links, so that they
>> don't need to be manually created by users.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>> 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 | 30 ++++++++++++++++++++++++++++++
>>  include/media/media-entity.h | 29 +++++++++++++++++++++++++++++
>>  2 files changed, 59 insertions(+)
>>
>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
>> index aeddc3f6310e..4e39e100ea03 100644
>> --- a/drivers/media/mc/mc-entity.c
>> +++ b/drivers/media/mc/mc-entity.c
>> @@ -1052,3 +1052,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->gobj0 = &primary->graph_obj;
>> +	link->gobj1 = &ancillary->graph_obj;
>> +	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..f7b1738cef88 100644
>> --- a/include/media/media-entity.h
>> +++ b/include/media/media-entity.h
>> @@ -1104,6 +1104,35 @@ 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.
>> + *
>> + * %MEDIA_LNK_FL_IMMUTABLE
>> + *   Indicates that the link enabled state can't be modified at runtime. If
>> + *   %MEDIA_LNK_FL_IMMUTABLE is set, then %MEDIA_LNK_FL_ENABLED must also be
>> + *   set, since an immutable link is always enabled.
> What's the use case for both of the flags?
>
> I know the flags are there but what will they mean in practice for
> ancillary links?


Within the kernel, I don't think they have any effect now (without patch
#3 of this series the graph iteration would have tried to walk them). I
mostly intended that they would be set so that future userspace users
wouldn't be able to flag them as disabled.

>
>> + */
>> +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)

Re: [PATCH 2/5] media: entity: Add link_type() helper

[Daniel Scally]

Hi Laurent

On 14/12/2021 21:54, Laurent Pinchart wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> On Mon, Dec 13, 2021 at 11:28:46PM +0000, 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.
>>
>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>> ---
>> 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 f40f41977142..d79eb88bc167 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(struct media_link *link)
> This could be named link_type_name() to avoid confusion with a function
> that would return the link type.


Probably yeah - I'll do that in v2

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


Thank you

>
>> +{
>> +	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(link),
>>  			media_id(link->gobj0),
>>  			media_id(link->gobj1));
>>  		break;

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

[Laurent Pinchart]

On Tue, Dec 14, 2021 at 05:01:23PM +0200, Sakari Ailus wrote:
> On Mon, Dec 13, 2021 at 11:28:47PM +0000, Daniel Scally wrote:
> > When iterating over the media graph, don't follow links that are not
> > pad-to-pad links.
> > 
> > Signed-off-by: Daniel Scally <djrscally@gmail.com>
> > ---
> > Changes since the rfc:
> > 
> > 	- new patch
> > 
> >  drivers/media/mc/mc-entity.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > index d79eb88bc167..aeddc3f6310e 100644
> > --- a/drivers/media/mc/mc-entity.c
> > +++ b/drivers/media/mc/mc-entity.c
> > @@ -325,6 +325,14 @@ 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 pad-to-pad link, don't follow it */
> 
> This comment should mention data links, not pad-to-pad links.
> 
> Seems fine apart from this.
> 
> > +	if ((link->flags & MEDIA_LNK_FL_LINK_TYPE) != MEDIA_LNK_FL_DATA_LINK) {
> > +		link_top(graph) = link_top(graph)->next;
> > +		dev_dbg(entity->graph_obj.mdev->dev, "walk: skipping %s link\n",
> > +			link_type(link));

I would drop the debug message. The other messages in this function can
be useful to figure out why graph walk doesn't behave like expected
(reporting, for instance, that a disabled link is not traversed), but I
don't think there's much value in indicating we're skipping non-data
links.

With these issues addressed,

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

> > +		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;

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Tue, Dec 14, 2021 at 09:54:32PM +0000, Daniel Scally wrote:
> On 14/12/2021 21:25, Sakari Ailus wrote:
> > On Mon, Dec 13, 2021 at 11:28:48PM +0000, Daniel Scally wrote:
> >> Add functions to create and destroy ancillary links, so that they
> >> don't need to be manually created by users.
> >>
> >> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> >> ---
> >> 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 | 30 ++++++++++++++++++++++++++++++
> >>  include/media/media-entity.h | 29 +++++++++++++++++++++++++++++
> >>  2 files changed, 59 insertions(+)
> >>
> >> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> >> index aeddc3f6310e..4e39e100ea03 100644
> >> --- a/drivers/media/mc/mc-entity.c
> >> +++ b/drivers/media/mc/mc-entity.c
> >> @@ -1052,3 +1052,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);

Not a candidate for this series, but returning an error pointer from
media_add_link() could be nice.

> >> +	if (!link)
> >> +		return ERR_PTR(-ENOMEM);
> >> +
> >> +	link->gobj0 = &primary->graph_obj;
> >> +	link->gobj1 = &ancillary->graph_obj;
> >> +	link->flags = flags | MEDIA_LNK_FL_ANCILLARY_LINK;
> >> +
> >> +	/* Initialize graph object embedded at the new link */

s/embedded at/embedded in/ ?

> >> +	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);

Non-static (and especially exported) functions must be declared in a
header file. You don't seem to use this function anywhere in this series
though, is it a leftover ?

> >> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> >> index fea489f03d57..f7b1738cef88 100644
> >> --- a/include/media/media-entity.h
> >> +++ b/include/media/media-entity.h
> >> @@ -1104,6 +1104,35 @@ 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

s/link/ancillary link/

> >> + *
> >> + * @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.
> >> + *
> >> + * %MEDIA_LNK_FL_IMMUTABLE
> >> + *   Indicates that the link enabled state can't be modified at runtime. If
> >> + *   %MEDIA_LNK_FL_IMMUTABLE is set, then %MEDIA_LNK_FL_ENABLED must also be
> >> + *   set, since an immutable link is always enabled.
> >
> > What's the use case for both of the flags?
> >
> > I know the flags are there but what will they mean in practice for
> > ancillary links?
> 
> Within the kernel, I don't think they have any effect now (without patch
> #3 of this series the graph iteration would have tried to walk them). I
> mostly intended that they would be set so that future userspace users
> wouldn't be able to flag them as disabled.

How about removing the flags parameter, hardcoding both
MEDIA_LNK_FL_ENABLED and MEDIA_LNK_FL_IMMUTABLE inside the function, and
specifying in the userspace API documentation that both flags are always
set of ancillary links ?

Thinking a bit further, what would be the implications of changing this
rule in the future ? I don't know what use cases may require that, but
let's assume we start exposing mutable ancillary links, or mutable and
disabled ancillary links. How will existing userspace react to that, do
we need to specify rules in the uAPI documentation to tell userspace how
to prepare for the future ?

> >> + */
> >> +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)

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Daniel,

Thank you for the patch.

On Mon, Dec 13, 2021 at 11:28:49PM +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 the rfc:
> 
> 	- None
> 
>  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,

Is there a guarantee at this point that notifier->sd->entity has already
been registered with the media_device ? That's done by
media_device_register_entity() called from
v4l2_device_register_subdev().

> +					   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

s/Setup/Create/ ("link setup" refers to another operation, enabling and
disabling links at runtime)

> + * should be done. At the moment, we only support cases where the notifier
> + * is a sensor and the subdev is a lens.

s/sensor/camera sensor/
s/lens/lens controller/

> + *
> + * 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)

s/)/)./

> +	 */
> +	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;

-- 
Regards,

Laurent Pinchart

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

[Daniel Scally]

Hi Laurent

On 14/12/2021 22:22, Laurent Pinchart wrote:
> Hi Daniel,
> 
> Thank you for the patch.
> 
> On Mon, Dec 13, 2021 at 11:28:49PM +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 the rfc:
>>
>> 	- None
>>
>>  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,
> 
> Is there a guarantee at this point that notifier->sd->entity has already
> been registered with the media_device ? That's done by
> media_device_register_entity() called from
> v4l2_device_register_subdev().

v4l2_async_match_notify() calls v4l2_device_register_subdev() before the
point that I've added the call to v4l2_async_try_create_links(), so I
think that's covered there.

>> +					   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
> 
> s/Setup/Create/ ("link setup" refers to another operation, enabling and
> disabling links at runtime)

Yes, good point; although that too is a piece of terminology I find a
bit jarring to be honest; I would have named that function
media_entity_configure_link()

> 
>> + * should be done. At the moment, we only support cases where the notifier
>> + * is a sensor and the subdev is a lens.
> 
> s/sensor/camera sensor/
> s/lens/lens controller/
> 
>> + *
>> + * 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)
> 
> s/)/)./
> 
>> +	 */
>> +	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;
> 

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

[Laurent Pinchart]

Hi Daniel,

On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote:
> On 14/12/2021 22:22, Laurent Pinchart wrote:
> > On Mon, Dec 13, 2021 at 11:28:49PM +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 the rfc:
> >>
> >> 	- None
> >>
> >>  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,
> > 
> > Is there a guarantee at this point that notifier->sd->entity has already
> > been registered with the media_device ? That's done by
> > media_device_register_entity() called from
> > v4l2_device_register_subdev().
> 
> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the
> point that I've added the call to v4l2_async_try_create_links(), so I
> think that's covered there.

It calls it on sd, not notifier->sd. It's the latter that concerns me.

> >> +					   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
> > 
> > s/Setup/Create/ ("link setup" refers to another operation, enabling and
> > disabling links at runtime)
> 
> Yes, good point; although that too is a piece of terminology I find a
> bit jarring to be honest; I would have named that function
> media_entity_configure_link()
> 
> >> + * should be done. At the moment, we only support cases where the notifier
> >> + * is a sensor and the subdev is a lens.
> > 
> > s/sensor/camera sensor/
> > s/lens/lens controller/
> > 
> >> + *
> >> + * 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)
> > 
> > s/)/)./
> > 
> >> +	 */
> >> +	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;

-- 
Regards,

Laurent Pinchart

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

[Daniel Scally]

Hi Laurent

On 14/12/2021 23:01, Laurent Pinchart wrote:
> Hi Daniel,
>
> On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote:
>> On 14/12/2021 22:22, Laurent Pinchart wrote:
>>> On Mon, Dec 13, 2021 at 11:28:49PM +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 the rfc:
>>>>
>>>> 	- None
>>>>
>>>>  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,
>>> Is there a guarantee at this point that notifier->sd->entity has already
>>> been registered with the media_device ? That's done by
>>> media_device_register_entity() called from
>>> v4l2_device_register_subdev().
>> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the
>> point that I've added the call to v4l2_async_try_create_links(), so I
>> think that's covered there.
> It calls it on sd, not notifier->sd. It's the latter that concerns me.


Ah, you're right of course...I guess in that case the notifier->sd would
get registered during the v4l2_async_match_notify() where the sensor
driver's subdev is sd, but I don't think there's any guarantee that that
would happen first...I haven't traced it through but my guess is that it
would depend on the order in which the ipu3-cio2, sensor and lens
controller drivers probed. I'll check to try and be sure how it works
tomorrow

>
>>>> +					   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
>>> s/Setup/Create/ ("link setup" refers to another operation, enabling and
>>> disabling links at runtime)
>> Yes, good point; although that too is a piece of terminology I find a
>> bit jarring to be honest; I would have named that function
>> media_entity_configure_link()
>>
>>>> + * should be done. At the moment, we only support cases where the notifier
>>>> + * is a sensor and the subdev is a lens.
>>> s/sensor/camera sensor/
>>> s/lens/lens controller/
>>>
>>>> + *
>>>> + * 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)
>>> s/)/)./
>>>
>>>> +	 */
>>>> +	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;

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

[Laurent Pinchart]

Hi Dan,

On Tue, Dec 14, 2021 at 11:41:27PM +0000, Daniel Scally wrote:
> On 14/12/2021 23:01, Laurent Pinchart wrote:
> > On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote:
> >> On 14/12/2021 22:22, Laurent Pinchart wrote:
> >>> On Mon, Dec 13, 2021 at 11:28:49PM +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 the rfc:
> >>>>
> >>>> 	- None
> >>>>
> >>>>  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,
> >>>
> >>> Is there a guarantee at this point that notifier->sd->entity has already
> >>> been registered with the media_device ? That's done by
> >>> media_device_register_entity() called from
> >>> v4l2_device_register_subdev().
> >>
> >> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the
> >> point that I've added the call to v4l2_async_try_create_links(), so I
> >> think that's covered there.
> >
> > It calls it on sd, not notifier->sd. It's the latter that concerns me.
> 
> Ah, you're right of course...I guess in that case the notifier->sd would
> get registered during the v4l2_async_match_notify() where the sensor
> driver's subdev is sd, but I don't think there's any guarantee that that
> would happen first...I haven't traced it through but my guess is that it
> would depend on the order in which the ipu3-cio2, sensor and lens
> controller drivers probed. I'll check to try and be sure how it works
> tomorrow

I was looking at media_device_register_entity(), and it sets

	INIT_LIST_HEAD(&entity->links);
	entity->num_links = 0;
	entity->num_backlinks = 0;

If we create links before that, things may go bad.

> >>>> +					   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
> >>>
> >>> s/Setup/Create/ ("link setup" refers to another operation, enabling and
> >>> disabling links at runtime)
> >>
> >> Yes, good point; although that too is a piece of terminology I find a
> >> bit jarring to be honest; I would have named that function
> >> media_entity_configure_link()
> >>
> >>>> + * should be done. At the moment, we only support cases where the notifier
> >>>> + * is a sensor and the subdev is a lens.
> >>>
> >>> s/sensor/camera sensor/
> >>> s/lens/lens controller/
> >>>
> >>>> + *
> >>>> + * 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)
> >>>
> >>> s/)/)./
> >>>
> >>>> +	 */
> >>>> +	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;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/5] Introduce ancillary links

[Mauro Carvalho Chehab]

Em Mon, 13 Dec 2021 23:28:44 +0000
Daniel Scally <djrscally@gmail.com> escreveu:

> 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.
> 
> Thoughts and comments very welcome 

The idea of ancillary link sounds interesting. I did a quick look at
the series. 

Laurent already did some good points during his review.
Besides that, one thing it is missing, though, is an implementation on
a driver. At least vimc should gain an implementation at this series,
in order to allow media developers to test and see how the graph will
be after the patch series.

Regards,
Mauro

> 
> Dan
> 
> Daniel Scally (5):
>   media: media.h: Add new media link type
>   media: entity: Add link_type() helper
>   media: entity: Skip non-data links in graph iteration
>   media: entity: Add support for ancillary links
>   media: v4l2-async: Create links during v4l2_async_match_notify()
> 
>  drivers/media/mc/mc-entity.c         | 56 ++++++++++++++++++++++++++--
>  drivers/media/v4l2-core/v4l2-async.c | 51 +++++++++++++++++++++++++
>  include/media/media-entity.h         | 29 ++++++++++++++
>  include/uapi/linux/media.h           |  1 +
>  4 files changed, 134 insertions(+), 3 deletions(-)
> 



Thanks,
Mauro

Re: [PATCH 0/5] Introduce ancillary links

[Daniel Scally]

Hi Mauro

On 15/12/2021 09:25, Mauro Carvalho Chehab wrote:
> Em Mon, 13 Dec 2021 23:28:44 +0000
> Daniel Scally <djrscally@gmail.com> escreveu:
>
>> 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.
>>
>> Thoughts and comments very welcome 
> The idea of ancillary link sounds interesting. I did a quick look at
> the series. 
>
> Laurent already did some good points during his review.
> Besides that, one thing it is missing, though, is an implementation on
> a driver. At least vimc should gain an implementation at this series,
> in order to allow media developers to test and see how the graph will
> be after the patch series.


We have this running through libcamera at the moment; this series piggy
backs onto the notifier that's set up by
v4l2_async_create_sensor_subdev() so that the connection is made and the
links created when a lens controller is linked to the sensor via the
lens-focus firmware property. I've been testing this using the dw9719
driver I posted [1], plus a series that adds support to libcamera [2]. I
believe that some folks from Cros have also tested it with the dw9714
driver too.


[1]
https://lore.kernel.org/linux-media/20211128232115.38833-1-djrscally@gmail.com/

[2]
https://lists.libcamera.org/pipermail/libcamera-devel/2021-December/028082.html

>
> Regards,
> Mauro
>
>> Dan
>>
>> Daniel Scally (5):
>>   media: media.h: Add new media link type
>>   media: entity: Add link_type() helper
>>   media: entity: Skip non-data links in graph iteration
>>   media: entity: Add support for ancillary links
>>   media: v4l2-async: Create links during v4l2_async_match_notify()
>>
>>  drivers/media/mc/mc-entity.c         | 56 ++++++++++++++++++++++++++--
>>  drivers/media/v4l2-core/v4l2-async.c | 51 +++++++++++++++++++++++++
>>  include/media/media-entity.h         | 29 ++++++++++++++
>>  include/uapi/linux/media.h           |  1 +
>>  4 files changed, 134 insertions(+), 3 deletions(-)
>>
>
>
> Thanks,
> Mauro

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

[Sakari Ailus]

On Wed, Dec 15, 2021 at 11:04:44AM +0200, Laurent Pinchart wrote:
> Hi Dan,
> 
> On Tue, Dec 14, 2021 at 11:41:27PM +0000, Daniel Scally wrote:
> > On 14/12/2021 23:01, Laurent Pinchart wrote:
> > > On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote:
> > >> On 14/12/2021 22:22, Laurent Pinchart wrote:
> > >>> On Mon, Dec 13, 2021 at 11:28:49PM +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 the rfc:
> > >>>>
> > >>>> 	- None
> > >>>>
> > >>>>  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,
> > >>>
> > >>> Is there a guarantee at this point that notifier->sd->entity has already
> > >>> been registered with the media_device ? That's done by
> > >>> media_device_register_entity() called from
> > >>> v4l2_device_register_subdev().
> > >>
> > >> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the
> > >> point that I've added the call to v4l2_async_try_create_links(), so I
> > >> think that's covered there.
> > >
> > > It calls it on sd, not notifier->sd. It's the latter that concerns me.
> > 
> > Ah, you're right of course...I guess in that case the notifier->sd would
> > get registered during the v4l2_async_match_notify() where the sensor
> > driver's subdev is sd, but I don't think there's any guarantee that that
> > would happen first...I haven't traced it through but my guess is that it
> > would depend on the order in which the ipu3-cio2, sensor and lens
> > controller drivers probed. I'll check to try and be sure how it works
> > tomorrow
> 
> I was looking at media_device_register_entity(), and it sets
> 
> 	INIT_LIST_HEAD(&entity->links);
> 	entity->num_links = 0;
> 	entity->num_backlinks = 0;
> 
> If we create links before that, things may go bad.

Yes.

There's a guarantee that the notifier's complete callback is called once
the notifier's subdevs as well as sub-notifiers are bound and complete. But
there's no guarantee on the initialisation of related entities.

Especially for sensors, the async subdev is registered after the sensor's
own async notifier.

I wonder if the ugly registered callback could be used for this purpose.
Better of course would be to avoid that.

-- 
Sakari Ailus

Re: [PATCH 0/5] Introduce ancillary links

[Laurent Pinchart]

Hi Mauro,

On Wed, Dec 15, 2021 at 10:25:09AM +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 13 Dec 2021 23:28:44 +0000 Daniel Scally escreveu:
> 
> > 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.
> > 
> > Thoughts and comments very welcome 
> 
> The idea of ancillary link sounds interesting. I did a quick look at
> the series. 
> 
> Laurent already did some good points during his review.
> Besides that, one thing it is missing, though, is an implementation on
> a driver. At least vimc should gain an implementation at this series,
> in order to allow media developers to test and see how the graph will
> be after the patch series.

The whole point of patch 4/4 is to create those links in the core
without requiring manual involvement of the drivers :-)

> > Daniel Scally (5):
> >   media: media.h: Add new media link type
> >   media: entity: Add link_type() helper
> >   media: entity: Skip non-data links in graph iteration
> >   media: entity: Add support for ancillary links
> >   media: v4l2-async: Create links during v4l2_async_match_notify()
> > 
> >  drivers/media/mc/mc-entity.c         | 56 ++++++++++++++++++++++++++--
> >  drivers/media/v4l2-core/v4l2-async.c | 51 +++++++++++++++++++++++++
> >  include/media/media-entity.h         | 29 ++++++++++++++
> >  include/uapi/linux/media.h           |  1 +
> >  4 files changed, 134 insertions(+), 3 deletions(-)

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

Hi Sakari,

On Wed, Dec 15, 2021 at 11:44:29AM +0200, Sakari Ailus wrote:
> On Wed, Dec 15, 2021 at 11:04:44AM +0200, Laurent Pinchart wrote:
> > On Tue, Dec 14, 2021 at 11:41:27PM +0000, Daniel Scally wrote:
> > > On 14/12/2021 23:01, Laurent Pinchart wrote:
> > > > On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote:
> > > >> On 14/12/2021 22:22, Laurent Pinchart wrote:
> > > >>> On Mon, Dec 13, 2021 at 11:28:49PM +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 the rfc:
> > > >>>>
> > > >>>> 	- None
> > > >>>>
> > > >>>>  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,
> > > >>>
> > > >>> Is there a guarantee at this point that notifier->sd->entity has already
> > > >>> been registered with the media_device ? That's done by
> > > >>> media_device_register_entity() called from
> > > >>> v4l2_device_register_subdev().
> > > >>
> > > >> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the
> > > >> point that I've added the call to v4l2_async_try_create_links(), so I
> > > >> think that's covered there.
> > > >
> > > > It calls it on sd, not notifier->sd. It's the latter that concerns me.
> > > 
> > > Ah, you're right of course...I guess in that case the notifier->sd would
> > > get registered during the v4l2_async_match_notify() where the sensor
> > > driver's subdev is sd, but I don't think there's any guarantee that that
> > > would happen first...I haven't traced it through but my guess is that it
> > > would depend on the order in which the ipu3-cio2, sensor and lens
> > > controller drivers probed. I'll check to try and be sure how it works
> > > tomorrow
> > 
> > I was looking at media_device_register_entity(), and it sets
> > 
> > 	INIT_LIST_HEAD(&entity->links);
> > 	entity->num_links = 0;
> > 	entity->num_backlinks = 0;
> > 
> > If we create links before that, things may go bad.
> 
> Yes.
> 
> There's a guarantee that the notifier's complete callback is called once
> the notifier's subdevs as well as sub-notifiers are bound and complete. But
> there's no guarantee on the initialisation of related entities.
> 
> Especially for sensors, the async subdev is registered after the sensor's
> own async notifier.
> 
> I wonder if the ugly registered callback could be used for this purpose.
> Better of course would be to avoid that.

I'd really like all these links to be created automatically by the code,
but given the very loosely defined rules regarding entity
initialization, I'm worried this may not be possible without quite a bit
of cleanup first :-(

It looks like quite a bit of the work done in
media_device_register_entity() could (and likely should) be moved to
media_entity_init(), but I'm not sure if that would be enough to
properly fix the issue.

-- 
Regards,

Laurent Pinchart

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

[Daniel Scally]

Hi Laurent, Sakari

On 15/12/2021 09:55, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wed, Dec 15, 2021 at 11:44:29AM +0200, Sakari Ailus wrote:
>> On Wed, Dec 15, 2021 at 11:04:44AM +0200, Laurent Pinchart wrote:
>>> On Tue, Dec 14, 2021 at 11:41:27PM +0000, Daniel Scally wrote:
>>>> On 14/12/2021 23:01, Laurent Pinchart wrote:
>>>>> On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote:
>>>>>> On 14/12/2021 22:22, Laurent Pinchart wrote:
>>>>>>> On Mon, Dec 13, 2021 at 11:28:49PM +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 the rfc:
>>>>>>>>
>>>>>>>> 	- None
>>>>>>>>
>>>>>>>>  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,
>>>>>>>
>>>>>>> Is there a guarantee at this point that notifier->sd->entity has already
>>>>>>> been registered with the media_device ? That's done by
>>>>>>> media_device_register_entity() called from
>>>>>>> v4l2_device_register_subdev().
>>>>>>
>>>>>> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the
>>>>>> point that I've added the call to v4l2_async_try_create_links(), so I
>>>>>> think that's covered there.
>>>>>
>>>>> It calls it on sd, not notifier->sd. It's the latter that concerns me.
>>>>
>>>> Ah, you're right of course...I guess in that case the notifier->sd would
>>>> get registered during the v4l2_async_match_notify() where the sensor
>>>> driver's subdev is sd, but I don't think there's any guarantee that that
>>>> would happen first...I haven't traced it through but my guess is that it
>>>> would depend on the order in which the ipu3-cio2, sensor and lens
>>>> controller drivers probed. I'll check to try and be sure how it works
>>>> tomorrow
>>>
>>> I was looking at media_device_register_entity(), and it sets
>>>
>>> 	INIT_LIST_HEAD(&entity->links);
>>> 	entity->num_links = 0;
>>> 	entity->num_backlinks = 0;
>>>
>>> If we create links before that, things may go bad.

Yep, that definitely looks like it would make things go badly wrong. I'm
building with a delayed ov8865 probe now, let's see what happens...

>>
>> Yes.
>>
>> There's a guarantee that the notifier's complete callback is called once
>> the notifier's subdevs as well as sub-notifiers are bound and complete. But
>> there's no guarantee on the initialisation of related entities.
>>
>> Especially for sensors, the async subdev is registered after the sensor's
>> own async notifier.
>>
>> I wonder if the ugly registered callback could be used for this purpose.
>> Better of course would be to avoid that.
> 
> I'd really like all these links to be created automatically by the code,
> but given the very loosely defined rules regarding entity
> initialization, I'm worried this may not be possible without quite a bit
> of cleanup first :-(


Yeah. At present at least the primary entity would need to be linked to
the media dev, as it's taking primary->graph_obj.mdev as the pointer to
use in media_gobj_create() in media_create_ancillary_link(). That's a
pretty big problem actually...but I'd really like to try and solve it as
we could cut a lot of code out the other drivers if we do the same thing
for the data links.

One way around it might be to defer matching in v4l2_async_find_match()
if the notifier's subdev hasn't picked up an mdev itself yet...which
could guarantee the ordering but sort of breaks the asynchronicity of it
all. I'm almost certainly missing some other reason that that's a
terrible idea too.

I'll try and explore some ways to do this that still keeps the link
setup within core - thanks for pointing it out Laurent

> It looks like quite a bit of the work done in
> media_device_register_entity() could (and likely should) be moved to
> media_entity_init(), but I'm not sure if that would be enough to
> properly fix the issue.

I guess you mean media_entity_pads_init()? Or media_device_init()?
> 

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

[Laurent Pinchart]

Hi Dan,

On Wed, Dec 15, 2021 at 11:10:09PM +0000, Daniel Scally wrote:
> On 15/12/2021 09:55, Laurent Pinchart wrote:
> > On Wed, Dec 15, 2021 at 11:44:29AM +0200, Sakari Ailus wrote:
> >> On Wed, Dec 15, 2021 at 11:04:44AM +0200, Laurent Pinchart wrote:
> >>> On Tue, Dec 14, 2021 at 11:41:27PM +0000, Daniel Scally wrote:
> >>>> On 14/12/2021 23:01, Laurent Pinchart wrote:
> >>>>> On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote:
> >>>>>> On 14/12/2021 22:22, Laurent Pinchart wrote:
> >>>>>>> On Mon, Dec 13, 2021 at 11:28:49PM +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 the rfc:
> >>>>>>>>
> >>>>>>>> 	- None
> >>>>>>>>
> >>>>>>>>  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,
> >>>>>>>
> >>>>>>> Is there a guarantee at this point that notifier->sd->entity has already
> >>>>>>> been registered with the media_device ? That's done by
> >>>>>>> media_device_register_entity() called from
> >>>>>>> v4l2_device_register_subdev().
> >>>>>>
> >>>>>> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the
> >>>>>> point that I've added the call to v4l2_async_try_create_links(), so I
> >>>>>> think that's covered there.
> >>>>>
> >>>>> It calls it on sd, not notifier->sd. It's the latter that concerns me.
> >>>>
> >>>> Ah, you're right of course...I guess in that case the notifier->sd would
> >>>> get registered during the v4l2_async_match_notify() where the sensor
> >>>> driver's subdev is sd, but I don't think there's any guarantee that that
> >>>> would happen first...I haven't traced it through but my guess is that it
> >>>> would depend on the order in which the ipu3-cio2, sensor and lens
> >>>> controller drivers probed. I'll check to try and be sure how it works
> >>>> tomorrow
> >>>
> >>> I was looking at media_device_register_entity(), and it sets
> >>>
> >>> 	INIT_LIST_HEAD(&entity->links);
> >>> 	entity->num_links = 0;
> >>> 	entity->num_backlinks = 0;
> >>>
> >>> If we create links before that, things may go bad.
> 
> Yep, that definitely looks like it would make things go badly wrong. I'm
> building with a delayed ov8865 probe now, let's see what happens...
> 
> >> Yes.
> >>
> >> There's a guarantee that the notifier's complete callback is called once
> >> the notifier's subdevs as well as sub-notifiers are bound and complete. But
> >> there's no guarantee on the initialisation of related entities.
> >>
> >> Especially for sensors, the async subdev is registered after the sensor's
> >> own async notifier.
> >>
> >> I wonder if the ugly registered callback could be used for this purpose.
> >> Better of course would be to avoid that.
> > 
> > I'd really like all these links to be created automatically by the code,
> > but given the very loosely defined rules regarding entity
> > initialization, I'm worried this may not be possible without quite a bit
> > of cleanup first :-(
> 
> Yeah. At present at least the primary entity would need to be linked to
> the media dev, as it's taking primary->graph_obj.mdev as the pointer to
> use in media_gobj_create() in media_create_ancillary_link(). That's a
> pretty big problem actually...but I'd really like to try and solve it as
> we could cut a lot of code out the other drivers if we do the same thing
> for the data links.
> 
> One way around it might be to defer matching in v4l2_async_find_match()
> if the notifier's subdev hasn't picked up an mdev itself yet...which
> could guarantee the ordering but sort of breaks the asynchronicity of it
> all. I'm almost certainly missing some other reason that that's a
> terrible idea too.

v4l2-async is a mess, that's the main reason why everybody is reluctant
to touch it :-) On my long todo list is a task to rewrite it from
scratch, with an API that wouldn't be V4L2-specific.

> I'll try and explore some ways to do this that still keeps the link
> setup within core - thanks for pointing it out Laurent
> 
> > It looks like quite a bit of the work done in
> > media_device_register_entity() could (and likely should) be moved to
> > media_entity_init(), but I'm not sure if that would be enough to
> > properly fix the issue.
> 
> I guess you mean media_entity_pads_init()? Or media_device_init()?

The former, sorry.

-- 
Regards,

Laurent Pinchart

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

[kernel test robot]

Hi Daniel,

I love your patch! Yet something to improve:

[auto build test ERROR on media-tree/master]
[also build test ERROR on v5.16-rc5 next-20211215]
[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/20211214-073020
base:   git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-r015-20211216 (https://download.01.org/0day-ci/archive/20211216/202112161906.gHHRLukN-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project dd245bab9fbb364faa1581e4f92ba3119a872fba)
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/7e7fcd65e8144f3ffa337760c26fb786f4028466
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Daniel-Scally/Introduce-ancillary-links/20211214-073020
        git checkout 7e7fcd65e8144f3ffa337760c26fb786f4028466
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 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:284:10: error: no member named 'entity' in 'struct v4l2_subdev'
           if (sd->entity.function != MEDIA_ENT_F_LENS &&
               ~~  ^
   drivers/media/v4l2-core/v4l2-async.c:285:10: error: no member named 'entity' in 'struct v4l2_subdev'
               sd->entity.function != MEDIA_ENT_F_FLASH)
               ~~  ^
   drivers/media/v4l2-core/v4l2-async.c:288:52: error: no member named 'entity' in 'struct v4l2_subdev'
           link = media_create_ancillary_link(&notifier->sd->entity, &sd->entity,
                                               ~~~~~~~~~~~~  ^
   drivers/media/v4l2-core/v4l2-async.c:288:65: error: no member named 'entity' in 'struct v4l2_subdev'
           link = media_create_ancillary_link(&notifier->sd->entity, &sd->entity,
                                                                      ~~  ^
   drivers/media/v4l2-core/v4l2-async.c:309:24: error: no member named 'entity' in 'struct v4l2_subdev'
           switch (notifier->sd->entity.function) {
                   ~~~~~~~~~~~~  ^
   5 errors generated.


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@lists.01.org

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

[Daniel Scally]

I guess this needs to be a no-op if the media controller API isn't
configured.

On 16/12/2021 11:10, kernel test robot wrote:
> Hi Daniel,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on media-tree/master]
> [also build test ERROR on v5.16-rc5 next-20211215]
> [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/20211214-073020
> base:   git://linuxtv.org/media_tree.git master
> config: x86_64-randconfig-r015-20211216 (https://download.01.org/0day-ci/archive/20211216/202112161906.gHHRLukN-lkp@intel.com/config)
> compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project dd245bab9fbb364faa1581e4f92ba3119a872fba)
> 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/7e7fcd65e8144f3ffa337760c26fb786f4028466
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Daniel-Scally/Introduce-ancillary-links/20211214-073020
>         git checkout 7e7fcd65e8144f3ffa337760c26fb786f4028466
>         # save the config file to linux build tree
>         mkdir build_dir
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 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:284:10: error: no member named 'entity' in 'struct v4l2_subdev'
>            if (sd->entity.function != MEDIA_ENT_F_LENS &&
>                ~~  ^
>    drivers/media/v4l2-core/v4l2-async.c:285:10: error: no member named 'entity' in 'struct v4l2_subdev'
>                sd->entity.function != MEDIA_ENT_F_FLASH)
>                ~~  ^
>    drivers/media/v4l2-core/v4l2-async.c:288:52: error: no member named 'entity' in 'struct v4l2_subdev'
>            link = media_create_ancillary_link(&notifier->sd->entity, &sd->entity,
>                                                ~~~~~~~~~~~~  ^
>    drivers/media/v4l2-core/v4l2-async.c:288:65: error: no member named 'entity' in 'struct v4l2_subdev'
>            link = media_create_ancillary_link(&notifier->sd->entity, &sd->entity,
>                                                                       ~~  ^
>    drivers/media/v4l2-core/v4l2-async.c:309:24: error: no member named 'entity' in 'struct v4l2_subdev'
>            switch (notifier->sd->entity.function) {
>                    ~~~~~~~~~~~~  ^
>    5 errors generated.
>
>
> 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@lists.01.org

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

[Daniel Scally]

Hi Laurent

On 15/12/2021 09:04, Laurent Pinchart wrote:

A month to the day! Sorry about the delay - more on that below...

> Hi Dan,
>
> On Tue, Dec 14, 2021 at 11:41:27PM +0000, Daniel Scally wrote:
>> On 14/12/2021 23:01, Laurent Pinchart wrote:
>>> On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote:
>>>> On 14/12/2021 22:22, Laurent Pinchart wrote:
>>>>> On Mon, Dec 13, 2021 at 11:28:49PM +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 the rfc:
>>>>>>
>>>>>> 	- None
>>>>>>
>>>>>>  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,
>>>>> Is there a guarantee at this point that notifier->sd->entity has already
>>>>> been registered with the media_device ? That's done by
>>>>> media_device_register_entity() called from
>>>>> v4l2_device_register_subdev().
>>>> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the
>>>> point that I've added the call to v4l2_async_try_create_links(), so I
>>>> think that's covered there.
>>> It calls it on sd, not notifier->sd. It's the latter that concerns me.
>> Ah, you're right of course...I guess in that case the notifier->sd would
>> get registered during the v4l2_async_match_notify() where the sensor
>> driver's subdev is sd, but I don't think there's any guarantee that that
>> would happen first...I haven't traced it through but my guess is that it
>> would depend on the order in which the ipu3-cio2, sensor and lens
>> controller drivers probed. I'll check to try and be sure how it works
>> tomorrow
> I was looking at media_device_register_entity(), and it sets
>
> 	INIT_LIST_HEAD(&entity->links);
> 	entity->num_links = 0;
> 	entity->num_backlinks = 0;
>
> If we create links before that, things may go bad.


It looks like there _is_ a guarantee of ordering actually. When the
ov8865's notifier is registered in v4l2_async_register_subdev_sensor(),
v4l2_async_nf_try_all_subdevs() is called against it, but at that point
v4l2_async_nf_find_v4l2_dev() won't find anything for the ov8865's
notifier even if the dw9719 has already probed and has it's async_subdev
waiting because the notifier has no parent and no directly assigned
v4l2_dev, so the function exits before trying to match anything (this
same logic guards all calls to v4l2_async_find_match()). Very shortly
after that v4l2_async_register_subdev() is called for the ov8865's
subdev which will match to ipu3-cio2's notifier. In
v4l2_async_match_notify() for that match the ipu3-cio2's notifier is
assigned as parent to the ov8865's notifier, but _after_
v4l2_device_register_subdev() is called for the ov8865. From that point
on v4l2_async_nf_find_v4l2_dev() will return a pointer and the matching
for the dw9719 will work correctly. So unless I've missed something, I
think it's ok.

This took me a long time to figure out, because I reset libcamera to
master for some reason and then totally forgot that I had done
that...which meant the auto-focus wasn't working when I tested it and I
convinced myself that my deliberate screwing of the probe ordering _did_
break it. After tearing my hair out for an embarrassing amount of time I
eventually figured out what I had done and got to the bottom of it -
sorry for the delay!

>
>>>>>> +					   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
>>>>> s/Setup/Create/ ("link setup" refers to another operation, enabling and
>>>>> disabling links at runtime)
>>>> Yes, good point; although that too is a piece of terminology I find a
>>>> bit jarring to be honest; I would have named that function
>>>> media_entity_configure_link()
>>>>
>>>>>> + * should be done. At the moment, we only support cases where the notifier
>>>>>> + * is a sensor and the subdev is a lens.
>>>>> s/sensor/camera sensor/
>>>>> s/lens/lens controller/
>>>>>
>>>>>> + *
>>>>>> + * 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)
>>>>> s/)/)./
>>>>>
>>>>>> +	 */
>>>>>> +	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;

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

[Daniel Scally]

Hi Laurent, sorry - realised I never replied to this one

On 14/12/2021 22:14, Laurent Pinchart wrote:
> Hi Daniel,
>
> Thank you for the patch.
>
> On Tue, Dec 14, 2021 at 09:54:32PM +0000, Daniel Scally wrote:
>> On 14/12/2021 21:25, Sakari Ailus wrote:
>>> On Mon, Dec 13, 2021 at 11:28:48PM +0000, Daniel Scally wrote:
>>>> Add functions to create and destroy ancillary links, so that they
>>>> don't need to be manually created by users.
>>>>
>>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
>>>> ---
>>>> 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 | 30 ++++++++++++++++++++++++++++++
>>>>  include/media/media-entity.h | 29 +++++++++++++++++++++++++++++
>>>>  2 files changed, 59 insertions(+)
>>>>
>>>> diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
>>>> index aeddc3f6310e..4e39e100ea03 100644
>>>> --- a/drivers/media/mc/mc-entity.c
>>>> +++ b/drivers/media/mc/mc-entity.c
>>>> @@ -1052,3 +1052,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);
> Not a candidate for this series, but returning an error pointer from
> media_add_link() could be nice.
>
>>>> +	if (!link)
>>>> +		return ERR_PTR(-ENOMEM);
>>>> +
>>>> +	link->gobj0 = &primary->graph_obj;
>>>> +	link->gobj1 = &ancillary->graph_obj;
>>>> +	link->flags = flags | MEDIA_LNK_FL_ANCILLARY_LINK;
>>>> +
>>>> +	/* Initialize graph object embedded at the new link */
> s/embedded at/embedded in/ ?
>
>>>> +	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);
> Non-static (and especially exported) functions must be declared in a
> header file. You don't seem to use this function anywhere in this series
> though, is it a leftover ?
>
>>>> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
>>>> index fea489f03d57..f7b1738cef88 100644
>>>> --- a/include/media/media-entity.h
>>>> +++ b/include/media/media-entity.h
>>>> @@ -1104,6 +1104,35 @@ 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
> s/link/ancillary link/
>
>>>> + *
>>>> + * @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.
>>>> + *
>>>> + * %MEDIA_LNK_FL_IMMUTABLE
>>>> + *   Indicates that the link enabled state can't be modified at runtime. If
>>>> + *   %MEDIA_LNK_FL_IMMUTABLE is set, then %MEDIA_LNK_FL_ENABLED must also be
>>>> + *   set, since an immutable link is always enabled.
>>> What's the use case for both of the flags?
>>>
>>> I know the flags are there but what will they mean in practice for
>>> ancillary links?
>> Within the kernel, I don't think they have any effect now (without patch
>> #3 of this series the graph iteration would have tried to walk them). I
>> mostly intended that they would be set so that future userspace users
>> wouldn't be able to flag them as disabled.
> How about removing the flags parameter, hardcoding both
> MEDIA_LNK_FL_ENABLED and MEDIA_LNK_FL_IMMUTABLE inside the function, and
> specifying in the userspace API documentation that both flags are always
> set of ancillary links ?


This is ok by me

>
> Thinking a bit further, what would be the implications of changing this
> rule in the future ? I don't know what use cases may require that, but
> let's assume we start exposing mutable ancillary links, or mutable and
> disabled ancillary links. How will existing userspace react to that, do
> we need to specify rules in the uAPI documentation to tell userspace how
> to prepare for the future ?


Yeah maybe; at the moment my libcamera implementation wouldn't treat a
disabled link any differently, and for a lens at least I can't really
see any situation where a disabled link would be valid. Perhaps multiple
LED drivers supporting the same sensor for different flash modes might
be a thing though, in which case it might be helpful to disable one of
the ancillary links so that you can flag a particular driver as the
active one, or something long those lines.


So perhaps the uAPI docs should explain that both ENABLED and IMMUTABLE
are currently hardcoded, but that userspace should still check for an
enabled link before following it.

>
>>>> + */
>>>> +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)