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

[Gjorgji Rosikopulos (Consultant)]

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.

Regards,
~Gjorgji