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

[Gjorgji Rosikopulos (Consultant)]

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.

~Gjorgji

> 
> Regards,
> Loic