[{"id":37567,"web_url":"https://patchwork.libcamera.org/comment/37567/","msgid":"","date":"2026-01-12T11:30:05","subject":"Re: [PATCH v4 06/22] libcamera: base: Add alignment utility\n\tfunctions","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"On Tue, Jan 06, 2026 at 05:57:38PM +0100, Barnabás Pőcze wrote:\n> Add a couple internal functions to check alignment, and to\n> align integers, pointers up to a given alignment.\n>\n> Signed-off-by: Barnabás Pőcze \n> Reviewed-by: Paul Elder \n> Reviewed-by: Kieran Bingham \n> ---\n> changes in v3:\n> * documentation\n> ---\n> include/libcamera/base/internal/align.h | 135 ++++++++++++++++++++++++\n\nThe previous c++20 polyfill introduced include/libcamera/base/internal/\n\nDo you think without c++20 support this should be moved to\ninclude/libcamera/base where utils.h lives ?\n\n> include/libcamera/base/meson.build | 1 +\n> 2 files changed, 136 insertions(+)\n> create mode 100644 include/libcamera/base/internal/align.h\n>\n> diff --git a/include/libcamera/base/internal/align.h b/include/libcamera/base/internal/align.h\n> new file mode 100644\n> index 000000000..d8ee4e369\n> --- /dev/null\n> +++ b/include/libcamera/base/internal/align.h\n> @@ -0,0 +1,135 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas on Board Oy\n> + *\n> + * Alignment utilities\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +\n> +/**\n> + * \\internal\n> + * \\file align.h\n> + * \\brief Utilities for handling alignment\n> + */\n\nWhat is the policy for documenting in headers ? I see only mojo files\nand swisp_stats.h doing that.\n\nWe have the following in out coding-style rules\n\nDocumentation relates to header files, but shall be stored in the .cpp source\n\nShould we change it or should you introduce a corresponding .cpp file\nfor align.h ?\n\n> +\n> +namespace libcamera::internal::align {\n> +\n> +/**\n> + * \\internal\n\nis it me missing it, or I don't see this being used anywere else (am I\ngrepping wrong ?)\n\n> + * \\brief Check if pointer is aligned\n> + * \\param[in] p pointer to check\n\nThe documentation of function parameters should start with a capital\nletter, here and in the rest of the file (and series)\n\n> + * \\param[in] alignment desired alignment\n> + * \\return true if \\a p is at least \\a alignment aligned, false otherwise\n> + */\n> +inline bool is(const void *p, std::uintptr_t alignment)\n> +{\n> +\tassert(alignment > 0);\n> +\n> +\treturn reinterpret_cast(p) % alignment == 0;\n> +}\n> +\n> +/**\n> + * \\internal\n> + * \\brief Align number up\n\nMaybe: \"Align a value up\" or \"number\" as you've used already\n\n> + * \\param[in] x number to check\n\n\"The number (or value if you accept the above suggestion) to align up\n\n> + * \\param[in] alignment desired alignment\n\nThe desired alignment\n\n> + * \\return true if \\a p is at least \\a alignment aligned, false otherwise\n\nTrue\n\nWhat do you mean with \"at least ?\"\n\n\n> + */\n> +template\n> +constexpr T up(T x, cxx20::type_identity_t alignment)\n> +{\n> +\tstatic_assert(std::is_unsigned_v);\n> +\tassert(alignment > 0);\n> +\n> +\tconst auto padding = (alignment - (x % alignment)) % alignment;\n\nIsn't \"alignement - (x % alignment)\" always < alignment ?\nIn other words, do you need the last \"% alignment\" ?\n\n> +\tassert(x + padding >= x);\n\nunless padding is negative, which I don't think it can happen, this\nwill always be true ?\n\n> +\n> +\treturn x + padding;\n> +}\n> +\n> +/**\n> + * \\internal\n> + * \\brief Align pointer up\n> + * \\param[in] p pointer to align\n> + * \\param[in] alignment desired alignment\n> + * \\return \\a p up-aligned to \\a alignment\n\n\n> + */\n> +template\n> +auto *up(T *p, std::uintptr_t alignment)\n> +{\n> +\tusing U = std::conditional_t<\n> +\t\tstd::is_const_v,\n> +\t\tconst std::byte,\n> +\t\tstd::byte\n> +\t>;\n> +\n> +\treturn reinterpret_cast(up(reinterpret_cast(p), alignment));\n\nI'm not sure why you're casting to (const) std::byte instead of\nreturning a T *\n\nAlso, I would question the idea of aligning a pointer to an arbitrary\n'alignment'. Should we guarantee that alignment is a multiple of of\nsizeof(T) ?\n\n> +}\n> +\n> +/**\n> + * \\internal\n> + * \\brief Align pointer up\n> + * \\param[in] size required number of bytes\n> + * \\param[in] alignment required alignment\n> + * \\param[in] ptr base pointer\n> + * \\param[in] avail number of available bytes\n> + *\n> + * This function checks if the storage starting at \\a ptr and continuing for \\a avail\n> + * bytes (if \\a avail is nullptr, then no size checking is done) can accommodate \\a size\n> + * bytes of data aligned to \\a alignment. If so, then a pointer to the start is the returned\n> + * and \\a ptr is adjusted to point right after the section of \\a size bytes. If present,\n> + * \\a avail is also adjusted.\n\ndoc over 80 cols\n\n> + *\n> + * Similar to std::align().\n\nI read the std::align() documentation as\n\n--------------------------------------------------------------------------------\nvoid* align( std::size_t alignment,\n std::size_t size,\n void*& ptr,\n std::size_t& space );\n\nGiven a pointer ptr to a buffer of size space, returns a pointer\naligned by the specified alignment for size number of bytes and\ndecreases space argument by the number of bytes used for alignment.\nThe first aligned address is returned.\n\nThe function modifies the pointer only if it would be possible to fit\nthe wanted number of bytes aligned by the given alignment into the\nbuffer. If the buffer is too small, the function does nothing and\nreturns nullptr.\n\nThe behavior is undefined if alignment is not a power of two.\n--------------------------------------------------------------------------------\n\nCould you help me understand where the difference is except for the\norder of parameters and the optional availability of 'avail' ?\n\nAccording to your previous reply to the RFC the inteneded usage\npattern of this function is:\n\nauto *p = details::align::up(buf, &avail); // allocate suitable aligned and sized storage\nauto *x = new (p) SomeType { ... }; // construct object in said storage\n\nThe only different I see with using plain std::align is that you can't\neasily use the placement new semantic with the return value of\nstd::align but you should rather use a plain assignment ?\n\nauto *p = std::align(...);\n*p = SomeType { ... };\n\n?\n\nI actually tried to re-model your\n\ntemplate\nU *up(T *&ptr, std::size_t *avail = nullptr)\n\nwith using std::align()\n\ntemplate\nU *up(T *&ptr, std::size_t *avail)\n{\n\tstd::size_t bufSize = *avail;\n\tauto *t = std::align(alignof(U), sizeof(U),\n\t\t\t reinterpret_cast(ptr), bufSize);\n\n\tt = static_cast(t) + sizeof(U);\n\n\t*avail = bufSize - sizeof(U);\n\treturn reinterpret_cast(t);\n}\n\nI presume you got a custom implementation because advancing \"ptr\"\nrequires quite some casting here and there. Is this the reason why a\ncustom re-implementation of std::align is needed ?\n\n> + *\n> + * \\return the appropriately aligned pointer, or nullptr if there is not enough space\n> + */\n> +template\n> +T *up(std::size_t size, std::size_t alignment, T *&ptr, std::size_t *avail = nullptr)\n> +{\n> +\tassert(alignment > 0);\n> +\n> +\tauto p = reinterpret_cast(ptr);\n> +\tconst auto padding = (alignment - (p % alignment)) % alignment;\n> +\n> +\tif (avail) {\n> +\t\tif (size > *avail || padding > *avail - size)\n> +\t\t\treturn nullptr;\n> +\n> +\t\t*avail -= size + padding;\n> +\t}\n> +\n> +\tp += padding;\n> +\tptr = reinterpret_cast(p + size);\n> +\n> +\treturn reinterpret_cast(p);\n> +}\n> +\n> +/**\n> + * \\internal\n> + * \\brief Align pointer up\n> + * \\tparam U desired type\n> + * \\param[in] ptr base pointer\n> + * \\param[in] avail number of available bytes\n> + *\n> + * A convenience wrapper around libcamera::internal::align::up(std::size_t, std::size_t, T *&, std::size_t *)\n> + * that takes the size and alignment information from the type \\a U.\n> + *\n> + * \\sa libcamera::internal::align::up(std::size_t, std::size_t, T *&, std::size_t *)\n> + */\n> +template\n> +U *up(T *&ptr, std::size_t *avail = nullptr)\n> +{\n> +\treturn reinterpret_cast, const U, U> *>(\n> +\t\tup(sizeof(U), alignof(U), ptr, avail)\n> +\t);\n> +}\n> +\n> +} /* namespace libcamera::internal::align */\n> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build\n> index bb4ed556b..305dfa0dd 100644\n> --- a/include/libcamera/base/meson.build\n> +++ b/include/libcamera/base/meson.build\n> @@ -34,6 +34,7 @@ libcamera_base_private_headers = files([\n>\n> libcamera_base_internal_headers = files([\n> 'internal/cxx20.h',\n> + 'internal/align.h',\n> ])\n>\n> libcamera_base_headers = [\n> --\n> 2.52.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 30C89BE08B\n\tfor ;\n\tMon, 12 Jan 2026 11:30:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 62E4E61FC1;\n\tMon, 12 Jan 2026 12:30:10 +0100 (CET)","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 3AFD16142F\n\tfor ;\n\tMon, 12 Jan 2026 12:30:08 +0100 (CET)","from ideasonboard.com (net-93-65-100-155.cust.vodafonedsl.it\n\t[93.65.100.155])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AFF3F63F;\n\tMon, 12 Jan 2026 12:29:42 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"t07NfvkE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1768217382;\n\tbh=q2ch8u/F9whSMh81XgytnaHZfof4Pr2GTdRTXugvHOM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=t07NfvkE0c2d3jFLRWioXWeZmiLZB9NvUsOk7xMnBsLsKL4miDngYTn7SWD/bjyQ8\n\t6vdTCca2N9qQg43/cGwpL322AnN3IdFgKo3g4DduPVQ8EGge8jxEMgRZ6dylrtY6rR\n\tWEJL89Cuw9Q9SC2NJdnMV0l2u+aR4r2Q6rGJGCmU=","Date":"Mon, 12 Jan 2026 12:30:05 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ,\n\tKieran Bingham ","Subject":"Re: [PATCH v4 06/22] libcamera: base: Add alignment utility\n\tfunctions","Message-ID":"","References":"<20260106165754.1759831-1-barnabas.pocze@ideasonboard.com>\n\t<20260106165754.1759831-7-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20260106165754.1759831-7-barnabas.pocze@ideasonboard.com>","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\" "}}]