| Message ID | 20260604095105.68798-4-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2026-06-04 10:50:45) > Processing parameters in software ISP are currently passed by value. > This is unlike hardware pipelines, which use a ring of buffers for the > purpose and pass references to the buffers currently selected from the > ring rather than passing the whole parameters buffers. > > This is a preparatory patch to introduce a similar mechanism in software > ISP, in order to resolve TODO #5. It adds a new argument paramsBufferId > for the future parameters buffer ids passed to the calls. The buffer > ids must be passed to the following groups of methods: > > - IPASoftSimple::prepare, in order to create the parameters in the given > buffer for the corresponding frame processing. > > - SoftwareIsp::saveIspParams, in order to signal that the parameters are > set by the IPA. > > - Debayer::process, in order to get the buffer with the processing > parameters. > > - SoftwareIsp::paramsBufferReady, in order to signal that the selected > parameters buffer from the ring is no longer needed for the given > frame and can be reused. This is a newly introduced signal. > > The type of the buffer id parameter is set to uint32_t because: > > - It can be used in mojom. > - It is consistent with the similar types in the hardware pipelines. > - It covers file descriptor number range, which will be used as buffer > ids (more on this in followup patches). > > This patch doesn't do more than adding the arguments, to keep the patch > simple. The buffer handling will be implemented in the followup > patches. > I'm curious if the actual 'ISP' part should know about the buffer id ... because hardware ISPs don't know the buffer id - they just get given the dmabufs etc. But I think for now it's fine? I would be curious to see how the actual 'ISP' part could be very clearly an independent component with a clear interface, almost mirroring the hardware devices, but I think this is already working towards simplifying things for parallel/pipelined operations so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > .../libcamera/internal/software_isp/software_isp.h | 3 ++- > include/libcamera/ipa/soft.mojom | 4 ++-- > src/ipa/simple/soft_simple.cpp | 8 +++++--- > src/libcamera/software_isp/debayer.cpp | 8 +++++++- > src/libcamera/software_isp/debayer.h | 6 +++++- > src/libcamera/software_isp/debayer_cpu.cpp | 6 +++++- > src/libcamera/software_isp/debayer_cpu.h | 4 +++- > src/libcamera/software_isp/debayer_egl.cpp | 6 +++++- > src/libcamera/software_isp/debayer_egl.h | 4 +++- > src/libcamera/software_isp/software_isp.cpp | 14 +++++++++++--- > 10 files changed, 48 insertions(+), 15 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h > index 86cb8f8de..85c9059e2 100644 > --- a/include/libcamera/internal/software_isp/software_isp.h > +++ b/include/libcamera/internal/software_isp/software_isp.h > @@ -88,7 +88,8 @@ public: > Signal<const ControlList &> setSensorControls; > > private: > - void saveIspParams(); > + void saveIspParams(const uint32_t paramsBufferId); > + void paramsBufferReady(const uint32_t paramsBufferId); > void setSensorCtrls(const ControlList &sensorControls); > void statsReady(uint32_t frame, uint32_t bufferId); > void inputReady(FrameBuffer *input); > diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom > index e75b03a3d..18789d5de 100644 > --- a/include/libcamera/ipa/soft.mojom > +++ b/include/libcamera/ipa/soft.mojom > @@ -25,7 +25,7 @@ interface IPASoftInterface { > => (int32 ret); > > [async] queueRequest(uint32 frame, libcamera.ControlList sensorControls); > - [async] computeParams(uint32 frame); > + [async] computeParams(uint32 frame, uint32 paramsBufferId); > [async] processStats(uint32 frame, > uint32 bufferId, > libcamera.ControlList sensorControls); > @@ -33,6 +33,6 @@ interface IPASoftInterface { > > interface IPASoftEventInterface { > setSensorControls(libcamera.ControlList sensorControls); > - paramsComputed(); > + paramsComputed(uint32 paramsBufferId); > metadataReady(uint32 frame, libcamera.ControlList metadata); > }; > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index cfc1389e4..69bfef302 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -64,7 +64,8 @@ public: > void stop() override; > > void queueRequest(const uint32_t frame, const ControlList &controls) override; > - void computeParams(const uint32_t frame) override; > + void computeParams(const uint32_t frame, > + const uint32_t paramsBufferId) override; > void processStats(const uint32_t frame, const uint32_t bufferId, > const ControlList &sensorControls) override; > > @@ -283,7 +284,8 @@ void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &contro > algo->queueRequest(context_, frame, frameContext, controls); > } > > -void IPASoftSimple::computeParams(const uint32_t frame) > +void IPASoftSimple::computeParams(const uint32_t frame, > + const uint32_t paramsBufferId) > { > context_.activeState.combinedMatrix = Matrix<float, 3, 3>::identity(); > > @@ -292,7 +294,7 @@ void IPASoftSimple::computeParams(const uint32_t frame) > algo->prepare(context_, frame, frameContext, params_); > params_->combinedMatrix = context_.activeState.combinedMatrix; > > - paramsComputed.emit(); > + paramsComputed.emit(paramsBufferId); > } > > void IPASoftSimple::processStats(const uint32_t frame, > diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp > index 2d7abfb83..7b1be52b2 100644 > --- a/src/libcamera/software_isp/debayer.cpp > +++ b/src/libcamera/software_isp/debayer.cpp > @@ -104,9 +104,10 @@ Debayer::~Debayer() > */ > > /** > - * \fn void Debayer::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) > + * \fn void Debayer::process(uint32_t frame, const uint32_t paramsBufferId, FrameBuffer *input, FrameBuffer *output, DebayerParams params) > * \brief Process the bayer data into the requested format > * \param[in] frame The frame number > + * \param[in] paramsBufferId The id of the params buffer in use > * \param[in] input The input buffer > * \param[in] output The output buffer > * \param[in] params The parameters to be used in debayering > @@ -142,6 +143,11 @@ Debayer::~Debayer() > * current stream. > */ > > +/** > + * \var Debayer::paramsBufferReady > + * \brief Signals when the processing params are no longer needed > + */ > + > /** > * \var Signal<FrameBuffer *> Debayer::inputBufferReady > * \brief Signals when the input buffer is ready > diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h > index a2a17ec18..9a97851b7 100644 > --- a/src/libcamera/software_isp/debayer.h > +++ b/src/libcamera/software_isp/debayer.h > @@ -47,7 +47,10 @@ public: > virtual std::tuple<unsigned int, unsigned int> > strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; > > - virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) = 0; > + virtual void process(uint32_t frame, > + const uint32_t paramsBufferId, > + FrameBuffer *input, FrameBuffer *output, > + const DebayerParams ¶ms) = 0; > virtual int start() { return 0; } > virtual void stop() {} > > @@ -59,6 +62,7 @@ public: > > Signal<FrameBuffer *> inputBufferReady; > Signal<FrameBuffer *> outputBufferReady; > + Signal<uint32_t> paramsBufferReady; > > struct DebayerInputConfig { > Size patternSize; > diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp > index 1f9b24da0..51b58fce5 100644 > --- a/src/libcamera/software_isp/debayer_cpu.cpp > +++ b/src/libcamera/software_isp/debayer_cpu.cpp > @@ -971,7 +971,9 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms) > params_ = params; > } > > -void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) > +void DebayerCpu::process(uint32_t frame, const uint32_t paramsBufferId, > + FrameBuffer *input, FrameBuffer *output, > + const DebayerParams ¶ms) > { > bench_.startFrame(); > > @@ -981,6 +983,8 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output > > updateLookupTables(params); > > + paramsBufferReady.emit(paramsBufferId); > + > /* Copy metadata from the input buffer */ > FrameMetadata &metadata = output->_d()->metadata(); > metadata.status = input->metadata().status; > diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h > index 68da95083..1b4e8548a 100644 > --- a/src/libcamera/software_isp/debayer_cpu.h > +++ b/src/libcamera/software_isp/debayer_cpu.h > @@ -42,7 +42,9 @@ public: > std::vector<PixelFormat> formats(PixelFormat input) override; > std::tuple<unsigned int, unsigned int> > strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) override; > - void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) override; > + void process(uint32_t frame, const uint32_t paramsBufferId, > + FrameBuffer *input, FrameBuffer *output, > + const DebayerParams ¶ms) override; > int start() override; > void stop() override; > SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) override; > diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp > index 99825d49e..60fd5463f 100644 > --- a/src/libcamera/software_isp/debayer_egl.cpp > +++ b/src/libcamera/software_isp/debayer_egl.cpp > @@ -546,7 +546,9 @@ int DebayerEGL::debayerGPU(FrameBuffer *input, FrameBuffer *output, const Debaye > return 0; > } > > -void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) > +void DebayerEGL::process(uint32_t frame, const uint32_t paramsBufferId, > + FrameBuffer *input, FrameBuffer *output, > + const DebayerParams ¶ms) > { > bench_.startFrame(); > > @@ -563,6 +565,7 @@ void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output > LOG(Debayer, Error) << "debayerGPU failed"; > goto error; > } > + paramsBufferReady.emit(paramsBufferId); > > metadata.planes()[0].bytesused = output->planes()[0].length; > > @@ -593,6 +596,7 @@ void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output > return; > > error: > + paramsBufferReady.emit(paramsBufferId); > bench_.finishFrame(); > metadata.status = FrameMetadata::FrameError; > return; > diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h > index 875e7cfc5..a3a4448e7 100644 > --- a/src/libcamera/software_isp/debayer_egl.h > +++ b/src/libcamera/software_isp/debayer_egl.h > @@ -51,7 +51,9 @@ public: > std::vector<PixelFormat> formats(PixelFormat input) override; > std::tuple<unsigned int, unsigned int> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) override; > > - void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) override; > + void process(uint32_t frame, const uint32_t paramsBufferId, > + FrameBuffer *input, FrameBuffer *output, > + const DebayerParams ¶ms) override; > int start() override; > void stop() override; > > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index f5b44f4e5..3d1dec21d 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -128,6 +128,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > > debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady); > debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady); > + debayer_->paramsBufferReady.connect(this, &SoftwareIsp::paramsBufferReady); > > ipa_ = pipe->createIPA<ipa::soft::IPAProxySoft>(0, 0); > if (!ipa_) { > @@ -409,16 +410,23 @@ void SoftwareIsp::stop() > */ > void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output) > { > - ipa_->computeParams(frame); > + /* \todo Provide a real value */ > + constexpr uint32_t paramsBufferId = 0; > + ipa_->computeParams(frame, paramsBufferId); > debayer_->invokeMethod(&Debayer::process, > - ConnectionTypeQueued, frame, input, output, debayerParams_); > + ConnectionTypeQueued, frame, paramsBufferId, > + input, output, debayerParams_); > } > > -void SoftwareIsp::saveIspParams() > +void SoftwareIsp::saveIspParams([[maybe_unused]] const uint32_t paramsBufferId) > { > debayerParams_ = *sharedParams_; > } > > +void SoftwareIsp::paramsBufferReady([[maybe_unused]] const uint32_t paramsBufferId) > +{ > +} > + > void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls) > { > setSensorControls.emit(sensorControls); > -- > 2.54.0 >
[I've apparently forgotten to clean up the patch directory before some final reordering and have posted duplicate patches. I'll post v4 fixing this.] Hi Kieran, thank you for review. Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2026-06-04 10:50:45) >> Processing parameters in software ISP are currently passed by value. >> This is unlike hardware pipelines, which use a ring of buffers for the > >> purpose and pass references to the buffers currently selected from the >> ring rather than passing the whole parameters buffers. >> >> This is a preparatory patch to introduce a similar mechanism in software >> ISP, in order to resolve TODO #5. It adds a new argument paramsBufferId >> for the future parameters buffer ids passed to the calls. The buffer >> ids must be passed to the following groups of methods: >> >> - IPASoftSimple::prepare, in order to create the parameters in the given >> buffer for the corresponding frame processing. >> >> - SoftwareIsp::saveIspParams, in order to signal that the parameters are >> set by the IPA. >> >> - Debayer::process, in order to get the buffer with the processing >> parameters. >> >> - SoftwareIsp::paramsBufferReady, in order to signal that the selected >> parameters buffer from the ring is no longer needed for the given >> frame and can be reused. This is a newly introduced signal. >> >> The type of the buffer id parameter is set to uint32_t because: >> >> - It can be used in mojom. >> - It is consistent with the similar types in the hardware pipelines. >> - It covers file descriptor number range, which will be used as buffer >> ids (more on this in followup patches). >> >> This patch doesn't do more than adding the arguments, to keep the patch >> simple. The buffer handling will be implemented in the followup >> patches. >> > > I'm curious if the actual 'ISP' part should know about the buffer id ... > because hardware ISPs don't know the buffer id - they just get given the > dmabufs etc. > > But I think for now it's fine? > > I would be curious to see how the actual 'ISP' part could be very > clearly an independent component with a clear interface, almost > mirroring the hardware devices, Maybe, IIRC I thought the way implemented here was easier and avoided one level of indirection somewhere. I don't feel like reworking it now would be wise, let's see how we can progress. > but I think this is already working towards simplifying things for > parallel/pipelined operations so: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> .../libcamera/internal/software_isp/software_isp.h | 3 ++- >> include/libcamera/ipa/soft.mojom | 4 ++-- >> src/ipa/simple/soft_simple.cpp | 8 +++++--- >> src/libcamera/software_isp/debayer.cpp | 8 +++++++- >> src/libcamera/software_isp/debayer.h | 6 +++++- >> src/libcamera/software_isp/debayer_cpu.cpp | 6 +++++- >> src/libcamera/software_isp/debayer_cpu.h | 4 +++- >> src/libcamera/software_isp/debayer_egl.cpp | 6 +++++- >> src/libcamera/software_isp/debayer_egl.h | 4 +++- >> src/libcamera/software_isp/software_isp.cpp | 14 +++++++++++--- >> 10 files changed, 48 insertions(+), 15 deletions(-) >> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h >> index 86cb8f8de..85c9059e2 100644 >> --- a/include/libcamera/internal/software_isp/software_isp.h >> +++ b/include/libcamera/internal/software_isp/software_isp.h >> @@ -88,7 +88,8 @@ public: >> Signal<const ControlList &> setSensorControls; >> >> private: >> - void saveIspParams(); >> + void saveIspParams(const uint32_t paramsBufferId); >> + void paramsBufferReady(const uint32_t paramsBufferId); >> void setSensorCtrls(const ControlList &sensorControls); >> void statsReady(uint32_t frame, uint32_t bufferId); >> void inputReady(FrameBuffer *input); >> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom >> index e75b03a3d..18789d5de 100644 >> --- a/include/libcamera/ipa/soft.mojom >> +++ b/include/libcamera/ipa/soft.mojom >> @@ -25,7 +25,7 @@ interface IPASoftInterface { >> => (int32 ret); >> >> [async] queueRequest(uint32 frame, libcamera.ControlList sensorControls); >> - [async] computeParams(uint32 frame); >> + [async] computeParams(uint32 frame, uint32 paramsBufferId); >> [async] processStats(uint32 frame, >> uint32 bufferId, >> libcamera.ControlList sensorControls); >> @@ -33,6 +33,6 @@ interface IPASoftInterface { >> >> interface IPASoftEventInterface { >> setSensorControls(libcamera.ControlList sensorControls); >> - paramsComputed(); >> + paramsComputed(uint32 paramsBufferId); >> metadataReady(uint32 frame, libcamera.ControlList metadata); >> }; >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >> index cfc1389e4..69bfef302 100644 >> --- a/src/ipa/simple/soft_simple.cpp >> +++ b/src/ipa/simple/soft_simple.cpp >> @@ -64,7 +64,8 @@ public: >> void stop() override; >> >> void queueRequest(const uint32_t frame, const ControlList &controls) override; >> - void computeParams(const uint32_t frame) override; >> + void computeParams(const uint32_t frame, >> + const uint32_t paramsBufferId) override; >> void processStats(const uint32_t frame, const uint32_t bufferId, >> const ControlList &sensorControls) override; >> >> @@ -283,7 +284,8 @@ void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &contro >> algo->queueRequest(context_, frame, frameContext, controls); >> } >> >> -void IPASoftSimple::computeParams(const uint32_t frame) >> +void IPASoftSimple::computeParams(const uint32_t frame, >> + const uint32_t paramsBufferId) >> { >> context_.activeState.combinedMatrix = Matrix<float, 3, 3>::identity(); >> >> @@ -292,7 +294,7 @@ void IPASoftSimple::computeParams(const uint32_t frame) >> algo->prepare(context_, frame, frameContext, params_); >> params_->combinedMatrix = context_.activeState.combinedMatrix; >> >> - paramsComputed.emit(); >> + paramsComputed.emit(paramsBufferId); >> } >> >> void IPASoftSimple::processStats(const uint32_t frame, >> diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp >> index 2d7abfb83..7b1be52b2 100644 >> --- a/src/libcamera/software_isp/debayer.cpp >> +++ b/src/libcamera/software_isp/debayer.cpp >> @@ -104,9 +104,10 @@ Debayer::~Debayer() >> */ >> >> /** >> - * \fn void Debayer::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) >> + * \fn void Debayer::process(uint32_t frame, const uint32_t paramsBufferId, FrameBuffer *input, FrameBuffer *output, DebayerParams params) >> * \brief Process the bayer data into the requested format >> * \param[in] frame The frame number >> + * \param[in] paramsBufferId The id of the params buffer in use >> * \param[in] input The input buffer >> * \param[in] output The output buffer >> * \param[in] params The parameters to be used in debayering >> @@ -142,6 +143,11 @@ Debayer::~Debayer() >> * current stream. >> */ >> >> +/** >> + * \var Debayer::paramsBufferReady >> + * \brief Signals when the processing params are no longer needed >> + */ >> + >> /** >> * \var Signal<FrameBuffer *> Debayer::inputBufferReady >> * \brief Signals when the input buffer is ready >> diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h >> index a2a17ec18..9a97851b7 100644 >> --- a/src/libcamera/software_isp/debayer.h >> +++ b/src/libcamera/software_isp/debayer.h >> @@ -47,7 +47,10 @@ public: >> virtual std::tuple<unsigned int, unsigned int> >> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; >> >> - virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) = 0; >> + virtual void process(uint32_t frame, >> + const uint32_t paramsBufferId, >> + FrameBuffer *input, FrameBuffer *output, >> + const DebayerParams ¶ms) = 0; >> virtual int start() { return 0; } >> virtual void stop() {} >> >> @@ -59,6 +62,7 @@ public: >> >> Signal<FrameBuffer *> inputBufferReady; >> Signal<FrameBuffer *> outputBufferReady; >> + Signal<uint32_t> paramsBufferReady; >> >> struct DebayerInputConfig { >> Size patternSize; >> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp >> index 1f9b24da0..51b58fce5 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.cpp >> +++ b/src/libcamera/software_isp/debayer_cpu.cpp >> @@ -971,7 +971,9 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms) >> params_ = params; >> } >> >> -void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) >> +void DebayerCpu::process(uint32_t frame, const uint32_t paramsBufferId, >> + FrameBuffer *input, FrameBuffer *output, >> + const DebayerParams ¶ms) >> { >> bench_.startFrame(); >> >> @@ -981,6 +983,8 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output >> >> updateLookupTables(params); >> >> + paramsBufferReady.emit(paramsBufferId); >> + >> /* Copy metadata from the input buffer */ >> FrameMetadata &metadata = output->_d()->metadata(); >> metadata.status = input->metadata().status; >> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h >> index 68da95083..1b4e8548a 100644 >> --- a/src/libcamera/software_isp/debayer_cpu.h >> +++ b/src/libcamera/software_isp/debayer_cpu.h >> @@ -42,7 +42,9 @@ public: >> std::vector<PixelFormat> formats(PixelFormat input) override; >> std::tuple<unsigned int, unsigned int> >> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) override; >> - void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) override; >> + void process(uint32_t frame, const uint32_t paramsBufferId, >> + FrameBuffer *input, FrameBuffer *output, >> + const DebayerParams ¶ms) override; >> int start() override; >> void stop() override; >> SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) override; >> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp >> index 99825d49e..60fd5463f 100644 >> --- a/src/libcamera/software_isp/debayer_egl.cpp >> +++ b/src/libcamera/software_isp/debayer_egl.cpp >> @@ -546,7 +546,9 @@ int DebayerEGL::debayerGPU(FrameBuffer *input, FrameBuffer *output, const Debaye >> return 0; >> } >> >> -void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) >> +void DebayerEGL::process(uint32_t frame, const uint32_t paramsBufferId, >> + FrameBuffer *input, FrameBuffer *output, >> + const DebayerParams ¶ms) >> { >> bench_.startFrame(); >> >> @@ -563,6 +565,7 @@ void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output >> LOG(Debayer, Error) << "debayerGPU failed"; >> goto error; >> } >> + paramsBufferReady.emit(paramsBufferId); >> >> metadata.planes()[0].bytesused = output->planes()[0].length; >> >> @@ -593,6 +596,7 @@ void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output >> return; >> >> error: >> + paramsBufferReady.emit(paramsBufferId); >> bench_.finishFrame(); >> metadata.status = FrameMetadata::FrameError; >> return; >> diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h >> index 875e7cfc5..a3a4448e7 100644 >> --- a/src/libcamera/software_isp/debayer_egl.h >> +++ b/src/libcamera/software_isp/debayer_egl.h >> @@ -51,7 +51,9 @@ public: >> std::vector<PixelFormat> formats(PixelFormat input) override; >> std::tuple<unsigned int, unsigned int> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) override; >> >> - void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) override; >> + void process(uint32_t frame, const uint32_t paramsBufferId, >> + FrameBuffer *input, FrameBuffer *output, >> + const DebayerParams ¶ms) override; >> int start() override; >> void stop() override; >> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >> index f5b44f4e5..3d1dec21d 100644 >> --- a/src/libcamera/software_isp/software_isp.cpp >> +++ b/src/libcamera/software_isp/software_isp.cpp >> @@ -128,6 +128,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, >> >> debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady); >> debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady); >> + debayer_->paramsBufferReady.connect(this, &SoftwareIsp::paramsBufferReady); >> >> ipa_ = pipe->createIPA<ipa::soft::IPAProxySoft>(0, 0); >> if (!ipa_) { >> @@ -409,16 +410,23 @@ void SoftwareIsp::stop() >> */ >> void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output) >> { >> - ipa_->computeParams(frame); >> + /* \todo Provide a real value */ >> + constexpr uint32_t paramsBufferId = 0; >> + ipa_->computeParams(frame, paramsBufferId); >> debayer_->invokeMethod(&Debayer::process, >> - ConnectionTypeQueued, frame, input, output, debayerParams_); >> + ConnectionTypeQueued, frame, paramsBufferId, >> + input, output, debayerParams_); >> } >> >> -void SoftwareIsp::saveIspParams() >> +void SoftwareIsp::saveIspParams([[maybe_unused]] const uint32_t paramsBufferId) >> { >> debayerParams_ = *sharedParams_; >> } >> >> +void SoftwareIsp::paramsBufferReady([[maybe_unused]] const uint32_t paramsBufferId) >> +{ >> +} >> + >> void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls) >> { >> setSensorControls.emit(sensorControls); >> -- >> 2.54.0 >>
diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h index 86cb8f8de..85c9059e2 100644 --- a/include/libcamera/internal/software_isp/software_isp.h +++ b/include/libcamera/internal/software_isp/software_isp.h @@ -88,7 +88,8 @@ public: Signal<const ControlList &> setSensorControls; private: - void saveIspParams(); + void saveIspParams(const uint32_t paramsBufferId); + void paramsBufferReady(const uint32_t paramsBufferId); void setSensorCtrls(const ControlList &sensorControls); void statsReady(uint32_t frame, uint32_t bufferId); void inputReady(FrameBuffer *input); diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom index e75b03a3d..18789d5de 100644 --- a/include/libcamera/ipa/soft.mojom +++ b/include/libcamera/ipa/soft.mojom @@ -25,7 +25,7 @@ interface IPASoftInterface { => (int32 ret); [async] queueRequest(uint32 frame, libcamera.ControlList sensorControls); - [async] computeParams(uint32 frame); + [async] computeParams(uint32 frame, uint32 paramsBufferId); [async] processStats(uint32 frame, uint32 bufferId, libcamera.ControlList sensorControls); @@ -33,6 +33,6 @@ interface IPASoftInterface { interface IPASoftEventInterface { setSensorControls(libcamera.ControlList sensorControls); - paramsComputed(); + paramsComputed(uint32 paramsBufferId); metadataReady(uint32 frame, libcamera.ControlList metadata); }; diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index cfc1389e4..69bfef302 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -64,7 +64,8 @@ public: void stop() override; void queueRequest(const uint32_t frame, const ControlList &controls) override; - void computeParams(const uint32_t frame) override; + void computeParams(const uint32_t frame, + const uint32_t paramsBufferId) override; void processStats(const uint32_t frame, const uint32_t bufferId, const ControlList &sensorControls) override; @@ -283,7 +284,8 @@ void IPASoftSimple::queueRequest(const uint32_t frame, const ControlList &contro algo->queueRequest(context_, frame, frameContext, controls); } -void IPASoftSimple::computeParams(const uint32_t frame) +void IPASoftSimple::computeParams(const uint32_t frame, + const uint32_t paramsBufferId) { context_.activeState.combinedMatrix = Matrix<float, 3, 3>::identity(); @@ -292,7 +294,7 @@ void IPASoftSimple::computeParams(const uint32_t frame) algo->prepare(context_, frame, frameContext, params_); params_->combinedMatrix = context_.activeState.combinedMatrix; - paramsComputed.emit(); + paramsComputed.emit(paramsBufferId); } void IPASoftSimple::processStats(const uint32_t frame, diff --git a/src/libcamera/software_isp/debayer.cpp b/src/libcamera/software_isp/debayer.cpp index 2d7abfb83..7b1be52b2 100644 --- a/src/libcamera/software_isp/debayer.cpp +++ b/src/libcamera/software_isp/debayer.cpp @@ -104,9 +104,10 @@ Debayer::~Debayer() */ /** - * \fn void Debayer::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, DebayerParams params) + * \fn void Debayer::process(uint32_t frame, const uint32_t paramsBufferId, FrameBuffer *input, FrameBuffer *output, DebayerParams params) * \brief Process the bayer data into the requested format * \param[in] frame The frame number + * \param[in] paramsBufferId The id of the params buffer in use * \param[in] input The input buffer * \param[in] output The output buffer * \param[in] params The parameters to be used in debayering @@ -142,6 +143,11 @@ Debayer::~Debayer() * current stream. */ +/** + * \var Debayer::paramsBufferReady + * \brief Signals when the processing params are no longer needed + */ + /** * \var Signal<FrameBuffer *> Debayer::inputBufferReady * \brief Signals when the input buffer is ready diff --git a/src/libcamera/software_isp/debayer.h b/src/libcamera/software_isp/debayer.h index a2a17ec18..9a97851b7 100644 --- a/src/libcamera/software_isp/debayer.h +++ b/src/libcamera/software_isp/debayer.h @@ -47,7 +47,10 @@ public: virtual std::tuple<unsigned int, unsigned int> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) = 0; - virtual void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) = 0; + virtual void process(uint32_t frame, + const uint32_t paramsBufferId, + FrameBuffer *input, FrameBuffer *output, + const DebayerParams ¶ms) = 0; virtual int start() { return 0; } virtual void stop() {} @@ -59,6 +62,7 @@ public: Signal<FrameBuffer *> inputBufferReady; Signal<FrameBuffer *> outputBufferReady; + Signal<uint32_t> paramsBufferReady; struct DebayerInputConfig { Size patternSize; diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp index 1f9b24da0..51b58fce5 100644 --- a/src/libcamera/software_isp/debayer_cpu.cpp +++ b/src/libcamera/software_isp/debayer_cpu.cpp @@ -971,7 +971,9 @@ void DebayerCpu::updateLookupTables(const DebayerParams ¶ms) params_ = params; } -void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) +void DebayerCpu::process(uint32_t frame, const uint32_t paramsBufferId, + FrameBuffer *input, FrameBuffer *output, + const DebayerParams ¶ms) { bench_.startFrame(); @@ -981,6 +983,8 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output updateLookupTables(params); + paramsBufferReady.emit(paramsBufferId); + /* Copy metadata from the input buffer */ FrameMetadata &metadata = output->_d()->metadata(); metadata.status = input->metadata().status; diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h index 68da95083..1b4e8548a 100644 --- a/src/libcamera/software_isp/debayer_cpu.h +++ b/src/libcamera/software_isp/debayer_cpu.h @@ -42,7 +42,9 @@ public: std::vector<PixelFormat> formats(PixelFormat input) override; std::tuple<unsigned int, unsigned int> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) override; - void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) override; + void process(uint32_t frame, const uint32_t paramsBufferId, + FrameBuffer *input, FrameBuffer *output, + const DebayerParams ¶ms) override; int start() override; void stop() override; SizeRange sizes(PixelFormat inputFormat, const Size &inputSize) override; diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp index 99825d49e..60fd5463f 100644 --- a/src/libcamera/software_isp/debayer_egl.cpp +++ b/src/libcamera/software_isp/debayer_egl.cpp @@ -546,7 +546,9 @@ int DebayerEGL::debayerGPU(FrameBuffer *input, FrameBuffer *output, const Debaye return 0; } -void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) +void DebayerEGL::process(uint32_t frame, const uint32_t paramsBufferId, + FrameBuffer *input, FrameBuffer *output, + const DebayerParams ¶ms) { bench_.startFrame(); @@ -563,6 +565,7 @@ void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output LOG(Debayer, Error) << "debayerGPU failed"; goto error; } + paramsBufferReady.emit(paramsBufferId); metadata.planes()[0].bytesused = output->planes()[0].length; @@ -593,6 +596,7 @@ void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output return; error: + paramsBufferReady.emit(paramsBufferId); bench_.finishFrame(); metadata.status = FrameMetadata::FrameError; return; diff --git a/src/libcamera/software_isp/debayer_egl.h b/src/libcamera/software_isp/debayer_egl.h index 875e7cfc5..a3a4448e7 100644 --- a/src/libcamera/software_isp/debayer_egl.h +++ b/src/libcamera/software_isp/debayer_egl.h @@ -51,7 +51,9 @@ public: std::vector<PixelFormat> formats(PixelFormat input) override; std::tuple<unsigned int, unsigned int> strideAndFrameSize(const PixelFormat &outputFormat, const Size &size) override; - void process(uint32_t frame, FrameBuffer *input, FrameBuffer *output, const DebayerParams ¶ms) override; + void process(uint32_t frame, const uint32_t paramsBufferId, + FrameBuffer *input, FrameBuffer *output, + const DebayerParams ¶ms) override; int start() override; void stop() override; diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index f5b44f4e5..3d1dec21d 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -128,6 +128,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, debayer_->inputBufferReady.connect(this, &SoftwareIsp::inputReady); debayer_->outputBufferReady.connect(this, &SoftwareIsp::outputReady); + debayer_->paramsBufferReady.connect(this, &SoftwareIsp::paramsBufferReady); ipa_ = pipe->createIPA<ipa::soft::IPAProxySoft>(0, 0); if (!ipa_) { @@ -409,16 +410,23 @@ void SoftwareIsp::stop() */ void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output) { - ipa_->computeParams(frame); + /* \todo Provide a real value */ + constexpr uint32_t paramsBufferId = 0; + ipa_->computeParams(frame, paramsBufferId); debayer_->invokeMethod(&Debayer::process, - ConnectionTypeQueued, frame, input, output, debayerParams_); + ConnectionTypeQueued, frame, paramsBufferId, + input, output, debayerParams_); } -void SoftwareIsp::saveIspParams() +void SoftwareIsp::saveIspParams([[maybe_unused]] const uint32_t paramsBufferId) { debayerParams_ = *sharedParams_; } +void SoftwareIsp::paramsBufferReady([[maybe_unused]] const uint32_t paramsBufferId) +{ +} + void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls) { setSensorControls.emit(sensorControls);
Processing parameters in software ISP are currently passed by value. This is unlike hardware pipelines, which use a ring of buffers for the purpose and pass references to the buffers currently selected from the ring rather than passing the whole parameters buffers. This is a preparatory patch to introduce a similar mechanism in software ISP, in order to resolve TODO #5. It adds a new argument paramsBufferId for the future parameters buffer ids passed to the calls. The buffer ids must be passed to the following groups of methods: - IPASoftSimple::prepare, in order to create the parameters in the given buffer for the corresponding frame processing. - SoftwareIsp::saveIspParams, in order to signal that the parameters are set by the IPA. - Debayer::process, in order to get the buffer with the processing parameters. - SoftwareIsp::paramsBufferReady, in order to signal that the selected parameters buffer from the ring is no longer needed for the given frame and can be reused. This is a newly introduced signal. The type of the buffer id parameter is set to uint32_t because: - It can be used in mojom. - It is consistent with the similar types in the hardware pipelines. - It covers file descriptor number range, which will be used as buffer ids (more on this in followup patches). This patch doesn't do more than adding the arguments, to keep the patch simple. The buffer handling will be implemented in the followup patches. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- .../libcamera/internal/software_isp/software_isp.h | 3 ++- include/libcamera/ipa/soft.mojom | 4 ++-- src/ipa/simple/soft_simple.cpp | 8 +++++--- src/libcamera/software_isp/debayer.cpp | 8 +++++++- src/libcamera/software_isp/debayer.h | 6 +++++- src/libcamera/software_isp/debayer_cpu.cpp | 6 +++++- src/libcamera/software_isp/debayer_cpu.h | 4 +++- src/libcamera/software_isp/debayer_egl.cpp | 6 +++++- src/libcamera/software_isp/debayer_egl.h | 4 +++- src/libcamera/software_isp/software_isp.cpp | 14 +++++++++++--- 10 files changed, 48 insertions(+), 15 deletions(-)