[libcamera-devel,v3,2/4] pipeline: vimc: Allocate mock IPA buffers
diff mbox series

Message ID 20210814050912.15113-3-umang.jain@ideasonboard.com
State Accepted
Delegated to: Umang Jain
Headers show
Series
  • Pass buffers to VIMC IPA
Related show

Commit Message

Umang Jain Aug. 14, 2021, 5:09 a.m. UTC 
VIMC is a virtual test driver that doesn't have statistics or
parameters buffers that are typically passed from a pipeline
handler to its platform IPA. To increase the test coverage going
forward, we can atleast mimick the typical interaction of how
a pipeline handler and IPA interacts, and use it to increase the
test coverage.

Hence, create simple (single plane) dmabuf-backed FrameBuffers,
which can act as mock IPA buffers and can be memory mapped(mmap)
to VIMC IPA. To create these buffers, temporarily hijack the output
video node and configure it with a V4L2DeviceFormat. Buffers then
can be exported from the output video node using
V4L2VideoDevice::exportBuffers(). These buffers will be mimicked as
IPA buffers in subsequent commits.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/libcamera/pipeline/vimc/vimc.cpp | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Paul Elder Aug. 14, 2021, 5:36 a.m. UTC | #1 
Hi Umang,

On Sat, Aug 14, 2021 at 10:39:10AM +0530, Umang Jain wrote:
> VIMC is a virtual test driver that doesn't have statistics or
> parameters buffers that are typically passed from a pipeline
> handler to its platform IPA. To increase the test coverage going
> forward, we can atleast mimick the typical interaction of how
> a pipeline handler and IPA interacts, and use it to increase the
> test coverage.
> 
> Hence, create simple (single plane) dmabuf-backed FrameBuffers,
> which can act as mock IPA buffers and can be memory mapped(mmap)
> to VIMC IPA. To create these buffers, temporarily hijack the output
> video node and configure it with a V4L2DeviceFormat. Buffers then
> can be exported from the output video node using
> V4L2VideoDevice::exportBuffers(). These buffers will be mimicked as
> IPA buffers in subsequent commits.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/pipeline/vimc/vimc.cpp | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 4c92729d..2dfa1418 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -50,6 +50,7 @@ public:
>  	}
>  
>  	int init();
> +	int allocateMockIPABuffers();
>  	void bufferReady(FrameBuffer *buffer);
>  
>  	MediaDevice *media_;
> @@ -61,6 +62,7 @@ public:
>  	Stream stream_;
>  
>  	std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_;
> +	std::vector<std::unique_ptr<FrameBuffer>> mockIPABufs_;
>  };
>  
>  class VimcCameraConfiguration : public CameraConfiguration
> @@ -500,6 +502,12 @@ int VimcCameraData::init()
>  	if (raw_->open())
>  		return -ENODEV;
>  
> +	ret = allocateMockIPABuffers();
> +	if (!ret) {
> +		LOG(VIMC, Warning) << "Cannot allocate mock IPA buffers";
> +		return ret;
> +	}
> +
>  	/* Initialise the supported controls. */
>  	const ControlInfoMap &controls = sensor_->controls();
>  	ControlInfoMap::Map ctrls;
> @@ -548,6 +556,21 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)
>  	pipe_->completeRequest(request);
>  }
>  
> +int VimcCameraData::allocateMockIPABuffers()
> +{
> +	constexpr unsigned int kBufCount = 2;
> +
> +	V4L2DeviceFormat format;
> +	format.fourcc = video_->toV4L2PixelFormat(formats::BGR888);
> +	format.size = Size (160, 120);
> +
> +	int ret = video_->setFormat(&format);
> +	if (ret < 0)
> +		return ret;
> +
> +	return video_->exportBuffers(kBufCount, &mockIPABufs_);

Do we ever need to free these buffers?


Paul

> +}
> +
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc)
>  
>  } /* namespace libcamera */
> -- 
> 2.31.1
>
Laurent Pinchart Aug. 14, 2021, 7:09 p.m. UTC | #2 
Hi Umang,

Thank you for the patch.

On Sat, Aug 14, 2021 at 10:39:10AM +0530, Umang Jain wrote:
> VIMC is a virtual test driver that doesn't have statistics or
> parameters buffers that are typically passed from a pipeline
> handler to its platform IPA. To increase the test coverage going
> forward, we can atleast mimick the typical interaction of how

s/atleast/at least/

> a pipeline handler and IPA interacts, and use it to increase the

s/interacts/interact/

