[libcamera-devel] [PATCH 03/20] libcamera: utils: Add alignUp and alignDown functions

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Jul 14, 2020 at 12:41:55PM +0200, Jacopo Mondi wrote:
> Add to libcamera utils library two functions to round up or down a
> value to an alignment and add a test in test/utils.cpp for the two
> new functions.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/internal/utils.h |  3 +++
>  src/libcamera/utils.cpp            | 22 ++++++++++++++++++++++
>  test/utils.cpp                     | 18 ++++++++++++++++++
>  3 files changed, 43 insertions(+)
> 
> diff --git a/include/libcamera/internal/utils.h b/include/libcamera/internal/utils.h
> index 8d026cc6c0fe..25eb24ec2d16 100644
> --- a/include/libcamera/internal/utils.h
> +++ b/include/libcamera/internal/utils.h
> @@ -200,6 +200,9 @@ details::StringSplitter split(const std::string &str, const std::string &delim);
>  std::string libcameraBuildPath();
>  std::string libcameraSourcePath();
>  
> +unsigned int alignUp(unsigned int value, unsigned int alignment);
> +unsigned int alignDown(unsigned int value, unsigned int alignment);

I would inline those two functions, to let the compiler optimize the
code. For instance it's common to have a constexpr alignment value,
which can often translate to more efficient code. I would also mark the
functions as constexpr. As constexpr implies inline,

constexpr unsigned int alignUp(unsigned int value, unsigned int alignment)
{
	return (value + alignment - 1) / alignment * alignment;
}

constexpr unsigned int alignDown(unsigned int value, unsigned int alignment)
{
	return value / alignment * alignment;
}

> +
>  } /* namespace utils */
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/utils.cpp b/src/libcamera/utils.cpp
> index 0567328fe31b..52fb0bb171d8 100644
> --- a/src/libcamera/utils.cpp
> +++ b/src/libcamera/utils.cpp
> @@ -444,6 +444,28 @@ std::string libcameraSourcePath()
>  	return path + "/";
>  }
>  
> +/**
> + * \brief Align \a value to \a alignment

Maybe s/value to/value up to/ ? Same for alignDown().

> + * \param[in] value The value to align
> + * \param[in] alignment The alignment
> + * \return The value rounded up to the nearest multiple of \a alignment
> + */
> +unsigned int alignUp(unsigned int value, unsigned int alignment)
> +{
> +	return (value + alignment - 1) / alignment * alignment;
> +}
> +
> +/**
> + * \brief Align \a value to \a alignment
> + * \param[in] value The value to align
> + * \param[in] alignment The alignment
> + * \return The value rounded down to the nearest multiple of \a alignment
> + */
> +unsigned int alignDown(unsigned int value, unsigned int alignment)
> +{
> +	return value / alignment * alignment;
> +}
> +
>  } /* namespace utils */
>  
>  } /* namespace libcamera */
> diff --git a/test/utils.cpp b/test/utils.cpp
> index f482e6a1d829..02054f46b00e 100644
> --- a/test/utils.cpp
> +++ b/test/utils.cpp
> @@ -7,6 +7,7 @@
>  
>  #include <iostream>
>  #include <map>
> +#include <random>
>  #include <sstream>
>  #include <string>
>  #include <vector>
> @@ -166,6 +167,23 @@ protected:
>  			return TestFail;
>  		}
>  
> +		/* utils::alignUp() and utils::alignDown() tests. */
> +		random_device random;
> +		unsigned int val = random();

Usage of random number in tests makes it difficult to reproduce
failures. Additionally, the random alignment, could be 0, leading to a
potential division by 0. I think you can just hardcode values :

		if (utils::alignUp(6, 3) != 6 || utils::alignUp(7, 3) != 9) {
			cerr << "utils::alignUp test failed" << endl;
			return TestFail;
		}

		if (utils::alignDown(6, 3) != 6 || utils::alignDown(7, 3) != 6) {
			cerr << "utils::alignDown test failed" << endl;
			return TestFail;
		}

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +		unsigned int align = random() % (val / 10);
> +		unsigned int roundUp = (val + align - 1) / align;
> +		unsigned int roundDown = val / align;
> +
> +		if (utils::alignUp(val, align) != align * roundUp) {
> +			cerr << "utils::alignUp test failed" << endl;
> +			return TestFail;
> +		}
> +
> +		if (utils::alignDown(val, align) != align * roundDown) {
> +			cerr << "utils::alignDown test failed" << endl;
> +			return TestFail;
> +		}
> +
>  		return TestPass;
>  	}
>  };

-- 
Regards,

Laurent Pinchart