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] = &isi->entity.asd;
isi->notifier.subdevs = subdevs;
isi->notifier.num_subdevs = 1;
isi->notifier.ops = &isi_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
