Re: [PATCH v2 2/9] media: qcom: camss: Introduce SOF sequence tracking

[Gjorgji Rosikopulos (Consultant)]

Hi Loic,

On 11/3/2026 11:56 AM, Loic Poulain wrote:
> On Tue, Nov 3, 2026 at 10:53 AM Loic Poulain
> <loic.poulain@xxxxxxxxxxxxxxxx> wrote:
>>
>> On Tue, Nov 3, 2026 at 10:42 AM Gjorgji Rosikopulos (Consultant)
>> <gjorgji.rosikopulos@xxxxxxxxxxxxxxxx> wrote:
>>>
>>> Hi Loic,
>>>
>>> On 11/3/2026 11:33 AM, Loic Poulain wrote:
>>>> On Tue, Nov 3, 2026 at 8:11 AM Gjorgji Rosikopulos (Consultant)
>>>> <gjorgji.rosikopulos@xxxxxxxxxxxxxxxx> wrote:
>>>>>
>>>>> Hi Bryan,
>>>>>
>>>>> On 11/2/2026 3:56 PM, bryan.odonoghue@xxxxxxxxxx wrote:
>>>>>> From: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
>>>>>>
>>>>>> In order to communicate to user-space CSI2 SOF events we will need to hook
>>>>>> into and control v4l2_ctrl_subdev_subscribe_event. In-line with other
>>>>>> implementations of FRAME_SYNC have CSI decoders which support the event
>>>>>> always generate it in our case if and only if an atomic user count
>>>>>> indicates it should be so.
>>>>>>
>>>>>> A flag in the CSID descriptor indicates if frame_sync_en is supported so
>>>>>> for each SoC that supports it this flag should be true. Since bool is false
>>>>>> by default no negative enumeration of existing or in-flight CSID is
>>>>>> required.
>>>>>>
>>>>>> Subsequent patches will enable on a per-CSID basis.
>>>>>>
>>>>>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
>>>>>> ---
>>>>>>  drivers/media/platform/qcom/camss/camss-csid.c | 54 +++++++++++++++++++++++++-
>>>>>>  drivers/media/platform/qcom/camss/camss-csid.h | 12 ++++++
>>>>>>  2 files changed, 65 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/media/platform/qcom/camss/camss-csid.c b/drivers/media/platform/qcom/camss/camss-csid.c
>>>>>> index a00791b00feb7..e9b399fcb9c7a 100644
>>>>>> --- a/drivers/media/platform/qcom/camss/camss-csid.c
>>>>>> +++ b/drivers/media/platform/qcom/camss/camss-csid.c
>>>>>> @@ -760,6 +760,7 @@ static int csid_set_power(struct v4l2_subdev *sd, int on)
>>>>>>  static int csid_set_stream(struct v4l2_subdev *sd, int enable)
>>>>>>  {
>>>>>>       struct csid_device *csid = v4l2_get_subdevdata(sd);
>>>>>> +     unsigned int i;
>>>>>>       int ret;
>>>>>>
>>>>>>       if (enable) {
>>>>>> @@ -778,6 +779,9 @@ static int csid_set_stream(struct v4l2_subdev *sd, int enable)
>>>>>>       }
>>>>>>
>>>>>>       if (csid->phy.need_vc_update) {
>>>>>> +             for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
>>>>>> +                     atomic_set(&csid->sof_seq[i], -1);
>>>>>> +
>>>>>>               csid->res->hw_ops->configure_stream(csid, enable);
>>>>>>               csid->phy.need_vc_update = false;
>>>>>>       }
>>>>>> @@ -1316,9 +1320,57 @@ static int csid_link_setup(struct media_entity *entity,
>>>>>>       return 0;
>>>>>>  }
>>>>>>
>>>>>> +static int csid_frame_sync_add(struct v4l2_subscribed_event *sev, unsigned int elems)
>>>>>> +{
>>>>>> +     struct csid_device *csid = v4l2_get_subdevdata(vdev_to_v4l2_subdev(sev->fh->vdev));
>>>>>> +
>>>>>> +     atomic_inc(&csid->frame_sync_users[sev->id]);
>>>>>> +
>>>>>> +     return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static void csid_frame_sync_del(struct v4l2_subscribed_event *sev)
>>>>>> +{
>>>>>> +     struct csid_device *csid = v4l2_get_subdevdata(vdev_to_v4l2_subdev(sev->fh->vdev));
>>>>>> +
>>>>>> +     atomic_dec(&csid->frame_sync_users[sev->id]);
>>>>>> +}
>>>>>> +
>>>>>> +static const struct v4l2_subscribed_event_ops csid_frame_sync_ops = {
>>>>>> +     .add = csid_frame_sync_add,
>>>>>> +     .del = csid_frame_sync_del,
>>>>>> +};
>>>>>> +
>>>>>> +static int csid_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
>>>>>> +                             struct v4l2_event_subscription *sub)
>>>>>> +{
>>>>>> +     struct csid_device *csid = v4l2_get_subdevdata(sd);
>>>>>> +
>>>>>> +     if (sub->type != V4L2_EVENT_FRAME_SYNC)
>>>>>> +             return v4l2_ctrl_subdev_subscribe_event(sd, fh, sub);
>>>>>> +
>>>>>> +     if (sub->id >= MSM_CSID_MAX_SRC_STREAMS || !csid->res->frame_sync_en)
>>>>>> +             return -EINVAL;
>>>>>> +
>>>>>> +     return v4l2_event_subscribe(fh, sub, 0, &csid_frame_sync_ops);
>>>>>> +}
>>>>>> +
>>>>>> +void csid_sof_event(struct csid_device *csid, unsigned int id)
>>>>>> +{
>>>>>> +     struct v4l2_event event = {
>>>>>> +             .type = V4L2_EVENT_FRAME_SYNC,
>>>>>> +             .id = id,
>>>>>> +             .u.frame_sync.frame_sequence =
>>>>>> +                     atomic_inc_return(&csid->sof_seq[id]),
>>>>>> +     };
>>>>>
>>>>> On first look this was looking strange because the id is used for the
>>>>> control id events not for frame sync. But on second thought it may make
>>>>> sense, but the question is how the userspace should interpret those ids?
>>>>> As of today userspace does not have any info for the VC and csid internal
>>>>> pipelines (so called lines). So with the current state of the code
>>>>> the id is VC which makes sense becouse the SOF/EOF packets are recieved
>>>>> based on VC. So i am thinking of two options:
>>>>>
>>>>> 1. Move SOF event in vfe (rdi/pix sub-devs) we have per line sub-device
>>>>>    and will be well known to userspace for which line is reported.
>>>>>
>>>>> 2. Add and propose extension of videodev2 documentation that
>>>>>    id of frame_sync event types represents SOF per virtual channel.
>>>>>
>>>>> Personally i votoe for (1) because it will well fit with stream-api
>>>>> support and usespace will have clear understanding of the SOF to which
>>>>> stream belongs.
>>>>>
>>>>> Given this in mind it will be good to open discussion with Laurent, Sakari
>>>>> regarding SOF event with stream api and GMSL cameras.
>>>>
>>>> I think both approaches can be applied independently, forwarding the
>>>> event through the VFE as event proxy would likely be a straightforward
>>>> addition.
>>>
>>> Yes i agree as well, but the first need broader audience and agreement what
>>> should contain this id.
>>>
>>>>
>>>> Note that the id here represents the interface index. While it
>>>> currently happens to be associated with a VC, that is not necessarily
>>>> what we want. Ultimately, multiple interfaces can be configured for
>>>> the same VC.
>>>
>>> Yes and that is changed with stream api implementation, and the
>>> following question: what should be the ID when streams with same VC
>>> are routed in more lines (hw pipelines).
>>> But that depends on what this id should represent.
>>
>> Yes, then maybe option 1 is the way to go for now.
> 
> Also noet that vfe_hw_ops already also have a sof callback already,
> maybe we want to do something with that as well.

Yes i agree. Apart from that for pix path we may need sof in vfe as well.

~Gjorgji