[PATCH] media: intel/ipu6: Set the VC of the stream as the SOF event id

[PATCH] media: intel/ipu6: Set the VC of the stream as the SOF event id

[Hongju Wang]

In the virtual channel case, they should use v4l2_event.id to distinguish
SOF events of different streams. Therefore, we set the virtual channel
number of the stream as the SOF event ID. This number is unique.

Signed-off-by: Hongju Wang <hongju.wang@intel.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
index 051898ce53f4..5ad426afa0f0 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
@@ -578,6 +578,7 @@ void ipu6_isys_csi2_sof_event_by_stream(struct ipu6_isys_stream *stream)
 		.type = V4L2_EVENT_FRAME_SYNC,
 	};
 
+	ev.id = stream->vc;
 	ev.u.frame_sync.frame_sequence = atomic_fetch_inc(&stream->sequence);
 	v4l2_event_queue(vdev, &ev);
 
-- 
2.34.1

Re: [PATCH] media: intel/ipu6: Set the VC of the stream as the SOF event id

[Sakari Ailus]

Hi Hongju,

On Thu, Oct 24, 2024 at 11:22:01AM +0800, Hongju Wang wrote:
> In the virtual channel case, they should use v4l2_event.id to distinguish
> SOF events of different streams. Therefore, we set the virtual channel
> number of the stream as the SOF event ID. This number is unique.
> 
> Signed-off-by: Hongju Wang <hongju.wang@intel.com>
> ---
>  drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> index 051898ce53f4..5ad426afa0f0 100644
> --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> @@ -578,6 +578,7 @@ void ipu6_isys_csi2_sof_event_by_stream(struct ipu6_isys_stream *stream)
>  		.type = V4L2_EVENT_FRAME_SYNC,
>  	};
>  
> +	ev.id = stream->vc;
>  	ev.u.frame_sync.frame_sequence = atomic_fetch_inc(&stream->sequence);
>  	v4l2_event_queue(vdev, &ev);
>  

The id field in struct v4l2_event isn't used for the FRAME_SYNC event and
also the virtual channel isn't communicated to the user space currently.

Doesn't the video device itself (and the routing configuration) associate
this to a given sensor already?

-- 
Kind regards,

Sakari Ailus

RE: [PATCH] media: intel/ipu6: Set the VC of the stream as the SOF event id

[Wang, Hongju]

Hi Sakari

> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> Sent: Thursday, October 24, 2024 6:31 PM
> To: Wang, Hongju <hongju.wang@intel.com>
> Cc: linux-media@vger.kernel.org; bingbu.cao@linux.intel.com
> Subject: Re: [PATCH] media: intel/ipu6: Set the VC of the stream as the
> SOF event id
> 
> Hi Hongju,
> 
> On Thu, Oct 24, 2024 at 11:22:01AM +0800, Hongju Wang wrote:
> > In the virtual channel case, they should use v4l2_event.id to
> > distinguish SOF events of different streams. Therefore, we set the
> > virtual channel number of the stream as the SOF event ID. This number is
> unique.
> >
> > Signed-off-by: Hongju Wang <hongju.wang@intel.com>
> > ---
> >  drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > index 051898ce53f4..5ad426afa0f0 100644
> > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > @@ -578,6 +578,7 @@ void ipu6_isys_csi2_sof_event_by_stream(struct
> ipu6_isys_stream *stream)
> >  		.type = V4L2_EVENT_FRAME_SYNC,
> >  	};
> >
> > +	ev.id = stream->vc;
> >  	ev.u.frame_sync.frame_sequence = atomic_fetch_inc(&stream->sequence);
> >  	v4l2_event_queue(vdev, &ev);
> >
> 
> The id field in struct v4l2_event isn't used for the FRAME_SYNC event and
> also the virtual channel isn't communicated to the user space currently.
	
  How to use the id field? User space should use SOF to do FRAME_SYNC,
  but the v4l2_event doesn't distinguish different streams. Do you have
  any good suggestions?

