From: jacopo mondi <jacopo@jmondi.org>
To: Steve Longerbeam <slongerbeam@gmail.com>
Cc: linux-media@vger.kernel.org,
"Steve Longerbeam" <steve_longerbeam@mentor.com>,
"Mauro Carvalho Chehab" <mchehab@kernel.org>,
"Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>,
"Sakari Ailus" <sakari.ailus@linux.intel.com>,
"Sebastian Reichel" <sre@kernel.org>,
"Hans Verkuil" <hans.verkuil@cisco.com>,
"open list" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v6 03/17] media: v4l2: async: Add v4l2_async_notifier_add_subdev
Date: Fri, 3 Aug 2018 17:13:46 +0200 [thread overview]
Message-ID: <20180803151346.GG4528@w540> (raw)
In-Reply-To: <1531175957-1973-4-git-send-email-steve_longerbeam@mentor.com>
[-- Attachment #1: Type: text/plain, Size: 13483 bytes --]
Hi Steven,
I've a small remark, which is probably not only related to your
patches but was there alreay... Anyway, please read below..
On Mon, Jul 09, 2018 at 03:39:03PM -0700, Steve Longerbeam wrote:
> v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
> that no other equivalent asd's have already been added to this notifier's
> asd list, or to other registered notifier's waiting or done lists, and
> increments num_subdevs.
>
> v4l2_async_notifier_add_subdev() does not make use of the notifier subdevs
> array, otherwise it would have to re-allocate the array every time the
> function was called. In place of the subdevs array, the function adds
> the newly allocated asd to a new master asd_list. The function will
> return error with a WARN() if it is ever called with the subdevs array
> allocated.
>
> Drivers are now required to call a v4l2_async_notifier_init(), before the
> first call to v4l2_async_notifier_add_subdev(), in order to initialize
> the asd_list.
>
> In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(),
> and v4l2_async_notifier_cleanup(), maintain backward compatibility with
> the subdevs array, by alternatively operate on the subdevs array or a
> non-empty notifier->asd_list.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
> Changes since v5:
> - export v4l2_async_notifier_init() which must be called by drivers.
> Suggested by Sakari Ailus.
> Changes since v4:
> - none
> Changes since v3:
> - init notifier lists after the sanity checks.
> Changes since v2:
> - add a NULL asd pointer check to v4l2_async_notifier_asd_valid().
> Changes since v1:
> - none
> ---
> drivers/media/v4l2-core/v4l2-async.c | 189 +++++++++++++++++++++++++++--------
> include/media/v4l2-async.h | 34 ++++++-
> 2 files changed, 180 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 0e7e529..8e52df2 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -363,16 +363,26 @@ static bool v4l2_async_notifier_has_async_subdev(
> struct v4l2_async_notifier *notifier, struct v4l2_async_subdev *asd,
> unsigned int this_index)
> {
> + struct v4l2_async_subdev *asd_y;
> unsigned int j;
>
> lockdep_assert_held(&list_lock);
>
> /* Check that an asd is not being added more than once. */
> - for (j = 0; j < this_index; j++) {
> - struct v4l2_async_subdev *asd_y = notifier->subdevs[j];
> -
> - if (asd_equal(asd, asd_y))
> - return true;
> + if (notifier->subdevs) {
> + for (j = 0; j < this_index; j++) {
> + asd_y = notifier->subdevs[j];
> + if (asd_equal(asd, asd_y))
> + return true;
> + }
> + } else {
> + j = 0;
> + list_for_each_entry(asd_y, ¬ifier->asd_list, asd_list) {
> + if (j++ >= this_index)
> + break;
> + if (asd_equal(asd, asd_y))
> + return true;
> + }
> }
>
> /* Check that an asd does not exist in other notifiers. */
> @@ -383,10 +393,46 @@ static bool v4l2_async_notifier_has_async_subdev(
> return false;
> }
>
> -static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> +static int v4l2_async_notifier_asd_valid(struct v4l2_async_notifier *notifier,
> + struct v4l2_async_subdev *asd,
> + unsigned int this_index)
> {
> struct device *dev =
> notifier->v4l2_dev ? notifier->v4l2_dev->dev : NULL;
> +
> + if (!asd)
> + return -EINVAL;
> +
> + switch (asd->match_type) {
> + case V4L2_ASYNC_MATCH_CUSTOM:
> + case V4L2_ASYNC_MATCH_DEVNAME:
> + case V4L2_ASYNC_MATCH_I2C:
> + case V4L2_ASYNC_MATCH_FWNODE:
> + if (v4l2_async_notifier_has_async_subdev(notifier, asd,
> + this_index))
> + return -EEXIST;
> + break;
> + default:
> + dev_err(dev, "Invalid match type %u on %p\n",
> + asd->match_type, asd);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +void v4l2_async_notifier_init(struct v4l2_async_notifier *notifier)
> +{
> + mutex_lock(&list_lock);
> +
> + INIT_LIST_HEAD(¬ifier->asd_list);
> +
> + mutex_unlock(&list_lock);
> +}
> +EXPORT_SYMBOL(v4l2_async_notifier_init);
> +
> +static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
> +{
> struct v4l2_async_subdev *asd;
> int ret;
> int i;
> @@ -399,29 +445,25 @@ static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
>
> mutex_lock(&list_lock);
>
> - for (i = 0; i < notifier->num_subdevs; i++) {
> - asd = notifier->subdevs[i];
> + if (notifier->subdevs) {
> + for (i = 0; i < notifier->num_subdevs; i++) {
> + asd = notifier->subdevs[i];
>
> - switch (asd->match_type) {
> - case V4L2_ASYNC_MATCH_CUSTOM:
> - case V4L2_ASYNC_MATCH_DEVNAME:
> - case V4L2_ASYNC_MATCH_I2C:
> - case V4L2_ASYNC_MATCH_FWNODE:
> - if (v4l2_async_notifier_has_async_subdev(
> - notifier, asd, i)) {
> - dev_err(dev,
> - "asd has already been registered or in notifier's subdev list\n");
> - ret = -EEXIST;
> + ret = v4l2_async_notifier_asd_valid(notifier, asd, i);
> + if (ret)
> goto err_unlock;
> - }
> - break;
> - default:
> - dev_err(dev, "Invalid match type %u on %p\n",
> - asd->match_type, asd);
> - ret = -EINVAL;
> - goto err_unlock;
> +
> + list_add_tail(&asd->list, ¬ifier->waiting);
> + }
> + } else {
> + i = 0;
> + list_for_each_entry(asd, ¬ifier->asd_list, asd_list) {
> + ret = v4l2_async_notifier_asd_valid(notifier, asd, i++);
Here the call stack looks like this, if I'm not mistaken:
list_for_each_entry(asd, notifier->asd_list, i) {
v4l2_async_notifier_asd_valid(notifier, asd, i):
v4l2_async_notifier_has_async_subdev(notifier, asd, i):
list_for_each_entry(asd_y, notifier->asd_list, j) {
if (j >= i) break;
if (asd == asd_y) return true;
}
}
Which is an optimization of O(n^2), but still bad.
This was there already there, it was:
for (i = 0; i < notifier->num_subdevs; i++) {
v4l2_async_notifier_has_async_subdev(notifier, notifier->subdevs[i], i):
for (j = 0; j < i; j++) {
if (notifier->subdevs[i] == notifier->subdevs[j])
return true;
}
}
}
We're not talking high performances here, but I see no reason to go through
the list twice, as after switching to use your here introduced
v4l2_async_notifier_add_subdev() async subdevices are tested at endpoint
parsing time in v4l2_async_notifier_fwnode_parse_endpoint(), which
guarantees we can't have doubles later, at notifier registration time.
If I'm not wrong, this can be anyway optimized later.
Thanks
j
> + if (ret)
> + goto err_unlock;
> +
> + list_add_tail(&asd->list, ¬ifier->waiting);
> }
> - list_add_tail(&asd->list, ¬ifier->waiting);
> }
>
> ret = v4l2_async_notifier_try_all_subdevs(notifier);
> @@ -511,36 +553,99 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
> }
> EXPORT_SYMBOL(v4l2_async_notifier_unregister);
>
> -void v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
> +static void __v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
> {
> + struct v4l2_async_subdev *asd, *tmp;
> unsigned int i;
>
> - if (!notifier || !notifier->max_subdevs)
> + if (!notifier)
> return;
>
> - for (i = 0; i < notifier->num_subdevs; i++) {
> - struct v4l2_async_subdev *asd = notifier->subdevs[i];
> + if (notifier->subdevs) {
> + if (!notifier->max_subdevs)
> + return;
>
> - switch (asd->match_type) {
> - case V4L2_ASYNC_MATCH_FWNODE:
> - fwnode_handle_put(asd->match.fwnode);
> - break;
> - default:
> - WARN_ON_ONCE(true);
> - break;
> + for (i = 0; i < notifier->num_subdevs; i++) {
> + asd = notifier->subdevs[i];
> +
> + switch (asd->match_type) {
> + case V4L2_ASYNC_MATCH_FWNODE:
> + fwnode_handle_put(asd->match.fwnode);
> + break;
> + default:
> + break;
> + }
> +
> + kfree(asd);
> }
>
> - kfree(asd);
> + notifier->max_subdevs = 0;
> + kvfree(notifier->subdevs);
> + notifier->subdevs = NULL;
> + } else {
> + list_for_each_entry_safe(asd, tmp,
> + ¬ifier->asd_list, asd_list) {
> + switch (asd->match_type) {
> + case V4L2_ASYNC_MATCH_FWNODE:
> + fwnode_handle_put(asd->match.fwnode);
> + break;
> + default:
> + break;
> + }
> +
> + list_del(&asd->asd_list);
> + kfree(asd);
> + }
> }
>
> - notifier->max_subdevs = 0;
> notifier->num_subdevs = 0;
> +}
> +
> +void v4l2_async_notifier_cleanup(struct v4l2_async_notifier *notifier)
> +{
> + mutex_lock(&list_lock);
> +
> + __v4l2_async_notifier_cleanup(notifier);
>
> - kvfree(notifier->subdevs);
> - notifier->subdevs = NULL;
> + mutex_unlock(&list_lock);
> }
> EXPORT_SYMBOL_GPL(v4l2_async_notifier_cleanup);
>
> +int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
> + struct v4l2_async_subdev *asd)
> +{
> + int ret;
> +
> + mutex_lock(&list_lock);
> +
> + if (notifier->num_subdevs >= V4L2_MAX_SUBDEVS) {
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> + /*
> + * If caller uses this function, it cannot also allocate and
> + * place asd's in the notifier->subdevs array.
> + */
> + if (WARN_ON(notifier->subdevs)) {
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> + ret = v4l2_async_notifier_asd_valid(notifier, asd,
> + notifier->num_subdevs);
> + if (ret)
> + goto unlock;
> +
> + list_add_tail(&asd->asd_list, ¬ifier->asd_list);
> + notifier->num_subdevs++;
> +
> +unlock:
> + mutex_unlock(&list_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_async_notifier_add_subdev);
> +
> int v4l2_async_register_subdev(struct v4l2_subdev *sd)
> {
> struct v4l2_async_notifier *subdev_notifier;
> @@ -614,7 +719,7 @@ void v4l2_async_unregister_subdev(struct v4l2_subdev *sd)
> mutex_lock(&list_lock);
>
> __v4l2_async_notifier_unregister(sd->subdev_notifier);
> - v4l2_async_notifier_cleanup(sd->subdev_notifier);
> + __v4l2_async_notifier_cleanup(sd->subdev_notifier);
> kfree(sd->subdev_notifier);
> sd->subdev_notifier = NULL;
>
> diff --git a/include/media/v4l2-async.h b/include/media/v4l2-async.h
> index 1592d32..ab4d7ac 100644
> --- a/include/media/v4l2-async.h
> +++ b/include/media/v4l2-async.h
> @@ -73,6 +73,8 @@ enum v4l2_async_match_type {
> * @match.custom.priv:
> * Driver-specific private struct with match parameters
> * to be used if %V4L2_ASYNC_MATCH_CUSTOM.
> + * @asd_list: used to add struct v4l2_async_subdev objects to the
> + * master notifier @asd_list
> * @list: used to link struct v4l2_async_subdev objects, waiting to be
> * probed, to a notifier->waiting list
> *
> @@ -98,6 +100,7 @@ struct v4l2_async_subdev {
>
> /* v4l2-async core private: not to be used by drivers */
> struct list_head list;
> + struct list_head asd_list;
> };
>
> /**
> @@ -127,6 +130,7 @@ struct v4l2_async_notifier_operations {
> * @v4l2_dev: v4l2_device of the root notifier, NULL otherwise
> * @sd: sub-device that registered the notifier, NULL otherwise
> * @parent: parent notifier
> + * @asd_list: master list of struct v4l2_async_subdev, replaces @subdevs
> * @waiting: list of struct v4l2_async_subdev, waiting for their drivers
> * @done: list of struct v4l2_subdev, already probed
> * @list: member in a global list of notifiers
> @@ -139,12 +143,38 @@ struct v4l2_async_notifier {
> struct v4l2_device *v4l2_dev;
> struct v4l2_subdev *sd;
> struct v4l2_async_notifier *parent;
> + struct list_head asd_list;
> struct list_head waiting;
> struct list_head done;
> struct list_head list;
> };
>
> /**
> + * v4l2_async_notifier_init - Initialize a notifier.
> + *
> + * @notifier: pointer to &struct v4l2_async_notifier
> + *
> + * This function initializes the notifier @asd_list. It must be called
> + * before the first call to @v4l2_async_notifier_add_subdev.
> + */
> +void v4l2_async_notifier_init(struct v4l2_async_notifier *notifier);
> +
> +/**
> + * v4l2_async_notifier_add_subdev - Add an async subdev to the
> + * notifier's master asd list.
> + *
> + * @notifier: pointer to &struct v4l2_async_notifier
> + * @asd: pointer to &struct v4l2_async_subdev
> + *
> + * This can be used before registering a notifier to add an
> + * asd to the notifiers @asd_list. If the caller uses this
> + * method to compose an asd list, it must never allocate
> + * or place asd's in the @subdevs array.
> + */
> +int v4l2_async_notifier_add_subdev(struct v4l2_async_notifier *notifier,
> + struct v4l2_async_subdev *asd);
> +
> +/**
> * v4l2_async_notifier_register - registers a subdevice asynchronous notifier
> *
> * @v4l2_dev: pointer to &struct v4l2_device
> @@ -177,7 +207,9 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier);
> * Release memory resources related to a notifier, including the async
> * sub-devices allocated for the purposes of the notifier but not the notifier
> * itself. The user is responsible for calling this function to clean up the
> - * notifier after calling @v4l2_async_notifier_parse_fwnode_endpoints or
> + * notifier after calling
> + * @v4l2_async_notifier_add_subdev,
> + * @v4l2_async_notifier_parse_fwnode_endpoints or
> * @v4l2_fwnode_reference_parse_sensor_common.
> *
> * There is no harm from calling v4l2_async_notifier_cleanup in other
> --
> 2.7.4
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
next prev parent reply other threads:[~2018-08-03 15:14 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1531175957-1973-1-git-send-email-steve_longerbeam@mentor.com>
2018-07-09 22:39 ` [PATCH v6 01/17] media: v4l2-fwnode: ignore endpoints that have no remote port parent Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 02/17] media: v4l2: async: Allow searching for asd of any type Steve Longerbeam
2018-09-24 17:06 ` Mauro Carvalho Chehab
2018-09-25 21:04 ` Steve Longerbeam
2018-09-25 22:20 ` Mauro Carvalho Chehab
2018-09-26 1:05 ` Steve Longerbeam
2018-09-26 9:33 ` Mauro Carvalho Chehab
2018-09-26 10:40 ` Sakari Ailus
2018-09-26 17:49 ` Steve Longerbeam
2018-09-28 12:16 ` Sakari Ailus
2018-09-29 17:40 ` Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 03/17] media: v4l2: async: Add v4l2_async_notifier_add_subdev Steve Longerbeam
2018-08-03 15:13 ` jacopo mondi [this message]
2018-08-04 16:39 ` Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 04/17] media: v4l2: async: Add convenience functions to allocate and add asd's Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 05/17] media: v4l2-fwnode: Switch to v4l2_async_notifier_add_subdev Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 06/17] media: v4l2-fwnode: Add a convenience function for registering subdevs with notifiers Steve Longerbeam
2018-09-13 10:37 ` jacopo mondi
2018-09-13 12:44 ` Sakari Ailus
2018-09-13 12:58 ` jacopo mondi
2018-09-14 0:57 ` Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 07/17] media: platform: video-mux: Register a subdev notifier Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 08/17] media: imx: csi: " Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 09/17] media: imx: mipi csi-2: " Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 10/17] media: staging/imx: of: Remove recursive graph walk Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 11/17] media: staging/imx: Loop through all registered subdevs for media links Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 12/17] media: staging/imx: Rename root notifier Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 13/17] media: staging/imx: Switch to v4l2_async_notifier_add_*_subdev Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 14/17] media: staging/imx: TODO: Remove one assumption about OF graph parsing Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 16/17] media: v4l2: async: Remove notifier subdevs array Steve Longerbeam
2018-07-23 12:35 ` Sakari Ailus
2018-07-23 16:44 ` Steve Longerbeam
2018-07-23 17:24 ` Sakari Ailus
2018-08-04 10:33 ` jacopo mondi
2018-08-04 17:20 ` Steve Longerbeam
2018-07-09 22:39 ` [PATCH v6 17/17] [media] v4l2-subdev.rst: Update doc regarding subdev descriptors Steve Longerbeam
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=20180803151346.GG4528@w540 \
--to=jacopo@jmondi.org \
--cc=hans.verkuil@cisco.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=mchehab@kernel.org \
--cc=niklas.soderlund+renesas@ragnatech.se \
--cc=sakari.ailus@linux.intel.com \
--cc=slongerbeam@gmail.com \
--cc=sre@kernel.org \
--cc=steve_longerbeam@mentor.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).