Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Sakari Ailus]

On Tue, Jun 26, 2018 at 01:47:45PM -0700, Steve Longerbeam wrote:
> 
> 
> On 06/26/2018 12:12 AM, Sakari Ailus wrote:
> > On Wed, May 09, 2018 at 04:06:32PM -0700, Steve Longerbeam wrote:
> > > 
> > > On 05/08/2018 03:12 AM, Sakari Ailus wrote:
> > > > On Fri, Apr 20, 2018 at 10:12:33AM -0700, Steve Longerbeam wrote:
> > > > > Hi Sakari,
> > > > > 
> > > > > 
> > > > > On 04/20/2018 05:24 AM, Sakari Ailus wrote:
> > > > > > Hi Steve,
> > > > > > 
> > > > > > Thanks for the patchset.
> > > > > > 
> > > > > > On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:
> > > > > > > v4l2_async_notifier_add_subdev() adds an asd to the notifier. It 
> > > > > > > checks
> > > > > > > that the asd's match_type is valid and 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 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.
> > > > > > > 
> > > > > > > In v4l2_async_notifier_has_async_subdev(), 
> > > > > > > __v4l2_async_notifier_register(),
> > > > > > > and v4l2_async_notifier_cleanup(), alternatively operate on the 
> > > > > > > subdevs
> > > > > > > array or a non-empty notifier->asd_list.
> > > > > > I do agree with the approach, but this patch leaves the remaining 
> > > > > > users of
> > > > > > the subdevs array in the notifier intact. Could you rework them to 
> > > > > > use the
> > > > > > v4l2_async_notifier_add_subdev() as well?
> > > > > > 
> > > > > > There seem to be just a few of them --- exynos4-is and rcar_drif.
> > > > > I count more than a few :)
> > > > > 
> > > > > % cd drivers/media && grep -l -r --include "*.[ch]"
> > > > > 'notifier[\.\-]>*subdevs[   ]*='
> > > > > v4l2-core/v4l2-async.c
> > > > > platform/pxa_camera.c
> > > > > platform/ti-vpe/cal.c
> > > > > platform/exynos4-is/media-dev.c
> > > > > platform/qcom/camss-8x16/camss.c
> > > > > platform/soc_camera/soc_camera.c
> > > > > platform/atmel/atmel-isi.c
> > > > > platform/atmel/atmel-isc.c
> > > > > platform/stm32/stm32-dcmi.c
> > > > > platform/davinci/vpif_capture.c
> > > > > platform/davinci/vpif_display.c
> > > > > platform/renesas-ceu.c
> > > > > platform/am437x/am437x-vpfe.c
> > > > > platform/xilinx/xilinx-vipp.c
> > > > > platform/rcar_drif.c
> > > > > 
> > > > > 
> > > > > So not including v4l2-core, the list is:
> > > > > 
> > > > > pxa_camera
> > > > > ti-vpe
> > > > > exynos4-is
> > > > > qcom
> > > > > soc_camera
> > > > > atmel
> > > > > stm32
> > > > > davinci
> > > > > renesas-ceu
> > > > > am437x
> > > > > xilinx
> > > > > rcar_drif
> > > > > 
> > > > > 
> > > > > Given such a large list of the users of the notifier->subdevs array,
> > > > > I think this should be done is two steps: submit this patchset first,
> > > > > that keeps the backward compatibility, and then a subsequent patchset
> > > > > that converts all drivers to use v4l2_async_notifier_add_subdev(), at
> > > > > which point the subdevs array can be removed from struct
> > > > > v4l2_async_notifier.
> > > > > 
> > > > > Or, do you still think this should be done all at once? I could add a
> > > > > large patch to this patchset that does the conversion and removes
> > > > > the subdevs array.
> > > > Sorry for the delay. I grepped for this, too, but I guess I ended up 
> > > > using
> > > > an expression that missed most of the users. :-(
> > > > 
> > > > I think it'd be very good to perform that conversion --- the code in the
> > > > framework would be quite a bit cleaner and easier to maintain whereas 
> > > > the
> > > > per-driver conversions seem pretty simple, such as this on in
> > > > drivers/media/platform/atmel/atmel-isi.c :
> > > > 
> > > > /* Register the subdevices notifier. */
> > > > subdevs = devm_kzalloc(isi->dev, sizeof(*subdevs), GFP_KERNEL);
> > > > if (!subdevs) {
> > > > of_node_put(isi->entity.node);
> > > > return -ENOMEM;
> > > > }
> > > > 
> > > > subdevs[0] = >entity.asd;
> > > > 
> > > > isi->notifier.subdevs = subdevs;
> > > > isi->notifier.num_subdevs = 1;
> > > > isi->notifier.ops = _graph_notify_ops;
> > > 
> > > Yes, the conversions are fairly straightforward. I've completed that work,
> > > but it was a very manual conversion, every platform is different and 
> > > needed
> > > careful study.
> > > 
> > > Although I was careful about getting the error-out paths correct, there
> > > could
> > > be 

Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Steve Longerbeam]

