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

[Kieran Bingham]

Hi Show

On 24/10/2019 07:40, Show Liu wrote:
> This is a simple test tool for rkisp1 pipeline refer from IPU3 pipeline test.
> 
> 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 | 112 ++++++++++++++++++
>  3 files changed, 125 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..274955e
> --- /dev/null
> +++ b/test/pipeline/rkisp1/rkisp1_pipeline_test.cpp
> @@ -0,0 +1,112 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.

As you're adding this code, perhaps this should be
	Copyright (C) 2019, Linaro

? (or 2020 now)

If you /want/ this to be Google to reflect the fact that this file was
duplicated from the IPU3 file then that's fine though, but it's up to
you I think.



> + *
> + * rkisp1_pipeline_test.cpp - Rockchip RK3399 rkisp1 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> rkisp1 =  enumerator->search(dm);

There is an extra space between the '=' and 'enumarator' above.

I'm sorry to say that I can't have expected you to see that when running
checkstyle.py on your side, as checkstyle.py was broken, and couldn't
parse your patch (its fault, not yours).

I've now fixed up checkstyle.py and submitted two patches to the list to
resolve it.

Other than the optional change to the copyright, and this extra space,
it looks like you've handled the earlier comments from Laurent, and this
gets the testing started on rkisp1... so

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Let us know what you want to do with the copyright message. If you want
to keep it the same, then we can fix this trivial space when applying.
--
Regards

Kieran


> +	if (!rkisp1) {
> +		cerr << "Failed to find rkisp1: test skip" << endl;
> +		return TestSkip;
> +	}
> +
> +	int ret = rkisp1->populate();
> +	if (ret) {
> +		cerr << "Failed to populate media device " 
> +			<< rkisp1->deviceNode() << endl;
> +		return TestFail;
> +	}
> +
> +	sensors_ = 0;
> +	const vector<MediaEntity *> &entities = 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
--
Kieran