> test coverage.
> 
> Hence, create simple (single plane) dmabuf-backed FrameBuffers,
> which can act as mock IPA buffers and can be memory mapped(mmap)

s/mapped/mapped /

> to VIMC IPA. To create these buffers, temporarily hijack the output
> video node and configure it with a V4L2DeviceFormat. Buffers then
> can be exported from the output video node using
> V4L2VideoDevice::exportBuffers(). These buffers will be mimicked as
> IPA buffers in subsequent commits.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/libcamera/pipeline/vimc/vimc.cpp | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 4c92729d..2dfa1418 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -50,6 +50,7 @@ public:
>  	}
>  
>  	int init();
> +	int allocateMockIPABuffers();
>  	void bufferReady(FrameBuffer *buffer);
>  
>  	MediaDevice *media_;
> @@ -61,6 +62,7 @@ public:
>  	Stream stream_;
>  
>  	std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_;
> +	std::vector<std::unique_ptr<FrameBuffer>> mockIPABufs_;
>  };
>  
>  class VimcCameraConfiguration : public CameraConfiguration
> @@ -500,6 +502,12 @@ int VimcCameraData::init()
>  	if (raw_->open())
>  		return -ENODEV;
>  
> +	ret = allocateMockIPABuffers();
> +	if (!ret) {

This should be ret < 0. Did test pass with !ret ?

> +		LOG(VIMC, Warning) << "Cannot allocate mock IPA buffers";
> +		return ret;
> +	}
> +
>  	/* Initialise the supported controls. */
>  	const ControlInfoMap &controls = sensor_->controls();
>  	ControlInfoMap::Map ctrls;
> @@ -548,6 +556,21 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)
>  	pipe_->completeRequest(request);
>  }
>  
> +int VimcCameraData::allocateMockIPABuffers()
> +{
> +	constexpr unsigned int kBufCount = 2;
> +
> +	V4L2DeviceFormat format;
> +	format.fourcc = video_->toV4L2PixelFormat(formats::BGR888);
> +	format.size = Size (160, 120);
> +
> +	int ret = video_->setFormat(&format);
> +	if (ret < 0)
> +		return ret;
> +
> +	return video_->exportBuffers(kBufCount, &mockIPABufs_);
> +}
> +
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc)
>  
>  } /* namespace libcamera */
Laurent Pinchart Aug. 14, 2021, 7:09 p.m. UTC | #3 
Hi Paul,

