[libcamera-devel] [PATCH 1/1] rkisp1: add pipeline test for rkisp1

[Laurent Pinchart]

Hi Show,

Thank you for the patch.

On Wed, Oct 23, 2019 at 05:52:54PM +0800, Show Liu wrote:

A commit message would be nice here, maybe just copied from the cover
letter.

> Signed-off-by: Show Liu <show.liu at linaro.org>
> ---
>  test/pipeline/meson.build                     |   1 +
>  test/pipeline/rkisp1/meson.build              |  12 ++
>  test/pipeline/rkisp1/rkisp1_pipeline_test.cpp | 109 ++++++++++++++++++
>  3 files changed, 122 insertions(+)
>  create mode 100644 test/pipeline/rkisp1/meson.build
>  create mode 100644 test/pipeline/rkisp1/rkisp1_pipeline_test.cpp
> 
> diff --git a/test/pipeline/meson.build b/test/pipeline/meson.build
> index f434c79..157f789 100644
> --- a/test/pipeline/meson.build
> +++ b/test/pipeline/meson.build
> @@ -1 +1,2 @@
>  subdir('ipu3')
> +subdir('rkisp1')
> diff --git a/test/pipeline/rkisp1/meson.build b/test/pipeline/rkisp1/meson.build
> new file mode 100644
> index 0000000..d3f9749
> --- /dev/null
> +++ b/test/pipeline/rkisp1/meson.build
> @@ -0,0 +1,12 @@
> +rkisp1_test = [
> +    ['rkisp1_pipeline_test',            'rkisp1_pipeline_test.cpp'],
> +]
> +
> +foreach t : rkisp1_test
> +    exe = executable(t[0], t[1],
> +                     dependencies : libcamera_dep,
> +                     link_with : test_libraries,
> +                     include_directories : test_includes_internal)
> +
> +    test(t[0], exe, suite : 'rkisp1', is_parallel : false)
> +endforeach
> diff --git a/test/pipeline/rkisp1/rkisp1_pipeline_test.cpp b/test/pipeline/rkisp1/rkisp1_pipeline_test.cpp
> new file mode 100644
> index 0000000..a463309
> --- /dev/null
> +++ b/test/pipeline/rkisp1/rkisp1_pipeline_test.cpp
> @@ -0,0 +1,109 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*

This is missing a copyright line.

> + * rkisp1_pipeline_test.cpp - Rockchip RK3399 pipeline test
> + */
> +
> +#include <iostream>
> +
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include <libcamera/camera.h>
> +#include <libcamera/camera_manager.h>
> +
> +#include "device_enumerator.h"
> +#include "media_device.h"
> +#include "media_object.h"
> +#include "test.h"
> +
> +using namespace std;
> +using namespace libcamera;
> +
> +/*
> + * Verify that the RK3399 pipeline handler gets matched and cameras
> + * are enumerated correctly.
> + *
> + * The test is supposed to be run on rockchip platform.
> + *
> + * The test lists all cameras registered in the system, if any camera is
> + * available at all.
> + */
> +class RKISP1PipelineTest : public Test
> +{
> +protected:
> +	int init();
> +	int run();
> +	void cleanup();
> +
> +private:
> +	CameraManager *cameraManager_;
> +	unsigned int sensors_;
> +};
> +
> +int RKISP1PipelineTest::init()
> +{
> +	unique_ptr<DeviceEnumerator> enumerator = DeviceEnumerator::create();
> +	if (!enumerator) {
> +		cerr << "Failed to create device enumerator" << endl;
> +		return TestFail;
> +	}
> +
> +	if (enumerator->enumerate()) {
> +		cerr << "Failed to enumerate media devices" << endl;
> +		return TestFail;
> +	}
> +
> +	DeviceMatch dm("rkisp1");
> +
> +	std::shared_ptr<MediaDevice> dm_rkisp1 =  enumerator->search(dm);

I don't think the prefer dm_ is right here. The dm above stands for
DeviceMatch. I would call this variable just rkisp1.

> +	if (!dm_rkisp1) {
> +		cerr << "Failed to find rkisp1: test skip" << endl;
> +		return TestSkip;
> +	}
> +
> +	int ret = dm_rkisp1->populate();
> +	if (ret) {
> +		cerr << "Failed to populate media device " << dm_rkisp1->deviceNode() << endl;

Could you wrap this line ? We try to keep lines under the 80 columns
limit when possible, with a hard limit at 120 when wrapping isn't
possible at all.

		cerr << "Failed to populate media device "
		     << dm_rkisp1->deviceNode() << endl;

Other than that the patch looks good to me. It duplicates code from the
IPU3 test, and it would be nice to share the code instead, but we plan
to work on pipeline-specific tests in the near future, so we'll handle
that refactoring.

Could you please send a v2 with those small issues fixed ?

> +		return TestFail;
> +	}
> +
> +	sensors_ = 0;
> +	const vector<MediaEntity *> &entities = dm_rkisp1->entities();
> +	for (MediaEntity *entity : entities) {
> +		if (entity->function() == MEDIA_ENT_F_CAM_SENSOR)
> +			sensors_++;
> +	}
> +
> +	cameraManager_ = new CameraManager();
> +	ret = cameraManager_->start();
> +	if (ret) {
> +		cerr << "Failed to start the CameraManager" << endl;
> +		return TestFail;
> +	}
> +
> +	return 0;
> +}
> +
> +int RKISP1PipelineTest::run()
> +{
> +	auto cameras = cameraManager_->cameras();
> +	for (const std::shared_ptr<Camera> &cam : cameras)
> +		cout << "Found camera '" << cam->name() << "'" << endl;
> +
> +	if (cameras.size() != sensors_) {
> +		cerr << cameras.size() << " cameras registered, but " << sensors_
> +		     << " were expected" << endl;
> +		return TestFail;
> +	}
> +
> +	return TestPass;
> +}
> +
> +void RKISP1PipelineTest::cleanup()
> +{
> +	cameraManager_->stop();
> +	delete cameraManager_;
> +}
> +
> +TEST_REGISTER(RKISP1PipelineTest)

-- 
Regards,

Laurent Pinchart