[libcamera-devel,09/14] libcamera: converter: Introduce dw100 converter
diff mbox series

Message ID 20220908184850.1874303-10-xavier.roumegue@oss.nxp.com
State Changes Requested
Headers show
Series
  • Add dw100 dewarper support to simple/rkisp1 pipeline
Related show

Commit Message

Xavier Roumegue Sept. 8, 2022, 6:48 p.m. UTC 
Add converter support for the Vivante DW100 Dewarp Processor IP core
found on i.MX8MP SoC.

The processor core applies a programmable geometrical transformation on
input images to correct distorsion introduced by lenses.
The transformation function is exposed as a grid map with 16x16 pixel
macroblocks indexed using X, Y vertex coordinates.

A set of input/output vertices mapping can be loaded at runtime through
a configuration file. If no mapping is loaded, the vertices mapping
fallbacks to an identity transformation. Scaling and pixel format
conversion can be used independently of the vertices remapping.

Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
---
 include/libcamera/internal/converter_dw100.h | 25 +++++++++++++++
 include/libcamera/internal/meson.build       |  1 +
 src/libcamera/converter_dw100.cpp            | 32 ++++++++++++++++++++
 src/libcamera/meson.build                    |  1 +
 4 files changed, 59 insertions(+)
 create mode 100644 include/libcamera/internal/converter_dw100.h
 create mode 100644 src/libcamera/converter_dw100.cpp

Comments

Laurent Pinchart Oct. 3, 2022, 11:02 p.m. UTC | #1 
Hi Xavier,

Thank you for the patch.

On Thu, Sep 08, 2022 at 08:48:45PM +0200, Xavier Roumegue via libcamera-devel wrote:
> Add converter support for the Vivante DW100 Dewarp Processor IP core
> found on i.MX8MP SoC.
> 
> The processor core applies a programmable geometrical transformation on
> input images to correct distorsion introduced by lenses.
> The transformation function is exposed as a grid map with 16x16 pixel
> macroblocks indexed using X, Y vertex coordinates.
> 
> A set of input/output vertices mapping can be loaded at runtime through
> a configuration file. If no mapping is loaded, the vertices mapping
> fallbacks to an identity transformation. Scaling and pixel format
> conversion can be used independently of the vertices remapping.
> 
> Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
> ---
>  include/libcamera/internal/converter_dw100.h | 25 +++++++++++++++
>  include/libcamera/internal/meson.build       |  1 +
>  src/libcamera/converter_dw100.cpp            | 32 ++++++++++++++++++++
>  src/libcamera/meson.build                    |  1 +
>  4 files changed, 59 insertions(+)
>  create mode 100644 include/libcamera/internal/converter_dw100.h
>  create mode 100644 src/libcamera/converter_dw100.cpp
> 
> diff --git a/include/libcamera/internal/converter_dw100.h b/include/libcamera/internal/converter_dw100.h
> new file mode 100644
> index 00000000..1972d6a2
> --- /dev/null
> +++ b/include/libcamera/internal/converter_dw100.h
> @@ -0,0 +1,25 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright 2022 NXP
> + *
> + * converter_dw100.h - V4l2 M2M dw100 format converter interface
> + */
> +
> +#pragma once
> +
> +#include <linux/dw100.h>
> +
> +#include "libcamera/internal/converter_v4l2_m2m.h"
> +#include "libcamera/internal/media_device.h"

You can drop this header, it's guaranteed to be included by
libcamera/internal/converter_v4l2_m2m.h as the MediaDevice is passed to
the V4L2M2MConverter constructor.

