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

[Loic Poulain]

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.