Re: [PATCH v2 3/5] media: v4l2-core: Add v4l2-stats interface

[Detlev Casanova <detlev.casanova@collabora.com>]

Hi Hans,

On 6/19/26 09:05, Hans Verkuil wrote:
> On 17/06/2026 20:10, Detlev Casanova wrote:
>> Provide helpers for media drivers to set fdinfo data and print the
>> key:value pairs in a standard way.
>>
>> User drivers can set stats values with helpers like:
>>   - v4l2_stats_update_hw_usage
>>   - v4l2_stats_set_media_dev_type
>>
>> And also call the show helpers from their show_fdinfo callback with:
>>   - v4l2_stats_show -- Shows the values set previously
>>   - v4l2_stats_show_clock -- Shows the main clock state.
>>
>> The show_clock helper is used instead of updating a clock value in
>> v4l2_stats for the following reasons:
>>   - Clocks are at the device level, this is not a per-fd information
>>   - This avoids having clock references in v4l2-core
>>   - Drivers can use different approaches to manage clocks
>>     (e.g.: bulk_data or not: A set helper wouldn't please all drivers)
>>   - Arguably, clocks could be exposed elsewhere (like a debugfs), but we
>>     want something close to what DRM does and centralizing information has
>>     its advantages for userspace tooling.
>>
>> In DRM the key:value pair format for clocks is documented and each driver
>> can write them directly based on that.
>> In this case, provide a helper and document the format.
>>
>> Signed-off-by: Detlev Casanova <detlev.casanova@collabora.com>
>> ---
>>   drivers/media/v4l2-core/Makefile     |  2 +-
>>   drivers/media/v4l2-core/v4l2-dev.c   |  2 ++
>>   drivers/media/v4l2-core/v4l2-fh.c    |  3 ++
>>   drivers/media/v4l2-core/v4l2-stats.c | 65 ++++++++++++++++++++++++++++++++++++
>>   include/media/v4l2-fh.h              |  2 ++
>>   include/media/v4l2-stats.h           | 44 ++++++++++++++++++++++++
>>   6 files changed, 117 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/v4l2-core/Makefile b/drivers/media/v4l2-core/Makefile
>> index 329f0eadce99..20e1ab74ac09 100644
>> --- a/drivers/media/v4l2-core/Makefile
>> +++ b/drivers/media/v4l2-core/Makefile
>> @@ -9,7 +9,7 @@ ccflags-y += -I$(srctree)/drivers/media/tuners
>>   tuner-objs	:=	tuner-core.o
>>   
>>   videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
>> -			v4l2-event.o v4l2-subdev.o v4l2-common.o \
>> +			v4l2-event.o v4l2-subdev.o v4l2-common.o v4l2-stats.o \
>>   			v4l2-ctrls-core.o v4l2-ctrls-api.o \
>>   			v4l2-ctrls-request.o v4l2-ctrls-defs.o
>>   
>> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>> index 3878fa2ff73e..3e7a6876dffd 100644
>> --- a/drivers/media/v4l2-core/v4l2-dev.c
>> +++ b/drivers/media/v4l2-core/v4l2-dev.c
>> @@ -486,6 +486,8 @@ static void v4l2_show_fdinfo(struct seq_file *m, struct file *filp)
>>   {
>>   	struct video_device *vdev = video_devdata(filp);
>>   
>> +	seq_printf(m, "media-driver:\t%s\n", vdev->v4l2_dev->name);
>> +
>>   	if (vdev->fops->show_fdinfo)
>>   		vdev->fops->show_fdinfo(m, filp);
>>   }
>> diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
>> index b184bed8aca9..1b655672c718 100644
>> --- a/drivers/media/v4l2-core/v4l2-fh.c
>> +++ b/drivers/media/v4l2-core/v4l2-fh.c
>> @@ -17,6 +17,7 @@
>>   #include <media/v4l2-event.h>
>>   #include <media/v4l2-ioctl.h>
>>   #include <media/v4l2-mc.h>
>> +#include <media/v4l2-stats.h>
>>   
>>   void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
>>   {
>> @@ -38,6 +39,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
>>   	INIT_LIST_HEAD(&fh->subscribed);
>>   	fh->sequence = -1;
>>   	mutex_init(&fh->subscribe_lock);
>> +	v4l2_stats_init(&fh->stats);
>>   }
>>   EXPORT_SYMBOL_GPL(v4l2_fh_init);
>>   
>> @@ -88,6 +90,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
>>   	v4l2_event_unsubscribe_all(fh);
>>   	mutex_destroy(&fh->subscribe_lock);
>>   	fh->vdev = NULL;
>> +	v4l2_stats_exit(&fh->stats);
>>   }
>>   EXPORT_SYMBOL_GPL(v4l2_fh_exit);
>>   
>> diff --git a/drivers/media/v4l2-core/v4l2-stats.c b/drivers/media/v4l2-core/v4l2-stats.c
>> new file mode 100644
>> index 000000000000..93e64ef2e7bb
>> --- /dev/null
>> +++ b/drivers/media/v4l2-core/v4l2-stats.c
>> @@ -0,0 +1,65 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * v4l2-stats.c
>> + *
>> + * V4L2 statistics management.
>> + *
>> + * Maintain a per-file handle list of statistics about the hardware and handle
>> + * exposing it in the fdinfo.
>> + *
>> + * Copyright (C) 2026 Collabora.
>> + *
>> + * Contact: Detlev Casanova <detlev.casanova@collabora.com>
>> + */
>> +
>> +#include <linux/types.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/clk.h>
>> +#include <media/v4l2-stats.h>
>> +
>> +static const char * const dev_type_name[] = {
>> +	[MEDIA_DEV_TYPE_V4L2] = "media",
>> +	[MEDIA_DEV_TYPE_V4L2_STATELESS_ENCODER] = "encoder",
>> +	[MEDIA_DEV_TYPE_V4L2_STATELESS_DECODER] = "decoder",
>> +};
>> +
>> +void v4l2_stats_init(struct v4l2_stats *stats)
>> +{
>> +	stats->hw_usage_time = 0;
>> +	stats->media_dev_type = MEDIA_DEV_TYPE_V4L2;
>> +}
>> +
>> +void v4l2_stats_exit(struct v4l2_stats *stats)
>> +{
>> +}
>> +
>> +void v4l2_stats_update_hw_usage(struct v4l2_stats *stats, u64 usage_time)
>> +{
>> +	stats->hw_usage_time += usage_time;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_stats_update_hw_usage);
>> +
>> +void v4l2_stats_set_media_dev_type(struct v4l2_stats *stats, enum v4l2_media_dev_type type)
>> +{
>> +	if (type >= MEDIA_DEV_TYPE_COUNT)
>> +		return;
>> +
>> +	stats->media_dev_type = type;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_stats_set_media_dev_type);
>> +
>> +void v4l2_stats_show(struct v4l2_stats *stats, struct seq_file *m)
>> +{
>> +	seq_printf(m, "media-type:\t%s\n", dev_type_name[stats->media_dev_type]);
>> +	seq_printf(m, "media-engine-usage:\t%llu ns\n", stats->hw_usage_time);
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_stats_show);
>> +
>> +void v4l2_stats_show_clock(struct seq_file *m, struct clk *clk)
>> +{
>> +	seq_printf(m, "media-maxfreq:\t%lu Hz\n",
>> +		   clk_get_rate(clk));
>> +	seq_printf(m, "media-curfreq:\t%lu Hz\n",
>> +		   clk_get_rate(clk));
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_stats_show_clock);
>> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
>> index aad4b3689d7e..ae6688722bee 100644
>> --- a/include/media/v4l2-fh.h
>> +++ b/include/media/v4l2-fh.h
>> @@ -17,6 +17,7 @@
>>   #include <linux/kconfig.h>
>>   #include <linux/list.h>
>>   #include <linux/videodev2.h>
>> +#include <media/v4l2-stats.h>
>>   
>>   struct video_device;
>>   struct v4l2_ctrl_handler;
>> @@ -43,6 +44,7 @@ struct v4l2_fh {
>>   	struct list_head	list;
>>   	struct video_device	*vdev;
>>   	struct v4l2_ctrl_handler *ctrl_handler;
>> +	struct v4l2_stats	stats;
>>   	enum v4l2_priority	prio;
>>   
>>   	/* Events */
>> diff --git a/include/media/v4l2-stats.h b/include/media/v4l2-stats.h
>> new file mode 100644
>> index 000000000000..d580933c4181
>> --- /dev/null
>> +++ b/include/media/v4l2-stats.h
>> @@ -0,0 +1,44 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * v4l2-stats.h
>> + *
>> + * V4L2 statistics management.
>> + *
>> + * Maintain a per-file handle list of statistics about the hardware and handle
>> + * exposing it in the fdinfo.
>> + *
>> + * Copyright (C) 2026 Collabora.
>> + *
>> + * Contact: Detlev Casanova <detlev.casanova@collabora.com>
>> + */
>> +#ifndef V4L2_STATS_H
>> +#define V4L2_STATS_H
>> +
>> +#include <linux/types.h>
>> +
>> +struct clk;
>> +struct seq_file;
>> +
>> +enum v4l2_media_dev_type {
>> +	MEDIA_DEV_TYPE_V4L2 = 0,
>> +	MEDIA_DEV_TYPE_V4L2_STATELESS_ENCODER,
>> +	MEDIA_DEV_TYPE_V4L2_STATELESS_DECODER,
>> +
>> +	MEDIA_DEV_TYPE_COUNT,
> I'm a bit unhappy about introducing yet another type. Do we need it?
I didn't want it either, but this will extend to other kind of drivers 
and I couldn't find a type enum to rule them all.
Maybe just using Device Capabilities Flags ?

It still won't differentiate encoder from decoder.
I wouldn't super mad if we loose that information (userspace can simply 
see it as a capture device) but it'd still be nice to have it.
>
>> +};
>> +
>> +struct v4l2_stats {
> Poor name, and it conflicts with ISP statistics.
>
> How about v4l2_pdinfo? And v4l2-fdinfo.h etc. That avoids any confusion and
> also clearly says what this is about.
As mentioned in the other patch, "v4l2_metrics" is more accurate.

Detlev.
>
>> +	u64 hw_usage_time;
>> +	enum v4l2_media_dev_type media_dev_type;
> This should be the first field.
>
>> +};
>> +
>> +void v4l2_stats_init(struct v4l2_stats *stats);
>> +void v4l2_stats_exit(struct v4l2_stats *stats);
>> +
>> +void v4l2_stats_update_hw_usage(struct v4l2_stats *stats, u64 usage_time);
>> +void v4l2_stats_set_media_dev_type(struct v4l2_stats *stats, enum v4l2_media_dev_type type);
>> +
>> +void v4l2_stats_show(struct v4l2_stats *stats, struct seq_file *m);
>> +void v4l2_stats_show_clock(struct seq_file *m, struct clk *clk);
>> +
>> +#endif /* V4L2_STATS_H */
>>
> Regards,
>
> 	Hans