Re: [PATCH v21 04/14] media: qcom: camss: Issue stream-on REG_UPDATE from CSID

[Loic Poulain]

On Wed, Oct 21, 2026 at 1:24 PM Hangxiang Ma
<hangxiang.ma@xxxxxxxxxxxxxxxx> wrote:
>
> On 10/21/2026 4:53 PM, Bryan O'Donoghue wrote:
> > On 21/10/2026 08:37, Wenmeng Liu wrote:
> >>
> >> Hi Bryan, Hangxiang,
> >>
> >> On 10/12/2026 2:45 PM, Hangxiang Ma wrote:
> >>> On stream-on, CAMSS starts the subdevices upstream from the video node
> >>> through their sink pads, so the VFE is started before the CSID.
> >>>
> >>> Starting with CSID 680, the REG_UPDATE is issued through the CSID
> >>> rather than the VFE. However, vfe_enable_output_v2() still issues a
> >>> REG_UPDATE after queueing each initial buffer, before the CSID has
> >>> configured the stream. The VFE then never signals BUF_DONE. Therefore,
> >>> the CAMSS now issues the REG_UPDATE from configure_stream() on CSID 680
> >>> and CSID Gen3 instead. The operation happens after the RX configuration
> >>> and before the RDI path is enabled.
> >>>
> >>> Skip the VFE-side stream-on REG_UPDATE only when the CSID connected to
> >>> the VFE sink pad implements the reg_update callback. Platforms whose
> >>> CSID does not provide this callback, as well as lines fed by ISPIF,
> >>> keep the existing VFE-side REG_UPDATE.
> >>>
> >>> Signed-off-by: Hangxiang Ma <hangxiang.ma@xxxxxxxxxxxxxxxx>
> >>> ---
> >>>   drivers/media/platform/qcom/camss/camss-csid-680.c |  3 ++
> >>>   .../media/platform/qcom/camss/camss-csid-gen3.c    |  3 ++
> >>>   drivers/media/platform/qcom/camss/camss-vfe.c      | 37 +++++++++++
> >>> + +++++++++-
> >>>   3 files changed, 42 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/platform/qcom/camss/camss-csid-680.c b/
> >>> drivers/media/platform/qcom/camss/camss-csid-680.c
> >>> index 345a67c8fb94..556cdfef6207 100644
> >>> --- a/drivers/media/platform/qcom/camss/camss-csid-680.c
> >>> +++ b/drivers/media/platform/qcom/camss/camss-csid-680.c
> >>> @@ -296,6 +296,8 @@ static void __csid_configure_rdi_stream(struct
> >>> csid_device *csid, u8 enable, u8
> >>>       writel(val, csid->base + CSID_RDI_CFG0(port));
> >>>   }
> >>> +static void csid_subdev_reg_update(struct csid_device *csid, int
> >>> port_id, bool is_clear);
> >>> +
> >>>   static void csid_configure_stream(struct csid_device *csid, u8 enable)
> >>>   {
> >>>       int i;
> >>> @@ -307,6 +309,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_subdev_reg_update(csid, i, false);
> >>>               __csid_ctrl_rdi(csid, enable, i);
> >>>           }
> >>>       }
> >>> diff --git a/drivers/media/platform/qcom/camss/camss-csid-gen3.c b/
> >>> drivers/media/platform/qcom/camss/camss-csid-gen3.c
> >>> index 0fdbf75fb27d..830c9c818d3d 100644
> >>> --- a/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> >>> +++ b/drivers/media/platform/qcom/camss/camss-csid-gen3.c
> >>> @@ -219,6 +219,8 @@ static void __csid_configure_rdi_stream(struct
> >>> csid_device *csid, u8 enable, u8
> >>>       writel(val, csid->base + CSID_RDI_CFG0(port));
> >>>   }
> >>> +static void csid_subdev_reg_update(struct csid_device *csid, int
> >>> port_id, bool is_clear);
> >>> +
> >>>   static void csid_configure_stream(struct csid_device *csid, u8 enable)
> >>>   {
> >>>       u8 i;
> >>> @@ -230,6 +232,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_subdev_reg_update(csid, i, false);
> >>>               __csid_ctrl_rdi(csid, enable, i);
> >>>           }
> >>>   }
> >>> diff --git a/drivers/media/platform/qcom/camss/camss-vfe.c b/drivers/
> >>> media/platform/qcom/camss/camss-vfe.c
> >>> index 2680a9914765..16a0b5663fe2 100644
> >>> --- a/drivers/media/platform/qcom/camss/camss-vfe.c
> >>> +++ b/drivers/media/platform/qcom/camss/camss-vfe.c
> >>> @@ -502,6 +502,37 @@ void vfe_buf_done(struct vfe_device *vfe, int wm)
> >>>       spin_unlock_irqrestore(&vfe->output_lock, flags);
> >>>   }
> >>> +/*
> >>> + * vfe_upstream_csid_has_reg_update - Check whether the CSID feeding
> >>> this line
> >>> + * issues its own streamon RUP (i.e. its configure_stream does the
> >>> RUP).
> >>> + *
> >>> + * The CSID->VFE link is created in camss.c for all CSIDs and VFEs,
> >>> so the
> >>> + * upstream entity of the sink pad is either a CSID (possibly with
> >>> RUP) or an
> >>> + * ISPIF (no RUP). Only enabled links are considered.
> >>> + */
> >>> +static bool vfe_upstream_csid_has_reg_update(struct vfe_line *line)
> >>> +{
> >>> +    struct vfe_device *vfe = to_vfe(line);
> >>> +    struct camss *camss = vfe->camss;
> >>> +    struct media_pad *remote;
> >>> +    unsigned int i;
> >>> +
> >>> +    remote = media_pad_remote_pad_first(&line->pads[MSM_VFE_PAD_SINK]);
> >>> +    if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
> >>> +        return false;
> >>> +
> >>> +    for (i = 0; i < camss->res->csid_num; i++) {
> >>> +        struct csid_device *csid = &camss->csid[i];
> >>> +
> >>> +        if (&csid->subdev.entity != remote->entity)
> >>> +            continue;
> >>> +
> >>> +        return !!csid->res->hw_ops->reg_update;
> >>> +    }
> >>> +
> >>> +    return false;
> >>> +}
> >>> +
> >>>   int vfe_enable_output_v2(struct vfe_line *line)
> >>>   {
> >>>       struct vfe_device *vfe = to_vfe(line);
> >>> @@ -511,6 +542,7 @@ int vfe_enable_output_v2(struct vfe_line *line)
> >>>       unsigned long flags;
> >>>       unsigned int frame_skip = 0;
> >>>       unsigned int i;
> >>> +    bool rup_in_csid;
> >>>       sensor_pad = camss_find_sensor_pad(&line->subdev.entity);
> >>>       if (sensor_pad) {
> >>> @@ -523,6 +555,8 @@ int vfe_enable_output_v2(struct vfe_line *line)
> >>>               frame_skip = VFE_FRAME_DROP_VAL - 1;
> >>>       }
> >>> +    rup_in_csid = vfe_upstream_csid_has_reg_update(line);
> >>> +
> >>>       spin_lock_irqsave(&vfe->output_lock, flags);
> >>>       ops->reg_update_clear(vfe, line->id);
> >>> @@ -552,7 +586,8 @@ int vfe_enable_output_v2(struct vfe_line *line)
> >>>           output->gen2.active_num++;
> >>>           ops->vfe_wm_update(vfe, output->wm_idx[0],
> >>>                      output->buf[i]->addr[0], line);
> >>> -        ops->reg_update(vfe, line->id);
> >>> +        if (!rup_in_csid)
> >>> +            ops->reg_update(vfe, line->id);
> >>>       }
> >>>       spin_unlock_irqrestore(&vfe->output_lock, flags);
> >>>
> >>
> >> According to the hardware documentation:
> >> Starting with CSID 680, CAMIF has moved into the CSID. As a result,
> >> RUP is now issued by the CSID and propagated to the IFE.
> >>
> >> Starting with CSID 980, software must also set the SET bit in
> >> RUP_AUP_CMD after issuing the RUP/AUP command. Otherwise, the hardware
> >> ignores the command and the corresponding register update does not
> >> take effect.
> >>
> >>
> >> Bryan, according to the HW documentation, the change itself is
> >> consistent with the required programming sequence. The main question
> >> is whether this is the right implementation from the driver
> >> perspective, or if there is a cleaner way to handle it.
> >>
> >> I have tested the change on the Hamoa EVK board.
> >> Tested-by: Wenmeng Liu <wenmeng.liu@xxxxxxxxxxxxxxxx> # Hamoa EVK
> >>
> >>
> >> Thanks,
> >> Wenmeng
> >
> > OK thank you for the explanation.
> >
> > OTOH I agree with Loic's statement - it is cleaner to subtract the
> > callback and disjunct on its presence than to add a flag to the same
> > effect in reverse.
> >
> > ---
> > bod
>
> Hi Loic, Bryan, thanks for the review. I wonder if it would be better to
> keep the check on the CSID side?
>
> vfe->reg_update() is also used outside stream-on, such as bufdone, queue
> buffer. It must be implemented in VFE. We only want to change the VFE
> behavior at stream-on stage, in function vfe_enable_output_v2(). So
> making vfe->reg_update() optional could affect runtime updates.

But the patch says:
'The REG_UPDATE is issued through the CSID rather than the VFE'

So you mean it's not always driven by CSID?

Also, today we perform a reg_update after queuing a buffer (i.e. after
programming ADDR). However, as far as I know, this is no longer
necessary in recent VFE/TFE/IFE generations. In those implementations,
the ADDR register is not double-buffered and is not part of the
reg-update set. Instead, it behaves as a FIFO entry, so issuing a
reg_update only results in a useless RUP_DONE interrupt.

>
> Maybe the cheapest way is still to check the CSID side reg_update
> implementation at stream-on stage? I'm very glad to hear your
> suggestions. Thanks a lot.
>
> ---
> Best Regards,
> Hangxiang