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

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

On 9/11/2026 5:17 PM, Gjorgji Rosikopulos (Consultant) wrote:
> 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.

In addition using configure_stream and one api for multiple streams will make the interface
more complex. We need to extend the interface with stream id's and pass vc/dt for each stream.
I think that current enable/disable stream is more simple for this implementation.

~Gjorgji