Re: [PATCH v7 10/23] media: qcom: camss: csid: Add support for CSID 980

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] media: qcom: camss: wrong dt_id calculation in
  __csid_configure_rdi_stream()
- [Medium] media: qcom: camss: misplaced lane_cnt fallback in
  camss-csid-980.c
- [Medium] media: qcom: camss: race on csid->reg_update and addr_update
  in csid-980
--

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

media: qcom: camss: csid: Add support for CSID 980

This commit adds the CSID 980 hardware operations driver and populates
the SM8750 CSID subdevice resource definitions.

> diff --git a/drivers/media/platform/qcom/camss/camss-csid-980.c b/drivers/media/platform/qcom/camss/camss-csid-980.c
> new file mode 100644
> index 0000000000000..713db25cd8c65
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-980.c
> @@ -0,0 +1,466 @@

[ ... ]

> +static void __csid_configure_rx(struct csid_device *csid,
> +				struct csid_phy_config *phy)
> +{
> +	int val;
> +
> +	val = (phy->lane_cnt - 1) << CSI2_RX_CFG0_NUM_ACTIVE_LANES;
> +	val |= phy->lane_assign << CSI2_RX_CFG0_DL0_INPUT_SEL;
> +	val |= (phy->csiphy_id + CSI2_RX_CFG0_PHY_SEL_BASE_IDX)
> +	       << CSI2_RX_CFG0_PHY_NUM_SEL;
> +	writel(val, csid->base + CSID_CSI2_RX_CFG0);
> +
> +	val = CSI2_RX_CFG1_ECC_CORRECTION_EN;
> +	writel(val, csid->base + CSID_CSI2_RX_CFG1);
> +}

[ ... ]

