[v2,5/8] pipelines: Use lambda functions to factor out buffer mapping code
diff mbox series

Message ID 20250815113400.20623-6-laurent.pinchart@ideasonboard.com
State Changes Requested
Headers show
Series
  • libcamera: Use span in FrameBuffer & assorted cleanups
Related show

Commit Message

Laurent Pinchart Aug. 15, 2025, 11:33 a.m. UTC
Multiple pipeline handlers duplicate code related to mapping params and
stats buffers to IPA modules. Factor out the code to lambda functions to
share it between the two buffer types.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp         | 16 +++++++------
 src/libcamera/pipeline/mali-c55/mali-c55.cpp | 24 ++++++++++----------
 src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 22 +++++++++---------
 3 files changed, 32 insertions(+), 30 deletions(-)

Comments

Barnabás Pőcze Aug. 19, 2025, 12:15 p.m. UTC | #1
Hi

2025. 08. 15. 13:33 keltezéssel, Laurent Pinchart írta:
> Multiple pipeline handlers duplicate code related to mapping params and
> stats buffers to IPA modules. Factor out the code to lambda functions to
> share it between the two buffer types.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   src/libcamera/pipeline/ipu3/ipu3.cpp         | 16 +++++++------
>   src/libcamera/pipeline/mali-c55/mali-c55.cpp | 24 ++++++++++----------
>   src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 22 +++++++++---------
>   3 files changed, 32 insertions(+), 30 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index ad20810e6a26..7ae85e5063db 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -678,15 +678,17 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
>   	/* Map buffers to the IPA. */
>   	unsigned int ipaBufferId = 1;
> 
> -	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_) {
> +	auto pushBuffer = [&](std::vector<IPABuffer> &buffers,
> +			      const std::unique_ptr<FrameBuffer> &buffer) {

I feel like the `buffers` argument is a bit unnecessary since it's only ever `ipaBuffers_`.


>   		buffer->setCookie(ipaBufferId++);
> -		ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
> -	}
> +		buffers.emplace_back(buffer->cookie(), buffer->planes());
> +	};
> 
> -	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_) {
> -		buffer->setCookie(ipaBufferId++);
> -		ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
> -	}
> +	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_)
> +		pushBuffer(ipaBuffers_, buffer);
> +
> +	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_)
> +		pushBuffer(ipaBuffers_, buffer);
> 
>   	data->ipa_->mapBuffers(ipaBuffers_);
> 
> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> index 76341ed3f363..f03a03fef35c 100644
> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> @@ -1133,27 +1133,27 @@ int PipelineHandlerMaliC55::allocateBuffers(Camera *camera)
>   		data->dsStream_.configuration().bufferCount,
>   	});
> 
> +	auto pushBuffer = [&](std::queue<FrameBuffer *> &queue,
> +			      std::vector<IPABuffer> &buffers,
> +			      const std::unique_ptr<FrameBuffer> &buffer) {
> +		buffer->setCookie(ipaBufferId++);
> +		buffers.emplace_back(buffer->cookie(), buffer->planes());
> +		queue.push(buffer.get());
> +	};
> +
>   	ret = stats_->allocateBuffers(bufferCount, &statsBuffers_);
>   	if (ret < 0)
>   		return ret;
> 
> -	for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_) {
> -		buffer->setCookie(ipaBufferId++);
> -		data->ipaStatBuffers_.emplace_back(buffer->cookie(),
> -						   buffer->planes());
> -		availableStatsBuffers_.push(buffer.get());
> -	}
> +	for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_)
> +		pushBuffer(availableStatsBuffers_, data->ipaStatBuffers_, buffer);
> 
>   	ret = params_->allocateBuffers(bufferCount, &paramsBuffers_);
>   	if (ret < 0)
>   		return ret;
> 
> -	for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_) {
> -		buffer->setCookie(ipaBufferId++);
> -		data->ipaParamBuffers_.emplace_back(buffer->cookie(),
> -						    buffer->planes());
> -		availableParamsBuffers_.push(buffer.get());
> -	}
> +	for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_)
> +		pushBuffer(availableParamsBuffers_, data->ipaParamBuffers_, buffer);
> 
>   	if (data->ipa_) {
>   		data->ipa_->mapBuffers(data->ipaStatBuffers_, true);
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 55d7d4442caf..99347c9f6258 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -1028,19 +1028,19 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
>   			availableMainPathBuffers_.push(buffer.get());
>   	}
> 
> -	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) {
> +	auto pushBuffer = [&](std::vector<IPABuffer> &buffers,
> +			      std::queue<FrameBuffer *> &queue,
> +			      const std::unique_ptr<FrameBuffer> &buffer) {

Same here.

Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>


>   		buffer->setCookie(ipaBufferId++);
> -		data->ipaBuffers_.emplace_back(buffer->cookie(),
> -					       buffer->planes());
> -		availableParamBuffers_.push(buffer.get());
> -	}
> +		buffers.emplace_back(buffer->cookie(), buffer->planes());
> +		queue.push(buffer.get());
> +	};
> 
> -	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_) {
> -		buffer->setCookie(ipaBufferId++);
> -		data->ipaBuffers_.emplace_back(buffer->cookie(),
> -					       buffer->planes());
> -		availableStatBuffers_.push(buffer.get());
> -	}
> +	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_)
> +		pushBuffer(data->ipaBuffers_, availableParamBuffers_, buffer);
> +
> +	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_)
> +		pushBuffer(data->ipaBuffers_, availableStatBuffers_, buffer);
> 
>   	data->ipa_->mapBuffers(data->ipaBuffers_);
> 
> --
> Regards,
> 
> Laurent Pinchart
>
Laurent Pinchart Aug. 19, 2025, 12:37 p.m. UTC | #2
On Tue, Aug 19, 2025 at 02:15:44PM +0200, Barnabás Pőcze wrote:
> 2025. 08. 15. 13:33 keltezéssel, Laurent Pinchart írta:
> > Multiple pipeline handlers duplicate code related to mapping params and
> > stats buffers to IPA modules. Factor out the code to lambda functions to
> > share it between the two buffer types.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   src/libcamera/pipeline/ipu3/ipu3.cpp         | 16 +++++++------
> >   src/libcamera/pipeline/mali-c55/mali-c55.cpp | 24 ++++++++++----------
> >   src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 22 +++++++++---------
> >   3 files changed, 32 insertions(+), 30 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index ad20810e6a26..7ae85e5063db 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -678,15 +678,17 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
> >   	/* Map buffers to the IPA. */
> >   	unsigned int ipaBufferId = 1;
> > 
> > -	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_) {
> > +	auto pushBuffer = [&](std::vector<IPABuffer> &buffers,
> > +			      const std::unique_ptr<FrameBuffer> &buffer) {
> 
> I feel like the `buffers` argument is a bit unnecessary since it's
> only ever `ipaBuffers_`.

You know, one of the reasons I like your reviews is because you point
out things that I wasn't sure about myself when sending patches :-) I've
gone back and forth on this, and decided to add the buffers argument for
consistency between pipeline handlers. If you think it's best to drop
it, I'll do that. What's your preference ?

> >   		buffer->setCookie(ipaBufferId++);
> > -		ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
> > -	}
> > +		buffers.emplace_back(buffer->cookie(), buffer->planes());
> > +	};
> > 
> > -	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_) {
> > -		buffer->setCookie(ipaBufferId++);
> > -		ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
> > -	}
> > +	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_)
> > +		pushBuffer(ipaBuffers_, buffer);
> > +
> > +	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_)
> > +		pushBuffer(ipaBuffers_, buffer);
> > 
> >   	data->ipa_->mapBuffers(ipaBuffers_);
> > 
> > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > index 76341ed3f363..f03a03fef35c 100644
> > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > @@ -1133,27 +1133,27 @@ int PipelineHandlerMaliC55::allocateBuffers(Camera *camera)
> >   		data->dsStream_.configuration().bufferCount,
> >   	});
> > 
> > +	auto pushBuffer = [&](std::queue<FrameBuffer *> &queue,
> > +			      std::vector<IPABuffer> &buffers,
> > +			      const std::unique_ptr<FrameBuffer> &buffer) {
> > +		buffer->setCookie(ipaBufferId++);
> > +		buffers.emplace_back(buffer->cookie(), buffer->planes());
> > +		queue.push(buffer.get());
> > +	};
> > +
> >   	ret = stats_->allocateBuffers(bufferCount, &statsBuffers_);
> >   	if (ret < 0)
> >   		return ret;
> > 
> > -	for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_) {
> > -		buffer->setCookie(ipaBufferId++);
> > -		data->ipaStatBuffers_.emplace_back(buffer->cookie(),
> > -						   buffer->planes());
> > -		availableStatsBuffers_.push(buffer.get());
> > -	}
> > +	for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_)
> > +		pushBuffer(availableStatsBuffers_, data->ipaStatBuffers_, buffer);
> > 
> >   	ret = params_->allocateBuffers(bufferCount, &paramsBuffers_);
> >   	if (ret < 0)
> >   		return ret;
> > 
> > -	for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_) {
> > -		buffer->setCookie(ipaBufferId++);
> > -		data->ipaParamBuffers_.emplace_back(buffer->cookie(),
> > -						    buffer->planes());
> > -		availableParamsBuffers_.push(buffer.get());
> > -	}
> > +	for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_)
> > +		pushBuffer(availableParamsBuffers_, data->ipaParamBuffers_, buffer);
> > 
> >   	if (data->ipa_) {
> >   		data->ipa_->mapBuffers(data->ipaStatBuffers_, true);
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 55d7d4442caf..99347c9f6258 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -1028,19 +1028,19 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> >   			availableMainPathBuffers_.push(buffer.get());
> >   	}
> > 
> > -	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) {
> > +	auto pushBuffer = [&](std::vector<IPABuffer> &buffers,
> > +			      std::queue<FrameBuffer *> &queue,
> > +			      const std::unique_ptr<FrameBuffer> &buffer) {
> 
> Same here.
> 
> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> 
> >   		buffer->setCookie(ipaBufferId++);
> > -		data->ipaBuffers_.emplace_back(buffer->cookie(),
> > -					       buffer->planes());
> > -		availableParamBuffers_.push(buffer.get());
> > -	}
> > +		buffers.emplace_back(buffer->cookie(), buffer->planes());
> > +		queue.push(buffer.get());
> > +	};
> > 
> > -	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_) {
> > -		buffer->setCookie(ipaBufferId++);
> > -		data->ipaBuffers_.emplace_back(buffer->cookie(),
> > -					       buffer->planes());
> > -		availableStatBuffers_.push(buffer.get());
> > -	}
> > +	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_)
> > +		pushBuffer(data->ipaBuffers_, availableParamBuffers_, buffer);
> > +
> > +	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_)
> > +		pushBuffer(data->ipaBuffers_, availableStatBuffers_, buffer);
> > 
> >   	data->ipa_->mapBuffers(data->ipaBuffers_);
> >
Barnabás Pőcze Aug. 19, 2025, 1:01 p.m. UTC | #3
2025. 08. 19. 14:37 keltezéssel, Laurent Pinchart írta:
> On Tue, Aug 19, 2025 at 02:15:44PM +0200, Barnabás Pőcze wrote:
>> 2025. 08. 15. 13:33 keltezéssel, Laurent Pinchart írta:
>>> Multiple pipeline handlers duplicate code related to mapping params and
>>> stats buffers to IPA modules. Factor out the code to lambda functions to
>>> share it between the two buffer types.
>>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>    src/libcamera/pipeline/ipu3/ipu3.cpp         | 16 +++++++------
>>>    src/libcamera/pipeline/mali-c55/mali-c55.cpp | 24 ++++++++++----------
>>>    src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 22 +++++++++---------
>>>    3 files changed, 32 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> index ad20810e6a26..7ae85e5063db 100644
>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> @@ -678,15 +678,17 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
>>>    	/* Map buffers to the IPA. */
>>>    	unsigned int ipaBufferId = 1;
>>>
>>> -	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_) {
>>> +	auto pushBuffer = [&](std::vector<IPABuffer> &buffers,
>>> +			      const std::unique_ptr<FrameBuffer> &buffer) {
>>
>> I feel like the `buffers` argument is a bit unnecessary since it's
>> only ever `ipaBuffers_`.
> 
> You know, one of the reasons I like your reviews is because you point
> out things that I wasn't sure about myself when sending patches :-) I've
> gone back and forth on this, and decided to add the buffers argument for
> consistency between pipeline handlers. If you think it's best to drop
> it, I'll do that. What's your preference ?

My thinking is that it's highly specific, so might as well go for the simplest
option. But I am not sure, I don't think it matters much in the end.


> 
>>>    		buffer->setCookie(ipaBufferId++);
>>> -		ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
>>> -	}
>>> +		buffers.emplace_back(buffer->cookie(), buffer->planes());
>>> +	};
>>>
>>> -	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_) {
>>> -		buffer->setCookie(ipaBufferId++);
>>> -		ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
>>> -	}
>>> +	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_)
>>> +		pushBuffer(ipaBuffers_, buffer);
>>> +
>>> +	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_)
>>> +		pushBuffer(ipaBuffers_, buffer);
>>>
>>>    	data->ipa_->mapBuffers(ipaBuffers_);
>>>
>>> diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>>> index 76341ed3f363..f03a03fef35c 100644
>>> --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>>> +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
>>> @@ -1133,27 +1133,27 @@ int PipelineHandlerMaliC55::allocateBuffers(Camera *camera)
>>>    		data->dsStream_.configuration().bufferCount,
>>>    	});
>>>
>>> +	auto pushBuffer = [&](std::queue<FrameBuffer *> &queue,
>>> +			      std::vector<IPABuffer> &buffers,
>>> +			      const std::unique_ptr<FrameBuffer> &buffer) {
>>> +		buffer->setCookie(ipaBufferId++);
>>> +		buffers.emplace_back(buffer->cookie(), buffer->planes());
>>> +		queue.push(buffer.get());
>>> +	};
>>> +
>>>    	ret = stats_->allocateBuffers(bufferCount, &statsBuffers_);
>>>    	if (ret < 0)
>>>    		return ret;
>>>
>>> -	for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_) {
>>> -		buffer->setCookie(ipaBufferId++);
>>> -		data->ipaStatBuffers_.emplace_back(buffer->cookie(),
>>> -						   buffer->planes());
>>> -		availableStatsBuffers_.push(buffer.get());
>>> -	}
>>> +	for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_)
>>> +		pushBuffer(availableStatsBuffers_, data->ipaStatBuffers_, buffer);
>>>
>>>    	ret = params_->allocateBuffers(bufferCount, &paramsBuffers_);
>>>    	if (ret < 0)
>>>    		return ret;
>>>
>>> -	for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_) {
>>> -		buffer->setCookie(ipaBufferId++);
>>> -		data->ipaParamBuffers_.emplace_back(buffer->cookie(),
>>> -						    buffer->planes());
>>> -		availableParamsBuffers_.push(buffer.get());
>>> -	}
>>> +	for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_)
>>> +		pushBuffer(availableParamsBuffers_, data->ipaParamBuffers_, buffer);
>>>
>>>    	if (data->ipa_) {
>>>    		data->ipa_->mapBuffers(data->ipaStatBuffers_, true);
>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> index 55d7d4442caf..99347c9f6258 100644
>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> @@ -1028,19 +1028,19 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
>>>    			availableMainPathBuffers_.push(buffer.get());
>>>    	}
>>>
>>> -	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) {
>>> +	auto pushBuffer = [&](std::vector<IPABuffer> &buffers,
>>> +			      std::queue<FrameBuffer *> &queue,
>>> +			      const std::unique_ptr<FrameBuffer> &buffer) {
>>
>> Same here.
>>
>> Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>
>>>    		buffer->setCookie(ipaBufferId++);
>>> -		data->ipaBuffers_.emplace_back(buffer->cookie(),
>>> -					       buffer->planes());
>>> -		availableParamBuffers_.push(buffer.get());
>>> -	}
>>> +		buffers.emplace_back(buffer->cookie(), buffer->planes());
>>> +		queue.push(buffer.get());
>>> +	};
>>>
>>> -	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_) {
>>> -		buffer->setCookie(ipaBufferId++);
>>> -		data->ipaBuffers_.emplace_back(buffer->cookie(),
>>> -					       buffer->planes());
>>> -		availableStatBuffers_.push(buffer.get());
>>> -	}
>>> +	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_)
>>> +		pushBuffer(data->ipaBuffers_, availableParamBuffers_, buffer);
>>> +
>>> +	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_)
>>> +		pushBuffer(data->ipaBuffers_, availableStatBuffers_, buffer);
>>>
>>>    	data->ipa_->mapBuffers(data->ipaBuffers_);
>>>
>
Jacopo Mondi Aug. 19, 2025, 3:31 p.m. UTC | #4
Hi

