| Message ID | 20260405191443.1209948-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent On Sun, Apr 05, 2026 at 10:14:33PM +0300, Laurent Pinchart wrote: > To prepare for merging the two software ISP TODO lists (TODO and > gpuisp-todo.txt), convert the TODO file to markdown. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/software_isp/{TODO => TODO.md} | 50 ++++++++++++-------- > utils/tuning/config-example.yaml | 2 +- > 2 files changed, 32 insertions(+), 20 deletions(-) > rename src/libcamera/software_isp/{TODO => TODO.md} (91%) > > diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO.md > similarity index 91% > rename from src/libcamera/software_isp/TODO > rename to src/libcamera/software_isp/TODO.md > index f19e15ae27a0..bdd4a7421766 100644 > --- a/src/libcamera/software_isp/TODO > +++ b/src/libcamera/software_isp/TODO.md > @@ -1,5 +1,16 @@ > -2. Reconsider stats sharing > +# Software ISP TODO list > > +This file contains the TODO list for the software ISP. > + > +## Common code and CPU-based implementation > + > +The TODO items in this section gather comments from patch review that were not > +deemed to require being addressed right away. The text in block quotes is > +copied directly from e-mail review. > + > +### Reconsider stats sharing > + > +``` > >>> +void SwStatsCpu::finishFrame(void) > >>> +{ > >>> + *sharedStats_ = stats_; > @@ -22,11 +33,11 @@ > > That would match how we deal with hardware ISPs, and I think that's a > good idea. It will help decoupling the processing side from the IPA. > +``` > > ---- > - > -3. Remove statsReady signal > +### Remove statsReady signal > > +``` > > class SwStatsCpu > > { > > /** > @@ -50,11 +61,11 @@ Removing the signal and refactoring those classes doesn't have to be > addressed now, I think it would be part of a larger refactoring > (possibly also considering platforms that have no ISP but can produce > stats in hardware, such as the i.MX7), but please keep it on your radar. > +``` > > ---- > - > -5. Store ISP parameters in per-frame buffers > +### Store ISP parameters in per-frame buffers > > +``` > > /** > > * \fn void Debayer::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) > > * \brief Process the bayer data into the requested format. > @@ -68,11 +79,11 @@ stats in hardware, such as the i.MX7), but please keep it on your radar. > > Possibly something to address later, by storing ISP parameters in > per-frame buffers like we do for hardware ISPs. > +``` > > ---- > - > -8. DebayerCpu cleanups > +### DebayerCpu cleanups > > +``` > > >> class DebayerCpu : public Debayer, public Object > > >> const SharedFD &getStatsFD() { return stats_->getStatsFD(); } > > > > @@ -105,21 +116,21 @@ the need for performances and the need for a maintainable architecture. > > I think this falls under the lets wait until we have a GPU > > based SoftISP MVP/POC and then do some refactoring to see which > > bits should go where. > +``` > > ---- > - > -8. Decouple pipeline and IPA naming > +### Decouple pipeline and IPA naming > > +``` > > The current src/ipa/meson.build assumes the IPA name to match the > > pipeline name. For this reason "-Dipas=simple" is used for the > > Soft IPA module. > > This should be addressed. > +``` > > ---- > - > -9. Doxyfile cleanup > +### Doxyfile cleanup > > +``` > >> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in > >> index a86ea6c1..2be8d47b 100644 > >> --- a/Documentation/Doxyfile.in > @@ -160,13 +171,14 @@ Yes, because, well... all the other IPAs were doing that... > > > It doesn't have to be done before merging, but could you > > address this sooner than later ? > +``` > > ---- > - > -13. Improve black level and colour gains application > +### Improve black level and colour gains application > > +``` > I think the black level should eventually be moved before debayering, and > ideally the colour gains as well. I understand the need for optimizations to > lower the CPU consumption, but at the same time I don't feel comfortable > building up on top of an implementation that may work a bit more by chance than > by correctness, as that's not very maintainable. > +``` > diff --git a/utils/tuning/config-example.yaml b/utils/tuning/config-example.yaml > index 5593eaef809e..680f9213b269 100644 > --- a/utils/tuning/config-example.yaml > +++ b/utils/tuning/config-example.yaml > @@ -51,4 +51,4 @@ general: > macbeth: > small: 1 > show: 0 > -# blacklevel: 32 > \ No newline at end of file > +# blacklevel: 32 Intentional ? > -- > Regards, > > Laurent Pinchart >
On Tue, Apr 07, 2026 at 11:56:56AM +0200, Jacopo Mondi wrote: > On Sun, Apr 05, 2026 at 10:14:33PM +0300, Laurent Pinchart wrote: > > To prepare for merging the two software ISP TODO lists (TODO and > > gpuisp-todo.txt), convert the TODO file to markdown. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > src/libcamera/software_isp/{TODO => TODO.md} | 50 ++++++++++++-------- > > utils/tuning/config-example.yaml | 2 +- > > 2 files changed, 32 insertions(+), 20 deletions(-) > > rename src/libcamera/software_isp/{TODO => TODO.md} (91%) > > > > diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO.md > > similarity index 91% > > rename from src/libcamera/software_isp/TODO > > rename to src/libcamera/software_isp/TODO.md > > index f19e15ae27a0..bdd4a7421766 100644 > > --- a/src/libcamera/software_isp/TODO > > +++ b/src/libcamera/software_isp/TODO.md > > @@ -1,5 +1,16 @@ > > -2. Reconsider stats sharing > > +# Software ISP TODO list > > > > +This file contains the TODO list for the software ISP. > > + > > +## Common code and CPU-based implementation > > + > > +The TODO items in this section gather comments from patch review that were not > > +deemed to require being addressed right away. The text in block quotes is > > +copied directly from e-mail review. > > + > > +### Reconsider stats sharing > > + > > +``` > > >>> +void SwStatsCpu::finishFrame(void) > > >>> +{ > > >>> + *sharedStats_ = stats_; > > @@ -22,11 +33,11 @@ > > > > That would match how we deal with hardware ISPs, and I think that's a > > good idea. It will help decoupling the processing side from the IPA. > > +``` > > > > ---- > > - > > -3. Remove statsReady signal > > +### Remove statsReady signal > > > > +``` > > > class SwStatsCpu > > > { > > > /** > > @@ -50,11 +61,11 @@ Removing the signal and refactoring those classes doesn't have to be > > addressed now, I think it would be part of a larger refactoring > > (possibly also considering platforms that have no ISP but can produce > > stats in hardware, such as the i.MX7), but please keep it on your radar. > > +``` > > > > ---- > > - > > -5. Store ISP parameters in per-frame buffers > > +### Store ISP parameters in per-frame buffers > > > > +``` > > > /** > > > * \fn void Debayer::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) > > > * \brief Process the bayer data into the requested format. > > @@ -68,11 +79,11 @@ stats in hardware, such as the i.MX7), but please keep it on your radar. > > > > Possibly something to address later, by storing ISP parameters in > > per-frame buffers like we do for hardware ISPs. > > +``` > > > > ---- > > - > > -8. DebayerCpu cleanups > > +### DebayerCpu cleanups > > > > +``` > > > >> class DebayerCpu : public Debayer, public Object > > > >> const SharedFD &getStatsFD() { return stats_->getStatsFD(); } > > > > > > @@ -105,21 +116,21 @@ the need for performances and the need for a maintainable architecture. > > > I think this falls under the lets wait until we have a GPU > > > based SoftISP MVP/POC and then do some refactoring to see which > > > bits should go where. > > +``` > > > > ---- > > - > > -8. Decouple pipeline and IPA naming > > +### Decouple pipeline and IPA naming > > > > +``` > > > The current src/ipa/meson.build assumes the IPA name to match the > > > pipeline name. For this reason "-Dipas=simple" is used for the > > > Soft IPA module. > > > > This should be addressed. > > +``` > > > > ---- > > - > > -9. Doxyfile cleanup > > +### Doxyfile cleanup > > > > +``` > > >> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in > > >> index a86ea6c1..2be8d47b 100644 > > >> --- a/Documentation/Doxyfile.in > > @@ -160,13 +171,14 @@ Yes, because, well... all the other IPAs were doing that... > > > > > It doesn't have to be done before merging, but could you > > > address this sooner than later ? > > +``` > > > > ---- > > - > > -13. Improve black level and colour gains application > > +### Improve black level and colour gains application > > > > +``` > > I think the black level should eventually be moved before debayering, and > > ideally the colour gains as well. I understand the need for optimizations to > > lower the CPU consumption, but at the same time I don't feel comfortable > > building up on top of an implementation that may work a bit more by chance than > > by correctness, as that's not very maintainable. > > +``` > > diff --git a/utils/tuning/config-example.yaml b/utils/tuning/config-example.yaml > > index 5593eaef809e..680f9213b269 100644 > > --- a/utils/tuning/config-example.yaml > > +++ b/utils/tuning/config-example.yaml > > @@ -51,4 +51,4 @@ general: > > macbeth: > > small: 1 > > show: 0 > > -# blacklevel: 32 > > \ No newline at end of file > > +# blacklevel: 32 > > Intentional ? No, I'll drop that. Feel free to send a R-b for the patch already, so I won't have to resubmit just for this :-)
Hi Laurent On Tue, Apr 07, 2026 at 01:18:43PM +0300, Laurent Pinchart wrote: > On Tue, Apr 07, 2026 at 11:56:56AM +0200, Jacopo Mondi wrote: > > On Sun, Apr 05, 2026 at 10:14:33PM +0300, Laurent Pinchart wrote: > > > To prepare for merging the two software ISP TODO lists (TODO and > > > gpuisp-todo.txt), convert the TODO file to markdown. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > src/libcamera/software_isp/{TODO => TODO.md} | 50 ++++++++++++-------- > > > utils/tuning/config-example.yaml | 2 +- > > > 2 files changed, 32 insertions(+), 20 deletions(-) > > > rename src/libcamera/software_isp/{TODO => TODO.md} (91%) > > > > > > diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO.md > > > similarity index 91% > > > rename from src/libcamera/software_isp/TODO > > > rename to src/libcamera/software_isp/TODO.md > > > index f19e15ae27a0..bdd4a7421766 100644 > > > --- a/src/libcamera/software_isp/TODO > > > +++ b/src/libcamera/software_isp/TODO.md > > > @@ -1,5 +1,16 @@ > > > -2. Reconsider stats sharing > > > +# Software ISP TODO list > > > > > > +This file contains the TODO list for the software ISP. > > > + > > > +## Common code and CPU-based implementation > > > + > > > +The TODO items in this section gather comments from patch review that were not > > > +deemed to require being addressed right away. The text in block quotes is > > > +copied directly from e-mail review. > > > + > > > +### Reconsider stats sharing > > > + > > > +``` > > > >>> +void SwStatsCpu::finishFrame(void) > > > >>> +{ > > > >>> + *sharedStats_ = stats_; > > > @@ -22,11 +33,11 @@ > > > > > > That would match how we deal with hardware ISPs, and I think that's a > > > good idea. It will help decoupling the processing side from the IPA. > > > +``` > > > > > > ---- > > > - > > > -3. Remove statsReady signal > > > +### Remove statsReady signal > > > > > > +``` > > > > class SwStatsCpu > > > > { > > > > /** > > > @@ -50,11 +61,11 @@ Removing the signal and refactoring those classes doesn't have to be > > > addressed now, I think it would be part of a larger refactoring > > > (possibly also considering platforms that have no ISP but can produce > > > stats in hardware, such as the i.MX7), but please keep it on your radar. > > > +``` > > > > > > ---- > > > - > > > -5. Store ISP parameters in per-frame buffers > > > +### Store ISP parameters in per-frame buffers > > > > > > +``` > > > > /** > > > > * \fn void Debayer::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) > > > > * \brief Process the bayer data into the requested format. > > > @@ -68,11 +79,11 @@ stats in hardware, such as the i.MX7), but please keep it on your radar. > > > > > > Possibly something to address later, by storing ISP parameters in > > > per-frame buffers like we do for hardware ISPs. > > > +``` > > > > > > ---- > > > - > > > -8. DebayerCpu cleanups > > > +### DebayerCpu cleanups > > > > > > +``` > > > > >> class DebayerCpu : public Debayer, public Object > > > > >> const SharedFD &getStatsFD() { return stats_->getStatsFD(); } > > > > > > > > @@ -105,21 +116,21 @@ the need for performances and the need for a maintainable architecture. > > > > I think this falls under the lets wait until we have a GPU > > > > based SoftISP MVP/POC and then do some refactoring to see which > > > > bits should go where. > > > +``` > > > > > > ---- > > > - > > > -8. Decouple pipeline and IPA naming > > > +### Decouple pipeline and IPA naming > > > > > > +``` > > > > The current src/ipa/meson.build assumes the IPA name to match the > > > > pipeline name. For this reason "-Dipas=simple" is used for the > > > > Soft IPA module. > > > > > > This should be addressed. > > > +``` > > > > > > ---- > > > - > > > -9. Doxyfile cleanup > > > +### Doxyfile cleanup > > > > > > +``` > > > >> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in > > > >> index a86ea6c1..2be8d47b 100644 > > > >> --- a/Documentation/Doxyfile.in > > > @@ -160,13 +171,14 @@ Yes, because, well... all the other IPAs were doing that... > > > > > > > It doesn't have to be done before merging, but could you > > > > address this sooner than later ? > > > +``` > > > > > > ---- > > > - > > > -13. Improve black level and colour gains application > > > +### Improve black level and colour gains application > > > > > > +``` > > > I think the black level should eventually be moved before debayering, and > > > ideally the colour gains as well. I understand the need for optimizations to > > > lower the CPU consumption, but at the same time I don't feel comfortable > > > building up on top of an implementation that may work a bit more by chance than > > > by correctness, as that's not very maintainable. > > > +``` > > > diff --git a/utils/tuning/config-example.yaml b/utils/tuning/config-example.yaml > > > index 5593eaef809e..680f9213b269 100644 > > > --- a/utils/tuning/config-example.yaml > > > +++ b/utils/tuning/config-example.yaml > > > @@ -51,4 +51,4 @@ general: > > > macbeth: > > > small: 1 > > > show: 0 > > > -# blacklevel: 32 > > > \ No newline at end of file > > > +# blacklevel: 32 > > > > Intentional ? > > No, I'll drop that. Feel free to send a R-b for the patch already, so I > won't have to resubmit just for this :-) > Oh sure, sorry, I forgot Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > -- > Regards, > > Laurent Pinchart
2026. 04. 05. 21:14 keltezéssel, Laurent Pinchart írta: > To prepare for merging the two software ISP TODO lists (TODO and > gpuisp-todo.txt), convert the TODO file to markdown. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/software_isp/{TODO => TODO.md} | 50 ++++++++++++-------- > utils/tuning/config-example.yaml | 2 +- > 2 files changed, 32 insertions(+), 20 deletions(-) > rename src/libcamera/software_isp/{TODO => TODO.md} (91%) > > diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO.md > similarity index 91% > rename from src/libcamera/software_isp/TODO > rename to src/libcamera/software_isp/TODO.md > index f19e15ae27a0..bdd4a7421766 100644 > --- a/src/libcamera/software_isp/TODO > +++ b/src/libcamera/software_isp/TODO.md > @@ -1,5 +1,16 @@ > -2. Reconsider stats sharing > +# Software ISP TODO list > > +This file contains the TODO list for the software ISP. > + > +## Common code and CPU-based implementation > + > +The TODO items in this section gather comments from patch review that were not > +deemed to require being addressed right away. The text in block quotes is > +copied directly from e-mail review. > + > +### Reconsider stats sharing > + > +``` > >>> +void SwStatsCpu::finishFrame(void) > >>> +{ > >>> + *sharedStats_ = stats_; > @@ -22,11 +33,11 @@ > > That would match how we deal with hardware ISPs, and I think that's a > good idea. It will help decoupling the processing side from the IPA. > +``` > > ---- > - > -3. Remove statsReady signal > +### Remove statsReady signal > > +``` > > class SwStatsCpu > > { > > /** > @@ -50,11 +61,11 @@ Removing the signal and refactoring those classes doesn't have to be > addressed now, I think it would be part of a larger refactoring > (possibly also considering platforms that have no ISP but can produce > stats in hardware, such as the i.MX7), but please keep it on your radar. > +``` > > ---- > - > -5. Store ISP parameters in per-frame buffers > +### Store ISP parameters in per-frame buffers > > +``` > > /** > > * \fn void Debayer::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) > > * \brief Process the bayer data into the requested format. > @@ -68,11 +79,11 @@ stats in hardware, such as the i.MX7), but please keep it on your radar. > > Possibly something to address later, by storing ISP parameters in > per-frame buffers like we do for hardware ISPs. > +``` > > ---- > - > -8. DebayerCpu cleanups > +### DebayerCpu cleanups > > +``` > > >> class DebayerCpu : public Debayer, public Object > > >> const SharedFD &getStatsFD() { return stats_->getStatsFD(); } > > > > @@ -105,21 +116,21 @@ the need for performances and the need for a maintainable architecture. > > I think this falls under the lets wait until we have a GPU > > based SoftISP MVP/POC and then do some refactoring to see which > > bits should go where. > +``` > > ---- > - > -8. Decouple pipeline and IPA naming > +### Decouple pipeline and IPA naming > > +``` > > The current src/ipa/meson.build assumes the IPA name to match the > > pipeline name. For this reason "-Dipas=simple" is used for the > > Soft IPA module. > > This should be addressed. > +``` > > ---- > - > -9. Doxyfile cleanup > +### Doxyfile cleanup > > +``` > >> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in > >> index a86ea6c1..2be8d47b 100644 > >> --- a/Documentation/Doxyfile.in > @@ -160,13 +171,14 @@ Yes, because, well... all the other IPAs were doing that... > > > It doesn't have to be done before merging, but could you > > address this sooner than later ? > +``` > > ---- > - > -13. Improve black level and colour gains application > +### Improve black level and colour gains application > > +``` > I think the black level should eventually be moved before debayering, and > ideally the colour gains as well. I understand the need for optimizations to > lower the CPU consumption, but at the same time I don't feel comfortable > building up on top of an implementation that may work a bit more by chance than > by correctness, as that's not very maintainable. > +``` > diff --git a/utils/tuning/config-example.yaml b/utils/tuning/config-example.yaml > index 5593eaef809e..680f9213b269 100644 > --- a/utils/tuning/config-example.yaml > +++ b/utils/tuning/config-example.yaml > @@ -51,4 +51,4 @@ general: > macbeth: > small: 1 > show: 0 > -# blacklevel: 32 > \ No newline at end of file > +# blacklevel: 32 Looks ok to me, apart from the above. Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO.md similarity index 91% rename from src/libcamera/software_isp/TODO rename to src/libcamera/software_isp/TODO.md index f19e15ae27a0..bdd4a7421766 100644 --- a/src/libcamera/software_isp/TODO +++ b/src/libcamera/software_isp/TODO.md @@ -1,5 +1,16 @@ -2. Reconsider stats sharing +# Software ISP TODO list +This file contains the TODO list for the software ISP. + +## Common code and CPU-based implementation + +The TODO items in this section gather comments from patch review that were not +deemed to require being addressed right away. The text in block quotes is +copied directly from e-mail review. + +### Reconsider stats sharing + +``` >>> +void SwStatsCpu::finishFrame(void) >>> +{ >>> + *sharedStats_ = stats_; @@ -22,11 +33,11 @@ That would match how we deal with hardware ISPs, and I think that's a good idea. It will help decoupling the processing side from the IPA. +``` ---- - -3. Remove statsReady signal +### Remove statsReady signal +``` > class SwStatsCpu > { > /** @@ -50,11 +61,11 @@ Removing the signal and refactoring those classes doesn't have to be addressed now, I think it would be part of a larger refactoring (possibly also considering platforms that have no ISP but can produce stats in hardware, such as the i.MX7), but please keep it on your radar. +``` ---- - -5. Store ISP parameters in per-frame buffers +### Store ISP parameters in per-frame buffers +``` > /** > * \fn void Debayer::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params) > * \brief Process the bayer data into the requested format. @@ -68,11 +79,11 @@ stats in hardware, such as the i.MX7), but please keep it on your radar. Possibly something to address later, by storing ISP parameters in per-frame buffers like we do for hardware ISPs. +``` ---- - -8. DebayerCpu cleanups +### DebayerCpu cleanups +``` > >> class DebayerCpu : public Debayer, public Object > >> const SharedFD &getStatsFD() { return stats_->getStatsFD(); } > > @@ -105,21 +116,21 @@ the need for performances and the need for a maintainable architecture. > I think this falls under the lets wait until we have a GPU > based SoftISP MVP/POC and then do some refactoring to see which > bits should go where. +``` ---- - -8. Decouple pipeline and IPA naming +### Decouple pipeline and IPA naming +``` > The current src/ipa/meson.build assumes the IPA name to match the > pipeline name. For this reason "-Dipas=simple" is used for the > Soft IPA module. This should be addressed. +``` ---- - -9. Doxyfile cleanup +### Doxyfile cleanup +``` >> diff --git a/Documentation/Doxyfile.in b/Documentation/Doxyfile.in >> index a86ea6c1..2be8d47b 100644 >> --- a/Documentation/Doxyfile.in @@ -160,13 +171,14 @@ Yes, because, well... all the other IPAs were doing that... > It doesn't have to be done before merging, but could you > address this sooner than later ? +``` ---- - -13. Improve black level and colour gains application +### Improve black level and colour gains application +``` I think the black level should eventually be moved before debayering, and ideally the colour gains as well. I understand the need for optimizations to lower the CPU consumption, but at the same time I don't feel comfortable building up on top of an implementation that may work a bit more by chance than by correctness, as that's not very maintainable. +``` diff --git a/utils/tuning/config-example.yaml b/utils/tuning/config-example.yaml index 5593eaef809e..680f9213b269 100644 --- a/utils/tuning/config-example.yaml +++ b/utils/tuning/config-example.yaml @@ -51,4 +51,4 @@ general: macbeth: small: 1 show: 0 -# blacklevel: 32 \ No newline at end of file +# blacklevel: 32
To prepare for merging the two software ISP TODO lists (TODO and gpuisp-todo.txt), convert the TODO file to markdown. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/software_isp/{TODO => TODO.md} | 50 ++++++++++++-------- utils/tuning/config-example.yaml | 2 +- 2 files changed, 32 insertions(+), 20 deletions(-) rename src/libcamera/software_isp/{TODO => TODO.md} (91%)