> +static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8 port, u8 vc)
> +{
> +	u8 lane_cnt = csid->phy.lane_cnt;
> +	u32 val;
> +
> +	/* Source pads matching RDI channels on hardware.
> +	 * E.g. Pad 1 -> RDI0, Pad 2 -> RDI1, etc.
> +	 */
> +	struct v4l2_mbus_framefmt *input_format = &csid->fmt[MSM_CSID_PAD_FIRST_SRC + port];
> +	const struct csid_format_info *format = csid_get_fmt_entry(csid->res->formats->formats,
> +								   csid->res->formats->nformats,
> +								   input_format->code);
> +
> +	if (!lane_cnt)
> +		lane_cnt = 4;

[Severity: Medium]
Was the lane_cnt fallback meant to be in __csid_configure_rx() instead of
__csid_configure_rdi_stream()?

In __csid_configure_rdi_stream(), lane_cnt is never read after being set,
which triggers a -Wunused-but-set-variable warning under W=1 (and precedes
the variable declarations below).  Meanwhile, __csid_configure_rx() uses
phy->lane_cnt - 1 directly in signed int arithmetic, so if phy->lane_cnt
is 0 (for instance, when csid_set_stream() disables a stream after
csid_set_power() without an active sink link), val becomes -1 and corrupts
all fields written to CSID_CSI2_RX_CFG0.

> +
> +	/*
> +	 * DT_ID is a two bit bitfield that is concatenated with
> +	 * the four least significant bits of the five bit VC
> +	 * bitfield to generate an internal CID value.
> +	 *
> +	 * CSID_RDI_CFG0(vc)
> +	 * DT_ID : 28:27
> +	 * VC    : 26:22
> +	 * DT    : 21:16
> +	 *
> +	 * CID   : VC 3:0 << 2 | DT_ID 1:0
> +	 */
> +	u8 dt_id = vc & 0x03;
> +	u32 rdi_cfg0_offset = CSID_RDI_CFG0(port);
> +	u32 rdi_cfg1_offset = CSID_RDI_CFG1(port);
> +	u32 rdi_ctrl_offset = CSID_RDI_CTRL(port);
> +
> +	val = RDI_CFG0_TIMESTAMP_EN;
> +	val |= RDI_CFG0_TIMESTAMP_STB_SEL;
> +	val |= RDI_CFG0_RETIME_BS;
> +
> +	/* note: for non-RDI path, this should be format->decode_format */
> +	val |= DECODE_FORMAT_PAYLOAD_ONLY << RDI_CFG0_DECODE_FORMAT;
> +	val |= vc << RDI_CFG0_VC;
> +	val |= format->data_type << RDI_CFG0_DT;
> +	val |= dt_id << RDI_CFG0_DT_ID;
> +	writel(val, csid->base + rdi_cfg0_offset);

[ ... ]

> +static void csid_configure_stream(struct csid_device *csid, u8 enable)
> +{
> +	u8 i, k;
> +
> +	__csid_configure_rx(csid, &csid->phy);
> +
> +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS_980; i++) {
> +		if (csid->phy.en_vc & BIT(i)) {
> +			__csid_configure_rdi_stream(csid, enable, i, 0);

[Severity: High]
Should dt_id in __csid_configure_rdi_stream() be derived from port & 0x03
rather than vc & 0x03?

csid_configure_stream() always passes vc = 0 while passing the RDI channel
index i as port.  Calculating dt_id from vc sets RDI_CFG0_DT_ID to 0 for
every RDI channel, so RDI1, RDI2, and RDI3 all receive internal CID 0 and
cannot stream properly, matching the issue fixed for CSID GEN3 in commit
ad136e52634d ("media: qcom: camss: Fix RDI streaming for CSID GEN3") and
for CSID GEN2 in commit 618765634cef ("media: qcom: camss: Fix RDI
streaming for CSID GEN2").

[ ... ]

> +static irqreturn_t csid_isr(int irq, void *dev)
> +{
> +	struct csid_device *csid = dev;
> +	u32 val, buf_done_val;
> +	u8 reset_done;
> +	int i;
> +
> +	val = readl(csid->base + CSID_TOP_IRQ_STATUS);
> +	writel(val, csid->base + CSID_TOP_IRQ_CLEAR);
> +
> +	reset_done = val & INFO_RST_DONE;
> +
> +	buf_done_val = readl(csid->base + CSID_BUF_DONE_IRQ_STATUS);
> +	writel(buf_done_val, csid->base + CSID_BUF_DONE_IRQ_CLEAR);
> +
> +	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS_980; i++) {
> +		if (csid->phy.en_vc & BIT(i)) {
> +			val = readl(csid->base + CSID_CSI2_RDIN_IRQ_STATUS(i));
> +			writel(val, csid->base + CSID_CSI2_RDIN_IRQ_CLEAR(i));
> +
> +			if (val & INFO_RUP_DONE) {
> +				__csid_addr_update_clear(csid, i);
> +				__csid_reg_update_clear(csid, i);
> +			}
> +
> +			if (buf_done_val & BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i))
> +				camss_buf_done(csid->camss, csid->id, i);
> +		}
> +	}

[Severity: Medium]
Can __csid_addr_update_clear() and __csid_reg_update_clear() in csid_isr()
race with concurrent updates to csid->reg_update and csid->addr_update?

In camss-vfe.c, vfe_queue_buffer_v2(), vfe_enable_output_v2(), and
vfe_buf_done() hold vfe->output_lock while calling the CSID addr_update
and reg_update callbacks, which perform non-atomic read-modify-write
updates on csid->reg_update and csid->addr_update.  However, csid_isr()
clears bits in csid->addr_update and csid->reg_update before
camss_buf_done() is called and without holding vfe->output_lock:

CPU0                              CPU1 (<hardirq>)
vfe_queue_buffer_v2()
  spin_lock_irqsave(&vfe->output_lock)
  __csid_addr_update(csid, i)
    csid->reg_update |= AUP_RDI(i)
    writel(csid->reg_update,
           CSID_LITE_RUP_AUP_CMD)
                                  csid_isr()
                                    if (val & INFO_RUP_DONE)
                                      __csid_addr_update_clear(csid, i)
                                        csid->reg_update &= ~AUP_RDI(i)
  __csid_reg_update(csid, i)
    csid->reg_update |= RUP_RDI(i)
    writel(csid->reg_update, ...) <-- overwrites CMD with AUP_RDI(i) cleared

On CSID Lite, where RUP and AUP share csid->reg_update and the
CSID_LITE_RUP_AUP_CMD register, this race can clear a newly queued AUP bit
in hardware before it is consumed, and across multiple active RDI ports it
can lose or retain stale update bits in csid->reg_update and
csid->addr_update.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-add-support-for-camss-on-sm8750-v7-0-8ff2a11835f6@oss.qualcomm.com?part=10