[libcamera-devel] [PATCH 3/4] tests: v4l2_videodevice: Propagate media bus format

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Mon, Aug 05, 2019 at 05:51:32PM +0200, Niklas Söderlund wrote:
> Most of the video device tests are based on vimc and Linux commit
> 85ab1aa1fac17bcd ("media: vimc: deb: fix default sink bayer format")
> changes the default media bus format for the debayer subdevices. This
> leads to a -EPIPE error when trying to use the raw capture video device
> nodes.
> 
> Fix this by propagating the media bus format used on the debayer
> subdevcie to the sensor. This allows the same code to function on

s/subdevcie/subdevice/

And as for patch 2/4, I would propagate it the other way around, from
the sensor to the debayering subdev.

> kernels previous to the change and after it.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  .../v4l2_videodevice_test.cpp                 | 22 +++++++++++++++++++
>  test/v4l2_videodevice/v4l2_videodevice_test.h |  7 +++++-
>  2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.cpp b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> index b26d06ad45197c8f..b1d4eaf6e9e4a4b1 100644
> --- a/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> +++ b/test/v4l2_videodevice/v4l2_videodevice_test.cpp
> @@ -62,6 +62,26 @@ int V4L2VideoDeviceTest::init()
>  	if (ret)
>  		return TestFail;
>  
> +	if (driver_ == "vimc") {

A bit hack-ish, but for tests I think we can live with this.

> +		V4L2SubdeviceFormat subformat = {};
> +		subformat.size.width = 640;
> +		subformat.size.height = 480;
> +
> +		sensor_ = new CameraSensor(media_->getEntityByName("Sensor A"));
> +		if (sensor_->init())
> +			return TestFail;
> +
> +		debayer_ = new V4L2Subdevice(media_->getEntityByName("Debayer A"));
> +		if (debayer_->open())
> +			return TestFail;
> +
> +		if (debayer_->setFormat(0, &subformat))
> +			return TestFail;
> +
> +		if (sensor_->setFormat(&subformat))
> +			return TestFail;
> +	}
> +
>  	if (capture_->open())
>  		return TestFail;
>  
> @@ -83,5 +103,7 @@ void V4L2VideoDeviceTest::cleanup()
>  	capture_->releaseBuffers();
>  	capture_->close();
>  
> +	delete debayer_;
> +	delete sensor_;
>  	delete capture_;
>  };
> diff --git a/test/v4l2_videodevice/v4l2_videodevice_test.h b/test/v4l2_videodevice/v4l2_videodevice_test.h
> index 3321b5a4f98fdb1d..34dd231c6d9d108d 100644
> --- a/test/v4l2_videodevice/v4l2_videodevice_test.h
> +++ b/test/v4l2_videodevice/v4l2_videodevice_test.h
> @@ -13,8 +13,10 @@
>  
>  #include "test.h"
>  
> +#include "camera_sensor.h"
>  #include "device_enumerator.h"
>  #include "media_device.h"
> +#include "v4l2_subdevice.h"
>  #include "v4l2_videodevice.h"
>  
>  using namespace libcamera;
> @@ -23,7 +25,8 @@ class V4L2VideoDeviceTest : public Test
>  {
>  public:
>  	V4L2VideoDeviceTest(const char *driver, const char *entity)
> -		: driver_(driver), entity_(entity), capture_(nullptr)
> +		: driver_(driver), entity_(entity), sensor_(nullptr),
> +		  debayer_(nullptr), capture_(nullptr)
>  	{
>  	}
>  
> @@ -35,6 +38,8 @@ protected:
>  	std::string entity_;
>  	std::unique_ptr<DeviceEnumerator> enumerator_;
>  	std::shared_ptr<MediaDevice> media_;
> +	CameraSensor *sensor_;
> +	V4L2Subdevice *debayer_;

Do you need to store these internally, can't they be local variables in
V4L2VideoDeviceTest::init() ?

>  	V4L2VideoDevice *capture_;
>  	BufferPool pool_;
>  };

-- 
Regards,

Laurent Pinchart