On 06/26/2018 12:12 AM, Sakari Ailus wrote:

On Wed, May 09, 2018 at 04:06:32PM -0700, Steve Longerbeam wrote:


On 05/08/2018 03:12 AM, Sakari Ailus wrote:

On Fri, Apr 20, 2018 at 10:12:33AM -0700, Steve Longerbeam wrote:

Hi Sakari,


On 04/20/2018 05:24 AM, Sakari Ailus wrote:

Hi Steve,

Thanks for the patchset.

On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:

v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
that the asd's match_type is valid and 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 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.

In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(),
and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
array or a non-empty notifier->asd_list.

I do agree with the approach, but this patch leaves the remaining users of
the subdevs array in the notifier intact. Could you rework them to use the
v4l2_async_notifier_add_subdev() as well?

There seem to be just a few of them --- exynos4-is and rcar_drif.

I count more than a few :)

% cd drivers/media && grep -l -r --include "*.[ch]"
'notifier[\.\-]>*subdevs[   ]*='
v4l2-core/v4l2-async.c
platform/pxa_camera.c
platform/ti-vpe/cal.c
platform/exynos4-is/media-dev.c
platform/qcom/camss-8x16/camss.c
platform/soc_camera/soc_camera.c
platform/atmel/atmel-isi.c
platform/atmel/atmel-isc.c
platform/stm32/stm32-dcmi.c
platform/davinci/vpif_capture.c
platform/davinci/vpif_display.c
platform/renesas-ceu.c
platform/am437x/am437x-vpfe.c
platform/xilinx/xilinx-vipp.c
platform/rcar_drif.c


So not including v4l2-core, the list is:

pxa_camera
ti-vpe
exynos4-is
qcom
soc_camera
atmel
stm32
davinci
renesas-ceu
am437x
xilinx
rcar_drif


Given such a large list of the users of the notifier->subdevs array,
I think this should be done is two steps: submit this patchset first,
that keeps the backward compatibility, and then a subsequent patchset
that converts all drivers to use v4l2_async_notifier_add_subdev(), at
which point the subdevs array can be removed from struct
v4l2_async_notifier.

Or, do you still think this should be done all at once? I could add a
large patch to this patchset that does the conversion and removes
the subdevs array.