> Doesn't the video device itself (and the routing configuration) associate
> this to a given sensor already?

  Yes, they are already connected. But we can't use the video device
  to do FRAME_SYNC, The CSI2 hardware interrupt has processing for FRAME_SYNC,
  we cannot pass it to the video device.
  

> 
> --
> Kind regards,
> 
> Sakari Ailus

Re: [PATCH] media: intel/ipu6: Set the VC of the stream as the SOF event id

[Sakari Ailus]

Hi Hongju,

On Mon, Nov 04, 2024 at 03:07:58AM +0000, Wang, Hongju wrote:
> Hi Sakari
> 
> > -----Original Message-----
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Sent: Thursday, October 24, 2024 6:31 PM
> > To: Wang, Hongju <hongju.wang@intel.com>
> > Cc: linux-media@vger.kernel.org; bingbu.cao@linux.intel.com
> > Subject: Re: [PATCH] media: intel/ipu6: Set the VC of the stream as the
> > SOF event id
> > 
> > Hi Hongju,
> > 
> > On Thu, Oct 24, 2024 at 11:22:01AM +0800, Hongju Wang wrote:
> > > In the virtual channel case, they should use v4l2_event.id to
> > > distinguish SOF events of different streams. Therefore, we set the
> > > virtual channel number of the stream as the SOF event ID. This number is
> > unique.
> > >
> > > Signed-off-by: Hongju Wang <hongju.wang@intel.com>
> > > ---
> > >  drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > index 051898ce53f4..5ad426afa0f0 100644
> > > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > @@ -578,6 +578,7 @@ void ipu6_isys_csi2_sof_event_by_stream(struct
> > ipu6_isys_stream *stream)
> > >  		.type = V4L2_EVENT_FRAME_SYNC,
> > >  	};
> > >
> > > +	ev.id = stream->vc;
> > >  	ev.u.frame_sync.frame_sequence = atomic_fetch_inc(&stream->sequence);
> > >  	v4l2_event_queue(vdev, &ev);
> > >
> > 
> > The id field in struct v4l2_event isn't used for the FRAME_SYNC event and
> > also the virtual channel isn't communicated to the user space currently.
> 	
>   How to use the id field? User space should use SOF to do FRAME_SYNC,
>   but the v4l2_event doesn't distinguish different streams. Do you have
>   any good suggestions?

Ah, now I think I do understand the problem. The events are produced by the
CSI-2 receiver sub-device but there are multiple streams and the event
isn't stream-aware?

It's nice that the id field of the FRAME_SYNC event isn't in use for any
other purpose. This should be taken into account in event subscription,
too.

I'll add a patch documenting this in the metadata series. Any thoughts?

Cc Laurent and Hans.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH] media: intel/ipu6: Set the VC of the stream as the SOF event id

[Laurent Pinchart]

