pipeline: imx8-isi: Fix crossbar's sink pad computation
diff mbox series

Message ID 20250814151735.3973329-1-antoine.bouyer@nxp.com
State Accepted
Commit a139cd3803071389053af66bdea16422743a11f2
Headers show
Series
  • pipeline: imx8-isi: Fix crossbar's sink pad computation
Related show

Commit Message

Antoine Bouyer Aug. 14, 2025, 3:17 p.m. UTC
In current implementation, the sink pad counter of the crossbar is not
incremented if the pad is not connected to any subdevice. This would lead
to incorrect routing and format configuration if CSI is not connected
to first sink pad.

To avoid such issue, every sink pads must be taken into account. Then if
CSI and sensor are present, current counter is used for routing at match(),
and stored in camera data to be reused during configure().

Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>
---
 src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Kieran Bingham Aug. 18, 2025, 11:36 a.m. UTC | #1
Quoting Antoine Bouyer (2025-08-14 16:17:35)
> In current implementation, the sink pad counter of the crossbar is not
> incremented if the pad is not connected to any subdevice. This would lead
> to incorrect routing and format configuration if CSI is not connected
> to first sink pad.
> 
> To avoid such issue, every sink pads must be taken into account. Then if
> CSI and sensor are present, current counter is used for routing at match(),
> and stored in camera data to be reused during configure().

This sounds pretty reasonable...

Although it's quite intrinsically in the main path of the ISI handler.

I assume this is being developed on i.MX9 ..
Can you confirm it's tested on both i.MX8 and i.MX9?

Are there any other developers or users who can provide a Tested-by: tag
from NXP or otherwise?

> 
> Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>



