Re: [PATCH v2 2/4] media: qcom: camss: vfe-4-1: program the RDI write master buffer size

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

On 09/11/2026 11:26, Allan Siriani wrote:
>   .../media/platform/qcom/camss/camss-vfe-4-1.c | 34 ++++++++++++++++---
>   .../media/platform/qcom/camss/camss-vfe-4-7.c |  3 +-
>   .../media/platform/qcom/camss/camss-vfe-4-8.c |  3 +-
>   .../platform/qcom/camss/camss-vfe-gen1.c      |  5 +--
>   .../platform/qcom/camss/camss-vfe-gen1.h      |  3 +-
>   5 files changed, 39 insertions(+), 9 deletions(-)
Requires a Fixes: tag

> diff --git a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
> index 9cf1ccdb2..53ebc12c4 100644
> --- a/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe-4-1.c
> @@ -107,6 +107,7 @@
>   #define VFE_0_BUS_IMAGE_MASTER_n_WR_UB_CFG_OFFSET_SHIFT	16
>   #define VFE_0_BUS_IMAGE_MASTER_n_WR_IMAGE_SIZE(n)	(0x080 + 0x24 * (n))
>   #define VFE_0_BUS_IMAGE_MASTER_n_WR_BUFFER_CFG(n)	(0x084 + 0x24 * (n))
> +#define VFE_0_BUS_IMAGE_MASTER_n_WR_BUFFER_CFG_BURST_LEN	0x3
>   #define VFE_0_BUS_IMAGE_MASTER_n_WR_FRAMEDROP_PATTERN(n)	\
>   							(0x088 + 0x24 * (n))
>   #define VFE_0_BUS_IMAGE_MASTER_n_WR_IRQ_SUBSAMPLE_PATTERN(n)	\
> @@ -269,14 +270,39 @@ static void vfe_wm_enable(struct vfe_device *vfe, u8 wm, u8 enable)
>   			    1 << VFE_0_BUS_IMAGE_MASTER_n_WR_CFG_WR_PATH_SHIFT);
>   }
>   
> -static void vfe_wm_frame_based(struct vfe_device *vfe, u8 wm, u8 enable)
> +/*
> + * In frame based mode (RDI) the write master stores the incoming data as is,
> + * with no line structure. Program its buffer size in WR_BUFFER_CFG (words per
> + * line, lines, burst length) as the downstream msm_vfe40_axi_cfg_wm_reg()
> + * does, and clear WR_IMAGE_SIZE, which may hold a stale line based setup,
> + * so that the write master is bounded by the size of the V4L2 buffer.
> + */
> +static void vfe_wm_frame_based(struct vfe_device *vfe, u8 wm,
> +			       struct v4l2_pix_format_mplane *pix, u8 enable)
>   {
> -	if (enable)
> +	u32 reg;
> +
> +	if (enable) {
> +		/*
> +		 * bytesperline is aligned to 8 bytes (one bus word) on the RDI
> +		 * outputs, and sizeimage >= bytesperline * height.
> +		 */

and sizeimage >= bytesperline * height ?

Which sizeimage => pix_mp->plane_fmt[i].sizeimage ?

This feels like a slop-comment which is no bueno - please read this 
stuff before posting and make sure it actually makes sense...

> +		reg = VFE_0_BUS_IMAGE_MASTER_n_WR_BUFFER_CFG_BURST_LEN;
> +		reg |= (min(pix->height, 4096U) - 1) << 4;

I don't agree with this clamp lets just have a consistent logic albeit 
flawed with vfe_wm_line_based()

=>

(pix->height - 1) << 4

Silently truncating here is incorrect. If we need additional validation 
it should happen in try_fmt(). For this patch just stuff into the 
bit-fields the value given.

---
bod