Re: [PATCH v5 06/11] media: v4l2-core: Built async and fwnode code into videodev.ko

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
>>
> 




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux