[v2,23/37] libcamera: utils: Move ALIGN_TO from camera_metadata.c to utils.h
diff mbox series

Message ID 20250824-b4-v0-5-2-gpuisp-v2-a-v2-23-96f4576c814e@linaro.org
State Superseded
Headers show
Series
  • Add GLES 2.0 GPUISP to libcamera
Related show

Commit Message

Bryan O'Donoghue Aug. 24, 2025, 12:48 a.m. UTC
ALIGN_TO() is useful to the GPU debayer logic, move to a shared header to
facilitate its reuse.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 include/libcamera/base/utils.h         | 3 +++
 src/android/metadata/camera_metadata.c | 4 +---
 2 files changed, 4 insertions(+), 3 deletions(-)

Comments

Milan Zamazal Aug. 26, 2025, 12:25 p.m. UTC | #1
Bryan O'Donoghue <bryan.odonoghue@linaro.org> writes:

> ALIGN_TO() is useful to the GPU debayer logic, move to a shared header to
> facilitate its reuse.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  include/libcamera/base/utils.h         | 3 +++
>  src/android/metadata/camera_metadata.c | 4 +---
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> index f21c6dc016ece00e9842ad17ae4dd036fb5683c2..a0d3ffc27804d27c6b225d7da8c74feb3c261f3d 100644
> --- a/include/libcamera/base/utils.h
> +++ b/include/libcamera/base/utils.h
> @@ -33,6 +33,9 @@
>  
>  #endif
>  
> +#define ALIGN_TO(val, alignment) \
> +    (((uintptr_t)(val) + ((alignment) - 1)) & ~((alignment) - 1))

Is uintptr_t the best type in the extended use?

Since we use the &~(alignment-1) pattern at more places in software ISP,
would it be useful to introduce something like ALIGN_UP and ALIGN_DOWN?