Sorry for the delay. I grepped for this, too, but I guess I ended up using
an expression that missed most of the users. :-(

I think it'd be very good to perform that conversion --- the code in the
framework would be quite a bit cleaner and easier to maintain whereas the
per-driver conversions seem pretty simple, such as this on in
drivers/media/platform/atmel/atmel-isi.c :

/* Register the subdevices notifier. */
subdevs = devm_kzalloc(isi->dev, sizeof(*subdevs), GFP_KERNEL);
if (!subdevs) {
of_node_put(isi->entity.node);
return -ENOMEM;
}

subdevs[0] = >entity.asd;

isi->notifier.subdevs = subdevs;
isi->notifier.num_subdevs = 1;
isi->notifier.ops = _graph_notify_ops;


Yes, the conversions are fairly straightforward. I've completed that work,
but it was a very manual conversion, every platform is different and needed
careful study.

Although I was careful about getting the error-out paths correct, there
could
be mistakes there, which would result in memory leaks. And obviously I can't
re-test all these platforms. So this patch is very high-risk. More eyes are
needed on it, ideally the maintainer(s) of each affected platform.

I just submitted v4 of this series, but did not include this large un-tested
patch in v4 for those reasons.

Instead, this patch, and follow-up patches that strips support for subdevs
array altogether from v4l2-async.c, and updates rst docs, are available at
my
media-tree mirror on github:

g...@github.com:slongerbeam/mediatree.git

in the branch 'remove-subdevs-array'. The branch is based off this series
(branch 'imx-subdev-notifiers.6').

Would you be able to post these to the list? I'd really like this being
done as part of the related patchset, rather than leaving the mess in the
framework.


Backward compatibility can look messy.

I can include the patch that converts all platforms at once. But as I
said it is completely untested.

So I'm curious, what is the policy in V4L2 community regarding
merging untested patches? Do we go ahead and merge and then
fixup errors as they are discovered, or should the patch get basic
validation by everyone who has access to the affected hardware
first?

Steve

Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Sakari Ailus]

