All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
To: Sakari Ailus <sakari.ailus@linux.intel.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	linux-media@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org,
	"Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
Subject: [PATCH] media: v4l: async: Fix completion of chained subnotifiers
Date: Mon, 29 Jan 2024 20:59:54 +0100	[thread overview]
Message-ID: <20240129195954.1110643-1-niklas.soderlund+renesas@ragnatech.se> (raw)

Allowing multiple connections between entities are very useful but the
addition of this feature did not considerate nested subnotifiers.

Consider the scenario,

rcar-vin.ko     rcar-isp.ko     rcar-csi2.ko    max96712.ko

video0 ---->    v4l-subdev0 ->  v4l-subdev1 ->  v4l-subdev2
video1 -´

Where each videoX or v4l-subdevX is controlled and register by a
separate instance of the driver listed above it. And each driver
instance registers a notifier (videoX) or a subnotifier (v4l-subdevX)
trying to bind to the device pointed to.

If the devices probe in any other except where the vidoeX ones are
probed last only one of them will have their complete callback called,
the one who last registered its notifier. Both of them will however have
their bind() callback called as expected.

This is due to v4l2_async_nf_try_complete() only walking the chain from
the subnotifier to one root notifier and completing it while ignoring
all other notifiers the subdevice might be part of. This works if there
are only one subnotifier in the mix. For example if either v4l-subdev0
or v4l-subdev1 was not part of the pipeline above.

This patch addresses the issue of nested subnotifiers by instead looking
at all notifiers and try to complete all the ones that contain the
subdevice which subnotifier was completed.

Fixes: 28a1295795d8 ("media: v4l: async: Allow multiple connections between entities")
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/v4l2-core/v4l2-async.c | 68 ++++++++++++++++++++--------
 1 file changed, 49 insertions(+), 19 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 3ec323bd528b..8b603527923c 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -176,15 +176,16 @@ static LIST_HEAD(notifier_list);
 static DEFINE_MUTEX(list_lock);
 
 static struct v4l2_async_connection *
-v4l2_async_find_match(struct v4l2_async_notifier *notifier,
-		      struct v4l2_subdev *sd)
+__v4l2_async_find_in_list(struct v4l2_async_notifier *notifier,
+			  struct v4l2_subdev *sd,
+			  struct list_head *list)
 {
 	bool (*match)(struct v4l2_async_notifier *notifier,
 		      struct v4l2_subdev *sd,
 		      struct v4l2_async_match_desc *match);
 	struct v4l2_async_connection *asc;
 
-	list_for_each_entry(asc, &notifier->waiting_list, asc_entry) {
+	list_for_each_entry(asc, list, asc_entry) {
 		/* bus_type has been verified valid before */
 		switch (asc->match.type) {
 		case V4L2_ASYNC_MATCH_TYPE_I2C:
@@ -207,6 +208,20 @@ v4l2_async_find_match(struct v4l2_async_notifier *notifier,
 	return NULL;
 }
 
+static struct v4l2_async_connection *
+v4l2_async_find_match(struct v4l2_async_notifier *notifier,
+		      struct v4l2_subdev *sd)
+{
+	return __v4l2_async_find_in_list(notifier, sd, &notifier->waiting_list);
+}
+
+static struct v4l2_async_connection *
+v4l2_async_find_done(struct v4l2_async_notifier *notifier,
+		     struct v4l2_subdev *sd)
+{
+	return __v4l2_async_find_in_list(notifier, sd, &notifier->done_list);
+}
+
 /* Compare two async match descriptors for equivalence */
 static bool v4l2_async_match_equal(struct v4l2_async_match_desc *match1,
 				   struct v4l2_async_match_desc *match2)
@@ -274,13 +289,14 @@ v4l2_async_nf_can_complete(struct v4l2_async_notifier *notifier)
 }
 
 /*
- * Complete the master notifier if possible. This is done when all async
+ * Complete the master notifiers if possible. This is done when all async
  * sub-devices have been bound; v4l2_device is also available then.
  */
 static int
 v4l2_async_nf_try_complete(struct v4l2_async_notifier *notifier)
 {
-	struct v4l2_async_notifier *__notifier = notifier;
+	struct v4l2_async_notifier *n;
+	int ret;
 
 	/* Quick check whether there are still more sub-devices here. */
 	if (!list_empty(&notifier->waiting_list))
@@ -290,24 +306,38 @@ v4l2_async_nf_try_complete(struct v4l2_async_notifier *notifier)
 		dev_dbg(notifier_dev(notifier),
 			"v4l2-async: trying to complete\n");
 
-	/* Check the entire notifier tree; find the root notifier first. */
-	while (notifier->parent)
-		notifier = notifier->parent;
+	/*
+	 * Notifiers without a parent are either a subnotifier that have not
+	 * yet been associated with it is a root notifier or a root notifier
+	 * itself. If it is a root notifier try to complete it.
+	 */
+	if (!notifier->parent) {
+		/* This is root if it has v4l2_dev. */
+		if (!notifier->v4l2_dev) {
+			dev_dbg(notifier_dev(notifier),
+				"v4l2-async: V4L2 device not available\n");
+			return 0;
+		}
 
-	/* This is root if it has v4l2_dev. */
-	if (!notifier->v4l2_dev) {
-		dev_dbg(notifier_dev(__notifier),
-			"v4l2-async: V4L2 device not available\n");
-		return 0;
-	}
+		/* Is everything ready? */
+		if (!v4l2_async_nf_can_complete(notifier))
+			return 0;
+
+		dev_dbg(notifier_dev(notifier), "v4l2-async: complete\n");
 
-	/* Is everything ready? */
-	if (!v4l2_async_nf_can_complete(notifier))
-		return 0;
+		return v4l2_async_nf_call_complete(notifier);
+	}
 
-	dev_dbg(notifier_dev(__notifier), "v4l2-async: complete\n");
+	/* Try to complete all notifiers containing the subdevices. */
+	list_for_each_entry(n, &notifier_list, notifier_entry) {
+		if (v4l2_async_find_done(n, notifier->sd)) {
+			ret = v4l2_async_nf_try_complete(n);
+			if (ret)
+				return ret;
+		}
+	}
 
-	return v4l2_async_nf_call_complete(notifier);
+	return 0;
 }
 
 static int
-- 
2.43.0


             reply	other threads:[~2024-01-29 20:01 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-01-29 19:59 Niklas Söderlund [this message]
2024-01-30 12:05 ` [PATCH] media: v4l: async: Fix completion of chained subnotifiers Sakari Ailus
2024-01-30 13:43   ` Niklas Söderlund
2024-01-30 15:27     ` Sakari Ailus
2024-01-30 15:40       ` Niklas Söderlund
2024-01-31  8:21         ` Sakari Ailus
2024-01-31 10:40           ` Niklas Söderlund
2024-02-08 17:31             ` Niklas Söderlund

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240129195954.1110643-1-niklas.soderlund+renesas@ragnatech.se \
    --to=niklas.soderlund+renesas@ragnatech.se \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=mchehab@kernel.org \
    --cc=sakari.ailus@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.