[RFC,v2,04/14] libcamera: software_isp: Track unused parameters buffers

26173 diff mbox series

As a preparation for introducing a ring of parameters buffers, this
patch introduces tracking of parameters buffers available for use.

SofwareIsp::availableParams_ is a queue of ids of the buffers available
for use.  When a fresh parameter buffer is needed, its id is retrieved
from there and once the buffer is no longer needed, it's put back there,
in a method invoked by a signal.  This is similar to what the hardware
pipelines do.

If a parameters buffer is requested but there is no free available, it
is an erroneous situation.  To recover from it, we have currently no
better mechanism than to wait for an available buffer.  Simply returning
from the processing would mean the finishing signals are not called,
resulting in e.g. the input buffer not being returned.  This is
different from the hardware pipelines, which are structured somewhat
differently.

We use buffers' file descriptors as buffer ids.  The parameters buffers
will be shared with the IPA and debayering and we need their common
identification everywhere.  The buffer file descriptors are shared
unchanged so they can be used for the purpose, avoiding a need to pass a
special id->buffer mapping to the IPA and debayering on initialization.

Note that the parameters buffer id is still actually unused and the only
buffer currently used is still copied when passed to debayering.  This
patch is just preparation for followup patches that will introduce
multiple buffers.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 .../internal/software_isp/software_isp.h       |  2 ++
 src/libcamera/software_isp/software_isp.cpp    | 18 +++++++++++++++---
 2 files changed, 17 insertions(+), 3 deletions(-)

2026. 02. 16. 21:30 keltezéssel, Milan Zamazal írta:
> As a preparation for introducing a ring of parameters buffers, this
> patch introduces tracking of parameters buffers available for use.
> 
> SofwareIsp::availableParams_ is a queue of ids of the buffers available
> for use.  When a fresh parameter buffer is needed, its id is retrieved
> from there and once the buffer is no longer needed, it's put back there,
> in a method invoked by a signal.  This is similar to what the hardware
> pipelines do.
> 
> If a parameters buffer is requested but there is no free available, it
> is an erroneous situation.  To recover from it, we have currently no
> better mechanism than to wait for an available buffer.  Simply returning
> from the processing would mean the finishing signals are not called,
> resulting in e.g. the input buffer not being returned.  This is
> different from the hardware pipelines, which are structured somewhat
> differently.
> 
> We use buffers' file descriptors as buffer ids.  The parameters buffers

That is not going to work if the IPA is isolated. Even though this IPA
is expected not to be isolated, but:
   * it would be nice to not break it regardless,
   * some distributions meddle with the signatures (e.g. opensuse) so
     everything runs isolated there.


> will be shared with the IPA and debayering and we need their common
> identification everywhere.  The buffer file descriptors are shared
> unchanged so they can be used for the purpose, avoiding a need to pass a
> special id->buffer mapping to the IPA and debayering on initialization.
> 
> Note that the parameters buffer id is still actually unused and the only
> buffer currently used is still copied when passed to debayering.  This
> patch is just preparation for followup patches that will introduce
> multiple buffers.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>   .../internal/software_isp/software_isp.h       |  2 ++
>   src/libcamera/software_isp/software_isp.cpp    | 18 +++++++++++++++---
>   2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index dc6b6f7c1..a8b7b1eb1 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -11,6 +11,7 @@
>   #include <functional>
>   #include <map>
>   #include <memory>
> +#include <queue>
>   #include <stdint.h>
>   #include <string>
>   #include <tuple>
> @@ -98,6 +99,7 @@ private:
>   	Thread ispWorkerThread_;
>   	SharedMemObject<DebayerParams> sharedParams_;
>   	DebayerParams debayerParams_;
> +	std::queue<uint32_t> availableParams_;

If this does not require FIFO operation, then I'd use a simple vector + {push,pop}_back().


>   	bool allocateParamsBuffers();
>   	DmaBufAllocator dmaHeap_;
>   	bool ccmEnabled_;
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index dc0a8eb84..adf2a03ab 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -180,6 +180,10 @@ bool SoftwareIsp::allocateParamsBuffers()
>   		return false;
>   	}
>   
> +	ASSERT(sharedParams_.fd().get() >= 0);
> +	const uint32_t bufferId = sharedParams_.fd().get();
> +	availableParams_.push(bufferId);
> +
>   	return true;
>   }
>   
> @@ -404,8 +408,15 @@ void SoftwareIsp::stop()
>    */
>   void SoftwareIsp::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output)
>   {
> -	/* \todo Provide a real value */
> -	constexpr uint32_t paramsBufferId = 0;
> +	if (availableParams_.empty()) {
> +		LOG(SoftwareIsp, Error) << "Parameters buffer underrun";
> +		/* Well, busy loop, but this situation shouldn't normally happen. */
> +		while (availableParams_.empty())
> +			;

This is problematic because it blocks the current thread, so the signals are not processed,
hence the queue will stay empty. And the loop itself also violates https://en.cppreference.com/cpp/language/multithread#Progress_guarantee
since this is a loop with no observable behaviour.

`Thread::current()->eventDispatcher()->processEvents()` could be used to
enter the event loop recursively, but I think that's not a good approach.


Regards,
Barnabás Pőcze


> +	}
> +
> +	const uint32_t paramsBufferId = availableParams_.front();
> +	availableParams_.pop();
>   	ipa_->computeParams(frame, paramsBufferId);
>   	debayer_->invokeMethod(&Debayer::process,
>   			       ConnectionTypeQueued, frame, paramsBufferId, input, output, debayerParams_);
> @@ -416,8 +427,9 @@ void SoftwareIsp::saveIspParams([[maybe_unused]] uint32_t paramsBufferId)
>   	debayerParams_ = *sharedParams_;
>   }
>   
> -void SoftwareIsp::releaseIspParams([[maybe_unused]] uint32_t paramsBufferId)
> +void SoftwareIsp::releaseIspParams(uint32_t paramsBufferId)
>   {
> +	availableParams_.push(paramsBufferId);
>   }
>   
>   void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls)