| Message ID | 20251015012251.17508-36-bryan.odonoghue@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
While I love to see this, I think we should have at least one release
with the swisp staying the default in order to avoid fallout - in
particular:
1. There is no Pipewire release with the necessary sandbox change yet
(in a month or two we could have 1.6 and 1.4.10)
2. With the current series I observe at least one crash specific to the
gpuisp (hitting the assert in eGL::cleanUp(void)) and I assume it
won't be the only one when getting more testing, even if I manage to
debug this in the next couple of days.
Being able to work those out without getting issues from users or having
to carry downstream patches would be nice :)
On 10/15/25 03:22, Bryan O'Donoghue wrote:
> In some cases the GPU can deliver 15x performance in Debayer with the
> CCM on, reference hardware Qualcomm RB5 with IMX512 sensor.
>
> Given this large performance difference it makes sense to make GPUISP
> the default for the Software ISP.
>
> If LIBCAMERA_SOFTISP_MODE is omitted gpu will be the default. If
> libcamera is compiled without gpuisp support, CPU Debayer will be used.
>
> It is still possible to select CPU mode with LIBCAMERA_SOFISP_MODE=cpu.
>
> Signed-off-by: Bryan O'Donoghue<bryan.odonoghue@linaro.org>
> ---
> src/libcamera/software_isp/software_isp.cpp | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 869f7320..1b4a29fd 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -120,10 +120,17 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,
> }
> stats->statsReady.connect(this, &SoftwareIsp::statsReady);
>
> -#if HAVE_DEBAYER_EGL
> const char *softISPMode = utils::secure_getenv("LIBCAMERA_SOFTISP_MODE");
> + if (softISPMode) {
> + if (strcmp(softISPMode, "gpu") && strcmp(softISPMode, "cpu")) {
> + LOG(SoftwareIsp, Error) << "LIBCAMERA_SOFISP_MODE " << softISPMode << " invalid. "
> + << "must be \"cpu\" or \"gpu\"";
> + return;
> + }
> + }
>
> - if (softISPMode && !strcmp(softISPMode, "gpu"))
> +#if HAVE_DEBAYER_EGL
> + if (!softISPMode || !strcmp(softISPMode, "gpu"))
> debayer_ = std::make_unique<DebayerEGL>(std::move(stats), configuration);
> #endif
> if (!debayer_)
On 10/16/25 11:06, Robert Mader wrote: > > While I love to see this, I think we should have at least one release > with the swisp staying the default in order to avoid fallout - in > particular: > > 1. There is no Pipewire release with the necessary sandbox change yet > (in a month or two we could have 1.6 and 1.4.10) > Sorry, correction: looks like the patch *did* already get backported already in 1.4.8. so this shouldn't be an issue. > > 1. With the current series I observe at least one crash specific to > the gpuisp (hitting the assert in eGL::cleanUp(void)) and I assume > it won't be the only one when getting more testing, even if I > manage to debug this in the next couple of days. > > Being able to work those out without getting issues from users or > having to carry downstream patches would be nice :) > > On 10/15/25 03:22, Bryan O'Donoghue wrote: >> In some cases the GPU can deliver 15x performance in Debayer with the >> CCM on, reference hardware Qualcomm RB5 with IMX512 sensor. >> >> Given this large performance difference it makes sense to make GPUISP >> the default for the Software ISP. >> >> If LIBCAMERA_SOFTISP_MODE is omitted gpu will be the default. If >> libcamera is compiled without gpuisp support, CPU Debayer will be used. >> >> It is still possible to select CPU mode with LIBCAMERA_SOFISP_MODE=cpu. >> >> Signed-off-by: Bryan O'Donoghue<bryan.odonoghue@linaro.org> >> --- >> src/libcamera/software_isp/software_isp.cpp | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >> index 869f7320..1b4a29fd 100644 >> --- a/src/libcamera/software_isp/software_isp.cpp >> +++ b/src/libcamera/software_isp/software_isp.cpp >> @@ -120,10 +120,17 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, >> } >> stats->statsReady.connect(this, &SoftwareIsp::statsReady); >> >> -#if HAVE_DEBAYER_EGL >> const char *softISPMode = utils::secure_getenv("LIBCAMERA_SOFTISP_MODE"); >> + if (softISPMode) { >> + if (strcmp(softISPMode, "gpu") && strcmp(softISPMode, "cpu")) { >> + LOG(SoftwareIsp, Error) << "LIBCAMERA_SOFISP_MODE " << softISPMode << " invalid. " >> + << "must be \"cpu\" or \"gpu\""; >> + return; >> + } >> + } >> >> - if (softISPMode && !strcmp(softISPMode, "gpu")) >> +#if HAVE_DEBAYER_EGL >> + if (!softISPMode || !strcmp(softISPMode, "gpu")) >> debayer_ = std::make_unique<DebayerEGL>(std::move(stats), configuration); >> #endif >> if (!debayer_) > -- > Robert Mader > Consultant Software Developer > > Collabora Ltd. > Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK > Registered in England & Wales, no. 5513718
Ok, sorry for the noise. With the camera restart issue fixed, I quickly tested it on all devices I have - PinePhone, Librem5, Pixel3a and OnePlus 6 - and the GPU-ISP seems to perform better or at least not regress across the board. So it looks like good enough to ship on PostmarketOS edge. On 16.10.25 11:21, Robert Mader wrote: > On 10/16/25 11:06, Robert Mader wrote: >> >> While I love to see this, I think we should have at least one release >> with the swisp staying the default in order to avoid fallout - in >> particular: >> >> 1. There is no Pipewire release with the necessary sandbox change >> yet (in a month or two we could have 1.6 and 1.4.10) >> > Sorry, correction: looks like the patch *did* already get backported > already in 1.4.8. so this shouldn't be an issue. >> >> 1. With the current series I observe at least one crash specific to >> the gpuisp (hitting the assert in eGL::cleanUp(void)) and I >> assume it won't be the only one when getting more testing, even >> if I manage to debug this in the next couple of days. >> >> Being able to work those out without getting issues from users or >> having to carry downstream patches would be nice :) >> >> On 10/15/25 03:22, Bryan O'Donoghue wrote: >>> In some cases the GPU can deliver 15x performance in Debayer with the >>> CCM on, reference hardware Qualcomm RB5 with IMX512 sensor. >>> >>> Given this large performance difference it makes sense to make GPUISP >>> the default for the Software ISP. >>> >>> If LIBCAMERA_SOFTISP_MODE is omitted gpu will be the default. If >>> libcamera is compiled without gpuisp support, CPU Debayer will be used. >>> >>> It is still possible to select CPU mode with LIBCAMERA_SOFISP_MODE=cpu. >>> >>> Signed-off-by: Bryan O'Donoghue<bryan.odonoghue@linaro.org> >>> --- >>> src/libcamera/software_isp/software_isp.cpp | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >>> index 869f7320..1b4a29fd 100644 >>> --- a/src/libcamera/software_isp/software_isp.cpp >>> +++ b/src/libcamera/software_isp/software_isp.cpp >>> @@ -120,10 +120,17 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, >>> } >>> stats->statsReady.connect(this, &SoftwareIsp::statsReady); >>> >>> -#if HAVE_DEBAYER_EGL >>> const char *softISPMode = utils::secure_getenv("LIBCAMERA_SOFTISP_MODE"); >>> + if (softISPMode) { >>> + if (strcmp(softISPMode, "gpu") && strcmp(softISPMode, "cpu")) { >>> + LOG(SoftwareIsp, Error) << "LIBCAMERA_SOFISP_MODE " << softISPMode << " invalid. " >>> + << "must be \"cpu\" or \"gpu\""; >>> + return; >>> + } >>> + } >>> >>> - if (softISPMode && !strcmp(softISPMode, "gpu")) >>> +#if HAVE_DEBAYER_EGL >>> + if (!softISPMode || !strcmp(softISPMode, "gpu")) >>> debayer_ = std::make_unique<DebayerEGL>(std::move(stats), configuration); >>> #endif >>> if (!debayer_) >> -- >> Robert Mader >> Consultant Software Developer >> >> Collabora Ltd. >> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK >> Registered in England & Wales, no. 5513718 > -- > Robert Mader > Consultant Software Developer > > Collabora Ltd. > Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK > Registered in England & Wales, no. 5513718
diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index 869f7320..1b4a29fd 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -120,10 +120,17 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, } stats->statsReady.connect(this, &SoftwareIsp::statsReady); -#if HAVE_DEBAYER_EGL const char *softISPMode = utils::secure_getenv("LIBCAMERA_SOFTISP_MODE"); + if (softISPMode) { + if (strcmp(softISPMode, "gpu") && strcmp(softISPMode, "cpu")) { + LOG(SoftwareIsp, Error) << "LIBCAMERA_SOFISP_MODE " << softISPMode << " invalid. " + << "must be \"cpu\" or \"gpu\""; + return; + } + } - if (softISPMode && !strcmp(softISPMode, "gpu")) +#if HAVE_DEBAYER_EGL + if (!softISPMode || !strcmp(softISPMode, "gpu")) debayer_ = std::make_unique<DebayerEGL>(std::move(stats), configuration); #endif if (!debayer_)
In some cases the GPU can deliver 15x performance in Debayer with the CCM on, reference hardware Qualcomm RB5 with IMX512 sensor. Given this large performance difference it makes sense to make GPUISP the default for the Software ISP. If LIBCAMERA_SOFTISP_MODE is omitted gpu will be the default. If libcamera is compiled without gpuisp support, CPU Debayer will be used. It is still possible to select CPU mode with LIBCAMERA_SOFISP_MODE=cpu. Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- src/libcamera/software_isp/software_isp.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)