Re: [PATCH 9/9] arm64: dts: qcom: sm8750: add the camera subsystem

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] media: camss: csid_set_stream() skips configure_stream() on stop and restart
--

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

arm64: dts: qcom: sm8750: add the camera subsystem

This commit adds the camera subsystem (camss) device tree node for the
Qualcomm SM8750 SoC, describing the CSID, CSIPHY, and VFE instances along
with their registers, clocks, interrupts, interconnects, and power domains.

> diff --git a/arch/arm64/boot/dts/qcom/sm8750.dtsi b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> index dd738d13df8e0..12dcf2f33854a 100644
> --- a/arch/arm64/boot/dts/qcom/sm8750.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8750.dtsi
> @@ -3495,6 +3495,185 @@ dispcc: clock-controller@af00000 {
>  			#power-domain-cells = <1>;
>  		};
>  
> +		camss: isp@ad27000 {
> +			compatible = "qcom,sm8750-camss";

[Severity: High]
Can stopping a stream or unwinding a failed stream start on SM8750 leave the
CSID RDI stream enabled with interrupts unmasked, and skip stream
configuration on a subsequent start?

In drivers/media/platform/qcom/camss/camss-csid.c, csid_set_stream() gates
csid->res->hw_ops->configure_stream() on csid->phy.need_vc_update and clears
that flag on the first call:

drivers/media/platform/qcom/camss/camss-csid.c:csid_set_stream() {
	...
	if (csid->phy.need_vc_update) {
		csid->res->hw_ops->configure_stream(csid, enable);
		csid->phy.need_vc_update = false;
	}

	return 0;
}

Because csid->phy.need_vc_update is only set in csid_set_power() (on power-up)
and csid_link_setup(), it is already false when streaming stops via
video_stop_streaming() -> video_stop_subdevices() or when
video_start_streaming() unwinds already-started subdevices on error:

drivers/media/platform/qcom/camss/camss-video.c:video_start_streaming() {
	...
error:
	video_stop_subdevices(video, entity);
	video_device_pipeline_stop(vdev);
	...
}

When v4l2_subdev_call(subdev, video, s_stream, 0) invokes
csid_set_stream(sd, 0), csid_configure_stream(csid, 0) in
drivers/media/platform/qcom/camss/camss-csid-980.c is skipped:

drivers/media/platform/qcom/camss/camss-csid-980.c:csid_configure_stream() {
	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(INFO_CAMIF_SOF, csid->base + CSID_CSI2_RDIN_IRQ_CLEAR(i));
			mask = enable ? mask | INFO_CAMIF_SOF : mask & ~INFO_CAMIF_SOF;
			writel(mask, csid->base + CSID_CSI2_RDIN_IRQ_MASK(i));
			mask = readl(csid->base + CSID_TOP_IRQ_MASK);
			mask = enable ? mask | CSID_RDI_TOP_IRQ(i) : mask & ~CSID_RDI_TOP_IRQ(i);
			writel(mask, csid->base + CSID_TOP_IRQ_MASK);
			writel(IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);
			__csid_configure_rdi_stream(csid, enable, i, 0);
			__csid_configure_rx_vc(csid, 0);

			for (k = 0; enable && k < CAMSS_INIT_BUF_COUNT; k++) {
				__csid_aup_update(csid, i);
				__csid_rup_update(csid, i);
			}

			__csid_ctrl_rdi(csid, enable, i);
		}
	}
}

As a result, INFO_CAMIF_SOF in CSID_CSI2_RDIN_IRQ_MASK(i) and
CSID_RDI_TOP_IRQ(i) in CSID_TOP_IRQ_MASK are not masked, and
__csid_ctrl_rdi(csid, 0, i) is never called to stop the RDI stream.

In addition, if streaming is restarted without an intervening CSID power
cycle or link change (for example, if video_start_streaming() fails in CSIPHY
or sensor s_stream(1) after CSID s_stream(1) has already run when
VIDIOC_STREAMON was called before min_queued_buffers were queued and a later
VIDIOC_QBUF triggered vb2_start_streaming(), or while another active VFE line
keeps the CSID entity powered), csid->phy.need_vc_update remains false.

On that subsequent start attempt, vfe_enable_output_v2() in
drivers/media/platform/qcom/camss/camss-vfe.c skips ops->reg_update() because
vfe->res->reg_update_after_csid_config is true on SM8750:

drivers/media/platform/qcom/camss/camss-vfe.c:vfe_enable_output_v2() {
	...
		if (!vfe->res->reg_update_after_csid_config)
			ops->reg_update(vfe, line->id);
	}
}

And because csid_set_stream(sd, 1) also skips csid_configure_stream(csid, 1),
the initial CAMSS_INIT_BUF_COUNT calls to __csid_aup_update() and
__csid_rup_update() are never issued, leaving the initial buffer addresses
unlatched and stalling capture.

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