[PATCH] libcamera: pipeline: Add Mali-C55 ISP pipeline

[Jacopo Mondi]

Hi Barnabás

On Fri, Mar 15, 2024 at 02:28:26AM +0000, Barnabás Pőcze wrote:
> Hi
>
>
> 2024. március 14., csütörtök 15:07 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:
>
> > [...]
> > diff --git a/src/libcamera/pipeline/mali-c55/mali-c55.cpp b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > new file mode 100644
> > index 000000000000..4508214b864d
> > --- /dev/null
> > +++ b/src/libcamera/pipeline/mali-c55/mali-c55.cpp
> > @@ -0,0 +1,1081 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2024, Ideas on Board Oy
> > + *
> > + * mali-c55.cpp - Pipeline Handler for ARM's Mali-C55 ISP
> > + */
> > +
> > +#include <algorithm>
> > +#include <array>
> > +#include <map>
> > +#include <memory>
> > +#include <set>
> > +#include <string>
> > +
> > +#include <linux/media-bus-format.h>
> > +#include <linux/media.h>
> > +
> > +#include <libcamera/base/log.h>
> > +
> > +#include <libcamera/camera.h>
> > +#include <libcamera/formats.h>
> > +#include <libcamera/geometry.h>
> > +#include <libcamera/stream.h>
> > +
> > +#include "libcamera/internal/bayer_format.h"
> > +#include "libcamera/internal/camera.h"
> > +#include "libcamera/internal/camera_sensor.h"
> > +#include "libcamera/internal/device_enumerator.h"
> > +#include "libcamera/internal/media_device.h"
> > +#include "libcamera/internal/pipeline_handler.h"
> > +#include "libcamera/internal/v4l2_subdevice.h"
> > +#include "libcamera/internal/v4l2_videodevice.h"
> > +
> > +namespace {
> > +
> > +bool isFormatRaw(const libcamera::PixelFormat &pixFmt)
> > +{
> > +	return libcamera::PixelFormatInfo::info(pixFmt).colourEncoding ==
> > +	       libcamera::PixelFormatInfo::ColourEncodingRAW;
> > +}
> > +
> > +}; /* namespace */
> > +
> > +namespace libcamera {
>
> Does this need to be in the libcamera namespace? This will export a lot of

Do you mean the pipeline handler and the associated classes ? Maybe we
should partition these even further in a per-pipeline namespaces, yes

> symbols that don't really need to be exported as far as I can see.

What are the implications in the generated .so file of exposing these
symbols in the libcamera namespace ? I don't think further namespace
nesting has any implication on the symbols' external visibility, but
it's only meant to restrict access to symbols within the code


> (Maybe in the future?) (Although to be fair, this affects most [all?] pipeline handlers.)

yeah, all pipelines (but rpi which has nested namespaces) are like
this if I'm not mistaken..

>
>
> > [...]
> > +std::unique_ptr<CameraConfiguration>
> > +PipelineHandlerMaliC55::generateConfiguration(Camera *camera,
> > +					      Span<const StreamRole> roles)
> > +{
> > +	MaliC55CameraData *data = cameraData(camera);
> > +	std::unique_ptr<CameraConfiguration> config =
> > +		std::make_unique<MaliC55CameraConfiguration>(data);
> > +	bool frPipeAvailable = true;
> > +
> > +	if (roles.empty())
> > +		return config;
> > +
> > +	/* Check if one stream is RAW to reserve the FR pipe for it. */
> > +	if (std::find_if(roles.begin(), roles.end(),
> > +			 [](const StreamRole &role) {
> > +				 return role == StreamRole::Raw;
> > +			 }) != roles.end())
>
> Doesn't
>
>   if (std::find(roles.begin(), roles.end(), StreamRole::Raw) != roles.end())
>
> work?
>

I presume it does, thanks!

>
> > +		frPipeAvailable = false;
> > +
> > +	for (const StreamRole &role : roles) {
> > +		struct MaliC55Pipe *pipe;
> > +
> > +		/* Assign pipe for this role. */
> > +		if (role == StreamRole::Raw) {
> > +			pipe = &pipes_[MaliC55FR];
> > +		} else {
> > +			if (frPipeAvailable) {
> > +				pipe = &pipes_[MaliC55FR];
> > +				frPipeAvailable = false;
> > +			} else {
> > +				pipe = &pipes_[MaliC55DS];
> > +			}
> > +		}
> > +
> > +		Size size = std::min(Size{ 1920, 1080 }, data->resolution());
> > +		PixelFormat pixelFormat;
> > +
> > +		switch (role) {
> > +		case StreamRole::StillCapture:
> > +			size = data->resolution();
> > +			/* fall-through */
>
>   [[fallthrough]];

Ah C++..

>
> ?
>
> > +		case StreamRole::VideoRecording:
> > +			pixelFormat = formats::NV12;
> > +			break;
> > +
> > +		case StreamRole::Viewfinder:
> > +			pixelFormat = formats::RGB565;
> > +			break;
> > +
> > +		case StreamRole::Raw:
> > +			pixelFormat = data->bestRawFormat();
> > +			if (!pixelFormat.isValid()) {
> > +				LOG(MaliC55, Error)
> > +					<< "Camera does not support RAW formats";
> > +				continue;
> > +			}
> > +
> > +			size = data->resolution();
> > +			break;
> > +
> > +		default:
> > +			LOG(MaliC55, Error)
> > +				<< "Requested stream role not supported: " << role;
> > +			return config;
> > +		}
> > [...]
>
>
> Regards,
> Barnabás Pőcze