> +
> +namespace libcamera {
> +
> +class DW100Converter : public V4L2M2MConverter
> +{
> +public:
> +	DW100Converter(MediaDevice *media)
> +		: V4L2M2MConverter(media){};

	DW100Converter(MediaDevice *media)
		: V4L2M2MConverter(media)
	{
	};

> +	virtual int applyMapping(Stream *stream, Mapping &mapping) override;

s/virtual //

> +};
> +
> +} /* namespace libcamera */
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index 132de5ef..c2dd094f 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -20,6 +20,7 @@ libcamera_internal_headers = files([
>      'control_serializer.h',
>      'control_validator.h',
>      'converter.h',
> +    'converter_dw100.h',
>      'converter_v4l2_m2m.h',
>      'delayed_controls.h',
>      'device_enumerator.h',
> diff --git a/src/libcamera/converter_dw100.cpp b/src/libcamera/converter_dw100.cpp
> new file mode 100644
> index 00000000..b079fb37
> --- /dev/null
> +++ b/src/libcamera/converter_dw100.cpp
> @@ -0,0 +1,32 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright 2022 NXP
> + *
> + * converter_dw100.cpp - V4L2 M2M dw100 format converter
> + */
> +
> +#include "libcamera/internal/converter_dw100.h"
> +
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/controls.h>
> +
> +#include "libcamera/internal/v4l2_videodevice.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(Converter)

Not used.

> +
> +int DW100Converter::applyMapping(Stream *stream, Mapping &mapping)
> +{
> +	ControlList ctrls;
> +	auto value = Span<const int32_t>(reinterpret_cast<const int32_t *>(mapping.getMapping()), mapping.getLength());
> +	ControlValue c(value);
> +	ctrls.set(V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP, c);
> +	stream->m2m_->capture()->setControls(&ctrls);
> +	return 0;
> +}

As we have a single converter that can perform dewarping, I'd prefer
moving the Mapping support from V4L2M2MConverter to this class. It's
hard to predict what other dewarpers will need, and designing an API
based on a single example usually doesn't produce the best design. We
can always refactor the code later when we'll get a second device.

> +
> +REGISTER_CONVERTER("dw100", DW100Converter)
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index b12c8401..83da3e5f 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -14,6 +14,7 @@ libcamera_sources = files([
>      'control_serializer.cpp',
>      'control_validator.cpp',
>      'converter.cpp',
> +    'converter_dw100.cpp',
>      'converter_v4l2_m2m.cpp',
>      'delayed_controls.cpp',
>      'device_enumerator.cpp',
Xavier Roumegue Oct. 7, 2022, 12:52 p.m. UTC | #2 
Hi Laurent,

On 10/4/22 01:02, Laurent Pinchart wrote:
> Hi Xavier,
> 
> Thank you for the patch.
> 
> On Thu, Sep 08, 2022 at 08:48:45PM +0200, Xavier Roumegue via libcamera-devel wrote:
>> Add converter support for the Vivante DW100 Dewarp Processor IP core
>> found on i.MX8MP SoC.
>>
>> The processor core applies a programmable geometrical transformation on
>> input images to correct distorsion introduced by lenses.
>> The transformation function is exposed as a grid map with 16x16 pixel
>> macroblocks indexed using X, Y vertex coordinates.
>>
>> A set of input/output vertices mapping can be loaded at runtime through
>> a configuration file. If no mapping is loaded, the vertices mapping
>> fallbacks to an identity transformation. Scaling and pixel format
>> conversion can be used independently of the vertices remapping.
>>
>> Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
>> ---
>>   include/libcamera/internal/converter_dw100.h | 25 +++++++++++++++
>>   include/libcamera/internal/meson.build       |  1 +
>>   src/libcamera/converter_dw100.cpp            | 32 ++++++++++++++++++++
>>   src/libcamera/meson.build                    |  1 +
>>   4 files changed, 59 insertions(+)
>>   create mode 100644 include/libcamera/internal/converter_dw100.h
>>   create mode 100644 src/libcamera/converter_dw100.cpp
>>
>> diff --git a/include/libcamera/internal/converter_dw100.h b/include/libcamera/internal/converter_dw100.h
>> new file mode 100644
>> index 00000000..1972d6a2
>> --- /dev/null
>> +++ b/include/libcamera/internal/converter_dw100.h
>> @@ -0,0 +1,25 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright 2022 NXP
>> + *
>> + * converter_dw100.h - V4l2 M2M dw100 format converter interface
>> + */
>> +
>> +#pragma once
>> +
>> +#include <linux/dw100.h>
>> +
>> +#include "libcamera/internal/converter_v4l2_m2m.h"
>> +#include "libcamera/internal/media_device.h"
> 
> You can drop this header, it's guaranteed to be included by
> libcamera/internal/converter_v4l2_m2m.h as the MediaDevice is passed to
> the V4L2M2MConverter constructor.
> 
>> +
>> +namespace libcamera {
>> +
>> +class DW100Converter : public V4L2M2MConverter
>> +{
>> +public:
>> +	DW100Converter(MediaDevice *media)
>> +		: V4L2M2MConverter(media){};
> 
> 	DW100Converter(MediaDevice *media)
> 		: V4L2M2MConverter(media)
> 	{
> 	};
> 
That's not aligned with what requests utils/checkstype.py

@@ -39,9 +37,7 @@

  public:
  	DW100Converter(MediaDevice *media)
-		: V4L2M2MConverter(media)
-	{
-	};
+		: V4L2M2MConverter(media){};

Either way is fine for me... So ?


>> +	virtual int applyMapping(Stream *stream, Mapping &mapping) override;
> 
> s/virtual //
> 
>> +};
>> +
>> +} /* namespace libcamera */
>> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
>> index 132de5ef..c2dd094f 100644
>> --- a/include/libcamera/internal/meson.build
>> +++ b/include/libcamera/internal/meson.build
>> @@ -20,6 +20,7 @@ libcamera_internal_headers = files([
>>       'control_serializer.h',
>>       'control_validator.h',
>>       'converter.h',
>> +    'converter_dw100.h',
>>       'converter_v4l2_m2m.h',
>>       'delayed_controls.h',
>>       'device_enumerator.h',
>> diff --git a/src/libcamera/converter_dw100.cpp b/src/libcamera/converter_dw100.cpp
>> new file mode 100644
>> index 00000000..b079fb37
>> --- /dev/null
>> +++ b/src/libcamera/converter_dw100.cpp
>> @@ -0,0 +1,32 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright 2022 NXP
>> + *
>> + * converter_dw100.cpp - V4L2 M2M dw100 format converter
>> + */
>> +
>> +#include "libcamera/internal/converter_dw100.h"
>> +
>> +#include <libcamera/base/log.h>
>> +
>> +#include <libcamera/controls.h>
>> +
>> +#include "libcamera/internal/v4l2_videodevice.h"
>> +
>> +namespace libcamera {
>> +
>> +LOG_DECLARE_CATEGORY(Converter)
> 
> Not used.
> 
>> +
>> +int DW100Converter::applyMapping(Stream *stream, Mapping &mapping)
>> +{
>> +	ControlList ctrls;
>> +	auto value = Span<const int32_t>(reinterpret_cast<const int32_t *>(mapping.getMapping()), mapping.getLength());
>> +	ControlValue c(value);
>> +	ctrls.set(V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP, c);
>> +	stream->m2m_->capture()->setControls(&ctrls);
>> +	return 0;
>> +}
> 
> As we have a single converter that can perform dewarping, I'd prefer
> moving the Mapping support from V4L2M2MConverter to this class. It's
> hard to predict what other dewarpers will need, and designing an API
> based on a single example usually doesn't produce the best design. We
> can always refactor the code later when we'll get a second device.
> 
>> +
>> +REGISTER_CONVERTER("dw100", DW100Converter)
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
>> index b12c8401..83da3e5f 100644
>> --- a/src/libcamera/meson.build
>> +++ b/src/libcamera/meson.build
>> @@ -14,6 +14,7 @@ libcamera_sources = files([
>>       'control_serializer.cpp',
>>       'control_validator.cpp',
>>       'converter.cpp',
>> +    'converter_dw100.cpp',
>>       'converter_v4l2_m2m.cpp',
>>       'delayed_controls.cpp',
>>       'device_enumerator.cpp',
>
Laurent Pinchart Oct. 7, 2022, 1:21 p.m. UTC | #3 
Hi Xavier,

On Fri, Oct 07, 2022 at 02:52:19PM +0200, Xavier Roumegue (OSS) wrote:
> On 10/4/22 01:02, Laurent Pinchart wrote:
> > On Thu, Sep 08, 2022 at 08:48:45PM +0200, Xavier Roumegue via libcamera-devel wrote:
> >> Add converter support for the Vivante DW100 Dewarp Processor IP core
> >> found on i.MX8MP SoC.
> >>
> >> The processor core applies a programmable geometrical transformation on
> >> input images to correct distorsion introduced by lenses.
> >> The transformation function is exposed as a grid map with 16x16 pixel
> >> macroblocks indexed using X, Y vertex coordinates.
> >>
> >> A set of input/output vertices mapping can be loaded at runtime through
> >> a configuration file. If no mapping is loaded, the vertices mapping
> >> fallbacks to an identity transformation. Scaling and pixel format
> >> conversion can be used independently of the vertices remapping.
> >>
> >> Signed-off-by: Xavier Roumegue <xavier.roumegue@oss.nxp.com>
> >> ---
> >>   include/libcamera/internal/converter_dw100.h | 25 +++++++++++++++
> >>   include/libcamera/internal/meson.build       |  1 +
> >>   src/libcamera/converter_dw100.cpp            | 32 ++++++++++++++++++++
> >>   src/libcamera/meson.build                    |  1 +
> >>   4 files changed, 59 insertions(+)
> >>   create mode 100644 include/libcamera/internal/converter_dw100.h
> >>   create mode 100644 src/libcamera/converter_dw100.cpp
> >>
> >> diff --git a/include/libcamera/internal/converter_dw100.h b/include/libcamera/internal/converter_dw100.h
> >> new file mode 100644
> >> index 00000000..1972d6a2
> >> --- /dev/null
> >> +++ b/include/libcamera/internal/converter_dw100.h
> >> @@ -0,0 +1,25 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright 2022 NXP
> >> + *
> >> + * converter_dw100.h - V4l2 M2M dw100 format converter interface
> >> + */
> >> +
> >> +#pragma once
> >> +
> >> +#include <linux/dw100.h>
> >> +
> >> +#include "libcamera/internal/converter_v4l2_m2m.h"
> >> +#include "libcamera/internal/media_device.h"
> > 
> > You can drop this header, it's guaranteed to be included by
> > libcamera/internal/converter_v4l2_m2m.h as the MediaDevice is passed to
> > the V4L2M2MConverter constructor.
> > 
> >> +
> >> +namespace libcamera {
> >> +
> >> +class DW100Converter : public V4L2M2MConverter
> >> +{
> >> +public:
> >> +	DW100Converter(MediaDevice *media)
> >> +		: V4L2M2MConverter(media){};
> > 
> > 	DW100Converter(MediaDevice *media)
> > 		: V4L2M2MConverter(media)
> > 	{
> > 	};
> > 
> 
> That's not aligned with what requests utils/checkstype.py
> 
> @@ -39,9 +37,7 @@
> 
>   public:
>   	DW100Converter(MediaDevice *media)
> -		: V4L2M2MConverter(media)
> -	{
> -	};
> +		: V4L2M2MConverter(media){};
> 
> Either way is fine for me... So ?

I need to find out how to get clang-format to behave the way we want
here. Please ignore the recommendation from checkstyle.py here, and also
drop the ; after } as that's not needed.

> >> +	virtual int applyMapping(Stream *stream, Mapping &mapping) override;
> > 
> > s/virtual //
> > 
> >> +};
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> >> index 132de5ef..c2dd094f 100644
> >> --- a/include/libcamera/internal/meson.build
> >> +++ b/include/libcamera/internal/meson.build
> >> @@ -20,6 +20,7 @@ libcamera_internal_headers = files([
> >>       'control_serializer.h',
> >>       'control_validator.h',
> >>       'converter.h',
> >> +    'converter_dw100.h',
> >>       'converter_v4l2_m2m.h',
> >>       'delayed_controls.h',
> >>       'device_enumerator.h',
> >> diff --git a/src/libcamera/converter_dw100.cpp b/src/libcamera/converter_dw100.cpp
> >> new file mode 100644
> >> index 00000000..b079fb37
> >> --- /dev/null
> >> +++ b/src/libcamera/converter_dw100.cpp
> >> @@ -0,0 +1,32 @@
> >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> >> +/*
> >> + * Copyright 2022 NXP
> >> + *
> >> + * converter_dw100.cpp - V4L2 M2M dw100 format converter
> >> + */
> >> +
> >> +#include "libcamera/internal/converter_dw100.h"
> >> +
> >> +#include <libcamera/base/log.h>
> >> +
> >> +#include <libcamera/controls.h>
> >> +
> >> +#include "libcamera/internal/v4l2_videodevice.h"
> >> +
> >> +namespace libcamera {
> >> +
> >> +LOG_DECLARE_CATEGORY(Converter)
> > 
> > Not used.
> > 
> >> +
> >> +int DW100Converter::applyMapping(Stream *stream, Mapping &mapping)
> >> +{
> >> +	ControlList ctrls;
> >> +	auto value = Span<const int32_t>(reinterpret_cast<const int32_t *>(mapping.getMapping()), mapping.getLength());
> >> +	ControlValue c(value);
> >> +	ctrls.set(V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP, c);
> >> +	stream->m2m_->capture()->setControls(&ctrls);
> >> +	return 0;
> >> +}
> > 
> > As we have a single converter that can perform dewarping, I'd prefer
> > moving the Mapping support from V4L2M2MConverter to this class. It's
> > hard to predict what other dewarpers will need, and designing an API
> > based on a single example usually doesn't produce the best design. We
> > can always refactor the code later when we'll get a second device.
> > 
> >> +
> >> +REGISTER_CONVERTER("dw100", DW100Converter)
> >> +
> >> +} /* namespace libcamera */
> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> >> index b12c8401..83da3e5f 100644
> >> --- a/src/libcamera/meson.build
> >> +++ b/src/libcamera/meson.build
> >> @@ -14,6 +14,7 @@ libcamera_sources = files([
> >>       'control_serializer.cpp',
> >>       'control_validator.cpp',
> >>       'converter.cpp',
> >> +    'converter_dw100.cpp',
> >>       'converter_v4l2_m2m.cpp',
> >>       'delayed_controls.cpp',
> >>       'device_enumerator.cpp',

Patch
diff mbox series 

diff --git a/include/libcamera/internal/converter_dw100.h b/include/libcamera/internal/converter_dw100.h
new file mode 100644
index 00000000..1972d6a2
--- /dev/null
+++ b/include/libcamera/internal/converter_dw100.h
@@ -0,0 +1,25 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright 2022 NXP
+ *
+ * converter_dw100.h - V4l2 M2M dw100 format converter interface
+ */
+
+#pragma once
+
+#include <linux/dw100.h>
+
+#include "libcamera/internal/converter_v4l2_m2m.h"
+#include "libcamera/internal/media_device.h"
+
+namespace libcamera {
+
+class DW100Converter : public V4L2M2MConverter
+{
+public:
+	DW100Converter(MediaDevice *media)
+		: V4L2M2MConverter(media){};
+	virtual int applyMapping(Stream *stream, Mapping &mapping) override;
+};
+
+} /* namespace libcamera */
diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
index 132de5ef..c2dd094f 100644
--- a/include/libcamera/internal/meson.build
+++ b/include/libcamera/internal/meson.build
@@ -20,6 +20,7 @@  libcamera_internal_headers = files([
     'control_serializer.h',
     'control_validator.h',
     'converter.h',
+    'converter_dw100.h',
     'converter_v4l2_m2m.h',
     'delayed_controls.h',
     'device_enumerator.h',
diff --git a/src/libcamera/converter_dw100.cpp b/src/libcamera/converter_dw100.cpp
new file mode 100644
index 00000000..b079fb37
--- /dev/null
+++ b/src/libcamera/converter_dw100.cpp
@@ -0,0 +1,32 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright 2022 NXP
+ *
+ * converter_dw100.cpp - V4L2 M2M dw100 format converter
+ */
+
+#include "libcamera/internal/converter_dw100.h"
+
+#include <libcamera/base/log.h>
+
+#include <libcamera/controls.h>
+
+#include "libcamera/internal/v4l2_videodevice.h"
+
+namespace libcamera {
+
+LOG_DECLARE_CATEGORY(Converter)
+
+int DW100Converter::applyMapping(Stream *stream, Mapping &mapping)
+{
+	ControlList ctrls;
+	auto value = Span<const int32_t>(reinterpret_cast<const int32_t *>(mapping.getMapping()), mapping.getLength());
+	ControlValue c(value);
+	ctrls.set(V4L2_CID_DW100_DEWARPING_16x16_VERTEX_MAP, c);
+	stream->m2m_->capture()->setControls(&ctrls);
+	return 0;
+}
+
+REGISTER_CONVERTER("dw100", DW100Converter)
+
+} /* namespace libcamera */
diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
index b12c8401..83da3e5f 100644
--- a/src/libcamera/meson.build
+++ b/src/libcamera/meson.build
@@ -14,6 +14,7 @@  libcamera_sources = files([
     'control_serializer.cpp',
     'control_validator.cpp',
     'converter.cpp',
+    'converter_dw100.cpp',
     'converter_v4l2_m2m.cpp',
     'delayed_controls.cpp',
     'device_enumerator.cpp',