On Wed, May 09, 2018 at 04:06:32PM -0700, Steve Longerbeam wrote:
> 
> 
> On 05/08/2018 03:12 AM, Sakari Ailus wrote:
> > On Fri, Apr 20, 2018 at 10:12:33AM -0700, Steve Longerbeam wrote:
> > > Hi Sakari,
> > > 
> > > 
> > > On 04/20/2018 05:24 AM, Sakari Ailus wrote:
> > > > Hi Steve,
> > > > 
> > > > Thanks for the patchset.
> > > > 
> > > > On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:
> > > > > v4l2_async_notifier_add_subdev() adds an asd to the notifier. It 
> > > > > checks
> > > > > that the asd's match_type is valid and 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 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.
> > > > > 
> > > > > In v4l2_async_notifier_has_async_subdev(), 
> > > > > __v4l2_async_notifier_register(),
> > > > > and v4l2_async_notifier_cleanup(), alternatively operate on the 
> > > > > subdevs
> > > > > array or a non-empty notifier->asd_list.
> > > > I do agree with the approach, but this patch leaves the remaining users 
> > > > of
> > > > the subdevs array in the notifier intact. Could you rework them to use 
> > > > the
> > > > v4l2_async_notifier_add_subdev() as well?
> > > > 
> > > > There seem to be just a few of them --- exynos4-is and rcar_drif.
> > > I count more than a few :)
> > > 
> > > % cd drivers/media && grep -l -r --include "*.[ch]"
> > > 'notifier[\.\-]>*subdevs[   ]*='
> > > v4l2-core/v4l2-async.c
> > > platform/pxa_camera.c
> > > platform/ti-vpe/cal.c
> > > platform/exynos4-is/media-dev.c
> > > platform/qcom/camss-8x16/camss.c
> > > platform/soc_camera/soc_camera.c
> > > platform/atmel/atmel-isi.c
> > > platform/atmel/atmel-isc.c
> > > platform/stm32/stm32-dcmi.c
> > > platform/davinci/vpif_capture.c
> > > platform/davinci/vpif_display.c
> > > platform/renesas-ceu.c
> > > platform/am437x/am437x-vpfe.c
> > > platform/xilinx/xilinx-vipp.c
> > > platform/rcar_drif.c
> > > 
> > > 
> > > So not including v4l2-core, the list is:
> > > 
> > > pxa_camera
> > > ti-vpe
> > > exynos4-is
> > > qcom
> > > soc_camera
> > > atmel
> > > stm32
> > > davinci
> > > renesas-ceu
> > > am437x
> > > xilinx
> > > rcar_drif
> > > 
> > > 
> > > Given such a large list of the users of the notifier->subdevs array,
> > > I think this should be done is two steps: submit this patchset first,
> > > that keeps the backward compatibility, and then a subsequent patchset
> > > that converts all drivers to use v4l2_async_notifier_add_subdev(), at
> > > which point the subdevs array can be removed from struct
> > > v4l2_async_notifier.
> > > 
> > > Or, do you still think this should be done all at once? I could add a
> > > large patch to this patchset that does the conversion and removes
> > > the subdevs array.
> > Sorry for the delay. I grepped for this, too, but I guess I ended up using
> > an expression that missed most of the users. :-(
> > 
> > I think it'd be very good to perform that conversion --- the code in the
> > framework would be quite a bit cleaner and easier to maintain whereas the
> > per-driver conversions seem pretty simple, such as this on in
> > drivers/media/platform/atmel/atmel-isi.c :
> > 
> > /* Register the subdevices notifier. */
> > subdevs = devm_kzalloc(isi->dev, sizeof(*subdevs), GFP_KERNEL);
> > if (!subdevs) {
> > of_node_put(isi->entity.node);
> > return -ENOMEM;
> > }
> > 
> > subdevs[0] = >entity.asd;
> > 
> > isi->notifier.subdevs = subdevs;
> > isi->notifier.num_subdevs = 1;
> > isi->notifier.ops = _graph_notify_ops;
> 
> 
> Yes, the conversions are fairly straightforward. I've completed that work,
> but it was a very manual conversion, every platform is different and needed
> careful study.
> 
> Although I was careful about getting the error-out paths correct, there
> could
> be mistakes there, which would result in memory leaks. And obviously I can't
> re-test all these platforms. So this patch is very high-risk. More eyes are
> needed on it, ideally the maintainer(s) of each affected platform.
> 
> I just submitted v4 of this series, but did not include this large un-tested
> patch in v4 for those reasons.
> 
> Instead, this patch, and follow-up patches that strips support for subdevs
> array altogether from v4l2-async.c, and updates rst docs, are available at
> my
> media-tree mirror on github:
> 
> g...@github.com:slongerbeam/mediatree.git
> 
> in the branch 'remove-subdevs-array'. The branch is based off this series
> (branch 

Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Steve Longerbeam]

On 05/08/2018 03:12 AM, Sakari Ailus wrote:

On Fri, Apr 20, 2018 at 10:12:33AM -0700, Steve Longerbeam wrote:

Hi Sakari,


On 04/20/2018 05:24 AM, Sakari Ailus wrote:

Hi Steve,

Thanks for the patchset.

On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:

v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
that the asd's match_type is valid and 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 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.

In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(),
and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
array or a non-empty notifier->asd_list.

I do agree with the approach, but this patch leaves the remaining users of
the subdevs array in the notifier intact. Could you rework them to use the
v4l2_async_notifier_add_subdev() as well?

There seem to be just a few of them --- exynos4-is and rcar_drif.

I count more than a few :)

% cd drivers/media && grep -l -r --include "*.[ch]"
'notifier[\.\-]>*subdevs[   ]*='
v4l2-core/v4l2-async.c
platform/pxa_camera.c
platform/ti-vpe/cal.c
platform/exynos4-is/media-dev.c
platform/qcom/camss-8x16/camss.c
platform/soc_camera/soc_camera.c
platform/atmel/atmel-isi.c
platform/atmel/atmel-isc.c
platform/stm32/stm32-dcmi.c
platform/davinci/vpif_capture.c
platform/davinci/vpif_display.c
platform/renesas-ceu.c
platform/am437x/am437x-vpfe.c
platform/xilinx/xilinx-vipp.c
platform/rcar_drif.c


So not including v4l2-core, the list is:

pxa_camera
ti-vpe
exynos4-is
qcom
soc_camera
atmel
stm32
davinci
renesas-ceu
am437x
xilinx
rcar_drif


