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

[Laurent Pinchart]

Hello,

On Tue, Jan 07, 2020 at 03:56:16PM +0000, Kieran Bingham wrote:
> 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.

The following is also an option.

 * Copyright (C) 2019, Linaro
 *
 * Based on test/pipeline/ipu3/ipu3_pipeline_test.cpp
 *
 * Copyright (C) 2019, Google Inc.

> > + *
> > + * 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.
> 
> > +	if (!rkisp1) {
> > +		cerr << "Failed to find rkisp1: test skip" << endl;
> > +		return TestSkip;
> > +	}
> > +
> > +	int ret = rkisp1->populate();
> > +	if (ret) {
> > +		cerr << "Failed to populate media device " 

There's also an extra space at the end of the line here, nicely pointed
out by the fixed version of checkstyle.py :-)

> > +			<< 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,

Laurent Pinchart