| Message ID | 20251127154519.2038844-1-antoine.bouyer@nxp.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Antoine, Thank you for the series. On Thu, Nov 27, 2025 at 04:45:16PM +0100, Antoine Bouyer wrote: > Submit this imx8-isi rework on behalf of Andrei. This series is about > using libcamera MediaPipeline class to simplify imx8-isi pipeline > configuration. > > Instead of going over each pipeline subdevices during imx8-isi match(), > all is handled by MediaPipeline class. It helps supporting complex > topologies, where subdevice(s) could be optional, typically on i.MX95 > SoC which has a formatter, while other i.MX SoCs don't have it. I'm sorry but I don't think this goes in the right direction. The MediaPipeline class has been designed to handle linear chains of entities for cases where we don't know what those entities are like in the simple pipeline handler, or for entities external to the SoC (such as serializers and deserializers - and in this case it's even arguably a bit of a hack until we develop explicit support for serdes and external ISP). Entities within the SoC should be handled explicitly in the pipeline handler, especially when they need device-specific configuration such as for the crossbar switch. The formatter of the i.MX95 should also be handled explicitly. Could you please rework the pipeline handler in that direction ? It can start with a revert of patch 2/2 if that's a simpler (to develop and/or review) solution. > It reuses the simple pipeline's locateSensors method, so external ISP are > also supported then. ISP is considered as the 'sensor' element. > > Tested on i.MX8MP SoC. > > --- > Changes in v3: > - Apply review comments from Jacopo: Add \brief tag and remove '.' at the > end of variable descriptions; rephrase MediaPipeline::Entity description. > - Use the shared_ptr get() accessor to retrieve the isiDev_ MediaDevice > because this was updated in all pipelines after v2. > - link to v2: https://patchwork.libcamera.org/cover/25072/ > > Changes in v2: > - Add missing documentation as suggested by Barnabás > - Move Entity parameters documentation from .h to .cpp file. > - Replace 'video' by 'last' node in source descriptions, because in > imx8-isi pipeline case, the last MediaPipeline entity (i.e. crossbar) is > not a video node. > - Apply review comments from Jacopo: move Entity definition to beginning > of public section; move entities() to const-callable as well. > - link to v1: https://patchwork.libcamera.org/patch/25009/ > > --- > Andrei Gansari (2): > libamera: media_pipeline: Add accessor for MediaPipeline list of > entities > pipeline: imx8-isi: Integrating MediaPipeline class > > include/libcamera/internal/media_pipeline.h | 29 +--- > src/libcamera/media_pipeline.cpp | 46 ++++++ > src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 159 ++++++++++++------- > 3 files changed, 151 insertions(+), 83 deletions(-)
Hi Laurent On 12/1/25 3:52 AM, Laurent Pinchart wrote: > > > Hi Antoine, > > Thank you for the series. > > On Thu, Nov 27, 2025 at 04:45:16PM +0100, Antoine Bouyer wrote: >> Submit this imx8-isi rework on behalf of Andrei. This series is about >> using libcamera MediaPipeline class to simplify imx8-isi pipeline >> configuration. >> >> Instead of going over each pipeline subdevices during imx8-isi match(), >> all is handled by MediaPipeline class. It helps supporting complex >> topologies, where subdevice(s) could be optional, typically on i.MX95 >> SoC which has a formatter, while other i.MX SoCs don't have it. > > I'm sorry but I don't think this goes in the right direction. The > MediaPipeline class has been designed to handle linear chains of > entities for cases where we don't know what those entities are like in > the simple pipeline handler, or for entities external to the SoC (such > as serializers and deserializers - and in this case it's even arguably a > bit of a hack until we develop explicit support for serdes and external > ISP). > > Entities within the SoC should be handled explicitly in the pipeline > handler, especially when they need device-specific configuration such as > for the crossbar switch. The formatter of the i.MX95 should also be > handled explicitly. Ok noticed. Actually that is purpose of this series too: MediaPipeline handles all entites from sensor up to crossbar. Crossbar entity is still managed by imx8-isi pipeline handler, so as crossbar routing. Indeed in the series, the MediaPipeline also handles some entities within the SoC, such as formatter, csi. But same is done in rkisp1 pipeline as per my understanding, so we did the same in imx8-isi. Best regards Antoine > > Could you please rework the pipeline handler in that direction ? It can > start with a revert of patch 2/2 if that's a simpler (to develop and/or > review) solution. > >> It reuses the simple pipeline's locateSensors method, so external ISP are >> also supported then. ISP is considered as the 'sensor' element. >> >> Tested on i.MX8MP SoC. >> >> --- >> Changes in v3: >> - Apply review comments from Jacopo: Add \brief tag and remove '.' at the >> end of variable descriptions; rephrase MediaPipeline::Entity description. >> - Use the shared_ptr get() accessor to retrieve the isiDev_ MediaDevice >> because this was updated in all pipelines after v2. >> - link to v2: https://patchwork.libcamera.org/cover/25072/ >> >> Changes in v2: >> - Add missing documentation as suggested by Barnabás >> - Move Entity parameters documentation from .h to .cpp file. >> - Replace 'video' by 'last' node in source descriptions, because in >> imx8-isi pipeline case, the last MediaPipeline entity (i.e. crossbar) is >> not a video node. >> - Apply review comments from Jacopo: move Entity definition to beginning >> of public section; move entities() to const-callable as well. >> - link to v1: https://patchwork.libcamera.org/patch/25009/ >> >> --- >> Andrei Gansari (2): >> libamera: media_pipeline: Add accessor for MediaPipeline list of >> entities >> pipeline: imx8-isi: Integrating MediaPipeline class >> >> include/libcamera/internal/media_pipeline.h | 29 +--- >> src/libcamera/media_pipeline.cpp | 46 ++++++ >> src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 159 ++++++++++++------- >> 3 files changed, 151 insertions(+), 83 deletions(-) > > -- > Regards, > > Laurent Pinchart
Submit this imx8-isi rework on behalf of Andrei. This series is about using libcamera MediaPipeline class to simplify imx8-isi pipeline configuration. Instead of going over each pipeline subdevices during imx8-isi match(), all is handled by MediaPipeline class. It helps supporting complex topologies, where subdevice(s) could be optional, typically on i.MX95 SoC which has a formatter, while other i.MX SoCs don't have it. It reuses the simple pipeline's locateSensors method, so external ISP are also supported then. ISP is considered as the 'sensor' element. Tested on i.MX8MP SoC. --- Changes in v3: - Apply review comments from Jacopo: Add \brief tag and remove '.' at the end of variable descriptions; rephrase MediaPipeline::Entity description. - Use the shared_ptr get() accessor to retrieve the isiDev_ MediaDevice because this was updated in all pipelines after v2. - link to v2: https://patchwork.libcamera.org/cover/25072/ Changes in v2: - Add missing documentation as suggested by Barnabás - Move Entity parameters documentation from .h to .cpp file. - Replace 'video' by 'last' node in source descriptions, because in imx8-isi pipeline case, the last MediaPipeline entity (i.e. crossbar) is not a video node. - Apply review comments from Jacopo: move Entity definition to beginning of public section; move entities() to const-callable as well. - link to v1: https://patchwork.libcamera.org/patch/25009/ --- Andrei Gansari (2): libamera: media_pipeline: Add accessor for MediaPipeline list of entities pipeline: imx8-isi: Integrating MediaPipeline class include/libcamera/internal/media_pipeline.h | 29 +--- src/libcamera/media_pipeline.cpp | 46 ++++++ src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 159 ++++++++++++------- 3 files changed, 151 insertions(+), 83 deletions(-)