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

Hans Verkuil
2025-11-10T08:55:56Z

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