Hej Niklas,
On Tue, Jan 30, 2024 at 02:43:41PM +0100, Niklas Söderlund wrote:
> Hi Sakari,
>
> Thanks for your feedback.
>
> On 2024-01-30 12:05:33 +0000, Sakari Ailus wrote:
> > Hi Niklas,
> >
> > Thanks for the patch.
> >
> > On Mon, Jan 29, 2024 at 08:59:54PM +0100, Niklas Söderlund wrote:
> > > 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.
> >
> > Why do you need this?
>
> I need this for the use-case described as an example above. In a
> separate series [1] I remove the rcar-vin workaround for the earlier
> lack of multiple connections between entities in v4l-async and without a
> solution this patch tries to address this breaks on some boards that
> already use nested subnotifiers but for which the rcar-vin workaround
> addresses.
>
> > This is also not a bug, the documentation for the complete callback says:
> >
> > * @complete: All connections have been bound successfully. The complete
> > * callback is only executed for the root notifier.
>
> Yes, and here there are two root notifiers. One in the driver
> registering video0 and the one registering video1. Both notifiers wish
> to bind to v4l-subdev0. And both notifers have their bind callback
> called when v4l-subdev0 is registered, but only one have its complete
> callback called.
In this respect the current framework isn't perfect, it only allows one
parent...
>
> >
> > Rather it would be better to get rid of this callback entirely, one reason
> > being the impossibility of error handling. We won't be there for quite some
> > time but extending its scope does go to the other direction.
>
> I agree this is the way to go. And I could do without it in my use-cases
> if I was allowed to register the video device at probe time instead of
> in the complete callback. I have brought this up over the years but
> always been told that the video device should be registered in the
> callback handler. If this is no longer true I can rework [1] and a fix
Are you sure?
I guess there may be differing opinions on the matter but drivers such as
ipu3-cio2 and omap3isp do it in probe. I don't think rcar-vin should be
different in this respect.
> like this wont be needed for my use-cases.
>
> Looking beyond my use-case do you agree that as long as we do have the
> complete callback it needs to be supported for nested subnotifiers?
>
> 1. [PATCH 0/6] media: rcar-vin: Make use of multiple connections in v4l-async
>
> >
> > >
> > > Fixes: 28a1295795d8 ("media: v4l: async: Allow multiple connections between entities")
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> > > ---
> > > 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, ¬ifier->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, ¬ifier->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, ¬ifier->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(¬ifier->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, ¬ifier_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
> >
--
Hälsningar,
Sakari Ailus