On Tue, Aug 19, 2025 at 03:01:50PM +0200, Barnabás Pőcze wrote:
> 2025. 08. 19. 14:37 keltezéssel, Laurent Pinchart írta:
> > On Tue, Aug 19, 2025 at 02:15:44PM +0200, Barnabás Pőcze wrote:
> > > 2025. 08. 15. 13:33 keltezéssel, Laurent Pinchart írta:
> > > > Multiple pipeline handlers duplicate code related to mapping params and
> > > > stats buffers to IPA modules. Factor out the code to lambda functions to
> > > > share it between the two buffer types.
> > > >
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >    src/libcamera/pipeline/ipu3/ipu3.cpp         | 16 +++++++------
> > > >    src/libcamera/pipeline/mali-c55/mali-c55.cpp | 24 ++++++++++----------
> > > >    src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 22 +++++++++---------
> > > >    3 files changed, 32 insertions(+), 30 deletions(-)
> > > >
> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > index ad20810e6a26..7ae85e5063db 100644
> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > @@ -678,15 +678,17 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
> > > >    	/* Map buffers to the IPA. */
> > > >    	unsigned int ipaBufferId = 1;
> > > >
> > > > -	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_) {
> > > > +	auto pushBuffer = [&](std::vector<IPABuffer> &buffers,
> > > > +			      const std::unique_ptr<FrameBuffer> &buffer) {
> > >
> > > I feel like the `buffers` argument is a bit unnecessary since it's
> > > only ever `ipaBuffers_`.
> >

Or you can pass the argument but avoid the default capture ? But then
you won't be able to access ipaBufferId.. (yes, you could capture that
one only, but I'm not sure all of this is worth it ?)


> > You know, one of the reasons I like your reviews is because you point
> > out things that I wasn't sure about myself when sending patches :-) I've
> > gone back and forth on this, and decided to add the buffers argument for
> > consistency between pipeline handlers. If you think it's best to drop
> > it, I'll do that. What's your preference ?
>
> My thinking is that it's highly specific, so might as well go for the simplest
> option. But I am not sure, I don't think it matters much in the end.
>
>
> >
> > > >    		buffer->setCookie(ipaBufferId++);
> > > > -		ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
> > > > -	}
> > > > +		buffers.emplace_back(buffer->cookie(), buffer->planes());
> > > > +	};
> > > >
> > > > -	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_) {

I wonder if you could pass 'imgu->statsBuffers_' instead and for-loop
in the lambda..

Anyways
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

> > > > -		buffer->setCookie(ipaBufferId++);
> > > > -		ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
> > > > -	}
> > > > +	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_)
> > > > +		pushBuffer(ipaBuffers_, buffer);
> > > > +
> > > > +	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_)
> > > > +		pushBuffer(ipaBuffers_, buffer);
> > > >
> > > >    	data->ipa_->mapBuffers(ipaBuffers_);
> > > >
> > > > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > > index 76341ed3f363..f03a03fef35c 100644
> > > > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > > @@ -1133,27 +1133,27 @@ int PipelineHandlerMaliC55::allocateBuffers(Camera *camera)
> > > >    		data->dsStream_.configuration().bufferCount,
> > > >    	});
> > > >
> > > > +	auto pushBuffer = [&](std::queue<FrameBuffer *> &queue,
> > > > +			      std::vector<IPABuffer> &buffers,
> > > > +			      const std::unique_ptr<FrameBuffer> &buffer) {
> > > > +		buffer->setCookie(ipaBufferId++);
> > > > +		buffers.emplace_back(buffer->cookie(), buffer->planes());
> > > > +		queue.push(buffer.get());
> > > > +	};
> > > > +
> > > >    	ret = stats_->allocateBuffers(bufferCount, &statsBuffers_);
> > > >    	if (ret < 0)
> > > >    		return ret;
> > > >
> > > > -	for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_) {
> > > > -		buffer->setCookie(ipaBufferId++);
> > > > -		data->ipaStatBuffers_.emplace_back(buffer->cookie(),
> > > > -						   buffer->planes());
> > > > -		availableStatsBuffers_.push(buffer.get());
> > > > -	}
> > > > +	for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_)
> > > > +		pushBuffer(availableStatsBuffers_, data->ipaStatBuffers_, buffer);
> > > >
> > > >    	ret = params_->allocateBuffers(bufferCount, &paramsBuffers_);
> > > >    	if (ret < 0)
> > > >    		return ret;
> > > >
> > > > -	for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_) {
> > > > -		buffer->setCookie(ipaBufferId++);
> > > > -		data->ipaParamBuffers_.emplace_back(buffer->cookie(),
> > > > -						    buffer->planes());
> > > > -		availableParamsBuffers_.push(buffer.get());
> > > > -	}
> > > > +	for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_)
> > > > +		pushBuffer(availableParamsBuffers_, data->ipaParamBuffers_, buffer);
> > > >
> > > >    	if (data->ipa_) {
> > > >    		data->ipa_->mapBuffers(data->ipaStatBuffers_, true);
> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > index 55d7d4442caf..99347c9f6258 100644
> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > @@ -1028,19 +1028,19 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> > > >    			availableMainPathBuffers_.push(buffer.get());
> > > >    	}
> > > >
> > > > -	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) {
> > > > +	auto pushBuffer = [&](std::vector<IPABuffer> &buffers,
> > > > +			      std::queue<FrameBuffer *> &queue,
> > > > +			      const std::unique_ptr<FrameBuffer> &buffer) {
> > >
> > > Same here.
> > >
> > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > >
> > > >    		buffer->setCookie(ipaBufferId++);
> > > > -		data->ipaBuffers_.emplace_back(buffer->cookie(),
> > > > -					       buffer->planes());
> > > > -		availableParamBuffers_.push(buffer.get());
> > > > -	}
> > > > +		buffers.emplace_back(buffer->cookie(), buffer->planes());
> > > > +		queue.push(buffer.get());
> > > > +	};
> > > >
> > > > -	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_) {
> > > > -		buffer->setCookie(ipaBufferId++);
> > > > -		data->ipaBuffers_.emplace_back(buffer->cookie(),
> > > > -					       buffer->planes());
> > > > -		availableStatBuffers_.push(buffer.get());
> > > > -	}
> > > > +	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_)
> > > > +		pushBuffer(data->ipaBuffers_, availableParamBuffers_, buffer);
> > > > +
> > > > +	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_)
> > > > +		pushBuffer(data->ipaBuffers_, availableStatBuffers_, buffer);
> > > >
> > > >    	data->ipa_->mapBuffers(data->ipaBuffers_);
> > > >
> >
>
Laurent Pinchart Aug. 19, 2025, 4:09 p.m. UTC | #5
On Tue, Aug 19, 2025 at 05:31:59PM +0200, Jacopo Mondi wrote:
> On Tue, Aug 19, 2025 at 03:01:50PM +0200, Barnabás Pőcze wrote:
> > 2025. 08. 19. 14:37 keltezéssel, Laurent Pinchart írta:
> > > On Tue, Aug 19, 2025 at 02:15:44PM +0200, Barnabás Pőcze wrote:
> > > > 2025. 08. 15. 13:33 keltezéssel, Laurent Pinchart írta:
> > > > > Multiple pipeline handlers duplicate code related to mapping params and
> > > > > stats buffers to IPA modules. Factor out the code to lambda functions to
> > > > > share it between the two buffer types.
> > > > >
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > ---
> > > > >    src/libcamera/pipeline/ipu3/ipu3.cpp         | 16 +++++++------
> > > > >    src/libcamera/pipeline/mali-c55/mali-c55.cpp | 24 ++++++++++----------
> > > > >    src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 22 +++++++++---------
> > > > >    3 files changed, 32 insertions(+), 30 deletions(-)
> > > > >
> > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > index ad20810e6a26..7ae85e5063db 100644
> > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > @@ -678,15 +678,17 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
> > > > >    	/* Map buffers to the IPA. */
> > > > >    	unsigned int ipaBufferId = 1;
> > > > >
> > > > > -	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_) {
> > > > > +	auto pushBuffer = [&](std::vector<IPABuffer> &buffers,
> > > > > +			      const std::unique_ptr<FrameBuffer> &buffer) {
> > > >
> > > > I feel like the `buffers` argument is a bit unnecessary since it's
> > > > only ever `ipaBuffers_`.
> 
> Or you can pass the argument but avoid the default capture ? But then
> you won't be able to access ipaBufferId.. (yes, you could capture that
> one only, but I'm not sure all of this is worth it ?)
> 
> > > You know, one of the reasons I like your reviews is because you point
> > > out things that I wasn't sure about myself when sending patches :-) I've
> > > gone back and forth on this, and decided to add the buffers argument for
> > > consistency between pipeline handlers. If you think it's best to drop
> > > it, I'll do that. What's your preference ?
> >
> > My thinking is that it's highly specific, so might as well go for the simplest
> > option. But I am not sure, I don't think it matters much in the end.
> >
> > > > >    		buffer->setCookie(ipaBufferId++);
> > > > > -		ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
> > > > > -	}
> > > > > +		buffers.emplace_back(buffer->cookie(), buffer->planes());
> > > > > +	};
> > > > >
> > > > > -	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_) {
> 
> I wonder if you could pass 'imgu->statsBuffers_' instead and for-loop
> in the lambda..

Interesting idea. I'll post of a v2 of this patch, you and Barnabás can
decide what you like best.

> Anyways
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> > > > > -		buffer->setCookie(ipaBufferId++);
> > > > > -		ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
> > > > > -	}
> > > > > +	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_)
> > > > > +		pushBuffer(ipaBuffers_, buffer);
> > > > > +
> > > > > +	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_)
> > > > > +		pushBuffer(ipaBuffers_, buffer);
> > > > >
> > > > >    	data->ipa_->mapBuffers(ipaBuffers_);
> > > > >
> > > > > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > > > index 76341ed3f363..f03a03fef35c 100644
> > > > > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > > > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > > > @@ -1133,27 +1133,27 @@ int PipelineHandlerMaliC55::allocateBuffers(Camera *camera)
> > > > >    		data->dsStream_.configuration().bufferCount,
> > > > >    	});
> > > > >
> > > > > +	auto pushBuffer = [&](std::queue<FrameBuffer *> &queue,
> > > > > +			      std::vector<IPABuffer> &buffers,
> > > > > +			      const std::unique_ptr<FrameBuffer> &buffer) {
> > > > > +		buffer->setCookie(ipaBufferId++);
> > > > > +		buffers.emplace_back(buffer->cookie(), buffer->planes());
> > > > > +		queue.push(buffer.get());
> > > > > +	};
> > > > > +
> > > > >    	ret = stats_->allocateBuffers(bufferCount, &statsBuffers_);
> > > > >    	if (ret < 0)
> > > > >    		return ret;
> > > > >
> > > > > -	for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_) {
> > > > > -		buffer->setCookie(ipaBufferId++);
> > > > > -		data->ipaStatBuffers_.emplace_back(buffer->cookie(),
> > > > > -						   buffer->planes());
> > > > > -		availableStatsBuffers_.push(buffer.get());
> > > > > -	}
> > > > > +	for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_)
> > > > > +		pushBuffer(availableStatsBuffers_, data->ipaStatBuffers_, buffer);
> > > > >
> > > > >    	ret = params_->allocateBuffers(bufferCount, &paramsBuffers_);
> > > > >    	if (ret < 0)
> > > > >    		return ret;
> > > > >
> > > > > -	for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_) {
> > > > > -		buffer->setCookie(ipaBufferId++);
> > > > > -		data->ipaParamBuffers_.emplace_back(buffer->cookie(),
> > > > > -						    buffer->planes());
> > > > > -		availableParamsBuffers_.push(buffer.get());
> > > > > -	}
> > > > > +	for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_)
> > > > > +		pushBuffer(availableParamsBuffers_, data->ipaParamBuffers_, buffer);
> > > > >
> > > > >    	if (data->ipa_) {
> > > > >    		data->ipa_->mapBuffers(data->ipaStatBuffers_, true);
> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > index 55d7d4442caf..99347c9f6258 100644
> > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > @@ -1028,19 +1028,19 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> > > > >    			availableMainPathBuffers_.push(buffer.get());
> > > > >    	}
> > > > >
> > > > > -	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) {
> > > > > +	auto pushBuffer = [&](std::vector<IPABuffer> &buffers,
> > > > > +			      std::queue<FrameBuffer *> &queue,
> > > > > +			      const std::unique_ptr<FrameBuffer> &buffer) {
> > > >
> > > > Same here.
> > > >
> > > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > >
> > > > >    		buffer->setCookie(ipaBufferId++);
> > > > > -		data->ipaBuffers_.emplace_back(buffer->cookie(),
> > > > > -					       buffer->planes());
> > > > > -		availableParamBuffers_.push(buffer.get());
> > > > > -	}
> > > > > +		buffers.emplace_back(buffer->cookie(), buffer->planes());
> > > > > +		queue.push(buffer.get());
> > > > > +	};
> > > > >
> > > > > -	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_) {
> > > > > -		buffer->setCookie(ipaBufferId++);
> > > > > -		data->ipaBuffers_.emplace_back(buffer->cookie(),
> > > > > -					       buffer->planes());
> > > > > -		availableStatBuffers_.push(buffer.get());
> > > > > -	}
> > > > > +	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_)
> > > > > +		pushBuffer(data->ipaBuffers_, availableParamBuffers_, buffer);
> > > > > +
> > > > > +	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_)
> > > > > +		pushBuffer(data->ipaBuffers_, availableStatBuffers_, buffer);
> > > > >
> > > > >    	data->ipa_->mapBuffers(data->ipaBuffers_);
> > > > >
Jacopo Mondi Aug. 19, 2025, 6 p.m. UTC | #6
On Tue, Aug 19, 2025 at 07:09:03PM +0300, Laurent Pinchart wrote:
> On Tue, Aug 19, 2025 at 05:31:59PM +0200, Jacopo Mondi wrote:
> > On Tue, Aug 19, 2025 at 03:01:50PM +0200, Barnabás Pőcze wrote:
> > > 2025. 08. 19. 14:37 keltezéssel, Laurent Pinchart írta:
> > > > On Tue, Aug 19, 2025 at 02:15:44PM +0200, Barnabás Pőcze wrote:
> > > > > 2025. 08. 15. 13:33 keltezéssel, Laurent Pinchart írta:
> > > > > > Multiple pipeline handlers duplicate code related to mapping params and
> > > > > > stats buffers to IPA modules. Factor out the code to lambda functions to
> > > > > > share it between the two buffer types.
> > > > > >
> > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > > ---
> > > > > >    src/libcamera/pipeline/ipu3/ipu3.cpp         | 16 +++++++------
> > > > > >    src/libcamera/pipeline/mali-c55/mali-c55.cpp | 24 ++++++++++----------
> > > > > >    src/libcamera/pipeline/rkisp1/rkisp1.cpp     | 22 +++++++++---------
> > > > > >    3 files changed, 32 insertions(+), 30 deletions(-)
> > > > > >
> > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > > index ad20810e6a26..7ae85e5063db 100644
> > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > > @@ -678,15 +678,17 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
> > > > > >    	/* Map buffers to the IPA. */
> > > > > >    	unsigned int ipaBufferId = 1;
> > > > > >
> > > > > > -	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_) {
> > > > > > +	auto pushBuffer = [&](std::vector<IPABuffer> &buffers,
> > > > > > +			      const std::unique_ptr<FrameBuffer> &buffer) {
> > > > >
> > > > > I feel like the `buffers` argument is a bit unnecessary since it's
> > > > > only ever `ipaBuffers_`.
> >
> > Or you can pass the argument but avoid the default capture ? But then
> > you won't be able to access ipaBufferId.. (yes, you could capture that
> > one only, but I'm not sure all of this is worth it ?)
> >
> > > > You know, one of the reasons I like your reviews is because you point
> > > > out things that I wasn't sure about myself when sending patches :-) I've
> > > > gone back and forth on this, and decided to add the buffers argument for
> > > > consistency between pipeline handlers. If you think it's best to drop
> > > > it, I'll do that. What's your preference ?
> > >
> > > My thinking is that it's highly specific, so might as well go for the simplest
> > > option. But I am not sure, I don't think it matters much in the end.
> > >
> > > > > >    		buffer->setCookie(ipaBufferId++);
> > > > > > -		ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
> > > > > > -	}
> > > > > > +		buffers.emplace_back(buffer->cookie(), buffer->planes());
> > > > > > +	};
> > > > > >
> > > > > > -	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_) {
> >
> > I wonder if you could pass 'imgu->statsBuffers_' instead and for-loop
> > in the lambda..
>
> Interesting idea. I'll post of a v2 of this patch, you and Barnabás can
> decide what you like best.

Only if you really prefer, I think it doesn't make much difference :)

>
> > Anyways
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > > > > > -		buffer->setCookie(ipaBufferId++);
> > > > > > -		ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
> > > > > > -	}
> > > > > > +	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_)
> > > > > > +		pushBuffer(ipaBuffers_, buffer);
> > > > > > +
> > > > > > +	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_)
> > > > > > +		pushBuffer(ipaBuffers_, buffer);
> > > > > >
> > > > > >    	data->ipa_->mapBuffers(ipaBuffers_);
> > > > > >
> > > > > > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > > > > index 76341ed3f363..f03a03fef35c 100644
> > > > > > --- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > > > > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > > > > > @@ -1133,27 +1133,27 @@ int PipelineHandlerMaliC55::allocateBuffers(Camera *camera)
> > > > > >    		data->dsStream_.configuration().bufferCount,
> > > > > >    	});
> > > > > >
> > > > > > +	auto pushBuffer = [&](std::queue<FrameBuffer *> &queue,
> > > > > > +			      std::vector<IPABuffer> &buffers,
> > > > > > +			      const std::unique_ptr<FrameBuffer> &buffer) {
> > > > > > +		buffer->setCookie(ipaBufferId++);
> > > > > > +		buffers.emplace_back(buffer->cookie(), buffer->planes());
> > > > > > +		queue.push(buffer.get());
> > > > > > +	};
> > > > > > +
> > > > > >    	ret = stats_->allocateBuffers(bufferCount, &statsBuffers_);
> > > > > >    	if (ret < 0)
> > > > > >    		return ret;
> > > > > >
> > > > > > -	for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_) {
> > > > > > -		buffer->setCookie(ipaBufferId++);
> > > > > > -		data->ipaStatBuffers_.emplace_back(buffer->cookie(),
> > > > > > -						   buffer->planes());
> > > > > > -		availableStatsBuffers_.push(buffer.get());
> > > > > > -	}
> > > > > > +	for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_)
> > > > > > +		pushBuffer(availableStatsBuffers_, data->ipaStatBuffers_, buffer);
> > > > > >
> > > > > >    	ret = params_->allocateBuffers(bufferCount, &paramsBuffers_);
> > > > > >    	if (ret < 0)
> > > > > >    		return ret;
> > > > > >
> > > > > > -	for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_) {
> > > > > > -		buffer->setCookie(ipaBufferId++);
> > > > > > -		data->ipaParamBuffers_.emplace_back(buffer->cookie(),
> > > > > > -						    buffer->planes());
> > > > > > -		availableParamsBuffers_.push(buffer.get());
> > > > > > -	}
> > > > > > +	for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_)
> > > > > > +		pushBuffer(availableParamsBuffers_, data->ipaParamBuffers_, buffer);
> > > > > >
> > > > > >    	if (data->ipa_) {
> > > > > >    		data->ipa_->mapBuffers(data->ipaStatBuffers_, true);
> > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > index 55d7d4442caf..99347c9f6258 100644
> > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > @@ -1028,19 +1028,19 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> > > > > >    			availableMainPathBuffers_.push(buffer.get());
> > > > > >    	}
> > > > > >
> > > > > > -	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) {
> > > > > > +	auto pushBuffer = [&](std::vector<IPABuffer> &buffers,
> > > > > > +			      std::queue<FrameBuffer *> &queue,
> > > > > > +			      const std::unique_ptr<FrameBuffer> &buffer) {
> > > > >
> > > > > Same here.
> > > > >
> > > > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > > > >
> > > > > >    		buffer->setCookie(ipaBufferId++);
> > > > > > -		data->ipaBuffers_.emplace_back(buffer->cookie(),
> > > > > > -					       buffer->planes());
> > > > > > -		availableParamBuffers_.push(buffer.get());
> > > > > > -	}
> > > > > > +		buffers.emplace_back(buffer->cookie(), buffer->planes());
> > > > > > +		queue.push(buffer.get());
> > > > > > +	};
> > > > > >
> > > > > > -	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_) {
> > > > > > -		buffer->setCookie(ipaBufferId++);
> > > > > > -		data->ipaBuffers_.emplace_back(buffer->cookie(),
> > > > > > -					       buffer->planes());
> > > > > > -		availableStatBuffers_.push(buffer.get());
> > > > > > -	}
> > > > > > +	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_)
> > > > > > +		pushBuffer(data->ipaBuffers_, availableParamBuffers_, buffer);
> > > > > > +
> > > > > > +	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_)
> > > > > > +		pushBuffer(data->ipaBuffers_, availableStatBuffers_, buffer);
> > > > > >
> > > > > >    	data->ipa_->mapBuffers(data->ipaBuffers_);
> > > > > >
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index ad20810e6a26..7ae85e5063db 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -678,15 +678,17 @@  int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
 	/* Map buffers to the IPA. */
 	unsigned int ipaBufferId = 1;
 
