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

Many thanks for working on this.

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.

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

-- 
Sakari Ailus



[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