[RFC,v1,08/27] libcamera: pipeline: imx8-isi: pipeFromStream(): Take `ISICameraData`
diff mbox series

Message ID 20260618123844.656396-9-barnabas.pocze@ideasonboard.com
State Superseded
Headers show
Series
  • Misc. changes before request-buffer split
Related show

Commit Message

Barnabás Pőcze June 18, 2026, 12:38 p.m. UTC 
The function actually needs the pipeline handler specific camera data,
and in most callers it is already available, so accept that as argument.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Comments

Jacopo Mondi June 22, 2026, 12:51 p.m. UTC | #1 
Hi Barnabás

So I count 5 call locations of pipeFromStream().

In three you can pass in 'data'.
In two of them you have to call cameraData() in the caller.

What have we gained with this change ? Avoid calling cameraData() (a
static cast) in three locations ?

Anyway, even without any other reason I might have missed for this patch
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>


On Thu, Jun 18, 2026 at 02:38:25PM +0200, Barnabás Pőcze wrote:
> The function actually needs the pipeline handler specific camera data,
> and in most callers it is already available, so accept that as argument.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

> ---
>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> index c3d2695556..5f15fbdc08 100644
> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> @@ -136,7 +136,7 @@ private:
>  		return static_cast<ISICameraData *>(camera->_d());
>  	}
>
> -	Pipe *pipeFromStream(Camera *camera, const Stream *stream);
> +	Pipe *pipeFromStream(ISICameraData *data, const Stream *stream);
>
>  	StreamConfiguration generateYUVConfiguration(Camera *camera,
>  						     const Size &size);
> @@ -865,7 +865,7 @@ int PipelineHandlerISI::configure(Camera *camera, CameraConfiguration *c)
>  	 */
>  	data->enabledStreams_.clear();
>  	for (const auto &config : *c) {
> -		Pipe *pipe = pipeFromStream(camera, config.stream());
> +		Pipe *pipe = pipeFromStream(data, config.stream());
>
>  		/*
>  		 * Configure the ISI sink compose rectangle to downscale the
> @@ -915,7 +915,7 @@ int PipelineHandlerISI::exportFrameBuffers(Camera *camera, Stream *stream,
>  					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
>  	unsigned int count = stream->configuration().bufferCount;
> -	Pipe *pipe = pipeFromStream(camera, stream);
> +	Pipe *pipe = pipeFromStream(cameraData(camera), stream);
>
>  	return pipe->capture->exportBuffers(count, buffers);
>  }
> @@ -926,7 +926,7 @@ int PipelineHandlerISI::start(Camera *camera,
>  	ISICameraData *data = cameraData(camera);
>
>  	for (const auto &stream : data->enabledStreams_) {
> -		Pipe *pipe = pipeFromStream(camera, stream);
> +		Pipe *pipe = pipeFromStream(data, stream);
>  		const StreamConfiguration &config = stream->configuration();
>
>  		int ret = pipe->capture->importBuffers(config.bufferCount);
> @@ -946,7 +946,7 @@ void PipelineHandlerISI::stopDevice(Camera *camera)
>  	ISICameraData *data = cameraData(camera);
>
>  	for (const auto &stream : data->enabledStreams_) {
> -		Pipe *pipe = pipeFromStream(camera, stream);
> +		Pipe *pipe = pipeFromStream(data, stream);
>
>  		pipe->capture->streamOff();
>  		pipe->capture->releaseBuffers();
> @@ -955,8 +955,10 @@ void PipelineHandlerISI::stopDevice(Camera *camera)
>
>  int PipelineHandlerISI::queueRequestDevice(Camera *camera, Request *request)
>  {
> +	ISICameraData *data = cameraData(camera);
> +
>  	for (auto &[stream, buffer] : request->buffers()) {
> -		Pipe *pipe = pipeFromStream(camera, stream);
> +		Pipe *pipe = pipeFromStream(data, stream);
>
>  		int ret = pipe->capture->queueBuffer(buffer);
>  		if (ret)
> @@ -1110,10 +1112,9 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>  	return numCameras > 0;
>  }
>
> -PipelineHandlerISI::Pipe *PipelineHandlerISI::pipeFromStream(Camera *camera,
> +PipelineHandlerISI::Pipe *PipelineHandlerISI::pipeFromStream(ISICameraData *data,
>  							     const Stream *stream)
>  {
> -	ISICameraData *data = cameraData(camera);
>  	unsigned int pipeIndex = data->pipeIndex(stream);
>
>  	ASSERT(pipeIndex < pipes_.size());
> --
> 2.54.0
>
Laurent Pinchart June 23, 2026, 4:54 p.m. UTC | #2 
On Mon, Jun 22, 2026 at 02:51:44PM +0200, Jacopo Mondi wrote:
> Hi Barnabás
> 
> So I count 5 call locations of pipeFromStream().
> 
> In three you can pass in 'data'.
> In two of them you have to call cameraData() in the caller.
> 
> What have we gained with this change ? Avoid calling cameraData() (a
> static cast) in three locations ?
> 
> Anyway, even without any other reason I might have missed for this patch
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

I also find that there's little value in this patch by itself, compared
to the review cost. It may prepare for future changes that will benefit
more from it though, so (and because I've now reviewed it)

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> On Thu, Jun 18, 2026 at 02:38:25PM +0200, Barnabás Pőcze wrote:
> > The function actually needs the pipeline handler specific camera data,
> > and in most callers it is already available, so accept that as argument.
> >
> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> 
> > ---
> >  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > index c3d2695556..5f15fbdc08 100644
> > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > @@ -136,7 +136,7 @@ private:
> >  		return static_cast<ISICameraData *>(camera->_d());
> >  	}
> >
> > -	Pipe *pipeFromStream(Camera *camera, const Stream *stream);
> > +	Pipe *pipeFromStream(ISICameraData *data, const Stream *stream);
> >
> >  	StreamConfiguration generateYUVConfiguration(Camera *camera,
> >  						     const Size &size);
> > @@ -865,7 +865,7 @@ int PipelineHandlerISI::configure(Camera *camera, CameraConfiguration *c)
> >  	 */
> >  	data->enabledStreams_.clear();
> >  	for (const auto &config : *c) {
> > -		Pipe *pipe = pipeFromStream(camera, config.stream());
> > +		Pipe *pipe = pipeFromStream(data, config.stream());
> >
> >  		/*
> >  		 * Configure the ISI sink compose rectangle to downscale the
> > @@ -915,7 +915,7 @@ int PipelineHandlerISI::exportFrameBuffers(Camera *camera, Stream *stream,
> >  					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >  {
> >  	unsigned int count = stream->configuration().bufferCount;
> > -	Pipe *pipe = pipeFromStream(camera, stream);
> > +	Pipe *pipe = pipeFromStream(cameraData(camera), stream);
> >
> >  	return pipe->capture->exportBuffers(count, buffers);
> >  }
> > @@ -926,7 +926,7 @@ int PipelineHandlerISI::start(Camera *camera,
> >  	ISICameraData *data = cameraData(camera);
> >
> >  	for (const auto &stream : data->enabledStreams_) {
> > -		Pipe *pipe = pipeFromStream(camera, stream);
> > +		Pipe *pipe = pipeFromStream(data, stream);
> >  		const StreamConfiguration &config = stream->configuration();
> >
> >  		int ret = pipe->capture->importBuffers(config.bufferCount);
> > @@ -946,7 +946,7 @@ void PipelineHandlerISI::stopDevice(Camera *camera)
> >  	ISICameraData *data = cameraData(camera);
> >
> >  	for (const auto &stream : data->enabledStreams_) {
> > -		Pipe *pipe = pipeFromStream(camera, stream);
> > +		Pipe *pipe = pipeFromStream(data, stream);
> >
> >  		pipe->capture->streamOff();
> >  		pipe->capture->releaseBuffers();
> > @@ -955,8 +955,10 @@ void PipelineHandlerISI::stopDevice(Camera *camera)
> >
> >  int PipelineHandlerISI::queueRequestDevice(Camera *camera, Request *request)
> >  {
> > +	ISICameraData *data = cameraData(camera);
> > +
> >  	for (auto &[stream, buffer] : request->buffers()) {
> > -		Pipe *pipe = pipeFromStream(camera, stream);
> > +		Pipe *pipe = pipeFromStream(data, stream);
> >
> >  		int ret = pipe->capture->queueBuffer(buffer);
> >  		if (ret)
> > @@ -1110,10 +1112,9 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
> >  	return numCameras > 0;
> >  }
> >
> > -PipelineHandlerISI::Pipe *PipelineHandlerISI::pipeFromStream(Camera *camera,
> > +PipelineHandlerISI::Pipe *PipelineHandlerISI::pipeFromStream(ISICameraData *data,
> >  							     const Stream *stream)
> >  {
> > -	ISICameraData *data = cameraData(camera);
> >  	unsigned int pipeIndex = data->pipeIndex(stream);
> >
> >  	ASSERT(pipeIndex < pipes_.size());
Barnabás Pőcze June 24, 2026, 9:33 a.m. UTC | #3 
2026. 06. 23. 18:54 keltezéssel, Laurent Pinchart írta:
> On Mon, Jun 22, 2026 at 02:51:44PM +0200, Jacopo Mondi wrote:
>> Hi Barnabás
>>
>> So I count 5 call locations of pipeFromStream().
>>
>> In three you can pass in 'data'.
>> In two of them you have to call cameraData() in the caller.
>>
>> What have we gained with this change ? Avoid calling cameraData() (a
>> static cast) in three locations ?
>>
>> Anyway, even without any other reason I might have missed for this patch
>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> 
> I also find that there's little value in this patch by itself, compared
> to the review cost. It may prepare for future changes that will benefit
> more from it though, so (and because I've now reviewed it)
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

If I recall correctly, I made these changes when trying to convert the pipeline handler,
but those changes have since been dropped, so it is not needed.


> 
>> On Thu, Jun 18, 2026 at 02:38:25PM +0200, Barnabás Pőcze wrote:
>>> The function actually needs the pipeline handler specific camera data,
>>> and in most callers it is already available, so accept that as argument.
>>>
>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>>
>>> ---
>>>   src/libcamera/pipeline/imx8-isi/imx8-isi.cpp | 17 +++++++++--------
>>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>>> index c3d2695556..5f15fbdc08 100644
>>> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>>> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>>> @@ -136,7 +136,7 @@ private:
>>>   		return static_cast<ISICameraData *>(camera->_d());
>>>   	}
>>>
>>> -	Pipe *pipeFromStream(Camera *camera, const Stream *stream);
>>> +	Pipe *pipeFromStream(ISICameraData *data, const Stream *stream);
>>>
>>>   	StreamConfiguration generateYUVConfiguration(Camera *camera,
>>>   						     const Size &size);
>>> @@ -865,7 +865,7 @@ int PipelineHandlerISI::configure(Camera *camera, CameraConfiguration *c)
>>>   	 */
>>>   	data->enabledStreams_.clear();
>>>   	for (const auto &config : *c) {
>>> -		Pipe *pipe = pipeFromStream(camera, config.stream());
>>> +		Pipe *pipe = pipeFromStream(data, config.stream());
>>>
>>>   		/*
>>>   		 * Configure the ISI sink compose rectangle to downscale the
>>> @@ -915,7 +915,7 @@ int PipelineHandlerISI::exportFrameBuffers(Camera *camera, Stream *stream,
>>>   					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>>>   {
>>>   	unsigned int count = stream->configuration().bufferCount;
>>> -	Pipe *pipe = pipeFromStream(camera, stream);
>>> +	Pipe *pipe = pipeFromStream(cameraData(camera), stream);
>>>
>>>   	return pipe->capture->exportBuffers(count, buffers);
>>>   }
>>> @@ -926,7 +926,7 @@ int PipelineHandlerISI::start(Camera *camera,
>>>   	ISICameraData *data = cameraData(camera);
>>>
>>>   	for (const auto &stream : data->enabledStreams_) {
>>> -		Pipe *pipe = pipeFromStream(camera, stream);
>>> +		Pipe *pipe = pipeFromStream(data, stream);
>>>   		const StreamConfiguration &config = stream->configuration();
>>>
>>>   		int ret = pipe->capture->importBuffers(config.bufferCount);
>>> @@ -946,7 +946,7 @@ void PipelineHandlerISI::stopDevice(Camera *camera)
>>>   	ISICameraData *data = cameraData(camera);
>>>
>>>   	for (const auto &stream : data->enabledStreams_) {
>>> -		Pipe *pipe = pipeFromStream(camera, stream);
>>> +		Pipe *pipe = pipeFromStream(data, stream);
>>>
>>>   		pipe->capture->streamOff();
>>>   		pipe->capture->releaseBuffers();
>>> @@ -955,8 +955,10 @@ void PipelineHandlerISI::stopDevice(Camera *camera)
>>>
>>>   int PipelineHandlerISI::queueRequestDevice(Camera *camera, Request *request)
>>>   {
>>> +	ISICameraData *data = cameraData(camera);
>>> +
>>>   	for (auto &[stream, buffer] : request->buffers()) {
>>> -		Pipe *pipe = pipeFromStream(camera, stream);
>>> +		Pipe *pipe = pipeFromStream(data, stream);
>>>
>>>   		int ret = pipe->capture->queueBuffer(buffer);
>>>   		if (ret)
>>> @@ -1110,10 +1112,9 @@ bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
>>>   	return numCameras > 0;
>>>   }
>>>
>>> -PipelineHandlerISI::Pipe *PipelineHandlerISI::pipeFromStream(Camera *camera,
>>> +PipelineHandlerISI::Pipe *PipelineHandlerISI::pipeFromStream(ISICameraData *data,
>>>   							     const Stream *stream)
>>>   {
>>> -	ISICameraData *data = cameraData(camera);
>>>   	unsigned int pipeIndex = data->pipeIndex(stream);
>>>
>>>   	ASSERT(pipeIndex < pipes_.size());
>

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
index c3d2695556..5f15fbdc08 100644
--- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
+++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
@@ -136,7 +136,7 @@  private:
 		return static_cast<ISICameraData *>(camera->_d());
 	}
 
-	Pipe *pipeFromStream(Camera *camera, const Stream *stream);
+	Pipe *pipeFromStream(ISICameraData *data, const Stream *stream);
 
 	StreamConfiguration generateYUVConfiguration(Camera *camera,
 						     const Size &size);
@@ -865,7 +865,7 @@  int PipelineHandlerISI::configure(Camera *camera, CameraConfiguration *c)
 	 */
 	data->enabledStreams_.clear();
 	for (const auto &config : *c) {
-		Pipe *pipe = pipeFromStream(camera, config.stream());
+		Pipe *pipe = pipeFromStream(data, config.stream());
 
 		/*
 		 * Configure the ISI sink compose rectangle to downscale the
@@ -915,7 +915,7 @@  int PipelineHandlerISI::exportFrameBuffers(Camera *camera, Stream *stream,
 					   std::vector<std::unique_ptr<FrameBuffer>> *buffers)
 {
 	unsigned int count = stream->configuration().bufferCount;
-	Pipe *pipe = pipeFromStream(camera, stream);
+	Pipe *pipe = pipeFromStream(cameraData(camera), stream);
 
 	return pipe->capture->exportBuffers(count, buffers);
 }
@@ -926,7 +926,7 @@  int PipelineHandlerISI::start(Camera *camera,
 	ISICameraData *data = cameraData(camera);
 
 	for (const auto &stream : data->enabledStreams_) {
-		Pipe *pipe = pipeFromStream(camera, stream);
+		Pipe *pipe = pipeFromStream(data, stream);
 		const StreamConfiguration &config = stream->configuration();
 
 		int ret = pipe->capture->importBuffers(config.bufferCount);
@@ -946,7 +946,7 @@  void PipelineHandlerISI::stopDevice(Camera *camera)
 	ISICameraData *data = cameraData(camera);
 
 	for (const auto &stream : data->enabledStreams_) {
-		Pipe *pipe = pipeFromStream(camera, stream);
+		Pipe *pipe = pipeFromStream(data, stream);
 
 		pipe->capture->streamOff();
 		pipe->capture->releaseBuffers();
@@ -955,8 +955,10 @@  void PipelineHandlerISI::stopDevice(Camera *camera)
 
 int PipelineHandlerISI::queueRequestDevice(Camera *camera, Request *request)
 {
+	ISICameraData *data = cameraData(camera);
+
 	for (auto &[stream, buffer] : request->buffers()) {
-		Pipe *pipe = pipeFromStream(camera, stream);
+		Pipe *pipe = pipeFromStream(data, stream);
 
 		int ret = pipe->capture->queueBuffer(buffer);
 		if (ret)
@@ -1110,10 +1112,9 @@  bool PipelineHandlerISI::match(DeviceEnumerator *enumerator)
 	return numCameras > 0;
 }
 
-PipelineHandlerISI::Pipe *PipelineHandlerISI::pipeFromStream(Camera *camera,
+PipelineHandlerISI::Pipe *PipelineHandlerISI::pipeFromStream(ISICameraData *data,
 							     const Stream *stream)
 {
-	ISICameraData *data = cameraData(camera);
 	unsigned int pipeIndex = data->pipeIndex(stream);
 
 	ASSERT(pipeIndex < pipes_.size());