Hi,
On 1/20/23 13:47, Sakari Ailus wrote:
> Hi Hans,
>
> Many thanks for working on this.
You are welcome.
> On Fri, Jan 20, 2023 at 12:45:19PM +0100, Hans de Goede wrote:
>> Currently the videodev.ko code may be builtin while e.g. v4l2-fwnode.ko
>> is build as a module.
>>
>> This makes it hard to add code depending on other subsystems spanning
>> both videodev.ko and v4l2-fwnode.ko. Specifically this block adding code
>> depending on the LED subsystem.
>>
>> This is made even harder because CONFIG_V4L2_FWNODE is selected,
>> not depended on so it itself cannot depend on another subsystem without
>> editing all the Kconfig symbols selecting it to also list the dependency
>> and there are many of such symbols.
>>
>> Adding a "select LED_CLASS if NEW_LEDS" to CONFIG_V4L2_FWNODE leads
>> to Kconfig erroring out with "error: recursive dependency detected!".
>>
>> To fix this dependency mess, change the V4L2_FWNODE and V4L2_ASYNC
>> (which V4L2_FWNODE selects) Kconfig symbols from tristate to bools and
>> link their code into videodev.ko instead of making them separate modules.
>>
>> This will allow using IS_REACHABLE(LED_CLASS) for the new LED integration
>> code without needing to worry that it expands to 0 in some places and
>> 1 in other places because some of the code being builtin vs modular.
>>
>> On x86_64 this leads to the following size changes for videodev.ko
>>
>> [hans@shalem linux]$ size drivers/media/v4l2-core/videodev.ko
>>
>> Before:
>> text data bss dec hex filename
>> 218206 14395 2448 235049 39629 drivers/media/v4l2-core/videodev.ko
>> After:
>> text data bss dec hex filename
>> 243213 17615 2456 263284 40474 drivers/media/v4l2-core/videodev.ko
>>
>> So (as expected) there is some increase in size here, but it
>> really is not that much.
>>
>> And the uncompressed no-debuginfo .ko file disk-usage actually shrinks
>> by 17 KiB (comparing the slightly larger videodev.ko against the
>> 3 original modules) and loading time will also be better.
>>
>> Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
>> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
>> ---
>> Changes in v5:
>> - Add a new v4l2-dev-priv.h for the async debugfs prototypes and add
>> static inline wrappers there when CONFIG_V4L2_ASYNC is not enabled
>>
>> Changes in v4:
>> - New patch in v4 of this patch-set
>> ---
>> drivers/media/v4l2-core/Kconfig | 4 ++--
>> drivers/media/v4l2-core/Makefile | 4 ++--
>> drivers/media/v4l2-core/v4l2-async.c | 17 ++++-------------
>> drivers/media/v4l2-core/v4l2-dev-priv.h | 19 +++++++++++++++++++
>> drivers/media/v4l2-core/v4l2-dev.c | 8 ++++++++
>> drivers/media/v4l2-core/v4l2-fwnode.c | 6 ------
>> 6 files changed, 35 insertions(+), 23 deletions(-)
>> create mode 100644 drivers/media/v4l2-core/v4l2-dev-priv.h
>>
>> diff --git a/drivers/media/v4l2-core/Kconfig b/drivers/media/v4l2-core/Kconfig
>> index 348559bc2468..73574d946010 100644
>> --- a/drivers/media/v4l2-core/Kconfig
>> +++ b/drivers/media/v4l2-core/Kconfig
>> @@ -68,11 +68,11 @@ config V4L2_FLASH_LED_CLASS
>> When in doubt, say N.
>>
>> config V4L2_FWNODE
>> - tristate
>> + bool
>> select V4L2_ASYNC
>>
>> config V4L2_ASYNC
>> - tristate
>> + bool
>>
>> # Used by drivers that need Videobuf modules
>> config VIDEOBUF_GEN
>> diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
>> index 41d91bd10cf2..8c5a1ab8d939 100644
>> --- a/drivers/media/v4l2-core/Makefile
>> +++ b/drivers/media/v4l2-core/Makefile
>> @@ -15,7 +15,9 @@ videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
>>
>> # Please keep it alphabetically sorted by Kconfig name
>> # (e. g. LC_ALL=C sort Makefile)
>> +videodev-$(CONFIG_V4L2_ASYNC) += v4l2-async.o
>> videodev-$(CONFIG_COMPAT) += v4l2-compat-ioctl32.o
>> +videodev-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
>> videodev-$(CONFIG_MEDIA_CONTROLLER) += v4l2-mc.o
>> videodev-$(CONFIG_SPI) += v4l2-spi.o
>> videodev-$(CONFIG_TRACEPOINTS) += v4l2-trace.o
>> @@ -24,9 +26,7 @@ videodev-$(CONFIG_VIDEO_V4L2_I2C) += v4l2-i2c.o
>> # Please keep it alphabetically sorted by Kconfig name
>> # (e. g. LC_ALL=C sort Makefile)
>>
>> -obj-$(CONFIG_V4L2_ASYNC) += v4l2-async.o
>> obj-$(CONFIG_V4L2_FLASH_LED_CLASS) += v4l2-flash-led-class.o
>> -obj-$(CONFIG_V4L2_FWNODE) += v4l2-fwnode.o
>> obj-$(CONFIG_V4L2_H264) += v4l2-h264.o
>> obj-$(CONFIG_V4L2_JPEG_HELPER) += v4l2-jpeg.o
>> obj-$(CONFIG_V4L2_MEM2MEM_DEV) += v4l2-mem2mem.o
>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
>> index 2f1b718a9189..024d6b82b50a 100644
>> --- a/drivers/media/v4l2-core/v4l2-async.c
>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>> @@ -11,7 +11,6 @@
>> #include <linux/i2c.h>
>> #include <linux/list.h>
>> #include <linux/mm.h>
>> -#include <linux/module.h>
>> #include <linux/mutex.h>
>> #include <linux/of.h>
>> #include <linux/platform_device.h>
>> @@ -24,6 +23,8 @@
>> #include <media/v4l2-fwnode.h>
>> #include <media/v4l2-subdev.h>
>>
>> +#include "v4l2-dev-priv.h"
>> +
>> static int v4l2_async_nf_call_bound(struct v4l2_async_notifier *n,
>> struct v4l2_subdev *subdev,
>> struct v4l2_async_subdev *asd)
>> @@ -900,25 +901,15 @@ DEFINE_SHOW_ATTRIBUTE(pending_subdevs);
>>
>> static struct dentry *v4l2_async_debugfs_dir;
>>
>> -static int __init v4l2_async_init(void)
>> +void __init v4l2_async_debugfs_init(void)
>> {
>> v4l2_async_debugfs_dir = debugfs_create_dir("v4l2-async", NULL);
>> debugfs_create_file("pending_async_subdevices", 0444,
>> v4l2_async_debugfs_dir, NULL,
>> &pending_subdevs_fops);
>> -
>> - return 0;
>> }
>>
>> -static void __exit v4l2_async_exit(void)
>> +void __exit v4l2_async_debugfs_exit(void)
>> {
>> debugfs_remove_recursive(v4l2_async_debugfs_dir);
>> }
>> -
>> -subsys_initcall(v4l2_async_init);
>> -module_exit(v4l2_async_exit);
>> -
>> -MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@xxxxxx>");
>> -MODULE_AUTHOR("Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>");
>> -MODULE_AUTHOR("Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>");
>> -MODULE_LICENSE("GPL");
>> diff --git a/drivers/media/v4l2-core/v4l2-dev-priv.h b/drivers/media/v4l2-core/v4l2-dev-priv.h
>> new file mode 100644
>> index 000000000000..b5b1ee78be20
>> --- /dev/null
>> +++ b/drivers/media/v4l2-core/v4l2-dev-priv.h
>
> Could we call this v4l2-async-debugfs.h? I don't necessarily expect more
> material here.
Renaming it is fine by my. I have renamed it to
v4l2-async-debugfs.h for the upcoming v6 of this series.
Regards,
Hans
>
>> @@ -0,0 +1,19 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * Video capture interface for Linux version 2 private header.
>> + *
>> + * Copyright (C) 2023 Hans de Goede <hdegoede@xxxxxxxxxx>
>> + */
>> +
>> +#ifndef _V4L2_DEV_PRIV_H_
>> +#define _V4L2_DEV_PRIV_H_
>> +
>> +#if IS_ENABLED(CONFIG_V4L2_ASYNC)
>> +void v4l2_async_debugfs_init(void);
>> +void v4l2_async_debugfs_exit(void);
>> +#else
>> +static inline void v4l2_async_debugfs_init(void) {}
>> +static inline void v4l2_async_debugfs_exit(void) {}
>> +#endif
>> +
>> +#endif
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index 397d553177fa..10ba2e4196a6 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -31,6 +31,8 @@
>> #include <media/v4l2-ioctl.h>
>> #include <media/v4l2-event.h>
>>
>> +#include "v4l2-dev-priv.h"
>> +
>> #define VIDEO_NUM_DEVICES 256
>> #define VIDEO_NAME "video4linux"
>>
>> @@ -1190,6 +1192,7 @@ static int __init videodev_init(void)
>> return -EIO;
>> }
>>
>> + v4l2_async_debugfs_init();
>> return 0;
>> }
>>
>> @@ -1197,6 +1200,7 @@ static void __exit videodev_exit(void)
>> {
>> dev_t dev = MKDEV(VIDEO_MAJOR, 0);
>>
>> + v4l2_async_debugfs_exit();
>> class_unregister(&video_class);
>> unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
>> }
>> @@ -1205,6 +1209,10 @@ subsys_initcall(videodev_init);
>> module_exit(videodev_exit)
>>
>> MODULE_AUTHOR("Alan Cox, Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>, Bill Dirks, Justin Schoeman, Gerd Knorr");
>> +MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@xxxxxx>");
>> +MODULE_AUTHOR("Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>");
>> +MODULE_AUTHOR("Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx>");
>> +MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>");
>> MODULE_DESCRIPTION("Video4Linux2 core driver");
>> MODULE_LICENSE("GPL");
>> MODULE_ALIAS_CHARDEV_MAJOR(VIDEO_MAJOR);
>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
>> index 3d9533c1b202..c8a2264262bc 100644
>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
>> @@ -17,7 +17,6 @@
>> #include <linux/acpi.h>
>> #include <linux/kernel.h>
>> #include <linux/mm.h>
>> -#include <linux/module.h>
>> #include <linux/of.h>
>> #include <linux/property.h>
>> #include <linux/slab.h>
>> @@ -1328,8 +1327,3 @@ int v4l2_async_register_subdev_sensor(struct v4l2_subdev *sd)
>> return ret;
>> }
>> EXPORT_SYMBOL_GPL(v4l2_async_register_subdev_sensor);
>> -
>> -MODULE_LICENSE("GPL");
>> -MODULE_AUTHOR("Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>");
>> -MODULE_AUTHOR("Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>");
>> -MODULE_AUTHOR("Guennadi Liakhovetski <g.liakhovetski@xxxxxx>");
>> --
>> 2.39.0
>>
>
