[libcamera-devel] [PATCH 3/5] libcamera: ipu3: Break-out ipu3 header file

[Laurent Pinchart]

Hi Jacopo,

On Thu, Feb 21, 2019 at 05:05:28PM +0100, Niklas Söderlund wrote:
> On 2019-02-20 14:17:55 +0100, Jacopo Mondi wrote:
> > As the class grows, break out the class definitions in a separate header
> > file, which can be used by other ipu3-related cpp files that will be
> > added in next commits.
> 
> I would mention that there is no other change in this commit then moving 
> the class definition to a separate header file.
> 
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 56 +-----------------
> >  src/libcamera/pipeline/ipu3/ipu3.h   | 85 ++++++++++++++++++++++++++++
> >  2 files changed, 86 insertions(+), 55 deletions(-)
> >  create mode 100644 src/libcamera/pipeline/ipu3/ipu3.h
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 9065073913a2..07029dd763c9 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -14,6 +14,7 @@
> >  
> >  #include "device_enumerator.h"
> >  #include "log.h"
> > +#include "ipu3.h"

Alphabetical order ? Bonus point to anyone who submits a patch for
checkstyle.py to catch these errors :-)

> >  #include "media_device.h"
> >  #include "pipeline_handler.h"
> >  #include "utils.h"
> > @@ -24,61 +25,6 @@ namespace libcamera {
> >  
> >  LOG_DEFINE_CATEGORY(IPU3)
> >  
> > -class PipelineHandlerIPU3 : public PipelineHandler
> > -{
> > -public:
> > -	PipelineHandlerIPU3(CameraManager *manager);
> > -	~PipelineHandlerIPU3();
> > -
> > -	std::map<Stream *, StreamConfiguration>
> > -	streamConfiguration(Camera *camera,
> > -			    std::vector<Stream *> &streams) override;
> > -	int configureStreams(Camera *camera,
> > -			     std::map<Stream *, StreamConfiguration> &config) override;
> > -
> > -	int allocateBuffers(Camera *camera, Stream *stream) override;
> > -	int freeBuffers(Camera *camera, Stream *stream) override;
> > -
> > -	int start(const Camera *camera) override;
> > -	void stop(const Camera *camera) override;
> > -
> > -	int queueRequest(const Camera *camera, Request *request) override;
> > -
> > -	bool match(DeviceEnumerator *enumerator);
> > -
> > -private:
> > -	class IPU3CameraData : public CameraData
> > -	{
> > -	public:
> > -		IPU3CameraData()
> > -			: cio2_(nullptr), csi2_(nullptr), sensor_(nullptr) {}
> > -
> > -		~IPU3CameraData()
> > -		{
> > -			delete cio2_;
> > -			delete csi2_;
> > -			delete sensor_;
> > -		}
> > -
> > -		V4L2Device *cio2_;
> > -		V4L2Subdevice *csi2_;
> > -		V4L2Subdevice *sensor_;
> > -
> > -		Stream stream_;
> > -	};
> > -
> > -	IPU3CameraData *cameraData(const Camera *camera)
> > -	{
> > -		return static_cast<IPU3CameraData *>(
> > -			PipelineHandler::cameraData(camera));
> > -	}
> > -
> > -	void registerCameras();
> > -
> > -	std::shared_ptr<MediaDevice> cio2_;
> > -	std::shared_ptr<MediaDevice> imgu_;
> > -};
> > -
> >  PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> >  	: PipelineHandler(manager), cio2_(nullptr), imgu_(nullptr)
> >  {
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.h b/src/libcamera/pipeline/ipu3/ipu3.h
> > new file mode 100644
> > index 000000000000..48c2a3e16980
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.h
> > @@ -0,0 +1,85 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2019, Google Inc.
> > + *
> > + * ipu3.h - Pipeline handler for Intel IPU3
> > + */
> > +
> > +#ifndef __LIBCAMERA_PIPELINE_IPU3_H__
> > +#define __LIBCAMERA_PIPELINE_IPU3_H__
> > +
> > +#include <memory>
> > +#include <vector>
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/request.h>
> > +#include <libcamera/stream.h>
> > +
> > +#include "device_enumerator.h"
> > +#include "media_device.h"
> > +#include "pipeline_handler.h"
> > +#include "v4l2_device.h"
> > +#include "v4l2_subdevice.h"
> 
> Do you really need to include all these headers here? Is it not enough 
> to include the bulk of them in the cpp file?

I think most of the classes can indeed be forwared-declared.

The patch otherwise looks fine to me, but neither patch 4/5 nor patch
5/5 need to access the PipelineHandlerIPU3 class. Do we really need to
split it out to ipu3.h ? I don't challenge the need of an ipu3.h header
to store the CIO2 and ImgU classes, but it seems that
PipelineHandlerIPU3 itself could stay in ipu3.cpp.

> > +
> > +namespace libcamera {
> > +
> > +class PipelineHandlerIPU3 : public PipelineHandler
> > +{
> > +public:
> > +	PipelineHandlerIPU3(CameraManager *manager);
> > +	~PipelineHandlerIPU3();
> > +
> > +	std::map<Stream *, StreamConfiguration>
> > +	streamConfiguration(Camera *camera,
> > +			    std::vector<Stream *> &streams) override;
> > +	int configureStreams(Camera *camera,
> > +			     std::map<Stream *, StreamConfiguration> &config) override;
> > +
> > +	int allocateBuffers(Camera *camera, Stream *stream) override;
> > +	int freeBuffers(Camera *camera, Stream *stream) override;
> > +
> > +	int start(const Camera *camera) override;
> > +	void stop(const Camera *camera) override;
> > +
> > +	int queueRequest(const Camera *camera, Request *request) override;
> > +
> > +	bool match(DeviceEnumerator *enumerator);
> > +
> > +private:
> > +	class IPU3CameraData : public CameraData
> > +	{
> > +	public:
> > +		IPU3CameraData()
> > +			: cio2_(nullptr), csi2_(nullptr), sensor_(nullptr) {}
> > +
> > +		~IPU3CameraData()
> > +		{
> > +			delete cio2_;
> > +			delete csi2_;
> > +			delete sensor_;
> > +		}
> > +
> > +		V4L2Device *cio2_;
> > +		V4L2Subdevice *csi2_;
> > +		V4L2Subdevice *sensor_;
> > +
> > +		Stream stream_;
> > +	};
> > +
> > +	IPU3CameraData *cameraData(const Camera *camera)
> > +	{
> > +		return static_cast<IPU3CameraData *>(
> > +			PipelineHandler::cameraData(camera));
> > +	}
> > +
> > +	void registerCameras();
> > +
> > +	std::shared_ptr<MediaDevice> cio2_;
> > +	std::shared_ptr<MediaDevice> imgu_;
> > +	IMGUDevice imgu0_;
> > +	IMGUDevice imgu1_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_PIPELINE_IPU3_H__ */

-- 
Regards,

Laurent Pinchart