[libcamera-devel] [PATCH 09/14] libcamera: converter: Introduce dw100 converter

[Xavier Roumegue (OSS)]

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 at 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',
>