| Message ID | 20251219211006.104821-1-robert.mader@collabora.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Robert, Quoting Robert Mader (2025-12-20 02:40:06) > Since commit "libcamera: simple: Make raw streams working" apps that rely > on raw streams - such as Millipixels - can work with the SoftISP being > enabled. Thus let's enable the later by default. > > Tested on a Librem5. > > Signed-off-by: Robert Mader <robert.mader@collabora.com> Could you also append i.MX7 under the list of platforms supported by CPU-based ISP in Documentation/isp-feature-matrix.rst ? For this patch, Reviewed-by: Jai Luthra <jai.luthra@ideasonboard.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index b30b0a122..4a0b9f58d 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -257,7 +257,7 @@ namespace { > > static const SimplePipelineInfo supportedDevices[] = { > { "dcmipp", {}, false }, > - { "imx7-csi", { { "pxp", 1 } }, false }, > + { "imx7-csi", { { "pxp", 1 } }, true }, > { "intel-ipu6", {}, true }, > { "intel-ipu7", {}, true }, > { "j721e-csi2rx", {}, true }, > -- > 2.52.0 > Thanks, Jai
On Fri, Dec 19, 2025 at 10:10:06PM +0100, Robert Mader wrote: > Since commit "libcamera: simple: Make raw streams working" apps that rely > on raw streams - such as Millipixels - can work with the SoftISP being > enabled. Thus let's enable the later by default. > > Tested on a Librem5. This is an interesting one. I assume the librem5 has enough CPU (and possibly GPU, once Bryan series lands) power to support this. The "imx7-csi" driver name is however also used by i.MX6UL, i.MX7S, i.MX7D and i.MX8MM. Do we really want to enable the soft ISP on all those platforms ? > Signed-off-by: Robert Mader <robert.mader@collabora.com> > --- > src/libcamera/pipeline/simple/simple.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index b30b0a122..4a0b9f58d 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -257,7 +257,7 @@ namespace { > > static const SimplePipelineInfo supportedDevices[] = { > { "dcmipp", {}, false }, > - { "imx7-csi", { { "pxp", 1 } }, false }, > + { "imx7-csi", { { "pxp", 1 } }, true }, > { "intel-ipu6", {}, true }, > { "intel-ipu7", {}, true }, > { "j721e-csi2rx", {}, true },
On 23.12.25 10:55, Laurent Pinchart wrote: > On Fri, Dec 19, 2025 at 10:10:06PM +0100, Robert Mader wrote: >> Since commit "libcamera: simple: Make raw streams working" apps that rely >> on raw streams - such as Millipixels - can work with the SoftISP being >> enabled. Thus let's enable the later by default. >> >> Tested on a Librem5. > This is an interesting one. I assume the librem5 has enough CPU (and > possibly GPU, once Bryan series lands) power to support this. The > "imx7-csi" driver name is however also used by i.MX6UL, i.MX7S, i.MX7D > and i.MX8MM. Do we really want to enable the soft ISP on all those > platforms ? Indeed, I suppose it raises the broader question of: do we need to protect users from using the soft ISP? Some platforms that are too slow to sustain high enough framerates for a viewfinder/video might still benefit from the possibility to take single pictures, IMO making it worth enable the feature. We might go as far as unconditionally enabling it for all platform - as long as it can be turned of at build or runtime. Do you see a reason / a scenario where this could have negative impact? >> Signed-off-by: Robert Mader<robert.mader@collabora.com> >> --- >> src/libcamera/pipeline/simple/simple.cpp | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index b30b0a122..4a0b9f58d 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -257,7 +257,7 @@ namespace { >> >> static const SimplePipelineInfo supportedDevices[] = { >> { "dcmipp", {}, false }, >> - { "imx7-csi", { { "pxp", 1 } }, false }, >> + { "imx7-csi", { { "pxp", 1 } }, true }, >> { "intel-ipu6", {}, true }, >> { "intel-ipu7", {}, true }, >> { "j721e-csi2rx", {}, true },
On 23/12/2025 10:23, Robert Mader wrote: > Indeed, I suppose it raises the broader question of: do we need to > protect users from using the soft ISP? Environment variable. The question is I think - should it be on by default for i.MX6/7/8 or off ? It'd be nice to think LLVM pipeline could perform some magic with eGL that somehow makes this usable where the CPU is not. Worth a test to confirm this is no so :) I have an i.MX7 Solo board with upstream CSI2 support and a sensor, I could even test this eventually myself. --- bod
Bryan O'Donoghue <bod.linux@nxsw.ie> writes: > On 23/12/2025 10:23, Robert Mader wrote: >> Indeed, I suppose it raises the broader question of: do we need to >> protect users from using the soft ISP? > > Environment variable. There is already a configuration file option for the purpose. > The question is I think - should it be on by default for i.MX6/7/8 or off ? If hardware ISP is supported on some of those platforms then I'd say it should be off by default and it should be documented how to enable it in case hardware ISP doesn't work. It can be overridden in the configuration on distros targeting specific boards.
On 02/01/2026 09:48, Milan Zamazal wrote: > Bryan O'Donoghue <bod.linux@nxsw.ie> writes: > >> On 23/12/2025 10:23, Robert Mader wrote: >>> Indeed, I suppose it raises the broader question of: do we need to >>> protect users from using the soft ISP? >> >> Environment variable. > > There is already a configuration file option for the purpose. > >> The question is I think - should it be on by default for i.MX6/7/8 or off ? > > If hardware ISP is supported on some of those platforms then I'd say it > should be off by default and it should be documented how to enable it in > case hardware ISP doesn't work. It can be overridden in the > configuration on distros targeting specific boards. > I definitely mean for the !HardISP case here. Or what I really mean is at least i.MX7 and similar hardware which doesn't have a GPU and does mesa via LLVM pipeline. Its an "interesting question" which would perform better, CPUISP or GPUISP/LLVM-pipeline but, I'd expect both to be tortuously slow on this arm32 single core running @ 400 MHz if memory serves. I don't think LLVM can magic away the basic facts of the hardware. So off by default on i.MX7 Solo at least makes sense to me.. --- bod
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index b30b0a122..4a0b9f58d 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -257,7 +257,7 @@ namespace { static const SimplePipelineInfo supportedDevices[] = { { "dcmipp", {}, false }, - { "imx7-csi", { { "pxp", 1 } }, false }, + { "imx7-csi", { { "pxp", 1 } }, true }, { "intel-ipu6", {}, true }, { "intel-ipu7", {}, true }, { "j721e-csi2rx", {}, true },
Since commit "libcamera: simple: Make raw streams working" apps that rely on raw streams - such as Millipixels - can work with the SoftISP being enabled. Thus let's enable the later by default. Tested on a Librem5. Signed-off-by: Robert Mader <robert.mader@collabora.com> --- src/libcamera/pipeline/simple/simple.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)