On Mon, Nov 04, 2024 at 07:56:51AM +0000, Sakari Ailus wrote:
> On Mon, Nov 04, 2024 at 03:07:58AM +0000, Wang, Hongju wrote:
> > On Thursday, October 24, 2024 6:31 PM, Sakari Ailus wrote:
> > > On Thu, Oct 24, 2024 at 11:22:01AM +0800, Hongju Wang wrote:
> > > > In the virtual channel case, they should use v4l2_event.id to
> > > > distinguish SOF events of different streams. Therefore, we set the
> > > > virtual channel number of the stream as the SOF event ID. This number is unique.
> > > >
> > > > Signed-off-by: Hongju Wang <hongju.wang@intel.com>
> > > > ---
> > > >  drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > >
> > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > index 051898ce53f4..5ad426afa0f0 100644
> > > > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > @@ -578,6 +578,7 @@ void ipu6_isys_csi2_sof_event_by_stream(struct ipu6_isys_stream *stream)
> > > >  		.type = V4L2_EVENT_FRAME_SYNC,
> > > >  	};
> > > >
> > > > +	ev.id = stream->vc;
> > > >  	ev.u.frame_sync.frame_sequence = atomic_fetch_inc(&stream->sequence);
> > > >  	v4l2_event_queue(vdev, &ev);
> > > >
> > > 
> > > The id field in struct v4l2_event isn't used for the FRAME_SYNC event and
> > > also the virtual channel isn't communicated to the user space currently.
> > 	
> >   How to use the id field? User space should use SOF to do FRAME_SYNC,
> >   but the v4l2_event doesn't distinguish different streams. Do you have
> >   any good suggestions?
> 
> Ah, now I think I do understand the problem. The events are produced by the
> CSI-2 receiver sub-device but there are multiple streams and the event
> isn't stream-aware?
> 
> It's nice that the id field of the FRAME_SYNC event isn't in use for any
> other purpose. This should be taken into account in event subscription,
> too.
> 
> I'll add a patch documenting this in the metadata series. Any thoughts?

I think we could use the id field for this purpose, but maybe not to
report the VC. Userspace isn't VC-aware, we should instead convert the
VC to a stream.

> Cc Laurent and Hans.

-- 
Regards,

Laurent Pinchart

RE: [PATCH] media: intel/ipu6: Set the VC of the stream as the SOF event id

[Wang, Hongju]

Hi Laurent,

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Monday, November 4, 2024 4:25 PM
> To: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: Wang, Hongju <hongju.wang@intel.com>; linux-media@vger.kernel.org;
> bingbu.cao@linux.intel.com; hverkuil@xs4all.nl
> Subject: Re: [PATCH] media: intel/ipu6: Set the VC of the stream as the
> SOF event id
> 
> On Mon, Nov 04, 2024 at 07:56:51AM +0000, Sakari Ailus wrote:
> > On Mon, Nov 04, 2024 at 03:07:58AM +0000, Wang, Hongju wrote:
> > > On Thursday, October 24, 2024 6:31 PM, Sakari Ailus wrote:
> > > > On Thu, Oct 24, 2024 at 11:22:01AM +0800, Hongju Wang wrote:
> > > > > In the virtual channel case, they should use v4l2_event.id to
> > > > > distinguish SOF events of different streams. Therefore, we set
> > > > > the virtual channel number of the stream as the SOF event ID. This
> number is unique.
> > > > >
> > > > > Signed-off-by: Hongju Wang <hongju.wang@intel.com>
> > > > > ---
> > > > >  drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 1 +
> > > > >  1 file changed, 1 insertion(+)
> > > > >
> > > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > > b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > > index 051898ce53f4..5ad426afa0f0 100644
> > > > > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > > @@ -578,6 +578,7 @@ void ipu6_isys_csi2_sof_event_by_stream(struct
> ipu6_isys_stream *stream)
> > > > >  		.type = V4L2_EVENT_FRAME_SYNC,
> > > > >  	};
> > > > >
> > > > > +	ev.id = stream->vc;
> > > > >  	ev.u.frame_sync.frame_sequence = atomic_fetch_inc(&stream-
> >sequence);
> > > > >  	v4l2_event_queue(vdev, &ev);
> > > > >
> > > >
> > > > The id field in struct v4l2_event isn't used for the FRAME_SYNC
> > > > event and also the virtual channel isn't communicated to the user
> space currently.
> > >
> > >   How to use the id field? User space should use SOF to do FRAME_SYNC,
> > >   but the v4l2_event doesn't distinguish different streams. Do you
> have
> > >   any good suggestions?
> >
> > Ah, now I think I do understand the problem. The events are produced
> > by the
> > CSI-2 receiver sub-device but there are multiple streams and the event
> > isn't stream-aware?
> >
> > It's nice that the id field of the FRAME_SYNC event isn't in use for
> > any other purpose. This should be taken into account in event
> > subscription, too.
> >
> > I'll add a patch documenting this in the metadata series. Any thoughts?
> 
> I think we could use the id field for this purpose, but maybe not to
> report the VC. Userspace isn't VC-aware, we should instead convert the VC
> to a stream.
>
  Do you mean that we use a stream id? But which stream is used isn't fixed.
  Only when video streams on, we select a stream for it. Which sensor uses
  Which VC is fixed by sensor driver.  

