| Message ID | 20260216203034.27558-1-mzamazal@redhat.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2026-02-16 20:30:19) > This patch series implements parameters and statistics buffer sharing in > software ISP. This is a rebase of an old patch series, adjusted for the > current master and GPU ISP. It implements the following software ISP > TODOs: > > - 2. Reconsider stats sharing > - 5. Store ISP parameters in per-frame buffers > > The first part of the patches deals with parameters buffers, the next > part with statistics buffers. > > It’s a way to make params correspond to the respective frames, which is > currently not the case with software ISP. But with the plans to compute > stats on a GPU and make everything synchronous, the only useful patches > from this series may eventually be those dropping the software ISP > TODOs. Which means I don’t plan to work on the patches further unless > we decide they are needed. I know it's only RFC, but just reporting the CI pinged me on some compile failures in here. Just trivials by the look of it, an extra semi-colon causing a warning. https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1606180 > > Changes in v2: > - Rebase on master and make it working with GPU ISP. > > Milan Zamazal (14): > libcamera: software_isp: Remove initializer_list include > libcamera: software_isp: Introduce arguments for parameters buffers > libcamera: software_isp: Separate allocation of the parameters buffer > libcamera: software_isp: Track unused parameters buffers > libcamera: software_isp: Allocation of multiple params buffers > libcamera: software_isp: Allocate multiple parameters buffers > libcamera: software_isp: Use multiple parameters buffers in IPA > libcamera: software_isp: Share parameters buffers with debayering > libcamera: software_isp: Remove per-frame params buffers TODO item > libcamera: software_isp: Introduce arguments for statistics buffers > libcamera: software_isp: Allocate statistics buffers > libcamera: software_isp: Track statistics buffers > libcamera: software_isp: Share statistics buffers with IPA > libcamera: software_isp: Remove stats-sharing TODO item > > .../internal/software_isp/software_isp.h | 21 ++- > .../internal/software_isp/swstats_cpu.h | 21 +-- > include/libcamera/ipa/soft.mojom | 11 +- > src/ipa/simple/soft_simple.cpp | 94 +++++++------ > src/libcamera/pipeline/simple/simple.cpp | 12 +- > src/libcamera/software_isp/TODO | 45 ------- > src/libcamera/software_isp/debayer.cpp | 42 ++++-- > src/libcamera/software_isp/debayer.h | 12 +- > src/libcamera/software_isp/debayer_cpu.cpp | 79 ++++++----- > src/libcamera/software_isp/debayer_cpu.h | 16 ++- > src/libcamera/software_isp/debayer_egl.cpp | 18 ++- > src/libcamera/software_isp/debayer_egl.h | 11 +- > src/libcamera/software_isp/software_isp.cpp | 126 +++++++++++++++--- > src/libcamera/software_isp/swstats_cpu.cpp | 66 ++++----- > 14 files changed, 336 insertions(+), 238 deletions(-) > > -- > 2.53.0 >
2026. 02. 16. 21:30 keltezéssel, Milan Zamazal írta: > This patch series implements parameters and statistics buffer sharing in > software ISP. This is a rebase of an old patch series, adjusted for the > current master and GPU ISP. It implements the following software ISP > TODOs: > > - 2. Reconsider stats sharing > - 5. Store ISP parameters in per-frame buffers > > The first part of the patches deals with parameters buffers, the next > part with statistics buffers. > > It’s a way to make params correspond to the respective frames, which is > currently not the case with software ISP. But with the plans to compute > stats on a GPU and make everything synchronous, the only useful patches I think I'm missing something because as far as I can see splitting the statistics collection would just introduce more async: 1. raw buffer arrives -> queue statistics calculation (async) 2. statistics arrive -> queue parameter computations (asnyc) 3. parameters arrive -> queue frame processing with parameters (async) 4. frame arrives -> signal completion at least assuming you want to use the statistics of the corresponding frame. > from this series may eventually be those dropping the software ISP > TODOs. Which means I don’t plan to work on the patches further unless > we decide they are needed. > > Changes in v2: > - Rebase on master and make it working with GPU ISP. > > Milan Zamazal (14): > libcamera: software_isp: Remove initializer_list include > libcamera: software_isp: Introduce arguments for parameters buffers > libcamera: software_isp: Separate allocation of the parameters buffer > libcamera: software_isp: Track unused parameters buffers > libcamera: software_isp: Allocation of multiple params buffers > libcamera: software_isp: Allocate multiple parameters buffers > libcamera: software_isp: Use multiple parameters buffers in IPA > libcamera: software_isp: Share parameters buffers with debayering > libcamera: software_isp: Remove per-frame params buffers TODO item > libcamera: software_isp: Introduce arguments for statistics buffers > libcamera: software_isp: Allocate statistics buffers > libcamera: software_isp: Track statistics buffers > libcamera: software_isp: Share statistics buffers with IPA > libcamera: software_isp: Remove stats-sharing TODO item > > .../internal/software_isp/software_isp.h | 21 ++- > .../internal/software_isp/swstats_cpu.h | 21 +-- > include/libcamera/ipa/soft.mojom | 11 +- > src/ipa/simple/soft_simple.cpp | 94 +++++++------ > src/libcamera/pipeline/simple/simple.cpp | 12 +- > src/libcamera/software_isp/TODO | 45 ------- > src/libcamera/software_isp/debayer.cpp | 42 ++++-- > src/libcamera/software_isp/debayer.h | 12 +- > src/libcamera/software_isp/debayer_cpu.cpp | 79 ++++++----- > src/libcamera/software_isp/debayer_cpu.h | 16 ++- > src/libcamera/software_isp/debayer_egl.cpp | 18 ++- > src/libcamera/software_isp/debayer_egl.h | 11 +- > src/libcamera/software_isp/software_isp.cpp | 126 +++++++++++++++--- > src/libcamera/software_isp/swstats_cpu.cpp | 66 ++++----- > 14 files changed, 336 insertions(+), 238 deletions(-) >
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2026-02-16 20:30:19) >> This patch series implements parameters and statistics buffer sharing in >> software ISP. This is a rebase of an old patch series, adjusted for the > >> current master and GPU ISP. It implements the following software ISP >> TODOs: >> >> - 2. Reconsider stats sharing >> - 5. Store ISP parameters in per-frame buffers >> >> The first part of the patches deals with parameters buffers, the next >> part with statistics buffers. >> >> It’s a way to make params correspond to the respective frames, which is >> currently not the case with software ISP. But with the plans to compute >> stats on a GPU and make everything synchronous, the only useful patches >> from this series may eventually be those dropping the software ISP >> TODOs. Which means I don’t plan to work on the patches further unless >> we decide they are needed. > > I know it's only RFC, but just reporting the CI pinged me on some > compile failures in here. Just trivials by the look of it, an extra > semi-colon causing a warning. > > https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1606180 Ah, right. Apparently clang only, I should check it next time. >> Changes in v2: >> - Rebase on master and make it working with GPU ISP. >> >> Milan Zamazal (14): >> libcamera: software_isp: Remove initializer_list include >> libcamera: software_isp: Introduce arguments for parameters buffers >> libcamera: software_isp: Separate allocation of the parameters buffer >> libcamera: software_isp: Track unused parameters buffers >> libcamera: software_isp: Allocation of multiple params buffers >> libcamera: software_isp: Allocate multiple parameters buffers >> libcamera: software_isp: Use multiple parameters buffers in IPA >> libcamera: software_isp: Share parameters buffers with debayering >> libcamera: software_isp: Remove per-frame params buffers TODO item >> libcamera: software_isp: Introduce arguments for statistics buffers >> libcamera: software_isp: Allocate statistics buffers >> libcamera: software_isp: Track statistics buffers >> libcamera: software_isp: Share statistics buffers with IPA >> libcamera: software_isp: Remove stats-sharing TODO item >> >> .../internal/software_isp/software_isp.h | 21 ++- >> .../internal/software_isp/swstats_cpu.h | 21 +-- >> include/libcamera/ipa/soft.mojom | 11 +- >> src/ipa/simple/soft_simple.cpp | 94 +++++++------ >> src/libcamera/pipeline/simple/simple.cpp | 12 +- >> src/libcamera/software_isp/TODO | 45 ------- >> src/libcamera/software_isp/debayer.cpp | 42 ++++-- >> src/libcamera/software_isp/debayer.h | 12 +- >> src/libcamera/software_isp/debayer_cpu.cpp | 79 ++++++----- >> src/libcamera/software_isp/debayer_cpu.h | 16 ++- >> src/libcamera/software_isp/debayer_egl.cpp | 18 ++- >> src/libcamera/software_isp/debayer_egl.h | 11 +- >> src/libcamera/software_isp/software_isp.cpp | 126 +++++++++++++++--- >> src/libcamera/software_isp/swstats_cpu.cpp | 66 ++++----- >> 14 files changed, 336 insertions(+), 238 deletions(-) >> >> -- >> 2.53.0 >>
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > 2026. 02. 16. 21:30 keltezéssel, Milan Zamazal írta: >> This patch series implements parameters and statistics buffer sharing in >> software ISP. This is a rebase of an old patch series, adjusted for the >> current master and GPU ISP. It implements the following software ISP >> TODOs: >> - 2. Reconsider stats sharing >> - 5. Store ISP parameters in per-frame buffers >> The first part of the patches deals with parameters buffers, the next >> part with statistics buffers. >> It’s a way to make params correspond to the respective frames, which is >> currently not the case with software ISP. But with the plans to compute >> stats on a GPU and make everything synchronous, the only useful patches > > I think I'm missing something because as far as I can see splitting the statistics > collection would just introduce more async: > > 1. raw buffer arrives -> queue statistics calculation (async) > 2. statistics arrive -> queue parameter computations (asnyc) > 3. parameters arrive -> queue frame processing with parameters (async) > 4. frame arrives -> signal completion > > at least assuming you want to use the statistics of the corresponding frame. Async is fine (or even useful) as long as we use the right data together. The idea is to replace the current mechanism of "use whatever is available at the moment" with buffer/frame id tracking. (I'm sure this doesn't work correctly at the moment as I just rebased the very old series, adjusted it for GPU ISP some way, made it to compile (with gcc), and checked it runs. For example, I can see there is still, unused, debayerParams_ around. I believe there are other bugs. Doing more ATM would be waste of time unless we decide we need this. Hence RFC.) >> from this series may eventually be those dropping the software ISP >> TODOs. Which means I don’t plan to work on the patches further unless >> we decide they are needed. >> Changes in v2: >> - Rebase on master and make it working with GPU ISP. >> Milan Zamazal (14): >> libcamera: software_isp: Remove initializer_list include >> libcamera: software_isp: Introduce arguments for parameters buffers >> libcamera: software_isp: Separate allocation of the parameters buffer >> libcamera: software_isp: Track unused parameters buffers >> libcamera: software_isp: Allocation of multiple params buffers >> libcamera: software_isp: Allocate multiple parameters buffers >> libcamera: software_isp: Use multiple parameters buffers in IPA >> libcamera: software_isp: Share parameters buffers with debayering >> libcamera: software_isp: Remove per-frame params buffers TODO item >> libcamera: software_isp: Introduce arguments for statistics buffers >> libcamera: software_isp: Allocate statistics buffers >> libcamera: software_isp: Track statistics buffers >> libcamera: software_isp: Share statistics buffers with IPA >> libcamera: software_isp: Remove stats-sharing TODO item >> .../internal/software_isp/software_isp.h | 21 ++- >> .../internal/software_isp/swstats_cpu.h | 21 +-- >> include/libcamera/ipa/soft.mojom | 11 +- >> src/ipa/simple/soft_simple.cpp | 94 +++++++------ >> src/libcamera/pipeline/simple/simple.cpp | 12 +- >> src/libcamera/software_isp/TODO | 45 ------- >> src/libcamera/software_isp/debayer.cpp | 42 ++++-- >> src/libcamera/software_isp/debayer.h | 12 +- >> src/libcamera/software_isp/debayer_cpu.cpp | 79 ++++++----- >> src/libcamera/software_isp/debayer_cpu.h | 16 ++- >> src/libcamera/software_isp/debayer_egl.cpp | 18 ++- >> src/libcamera/software_isp/debayer_egl.h | 11 +- >> src/libcamera/software_isp/software_isp.cpp | 126 +++++++++++++++--- >> src/libcamera/software_isp/swstats_cpu.cpp | 66 ++++----- >> 14 files changed, 336 insertions(+), 238 deletions(-) >>
Hi Milan, On 16-Feb-26 9:30 PM, Milan Zamazal wrote: > This patch series implements parameters and statistics buffer sharing in > software ISP. This is a rebase of an old patch series, adjusted for the > current master and GPU ISP. It implements the following software ISP > TODOs: > > - 2. Reconsider stats sharing > - 5. Store ISP parameters in per-frame buffers > > The first part of the patches deals with parameters buffers, the next > part with statistics buffers. > > It’s a way to make params correspond to the respective frames, which is > currently not the case with software ISP. But with the plans to compute > stats on a GPU and make everything synchronous, the only useful patches > from this series may eventually be those dropping the software ISP > TODOs. Which means I don’t plan to work on the patches further unless > we decide they are needed. > > Changes in v2: > - Rebase on master and make it working with GPU ISP. Thank you for your series and sorry for being very slow to respond to this. I agree that we should move in this direction both to fix potential races with the currently shared single stat and params buffers as well as to make the software ISP behave more like real hw ISPs making things more consistent. Regards, Hans > > Milan Zamazal (14): > libcamera: software_isp: Remove initializer_list include > libcamera: software_isp: Introduce arguments for parameters buffers > libcamera: software_isp: Separate allocation of the parameters buffer > libcamera: software_isp: Track unused parameters buffers > libcamera: software_isp: Allocation of multiple params buffers > libcamera: software_isp: Allocate multiple parameters buffers > libcamera: software_isp: Use multiple parameters buffers in IPA > libcamera: software_isp: Share parameters buffers with debayering > libcamera: software_isp: Remove per-frame params buffers TODO item > libcamera: software_isp: Introduce arguments for statistics buffers > libcamera: software_isp: Allocate statistics buffers > libcamera: software_isp: Track statistics buffers > libcamera: software_isp: Share statistics buffers with IPA > libcamera: software_isp: Remove stats-sharing TODO item > > .../internal/software_isp/software_isp.h | 21 ++- > .../internal/software_isp/swstats_cpu.h | 21 +-- > include/libcamera/ipa/soft.mojom | 11 +- > src/ipa/simple/soft_simple.cpp | 94 +++++++------ > src/libcamera/pipeline/simple/simple.cpp | 12 +- > src/libcamera/software_isp/TODO | 45 ------- > src/libcamera/software_isp/debayer.cpp | 42 ++++-- > src/libcamera/software_isp/debayer.h | 12 +- > src/libcamera/software_isp/debayer_cpu.cpp | 79 ++++++----- > src/libcamera/software_isp/debayer_cpu.h | 16 ++- > src/libcamera/software_isp/debayer_egl.cpp | 18 ++- > src/libcamera/software_isp/debayer_egl.h | 11 +- > src/libcamera/software_isp/software_isp.cpp | 126 +++++++++++++++--- > src/libcamera/software_isp/swstats_cpu.cpp | 66 ++++----- > 14 files changed, 336 insertions(+), 238 deletions(-) >
On Thu, Apr 30, 2026 at 03:53:01PM +0200, johannes.goede@oss.qualcomm.com wrote: > On 16-Feb-26 9:30 PM, Milan Zamazal wrote: > > This patch series implements parameters and statistics buffer sharing in > > software ISP. This is a rebase of an old patch series, adjusted for the > > current master and GPU ISP. It implements the following software ISP > > TODOs: > > > > - 2. Reconsider stats sharing > > - 5. Store ISP parameters in per-frame buffers > > > > The first part of the patches deals with parameters buffers, the next > > part with statistics buffers. > > > > It’s a way to make params correspond to the respective frames, which is > > currently not the case with software ISP. But with the plans to compute > > stats on a GPU and make everything synchronous, the only useful patches > > from this series may eventually be those dropping the software ISP > > TODOs. Which means I don’t plan to work on the patches further unless > > we decide they are needed. > > > > Changes in v2: > > - Rebase on master and make it working with GPU ISP. > > Thank you for your series and sorry for being very slow to respond > to this. > > I agree that we should move in this direction both to fix potential > races with the currently shared single stat and params buffers as well > as to make the software ISP behave more like real hw ISPs making > things more consistent. Ack, I also think this does in the right direction. If the series introduces delays (as pointed out by Barnabás), I think we can then optimize performance on top, while keeping the base mechanism of buffer sharing and per-frame stats and params buffers. > > Milan Zamazal (14): > > libcamera: software_isp: Remove initializer_list include > > libcamera: software_isp: Introduce arguments for parameters buffers > > libcamera: software_isp: Separate allocation of the parameters buffer > > libcamera: software_isp: Track unused parameters buffers > > libcamera: software_isp: Allocation of multiple params buffers > > libcamera: software_isp: Allocate multiple parameters buffers > > libcamera: software_isp: Use multiple parameters buffers in IPA > > libcamera: software_isp: Share parameters buffers with debayering > > libcamera: software_isp: Remove per-frame params buffers TODO item > > libcamera: software_isp: Introduce arguments for statistics buffers > > libcamera: software_isp: Allocate statistics buffers > > libcamera: software_isp: Track statistics buffers > > libcamera: software_isp: Share statistics buffers with IPA > > libcamera: software_isp: Remove stats-sharing TODO item > > > > .../internal/software_isp/software_isp.h | 21 ++- > > .../internal/software_isp/swstats_cpu.h | 21 +-- > > include/libcamera/ipa/soft.mojom | 11 +- > > src/ipa/simple/soft_simple.cpp | 94 +++++++------ > > src/libcamera/pipeline/simple/simple.cpp | 12 +- > > src/libcamera/software_isp/TODO | 45 ------- > > src/libcamera/software_isp/debayer.cpp | 42 ++++-- > > src/libcamera/software_isp/debayer.h | 12 +- > > src/libcamera/software_isp/debayer_cpu.cpp | 79 ++++++----- > > src/libcamera/software_isp/debayer_cpu.h | 16 ++- > > src/libcamera/software_isp/debayer_egl.cpp | 18 ++- > > src/libcamera/software_isp/debayer_egl.h | 11 +- > > src/libcamera/software_isp/software_isp.cpp | 126 +++++++++++++++--- > > src/libcamera/software_isp/swstats_cpu.cpp | 66 ++++----- > > 14 files changed, 336 insertions(+), 238 deletions(-)