On Sat, Aug 14, 2021 at 02:36:44PM +0900, paul.elder@ideasonboard.com wrote:
> On Sat, Aug 14, 2021 at 10:39:10AM +0530, Umang Jain wrote:
> > VIMC is a virtual test driver that doesn't have statistics or
> > parameters buffers that are typically passed from a pipeline
> > handler to its platform IPA. To increase the test coverage going
> > forward, we can atleast mimick the typical interaction of how
> > a pipeline handler and IPA interacts, and use it to increase the
> > test coverage.
> > 
> > Hence, create simple (single plane) dmabuf-backed FrameBuffers,
> > which can act as mock IPA buffers and can be memory mapped(mmap)
> > to VIMC IPA. To create these buffers, temporarily hijack the output
> > video node and configure it with a V4L2DeviceFormat. Buffers then
> > can be exported from the output video node using
> > V4L2VideoDevice::exportBuffers(). These buffers will be mimicked as
> > IPA buffers in subsequent commits.
> > 
> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/libcamera/pipeline/vimc/vimc.cpp | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index 4c92729d..2dfa1418 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -50,6 +50,7 @@ public:
> >  	}
> >  
> >  	int init();
> > +	int allocateMockIPABuffers();
> >  	void bufferReady(FrameBuffer *buffer);
> >  
> >  	MediaDevice *media_;
> > @@ -61,6 +62,7 @@ public:
> >  	Stream stream_;
> >  
> >  	std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_;
> > +	std::vector<std::unique_ptr<FrameBuffer>> mockIPABufs_;
> >  };
> >  
> >  class VimcCameraConfiguration : public CameraConfiguration
> > @@ -500,6 +502,12 @@ int VimcCameraData::init()
> >  	if (raw_->open())
> >  		return -ENODEV;
> >  
> > +	ret = allocateMockIPABuffers();
> > +	if (!ret) {
> > +		LOG(VIMC, Warning) << "Cannot allocate mock IPA buffers";
> > +		return ret;
> > +	}
> > +
> >  	/* Initialise the supported controls. */
> >  	const ControlInfoMap &controls = sensor_->controls();
> >  	ControlInfoMap::Map ctrls;
> > @@ -548,6 +556,21 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)
> >  	pipe_->completeRequest(request);
> >  }
> >  
> > +int VimcCameraData::allocateMockIPABuffers()
> > +{
> > +	constexpr unsigned int kBufCount = 2;
> > +
> > +	V4L2DeviceFormat format;
> > +	format.fourcc = video_->toV4L2PixelFormat(formats::BGR888);
> > +	format.size = Size (160, 120);
> > +
> > +	int ret = video_->setFormat(&format);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return video_->exportBuffers(kBufCount, &mockIPABufs_);
> 
> Do we ever need to free these buffers?

It's the beauty of a std::vector<std::unique_ptr<>> :-)

> > +}
> > +
> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc)
> >  
> >  } /* namespace libcamera */
Paul Elder Aug. 16, 2021, 4:41 a.m. UTC | #4 
On Sat, Aug 14, 2021 at 10:09:41PM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> On Sat, Aug 14, 2021 at 02:36:44PM +0900, paul.elder@ideasonboard.com wrote:
> > On Sat, Aug 14, 2021 at 10:39:10AM +0530, Umang Jain wrote:
> > > VIMC is a virtual test driver that doesn't have statistics or
> > > parameters buffers that are typically passed from a pipeline
> > > handler to its platform IPA. To increase the test coverage going
> > > forward, we can atleast mimick the typical interaction of how
> > > a pipeline handler and IPA interacts, and use it to increase the
> > > test coverage.
> > > 
> > > Hence, create simple (single plane) dmabuf-backed FrameBuffers,
> > > which can act as mock IPA buffers and can be memory mapped(mmap)
> > > to VIMC IPA. To create these buffers, temporarily hijack the output
> > > video node and configure it with a V4L2DeviceFormat. Buffers then
> > > can be exported from the output video node using
> > > V4L2VideoDevice::exportBuffers(). These buffers will be mimicked as
> > > IPA buffers in subsequent commits.
> > > 
> > > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  src/libcamera/pipeline/vimc/vimc.cpp | 23 +++++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > > 
> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > > index 4c92729d..2dfa1418 100644
> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > @@ -50,6 +50,7 @@ public:
> > >  	}
> > >  
> > >  	int init();
> > > +	int allocateMockIPABuffers();
> > >  	void bufferReady(FrameBuffer *buffer);
> > >  
> > >  	MediaDevice *media_;
> > > @@ -61,6 +62,7 @@ public:
> > >  	Stream stream_;
> > >  
> > >  	std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_;
> > > +	std::vector<std::unique_ptr<FrameBuffer>> mockIPABufs_;
> > >  };
> > >  
> > >  class VimcCameraConfiguration : public CameraConfiguration
> > > @@ -500,6 +502,12 @@ int VimcCameraData::init()
> > >  	if (raw_->open())
> > >  		return -ENODEV;
> > >  
> > > +	ret = allocateMockIPABuffers();
> > > +	if (!ret) {
> > > +		LOG(VIMC, Warning) << "Cannot allocate mock IPA buffers";
> > > +		return ret;
> > > +	}
> > > +
> > >  	/* Initialise the supported controls. */
> > >  	const ControlInfoMap &controls = sensor_->controls();
> > >  	ControlInfoMap::Map ctrls;
> > > @@ -548,6 +556,21 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)
> > >  	pipe_->completeRequest(request);
> > >  }
> > >  
> > > +int VimcCameraData::allocateMockIPABuffers()
> > > +{
> > > +	constexpr unsigned int kBufCount = 2;
> > > +
> > > +	V4L2DeviceFormat format;
> > > +	format.fourcc = video_->toV4L2PixelFormat(formats::BGR888);
> > > +	format.size = Size (160, 120);
> > > +
> > > +	int ret = video_->setFormat(&format);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return video_->exportBuffers(kBufCount, &mockIPABufs_);
> > 
> > Do we ever need to free these buffers?
> 
> It's the beauty of a std::vector<std::unique_ptr<>> :-)

Ooh, fancy :D

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> 
> > > +}
> > > +
> > >  REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc)
> > >  
> > >  } /* namespace libcamera */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Umang Jain Aug. 16, 2021, 7:06 a.m. UTC | #5 
Hi Laurent

