| Message ID | 20210129151154.1051163-1-naush@raspberrypi.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi all, Gentle nudge for some feedback on this please? Thanks, Naush On Fri, 29 Jan 2021 at 15:11, Naushir Patuck <naush@raspberrypi.com> wrote: > Hi, > > Version 5 of this patch series addresses all the review comments in v4: > - Switch DenoiseMode to an enum class. > - Avoid double lookup in the DenoiseMode conversion table. > - Add controls::draft::NoiseReductionMode to the available controls > handled by the IPA. > > Regards, > Naush > > > Naushir Patuck (6): > pipeline: raspberrypi: Refactor stream configuration routine > pipeline: raspberrypi: Set the ISP Output1 to 1/4 resolution if unused > uapi: raspberrypi: Update the bcm2835-isp header definition > ipa: raspberrypi: Rename SdnStatus to DenoiseStatus > ipa: raspberrypi: Add a DenoiseAlgorithm class to the Controller > ipa: raspberrypi: Handle control::NoiseReductionMode in the controller > > include/libcamera/ipa/raspberrypi.h | 1 + > include/linux/bcm2835-isp.h | 32 ++++++- > .../controller/denoise_algorithm.hpp | 23 +++++ > .../{sdn_status.h => denoise_status.h} | 7 +- > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 17 ++-- > src/ipa/raspberrypi/controller/rpi/sdn.hpp | 5 +- > src/ipa/raspberrypi/raspberrypi.cpp | 59 +++++++++++-- > .../pipeline/raspberrypi/raspberrypi.cpp | 86 ++++++++++--------- > 8 files changed, 174 insertions(+), 56 deletions(-) > create mode 100644 src/ipa/raspberrypi/controller/denoise_algorithm.hpp > rename src/ipa/raspberrypi/controller/{sdn_status.h => denoise_status.h} > (62%) > > -- > 2.25.1 > >
Hi Naush, On Fri, Jan 29, 2021 at 03:11:48PM +0000, Naushir Patuck wrote: > Hi, > > Version 5 of this patch series addresses all the review comments in v4: > - Switch DenoiseMode to an enum class. > - Avoid double lookup in the DenoiseMode conversion table. > - Add controls::draft::NoiseReductionMode to the available controls handled by the IPA. I've reviewed the whole series and only have minor comments. I've offered to fix the small issues when applying (if you agree with the comments of course) in the first few patches, but as there are a few additional questions later, it may be easier if you could send a v6. Other patches to the RPi IPA have also been merged in the meantime, generating a few minor conflicts, and I'd prefer if you could resolve them and test the result. > Naushir Patuck (6): > pipeline: raspberrypi: Refactor stream configuration routine > pipeline: raspberrypi: Set the ISP Output1 to 1/4 resolution if unused > uapi: raspberrypi: Update the bcm2835-isp header definition > ipa: raspberrypi: Rename SdnStatus to DenoiseStatus > ipa: raspberrypi: Add a DenoiseAlgorithm class to the Controller > ipa: raspberrypi: Handle control::NoiseReductionMode in the controller > > include/libcamera/ipa/raspberrypi.h | 1 + > include/linux/bcm2835-isp.h | 32 ++++++- > .../controller/denoise_algorithm.hpp | 23 +++++ > .../{sdn_status.h => denoise_status.h} | 7 +- > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 17 ++-- > src/ipa/raspberrypi/controller/rpi/sdn.hpp | 5 +- > src/ipa/raspberrypi/raspberrypi.cpp | 59 +++++++++++-- > .../pipeline/raspberrypi/raspberrypi.cpp | 86 ++++++++++--------- > 8 files changed, 174 insertions(+), 56 deletions(-) > create mode 100644 src/ipa/raspberrypi/controller/denoise_algorithm.hpp > rename src/ipa/raspberrypi/controller/{sdn_status.h => denoise_status.h} (62%)
Hi Laurent, On Thu, 4 Feb 2021 at 21:49, Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Naush, > > On Fri, Jan 29, 2021 at 03:11:48PM +0000, Naushir Patuck wrote: > > Hi, > > > > Version 5 of this patch series addresses all the review comments in v4: > > - Switch DenoiseMode to an enum class. > > - Avoid double lookup in the DenoiseMode conversion table. > > - Add controls::draft::NoiseReductionMode to the available controls > handled by the IPA. > > I've reviewed the whole series and only have minor comments. I've > offered to fix the small issues when applying (if you agree with the > comments of course) in the first few patches, but as there are a few > additional questions later, it may be easier if you could send a v6. > Other patches to the RPi IPA have also been merged in the meantime, > generating a few minor conflicts, and I'd prefer if you could resolve > them and test the result. > Thanks for all the reviews. I have incorporated all the suggested changes, and done a rebase. I will post a new version 6 of the series after a bit more testing. Regards, Naush > > > Naushir Patuck (6): > > pipeline: raspberrypi: Refactor stream configuration routine > > pipeline: raspberrypi: Set the ISP Output1 to 1/4 resolution if unused > > uapi: raspberrypi: Update the bcm2835-isp header definition > > ipa: raspberrypi: Rename SdnStatus to DenoiseStatus > > ipa: raspberrypi: Add a DenoiseAlgorithm class to the Controller > > ipa: raspberrypi: Handle control::NoiseReductionMode in the controller > > > > include/libcamera/ipa/raspberrypi.h | 1 + > > include/linux/bcm2835-isp.h | 32 ++++++- > > .../controller/denoise_algorithm.hpp | 23 +++++ > > .../{sdn_status.h => denoise_status.h} | 7 +- > > src/ipa/raspberrypi/controller/rpi/sdn.cpp | 17 ++-- > > src/ipa/raspberrypi/controller/rpi/sdn.hpp | 5 +- > > src/ipa/raspberrypi/raspberrypi.cpp | 59 +++++++++++-- > > .../pipeline/raspberrypi/raspberrypi.cpp | 86 ++++++++++--------- > > 8 files changed, 174 insertions(+), 56 deletions(-) > > create mode 100644 src/ipa/raspberrypi/controller/denoise_algorithm.hpp > > rename src/ipa/raspberrypi/controller/{sdn_status.h => > denoise_status.h} (62%) > > -- > Regards, > > Laurent Pinchart >
Hi, Version 5 of this patch series addresses all the review comments in v4: - Switch DenoiseMode to an enum class. - Avoid double lookup in the DenoiseMode conversion table. - Add controls::draft::NoiseReductionMode to the available controls handled by the IPA. Regards, Naush Naushir Patuck (6): pipeline: raspberrypi: Refactor stream configuration routine pipeline: raspberrypi: Set the ISP Output1 to 1/4 resolution if unused uapi: raspberrypi: Update the bcm2835-isp header definition ipa: raspberrypi: Rename SdnStatus to DenoiseStatus ipa: raspberrypi: Add a DenoiseAlgorithm class to the Controller ipa: raspberrypi: Handle control::NoiseReductionMode in the controller include/libcamera/ipa/raspberrypi.h | 1 + include/linux/bcm2835-isp.h | 32 ++++++- .../controller/denoise_algorithm.hpp | 23 +++++ .../{sdn_status.h => denoise_status.h} | 7 +- src/ipa/raspberrypi/controller/rpi/sdn.cpp | 17 ++-- src/ipa/raspberrypi/controller/rpi/sdn.hpp | 5 +- src/ipa/raspberrypi/raspberrypi.cpp | 59 +++++++++++-- .../pipeline/raspberrypi/raspberrypi.cpp | 86 ++++++++++--------- 8 files changed, 174 insertions(+), 56 deletions(-) create mode 100644 src/ipa/raspberrypi/controller/denoise_algorithm.hpp rename src/ipa/raspberrypi/controller/{sdn_status.h => denoise_status.h} (62%)