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

[Loic Poulain]

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.