Re: [PATCH v17 07/11] media: qcom: camss: vfe: Add support for VFE Gen4

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] camss: hardware constraint violation due to unconditional AUP_UPDATE loop in CSID
- [High] camss: subsystem API violation where IFE Lite's 4th RDI line is incorrectly configured as PIX
--

commit def99c6cf972d2305c4c27701233f42e411f7857
Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>

media: qcom: camss: vfe: Add support for VFE Gen4

This commit adds support for the VFE Gen4 (TFE) found on the Kaanapali SoC.
It configures the hardware blocks and clock requirements, while deferring
REG_UPDATE and AUP_UPDATE until after CSID configuration completes.

> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c

[ ... ]

> @@ -552,7 +554,9 @@ int vfe_enable_output_v2(struct vfe_line *line)
>  		output->gen2.active_num++;
>  		ops->vfe_wm_update(vfe, output->wm_idx[0],
>  				   output->buf[i]->addr[0], line);
> -		ops->reg_update(vfe, line->id);
> +
> +		if (!vfe->res->reg_update_after_csid_config)
> +			ops->reg_update(vfe, line->id);
>  	}
>  
>  	spin_unlock_irqrestore(&vfe->output_lock, flags);

[Severity: High]
The commit message states: "the number of AUP_UPDATE requests must match the
number of buffers queued to the write master while it is being enabled."

In vfe_enable_output_v2(), the VFE loop shown above queues pending buffers
dynamically up to CAMSS_INIT_BUF_COUNT, but breaks early if userspace queued
fewer buffers. By setting reg_update_after_csid_config to true for Kaanapali,
this defers the update to CSID.

However, the corresponding CSID code at csid_configure_stream() in
camss-csid-gen4.c blindly issues AUP_UPDATEs matching the maximum buffer
count:

for (k = 0; k < CAMSS_INIT_BUF_COUNT; k++)
    __csid_aup_update(csid, i);

If userspace queues fewer buffers before starting the stream, does this cause
a mismatch between the generated AUP_UPDATEs and the actually queued buffers,
violating the hardware constraint?

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c

[ ... ]

> @@ -158,6 +158,157 @@ static const struct camss_subdev_resources csid_res_kaanapali[] = {

[ ... ]

> +	/* VFE3 - IFE Lite */
> +	{
> +		.clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb",
> +			   "vfe_lite_ahb", "vfe_lite",
> +			   "cpas_vfe_lite", "camnoc_rt_axi",
> +			   "camnoc_nrt_axi", "qdss_debug_xo" },
> +		.clock_rate = { { 0 },
> +				{ 0 },
> +				{ 0 },
> +				{ 0 },
> +				{ 266666667, 400000000, 480000000 },
> +				{ 0 },
> +				{ 200000000, 300000000, 400000000, 480000000 },
> +				{ 0 },
> +				{ 0 } },
> +		.reg = { "vfe_lite0" },
> +		.interrupt = { "vfe_lite0" },
> +		.vfe = {
> +			.line_num = 4,
> +			.is_lite = true,

[Severity: High]
The commit message explicitly states: "Support is currently limited to three
output lines, matching the constraints of the CAMSS framework."

But line_num is initialized to 4 for the IFE Lite blocks.

Since the CAMSS framework hardcodes VFE_LINE_PIX to index 3, does configuring
4 lines cause the 4th line (RDI3 with id 3) to be incorrectly subjected to
PIX-specific logic? For example, in vfe_try_format(), this would force a raw
RDI dump stream to use PIX format tables and validation:

if (line->id == VFE_LINE_PIX) {
    struct v4l2_rect *rect;
    rect = __vfe_get_crop(line, sd_state, which);
    fmt->width = rect->width;
    fmt->height = rect->height;
}

Is this initialization to 4 lines unintentional given the 3-line limit
mentioned in the commit?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-kaanapali-camss-v17-0-dcf3fd37f76c@oss.qualcomm.com?part=7