> > Cc Laurent and Hans.
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH] media: intel/ipu6: Set the VC of the stream as the SOF event id

[Laurent Pinchart]

On Tue, Nov 05, 2024 at 03:23:50AM +0000, Wang, Hongju wrote:
> On Monday, November 4, 2024 4:25 PM, Laurent Pinchart wrote:
> > On Mon, Nov 04, 2024 at 07:56:51AM +0000, Sakari Ailus wrote:
> > > On Mon, Nov 04, 2024 at 03:07:58AM +0000, Wang, Hongju wrote:
> > > > On Thursday, October 24, 2024 6:31 PM, Sakari Ailus wrote:
> > > > > On Thu, Oct 24, 2024 at 11:22:01AM +0800, Hongju Wang wrote:
> > > > > > In the virtual channel case, they should use v4l2_event.id to
> > > > > > distinguish SOF events of different streams. Therefore, we set
> > > > > > the virtual channel number of the stream as the SOF event ID. This number is unique.
> > > > > >
> > > > > > Signed-off-by: Hongju Wang <hongju.wang@intel.com>
> > > > > > ---
> > > > > >  drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 1 +
> > > > > >  1 file changed, 1 insertion(+)
> > > > > >
> > > > > > diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > > > b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > > > index 051898ce53f4..5ad426afa0f0 100644
> > > > > > --- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > > > +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
> > > > > > @@ -578,6 +578,7 @@ void ipu6_isys_csi2_sof_event_by_stream(struct ipu6_isys_stream *stream)
> > > > > >  		.type = V4L2_EVENT_FRAME_SYNC,
> > > > > >  	};
> > > > > >
> > > > > > +	ev.id = stream->vc;
> > > > > >  	ev.u.frame_sync.frame_sequence = atomic_fetch_inc(&stream->sequence);
> > > > > >  	v4l2_event_queue(vdev, &ev);
> > > > > >
> > > > >
> > > > > The id field in struct v4l2_event isn't used for the FRAME_SYNC
> > > > > event and also the virtual channel isn't communicated to the user space currently.
> > > >
> > > >   How to use the id field? User space should use SOF to do FRAME_SYNC,
> > > >   but the v4l2_event doesn't distinguish different streams. Do you have
> > > >   any good suggestions?
> > >
> > > Ah, now I think I do understand the problem. The events are produced by the
> > > CSI-2 receiver sub-device but there are multiple streams and the event
> > > isn't stream-aware?
> > >
> > > It's nice that the id field of the FRAME_SYNC event isn't in use for
> > > any other purpose. This should be taken into account in event
> > > subscription, too.
> > >
> > > I'll add a patch documenting this in the metadata series. Any thoughts?
> > 
> > I think we could use the id field for this purpose, but maybe not to
> > report the VC. Userspace isn't VC-aware, we should instead convert the VC
> > to a stream.
>
> Do you mean that we use a stream id? But which stream is used isn't fixed.
> Only when video streams on, we select a stream for it. Which sensor uses
> Which VC is fixed by sensor driver.  

The event isn't generated until you start streaming, so by that time you
should be able to convert the VC to a stream ID.

The VC can't easily be used by userspace, as there's no way for an
application to map VCs to streams. To make this API really usable, we
need something that applications can meaningfully use. I would like to
see a userspace implementation (ideally in libcamera) to showcase that
the API works as expected. 

> > > Cc Laurent and Hans.

-- 
Regards,

Laurent Pinchart