Re: [PATCH 05/14] media: qcom: camss: csid-gen3: Enable RDI SOF driving

[Loic Poulain]

On Thu, Oct 29, 2026 at 4:43 PM Loic Poulain
<loic.poulain@xxxxxxxxxxxxxxxx> wrote:
>
> On Thu, Oct 29, 2026 at 10:55 AM <bryan.odonoghue@xxxxxxxxxx> wrote:
> >
> > From: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> >
> > Add in silicon enabling code to generate SOF and trigger the routine which
> > produces V4L2_EVENT_FRAME_SYNC based on SOF per standard v4l2 behaviour.
> >
> > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
> > ---
> >  .../media/platform/qcom/camss/camss-csid-gen3.c    | 27 ++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.c b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> > index 0fdbf75fb27db..6a538d825f78b 100644
> > --- a/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> > +++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> > @@ -34,6 +34,7 @@
> >  #define                CSID_RUP_AUP_RDI(rdi)           ((BIT(4) | BIT(20)) << (rdi))
> >
> >  #define CSID_TOP_IRQ_STATUS            0x7C
> > +#define                TOP_IRQ_RDI(rdi)                BIT(8 + (rdi))
> >  #define                 TOP_IRQ_STATUS_RESET_DONE      BIT(0)
> >
> >  #define CSID_TOP_IRQ_MASK              0x80
> > @@ -58,6 +59,9 @@
> >  #define CSID_CSI2_RDIN_IRQ_STATUS(rdi) (0xEC + 0x10 * (rdi))
> >  #define                RUP_DONE_IRQ_STATUS             BIT(23)
> >
> > +#define CSID_CSI2_RDIN_IRQ_MASK(rdi)   (0xF0 + 0x10 * (rdi))
> > +#define                INFO_INPUT_SOF_IRQ              BIT(12)
> > +
> >  #define CSID_CSI2_RDIN_IRQ_CLEAR(rdi)  (0xF4 + 0x10 * (rdi))
> >  #define CSID_CSI2_RDIN_IRQ_SET(rdi)    (0xF8 + 0x10 * (rdi))
> >
> > @@ -219,6 +223,25 @@ static void __csid_configure_rdi_stream(struct csid_device *csid, u8 enable, u8
> >         writel(val, csid->base + CSID_RDI_CFG0(port));
> >  }
> >
> > +static void __csid_configure_rdi_irq(struct csid_device *csid, u8 enable, u8 rdi)
> > +{
> > +       u32 val = 0;
> > +
> > +       if (enable)
> > +               val = INFO_INPUT_SOF_IRQ;
> > +
> > +       writel(val, csid->base + CSID_CSI2_RDIN_IRQ_MASK(rdi));
> > +
> > +       val = readl(csid->base + CSID_TOP_IRQ_MASK);
> > +
> > +       if (enable)
> > +               val |= TOP_IRQ_RDI(rdi);
> > +       else
> > +               val &= ~TOP_IRQ_RDI(rdi);
> > +
> > +       writel(val, csid->base + CSID_TOP_IRQ_MASK);
> > +}
> > +
> >  static void csid_configure_stream(struct csid_device *csid, u8 enable)
> >  {
> >         u8 i;
> > @@ -230,6 +253,7 @@ static void csid_configure_stream(struct csid_device *csid, u8 enable)
> >                 if (csid->phy.en_vc & BIT(i)) {
> >                         __csid_configure_rdi_stream(csid, enable, i, 0);
> >                         __csid_configure_rx(csid, &csid->phy, 0);
> > +                       __csid_configure_rdi_irq(csid, enable, i);
>
> Is this something that should also be tied to frame_sync_en?
>
> Also, just thinking out loud, are there cases where we'd want to
> toggle this dynamically at runtime based on any V4L2 configuration or
> pipeline path? For example, should it be enabled for the pixel path
> but disabled for RDI?

Obviously, if userspace subscribes to the related framesync event, we
should enable it regardless.

>
> I'm asking because one of the appreciated strengths of CAMSS is its
> relatively low-latency and lightweight capture path. As we add more
> features, it may be worth preserving that characteristic where
> possible. Not saying this particular feature has a meaningful latency
> or compute cost, but more as a general design consideration.
>
> Regards,
> Loic
>
> >                         __csid_ctrl_rdi(csid, enable, i);
> >                 }
> >  }
> > @@ -279,6 +303,9 @@ static irqreturn_t csid_isr(int irq, void *dev)
> >                         val = readl(csid->base + CSID_CSI2_RDIN_IRQ_STATUS(i));
> >                         writel(val, csid->base + CSID_CSI2_RDIN_IRQ_CLEAR(i));
> >
> > +                       if (val & INFO_INPUT_SOF_IRQ)
> > +                               csid_sof_event(csid, i);
> > +
> >                         if (val & RUP_DONE_IRQ_STATUS)
> >                                 /* clear the reg update bit */
> >                                 csid_subdev_reg_update(csid, i, true);
> >
> > --
> > 2.56.0
> >