> ---
>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> index 72e055e400c6..de09431cb9b9 100644
> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> @@ -1039,7 +1039,7 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>         for (MediaPad *pad : crossbar_->entity()->pads()) {
>                 unsigned int sink = numSinks;
>  
> -               if (!(pad->flags() & MEDIA_PAD_FL_SINK) || pad->links().empty())
> +               if (!(pad->flags() & MEDIA_PAD_FL_SINK))
>                         continue;
>  
>                 /*
> @@ -1048,6 +1048,9 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>                  */
>                 numSinks++;
>  
> +               if (pad->links().empty())
> +                       continue;
> +
>                 MediaEntity *csi = pad->links()[0]->source()->entity();
>                 if (csi->pads().size() != 2) {
>                         LOG(ISI, Debug) << "Skip unsupported CSI-2 receiver "
> @@ -1082,6 +1085,7 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>                 LOG(ISI, Debug)
>                         << "cam" << numCameras
>                         << " streams " << data->streams_.size()
> +                       << " sink " << data->xbarSink_
>                         << " offset " << data->xbarSourceOffset_;
>  
>                 ret = data->init();
> -- 
> 2.34.1
>
Pavel Löbl Aug. 23, 2025, 8:42 a.m. UTC | #2
On Mon, 18 Aug 2025 12:36:04 +0100
kieran.bingham at ideasonboard.com (Kieran Bingham) wrote:

> Quoting Antoine Bouyer (2025-08-14 16:17:35)
> > In current implementation, the sink pad counter of the crossbar is
> > not incremented if the pad is not connected to any subdevice. This
> > would lead to incorrect routing and format configuration if CSI is
> > not connected to first sink pad.
> >
> > To avoid such issue, every sink pads must be taken into account.
> > Then if CSI and sensor are present, current counter is used for
> > routing at match(), and stored in camera data to be reused during
> > configure().  
> 
> This sounds pretty reasonable...
> 
> Although it's quite intrinsically in the main path of the ISI handler.
> 
> I assume this is being developed on i.MX9 ..
> Can you confirm it's tested on both i.MX8 and i.MX9?
> 
> Are there any other developers or users who can provide a Tested-by:
> tag from NXP or otherwise?
> 

I did exactly the same thing recently, while bringing up sensor
connected to second CSI of i.MX8MP, so you can add mine.

Tested-by: Pavel Löbl <pavel@loebl.cz>

Pavel

> >
> > Signed-off-by: Antoine Bouyer <antoine.bouyer at nxp.com>  
> 
> 
> 
> > ---
> >  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp index
> > 72e055e400c6..de09431cb9b9 100644 ---
> > a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp +++
> > b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp @@ -1039,7 +1039,7
> > @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator) for
> > (MediaPad *pad : crossbar_->entity()->pads()) { unsigned int sink =
> > numSinks;
> >
> > -               if (!(pad->flags() & MEDIA_PAD_FL_SINK) ||
> > pad->links().empty())
> > +               if (!(pad->flags() & MEDIA_PAD_FL_SINK))
> >                         continue;
> >
> >                 /*
> > @@ -1048,6 +1048,9 @@ bool
> > PipelineHandlerISI::match(DeviceEnumerator *enumerator) */
> >                 numSinks++;
> >
> > +               if (pad->links().empty())
> > +                       continue;
> > +
> >                 MediaEntity *csi =
> > pad->links()[0]->source()->entity(); if (csi->pads().size() != 2) {
> >                         LOG(ISI, Debug) << "Skip unsupported CSI-2
> > receiver " @@ -1082,6 +1085,7 @@ bool
> > PipelineHandlerISI::match(DeviceEnumerator *enumerator) LOG(ISI,
> > Debug) << "cam" << numCameras
> >                         << " streams " << data->streams_.size()
> > +                       << " sink " << data->xbarSink_
> >                         << " offset " << data->xbarSourceOffset_;
> >
> >                 ret = data->init();
> > --
> > 2.34.1
> >
Julien Vuillaumier Aug. 28, 2025, 3:57 p.m. UTC | #3
Hi Kieran,

On 18/08/2025 13:36, Kieran Bingham wrote:
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> 
> 
> Quoting Antoine Bouyer (2025-08-14 16:17:35)
>> In current implementation, the sink pad counter of the crossbar is not
>> incremented if the pad is not connected to any subdevice. This would lead
>> to incorrect routing and format configuration if CSI is not connected
>> to first sink pad.
>>
>> To avoid such issue, every sink pads must be taken into account. Then if
>> CSI and sensor are present, current counter is used for routing at match(),
>> and stored in camera data to be reused during configure().
> 
> This sounds pretty reasonable...
> 
> Although it's quite intrinsically in the main path of the ISI handler.
> 
> I assume this is being developed on i.MX9 ..
> Can you confirm it's tested on both i.MX8 and i.MX9?
> 
> Are there any other developers or users who can provide a Tested-by: tag
> from NXP or otherwise?


Just to confirm, that change has been verified on multiple i.MX8 
platforms including i.MX8M Plus with 2 cameras.

Tested-by: Julien Vuillaumier <julien.vuillaumier@nxp.com>

For i.MX95 support by imx8-isi pipeline, a couple of additional changes 
are still needed that will be posted here.

Thanks,
Julien

> 
>>
>> Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>
> 
> 
> 
>> ---
>>   src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>> index 72e055e400c6..de09431cb9b9 100644
>> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>> @@ -1039,7 +1039,7 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>>          for (MediaPad *pad : crossbar_->entity()->pads()) {
>>                  unsigned int sink = numSinks;
>>
>> -               if (!(pad->flags() & MEDIA_PAD_FL_SINK) || pad->links().empty())
>> +               if (!(pad->flags() & MEDIA_PAD_FL_SINK))
>>                          continue;
>>
>>                  /*
>> @@ -1048,6 +1048,9 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>>                   */
>>                  numSinks++;
>>
>> +               if (pad->links().empty())
>> +                       continue;
>> +
>>                  MediaEntity *csi = pad->links()[0]->source()->entity();
>>                  if (csi->pads().size() != 2) {
>>                          LOG(ISI, Debug) << "Skip unsupported CSI-2 receiver "
>> @@ -1082,6 +1085,7 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>>                  LOG(ISI, Debug)
>>                          << "cam" << numCameras
>>                          << " streams " << data->streams_.size()
>> +                       << " sink " << data->xbarSink_
>>                          << " offset " << data->xbarSourceOffset_;
>>
>>                  ret = data->init();
>> --
>> 2.34.1
>>
Kieran Bingham Aug. 29, 2025, 9:36 p.m. UTC | #4
Quoting Antoine Bouyer (2025-08-14 16:17:35)
> In current implementation, the sink pad counter of the crossbar is not
> incremented if the pad is not connected to any subdevice. This would lead
> to incorrect routing and format configuration if CSI is not connected
> to first sink pad.
> 
> To avoid such issue, every sink pads must be taken into account. Then if
> CSI and sensor are present, current counter is used for routing at match(),
> and stored in camera data to be reused during configure().
> 

Ok, merging this.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Antoine Bouyer <antoine.bouyer@nxp.com>
> ---
>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> index 72e055e400c6..de09431cb9b9 100644
> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> @@ -1039,7 +1039,7 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>         for (MediaPad *pad : crossbar_->entity()->pads()) {
>                 unsigned int sink = numSinks;
>  
> -               if (!(pad->flags() & MEDIA_PAD_FL_SINK) || pad->links().empty())
> +               if (!(pad->flags() & MEDIA_PAD_FL_SINK))
>                         continue;
>  
>                 /*
> @@ -1048,6 +1048,9 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>                  */
>                 numSinks++;
>  
> +               if (pad->links().empty())
> +                       continue;
> +
>                 MediaEntity *csi = pad->links()[0]->source()->entity();
>                 if (csi->pads().size() != 2) {
>                         LOG(ISI, Debug) << "Skip unsupported CSI-2 receiver "
> @@ -1082,6 +1085,7 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>                 LOG(ISI, Debug)
>                         << "cam" << numCameras
>                         << " streams " << data->streams_.size()
> +                       << " sink " << data->xbarSink_
>                         << " offset " << data->xbarSourceOffset_;
>  
>                 ret = data->init();
> -- 
> 2.34.1
>

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
index 72e055e400c6..de09431cb9b9 100644
--- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
+++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
@@ -1039,7 +1039,7 @@  bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
 	for (MediaPad *pad : crossbar_->entity()->pads()) {
 		unsigned int sink = numSinks;
 
-		if (!(pad->flags() & MEDIA_PAD_FL_SINK) || pad->links().empty())
+		if (!(pad->flags() & MEDIA_PAD_FL_SINK))
 			continue;
 
 		/*
@@ -1048,6 +1048,9 @@  bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
 		 */
 		numSinks++;
 
+		if (pad->links().empty())
+			continue;
+
 		MediaEntity *csi = pad->links()[0]->source()->entity();
 		if (csi->pads().size() != 2) {
 			LOG(ISI, Debug) << "Skip unsupported CSI-2 receiver "
@@ -1082,6 +1085,7 @@  bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
 		LOG(ISI, Debug)
 			<< "cam" << numCameras
 			<< " streams " << data->streams_.size()
+			<< " sink " << data->xbarSink_
 			<< " offset " << data->xbarSourceOffset_;
 
 		ret = data->init();