[v4,06/22] libcamera: base: Add alignment utility functions

25636 diff mbox series

Add a couple internal functions to check alignment, and to
align integers, pointers up to a given alignment.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
changes in v3:
  * documentation
---
 include/libcamera/base/internal/align.h | 135 ++++++++++++++++++++++++
 include/libcamera/base/meson.build      |   1 +
 2 files changed, 136 insertions(+)
 create mode 100644 include/libcamera/base/internal/align.h

On Tue, Jan 06, 2026 at 05:57:38PM +0100, Barnabás Pőcze wrote:
> Add a couple internal functions to check alignment, and to
> align integers, pointers up to a given alignment.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> changes in v3:
>   * documentation
> ---
>  include/libcamera/base/internal/align.h | 135 ++++++++++++++++++++++++

The previous c++20 polyfill introduced include/libcamera/base/internal/

Do you think without c++20 support this should be moved to
include/libcamera/base where utils.h lives ?

>  include/libcamera/base/meson.build      |   1 +
>  2 files changed, 136 insertions(+)
>  create mode 100644 include/libcamera/base/internal/align.h
>
> diff --git a/include/libcamera/base/internal/align.h b/include/libcamera/base/internal/align.h
> new file mode 100644
> index 000000000..d8ee4e369
> --- /dev/null
> +++ b/include/libcamera/base/internal/align.h
> @@ -0,0 +1,135 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2025, Ideas on Board Oy
> + *
> + * Alignment utilities
> + */
> +
> +#pragma once
> +
> +#include <cassert>
> +#include <cstddef>
> +#include <cstdint>
> +
> +#include <libcamera/base/internal/cxx20.h>
> +
> +/**
> + * \internal
> + * \file align.h
> + * \brief Utilities for handling alignment
> + */

What is the policy for documenting in headers ? I see only mojo files
and swisp_stats.h doing that.

We have the following in out coding-style rules

Documentation relates to header files, but shall be stored in the .cpp source

Should we change it or should you introduce a corresponding .cpp file
for align.h ?