Given such a large list of the users of the notifier->subdevs array,
I think this should be done is two steps: submit this patchset first,
that keeps the backward compatibility, and then a subsequent patchset
that converts all drivers to use v4l2_async_notifier_add_subdev(), at
which point the subdevs array can be removed from struct
v4l2_async_notifier.

Or, do you still think this should be done all at once? I could add a
large patch to this patchset that does the conversion and removes
the subdevs array.

Sorry for the delay. I grepped for this, too, but I guess I ended up using
an expression that missed most of the users. :-(

I think it'd be very good to perform that conversion --- the code in the
framework would be quite a bit cleaner and easier to maintain whereas the
per-driver conversions seem pretty simple, such as this on in
drivers/media/platform/atmel/atmel-isi.c :

/* Register the subdevices notifier. */
subdevs = devm_kzalloc(isi->dev, sizeof(*subdevs), GFP_KERNEL);
if (!subdevs) {
of_node_put(isi->entity.node);
return -ENOMEM;
}

subdevs[0] = >entity.asd;

isi->notifier.subdevs = subdevs;
isi->notifier.num_subdevs = 1;
isi->notifier.ops = _graph_notify_ops;



Yes, the conversions are fairly straightforward. I've completed that work,
but it was a very manual conversion, every platform is different and needed
careful study.

Although I was careful about getting the error-out paths correct, there 
could

be mistakes there, which would result in memory leaks. And obviously I can't
re-test all these platforms. So this patch is very high-risk. More eyes are
needed on it, ideally the maintainer(s) of each affected platform.

I just submitted v4 of this series, but did not include this large un-tested
patch in v4 for those reasons.

Instead, this patch, and follow-up patches that strips support for subdevs
array altogether from v4l2-async.c, and updates rst docs, are available 
at my

media-tree mirror on github:

g...@github.com:slongerbeam/mediatree.git

in the branch 'remove-subdevs-array'. The branch is based off this series
(branch 'imx-subdev-notifiers.6').

Steve

Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Sakari Ailus]

On Fri, Apr 20, 2018 at 10:12:33AM -0700, Steve Longerbeam wrote:
> Hi Sakari,
> 
> 
> On 04/20/2018 05:24 AM, Sakari Ailus wrote:
> > Hi Steve,
> > 
> > Thanks for the patchset.
> > 
> > On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:
> > > v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
> > > that the asd's match_type is valid and 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 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.
> > > 
> > > In v4l2_async_notifier_has_async_subdev(), 
> > > __v4l2_async_notifier_register(),
> > > and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
> > > array or a non-empty notifier->asd_list.
> > I do agree with the approach, but this patch leaves the remaining users of
> > the subdevs array in the notifier intact. Could you rework them to use the
> > v4l2_async_notifier_add_subdev() as well?
> > 
> > There seem to be just a few of them --- exynos4-is and rcar_drif.
> 
> I count more than a few :)
> 
> % cd drivers/media && grep -l -r --include "*.[ch]"
> 'notifier[\.\-]>*subdevs[   ]*='
> v4l2-core/v4l2-async.c
> platform/pxa_camera.c
> platform/ti-vpe/cal.c
> platform/exynos4-is/media-dev.c
> platform/qcom/camss-8x16/camss.c
> platform/soc_camera/soc_camera.c
> platform/atmel/atmel-isi.c
> platform/atmel/atmel-isc.c
> platform/stm32/stm32-dcmi.c
> platform/davinci/vpif_capture.c
> platform/davinci/vpif_display.c
> platform/renesas-ceu.c
> platform/am437x/am437x-vpfe.c
> platform/xilinx/xilinx-vipp.c
> platform/rcar_drif.c
> 
> 
> So not including v4l2-core, the list is:
> 
> pxa_camera
> ti-vpe
> exynos4-is
> qcom
> soc_camera
> atmel
> stm32
> davinci
> renesas-ceu
> am437x
> xilinx
> rcar_drif
> 
> 
> Given such a large list of the users of the notifier->subdevs array,
> I think this should be done is two steps: submit this patchset first,
> that keeps the backward compatibility, and then a subsequent patchset
> that converts all drivers to use v4l2_async_notifier_add_subdev(), at
> which point the subdevs array can be removed from struct
> v4l2_async_notifier.
> 
> Or, do you still think this should be done all at once? I could add a
> large patch to this patchset that does the conversion and removes
> the subdevs array.

