Re: [PATCH 66/74] media: qcom: camss: vfe-780: Produce the AEC Bayer histogram statistics

["Gjorgji Rosikopulos (Consultant)" <gjorgji.rosikopulos@oss.qualcomm.com>]

Hi Bryan,

On 10/5/2026 8:14 PM, bod@kernel.org wrote:
> From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> 
> Stream the AEC Bayer histogram on the pixel line's statistics node, as
> the V4L2_META_FMT_QCOM_ISP_STATS block CAMSS_STATS_AEC_BHIST.
> 
> The AEC BHIST write master is frame based, PLAIN64, in completion group
> 4. Write master addresses are 256 byte aligned and the v4l2-isp block
> payloads are not, so the write master fills a kernel buffer and the
> completion of group 4 serialises it into the next queued statistics
> buffer with v4l2_isp_stats_init_buffer() and
> v4l2_isp_stats_init_block(). The copy runs a frame before the write
> master writes the kernel buffer again.
> 
> As with image buffers, each frame consumes one programmed write master
> address, so the write master is handed the kernel buffer once per queued
> statistics buffer. Without a queued buffer it has no address and the
> frame's statistics are dropped; the sequence numbers show the gap.
> 
> The statistics output runs when msm_vfeN_stats is streaming as the
> image node starts. The node may stop and start again while the line
> runs: vfe_flush_buffers() now forgets the buffers it returns.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/platform/qcom/camss/camss-vfe-780.c | 64 +++++++++++++++++++++
>  drivers/media/platform/qcom/camss/camss-vfe.c     | 68 +++++++++++++++++++++++
>  drivers/media/platform/qcom/camss/camss-vfe.h     |  9 +++
>  3 files changed, 141 insertions(+)
> 
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-780.c b/drivers/media/platform/qcom/camss/camss-vfe-780.c
> index 1d00a3656d7c3..7642d9d4500de 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-780.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-780.c
> @@ -114,8 +114,49 @@ static void vfe_wm_start_pix(struct vfe_device *vfe, struct vfe_line *line)
>  	}
>  }
>  
> +static struct vfe_output *vfe_stats_output(struct vfe_line *line, u8 wm)
> +{
> +	unsigned int o;
> +
> +	if (!line->is_pix)
> +		return NULL;
> +
> +	for (o = 1; o < line->num_outputs; o++)
> +		if (line->output[o].pad == MSM_VFE_PAD_SRC_STATS &&
> +		    line->output[o].wm_num &&
> +		    line->output[o].wm[0].bus_client == wm)
> +			return &line->output[o];
> +
> +	return NULL;
> +}
> +
> +/* Statistics write masters are frame based and fill the output's kernel buffer */
> +static void vfe_wm_start_stats(struct vfe_device *vfe, struct vfe_output *output)
> +{
> +	u8 wm = output->wm[0].bus_client;
> +
> +	writel(0, vfe->base + VFE_BUS_WM_IMAGE_CFG_0(wm));
> +	writel(0, vfe->base + VFE_BUS_WM_IMAGE_CFG_1(wm));
> +	writel(1, vfe->base + VFE_BUS_WM_IMAGE_CFG_2(wm));
> +	writel(VFE_BUS_WM_PACKER_FMT_V3_PLAIN_64,
> +	       vfe->base + VFE_BUS_WM_PACKER_CFG(wm));
> +
> +	/* no dropped frames, one irq per frame */
> +	writel(0, vfe->base + VFE_BUS_WM_FRAMEDROP_PERIOD(wm));
> +	writel(1, vfe->base + VFE_BUS_WM_FRAMEDROP_PATTERN(wm));
> +	writel(0, vfe->base + VFE_BUS_WM_IRQ_SUBSAMPLE_PERIOD(wm));
> +	writel(1, vfe->base + VFE_BUS_WM_IRQ_SUBSAMPLE_PATTERN(wm));
> +
> +	writel(1, vfe->base + VFE_BUS_WM_MMU_PREFETCH_CFG(wm));
> +	writel(0xFFFFFFFF, vfe->base + VFE_BUS_WM_MMU_PREFETCH_MAX_OFFSET(wm));
> +
> +	writel(WM_CFG_EN | WM_CFG_MODE, vfe->base + VFE_BUS_WM_CFG(wm));
> +}
> +
>  static void vfe_wm_start(struct vfe_device *vfe, u8 wm, struct vfe_line *line)
>  {
> +	struct vfe_output *stats = vfe_stats_output(line, wm);
> +
>  	struct v4l2_pix_format_mplane *pix =
>  		&line->output[0].video_out.active_fmt.fmt.pix_mp;
>  
> @@ -126,6 +167,11 @@ static void vfe_wm_start(struct vfe_device *vfe, u8 wm, struct vfe_line *line)
>  
>  	writel(0x0, vfe->base + VFE_BUS_WM_TEST_BUS_CTRL);
>  
> +	if (stats) {
> +		vfe_wm_start_stats(vfe, stats);
> +		return;
> +	}
> +
>  	if (line->is_pix) {
>  		vfe_wm_start_pix(vfe, line);
>  		return;
> @@ -157,6 +203,11 @@ static void vfe_wm_stop(struct vfe_device *vfe, u8 wm, struct vfe_line *line)
>  	struct vfe_output *output = &line->output[0];
>  	unsigned int i;
>  
> +	if (vfe_stats_output(line, wm)) {
> +		writel(0, vfe->base + VFE_BUS_WM_CFG(wm));
> +		return;
> +	}
> +
>  	for (i = 0; i < output->wm_num; i++)
>  		writel(0, vfe->base + VFE_BUS_WM_CFG(output->wm[i].bus_client));
>  }
> @@ -165,8 +216,19 @@ static void vfe_wm_update(struct vfe_device *vfe, u8 wm, struct camss_buffer *bu
>  			  struct vfe_line *line)
>  {
>  	struct vfe_output *output = &line->output[0];
> +	struct vfe_output *stats;
>  	unsigned int i;
>  
> +	/*
> +	 * Each frame consumes one programmed address, as for images: hand the
> +	 * write master the output's kernel buffer once per queued buffer.
> +	 */
> +	stats = vfe_stats_output(line, wm);
> +	if (stats) {
> +		writel(stats->dma_addr >> 8, vfe->base + VFE_BUS_WM_IMAGE_ADDR(wm));
> +		return;
> +	}
> +
>  	/* camss-video sets addr[1] = addr[0] + bytesperline * height for NV12 */
>  	for (i = 0; i < output->wm_num; i++) {
>  		u32 addr = buffer->addr[output->wm[i].plane];
> @@ -237,6 +299,8 @@ static void vfe_subdev_init(struct device *dev, struct vfe_device *vfe)
>  	vfe_add_ds(&pix->output[1], MSM_VFE_PAD_SRC_DS4, VFE_WM_VIDEO_DS_4, VFE_V3_COMP_GRP_0);
>  	vfe_add_ds(&pix->output[2], MSM_VFE_PAD_SRC_DS16, VFE_WM_VIDEO_DS_16, VFE_V3_COMP_GRP_0);
>  	vfe_add_stats(&pix->output[3], VFE_WM_STATS_AEC_BHIST, VFE_V3_COMP_GRP_4);
> +	if (vfe_alloc_stats(vfe, &pix->output[3]))
> +		pix->output[3].wm_num = 0;	/* no statistics without their buffer */
>  	vfe_add_params(&pix->output[4], VFE_V3_COMP_GRP_0);
>  
>  	/* PIX mode */
> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> index b90b538c1aaaf..1522f1046d8dd 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> @@ -623,6 +623,63 @@ u32 vfe_hw_version(struct vfe_device *vfe)
>   * @vfe: VFE Device
>   * @wm: Write master id
>   */
> +static const struct v4l2_isp_stats_block_type_info vfe_stats_block_info[] = {
> +	[CAMSS_STATS_AEC_BHIST] = { .size = sizeof(struct camss_stats_bhist) },
> +};
> +
> +/*
> + * vfe_alloc_stats - Allocate the write master target of a statistics output
> + * @vfe: VFE device
> + * @output: statistics output
> + *
> + * Write master addresses are 256 byte aligned and the v4l2-isp block payloads
> + * are not, so statistics are written to a kernel buffer and copied into the
> + * userspace buffer when their completion group fires.
> + *
> + * Return 0 on success or -ENOMEM
> + */
> +int vfe_alloc_stats(struct vfe_device *vfe, struct vfe_output *output)
> +{
> +	output->dma_size = sizeof_field(struct camss_stats_bhist, bins);
> +	output->dma_vaddr = dmam_alloc_coherent(vfe->camss->dev,
> +						output->dma_size,
> +						&output->dma_addr, GFP_KERNEL);
> +
> +	return output->dma_vaddr ? 0 : -ENOMEM;
> +}
> +
> +/*
> + * vfe_stats_fill - Serialise a frame's statistics into a userspace buffer
> + * @vfe: VFE device
> + * @output: statistics output
> + * @buf: buffer to fill
> + *
> + * Runs from the completion of the statistics group, a frame before the write
> + * master writes the kernel buffer again.
> + */
> +static void vfe_stats_fill(struct vfe_device *vfe, struct vfe_output *output,
> +			   struct camss_buffer *buf)
> +{
> +	struct v4l2_isp_buffer *stats = vb2_plane_vaddr(&buf->vb.vb2_buf, 0);
> +	struct v4l2_isp_block_header *block;
> +	struct camss_stats_bhist *bhist;
> +
> +	v4l2_isp_stats_init_buffer(stats, V4L2_ISP_VERSION_V1);
> +
> +	block = v4l2_isp_stats_init_block(vfe->camss->dev, stats,
> +					  vfe_stats_block_info,
> +					  ARRAY_SIZE(vfe_stats_block_info),
> +					  CAMSS_STATS_AEC_BHIST,
> +					  CAMSS_STATS_MAX_PAYLOAD);
> +	if (!IS_ERR(block)) {
> +		bhist = container_of(block, struct camss_stats_bhist, header);
> +		memcpy(bhist->bins, output->dma_vaddr, sizeof(bhist->bins));
> +	}

According to my understanding, the v4l2-isp documentation explains that parameters
are copied to prevent userspace from updating and potentially corrupting the engine
configuration, which could pose a security risk
(though I don't think this concern applies to lookup tables like gamma/lsc).
That's a separate discussion anyway.

What I don't understand is why statistics are being copied. They should work the same way as
image buffers—from the engine's perspective, they're just buffers containing data, and from
userspace's perspective, they should be identical.
I may have missed an earlier discussion about this, but copying one or more statistics on
every frame (and there will be more in the future) doesn't seem right.

~Gjorgji