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

[Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>]

On 9/29/2026 2:17 PM, sashiko-bot@kernel.org wrote:
> 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?
> 
No. The hardware accept this operation and no issue is found in tests.

>> 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?
> 

It's acceptable to align with the commits. Actually this is a CAMSS 
framework restriction which needs additional efforts. Will keep it the 
same for TFE Full and TFE Lite in next revision.

Best Regards,
Hangxiang


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy