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

[Milan Zamazal]

Kieran Bingham <kieran.bingham at ideasonboard.com> writes:

> 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.

Yes, that's the expected behavior.  Then ccmEnabled flag remains false
(the default value) and things work accordingly, mostly the same as
before this patch set.