[PATCH v4 8/9] libcamera: software_isp: Track whether CCM is enabled

[Kieran Bingham]

Quoting Milan Zamazal (2025-01-29 10:45:45)
> Hi Laurent,
> 
> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> 
> > Hi Milan,
> >
> > On Tue, Jan 28, 2025 at 04:19:39PM +0100, Milan Zamazal wrote:
> >> Laurent Pinchart writes:
> >> > On Mon, Jan 13, 2025 at 02:51:05PM +0100, Milan Zamazal wrote:
> >> >> Applying color correction matrix (CCM) in software ISP is optional due
> >> >> to performance reasons.  CCM is applied if and only if `Ccm' algorithm
> >> >> is present in the tuning file and it defines some CCM.
> >> >> 
> >> >> Software ISP debayering is a performance critical piece of code and we
> >> >> do not want to use dynamic conditionals there.  Therefore we pass
> >> >> information about CCM application to debayering configuration and let it
> >> >> select the right versions of debayering functions using templates.  This
> >> >> is a similar trick as the previously used one for adding or not adding
> >> >> an alpha channel to the output.
> >> >> 
> >> >> Debayering gets this information but it ignores it in this patch.
> >> >> Actual processing with CCM is added in the followup patch.
> >> >> 
> >> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> >> >> ---
> >> >>  include/libcamera/ipa/soft.mojom            |  2 +-
> >> >>  src/ipa/simple/algorithms/ccm.cpp           | 11 +++++--
> >> >>  src/ipa/simple/algorithms/ccm.h             |  1 +
> >> >>  src/ipa/simple/soft_simple.cpp              |  6 ++--
> >> >>  src/libcamera/software_isp/debayer.cpp      |  3 +-
> >> >>  src/libcamera/software_isp/debayer.h        |  3 +-
> >> >>  src/libcamera/software_isp/debayer_cpu.cpp  | 35 ++++++++++++---------
> >> >>  src/libcamera/software_isp/debayer_cpu.h    | 24 +++++++-------
> >> >>  src/libcamera/software_isp/software_isp.cpp |  5 +--
> >> >>  9 files changed, 54 insertions(+), 36 deletions(-)
> >> >> 
> >> >> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> >> >> index d52e6f1a..b5ed3905 100644
> >> >> --- a/include/libcamera/ipa/soft.mojom
> >> >> +++ b/include/libcamera/ipa/soft.mojom
> >> >> @@ -21,7 +21,7 @@ interface IPASoftInterface {
> >> >>   start() => (int32 ret);
> >> >>   stop();
> >> >>   configure(IPAConfigInfo configInfo)
> >> >> -         => (int32 ret);
> >> >> +         => (int32 ret, bool ccmEnabled);
> >> >
> >> > Do you foresee cases where the CCM algorithm could be enabled or
> >> > disabled depending on the camera configuration ? If not, I would return
> >> > this from the init() function instead of the configure() function.
> >> 
> >> OK, it's a bit more complicated to pass to the right place but possible.
> >> I'll do it in v5.
> >> 
> >> >>   [async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);
> >> >>   [async] computeParams(uint32 frame);
> >> >> diff --git a/src/ipa/simple/algorithms/ccm.cpp b/src/ipa/simple/algorithms/ccm.cpp
> >> >> index 3c7fca2d..96038966 100644
> >> >> --- a/src/ipa/simple/algorithms/ccm.cpp
> >> >> +++ b/src/ipa/simple/algorithms/ccm.cpp
> >> >> @@ -38,12 +38,17 @@ int Ccm::init([[maybe_unused]] IPAContext &context, const YamlObject &tuningData
> >> >>   return 0;
> >> >>  }
> >> >>  
> >> >> -void Ccm::prepare(IPAContext &context, const uint32_t frame,
> >> >> -           IPAFrameContext &frameContext, [[maybe_unused]] DebayerParams *params)
> >> >> +int Ccm::configure(IPAContext &context,
> >> >> +            [[maybe_unused]] const IPAConfigInfo &configInfo)
> >> >
> >> > I think you can skip this and handled the ccmEnabled variable purely in
> >> > the IPA module, but checking if the CCM algorithm has been instantiated.
> >> > This can be done by inspecting the algorithms list.
> >> 
> >> I wouldn't like inspecting the algorithms list, I think it's cleaner to
> >> provide the information from the algorithm itself.  Moreover, other
> >> algorithms need information about CCM.
> >
> > You need to support the case where the CCM algorithm is not listed in
> > the tuning file, so the information has to be handled in the IPA module
> > itself. If the need is to check if a CCM algorithm has been
> > instantiated, then looking at the algorithms list seems the best fit.
> 
> Why is it better to check the list of the algorithms rather than the
> algorithm self-reporting itself by setting a flag in the context, which
> must be done anyway so that the other algorithms know (and the IPA
> module just passes this info to software ISP)?

Can you check what happens when there is no ccm: section in the tuning
file ?

I expect no ccm: in tuning file will mean no CCM algorithm gets
loaded/executed at all in the IPA. That's how the other libipa modules
work.

--
Kieran