-	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_) {
+	auto pushBuffer = [&](std::vector<IPABuffer> &buffers,
+			      const std::unique_ptr<FrameBuffer> &buffer) {
 		buffer->setCookie(ipaBufferId++);
-		ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
-	}
+		buffers.emplace_back(buffer->cookie(), buffer->planes());
+	};
 
-	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_) {
-		buffer->setCookie(ipaBufferId++);
-		ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes());
-	}
+	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->paramBuffers_)
+		pushBuffer(ipaBuffers_, buffer);
+
+	for (const std::unique_ptr<FrameBuffer> &buffer : imgu->statBuffers_)
+		pushBuffer(ipaBuffers_, buffer);
 
 	data->ipa_->mapBuffers(ipaBuffers_);
 
diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
index 76341ed3f363..f03a03fef35c 100644
--- a/src/libcamera/pipeline/mali-c55/mali-c55.cpp
+++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
@@ -1133,27 +1133,27 @@  int PipelineHandlerMaliC55::allocateBuffers(Camera *camera)
 		data->dsStream_.configuration().bufferCount,
 	});
 
+	auto pushBuffer = [&](std::queue<FrameBuffer *> &queue,
+			      std::vector<IPABuffer> &buffers,
+			      const std::unique_ptr<FrameBuffer> &buffer) {
+		buffer->setCookie(ipaBufferId++);
+		buffers.emplace_back(buffer->cookie(), buffer->planes());
+		queue.push(buffer.get());
+	};
+
 	ret = stats_->allocateBuffers(bufferCount, &statsBuffers_);
 	if (ret < 0)
 		return ret;
 
