[PATCH v24 09/15] media: qcom: camss: csid: Add support for CSID 1080
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Mon Nov 2 03:02:56 PST 2026
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] media: qcom: camss: unconditional AUP triggers in csid_configure_stream()
- [Medium] media: qcom: camss: out-of-bounds csid->fmt access in __csid_configure_rdi_stream()
- [Medium] media: qcom: camss: race on addr_update and reg_update in csid_isr()
- [Low] media: qcom: camss: unused variable lane_cnt in __csid_configure_rdi_stream()
- [Low] media: qcom: camss: malformed kernel-doc Return sections in camss-csid-1080.c
--
commit b46b9d6d26210a3faaa024307adfd55c2501ce00
Author: Hangxiang Ma <hangxiang.ma at oss.qualcomm.com>
media: qcom: camss: csid: Add support for CSID 1080
This commit adds hardware resource descriptions, register programming,
reset handling, and interrupt support for the CSID 1080 module found on
the Kaanapali SoC.
> diff --git a/drivers/media/platform/qcom/camss/camss-csid-1080.c b/drivers/media/platform/qcom/camss/camss-csid-1080.c
> new file mode 100644
> index 0000000000000..3d90d31201fa5
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-1080.c
> @@ -0,0 +1,402 @@
[ ... ]
> +#define MSM_CSID_MAX_SRC_STREAMS_1080 (csid_is_lite(csid) ? 4 : 5)
[ ... ]
> +static void __csid_aup_rup_trigger(struct csid_device *csid)
> +{
> + /* trigger SET in combined register */
> + writel(RUP_SET, csid->base + CSID_RUP_AUP_CMD);
> +}
> +
> +static void __csid_addr_update(struct csid_device *csid, int port_id)
> +{
> + csid->addr_update |= CSID_RUP_AUP_RDI(port_id);
> + writel(csid->addr_update, csid->base + CSID_AUP_CMD);
> +
> + __csid_aup_rup_trigger(csid);
> +}
> +
> +static void __csid_addr_update_clear(struct csid_device *csid, int port_id)
> +{
> + csid->addr_update &= ~CSID_RUP_AUP_RDI(port_id);
> +}
> +
> +static void __csid_reg_update(struct csid_device *csid, int port_id)
> +{
> + csid->reg_update |= CSID_RUP_AUP_RDI(port_id);
> + writel(csid->reg_update, csid->base + CSID_RUP_CMD);
> +
> + __csid_aup_rup_trigger(csid);
> +}
> +
> +static void __csid_reg_update_clear(struct csid_device *csid, int port_id)
> +{
> + csid->reg_update &= ~CSID_RUP_AUP_RDI(port_id);
> +}
[Severity: Medium]
As reported by Sashiko on v14 [1], can concurrent updates to
csid->addr_update and csid->reg_update race between process context and
interrupt context?
In __csid_addr_update(), __csid_addr_update_clear(), __csid_reg_update(),
and __csid_reg_update_clear(), the bitmasks in csid->addr_update and
csid->reg_update are modified with non-atomic |= and &= ~ operations, and
the full multi-port mask is written to CSID_AUP_CMD and CSID_RUP_CMD.
While vfe_queue_buffer_v2() and vfe_enable_output_v2() invoke
csid_subdev_addr_update() and csid_subdev_reg_update() under
vfe->output_lock, csid_configure_stream() calls __csid_addr_update() and
__csid_reg_update() without a lock, and csid_isr() clears those fields via
__csid_addr_update_clear() and __csid_reg_update_clear() before
camss_buf_done() acquires vfe->output_lock.
In addition, within csid_isr(), camss_buf_done() is called for port i
before higher-numbered RDI ports have had their INFO_RUP_DONE status
checked and cleared in the loop, so a buffer update triggered from
camss_buf_done() on port i can re-write CSID_AUP_CMD and CSID_RUP_CMD
with stale bits still set for those higher-numbered ports.
[ ... ]
> +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: Low]
As reported by Sashiko on v23 [2], lane_cnt is initialized and updated in
__csid_configure_rdi_stream(), but it is never read anywhere in the
function. Will this trigger an unused-but-set-variable compiler warning?
> +
> + 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;
> + writel(val, csid->base + CSID_RDI_CFG0(port));
> +
> + val = RDI_CFG1_PACKING_FORMAT_MIPI;
> + writel(val, csid->base + CSID_RDI_CFG1(port));
> +
> + /* Configure pixel store using dedicated register in 1080 */
> + if (!csid_is_lite(csid))
> + __csid_configure_rdi_pix_store(csid, port);
> +
> + val = 0;
> + writel(val, csid->base + CSID_RDI_CTRL(port));
> +
> + val = readl(csid->base + CSID_RDI_CFG0(port));
> +
> + if (enable)
> + val |= RDI_CFG0_EN;
> +
> + writel(val, csid->base + CSID_RDI_CFG0(port));
> +}
> +
> +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_1080; i++) {
> + if (csid->phy.en_vc & BIT(i)) {
> + __csid_configure_rdi_stream(csid, enable, i, 0);
> + __csid_configure_rx_vc(csid, 0);
[Severity: Medium]
As reported by Sashiko on v14 [1], can __csid_configure_rdi_stream() index
past the end of csid->fmt[] when i reaches 4?
For non-lite CSID 1080 instances, MSM_CSID_MAX_SRC_STREAMS_1080 evaluates
to 5, so csid_configure_stream() iterates i from 0 to 4 and passes i as
port to __csid_configure_rdi_stream():
struct v4l2_mbus_framefmt *input_format = &csid->fmt[MSM_CSID_PAD_FIRST_SRC + port];
However, in drivers/media/platform/qcom/camss/camss-csid.h,
MSM_CSID_PAD_FIRST_SRC is 1 and MSM_CSID_PADS_NUM is 5 (1 sink pad and 4
source pads), sizing both csid->pads[MSM_CSID_PADS_NUM] and
csid->fmt[MSM_CSID_PADS_NUM] to 5 elements (indices 0..4). When port is 4,
MSM_CSID_PAD_FIRST_SRC + port evaluates to 5, which is one element past
the end of csid->fmt[].
> +
> + for (k = 0; k < CAMSS_INIT_BUF_COUNT; k++)
> + __csid_addr_update(csid, i);
> +
> + __csid_reg_update(csid, i);
[Severity: High]
Does unconditionally calling __csid_addr_update() CAMSS_INIT_BUF_COUNT (2)
times here match the buffers actually programmed in vfe_enable_output_v2()?
During stream startup, vfe_enable_output_v2() in
drivers/media/platform/qcom/camss/camss-vfe.c only calls
ops->vfe_wm_update() for buffers actually present in the pending queue:
for (i = 0; i < CAMSS_INIT_BUF_COUNT; i++) {
output->buf[i] = vfe_buf_get_pending(output);
if (!output->buf[i])
break;
output->gen2.active_num++;
ops->vfe_wm_update(vfe, output->wm_idx[0],
output->buf[i]->addr[0], line);
if (!ops->addr_update)
ops->reg_update(vfe, line->id);
}
If userspace starts streaming with 0 or 1 buffer queued (or when
csid_configure_stream() is called with enable == 0),
csid_configure_stream() still triggers two AUPs and one RUP, which can
latch uninitialized or duplicate WM buffer addresses into the hardware
FIFO. Even when 2 buffers are queued prior to stream-on,
vfe_enable_output_v2() writes both buffer addresses to the WM register
before any AUP is issued.
> +
> + __csid_ctrl_rdi(csid, enable, i);
> + }
> + }
> +}
[ ... ]
> +/**
> + * csid_isr - CSID module interrupt service routine
> + * @irq: Interrupt line
> + * @dev: CSID device
> + *
> + * Return IRQ_HANDLED on success
> + */
> +static irqreturn_t csid_isr(int irq, void *dev)
> +{
[ ... ]
> +}
> +
> +/**
> + * csid_reset - Trigger reset on CSID module and wait to complete
> + * @csid: CSID device
> + *
> + * Return 0 on success or a negative error code otherwise
> + */
[Severity: Low]
This isn't a bug, but the kernel-doc comments for csid_isr() and
csid_reset() use "Return" instead of "Return:", which causes
scripts/kernel-doc to warn about a missing return value description under
W=1 builds.
[1] https://lore.kernel.org/r/20260601162737.8D2531F00893@smtp.kernel.org
[2] https://lore.kernel.org/r/20261028-kaanapali-camss-v23-9-95f250b18612@oss.qualcomm.com
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-kaanapali-camss-v24-0-d6b8f3f6f66f@oss.qualcomm.com?part=9
More information about the linux-phy
mailing list