On 8/15/21 12:39 AM, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Sat, Aug 14, 2021 at 10:39:10AM +0530, Umang Jain wrote:
>> VIMC is a virtual test driver that doesn't have statistics or
>> parameters buffers that are typically passed from a pipeline
>> handler to its platform IPA. To increase the test coverage going
>> forward, we can atleast mimick the typical interaction of how
> s/atleast/at least/
>
>> a pipeline handler and IPA interacts, and use it to increase the
> s/interacts/interact/
>
>> test coverage.
>>
>> Hence, create simple (single plane) dmabuf-backed FrameBuffers,
>> which can act as mock IPA buffers and can be memory mapped(mmap)
> s/mapped/mapped /
>
>> to VIMC IPA. To create these buffers, temporarily hijack the output
>> video node and configure it with a V4L2DeviceFormat. Buffers then
>> can be exported from the output video node using
>> V4L2VideoDevice::exportBuffers(). These buffers will be mimicked as
>> IPA buffers in subsequent commits.
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>   src/libcamera/pipeline/vimc/vimc.cpp | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
>> index 4c92729d..2dfa1418 100644
>> --- a/src/libcamera/pipeline/vimc/vimc.cpp
>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
>> @@ -50,6 +50,7 @@ public:
>>   	}
>>   
>>   	int init();
>> +	int allocateMockIPABuffers();
>>   	void bufferReady(FrameBuffer *buffer);
>>   
>>   	MediaDevice *media_;
>> @@ -61,6 +62,7 @@ public:
>>   	Stream stream_;
>>   
>>   	std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_;
>> +	std::vector<std::unique_ptr<FrameBuffer>> mockIPABufs_;
>>   };
>>   
>>   class VimcCameraConfiguration : public CameraConfiguration
>> @@ -500,6 +502,12 @@ int VimcCameraData::init()
>>   	if (raw_->open())
>>   		return -ENODEV;
>>   
>> +	ret = allocateMockIPABuffers();
>> +	if (!ret) {
> This should be ret < 0. Did test pass with !ret ?
Yes, it was. But I agree it should be ret < 0, I'll fixup! during push
>
>> +		LOG(VIMC, Warning) << "Cannot allocate mock IPA buffers";
>> +		return ret;
>> +	}
>> +
>>   	/* Initialise the supported controls. */
>>   	const ControlInfoMap &controls = sensor_->controls();
>>   	ControlInfoMap::Map ctrls;
>> @@ -548,6 +556,21 @@ void VimcCameraData::bufferReady(FrameBuffer *buffer)
>>   	pipe_->completeRequest(request);
>>   }
>>   
>> +int VimcCameraData::allocateMockIPABuffers()
>> +{
>> +	constexpr unsigned int kBufCount = 2;
>> +
>> +	V4L2DeviceFormat format;
>> +	format.fourcc = video_->toV4L2PixelFormat(formats::BGR888);
>> +	format.size = Size (160, 120);
>> +
>> +	int ret = video_->setFormat(&format);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return video_->exportBuffers(kBufCount, &mockIPABufs_);
>> +}
>> +
>>   REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc)
>>   
>>   } /* namespace libcamera */

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index 4c92729d..2dfa1418 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -50,6 +50,7 @@  public:
 	}
 
 	int init();
+	int allocateMockIPABuffers();
 	void bufferReady(FrameBuffer *buffer);
 
 	MediaDevice *media_;
@@ -61,6 +62,7 @@  public:
 	Stream stream_;
 
 	std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_;
+	std::vector<std::unique_ptr<FrameBuffer>> mockIPABufs_;
 };
 
 class VimcCameraConfiguration : public CameraConfiguration
@@ -500,6 +502,12 @@  int VimcCameraData::init()
 	if (raw_->open())
 		return -ENODEV;
 
+	ret = allocateMockIPABuffers();
+	if (!ret) {
+		LOG(VIMC, Warning) << "Cannot allocate mock IPA buffers";
+		return ret;
+	}
+
 	/* Initialise the supported controls. */
 	const ControlInfoMap &controls = sensor_->controls();
 	ControlInfoMap::Map ctrls;
@@ -548,6 +556,21 @@  void VimcCameraData::bufferReady(FrameBuffer *buffer)
 	pipe_->completeRequest(request);
 }
 
+int VimcCameraData::allocateMockIPABuffers()
+{
+	constexpr unsigned int kBufCount = 2;
+
+	V4L2DeviceFormat format;
+	format.fourcc = video_->toV4L2PixelFormat(formats::BGR888);
+	format.size = Size (160, 120);
+
+	int ret = video_->setFormat(&format);
+	if (ret < 0)
+		return ret;
+
+	return video_->exportBuffers(kBufCount, &mockIPABufs_);
+}
+
 REGISTER_PIPELINE_HANDLER(PipelineHandlerVimc)
 
 } /* namespace libcamera */