Re: [PATCH 3/8] media: qcom: camss: Implement CSID streams API hw_ops for gen2

["Gjorgji Rosikopulos (Consultant)" <gjorgji.rosikopulos@oss.qualcomm.com>]

Hi Loic, 

Thanks for the review,

On 9/11/2026 4:30 PM, Loic Poulain wrote:
> Hi Gjorgji,
> 
> On Fri, Sep 11, 2026 at 8:22 AM Gjorgji Rosikopulos
> <gjorgji.rosikopulos@oss.qualcomm.com> wrote:
>>
>> From: Gjorgji Rosikopulos <gjorgji.rosikopulos@oss.qualcomm.com>
>>
>> Implement the configure_rx/enable_stream/disable_stream hw_ops added to
>> struct csid_hw_ops, for the gen2 CSID hardware backend used by SM8250.
>>
>> __csid_configure_rdi_stream() now takes an explicit data type parameter
>> instead of deriving it from the CSID source pad's format, since the
>> streams API keys each stream by stream_id rather than by pad. The
>> existing configure_stream() path is updated to pass its per-port format
>> lookup through to this parameter, preserving current behavior.
>>
>> Signed-off-by: Gjorgji Rosikopulos <gjorgji.rosikopulos@oss.qualcomm.com>
>> ---
>>  .../platform/qcom/camss/camss-csid-gen2.c     | 59 ++++++++++++++++---
>>  1 file changed, 51 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen2.c b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
>> index eadcb2f7e3aa..2512cdb16d94 100644
>> --- a/drivers/media/platform/qcom/camss/camss-csid-gen2.c
>> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen2.c
>> @@ -253,13 +253,8 @@ static void __csid_configure_testgen(struct csid_device *csid, u8 enable, u8 por
>>         writel_relaxed(val, csid->base + CSID_TPG_CTRL);
>>  }
>>
>> -static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8 port, u8 vc)
>> +static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8 port, u8 vc, u8 dt)
>>  {
>> -       /* Source pads matching RDI channels on hardware. 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);
>>         u32 val;
>>
>>         /*
>> @@ -281,7 +276,7 @@ static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8
>>         val |= 1 << RDI_CFG0_TIMESTAMP_EN;
>>         /* 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 |= dt << RDI_CFG0_DATA_TYPE;
>>         val |= vc << RDI_CFG0_VIRTUAL_CHANNEL;
>>         val |= dt_id << RDI_CFG0_DT_ID;
>>         writel_relaxed(val, csid->base + CSID_RDI_CFG0(port));
>> @@ -330,15 +325,60 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)
>>         /* 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)) {
>> +                       /* Source pads match RDI channels: pad 1 -> RDI0, pad 2 -> RDI1, etc. */
>> +                       struct v4l2_mbus_framefmt *input_format =
>> +                               &csid->fmt[MSM_CSID_PAD_FIRST_SRC + i];
>> +                       const struct csid_format_info *format =
>> +                               csid_get_fmt_entry(csid->res->formats->formats,
>> +                                                  csid->res->formats->nformats,
>> +                                                  input_format->code);
>> +                       u8 vc = 0;
>> +
>>                         if (tg->enabled)
>>                                 __csid_configure_testgen(csid, enable, i, 0);
>>
>> -                       __csid_configure_rdi_stream(csid, enable, i, 0);
>> +                       __csid_configure_rdi_stream(csid, enable, i, vc, format->data_type);
>>                         __csid_configure_rx(csid, &csid->phy, 0);
>>                         __csid_ctrl_rdi(csid, enable, i);
>>                 }
>>  }
>>
>> +/*
>> + * configure_rx - Configure the CSID Rx front-end
>> + */
>> +static void csid_configure_rx(struct csid_device *csid)
>> +{
>> +       __csid_configure_rx(csid, &csid->phy, 0);
>> +}
>> +
>> +/*
>> + * stream_id is used directly as the hardware RDI port index below. This
>> + * assumes a 1:1 stream-to-port mapping and should be revisited once
>> + * per-platform src_streams data (stream id -> hw pipe) is added.
>> + */
>> +static void csid_enable_stream(struct csid_device *csid, u32 stream_id, u8 vc, u8 dt)
>> +{
>> +       struct csid_testgen_config *tg = &csid->testgen;
>> +
>> +       if (tg->enabled)
>> +               __csid_configure_testgen(csid, 1, stream_id, vc);
>> +
>> +       __csid_configure_rdi_stream(csid, 1, stream_id, vc, dt);
>> +       __csid_ctrl_rdi(csid, 1, stream_id);
>> +}
>> +
>> +static void csid_disable_stream(struct csid_device *csid, u32 stream_id)
>> +{
>> +       struct csid_testgen_config *tg = &csid->testgen;
>> +
>> +       __csid_ctrl_rdi(csid, 0, stream_id);
>> +
>> +       if (tg->enabled)
>> +               __csid_configure_testgen(csid, 0, stream_id, 0);
>> +
>> +       __csid_configure_rdi_stream(csid, 0, stream_id, 0, 0);
>> +}
>> +
>>  static int csid_configure_testgen_pattern(struct csid_device *csid, s32 val)
>>  {
>>         if (val > 0 && val <= csid->testgen.nmodes)
>> @@ -425,6 +465,9 @@ static void csid_subdev_init(struct csid_device *csid)
>>  const struct csid_hw_ops csid_ops_gen2 = {
>>         .configure_stream = csid_configure_stream,
>>         .configure_testgen_pattern = csid_configure_testgen_pattern,
>> +       .configure_rx = csid_configure_rx,
>> +       .enable_stream = csid_enable_stream,
>> +       .disable_stream = csid_disable_stream,
> 
> Would it be possible to leverage the existing CSID configure_stream()
> callback to implement the stream API? This would likely require
> reconfiguring all CSID ports whenever a stream is started/stopped, but
> if the configuration remains unchanged, I do not think that would be
> problematic. That said, we probably need to address the current
> ambiguity between the concepts of "VC" and "PORT" in the existing
> drivers first.

I have experimented with something similar, we know in advance which
streams will be routed and active using stream routes, we can enable
them in advance. The drawback of that was that all the streams (even inactive)
will be enabled when first stream is enabled, but that is the case even today.

What you propose is something different, but that will require changing of the existing
api and verification of all csid implementations, i am ok with the proposal 
if someone can help with the verification of other devices.

~Gjorgji