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

[Hangxiang Ma <hangxiang.ma@oss.qualcomm.com>]

On 10/21/2026 9:00 PM, Loic Poulain wrote:
> On Wed, Oct 21, 2026 at 1:24 PM Hangxiang Ma
> <hangxiang.ma@oss.qualcomm.com> 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@oss.qualcomm.com>
>>>>> ---
>>>>>    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@oss.qualcomm.com> # 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

Yes. Before the 680 platform, VFE issued the REG_UPDATE itself. Starting 
with 680, however, the REG_UPDATE signal is controlled by CSID rather 
than VFE. The CSID stream-on RUP is issued from the CSID start path 
after VFE has been started.

Wenmeng previously described the hardware evolution and the documented 
initialization sequence in this thread. I hope this helps clarify the 
background.

https://lore.kernel.org/all/3843110c-4766-4352-ab2a-6d3017336534@oss.qualcomm.com/

Going back to the split AUP/RUP design, this was introduced in the 
Pakala (980) generation. The split AUP/RUP design allows us to select 
ADDR or REG updates independently. However, since the 680 generation, 
REG_UPDATE has been moved to the CSID side, which does not have the same 
split capability.

I agree that combining AUP and RUP would generate an unnecessary 
RUP_DONE interrupt. However, please take a look at the implementation of 
csid_subdev_reg_updatem() in camss-csid-1080.c. In this series, we have 
already followed the approach you suggested above: during runtime, we 
configure and trigger only AUP, without RUP. Therefore, no unnecessary 
RUP_DONE interrupt will be generated.

I'm very open to further discussion and sincerely appreciate your 
feedback. If there is anything I have misunderstood, or if you have a 
better approach in mind, please feel free to point it out. I'd be more 
than happy to clarify any questions and learn from your suggestions.

---
Best Regards,
Hangxiang