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

[Loic Poulain]

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.

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.

Regards,
Loic