Re: [PATCH 4/9] media: camss: csid: add the 980 generation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] media: camss: csid: INFO_RUP_DONE left masked in csid_configure_stream()
- [High] media: camss: csid: wrong dt_id in __csid_configure_rdi_stream()
- [Medium] media: camss: csid-980: race on reg_update, rup_update, and aup_update
--

commit 9e06166c718bdeac203de00c1a26a347c6114d7a
Author: Qianhui Xu <xiaqinghuan@mail.ustc.edu.cn>

media: camss: csid: add the 980 generation

This commit adds support for the CSID 980 generation found on SM8750,
including register maps for full and lite variants, split RUP and AUP
register update commands, RDI stream configuration, and buffer-done
interrupt handling.

> 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..45f3b37867040
> --- /dev/null
> +++ b/drivers/media/platform/qcom/camss/camss-csid-980.c

[ ... ]

> +static void __csid_aup_update(struct csid_device *csid, int port_id)
> +{
> +	if (csid_is_lite(csid)) {
> +		/* CSID Lites in v980 follow the legacy way of a combined RUP
> +		 * and AUP commands without an explicit SET register.
> +		 */
> +		csid->reg_update |= CSID_LITE_AUP_RDI(port_id);
> +		writel(csid->reg_update, csid->base + CSID_LITE_RUP_AUP_CMD);
> +	} else {
> +		csid->aup_update |= CSID_RUP_AUP_RDI(port_id);
> +		writel(csid->aup_update, csid->base + CSID_AUP_CMD);
> +
> +		/* CSID Fulls in v980 split AUP and RUP commands, which requires
> +		 * additional SET operation to make registers modification take
> +		 * effect.
> +		 */
> +		__csid_full_aup_rup_trigger(csid);
> +	}
> +}
> +
> +static void __csid_rup_update(struct csid_device *csid, int port_id)
> +{
> +	if (csid_is_lite(csid)) {
> +		/* CSID Lites in v980 follow the legacy way of a combined RUP
> +		 * and AUP commands without an explicit SET register.
> +		 */
> +		csid->reg_update |= CSID_LITE_RUP_RDI(port_id);
> +		writel(csid->reg_update, csid->base + CSID_LITE_RUP_AUP_CMD);
> +	} else {
> +		csid->rup_update |= CSID_RUP_AUP_RDI(port_id);
> +		writel(csid->rup_update, csid->base + CSID_RUP_CMD);
> +
> +		/* CSID Fulls in v980 split AUP and RUP commands, which requires
> +		 * additional SET operation to make registers modification take
> +		 * effect.
> +		 */
> +		__csid_full_aup_rup_trigger(csid);
> +	}
> +}
> +
> +static void __csid_aup_rup_clear(struct csid_device *csid, int port_id)
> +{
> +	/* Hardware clears the registers upon consuming the settings */
> +	if (csid_is_lite(csid)) {
> +		csid->reg_update &= ~CSID_LITE_RUP_RDI(port_id);
> +		csid->reg_update &= ~CSID_LITE_AUP_RDI(port_id);
> +	} else {
> +		csid->aup_update &= ~CSID_RUP_AUP_RDI(port_id);
> +		csid->rup_update &= ~CSID_RUP_AUP_RDI(port_id);
> +	}
> +}

[Severity: Medium]
Can concurrent updates to csid->reg_update, csid->rup_update, and
csid->aup_update race with each other?

In csid_isr(), when INFO_RUP_DONE is set, the interrupt handler calls
csid_subdev_reg_update(csid, i, true) -> __csid_aup_rup_clear() in hardirq
context without holding vfe->output_lock or any CSID lock:

    if (val & INFO_RUP_DONE)
        csid_subdev_reg_update(csid, i, true);

Meanwhile, on another CPU, vfe_queue_buffer_v2() (holding only
vfe->output_lock) or csid_configure_stream() (holding no spinlock while
csid->irq is enabled) can call __csid_aup_update() or __csid_rup_update().

Because both paths perform non-atomic read-modify-write operations on
csid->reg_update, csid->aup_update, and csid->rup_update before writing the
full mask to CSID_LITE_RUP_AUP_CMD, CSID_AUP_CMD, or CSID_RUP_CMD, could this
clobber newly set update bits or re-write stale RDI bits to the command
registers?

[ ... ]

> +static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8 port, u8 vc)
> +{
> +	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);
> +
> +	/*
> +	 * 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;

[Severity: High]
Should dt_id be computed as port & 0x03 instead of vc & 0x03 here?

In csid_configure_stream(), __csid_configure_rdi_stream(csid, enable, i, 0)
is always called with vc = 0 for every port i. Calculating dt_id from vc
programs dt_id = 0 and an identical internal CID = 0 in CSID_RDI_CFG0(port)
for all RDI ports (RDI0 through RDI3), which breaks streaming on non-zero RDI
ports just like the issue fixed for CSID gen3 in commit ad136e52634d ("media:
qcom: camss: Fix RDI streaming for CSID GEN3").

[ ... ]

> +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)) {
> +			u32 mask = readl(csid->base + CSID_CSI2_RDIN_IRQ_MASK(i));
> +
> +			writel(mask, csid->base + CSID_CSI2_RDIN_IRQ_MASK(i));

[Severity: High]
Does csid_configure_stream() leave INFO_RUP_DONE masked in
CSID_CSI2_RDIN_IRQ_MASK(i)?

Here, CSID_CSI2_RDIN_IRQ_MASK(i) is read into mask and written back unchanged
without setting INFO_RUP_DONE, and csid_reset() does not unmask INFO_RUP_DONE
in CSID_CSI2_RDIN_IRQ_MASK(i) either (even after follow-up commit
7457e598955f ("media: camss: csid: report frame starts from CSID"), which
only adds INFO_CAMIF_SOF to mask).

In csid_isr(), the handler checks for INFO_RUP_DONE to clear the software
update state:

    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_subdev_reg_update(csid, i, true);

Without INFO_RUP_DONE unmasked in CSID_CSI2_RDIN_IRQ_MASK(i), won't
csid_subdev_reg_update(csid, i, true) never be called, leaving
CSID_LITE_RUP_RDI(i) permanently set in csid->reg_update on CSID Lite (so
every subsequent per-frame __csid_aup_update() call in vfe_buf_done()
re-asserts RUP alongside AUP) and leaving stale bits set in csid->aup_update,
csid->rup_update, and csid->reg_update across streams?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261028034105.326535-1-xiaqinghuan@mail.ustc.edu.cn?part=4