Re: [PATCH v21 04/14] media: qcom: camss: Issue stream-on REG_UPDATE from CSID

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

On 10/12/2026 3:01 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [High] media: qcom: camss: lost initial buffer due to skipped REG_UPDATE
> - [High] media: data race on csid->reg_update in stream-on
> - [High] media: camss: cross-routing breaks due to hardcoded VFE ID in reg_update
> --
> 
> commit cb036f0b44cb6248a2bce2553092074a74c00d9a
> Author: Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>
> Subject: media: qcom: camss: Issue stream-on REG_UPDATE from CSID
> 
> This patch moves the stream-on REG_UPDATE from the VFE to the CSID for
> CSID 680 and CSID Gen3 platforms. It skips the VFE-side REG_UPDATE when
> the upstream CSID implements the reg_update callback.
> 
>> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.c b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
>> index 0fdbf75fb27db..830c9c818d3d2 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csid-gen3.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> 
> [ ... ]
> 
>> @@ -230,6 +232,7 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)
>>   		if (csid->phy.en_vc & BIT(i)) {
>>   			__csid_configure_rdi_stream(csid, enable, i, 0);
>>   			__csid_configure_rx(csid, &csid->phy, 0);
>> +			csid_subdev_reg_update(csid, i, false);
>>   			__csid_ctrl_rdi(csid, enable, i);
>>   		}
>>   }
> 
> [Severity: High]
> Does this new process-context call expose csid->reg_update to a data race?
> 
> Looking at csid_subdev_reg_update(), it performs non-atomic
> read-modify-write operations on the shared csid->reg_update mask:
> 
>      if (clear) {
>          csid->reg_update &= ~CSID_RUP_AUP_RDI(port_id);
>      } else {
>          csid->reg_update |= CSID_RUP_AUP_RDI(port_id);
>          writel(csid->reg_update, csid->base + CSID_RUP_AUP_CMD);
>      }
> 
> Concurrently, when the hardware generates a RUP_DONE interrupt, the
> hardirq handler csid_isr() calls this same function:
> 
>      if (val & RUP_DONE_IRQ_STATUS)
>          csid_subdev_reg_update(csid, i, true);
> 
> Since csid_configure_stream() is called in process context without holding
> a spinlock or disabling local interrupts, couldn't csid_isr() preempt it
> midway through its read-modify-write? This could cause corruptions to the
> mask, leading to lost or unintended hardware register updates, which might
> cause streams to fail to start or stop.
> 

I agree but this should relate to CAMSS framework design and now it 
seems the CAMSS hasn't taken this into consideration. Future framework 
upgrade will cover that but KNP series will still focus on enablement.

>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/media/platform/qcom/camss/camss-vfe.c
>> index 2680a9914765f..16a0b5663fe2b 100644
>> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> 
> [ ... ]
> 
>> @@ -552,7 +586,8 @@ 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 (!rup_in_csid)
>> +			ops->reg_update(vfe, line->id);
>>   	}
> 
> [Severity: High]
> Will skipping the reg_update here cause the first queued buffer address to
> be permanently lost?
> 
> During stream initialization, this loop runs CAMSS_INIT_BUF_COUNT (2) times.
> The vfe_wm_update() callback writes the buffer's address to a single hardware
> shadow register (for example, VFE_BUS_IMAGE_ADDR). Previously, reg_update()
> was called after each write to latch the shadow register into the active
> hardware queue.
> 
> With this patch, if rup_in_csid is true, reg_update() is skipped inside the
> loop. This means the second iteration will overwrite the first buffer's
> address in the shadow register before it is ever latched.
> 
> When the CSID eventually issues the single REG_UPDATE at stream-on, only
> the address of buf[1] will be latched. The hardware will then write the
> first captured frame to buf[1], but the driver's completion handler
> vfe_buf_done() expects buf[0] and will return that uninitialized buffer
> to userspace, resulting in silent data corruption and shifted frames.
> 

No. We have tested on specific hardware like 680 and 1080 and no issue 
is found. Actually, this is a recommended initialization sequence since 
CSID 680. However, on Kaanapali platform, the requirement for sequence 
order becomes strict and we have to follow that guidance. The reg_update 
is split into two parts since 1080, rup_update and aup_update. The 
CAMSS_INIT_BUF_COUNT should only impact the hardware starts from 1080 
version. The previous generation won't be influenced.

> [Severity: High]
> Does this logic also need to be applied to the runtime buffer completion
> path to prevent cross-routing failures?
> 
> While this patch correctly identifies the active upstream CSID for the
> stream-on phase by traversing the media graph, it appears the runtime
> path still uses the hardcoded ID.
> 
> In vfe_buf_done(), when buffers complete, the driver unconditionally invokes:
> 
>      ops->vfe_wm_update(vfe, output->wm_idx[0],
>                         output->buf[index]->addr[0], line);
>      ops->reg_update(vfe, line->id);
> 
> On Gen3 and 680 platforms, this routes through vfe_reg_update() to
> camss_reg_update() using the hardcoded vfe->id:
> 
>      camss_reg_update(vfe->camss, vfe->id, port_id, false);
> 
> If a user configures a cross-routed media graph linking CSID N to VFE M
> (where N is not equal to M), this hardcoded routing means the register
> updates for new buffers are sent to the wrong CSID. This leaves the active
> CSID's shadow registers unlatched and potentially corrupts the unrelated
> CSID's state, causing streams to freeze.
> 

This should be one-one mapping for CSID and VFE. We have moved the 
REG_UPDATE from VFE to CSID since 680 and the BUF_DONE event can be 
found though CSID id. These two evidence from hardware can prove this 
intentional one-one mapping design.

---
Best Regards,
Hangxiang