Re: [PATCH v3 1/5] media: qcom: camss: Fix RDI streaming for CSID 680

[Loic Poulain]

On Tue, Apr 7, 2026 at 12:35 PM <bod@xxxxxxxxxx> wrote:
>
> From: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
>
> Fix streaming to RDI1 and RDI2. csid->phy.en_vc contains a bitmask of
> enabled CSID ports not virtual channels.
>
> We cycle through the number of available CSID ports and test this value
> against the vc_en bitmask.
>
> We then use the passed value both as an index to the port configuration
> macros and as a virtual channel index.
>
> This is a very broken pattern. Reviewing the initial introduction of VC
> support it states that you can only map one CSID to one VFE. This is true
> however each CSID has multiple sources which can sink inside of the VFE -
> for example there is a "pixel" path for bayer stats which sources @
> CSID(x):3 and sinks on VFE(x):pix.
>
> That is CSID port # 3 should drive VFE port #3. With our current setup only
> a sensor which drives virtual channel number #3 could possibly enable that
> setup.
>
> This is deeply wrong the virtual channel has no relevance to hooking CSID
> to VFE, a fact that is proven after this patch is applied allowing
> RDI0,RDI1 and RDI2 to function with VC0 whereas before only RDI1 worked.
>
> Another way the current model breaks is the DT field. A sensor driving
> different data-types on the same VC would not be able to separate the VC:DT
> pair to separate RDI outputs, thus breaking another feature of VCs in the
> MIPI data-stream.
>
> Default the VC back to zero. A follow on series will implement subdev
> streams to actually enable VCs without breaking CSID source to VFE sink.
>
> Fixes: 253314b20408 ("media: qcom: camss: Add CSID 680 support")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>

Reviewed-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>


> ---
>  drivers/media/platform/qcom/camss/camss-csid-680.c | 30 +++++++++++-----------
>  1 file changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/media/platform/qcom/camss/camss-csid-680.c b/drivers/media/platform/qcom/camss/camss-csid-680.c
> index 3ad3a174bcfb8..edf01ba79907d 100644
> --- a/drivers/media/platform/qcom/camss/camss-csid-680.c
> +++ b/drivers/media/platform/qcom/camss/camss-csid-680.c
> @@ -219,9 +219,9 @@ static void __csid_configure_top(struct csid_device *csid)
>             CSID_TOP_IO_PATH_CFG0(csid->id));
>  }
>
> -static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8 vc)
> +static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8 port, u8 vc)
>  {
> -       struct v4l2_mbus_framefmt *input_format = &csid->fmt[MSM_CSID_PAD_FIRST_SRC + vc];
> +       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);
> @@ -233,28 +233,28 @@ static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8
>                 lane_cnt = 4;
>
>         val = 0;
> -       writel(val, csid->base + CSID_RDI_FRM_DROP_PERIOD(vc));
> +       writel(val, csid->base + CSID_RDI_FRM_DROP_PERIOD(port));
>
>         /*
>          * 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)
> +        * CSID_RDI_CFG0(port)
>          * DT_ID : 28:27
>          * VC    : 26:22
>          * DT    : 21:16
>          *
>          * CID   : VC 3:0 << 2 | DT_ID 1:0
>          */
> -       dt_id = vc & 0x03;
> +       dt_id = port & 0x03;
>
>         /* note: for non-RDI path, this should be format->decode_format */
>         val |= DECODE_FORMAT_PAYLOAD_ONLY << RDI_CFG0_DECODE_FORMAT;
>         val |= format->data_type << RDI_CFG0_DATA_TYPE;
>         val |= vc << RDI_CFG0_VIRTUAL_CHANNEL;
>         val |= dt_id << RDI_CFG0_DT_ID;
> -       writel(val, csid->base + CSID_RDI_CFG0(vc));
> +       writel(val, csid->base + CSID_RDI_CFG0(port));
>
>         val = RDI_CFG1_TIMESTAMP_STB_FRAME;
>         val |= RDI_CFG1_BYTE_CNTR_EN;
> @@ -265,23 +265,23 @@ static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8
>         val |= RDI_CFG1_CROP_V_EN;
>         val |= RDI_CFG1_PACKING_MIPI;
>
> -       writel(val, csid->base + CSID_RDI_CFG1(vc));
> +       writel(val, csid->base + CSID_RDI_CFG1(port));
>
>         val = 0;
> -       writel(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PERIOD(vc));
> +       writel(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PERIOD(port));
>
>         val = 1;
> -       writel(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PATTERN(vc));
> +       writel(val, csid->base + CSID_RDI_IRQ_SUBSAMPLE_PATTERN(port));
>
>         val = 0;
> -       writel(val, csid->base + CSID_RDI_CTRL(vc));
> +       writel(val, csid->base + CSID_RDI_CTRL(port));
>
> -       val = readl(csid->base + CSID_RDI_CFG0(vc));
> +       val = readl(csid->base + CSID_RDI_CFG0(port));
>         if (enable)
>                 val |= RDI_CFG0_ENABLE;
>         else
>                 val &= ~RDI_CFG0_ENABLE;
> -       writel(val, csid->base + CSID_RDI_CFG0(vc));
> +       writel(val, csid->base + CSID_RDI_CFG0(port));
>  }
>
>  static void csid_configure_stream(struct csid_device *csid, u8 enable)
> @@ -290,11 +290,11 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)
>
>         __csid_configure_top(csid);
>
> -       /* Loop through all enabled VCs and configure stream for each */
> +       /* Loop through all enabled ports and configure a stream for each */
>         for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++) {
>                 if (csid->phy.en_vc & BIT(i)) {
> -                       __csid_configure_rdi_stream(csid, enable, i);
> -                       __csid_configure_rx(csid, &csid->phy, i);
> +                       __csid_configure_rdi_stream(csid, enable, i, 0);
> +                       __csid_configure_rx(csid, &csid->phy, 0);
>                         __csid_ctrl_rdi(csid, enable, i);
>                 }
>         }
>
> --
> 2.52.0
>