> +
>  namespace libcamera {
>  
>  namespace utils {
> diff --git a/src/android/metadata/camera_metadata.c b/src/android/metadata/camera_metadata.c
> index b86586a7e6857a0920234ea312eb5181b27c3ac0..4fafb54076a4a4be41a31f5ce3bd8c74bf649f59 100644
> --- a/src/android/metadata/camera_metadata.c
> +++ b/src/android/metadata/camera_metadata.c
> @@ -24,6 +24,7 @@
>   */
>  #define ALOGE(...) fprintf(stderr, LOG_TAG __VA_ARGS__)
>  
> +#include <libcamera/base/utils.h>
>  #include <system/camera_metadata.h>
>  #include <camera_metadata_hidden.h>
>  
> @@ -39,9 +40,6 @@
>  #define NOT_FOUND       (-ENOENT)
>  #define SN_EVENT_LOG_ID 0x534e4554
>  
> -#define ALIGN_TO(val, alignment) \
> -    (((uintptr_t)(val) + ((alignment) - 1)) & ~((alignment) - 1))
> -
>  /**
>   * A single metadata entry, storing an array of values of a given type. If the
>   * array is no larger than 4 bytes in size, it is stored in the data.value[]
Barnabás Pőcze Sept. 25, 2025, 3:16 p.m. UTC | #2
Hi

2025. 08. 24. 2:48 keltezéssel, Bryan O'Donoghue írta:
> ALIGN_TO() is useful to the GPU debayer logic, move to a shared header to
> facilitate its reuse.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>   include/libcamera/base/utils.h         | 3 +++
>   src/android/metadata/camera_metadata.c | 4 +---
>   2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
> index f21c6dc016ece00e9842ad17ae4dd036fb5683c2..a0d3ffc27804d27c6b225d7da8c74feb3c261f3d 100644
> --- a/include/libcamera/base/utils.h
> +++ b/include/libcamera/base/utils.h
> @@ -33,6 +33,9 @@
>   
>   #endif
>   
> +#define ALIGN_TO(val, alignment) \
> +    (((uintptr_t)(val) + ((alignment) - 1)) & ~((alignment) - 1))
> +
>   namespace libcamera {
>   
>   namespace utils {
> diff --git a/src/android/metadata/camera_metadata.c b/src/android/metadata/camera_metadata.c
> index b86586a7e6857a0920234ea312eb5181b27c3ac0..4fafb54076a4a4be41a31f5ce3bd8c74bf649f59 100644
> --- a/src/android/metadata/camera_metadata.c
> +++ b/src/android/metadata/camera_metadata.c

This file is imported from android. I think it would be best to avoid
modifications, even at the cost of duplicating the same thing.

There is already `alignUp` and `alignDown` in `utils.h`. If `unsigned int` is not
sufficient, I would just make them templates.


Regards,
Barnabás Pőcze


> @@ -24,6 +24,7 @@
>    */
>   #define ALOGE(...) fprintf(stderr, LOG_TAG __VA_ARGS__)
>   
> +#include <libcamera/base/utils.h>
>   #include <system/camera_metadata.h>
>   #include <camera_metadata_hidden.h>
>   
> @@ -39,9 +40,6 @@
>   #define NOT_FOUND       (-ENOENT)
>   #define SN_EVENT_LOG_ID 0x534e4554
>   
> -#define ALIGN_TO(val, alignment) \
> -    (((uintptr_t)(val) + ((alignment) - 1)) & ~((alignment) - 1))
> -
>   /**
>    * A single metadata entry, storing an array of values of a given type. If the
>    * array is no larger than 4 bytes in size, it is stored in the data.value[]
>
Bryan O'Donoghue Oct. 6, 2025, 9:03 p.m. UTC | #3
On 25/09/2025 16:16, Barnabás Pőcze wrote:
> Hi
> 
> 2025. 08. 24. 2:48 keltezéssel, Bryan O'Donoghue írta:
>> ALIGN_TO() is useful to the GPU debayer logic, move to a shared header to
>> facilitate its reuse.
>>
>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
>> ---
>>    include/libcamera/base/utils.h         | 3 +++
>>    src/android/metadata/camera_metadata.c | 4 +---
>>    2 files changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
>> index f21c6dc016ece00e9842ad17ae4dd036fb5683c2..a0d3ffc27804d27c6b225d7da8c74feb3c261f3d 100644
>> --- a/include/libcamera/base/utils.h
>> +++ b/include/libcamera/base/utils.h
>> @@ -33,6 +33,9 @@
>>
>>    #endif
>>
>> +#define ALIGN_TO(val, alignment) \
>> +    (((uintptr_t)(val) + ((alignment) - 1)) & ~((alignment) - 1))
>> +
>>    namespace libcamera {
>>
>>    namespace utils {
>> diff --git a/src/android/metadata/camera_metadata.c b/src/android/metadata/camera_metadata.c
>> index b86586a7e6857a0920234ea312eb5181b27c3ac0..4fafb54076a4a4be41a31f5ce3bd8c74bf649f59 100644
>> --- a/src/android/metadata/camera_metadata.c
>> +++ b/src/android/metadata/camera_metadata.c
> 
> This file is imported from android. I think it would be best to avoid
> modifications, even at the cost of duplicating the same thing.
> 
> There is already `alignUp` and `alignDown` in `utils.h`. If `unsigned int` is not
> sufficient, I would just make them templates.
> 
> 
> Regards,
> Barnabás Pőcze
> 
> 
>> @@ -24,6 +24,7 @@
>>     */
>>    #define ALOGE(...) fprintf(stderr, LOG_TAG __VA_ARGS__)
>>
>> +#include <libcamera/base/utils.h>
>>    #include <system/camera_metadata.h>
>>    #include <camera_metadata_hidden.h>
>>
>> @@ -39,9 +40,6 @@
>>    #define NOT_FOUND       (-ENOENT)
>>    #define SN_EVENT_LOG_ID 0x534e4554
>>
>> -#define ALIGN_TO(val, alignment) \
>> -    (((uintptr_t)(val) + ((alignment) - 1)) & ~((alignment) - 1))
>> -
>>    /**
>>     * A single metadata entry, storing an array of values of a given type. If the
>>     * array is no larger than 4 bytes in size, it is stored in the data.value[]
>>
> 

Dropped this patch in favour of your suggested change.

---
bod

Patch
diff mbox series

diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h
index f21c6dc016ece00e9842ad17ae4dd036fb5683c2..a0d3ffc27804d27c6b225d7da8c74feb3c261f3d 100644
--- a/include/libcamera/base/utils.h
+++ b/include/libcamera/base/utils.h
@@ -33,6 +33,9 @@ 
 
 #endif
 
+#define ALIGN_TO(val, alignment) \
+    (((uintptr_t)(val) + ((alignment) - 1)) & ~((alignment) - 1))
+
 namespace libcamera {
 
 namespace utils {
diff --git a/src/android/metadata/camera_metadata.c b/src/android/metadata/camera_metadata.c
index b86586a7e6857a0920234ea312eb5181b27c3ac0..4fafb54076a4a4be41a31f5ce3bd8c74bf649f59 100644
--- a/src/android/metadata/camera_metadata.c
+++ b/src/android/metadata/camera_metadata.c
@@ -24,6 +24,7 @@ 
  */
 #define ALOGE(...) fprintf(stderr, LOG_TAG __VA_ARGS__)
 
+#include <libcamera/base/utils.h>
 #include <system/camera_metadata.h>
 #include <camera_metadata_hidden.h>
 
@@ -39,9 +40,6 @@ 
 #define NOT_FOUND       (-ENOENT)
 #define SN_EVENT_LOG_ID 0x534e4554
 
-#define ALIGN_TO(val, alignment) \
-    (((uintptr_t)(val) + ((alignment) - 1)) & ~((alignment) - 1))
-
 /**
  * A single metadata entry, storing an array of values of a given type. If the
  * array is no larger than 4 bytes in size, it is stored in the data.value[]