> +
> +namespace libcamera::internal::align {
> +
> +/**
> + * \internal

is it me missing it, or I don't see this being used anywere else (am I
grepping wrong ?)

> + * \brief Check if pointer is aligned
> + * \param[in] p pointer to check

The documentation of function parameters should start with a capital
letter, here and in the rest of the file (and series)

> + * \param[in] alignment desired alignment
> + * \return true if \a p is at least \a alignment aligned, false otherwise
> + */
> +inline bool is(const void *p, std::uintptr_t alignment)
> +{
> +	assert(alignment > 0);
> +
> +	return reinterpret_cast<std::uintptr_t>(p) % alignment == 0;
> +}
> +
> +/**
> + * \internal
> + * \brief Align number up

Maybe: "Align a value up" or "number" as you've used already

> + * \param[in] x number to check

"The number (or value if you accept the above suggestion) to align up

> + * \param[in] alignment desired alignment

The desired alignment

> + * \return true if \a p is at least \a alignment aligned, false otherwise

True

What do you mean with "at least ?"


> + */
> +template<typename T>
> +constexpr T up(T x, cxx20::type_identity_t<T> alignment)
> +{
> +	static_assert(std::is_unsigned_v<T>);
> +	assert(alignment > 0);
> +
> +	const auto padding = (alignment - (x % alignment)) % alignment;

Isn't "alignement - (x % alignment)" always < alignment ?
In other words, do you need the last "% alignment" ?

> +	assert(x + padding >= x);

unless padding is negative, which I don't think it can happen, this
will always be true ?

> +
> +	return x + padding;
> +}
> +
> +/**
> + * \internal
> + * \brief Align pointer up
> + * \param[in] p pointer to align
> + * \param[in] alignment desired alignment
> + * \return \a p up-aligned to \a alignment


> + */
> +template<typename T>
> +auto *up(T *p, std::uintptr_t alignment)
> +{
> +	using U = std::conditional_t<
> +		std::is_const_v<T>,
> +		const std::byte,
> +		std::byte
> +	>;
> +
> +	return reinterpret_cast<U *>(up(reinterpret_cast<std::uintptr_t>(p), alignment));

I'm not sure why you're casting to (const) std::byte instead of
returning a T *

Also, I would question the idea of aligning a pointer to an arbitrary
'alignment'. Should we guarantee that alignment is a multiple of of
sizeof(T) ?

> +}
> +
> +/**
> + * \internal
> + * \brief Align pointer up
> + * \param[in] size required number of bytes
> + * \param[in] alignment required alignment
> + * \param[in] ptr base pointer
> + * \param[in] avail number of available bytes
> + *
> + * This function checks if the storage starting at \a ptr and continuing for \a avail
> + * bytes (if \a avail is nullptr, then no size checking is done) can accommodate \a size
> + * bytes of data aligned to \a alignment. If so, then a pointer to the start is the returned
> + * and \a ptr is adjusted to point right after the section of \a size bytes. If present,
> + * \a avail is also adjusted.

doc over 80 cols

> + *
> + * Similar to std::align().

I read the std::align() documentation as

--------------------------------------------------------------------------------
void* align( std::size_t alignment,
             std::size_t size,
             void*& ptr,
             std::size_t& space );

Given a pointer ptr to a buffer of size space, returns a pointer
aligned by the specified alignment for size number of bytes and
decreases space argument by the number of bytes used for alignment.
The first aligned address is returned.

The function modifies the pointer only if it would be possible to fit
the wanted number of bytes aligned by the given alignment into the
buffer. If the buffer is too small, the function does nothing and
returns nullptr.

The behavior is undefined if alignment is not a power of two.
--------------------------------------------------------------------------------

Could you help me understand where the difference is except for the
order of parameters and the optional availability of 'avail' ?

According to your previous reply to the RFC the inteneded usage
pattern of this function is:

auto *p = details::align::up<SomeType>(buf, &avail); // allocate suitable aligned and sized storage
auto *x = new (p) SomeType { ... }; // construct object in said storage

The only different I see with using plain std::align is that you can't
easily use the placement new semantic with the return value of
std::align but you should rather use a plain assignment ?

auto *p = std::align(...);
*p = SomeType { ... };

?

I actually tried to re-model your

template<typename U, typename T>
U *up(T *&ptr, std::size_t *avail = nullptr)

with using std::align()

template<typename U, typename T>
U *up(T *&ptr, std::size_t *avail)
{
	std::size_t bufSize = *avail;
	auto *t = std::align(alignof(U), sizeof(U),
			     reinterpret_cast<void*&>(ptr), bufSize);

	t = static_cast<std::byte *>(t) + sizeof(U);

	*avail = bufSize - sizeof(U);
	return reinterpret_cast<U *>(t);
}

I presume you got a custom implementation because advancing "ptr"
requires quite some casting here and there. Is this the reason why a
custom re-implementation of std::align is needed ?

> + *
> + * \return the appropriately aligned pointer, or nullptr if there is not enough space
> + */
> +template<typename T>
> +T *up(std::size_t size, std::size_t alignment, T *&ptr, std::size_t *avail = nullptr)
> +{
> +	assert(alignment > 0);
> +
> +	auto p = reinterpret_cast<std::uintptr_t>(ptr);
> +	const auto padding = (alignment - (p % alignment)) % alignment;
> +
> +	if (avail) {
> +		if (size > *avail || padding > *avail - size)
> +			return nullptr;
> +
> +		*avail -= size + padding;
> +	}
> +
> +	p += padding;
> +	ptr = reinterpret_cast<T *>(p + size);
> +
> +	return reinterpret_cast<T *>(p);
> +}
> +
> +/**
> + * \internal
> + * \brief Align pointer up
> + * \tparam U desired type
> + * \param[in] ptr base pointer
> + * \param[in] avail number of available bytes
> + *
> + * A convenience wrapper around libcamera::internal::align::up(std::size_t, std::size_t, T *&, std::size_t *)
> + * that takes the size and alignment information from the type \a U.
> + *
> + * \sa libcamera::internal::align::up(std::size_t, std::size_t, T *&, std::size_t *)
> + */
> +template<typename U, typename T>
> +U *up(T *&ptr, std::size_t *avail = nullptr)
> +{
> +	return reinterpret_cast<std::conditional_t<std::is_const_v<T>, const U, U> *>(
> +		up(sizeof(U), alignof(U), ptr, avail)
> +	);
> +}
> +
> +} /* namespace libcamera::internal::align */
> diff --git a/include/libcamera/base/meson.build b/include/libcamera/base/meson.build
> index bb4ed556b..305dfa0dd 100644
> --- a/include/libcamera/base/meson.build
> +++ b/include/libcamera/base/meson.build
> @@ -34,6 +34,7 @@ libcamera_base_private_headers = files([
>
>  libcamera_base_internal_headers = files([
>      'internal/cxx20.h',
> +    'internal/align.h',
>  ])
>
>  libcamera_base_headers = [
> --
> 2.52.0
>