[libcamera-devel,3/5] libcamera: ipu3: Break-out ipu3 header file
diff mbox series

Message ID 20190220131757.14004-4-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: IPU3: create CIO2 and IMGU devices
Related show

Commit Message

Jacopo Mondi Feb. 20, 2019, 1:17 p.m. UTC 
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.

Signed-off-by: Jacopo Mondi <jacopo@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

Comments

Niklas Söderlund Feb. 21, 2019, 4:05 p.m. UTC | #1 
Hi Jacopo,

Thanks for your patch.

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@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"
>  #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?

> +
> +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__ */
> -- 
> 2.20.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Feb. 21, 2019, 11:44 p.m. UTC | #2 
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@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__ */

Patch
diff mbox series 

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"
 #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"
+
+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__ */