| Message ID | 20170925222540.371-15-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26/09/17 00:25, Sakari Ailus wrote: > Refactor the V4L2 async framework a little in preparation for async > sub-device notifiers. Perhaps extend this a bit to also state the reason for these changes? > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> Anyway, Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 66 +++++++++++++++++++++++++----------- > 1 file changed, 47 insertions(+), 19 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index 77b9f851bfa9..1d4132305243 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -125,12 +125,13 @@ static struct v4l2_async_subdev *v4l2_async_find_match( > } > > static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, > + struct v4l2_device *v4l2_dev, > struct v4l2_subdev *sd, > struct v4l2_async_subdev *asd) > { > int ret; > > - ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd); > + ret = v4l2_device_register_subdev(v4l2_dev, sd); > if (ret < 0) > return ret; > > @@ -154,6 +155,31 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, > return 0; > } > > +/* Test all async sub-devices in a notifier for a match. */ > +static int v4l2_async_notifier_try_all_subdevs( > + struct v4l2_async_notifier *notifier) > +{ > + struct v4l2_device *v4l2_dev = notifier->v4l2_dev; > + struct v4l2_subdev *sd, *tmp; > + > + list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) { > + struct v4l2_async_subdev *asd; > + int ret; > + > + asd = v4l2_async_find_match(notifier, sd); > + if (!asd) > + continue; > + > + ret = v4l2_async_match_notify(notifier, v4l2_dev, sd, asd); > + if (ret < 0) { > + mutex_unlock(&list_lock); > + return ret; > + } > + } > + > + return 0; > +} > + > static void v4l2_async_cleanup(struct v4l2_subdev *sd) > { > v4l2_device_unregister_subdev(sd); > @@ -163,17 +189,15 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd) > sd->dev = NULL; > } > > -int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > - struct v4l2_async_notifier *notifier) > +static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier) > { > - struct v4l2_subdev *sd, *tmp; > struct v4l2_async_subdev *asd; > + int ret; > int i; > > - if (!v4l2_dev || notifier->num_subdevs > V4L2_MAX_SUBDEVS) > + if (notifier->num_subdevs > V4L2_MAX_SUBDEVS) > return -EINVAL; > > - notifier->v4l2_dev = v4l2_dev; > INIT_LIST_HEAD(¬ifier->waiting); > INIT_LIST_HEAD(¬ifier->done); > > @@ -206,18 +230,10 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > > mutex_lock(&list_lock); > > - list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) { > - int ret; > - > - asd = v4l2_async_find_match(notifier, sd); > - if (!asd) > - continue; > - > - ret = v4l2_async_match_notify(notifier, sd, asd); > - if (ret < 0) { > - mutex_unlock(&list_lock); > - return ret; > - } > + ret = v4l2_async_notifier_try_all_subdevs(notifier); > + if (ret) { > + mutex_unlock(&list_lock); > + return ret; > } > > /* Keep also completed notifiers on the list */ > @@ -227,6 +243,17 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > > return 0; > } > + > +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > + struct v4l2_async_notifier *notifier) > +{ > + if (WARN_ON(!v4l2_dev)) > + return -EINVAL; > + > + notifier->v4l2_dev = v4l2_dev; > + > + return __v4l2_async_notifier_register(notifier); > +} > EXPORT_SYMBOL(v4l2_async_notifier_register); > > void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > @@ -303,7 +330,8 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > struct v4l2_async_subdev *asd = v4l2_async_find_match(notifier, > sd); > if (asd) { > - int ret = v4l2_async_match_notify(notifier, sd, asd); > + int ret = v4l2_async_match_notify( > + notifier, notifier->v4l2_dev, sd, asd); > mutex_unlock(&list_lock); > return ret; > } >
Hi Hans, On Tue, Sep 26, 2017 at 10:12:50AM +0200, Hans Verkuil wrote: > On 26/09/17 00:25, Sakari Ailus wrote: > > Refactor the V4L2 async framework a little in preparation for async > > sub-device notifiers. > > Perhaps extend this a bit to also state the reason for these changes? Well, the true reason is that Laurent felt the following patch was doing too much, but most of the changes in it are actually tightly interconnected. How about adding: This avoids making some structural changes in the patch actually implementing sub-device notifiers, making that patch easier to review. > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Anyway, > > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> Thanks! > > > --- > > drivers/media/v4l2-core/v4l2-async.c | 66 +++++++++++++++++++++++++----------- > > 1 file changed, 47 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > > index 77b9f851bfa9..1d4132305243 100644 > > --- a/drivers/media/v4l2-core/v4l2-async.c > > +++ b/drivers/media/v4l2-core/v4l2-async.c > > @@ -125,12 +125,13 @@ static struct v4l2_async_subdev *v4l2_async_find_match( > > } > > > > static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, > > + struct v4l2_device *v4l2_dev, > > struct v4l2_subdev *sd, > > struct v4l2_async_subdev *asd) > > { > > int ret; > > > > - ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd); > > + ret = v4l2_device_register_subdev(v4l2_dev, sd); > > if (ret < 0) > > return ret; > > > > @@ -154,6 +155,31 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, > > return 0; > > } > > > > +/* Test all async sub-devices in a notifier for a match. */ > > +static int v4l2_async_notifier_try_all_subdevs( > > + struct v4l2_async_notifier *notifier) > > +{ > > + struct v4l2_device *v4l2_dev = notifier->v4l2_dev; > > + struct v4l2_subdev *sd, *tmp; > > + > > + list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) { > > + struct v4l2_async_subdev *asd; > > + int ret; > > + > > + asd = v4l2_async_find_match(notifier, sd); > > + if (!asd) > > + continue; > > + > > + ret = v4l2_async_match_notify(notifier, v4l2_dev, sd, asd); > > + if (ret < 0) { > > + mutex_unlock(&list_lock); > > + return ret; > > + } > > + } > > + > > + return 0; > > +} > > + > > static void v4l2_async_cleanup(struct v4l2_subdev *sd) > > { > > v4l2_device_unregister_subdev(sd); > > @@ -163,17 +189,15 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd) > > sd->dev = NULL; > > } > > > > -int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > > - struct v4l2_async_notifier *notifier) > > +static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier) > > { > > - struct v4l2_subdev *sd, *tmp; > > struct v4l2_async_subdev *asd; > > + int ret; > > int i; > > > > - if (!v4l2_dev || notifier->num_subdevs > V4L2_MAX_SUBDEVS) > > + if (notifier->num_subdevs > V4L2_MAX_SUBDEVS) > > return -EINVAL; > > > > - notifier->v4l2_dev = v4l2_dev; > > INIT_LIST_HEAD(¬ifier->waiting); > > INIT_LIST_HEAD(¬ifier->done); > > > > @@ -206,18 +230,10 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > > > > mutex_lock(&list_lock); > > > > - list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) { > > - int ret; > > - > > - asd = v4l2_async_find_match(notifier, sd); > > - if (!asd) > > - continue; > > - > > - ret = v4l2_async_match_notify(notifier, sd, asd); > > - if (ret < 0) { > > - mutex_unlock(&list_lock); > > - return ret; > > - } > > + ret = v4l2_async_notifier_try_all_subdevs(notifier); > > + if (ret) { > > + mutex_unlock(&list_lock); > > + return ret; > > } > > > > /* Keep also completed notifiers on the list */ > > @@ -227,6 +243,17 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > > > > return 0; > > } > > + > > +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, > > + struct v4l2_async_notifier *notifier) > > +{ > > + if (WARN_ON(!v4l2_dev)) > > + return -EINVAL; > > + > > + notifier->v4l2_dev = v4l2_dev; > > + > > + return __v4l2_async_notifier_register(notifier); > > +} > > EXPORT_SYMBOL(v4l2_async_notifier_register); > > > > void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > > @@ -303,7 +330,8 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) > > struct v4l2_async_subdev *asd = v4l2_async_find_match(notifier, > > sd); > > if (asd) { > > - int ret = v4l2_async_match_notify(notifier, sd, asd); > > + int ret = v4l2_async_match_notify( > > + notifier, notifier->v4l2_dev, sd, asd); > > mutex_unlock(&list_lock); > > return ret; > > } > > >
On 26/09/17 10:17, Sakari Ailus wrote: > Hi Hans, > > On Tue, Sep 26, 2017 at 10:12:50AM +0200, Hans Verkuil wrote: >> On 26/09/17 00:25, Sakari Ailus wrote: >>> Refactor the V4L2 async framework a little in preparation for async >>> sub-device notifiers. >> >> Perhaps extend this a bit to also state the reason for these changes? > > Well, the true reason is that Laurent felt the following patch was doing > too much, but most of the changes in it are actually tightly > interconnected. > > How about adding: > > This avoids making some structural changes in the patch actually > implementing sub-device notifiers, making that patch easier to review. That will do. I preferred the original version, that way I could see WHY things were done, even though the patch was larger. Regards, Hans > >> >>> >>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> >> Anyway, >> >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > > Thanks! > >> >>> --- >>> drivers/media/v4l2-core/v4l2-async.c | 66 +++++++++++++++++++++++++----------- >>> 1 file changed, 47 insertions(+), 19 deletions(-) >>> >>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c >>> index 77b9f851bfa9..1d4132305243 100644 >>> --- a/drivers/media/v4l2-core/v4l2-async.c >>> +++ b/drivers/media/v4l2-core/v4l2-async.c >>> @@ -125,12 +125,13 @@ static struct v4l2_async_subdev *v4l2_async_find_match( >>> } >>> >>> static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, >>> + struct v4l2_device *v4l2_dev, >>> struct v4l2_subdev *sd, >>> struct v4l2_async_subdev *asd) >>> { >>> int ret; >>> >>> - ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd); >>> + ret = v4l2_device_register_subdev(v4l2_dev, sd); >>> if (ret < 0) >>> return ret; >>> >>> @@ -154,6 +155,31 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, >>> return 0; >>> } >>> >>> +/* Test all async sub-devices in a notifier for a match. */ >>> +static int v4l2_async_notifier_try_all_subdevs( >>> + struct v4l2_async_notifier *notifier) >>> +{ >>> + struct v4l2_device *v4l2_dev = notifier->v4l2_dev; >>> + struct v4l2_subdev *sd, *tmp; >>> + >>> + list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) { >>> + struct v4l2_async_subdev *asd; >>> + int ret; >>> + >>> + asd = v4l2_async_find_match(notifier, sd); >>> + if (!asd) >>> + continue; >>> + >>> + ret = v4l2_async_match_notify(notifier, v4l2_dev, sd, asd); >>> + if (ret < 0) { >>> + mutex_unlock(&list_lock); >>> + return ret; >>> + } >>> + } >>> + >>> + return 0; >>> +} >>> + >>> static void v4l2_async_cleanup(struct v4l2_subdev *sd) >>> { >>> v4l2_device_unregister_subdev(sd); >>> @@ -163,17 +189,15 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd) >>> sd->dev = NULL; >>> } >>> >>> -int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, >>> - struct v4l2_async_notifier *notifier) >>> +static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier) >>> { >>> - struct v4l2_subdev *sd, *tmp; >>> struct v4l2_async_subdev *asd; >>> + int ret; >>> int i; >>> >>> - if (!v4l2_dev || notifier->num_subdevs > V4L2_MAX_SUBDEVS) >>> + if (notifier->num_subdevs > V4L2_MAX_SUBDEVS) >>> return -EINVAL; >>> >>> - notifier->v4l2_dev = v4l2_dev; >>> INIT_LIST_HEAD(¬ifier->waiting); >>> INIT_LIST_HEAD(¬ifier->done); >>> >>> @@ -206,18 +230,10 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, >>> >>> mutex_lock(&list_lock); >>> >>> - list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) { >>> - int ret; >>> - >>> - asd = v4l2_async_find_match(notifier, sd); >>> - if (!asd) >>> - continue; >>> - >>> - ret = v4l2_async_match_notify(notifier, sd, asd); >>> - if (ret < 0) { >>> - mutex_unlock(&list_lock); >>> - return ret; >>> - } >>> + ret = v4l2_async_notifier_try_all_subdevs(notifier); >>> + if (ret) { >>> + mutex_unlock(&list_lock); >>> + return ret; >>> } >>> >>> /* Keep also completed notifiers on the list */ >>> @@ -227,6 +243,17 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, >>> >>> return 0; >>> } >>> + >>> +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, >>> + struct v4l2_async_notifier *notifier) >>> +{ >>> + if (WARN_ON(!v4l2_dev)) >>> + return -EINVAL; >>> + >>> + notifier->v4l2_dev = v4l2_dev; >>> + >>> + return __v4l2_async_notifier_register(notifier); >>> +} >>> EXPORT_SYMBOL(v4l2_async_notifier_register); >>> >>> void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) >>> @@ -303,7 +330,8 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) >>> struct v4l2_async_subdev *asd = v4l2_async_find_match(notifier, >>> sd); >>> if (asd) { >>> - int ret = v4l2_async_match_notify(notifier, sd, asd); >>> + int ret = v4l2_async_match_notify( >>> + notifier, notifier->v4l2_dev, sd, asd); >>> mutex_unlock(&list_lock); >>> return ret; >>> } >>> >> >
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index 77b9f851bfa9..1d4132305243 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -125,12 +125,13 @@ static struct v4l2_async_subdev *v4l2_async_find_match( } static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, + struct v4l2_device *v4l2_dev, struct v4l2_subdev *sd, struct v4l2_async_subdev *asd) { int ret; - ret = v4l2_device_register_subdev(notifier->v4l2_dev, sd); + ret = v4l2_device_register_subdev(v4l2_dev, sd); if (ret < 0) return ret; @@ -154,6 +155,31 @@ static int v4l2_async_match_notify(struct v4l2_async_notifier *notifier, return 0; } +/* Test all async sub-devices in a notifier for a match. */ +static int v4l2_async_notifier_try_all_subdevs( + struct v4l2_async_notifier *notifier) +{ + struct v4l2_device *v4l2_dev = notifier->v4l2_dev; + struct v4l2_subdev *sd, *tmp; + + list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) { + struct v4l2_async_subdev *asd; + int ret; + + asd = v4l2_async_find_match(notifier, sd); + if (!asd) + continue; + + ret = v4l2_async_match_notify(notifier, v4l2_dev, sd, asd); + if (ret < 0) { + mutex_unlock(&list_lock); + return ret; + } + } + + return 0; +} + static void v4l2_async_cleanup(struct v4l2_subdev *sd) { v4l2_device_unregister_subdev(sd); @@ -163,17 +189,15 @@ static void v4l2_async_cleanup(struct v4l2_subdev *sd) sd->dev = NULL; } -int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, - struct v4l2_async_notifier *notifier) +static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier) { - struct v4l2_subdev *sd, *tmp; struct v4l2_async_subdev *asd; + int ret; int i; - if (!v4l2_dev || notifier->num_subdevs > V4L2_MAX_SUBDEVS) + if (notifier->num_subdevs > V4L2_MAX_SUBDEVS) return -EINVAL; - notifier->v4l2_dev = v4l2_dev; INIT_LIST_HEAD(¬ifier->waiting); INIT_LIST_HEAD(¬ifier->done); @@ -206,18 +230,10 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, mutex_lock(&list_lock); - list_for_each_entry_safe(sd, tmp, &subdev_list, async_list) { - int ret; - - asd = v4l2_async_find_match(notifier, sd); - if (!asd) - continue; - - ret = v4l2_async_match_notify(notifier, sd, asd); - if (ret < 0) { - mutex_unlock(&list_lock); - return ret; - } + ret = v4l2_async_notifier_try_all_subdevs(notifier); + if (ret) { + mutex_unlock(&list_lock); + return ret; } /* Keep also completed notifiers on the list */ @@ -227,6 +243,17 @@ int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, return 0; } + +int v4l2_async_notifier_register(struct v4l2_device *v4l2_dev, + struct v4l2_async_notifier *notifier) +{ + if (WARN_ON(!v4l2_dev)) + return -EINVAL; + + notifier->v4l2_dev = v4l2_dev; + + return __v4l2_async_notifier_register(notifier); +} EXPORT_SYMBOL(v4l2_async_notifier_register); void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) @@ -303,7 +330,8 @@ int v4l2_async_register_subdev(struct v4l2_subdev *sd) struct v4l2_async_subdev *asd = v4l2_async_find_match(notifier, sd); if (asd) { - int ret = v4l2_async_match_notify(notifier, sd, asd); + int ret = v4l2_async_match_notify( + notifier, notifier->v4l2_dev, sd, asd); mutex_unlock(&list_lock); return ret; }
Refactor the V4L2 async framework a little in preparation for async sub-device notifiers. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/media/v4l2-core/v4l2-async.c | 66 +++++++++++++++++++++++++----------- 1 file changed, 47 insertions(+), 19 deletions(-)