[2/2] media: vivid: Introduce VIDEO_VIVID_OSD

vivid-osd depends on CONFIG_FB, which can be a large dependency. Introduce
CONFIG_VIDEO_VIVID_OSD to control enabling support for testing output
overlay.

Suggested-by: Slawomir Rosek <srosek@google.com>
Co-developed-by: Slawomir Rosek <srosek@google.com>
Signed-off-by: Slawomir Rosek <srosek@google.com>
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/test-drivers/vivid/Kconfig         | 12 ++++++++++--
 drivers/media/test-drivers/vivid/Makefile        |  5 ++++-
 drivers/media/test-drivers/vivid/vivid-core.c    |  4 ++++
 drivers/media/test-drivers/vivid/vivid-core.h    |  2 ++
 drivers/media/test-drivers/vivid/vivid-osd.h     | 11 +++++++++++
 drivers/media/test-drivers/vivid/vivid-vid-out.c |  7 ++++---
 6 files changed, 35 insertions(+), 6 deletions(-)

Hi Ricardo,

I like this change, but I have a few comments:

On 28/02/2025 14:08, Ricardo Ribalda wrote:
> vivid-osd depends on CONFIG_FB, which can be a large dependency. Introduce
> CONFIG_VIDEO_VIVID_OSD to control enabling support for testing output
> overlay.
> 
> Suggested-by: Slawomir Rosek <srosek@google.com>
> Co-developed-by: Slawomir Rosek <srosek@google.com>
> Signed-off-by: Slawomir Rosek <srosek@google.com>
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/test-drivers/vivid/Kconfig         | 12 ++++++++++--
>  drivers/media/test-drivers/vivid/Makefile        |  5 ++++-
>  drivers/media/test-drivers/vivid/vivid-core.c    |  4 ++++
>  drivers/media/test-drivers/vivid/vivid-core.h    |  2 ++
>  drivers/media/test-drivers/vivid/vivid-osd.h     | 11 +++++++++++
>  drivers/media/test-drivers/vivid/vivid-vid-out.c |  7 ++++---
>  6 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/test-drivers/vivid/Kconfig b/drivers/media/test-drivers/vivid/Kconfig
> index ec2e71d76965..e95edc0f22bf 100644
> --- a/drivers/media/test-drivers/vivid/Kconfig
> +++ b/drivers/media/test-drivers/vivid/Kconfig
> @@ -1,9 +1,8 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  config VIDEO_VIVID
>  	tristate "Virtual Video Test Driver"
> -	depends on VIDEO_DEV && !SPARC32 && !SPARC64 && FB
> +	depends on VIDEO_DEV && !SPARC32 && !SPARC64
>  	depends on HAS_DMA
> -	select FB_IOMEM_HELPERS
>  	select FONT_SUPPORT
>  	select FONT_8x16
>  	select VIDEOBUF2_VMALLOC
> @@ -31,6 +30,15 @@ config VIDEO_VIVID_CEC
>  	  When selected the vivid module will emulate the optional
>  	  HDMI CEC feature.
>  
> +config VIDEO_VIVID_OSD
> +	bool "Enable Framebuffer for testing Output Overlay"
> +	depends on VIDEO_VIVID && FB
> +	default y
> +	select FB_IOMEM_HELPERS
> +	help
> +	  When selected the vivid module will emulate a Framebuffer for
> +	  testing Output Overlay.
> +
>  config VIDEO_VIVID_MAX_DEVS
>  	int "Maximum number of devices"
>  	depends on VIDEO_VIVID
> diff --git a/drivers/media/test-drivers/vivid/Makefile b/drivers/media/test-drivers/vivid/Makefile
> index b12ad0152a3e..284a59e97335 100644
> --- a/drivers/media/test-drivers/vivid/Makefile
> +++ b/drivers/media/test-drivers/vivid/Makefile
> @@ -3,10 +3,13 @@ vivid-objs := vivid-core.o vivid-ctrls.o vivid-vid-common.o vivid-vbi-gen.o \
>  		vivid-vid-cap.o vivid-vid-out.o vivid-kthread-cap.o vivid-kthread-out.o \
>  		vivid-radio-rx.o vivid-radio-tx.o vivid-radio-common.o \
>  		vivid-rds-gen.o vivid-sdr-cap.o vivid-vbi-cap.o vivid-vbi-out.o \
> -		vivid-osd.o vivid-meta-cap.o vivid-meta-out.o \
> +		vivid-meta-cap.o vivid-meta-out.o \
>  		vivid-kthread-touch.o vivid-touch-cap.o
>  ifeq ($(CONFIG_VIDEO_VIVID_CEC),y)
>    vivid-objs += vivid-cec.o
>  endif
> +ifeq ($(CONFIG_VIDEO_VIVID_OSD),y)
> +  vivid-objs += vivid-osd.o
> +endif
>  
>  obj-$(CONFIG_VIDEO_VIVID) += vivid.o
> diff --git a/drivers/media/test-drivers/vivid/vivid-core.c b/drivers/media/test-drivers/vivid/vivid-core.c
> index 10f5bef3f49c..6af12a76a067 100644
> --- a/drivers/media/test-drivers/vivid/vivid-core.c
> +++ b/drivers/media/test-drivers/vivid/vivid-core.c
> @@ -125,7 +125,9 @@ MODULE_PARM_DESC(node_types, " node types, default is 0xe1d3d. Bitmask with the
>  			     "\t\t    bit 8: Video Output node\n"
>  			     "\t\t    bit 10-11: VBI Output node: 0 = none, 1 = raw vbi, 2 = sliced vbi, 3 = both\n"
>  			     "\t\t    bit 12: Radio Transmitter node\n"
> +#if IS_ENABLED(CONFIG_VIDEO_VIVID_OSD)

Just use #ifdef CONFIG_VIDEO_VIVID_OSD. It's a simple bool config option, after all.

>  			     "\t\t    bit 16: Framebuffer for testing output overlays\n"
> +#endif
>  			     "\t\t    bit 17: Metadata Capture node\n"
>  			     "\t\t    bit 18: Metadata Output node\n"
>  			     "\t\t    bit 19: Touch Capture node\n");
> @@ -1071,9 +1073,11 @@ static int vivid_detect_feature_set(struct vivid_dev *dev, int inst,
>  	/* do we have a modulator? */
>  	*has_modulator = dev->has_radio_tx;
>  
> +#if IS_ENABLED(CONFIG_VIDEO_VIVID_OSD)
>  	if (dev->has_vid_cap)
>  		/* do we have a framebuffer for overlay testing? */
>  		dev->has_fb = node_type & 0x10000;
> +#endif
>  
>  	/* can we do crop/compose/scaling while capturing? */
>  	if (no_error_inj && *ccs_cap == -1)
> diff --git a/drivers/media/test-drivers/vivid/vivid-core.h b/drivers/media/test-drivers/vivid/vivid-core.h
> index d2d52763b119..72bd48031ba0 100644
> --- a/drivers/media/test-drivers/vivid/vivid-core.h
> +++ b/drivers/media/test-drivers/vivid/vivid-core.h
> @@ -403,9 +403,11 @@ struct vivid_dev {
>  	int				display_byte_stride;
>  	int				bits_per_pixel;
>  	int				bytes_per_pixel;
> +#if IS_ENABLED(CONFIG_VIDEO_VIVID_OSD)
>  	struct fb_info			fb_info;
>  	struct fb_var_screeninfo	fb_defined;
>  	struct fb_fix_screeninfo	fb_fix;
> +#endif
>  
>  	/* Error injection */
>  	bool				disconnect_error;
> diff --git a/drivers/media/test-drivers/vivid/vivid-osd.h b/drivers/media/test-drivers/vivid/vivid-osd.h
> index 883459552fa9..6830e6d63dc5 100644
> --- a/drivers/media/test-drivers/vivid/vivid-osd.h
> +++ b/drivers/media/test-drivers/vivid/vivid-osd.h
> @@ -8,8 +8,19 @@
>  #ifndef _VIVID_OSD_H_
>  #define _VIVID_OSD_H_
>  
> +#if IS_ENABLED(CONFIG_VIDEO_VIVID_OSD)
>  int vivid_fb_init(struct vivid_dev *dev);
>  void vivid_fb_deinit(struct vivid_dev *dev);
>  void vivid_clear_fb(struct vivid_dev *dev);

Let's rename this to vivid_fb_clear (that can be done in the previous patch).

That way all these functions use a common prefix.

> +#else
> +static inline int vivid_fb_init(struct vivid_dev *dev)
> +{
> +	return -ENODEV;
> +}
> +
> +static inline void vivid_fb_deinit(struct vivid_dev *dev) {}
> +

Just drop this empty line.

> +static inline void vivid_clear_fb(struct vivid_dev *dev) {}
> +#endif
>  
>  #endif
> diff --git a/drivers/media/test-drivers/vivid/vivid-vid-out.c b/drivers/media/test-drivers/vivid/vivid-vid-out.c
> index 5ec84db934d6..75b24751b9a4 100644
> --- a/drivers/media/test-drivers/vivid/vivid-vid-out.c
> +++ b/drivers/media/test-drivers/vivid/vivid-vid-out.c
> @@ -907,10 +907,11 @@ int vivid_vid_out_g_fbuf(struct file *file, void *fh,
>  	a->base = (void *)dev->video_pbase;
>  	a->fmt.width = dev->display_width;
>  	a->fmt.height = dev->display_height;
> -	if (dev->fb_defined.green.length == 5)
> -		a->fmt.pixelformat = V4L2_PIX_FMT_ARGB555;
> -	else
> +	a->fmt.pixelformat = V4L2_PIX_FMT_ARGB555;
> +#if IS_ENABLED(CONFIG_VIDEO_VIVID_FB)
> +	if (dev->fb_defined.green.length != 5)
>  		a->fmt.pixelformat = V4L2_PIX_FMT_RGB565;
> +#endif

I prefer that this uses a new helper: vivid_fb_green_bits(). Again, that can
be done in the previous patch. With that helper you can keep the original
if-else construct, which is easier to read.

The helper should return 5 if CONFIG_VIDEO_VIVID_OSD is not defined.

>  	a->fmt.bytesperline = dev->display_byte_stride;
>  	a->fmt.sizeimage = a->fmt.height * a->fmt.bytesperline;
>  	a->fmt.field = V4L2_FIELD_NONE;
> 

Regards,

	Hans