Sorry for the delay. I grepped for this, too, but I guess I ended up using
an expression that missed most of the users. :-(

I think it'd be very good to perform that conversion --- the code in the
framework would be quite a bit cleaner and easier to maintain whereas the
per-driver conversions seem pretty simple, such as this on in
drivers/media/platform/atmel/atmel-isi.c :

/* Register the subdevices notifier. */
subdevs = devm_kzalloc(isi->dev, sizeof(*subdevs), GFP_KERNEL);
if (!subdevs) {
of_node_put(isi->entity.node);
return -ENOMEM;
}

subdevs[0] = >entity.asd;

isi->notifier.subdevs = subdevs;
isi->notifier.num_subdevs = 1;
isi->notifier.ops = _graph_notify_ops;

> 
> Steve
> 
> 
> > 
> > > Signed-off-by: Steve Longerbeam 
> > > ---
> > > 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 | 206 
> > > +++
> > >   include/media/v4l2-async.h   |  22 
> > >   2 files changed, 184 insertions(+), 44 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> > > b/drivers/media/v4l2-core/v4l2-async.c
> > > index b59bbac..7b7f7e2 100644
> > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > @@ -366,16 +366,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(_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 

Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Steve Longerbeam]

Hi Sakari,


On 04/20/2018 05:24 AM, Sakari Ailus wrote:

Hi Steve,

Thanks for the patchset.

On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:

v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
that the asd's match_type is valid and 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 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.

In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(),
and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
array or a non-empty notifier->asd_list.

I do agree with the approach, but this patch leaves the remaining users of
the subdevs array in the notifier intact. Could you rework them to use the
v4l2_async_notifier_add_subdev() as well?

There seem to be just a few of them --- exynos4-is and rcar_drif.


I count more than a few :)

% cd drivers/media && grep -l -r --include "*.[ch]" 
'notifier[\.\-]>*subdevs[   ]*='

v4l2-core/v4l2-async.c
platform/pxa_camera.c
platform/ti-vpe/cal.c
platform/exynos4-is/media-dev.c
platform/qcom/camss-8x16/camss.c
platform/soc_camera/soc_camera.c
platform/atmel/atmel-isi.c
platform/atmel/atmel-isc.c
platform/stm32/stm32-dcmi.c
platform/davinci/vpif_capture.c
platform/davinci/vpif_display.c
platform/renesas-ceu.c
platform/am437x/am437x-vpfe.c
platform/xilinx/xilinx-vipp.c
platform/rcar_drif.c


So not including v4l2-core, the list is:

pxa_camera
ti-vpe
exynos4-is
qcom
soc_camera
atmel
stm32
davinci
renesas-ceu
am437x
xilinx
rcar_drif


Given such a large list of the users of the notifier->subdevs array,
I think this should be done is two steps: submit this patchset first,
that keeps the backward compatibility, and then a subsequent patchset
that converts all drivers to use v4l2_async_notifier_add_subdev(), at
which point the subdevs array can be removed from struct 
v4l2_async_notifier.


Or, do you still think this should be done all at once? I could add a
large patch to this patchset that does the conversion and removes
the subdevs array.

Steve





Signed-off-by: Steve Longerbeam 
---
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 | 206 +++
  include/media/v4l2-async.h   |  22 
  2 files changed, 184 insertions(+), 44 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c 
