Re: [PATCH 08/18] media: platform: microchip: Add new histogram submodule

[Hans Verkuil <hverkuil+cisco@kernel.org>]

On 09/10/2025 17:52, Balamanikandan Gunasundar wrote:
> From: Balakrishnan Sambath <balakrishnan.s@microchip.com>
> 
> Add new histogram submodule driver to export raw histogram statistics
> and data to userspace.
> 
> Signed-off-by: Balakrishnan Sambath <balakrishnan.s@microchip.com>
> ---
>  drivers/media/platform/microchip/Kconfig      |   2 +
>  drivers/media/platform/microchip/Makefile     |   2 +-
>  .../platform/microchip/microchip-isc-stats.c  | 549 ++++++++++++++++++
>  .../media/platform/microchip/microchip-isc.h  |  24 +
>  4 files changed, 576 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/media/platform/microchip/microchip-isc-stats.c
> 
> diff --git a/drivers/media/platform/microchip/Kconfig b/drivers/media/platform/microchip/Kconfig
> index 4734ecced029..2864a57e2ff4 100644
> --- a/drivers/media/platform/microchip/Kconfig
> +++ b/drivers/media/platform/microchip/Kconfig
> @@ -10,6 +10,7 @@ config VIDEO_MICROCHIP_ISC
>  	select MEDIA_CONTROLLER
>  	select VIDEO_V4L2_SUBDEV_API
>  	select VIDEOBUF2_DMA_CONTIG
> +	select VIDEOBUF2_VMALLOC
>  	select REGMAP_MMIO
>  	select V4L2_FWNODE
>  	select VIDEO_MICROCHIP_ISC_BASE
> @@ -26,6 +27,7 @@ config VIDEO_MICROCHIP_XISC
>  	depends on VIDEO_DEV && COMMON_CLK
>  	depends on ARCH_AT91 || COMPILE_TEST
>  	select VIDEOBUF2_DMA_CONTIG
> +	select VIDEOBUF2_VMALLOC
>  	select REGMAP_MMIO
>  	select V4L2_FWNODE
>  	select VIDEO_MICROCHIP_ISC_BASE
> diff --git a/drivers/media/platform/microchip/Makefile b/drivers/media/platform/microchip/Makefile
> index bd8d6e779c51..94c64d3d242c 100644
> --- a/drivers/media/platform/microchip/Makefile
> +++ b/drivers/media/platform/microchip/Makefile
> @@ -1,7 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  microchip-isc-objs = microchip-sama5d2-isc.o
>  microchip-xisc-objs = microchip-sama7g5-isc.o
> -microchip-isc-common-objs = microchip-isc-base.o microchip-isc-clk.o microchip-isc-scaler.o
> +microchip-isc-common-objs = microchip-isc-base.o microchip-isc-clk.o microchip-isc-scaler.o microchip-isc-stats.o
>  
>  obj-$(CONFIG_VIDEO_MICROCHIP_ISC_BASE) += microchip-isc-common.o
>  obj-$(CONFIG_VIDEO_MICROCHIP_ISC) += microchip-isc.o
> diff --git a/drivers/media/platform/microchip/microchip-isc-stats.c b/drivers/media/platform/microchip/microchip-isc-stats.c
> new file mode 100644
> index 000000000000..d7813c9d95ac
> --- /dev/null
> +++ b/drivers/media/platform/microchip/microchip-isc-stats.c
> @@ -0,0 +1,549 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Microchip ISC Driver - Statistics Subdevice
> + * Raw Histogram Export for Userspace Applications
> + *
> + * Copyright (C) 2025 Microchip Technology Inc.
> + *
> + * Author: Balakrishnan Sambath <balakrishnan.s@microchip.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/videobuf2-core.h>
> +#include <media/videobuf2-vmalloc.h>
> +#include "microchip-isc-regs.h"
> +#include "microchip-isc.h"
> +
> +#define ISC_STATS_DEV_NAME	"microchip-isc_stats"
> +#define ISC_STATS_MIN_BUFS	2
> +#define ISC_STATS_MAX_BUFS	8
> +
> +/**
> + * struct isc_stat_buffer - Raw histogram statistics buffer structure
> + * @frame_number: Sequential frame number from capture
> + * @timestamp: Frame capture timestamp in nanoseconds
> + * @meas_type: Bitmask of measurement types available (ISC_CIF_ISP_STAT_*)
> + * @hist: Array of histogram data for each Bayer channel
> + * @hist.hist_bins: Raw 512-bin histogram data from hardware
> + * @hist.hist_min: Minimum pixel value observed in channel
> + * @hist.hist_max: Maximum pixel value observed in channel
> + * @hist.total_pixels: Total number of pixels processed in channel
> + * @valid_channels: Bitmask indicating which Bayer channels contain valid data
> + * @bayer_pattern: Current Bayer pattern configuration (CFA_BAYCFG_*)
> + * @reserved: Padding for future expansion and alignment
> + *
> + * This structure contains raw, unprocessed histogram data from the ISC
> + * hardware for all four Bayer channels (GR, R, GB, B). No algorithmic
> + * processing is performed - data is exported directly from hardware
> + * registers for userspace processing applications.
> + */
> +struct isc_stat_buffer {
> +	u32 frame_number;
> +	u64 timestamp;

Swap the two fields above to avoid introducing holes due to alignment problems.

> +	u32 meas_type;
> +
> +	struct {
> +		u32 hist_bins[HIST_ENTRIES];
> +		u32 hist_min;
> +		u32 hist_max;
> +		u32 total_pixels;
> +	} hist[HIST_BAYER];
> +
> +	u8 valid_channels;
> +	u8 bayer_pattern;
> +	u16 reserved[2];
> +} __packed;

After swapping those two fields you probably can drop __packed, but I'm not certain.
It's probably a good idea to check with the pahole utility if there are no holes in
this structure.

This structure must be part of include/uapi/linux/, probably
include/uapi/linux/media/microchip/something.h

Userspace must have access to this, otherwise it can't parse the metadata. Note
that that also means that the HIST_ENTRIES and HIST_BAYER/ISC_HIS_CFG_MODE_B
defines are in that uapi include as well.

> +
> +/* Statistics measurement type flags */
> +#define ISC_CIF_ISP_STAT_HIST		BIT(0)
> +

Regards,

	Hans