Hi Dan,
thank you for review.
Dan Scally <dan.scally at ideasonboard.com> writes:
> Hi Milan
>
> On 17/07/2024 09:54, Milan Zamazal wrote:
>> This patch adds Algorithm::queueRequest call for the defined algorithms.
>> As there are currently no control knobs in software ISP nor the
>> corresponding queueRequest call chain, the patch also introduces the
>> queueRequest methods and calls from the pipeline to the IPA.
>>
>> This is preparation only since there are currently no Algorithm based
>> algorithms defined and no current software ISP algorithms support
>> control knobs.
>>
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>> include/libcamera/internal/software_isp/software_isp.h | 1 +
>> include/libcamera/ipa/soft.mojom | 1 +
>> src/ipa/simple/soft_simple.cpp | 9 +++++++++
>> src/libcamera/pipeline/simple/simple.cpp | 2 ++
>> src/libcamera/software_isp/software_isp.cpp | 10 ++++++++++
>> 5 files changed, 23 insertions(+)
>>
>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
>> index c5d5a46f..a3e3a9da 100644
>> --- a/include/libcamera/internal/software_isp/software_isp.h
>> +++ b/include/libcamera/internal/software_isp/software_isp.h
>> @@ -73,6 +73,7 @@ public:
>> int start();
>> void stop();
>> + void queueRequest(const uint32_t frame, const ControlList &controls);
>
> As mentioned before, I think we should move away from calling this variable "frame". It's not the frame number from the driver, in most
> implementations (including this one) it's the request sequence number, so I think naming it after that somehow is the better approach.
I agree but I wouldn't like to diverge from the other pipelines. Is it
preferable to change the variable name in this series and possibly later
also in the other pipelines, or to keep `frame' here for now and then
to make mass-rename everywhere?
> Other than the variable name, patch looks fine to me:
>
>
> Reviewed-by: Daniel Scally <dan.scally at ideasonboard.com>
>
>> int queueBuffers(uint32_t frame, FrameBuffer *input,
>> const std::map<const Stream *, FrameBuffer *> &outputs);
>> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
>> index 5e124016..34b91657 100644
>> --- a/include/libcamera/ipa/soft.mojom
>> +++ b/include/libcamera/ipa/soft.mojom
>> @@ -23,6 +23,7 @@ interface IPASoftInterface {
>> configure(IPAConfigInfo configInfo)
>> => (int32 ret);
>> + [async] queueRequest(uint32 frame, libcamera.ControlList sensorControls);
>> [async] processStats(uint32 frame, uint32 bufferId, libcamera.ControlList sensorControls);
>> };
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index 49fff312..bd0dea5f 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -77,6 +77,7 @@ public:
>> int start() override;
>> void stop() override;
>> + void queueRequest(const uint32_t frame, const ControlList &controls) override;
>> void processStats(const uint32_t frame, const uint32_t bufferId,
>> const ControlList &sensorControls) override;
>> @@ -269,6 +270,14 @@ void IPASoftSimple::stop()
>> {
>> }
>> +void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &controls)
>> +{
>> + IPAFrameContext &frameContext = context_.frameContexts.alloc(frame);
>> +
>> + for (auto const &algo : algorithms())
>> + algo->queueRequest(context_, frame, frameContext, controls);
>> +}
>> +
>> void IPASoftSimple::processStats(
>> [[maybe_unused]] const uint32_t frame,
>> [[maybe_unused]] const uint32_t bufferId,
>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>> index 4326d14c..984c3a77 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -1429,6 +1429,8 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>> if (data->useConversion_)
>> data->conversionQueue_.push(std::move(buffers));
>> + data->swIsp_->queueRequest(request->sequence(), request->controls());
>> +
>> return 0;
>> }
>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>> index 9793df18..0b9c5fdf 100644
>> --- a/src/libcamera/software_isp/software_isp.cpp
>> +++ b/src/libcamera/software_isp/software_isp.cpp
>> @@ -279,6 +279,16 @@ int SoftwareIsp::exportBuffers(const Stream *stream, unsigned int count,
>> return count;
>> }
>> +/**
>> + * \brief Queue a request and process the control list from the application
>> + * \param[in] frame The number of the frame which will be processed next
>> + * \param[in] controls The controls for the \a frame
>> + */
>> +void SoftwareIsp::queueRequest(const uint32_t frame, const ControlList &controls)
>> +{
>> + ipa_->queueRequest(frame, controls);
>> +}
>> +
>> /**
>> * \brief Queue buffers to Software ISP
>> * \param[in] frame The frame number