[{"id":34181,"web_url":"https://patchwork.libcamera.org/comment/34181/","msgid":"<174697622921.4078945.1507876877981071804@ping.linuxembedded.co.uk>","date":"2025-05-11T15:10:29","subject":"Re: [PATCH v2 8/8] libcamera: Add new atomisp pipeline handler","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hans de Goede (2025-05-10 15:12:20)\n> Add a basic atomisp pipeline handler which supports configuring\n> the pipeline, capturing frames and selecting front/back sensor.\n> \n> The atomisp ISP needs some extra lines/columns when debayering and also\n> has some max resolution limitations, this causes the available output\n> resolutions to differ from the sensor resolutions.\n> \n> The atomisp driver's Android heritage means that it mostly works as a non\n> media-controller centric v4l2 device, primarily controlled through its\n> /dev/video# node. The driver takes care of setting up the pipeline itself\n> propagating try / set fmt calls down from its single /dev/video# node to\n> the selected sensor taking the necessary padding, etc. into account.\n> \n> Therefor things like getting the list of support formats / sizes and\n> setFmt() calls are all done on the /dev/video# node instead of on subdevs,\n> this avoids having to duplicate the padding, etc. logic in the pipeline\n> handler.\n> \n> Since the statistics buffers which we get from the ISP2 are not documented\n> this uses the swstats_cpu and simple-IPA from the swisp. At the moment only\n> aec/agc is supported.\n> \n> awb support will be added in a follow-up patch.\n> \n> Signed-off-by: Hans de Goede \n> ---\n> meson.build | 1 +\n> meson_options.txt | 1 +\n> src/ipa/simple/data/uncalibrated_atomisp.yaml | 7 +\n> src/libcamera/pipeline/atomisp/atomisp.cpp | 636 ++++++++++++++++++\n> src/libcamera/pipeline/atomisp/meson.build | 5 +\n> src/libcamera/software_isp/meson.build | 2 +-\n> 6 files changed, 651 insertions(+), 1 deletion(-)\n> create mode 100644 src/ipa/simple/data/uncalibrated_atomisp.yaml\n> create mode 100644 src/libcamera/pipeline/atomisp/atomisp.cpp\n> create mode 100644 src/libcamera/pipeline/atomisp/meson.build\n> \n> diff --git a/meson.build b/meson.build\n> index 9ba5e2ca..5c4981d8 100644\n> --- a/meson.build\n> +++ b/meson.build\n> @@ -211,6 +211,7 @@ wanted_pipelines = get_option('pipelines')\n> arch_arm = ['arm', 'aarch64']\n> arch_x86 = ['x86', 'x86_64']\n> pipelines_support = {\n> + 'atomisp': arch_x86,\n> 'imx8-isi': arch_arm,\n> 'ipu3': arch_x86,\n> 'mali-c55': arch_arm,\n> diff --git a/meson_options.txt b/meson_options.txt\n> index 2104469e..c7051ee7 100644\n> --- a/meson_options.txt\n> +++ b/meson_options.txt\n> @@ -47,6 +47,7 @@ option('pipelines',\n> value : ['auto'],\n> choices : [\n> 'all',\n> + 'atomisp',\n\nall and auto are 'magic' choices. Previously we were lucky that they\nwere together alphabetically - I wonder if we should keep them together\nat the top of the list (with a blank line separator?) to keep them\ngrouped?\n\nBut also - just keeping it inline alphabetical isn't going to make this\nharder to see the grouping until we have an allwinner pipeline handler,\namlogic pipeline handler, and an apple pipeline handler .... err ...\nmaybe there are more 'a's than I first thought ...\n\nWhich makes me think grouping 'all', 'auto' as always first might be\nmore justified ...\n\n\n> 'auto',\n> 'imx8-isi',\n> 'ipu3',\n> diff --git a/src/ipa/simple/data/uncalibrated_atomisp.yaml b/src/ipa/simple/data/uncalibrated_atomisp.yaml\n> new file mode 100644\n> index 00000000..6dcc0295\n> --- /dev/null\n> +++ b/src/ipa/simple/data/uncalibrated_atomisp.yaml\n\nCan't this use the same 'uncalibrated.yaml' - the file here is supposed\nto be IPA specific - ... does it need to be different for atomisp?\n\n\n> @@ -0,0 +1,7 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +%YAML 1.1\n> +---\n> +version: 1\n> +algorithms:\n> + - Agc:\n> +...\n> diff --git a/src/libcamera/pipeline/atomisp/atomisp.cpp b/src/libcamera/pipeline/atomisp/atomisp.cpp\n> new file mode 100644\n> index 00000000..959c3f0d\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/atomisp/atomisp.cpp\n> @@ -0,0 +1,636 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * atomisp.cpp - Pipeline handler for atomisp devices\n> + *\n> + * The atomisp ISP needs some extra lines/columns when debayering and also has\n> + * some max resolution limitations, this causes the available output resolutions\n> + * to differ from the sensor resolutions.\n\nI think we need such concepts more globally in libcamera too - so that\nwe can define that a sensor is configured to a larger size and the\nISP/downstream components will be able to apply a mandatory crop (for\nimage processing reasons) meaning a smaller output size than the input.\n\n\n> + * The atomisp driver's Android heritage means that it mostly works as a non\n> + * media-controller centric v4l2 device, primarily controlled through its\n> + * /dev/video# node. The driver takes care of setting up the pipeline itself\n> + * propagating try / set fmt calls down from its single /dev/video# node to\n> + * the selected sensor taking the necessary padding, etc. into account.\n> + *\n> + * Therefor things like getting the list of support formats / sizes and tryFmt()\n\ns/Therefor/Therefore/\n\nhttps://www.grammarly.com/blog/commonly-confused-words/therefore-vs-therefor/\n\n(I think ... it's still hard to be sure :D)\n\n> + * / setFmt() calls are all done on the /dev/video# node instead of on subdevs,\n> + * this avoids having to duplicate the padding, etc. logic here.\n> + * Note this requires enabling the ISP <-> CSI receiver for the current sensor\n> + * even when only querying / trying formats.\n> + *\n> + * Copyright (C) 2024, Hans de Goede \n> + *\n> + * Partially based on simple.cpp and uvcvideo.cpp which are:\n> + * Copyright (C) 2020, Laurent Pinchart\n> + * Copyright (C) 2019, Martijn Braam\n> + * Copyright (C) 2019, Google Inc.\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/camera.h\"\n> +#include \"libcamera/internal/camera_sensor.h\"\n> +#include \"libcamera/internal/delayed_controls.h\"\n> +#include \"libcamera/internal/device_enumerator.h\"\n> +#include \"libcamera/internal/ipa_manager.h\"\n> +#include \"libcamera/internal/media_device.h\"\n> +#include \"libcamera/internal/pipeline_handler.h\"\n> +#include \"libcamera/internal/software_isp/debayer_params.h\"\n> +#include \"libcamera/internal/software_isp/swstats_cpu.h\"\n> +#include \"libcamera/internal/v4l2_videodevice.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(Atomisp)\n> +\n> +class AtomispCameraData : public Camera::Private\n> +{\n> +public:\n> + AtomispCameraData(PipelineHandler *pipe, V4L2VideoDevice *video)\n> + : Camera::Private(pipe), video_(video)\n> + {\n> + }\n> +\n> + int init(MediaEntity *sensor);\n> + void imageBufferReady(FrameBuffer *buffer);\n> + void statsReady(uint32_t frame, uint32_t bufferId);\n> + void setSensorControls(const ControlList &sensorControls);\n> +\n> + /* This is owned by AtomispPipelineHandler and shared by the cameras */\n> + V4L2VideoDevice *video_;\n> + std::unique_ptr sensor_;\n> + std::unique_ptr delayedCtrls_;\n> + std::unique_ptr stats_;\n> + std::unique_ptr ipa_;\n> + SharedMemObject debayerParams_;\n> + Stream stream_;\n> + std::map> formats_;\n> + MediaLink *csiReceiverIspLink_;\n> +};\n> +\n> +class AtomispCameraConfiguration : public CameraConfiguration\n> +{\n> +public:\n> + AtomispCameraConfiguration()\n> + : CameraConfiguration() {}\n> +\n> + Status validate() override;\n> +};\n> +\n> +class AtomispPipelineHandler : public PipelineHandler\n> +{\n> +public:\n> + AtomispPipelineHandler(CameraManager *manager)\n> + : PipelineHandler(manager) {}\n> +\n> + std::unique_ptr generateConfiguration(Camera *camera,\n> + Span roles) override;\n> + int configure(Camera *camera, CameraConfiguration *config) override;\n> +\n> + int exportFrameBuffers(Camera *camera, Stream *stream,\n> + std::vector> *buffers) override;\n> +\n> + int start(Camera *camera, const ControlList *controls) override;\n> + void stopDevice(Camera *camera) override;\n> +\n> + int queueRequestDevice(Camera *camera, Request *request) override;\n> +\n> + bool match(DeviceEnumerator *enumerator) override;\n> +\n> +private:\n> + bool acquireDevice(Camera *camera) override;\n> + void releaseDevice(Camera *camera) override;\n> +\n> + AtomispCameraData *cameraData(Camera *camera)\n> + {\n> + return static_cast(camera->_d());\n> + }\n> +\n> + std::unique_ptr video_;\n> +};\n> +\n> +bool AtomispPipelineHandler::acquireDevice(Camera *camera)\n> +{\n> + AtomispCameraData *data = cameraData(camera);\n> +\n> + /* atomisp can run only 1 sensor / camera at a time */\n> + if (data->video_->isOpen())\n> + return false;\n> +\n> + int ret = data->video_->open();\n> + if (ret != 0)\n> + return false;\n> +\n> + ret = data->csiReceiverIspLink_->setEnabled(true);\n> + if (ret) {\n> + data->video_->close();\n> + return false;\n> + }\n> +\n> + return true;\n> +}\n> +\n> +void AtomispPipelineHandler::releaseDevice(Camera *camera)\n> +{\n> + AtomispCameraData *data = cameraData(camera);\n> +\n> + data->csiReceiverIspLink_->setEnabled(false);\n> + data->video_->close();\n> +}\n> +\n> +CameraConfiguration::Status AtomispCameraConfiguration::validate()\n> +{\n> + Status status = Valid;\n> +\n> + if (config_.empty())\n> + return Invalid;\n> +\n> + if (orientation != Orientation::Rotate0) {\n> + orientation = Orientation::Rotate0;\n> + status = Adjusted;\n> + }\n> +\n> + /* Cap the number of entries to the available streams. */\n> + if (config_.size() > 1) {\n> + config_.resize(1);\n> + status = Adjusted;\n> + }\n> +\n> + StreamConfiguration &cfg = config_[0];\n> + const StreamFormats &formats = cfg.formats();\n> + const PixelFormat pixelFormat = cfg.pixelFormat;\n> + const Size size = cfg.size;\n> +\n> + const std::vector pixelFormats = formats.pixelformats();\n> + auto iter = std::find(pixelFormats.begin(), pixelFormats.end(), pixelFormat);\n> + if (iter == pixelFormats.end()) {\n> + cfg.pixelFormat = pixelFormats.front();\n> + LOG(Atomisp, Debug)\n> + << \"Adjusting pixel format from \" << pixelFormat\n> + << \" to \" << cfg.pixelFormat;\n> + status = Adjusted;\n> + }\n> +\n> + const std::vector &formatSizes = formats.sizes(cfg.pixelFormat);\n> + cfg.size = formatSizes.front();\n> + for (const Size &formatsSize : formatSizes) {\n> + if (formatsSize > size)\n> + break;\n> +\n> + cfg.size = formatsSize;\n> + }\n> +\n> + if (cfg.size != size) {\n> + LOG(Atomisp, Debug)\n> + << \"Adjusting size from \" << size << \" to \" << cfg.size;\n> + status = Adjusted;\n> + }\n> +\n> + /*\n> + * The atomisp has an internal pipeline length of 3 frames,\n> + * it generates 3A statistics info 1 - 2 frames before generating\n> + * the video buffer with the final image.\n> + *\n> + * We need to make sure this pipeline is fed with buffers all\n> + * the time since the firmware simply asserts on buffer underruns\n> + * after which the driver has to recover by stopping streaming,\n> + * resetting the ISP and then starting the stream again.\n> + *\n> + * Use a buffercount of 8 so that we can have 1 - 3 buffers waiting\n> + * on userspace while still having 2 reserve buffers owned by\n> + * the kernel + 3 buffers owned by the ISP.\n> + */\n> + cfg.bufferCount = 8;\n\nLots of active discussions about how to fix/change these ... but I\ndon't think you need to change anything until we have a resolution.\nPerhaps getting atomisp pipeline handler merged as soon as possible will\nhelp to keep it aligned there ;-)\n\n\n> +\n> + switch (cfg.pixelFormat) {\n> + case formats::YUV420:\n> + /* atomisp stride must be a multiple of 32 */\n> + cfg.stride = (cfg.size.width + 31) & ~31;\n\nI think you can use alignUp(value, alignment) from\ninclude/libcamera/base/utils for this.\n\n\n> + cfg.frameSize = cfg.stride * cfg.size.height * 3 / 2;\n\nShouldn't all this information come from the kernel though ?\n\n> + break;\n> + default:\n> + LOG(Atomisp, Error)\n> + << \"Unknown pixel-format \" << cfg.pixelFormat;\n> + return Invalid;\n> + }\n> +\n> + if (cfg.colorSpace != ColorSpace::Rec709) {\n> + cfg.colorSpace = ColorSpace::Rec709;\n> + status = Adjusted;\n> + }\n> +\n> + return status;\n> +}\n> +\n> +std::unique_ptr\n> +AtomispPipelineHandler::generateConfiguration(Camera *camera,\n> + Span roles)\n> +{\n> + AtomispCameraData *data = cameraData(camera);\n> + std::unique_ptr config =\n> + std::make_unique();\n> +\n> + if (roles.empty())\n> + return config;\n> +\n> + StreamFormats formats(data->formats_);\n> + StreamConfiguration cfg(formats);\n> +\n> + cfg.pixelFormat = formats.pixelformats().front();\n> + cfg.size = formats.sizes(cfg.pixelFormat).back();\n> + cfg.bufferCount = 4;\n\nMaybe we can drop cfg.bufferCount here as it gets reset in validate()\nbelow?\n\n> + config->addConfiguration(cfg);\n> +\n> + config->validate();\n> +\n> + return config;\n> +}\n> +\n> +int AtomispPipelineHandler::configure(Camera *camera, CameraConfiguration *config)\n> +{\n> + AtomispCameraData *data = cameraData(camera);\n> + StreamConfiguration &cfg = config->at(0);\n> +\n> + int ret = data->stats_->configure(cfg);\n> + if (ret)\n> + return ret;\n> +\n> + ipa::soft::IPAConfigInfo configInfo;\n> + configInfo.sensorControls = data->sensor_->controls();\n> +\n> + ret = data->ipa_->configure(configInfo);\n> + if (ret)\n> + return ret;\n> +\n> + V4L2PixelFormat fourcc = data->video_->toV4L2PixelFormat(cfg.pixelFormat);\n> + V4L2DeviceFormat format;\n> +\n> + format.size = cfg.size;\n> + format.fourcc = fourcc;\n> + ret = data->video_->setFormat(&format);\n> + if (ret)\n> + return ret;\n> +\n> + if (format.size != cfg.size) {\n> + LOG(Atomisp, Error)\n> + << \"format mismatch req \" << cfg.size << \" got \" << format.size;\n> + return -EINVAL;\n> + }\n> +\n> + if (format.fourcc != fourcc) {\n> + LOG(Atomisp, Error)\n> + << \"format mismatch req \" << fourcc << \" got \" << format.fourcc;\n> + return -EINVAL;\n> + }\n> +\n> + data->stats_->setWindow(Rectangle(cfg.size));\n> +\n> + cfg.setStream(&data->stream_);\n> +\n> + std::unordered_map params = {\n> + { V4L2_CID_ANALOGUE_GAIN, { 2, false } },\n> + { V4L2_CID_EXPOSURE, { 2, false } },\n> + };\n\nCan this already be updated to get the values from the CameraSensor\nHelpers ? Otherwise this is 'wrong' already for n% of devices. Or at\nleast query and fall back to uncalibrated defaults.\n\n> + data->delayedCtrls_ =\n> + std::make_unique(data->sensor_->device(),\n> + params);\n> + data->video_->frameStart.connect(data->delayedCtrls_.get(),\n> + &DelayedControls::applyControls);\n> + return 0;\n> +}\n> +\n> +int AtomispPipelineHandler::exportFrameBuffers(Camera *camera, Stream *stream,\n> + std::vector> *buffers)\n> +{\n> + AtomispCameraData *data = cameraData(camera);\n> + unsigned int count = stream->configuration().bufferCount;\n> +\n> + return data->video_->exportBuffers(count, buffers);\n> +}\n> +\n> +int AtomispPipelineHandler::start(Camera *camera, [[maybe_unused]] const ControlList *controls)\n> +{\n> + AtomispCameraData *data = cameraData(camera);\n> + unsigned int count = data->stream_.configuration().bufferCount;\n> + int ret;\n> +\n> + ret = data->video_->importBuffers(count);\n> + if (ret)\n> + return ret;\n> +\n> + video_->bufferReady.connect(data, &AtomispCameraData::imageBufferReady);\n> +\n> + ret = data->video_->streamOn();\n> + if (ret) {\n> + video_->bufferReady.disconnect(data, &AtomispCameraData::imageBufferReady);\n> + data->video_->releaseBuffers();\n> + return ret;\n> + }\n> +\n> + ret = data->ipa_->start();\n> + if (ret) {\n> + data->video_->streamOff();\n> + video_->bufferReady.disconnect(data, &AtomispCameraData::imageBufferReady);\n> + data->video_->releaseBuffers();\n> + return ret;\n> + }\n> +\n> + return 0;\n> +}\n> +\n> +void AtomispPipelineHandler::stopDevice(Camera *camera)\n> +{\n> + AtomispCameraData *data = cameraData(camera);\n> +\n> + data->video_->streamOff();\n> + video_->bufferReady.disconnect(data, &AtomispCameraData::imageBufferReady);\n> + data->video_->releaseBuffers();\n> + data->ipa_->stop();\n> +}\n> +\n> +int AtomispPipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> +{\n> + AtomispCameraData *data = cameraData(camera);\n> + FrameBuffer *buffer = request->findBuffer(&data->stream_);\n> + if (!buffer) {\n> + LOG(Atomisp, Error)\n> + << \"Attempt to queue request with invalid stream\";\n> +\n> + return -ENOENT;\n> + }\n> +\n> + int ret = data->video_->queueBuffer(buffer);\n> + if (ret)\n> + return ret;\n> +\n> + data->ipa_->queueRequest(request->sequence(), request->controls());\n> + return 0;\n> +}\n> +\n> +/* -----------------------------------------------------------------------------\n> + * Match and Setup\n> + */\n> +bool AtomispPipelineHandler::match(DeviceEnumerator *enumerator)\n> +{\n> + std::vector sensors;\n> + MediaEntity *videoEntity = NULL;\n> + DeviceMatch dm(\"atomisp-isp2\");\n> + MediaDevice *media;\n> +\n> + media = acquireMediaDevice(enumerator, dm);\n> + if (!media)\n> + return false;\n> +\n> + for (MediaEntity *entity : media->entities()) {\n> + switch (entity->function()) {\n> + case MEDIA_ENT_F_CAM_SENSOR:\n> + sensors.push_back(entity);\n> + break;\n> + case MEDIA_ENT_F_IO_V4L:\n> + videoEntity = entity;\n> + break;\n> + }\n> + }\n> +\n> + if (!videoEntity) {\n> + LOG(Atomisp, Error) << \"Could not find the video device\";\n> + return false;\n> + }\n> + if (sensors.empty()) {\n> + LOG(Atomisp, Error) << \"No sensor found\";\n> + return false;\n> + }\n> +\n> + /* Create and open the video device. */\n> + video_ = std::make_unique(videoEntity);\n> + int ret = video_->open();\n> + if (ret)\n> + return false;\n> +\n> + /* Create and register a camera for each sensor */\n> + bool registered = false;\n> + for (MediaEntity *sensor : sensors) {\n> + std::unique_ptr data =\n> + std::make_unique(this, video_.get());\n> +\n> + if (data->init(sensor))\n> + continue;\n> +\n> + const std::string &id = data->sensor_->id();\n> + std::set streams{ &data->stream_ };\n> + std::shared_ptr camera =\n> + Camera::create(std::move(data), id, streams);\n> + registerCamera(std::move(camera));\n> + registered = true;\n> + }\n> +\n> + /*\n> + * atomisp cameras share a single /dev/video# node. The shared node\n> + * gets opened from acquireDevice() to allow only one camera to be\n> + * acquired at a time.\n> + * This also works around a kernel bug (which needs to be fixed) where\n> + * the node needs to be closed for the ISP to runtime-suspend.\n> + */\n> + video_->close();\n> +\n> + return registered;\n> +}\n> +\n> +int AtomispCameraData::init(MediaEntity *sensor)\n> +{\n> + MediaEntity *source, *sink, *csi_receiver = NULL;\n> + const MediaPad *source_pad;\n> + int source_pad_idx = 0;\n> +\n> + sensor_ = CameraSensorFactoryBase::create(sensor);\n> + if (!sensor_)\n> + return -ENODEV;\n> +\n> + debayerParams_ = SharedMemObject(\"debayer_params\");\n> + if (!debayerParams_) {\n> + LOG(Atomisp, Error) << \"Failed to create shared memory for parameters\";\n> + return -ENOMEM;\n> + }\n> +\n> + stats_ = std::make_unique();\n> + if (!stats_->isValid()) {\n> + LOG(Atomisp, Error) << \"Failed to create SwStatsCpu object\";\n> + return -ENOMEM;\n> + }\n> +\n> + ipa_ = IPAManager::createIPA(pipe(), 0, 0, \"simple\");\n> + if (!ipa_) {\n> + LOG(Atomisp, Error) << \"Creating IPA failed\";\n> + return -ENOMEM;\n> + }\n> +\n> + /*\n> + * The API tuning file is made from the sensor name. If the tuning file\n> + * isn't found, fall back to the 'uncalibrated' file.\n> + */\n> + std::string ipaTuningFile =\n> + ipa_->configurationFile(sensor_->model() + \"_atomisp.yaml\",\n> + \"uncalibrated_atomisp.yaml\");\n> +\n> + IPACameraSensorInfo sensorInfo{};\n> + int ret = sensor_->sensorInfo(&sensorInfo);\n> + if (ret) {\n> + LOG(Atomisp, Error) << \"Camera sensor information not available\";\n> + return -ENODEV;\n> + }\n> +\n> + /* Passing CCM parameters to the ISP is not support (yet?) */\n> + bool ccmEnabled = false;\n> +\n> + ret = ipa_->init(IPASettings{ ipaTuningFile, sensor_->model() },\n> + stats_->getStatsFD(),\n> + debayerParams_.fd(),\n> + sensorInfo,\n> + sensor_->controls(),\n> + &controlInfo_,\n> + &ccmEnabled);\n> + if (ret) {\n> + LOG(Atomisp, Error) << \"IPA init failed\";\n> + return ret;\n> + }\n> +\n> + source = sensor;\n> + for (int i = 0; i < 2; i++) {\n> + source_pad = source->getPadByIndex(source_pad_idx);\n> + if (source_pad == nullptr) {\n> + LOG(Atomisp, Error)\n> + << source << \" doesn't have pad \" << source_pad_idx;\n> + return -ENODEV;\n> + }\n> +\n> + sink = source_pad->links()[0]->sink()->entity();\n> + switch (sink->function()) {\n> + case MEDIA_ENT_F_VID_IF_BRIDGE:\n> + /* Found the CSI2 receiver */\n> + csi_receiver = sink;\n> + break;\n> + case MEDIA_ENT_F_PROC_VIDEO_ISP:\n> + /*\n> + * Sensor with builtin ISP, e.g. MT9M114.\n> + * CSI receiver is downstream of this entity.\n> + */\n> + source = sink;\n> + source_pad_idx = 1;\n> + continue;\n> + default:\n> + LOG(Atomisp, Error)\n> + << sink << \" has unexpected function \" << sink->function();\n> + return -ENODEV;\n> + }\n> + }\n> + if (!csi_receiver) {\n> + LOG(Atomisp, Error)\n> + << \"Camera \" << sensor_->model() << \" cannot find CSI receiver\";\n> + return -ENODEV;\n> + }\n> +\n> + source_pad = csi_receiver->getPadByIndex(1);\n> + if (source_pad == nullptr) {\n> + LOG(Atomisp, Error)\n> + << \"CSI receiver for \" << sensor_->model() << \" doesn't have pad1\";\n> + return -ENODEV;\n> + }\n> +\n> + csiReceiverIspLink_ = source_pad->links()[0];\n> +\n> + ret = csiReceiverIspLink_->setEnabled(true);\n> + if (ret)\n> + return ret;\n> +\n> + /*\n> + * The atomisp supports many different output formats but SwStatsCpu,\n> + * used for 3A due to atomisp statistics being undocumented,\n> + * only supports a few formats.\n> + */\n> + std::vector supported({ formats::YUV420 });\n> +\n> + for (const auto &format : video_->formats()) {\n> + PixelFormat pixelFormat = format.first.toPixelFormat();\n> +\n> + if (std::find(supported.begin(), supported.end(), pixelFormat) == supported.end())\n> + continue;\n> +\n> + formats_[pixelFormat] = format.second;\n> + }\n> +\n> + csiReceiverIspLink_->setEnabled(false);\n\nWhy do we disable this link here? (Maybe a comment above, but I'm not\nsure I've understood why it needed to be disabled - is it also part of\nthe power management state ?)\n\n> +\n> + if (formats_.empty()) {\n> + LOG(Atomisp, Error)\n> + << \"Camera \" << sensor_->model()\n> + << \" doesn't expose any supported format\";\n> + return -EINVAL;\n> + }\n> +\n> + properties_ = sensor_->properties();\n> +\n> + stats_->statsReady.connect(this, &AtomispCameraData::statsReady);\n> + ipa_->setSensorControls.connect(this, &AtomispCameraData::setSensorControls);\n> +\n> + return 0;\n> +}\n> +\n> +void AtomispCameraData::imageBufferReady(FrameBuffer *buffer)\n> +{\n> + Request *request = buffer->request();\n> +\n> + if (buffer->metadata().status == FrameMetadata::FrameSuccess) {\n> + ipa_->computeParams(request->sequence());\n> +\n> + /*\n> + * Buffer ids are currently not used, so pass zero as buffer id.\n> + *\n> + * \\todo Pass real bufferId once stats buffer passing is changed.\n> + */\n> + stats_->processFrame(request->sequence(), 0, buffer);\n\nWe've spent a long time this week discussing how things /really/ should\nbe paced in IPAs from the sensor sequence numbers. Does this mean the\nbuffer ID is not available in the kernel driver? Or are you setting zero\nbecause you don't think it's used yet.\n\nDo we have a way to obtain the expected sensor sequence number ? (which\nI assume comes from the buffer sequence number at some level, but could\nbe something different depending on the hardware/driver implementation).\n\nNothing to block this now though ...\n\n> +\n> + request->metadata().set(controls::SensorTimestamp,\n> + buffer->metadata().timestamp);\n> + }\n> +\n> + pipe()->completeBuffer(request, buffer);\n> + pipe()->completeRequest(request);\n> +}\n> +\n> +void AtomispCameraData::statsReady(uint32_t frame, uint32_t bufferId)\n> +{\n> + ipa_->processStats(frame, bufferId, delayedCtrls_->get(frame));\n\nAnd this is where it's important. I think we need to make sure that\n'frame' here is a frame / sensor sequence number for timings.\n\n> +}\n> +\n> +void AtomispCameraData::setSensorControls(const ControlList &sensorControls)\n> +{\n> + delayedCtrls_->push(sensorControls);\n> + ControlList ctrls(sensorControls);\n> + sensor_->setControls(&ctrls);\n> +}\n> +\n> +REGISTER_PIPELINE_HANDLER(AtomispPipelineHandler, \"atomisp\")\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/pipeline/atomisp/meson.build b/src/libcamera/pipeline/atomisp/meson.build\n> new file mode 100644\n> index 00000000..179a35ef\n> --- /dev/null\n> +++ b/src/libcamera/pipeline/atomisp/meson.build\n> @@ -0,0 +1,5 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +libcamera_internal_sources += files([\n> + 'atomisp.cpp',\n> +])\n> diff --git a/src/libcamera/software_isp/meson.build b/src/libcamera/software_isp/meson.build\n> index 59fa5f02..81c4c255 100644\n> --- a/src/libcamera/software_isp/meson.build\n> +++ b/src/libcamera/software_isp/meson.build\n> @@ -1,6 +1,6 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> -softisp_enabled = pipelines.contains('simple')\n> +softisp_enabled = pipelines.contains('simple') or pipelines.contains('atomisp')\n> summary({'SoftISP support' : softisp_enabled}, section : 'Configuration')\n> \n\nI'd like to see this merged ... I know you're going to maintain this,\nand build upon it as you further develop it - and I think that's easier\nfor you if the base gets in.\n\nSo any of my comments above are for you to consider as you wish:\n\nAcked-by: Kieran Bingham \n\n> if not softisp_enabled\n> -- \n> 2.49.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4A0EBC3200\n\tfor ;\n\tSun, 11 May 2025 15:10:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A49C68B64;\n\tSun, 11 May 2025 17:10:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E114468B51\n\tfor ;\n\tSun, 11 May 2025 17:10:31 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8CC21605;\n\tSun, 11 May 2025 17:10:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"p8rsFvaf\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746976217;\n\tbh=PRXh3UZtq0o3INDHQx7CKDvrEDs98SieGRcYGAiCQTM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=p8rsFvafGCsbzYsT6stpvol2dIghRQPN9uhvFkEoZPHroDPk4zmyjBOx5BUdXMVKI\n\tongWlFJ53qcriPmwxDLOJXbX68rKznvDkvmBKOkuPPNnVaC4r4+X9sDhDX/lZDBCGZ\n\tSPhPofhxk5rYFp6pOcZfZSsM0BAuZw/4AOjk7KGM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250510141220.54872-9-hdegoede@redhat.com>","References":"<20250510141220.54872-1-hdegoede@redhat.com>\n\t<20250510141220.54872-9-hdegoede@redhat.com>","Subject":"Re: [PATCH v2 8/8] libcamera: Add new atomisp pipeline handler","From":"Kieran Bingham ","Cc":"Milan Zamazal , Hans de Goede ","To":"Hans de Goede , libcamera-devel@lists.libcamera.org","Date":"Sun, 11 May 2025 16:10:29 +0100","Message-ID":"<174697622921.4078945.1507876877981071804@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]