Re: [PATCH v2 05/18] arm64: dts: ti: k3-am62p-j722s: Add multiple channels for CSI2RX DMA

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Incomplete multistream implementation for J722S SoC: missing DMA channels for CSI2RX instances 1, 2, and 3.

Pre-existing issues:
- [High] The driver j721e-csi2rx.c leaks DMA channels on probe error, remove, and runtime_resume.
--

--- Patch [5]: [PATCH v2 05/18] arm64: dts: ti: k3-am62p-j722s: Add multiple channels for CSI2RX DMA ---
commit 57c14c001a8a9799dd013f93cc44d67678cdb58d
Author: Vaishnav Achath <vaishnav.a@ti.com>

arm64: dts: ti: k3-am62p-j722s: Add multiple channels for CSI2RX DMA
    
J721E CSI2RX SHIM layer can support up to 32 DMA channel contexts. This patch
adds additional DMA channels to enable multistream support for CSI2RX.

> diff --git a/arch/arm64/boot/dts/ti/k3-am62p-j722s-common-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-j722s-common-main.dtsi
> index f130c7cb998d7..ecb537b96fcaa 100644
> --- a/arch/arm64/boot/dts/ti/k3-am62p-j722s-common-main.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am62p-j722s-common-main.dtsi
> @@ -1053,8 +1053,10 @@ ti_csi2rx0: ticsi2rx@30102000 {
>  		ranges;
>  		#address-cells = <2>;
>  		#size-cells = <2>;
> -		dmas = <&main_bcdma_csi 0 0x5000 0>;
> -		dma-names = "rx0";
> +		dmas = <&main_bcdma_csi 0 0x5000 0>, <&main_bcdma_csi 0 0x5001 0>,
> +		       <&main_bcdma_csi 0 0x5002 0>, <&main_bcdma_csi 0 0x5003 0>,
> +		       <&main_bcdma_csi 0 0x5004 0>, <&main_bcdma_csi 0 0x5005 0>;
> +		dma-names = "rx0", "rx1", "rx2", "rx3", "rx4", "rx5";

[Severity: High]
This isn't a bug introduced by this patch, but increasing the number of DMA
channels here heavily amplifies a channel leak in the j721e-csi2rx.c driver.

When probe fails after channels are requested, ti_csi2rx_cleanup_ctx() checks
the PM runtime status:

drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c:ti_csi2rx_cleanup_ctx() {
        if (!pm_runtime_status_suspended(ctx->csi->dev))
                dma_release_channel(ctx->dma.chan);
}

Since the PM status is default suspended during a probe failure, the channels
are never released.

Similarly, during driver removal, the driver calls pm_runtime_set_suspended()
before cleaning up contexts, causing the same leak for active devices:

drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c:ti_csi2rx_remove() {
        if (!pm_runtime_status_suspended(&pdev->dev))
                pm_runtime_set_suspended(&pdev->dev);

        for (unsigned int i = 0; i < csi->num_ctx; i++)
                ti_csi2rx_cleanup_ctx(&csi->ctx[i]);
}

Finally, by increasing the context count to 6 here, this activates a mid-loop
failure path in ti_csi2rx_runtime_resume() that was impossible with 1 channel:

drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c:ti_csi2rx_runtime_resume() {
        for (unsigned int i = 0; i < csi->num_ctx; i++) {
                ret = ti_csi2rx_init_dma(&csi->ctx[i]);
                if (ret)
                        return ret;
        }
}

If ti_csi2rx_init_dma() fails at i=1, does this return without releasing the
channel successfully requested at i=0? Would it be possible to address these
leaks in the driver prior to or alongside expanding the channel counts in
the DT?

[Severity: Medium]
Are there missing multistream DMA channel definitions for the other CSI2RX
instances?

The J722S SoC has four identical instances (ti_csi2rx0 through ti_csi2rx3).
While this updates ti_csi2rx0 in k3-am62p-j722s-common-main.dtsi, the other
instances in k3-j722s-main.dtsi appear to still only have one DMA context:

arch/arm64/boot/dts/ti/k3-j722s-main.dtsi:ti_csi2rx1() {
        ...
        dmas = <&main_bcdma_csi 0 0x5100 0>;
        dma-names = "rx0";
        ...
}

If a user connects a multistream camera to CSI ports 1, 2, or 3, will they
be prevented from using multistream capture because rx1 through rx5 are not
defined? Should instances 1 through 3 be updated to match ti_csi2rx0?

>  		power-domains = <&k3_pds 182 TI_SCI_PD_EXCLUSIVE>;
>  		status = "disabled";

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713113423.2310443-1-y-abhilashchandra@ti.com?part=5