b/drivers/media/v4l2-core/v4l2-async.c
index b59bbac..7b7f7e2 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -366,16 +366,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(_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, >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. */

@@ -386,10 +396,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 

Re: [PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Sakari Ailus]

Hi Steve,

Thanks for the patchset.

On Tue, Mar 20, 2018 at 05:37:19PM -0700, Steve Longerbeam wrote:
> v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
> that the asd's match_type is valid and 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 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.
> 
> In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(),
> and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
> array or a non-empty notifier->asd_list.

I do agree with the approach, but this patch leaves the remaining users of
the subdevs array in the notifier intact. Could you rework them to use the
v4l2_async_notifier_add_subdev() as well?

There seem to be just a few of them --- exynos4-is and rcar_drif.

> 
> Signed-off-by: Steve Longerbeam 
> ---
> 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 | 206 
> +++
>  include/media/v4l2-async.h   |  22 
>  2 files changed, 184 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c 
> b/drivers/media/v4l2-core/v4l2-async.c
> index b59bbac..7b7f7e2 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -366,16 +366,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(_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, >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. */
> @@ -386,10 +396,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;
> +}
> +
> +static void __v4l2_async_notifier_init(struct v4l2_async_notifier *notifier)
> +{
> + lockdep_assert_held(_lock);
> +
> + INIT_LIST_HEAD(>asd_list);
> + INIT_LIST_HEAD(>waiting);
> + INIT_LIST_HEAD(>done);
> + notifier->lists_initialized = true;
> +}
> +
> +static int __v4l2_async_notifier_register(struct v4l2_async_notifier 
> *notifier)
> +{
>   struct v4l2_async_subdev *asd;
>   int ret;
>   int i;
> @@ -397,34 +443,40 @@ static int __v4l2_async_notifier_register(struct 
> v4l2_async_notifier *notifier)
>   if (notifier->num_subdevs > V4L2_MAX_SUBDEVS)
>   return -EINVAL;
>  
> - INIT_LIST_HEAD(>waiting);
> - INIT_LIST_HEAD(>done);
> -
>   mutex_lock(_lock);
>  
> - for (i = 0; i < notifier->num_subdevs; i++) {
> - asd 

[PATCH v3 03/13] media: v4l2: async: Add v4l2_async_notifier_add_subdev

[Steve Longerbeam]

v4l2_async_notifier_add_subdev() adds an asd to the notifier. It checks
that the asd's match_type is valid and 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 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.

In v4l2_async_notifier_has_async_subdev(), __v4l2_async_notifier_register(),
and v4l2_async_notifier_cleanup(), alternatively operate on the subdevs
array or a non-empty notifier->asd_list.

Signed-off-by: Steve Longerbeam 
---
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 | 206 +++
 include/media/v4l2-async.h   |  22 
 2 files changed, 184 insertions(+), 44 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c 
b/drivers/media/v4l2-core/v4l2-async.c
index b59bbac..7b7f7e2 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -366,16 +366,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(_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, >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. */
@@ -386,10 +396,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;
+}
+
+static void __v4l2_async_notifier_init(struct v4l2_async_notifier *notifier)
+{
+   lockdep_assert_held(_lock);
+
+   INIT_LIST_HEAD(>asd_list);
+   INIT_LIST_HEAD(>waiting);
+   INIT_LIST_HEAD(>done);
+   notifier->lists_initialized = true;
+}
+
+static int __v4l2_async_notifier_register(struct v4l2_async_notifier *notifier)
+{
struct v4l2_async_subdev *asd;
int ret;
int i;
@@ -397,34 +443,40 @@ static int __v4l2_async_notifier_register(struct 
v4l2_async_notifier *notifier)
if (notifier->num_subdevs > V4L2_MAX_SUBDEVS)
return -EINVAL;
 
-   INIT_LIST_HEAD(>waiting);
-   INIT_LIST_HEAD(>done);
-
mutex_lock(_lock);
 
-   for (i = 0; i < notifier->num_subdevs; i++) {
-   asd = notifier->subdevs[i];
+   if (!notifier->lists_initialized)
+   __v4l2_async_notifier_init(notifier);
 
-   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)) {
-