[PATCH v4 4/5] libcamera: converter: Add DW100 converter class

[Paul Elder]

On Thu, Jun 27, 2024 at 07:16:55PM +0530, Umang Jain wrote:
> DW100 Dewarp engine is present on i.MX8MP SoC. This patch
> provides a converter class (inherited from V4L2M2MConverter).
> 
> The DW100 converter will be used for scaling capabilites hence, has
> a helper to set scaler crop rectangles. Plumbing in the RkISP1
> pipeline-handler is done in the subsequent patch.
> 
> The ConverterDW100 class can be extended later (as additional
> development) to support the dewarping by setting a dewarp vertex map.
> 
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  .../internal/converter/converter_dw100.h      | 26 ++++++++
>  .../libcamera/internal/converter/meson.build  |  1 +
>  src/libcamera/converter/converter_dw100.cpp   | 64 +++++++++++++++++++
>  src/libcamera/converter/meson.build           |  1 +
>  4 files changed, 92 insertions(+)
>  create mode 100644 include/libcamera/internal/converter/converter_dw100.h
>  create mode 100644 src/libcamera/converter/converter_dw100.cpp
> 
> diff --git a/include/libcamera/internal/converter/converter_dw100.h b/include/libcamera/internal/converter/converter_dw100.h
> new file mode 100644
> index 00000000..dbd39e08
> --- /dev/null
> +++ b/include/libcamera/internal/converter/converter_dw100.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board Oy
> + *
> + * i.MX8MP Dewarp Engine integration
> + */
> +
> +#pragma once
> +
> +#include "libcamera/internal/converter/converter_v4l2_m2m.h"
> +
> +namespace libcamera {
> +
> +class MediaDevice;
> +class Rectangle;
> +class Stream;
> +
> +class ConverterDW100 : public V4L2M2MConverter
> +{
> +public:
> +	ConverterDW100(std::shared_ptr<MediaDevice> media);
> +
> +	int setScalerCrop(const Stream *stream, Rectangle *rect);
> +};
> +
> +} /* namespace libcamera */
> diff --git a/include/libcamera/internal/converter/meson.build b/include/libcamera/internal/converter/meson.build
> index 891e79e7..85007a4b 100644
> --- a/include/libcamera/internal/converter/meson.build
> +++ b/include/libcamera/internal/converter/meson.build
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  libcamera_internal_headers += files([
> +    'converter_dw100.h',
>      'converter_v4l2_m2m.h',
>  ])
> diff --git a/src/libcamera/converter/converter_dw100.cpp b/src/libcamera/converter/converter_dw100.cpp
> new file mode 100644
> index 00000000..78802615
> --- /dev/null
> +++ b/src/libcamera/converter/converter_dw100.cpp
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (C) 2024, Ideas On Board Oy
> + *
> + * i.MX8MP Dewarp Engine integration
> + */
> +
> +#include "libcamera/internal/converter/converter_dw100.h"
> +
> +#include <libcamera/base/log.h>
> +
> +#include <libcamera/geometry.h>
> +
> +#include "libcamera/internal/media_device.h"
> +#include "libcamera/internal/v4l2_videodevice.h"
> +
> +namespace libcamera {
> +
> +LOG_DECLARE_CATEGORY(Converter)
> +
> +/**
> + * \class libcamera::ConverterDW100
> + * \brief The i.MX8MP dewarp converter implements the converter interface based
> + * on V4L2 M2M device.
> +*/
> +
> +/**
> + * \fn ConverterDW100::ConverterDW100
> + * \brief Construct a ConverterDW100 instance
> + * \param[in] media The media device implementing the converter
> + */
> +ConverterDW100::ConverterDW100(std::shared_ptr<MediaDevice> media)
> +	: V4L2M2MConverter(media.get(), Feature::Crop)
> +{
> +}
> +
> +/**
> + * \brief Set the scaler crop rectangle \a rect on \a stream
> + * \param[in] stream Pointer to output stream
> + * \param[inout] rect The selection rectangle to be applied

rect doesn't seem to be used as an output parameter?

> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int ConverterDW100::setScalerCrop(const Stream *stream, Rectangle *rect)
> +{
> +	Rectangle apply = *rect;

This dereference scares me but I see you want to use it as an output
parameter (I assume to report the rectangle that was actually set).

> +	int ret;
> +
> +	ret = setCrop(stream, &apply);
> +	if (ret < 0)
> +		LOG(Converter, Error) << "Failed to set scaler crop "
> +				      << apply.toString() << " on dewarper "
> +				      << strerror(-ret);

Can't we just return here?

> +
> +	if (*rect != apply) {
> +		/* \todo This case needs proper after handling. */

I think it's ok to just return the scaler crop that was able to be set.


Paul

> +		LOG(Converter, Warning) << "Applied rectangle " << apply.toString()
> +					<< " differs from requested " << rect->toString();
> +	}
> +
> +	return ret;
> +}
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/converter/meson.build b/src/libcamera/converter/meson.build
> index 2aa72fe4..175581b8 100644
> --- a/src/libcamera/converter/meson.build
> +++ b/src/libcamera/converter/meson.build
> @@ -1,5 +1,6 @@
>  # SPDX-License-Identifier: CC0-1.0
>  
>  libcamera_sources += files([
> +        'converter_dw100.cpp',
>          'converter_v4l2_m2m.cpp'
>  ])
> -- 
> 2.44.0
>