Message ID | 20250814151735.3973329-1-antoine.bouyer@nxp.com |
---|---|
State | Accepted |
Commit | a139cd3803071389053af66bdea16422743a11f2 |
Headers | show |
Series |
|
Related | show |
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 >
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 > >
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 >>
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 >
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();
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(-)