Re: [PATCH 2/2] media: vivid: Introduce VIDEO_VIVID_OSD

From: Hans Verkuil
Date: Thu Mar 06 2025 - 05:03:31 EST

Next message: tip-bot2 for Shrikanth Hegde: " [tip: sched/urgent] sched/deadline: Use online cpus for validating runtime"
Previous message: Hangbin Liu: "Re: [PATCHv4 net 1/3] bonding: move IPsec deletion to bond_ipsec_free_sa"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
> Co-developed-by: Slawomir Rosek <srosek@xxxxxxxxxx>
> Signed-off-by: Slawomir Rosek <srosek@xxxxxxxxxx>
> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> ---
> 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

Next message: tip-bot2 for Shrikanth Hegde: " [tip: sched/urgent] sched/deadline: Use online cpus for validating runtime"
Previous message: Hangbin Liu: "Re: [PATCHv4 net 1/3] bonding: move IPsec deletion to bond_ipsec_free_sa"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]