-	for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_) {
-		buffer->setCookie(ipaBufferId++);
-		data->ipaStatBuffers_.emplace_back(buffer->cookie(),
-						   buffer->planes());
-		availableStatsBuffers_.push(buffer.get());
-	}
+	for (std::unique_ptr<FrameBuffer> &buffer : statsBuffers_)
+		pushBuffer(availableStatsBuffers_, data->ipaStatBuffers_, buffer);
 
 	ret = params_->allocateBuffers(bufferCount, &paramsBuffers_);
 	if (ret < 0)
 		return ret;
 
-	for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_) {
-		buffer->setCookie(ipaBufferId++);
-		data->ipaParamBuffers_.emplace_back(buffer->cookie(),
-						    buffer->planes());
-		availableParamsBuffers_.push(buffer.get());
-	}
+	for (std::unique_ptr<FrameBuffer> &buffer : paramsBuffers_)
+		pushBuffer(availableParamsBuffers_, data->ipaParamBuffers_, buffer);
 
 	if (data->ipa_) {
 		data->ipa_->mapBuffers(data->ipaStatBuffers_, true);
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 55d7d4442caf..99347c9f6258 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -1028,19 +1028,19 @@  int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
 			availableMainPathBuffers_.push(buffer.get());
 	}
 
-	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) {
+	auto pushBuffer = [&](std::vector<IPABuffer> &buffers,
+			      std::queue<FrameBuffer *> &queue,
+			      const std::unique_ptr<FrameBuffer> &buffer) {
 		buffer->setCookie(ipaBufferId++);
-		data->ipaBuffers_.emplace_back(buffer->cookie(),
-					       buffer->planes());
-		availableParamBuffers_.push(buffer.get());
-	}
+		buffers.emplace_back(buffer->cookie(), buffer->planes());
+		queue.push(buffer.get());
+	};
 
-	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_) {
-		buffer->setCookie(ipaBufferId++);
-		data->ipaBuffers_.emplace_back(buffer->cookie(),
-					       buffer->planes());
-		availableStatBuffers_.push(buffer.get());
-	}
+	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_)
+		pushBuffer(data->ipaBuffers_, availableParamBuffers_, buffer);
+
+	for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_)
+		pushBuffer(data->ipaBuffers_, availableStatBuffers_, buffer);
 
 	data->ipa_->mapBuffers(data->ipaBuffers_);