All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout
@ 2021-08-31 18:51 Jean-Michel Hautbois
  2021-08-31 21:33 ` Laurent Pinchart
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Jean-Michel Hautbois @ 2021-08-31 18:51 UTC (permalink / raw)
  To: linux-media
  Cc: sakari.ailus, bingbu.cao, laurent.pinchart, Jean-Michel Hautbois

While parsing the RAW AWB metadata, the AWB layout was missing to fully
understand which byte corresponds to which feature. Make the field names
and usage explicit, as it is used by the userspace applications.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
---
This structure layout is defined in CrOs:
https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/awb_public.h

There are a few things not really understood right now:
- Is sat_ratio a full scale ratio (I can't get more than some values out
  of it, is it a ratio of 25%, 50%, 75%, 100% ?)
- What are the real minimum and maximum values for the grid size ? From
  CrOs it appears to be [16, 80] for width and [16, 60] for height while
  in this file it seems to be [16, 160] for width and not really defined
  for height AFAICT ?
- Same for the block_width_log2 and block_height_log2 which are [3, 7]
  in this file and [3, 6] in the awb_public.h header ?

 .../media/ipu3/include/uapi/intel-ipu3.h      | 38 ++++++++++++++-----
 1 file changed, 29 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
index fa3d6ee5adf2..83191aff2ddd 100644
--- a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
+++ b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
@@ -61,20 +61,40 @@ struct ipu3_uapi_grid_config {
 	__u16 y_end;
 } __packed;
 
+/**
+ * struct ipu3_uapi_awb_raw_buffer - Memory layout for each cell in AWB
+ *
+ * @Gr_avg:	Green average for red lines in the cell.
+ * @R_avg:	Red average in the cell.
+ * @B_avg:	Blue average in the cell.
+ * @Gb_avg:	Green average for blue lines in the cell.
+ * @sat_ratio:  Saturation ratio in the cell.
+ * @padding0:   Unused byte for padding.
+ * @padding1:   Unused byte for padding.
+ * @padding2:   Unused byte for padding.
+ */
+struct ipu3_uapi_awb_raw_buffer {
+    unsigned char Gr_avg;
+    unsigned char R_avg;
+    unsigned char B_avg;
+    unsigned char Gb_avg;
+    unsigned char sat_ratio;
+    unsigned char padding0;
+    unsigned char padding1;
+    unsigned char padding2;
+} __packed;
+
 /*
  * The grid based data is divided into "slices" called set, each slice of setX
  * refers to ipu3_uapi_grid_config width * height_per_slice.
  */
 #define IPU3_UAPI_AWB_MAX_SETS				60
-/* Based on grid size 80 * 60 and cell size 16 x 16 */
-#define IPU3_UAPI_AWB_SET_SIZE				1280
-#define IPU3_UAPI_AWB_MD_ITEM_SIZE			8
-#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \
-	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
-	 IPU3_UAPI_AWB_MD_ITEM_SIZE)
+#define AWB_PUBLIC_NUM_OF_ITEMS_IN_SET			160
+/* Based on max grid height + Spare for bubbles */
+#define AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER IPU3_UAPI_AWB_MAX_SETS + \
+	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES)
 #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
-	(IPU3_UAPI_AWB_MAX_SETS * \
-	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
+        AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER * AWB_PUBLIC_NUM_OF_ITEMS_IN_SET
 
 /**
  * struct ipu3_uapi_awb_raw_buffer - AWB raw buffer
@@ -83,7 +103,7 @@ struct ipu3_uapi_grid_config {
  *		the average values for each color channel.
  */
 struct ipu3_uapi_awb_raw_buffer {
-	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
+	struct ipu3_uapi_awb_raw_buffer meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
 		__attribute__((aligned(32)));
 } __packed;
 
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout
  2021-08-31 18:51 [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout Jean-Michel Hautbois
@ 2021-08-31 21:33 ` Laurent Pinchart
  2021-09-08  8:17   ` Laurent Pinchart
  2021-09-01  1:00   ` kernel test robot
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2021-08-31 21:33 UTC (permalink / raw)
  To: Jean-Michel Hautbois; +Cc: linux-media, sakari.ailus, bingbu.cao, Tian Shu Qiu

Hi Jean-Michel,

Just replying to CC Tian Shu as well.

On Tue, Aug 31, 2021 at 08:51:40PM +0200, Jean-Michel Hautbois wrote:
> While parsing the RAW AWB metadata, the AWB layout was missing to fully
> understand which byte corresponds to which feature. Make the field names
> and usage explicit, as it is used by the userspace applications.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
> This structure layout is defined in CrOs:
> https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/awb_public.h
> 
> There are a few things not really understood right now:
> - Is sat_ratio a full scale ratio (I can't get more than some values out
>   of it, is it a ratio of 25%, 50%, 75%, 100% ?)
> - What are the real minimum and maximum values for the grid size ? From
>   CrOs it appears to be [16, 80] for width and [16, 60] for height while
>   in this file it seems to be [16, 160] for width and not really defined
>   for height AFAICT ?
> - Same for the block_width_log2 and block_height_log2 which are [3, 7]
>   in this file and [3, 6] in the awb_public.h header ?
> 
>  .../media/ipu3/include/uapi/intel-ipu3.h      | 38 ++++++++++++++-----
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
> index fa3d6ee5adf2..83191aff2ddd 100644
> --- a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
> +++ b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
> @@ -61,20 +61,40 @@ struct ipu3_uapi_grid_config {
>  	__u16 y_end;
>  } __packed;
>  
> +/**
> + * struct ipu3_uapi_awb_raw_buffer - Memory layout for each cell in AWB
> + *
> + * @Gr_avg:	Green average for red lines in the cell.
> + * @R_avg:	Red average in the cell.
> + * @B_avg:	Blue average in the cell.
> + * @Gb_avg:	Green average for blue lines in the cell.
> + * @sat_ratio:  Saturation ratio in the cell.
> + * @padding0:   Unused byte for padding.
> + * @padding1:   Unused byte for padding.
> + * @padding2:   Unused byte for padding.
> + */
> +struct ipu3_uapi_awb_raw_buffer {
> +    unsigned char Gr_avg;
> +    unsigned char R_avg;
> +    unsigned char B_avg;
> +    unsigned char Gb_avg;
> +    unsigned char sat_ratio;
> +    unsigned char padding0;
> +    unsigned char padding1;
> +    unsigned char padding2;
> +} __packed;
> +
>  /*
>   * The grid based data is divided into "slices" called set, each slice of setX
>   * refers to ipu3_uapi_grid_config width * height_per_slice.
>   */
>  #define IPU3_UAPI_AWB_MAX_SETS				60
> -/* Based on grid size 80 * 60 and cell size 16 x 16 */
> -#define IPU3_UAPI_AWB_SET_SIZE				1280
> -#define IPU3_UAPI_AWB_MD_ITEM_SIZE			8
> -#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \
> -	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
> -	 IPU3_UAPI_AWB_MD_ITEM_SIZE)
> +#define AWB_PUBLIC_NUM_OF_ITEMS_IN_SET			160
> +/* Based on max grid height + Spare for bubbles */
> +#define AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER IPU3_UAPI_AWB_MAX_SETS + \
> +	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES)
>  #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
> -	(IPU3_UAPI_AWB_MAX_SETS * \
> -	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
> +        AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER * AWB_PUBLIC_NUM_OF_ITEMS_IN_SET
>  
>  /**
>   * struct ipu3_uapi_awb_raw_buffer - AWB raw buffer
> @@ -83,7 +103,7 @@ struct ipu3_uapi_grid_config {
>   *		the average values for each color channel.
>   */
>  struct ipu3_uapi_awb_raw_buffer {
> -	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> +	struct ipu3_uapi_awb_raw_buffer meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
>  		__attribute__((aligned(32)));
>  } __packed;
>  

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout
  2021-08-31 18:51 [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout Jean-Michel Hautbois
@ 2021-09-01  1:00   ` kernel test robot
  2021-09-01  1:00   ` kernel test robot
                     ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2021-09-01  1:00 UTC (permalink / raw)
  To: Jean-Michel Hautbois; +Cc: llvm, kbuild-all

[-- Attachment #1: Type: text/plain, Size: 6390 bytes --]

Hi Jean-Michel,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/Jean-Michel-Hautbois/media-staging-ipu3-imgu-add-the-AWB-memory-layout/20210901-025225
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 4adb389e08c95fdf91995271932c59250ff0d561
config: x86_64-randconfig-a013-20210831 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 4b1fde8a2b681dad2ce0c082a5d6422caa06b0bc)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/e9de209cdb5156e33f665630622ea9f4769937f3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jean-Michel-Hautbois/media-staging-ipu3-imgu-add-the-AWB-memory-layout/20210901-025225
        git checkout e9de209cdb5156e33f665630622ea9f4769937f3
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/staging/media/ipu3/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/staging/media/ipu3/ipu3-dmamap.c:12:
   In file included from drivers/staging/media/ipu3/ipu3.h:14:
   In file included from drivers/staging/media/ipu3/ipu3-css.h:10:
   In file included from drivers/staging/media/ipu3/ipu3-abi.h:7:
>> drivers/staging/media/ipu3/include/uapi/intel-ipu3.h:105:8: error: redefinition of 'ipu3_uapi_awb_raw_buffer'
   struct ipu3_uapi_awb_raw_buffer {
          ^
   drivers/staging/media/ipu3/include/uapi/intel-ipu3.h:76:8: note: previous definition is here
   struct ipu3_uapi_awb_raw_buffer {
          ^
   1 error generated.
--
   In file included from drivers/staging/media/ipu3/ipu3-css-params.c:6:
   In file included from drivers/staging/media/ipu3/ipu3-css.h:10:
   In file included from drivers/staging/media/ipu3/ipu3-abi.h:7:
>> drivers/staging/media/ipu3/include/uapi/intel-ipu3.h:105:8: error: redefinition of 'ipu3_uapi_awb_raw_buffer'
   struct ipu3_uapi_awb_raw_buffer {
          ^
   drivers/staging/media/ipu3/include/uapi/intel-ipu3.h:76:8: note: previous definition is here
   struct ipu3_uapi_awb_raw_buffer {
          ^
   drivers/staging/media/ipu3/ipu3-css-params.c:774:8: warning: variable 'pin_scale' set but not used [-Wunused-but-set-variable]
                           int pin_scale = 0;
                               ^
   1 warning and 1 error generated.


vim +/ipu3_uapi_awb_raw_buffer +105 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h

e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   86  
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   87  /*
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   88   * The grid based data is divided into "slices" called set, each slice of setX
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   89   * refers to ipu3_uapi_grid_config width * height_per_slice.
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   90   */
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   91  #define IPU3_UAPI_AWB_MAX_SETS				60
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   92  #define AWB_PUBLIC_NUM_OF_ITEMS_IN_SET			160
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   93  /* Based on max grid height + Spare for bubbles */
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   94  #define AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER IPU3_UAPI_AWB_MAX_SETS + \
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   95  	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES)
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   96  #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   97          AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER * AWB_PUBLIC_NUM_OF_ITEMS_IN_SET
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   98  
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   99  /**
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  100   * struct ipu3_uapi_awb_raw_buffer - AWB raw buffer
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  101   *
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  102   * @meta_data: buffer to hold auto white balance meta data which is
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  103   *		the average values for each color channel.
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  104   */
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06 @105  struct ipu3_uapi_awb_raw_buffer {
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31  106  	struct ipu3_uapi_awb_raw_buffer meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  107  		__attribute__((aligned(32)));
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  108  } __packed;
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  109  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 37750 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout
@ 2021-09-01  1:00   ` kernel test robot
  0 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2021-09-01  1:00 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 6474 bytes --]

Hi Jean-Michel,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/Jean-Michel-Hautbois/media-staging-ipu3-imgu-add-the-AWB-memory-layout/20210901-025225
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 4adb389e08c95fdf91995271932c59250ff0d561
config: x86_64-randconfig-a013-20210831 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 4b1fde8a2b681dad2ce0c082a5d6422caa06b0bc)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/e9de209cdb5156e33f665630622ea9f4769937f3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jean-Michel-Hautbois/media-staging-ipu3-imgu-add-the-AWB-memory-layout/20210901-025225
        git checkout e9de209cdb5156e33f665630622ea9f4769937f3
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/staging/media/ipu3/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/staging/media/ipu3/ipu3-dmamap.c:12:
   In file included from drivers/staging/media/ipu3/ipu3.h:14:
   In file included from drivers/staging/media/ipu3/ipu3-css.h:10:
   In file included from drivers/staging/media/ipu3/ipu3-abi.h:7:
>> drivers/staging/media/ipu3/include/uapi/intel-ipu3.h:105:8: error: redefinition of 'ipu3_uapi_awb_raw_buffer'
   struct ipu3_uapi_awb_raw_buffer {
          ^
   drivers/staging/media/ipu3/include/uapi/intel-ipu3.h:76:8: note: previous definition is here
   struct ipu3_uapi_awb_raw_buffer {
          ^
   1 error generated.
--
   In file included from drivers/staging/media/ipu3/ipu3-css-params.c:6:
   In file included from drivers/staging/media/ipu3/ipu3-css.h:10:
   In file included from drivers/staging/media/ipu3/ipu3-abi.h:7:
>> drivers/staging/media/ipu3/include/uapi/intel-ipu3.h:105:8: error: redefinition of 'ipu3_uapi_awb_raw_buffer'
   struct ipu3_uapi_awb_raw_buffer {
          ^
   drivers/staging/media/ipu3/include/uapi/intel-ipu3.h:76:8: note: previous definition is here
   struct ipu3_uapi_awb_raw_buffer {
          ^
   drivers/staging/media/ipu3/ipu3-css-params.c:774:8: warning: variable 'pin_scale' set but not used [-Wunused-but-set-variable]
                           int pin_scale = 0;
                               ^
   1 warning and 1 error generated.


vim +/ipu3_uapi_awb_raw_buffer +105 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h

e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   86  
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   87  /*
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   88   * The grid based data is divided into "slices" called set, each slice of setX
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   89   * refers to ipu3_uapi_grid_config width * height_per_slice.
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   90   */
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   91  #define IPU3_UAPI_AWB_MAX_SETS				60
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   92  #define AWB_PUBLIC_NUM_OF_ITEMS_IN_SET			160
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   93  /* Based on max grid height + Spare for bubbles */
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   94  #define AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER IPU3_UAPI_AWB_MAX_SETS + \
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   95  	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES)
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   96  #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   97          AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER * AWB_PUBLIC_NUM_OF_ITEMS_IN_SET
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   98  
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   99  /**
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  100   * struct ipu3_uapi_awb_raw_buffer - AWB raw buffer
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  101   *
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  102   * @meta_data: buffer to hold auto white balance meta data which is
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  103   *		the average values for each color channel.
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  104   */
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06 @105  struct ipu3_uapi_awb_raw_buffer {
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31  106  	struct ipu3_uapi_awb_raw_buffer meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  107  		__attribute__((aligned(32)));
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  108  } __packed;
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  109  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 37750 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout
  2021-08-31 18:51 [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout Jean-Michel Hautbois
  2021-08-31 21:33 ` Laurent Pinchart
  2021-09-01  1:00   ` kernel test robot
@ 2021-09-01  2:35 ` kernel test robot
  2021-09-21 11:29 ` Laurent Pinchart
  3 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2021-09-01  2:35 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 5713 bytes --]

Hi Jean-Michel,

[FYI, it's a private test report for your RFC patch.]
[auto build test ERROR on staging/staging-testing]

url:    https://github.com/0day-ci/linux/commits/Jean-Michel-Hautbois/media-staging-ipu3-imgu-add-the-AWB-memory-layout/20210901-025225
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git 4adb389e08c95fdf91995271932c59250ff0d561
config: x86_64-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/e9de209cdb5156e33f665630622ea9f4769937f3
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Jean-Michel-Hautbois/media-staging-ipu3-imgu-add-the-AWB-memory-layout/20210901-025225
        git checkout e9de209cdb5156e33f665630622ea9f4769937f3
        # save the attached .config to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from drivers/staging/media/ipu3/ipu3-abi.h:7,
                    from drivers/staging/media/ipu3/ipu3-css.h:10,
                    from drivers/staging/media/ipu3/ipu3.h:14,
                    from drivers/staging/media/ipu3/ipu3-dmamap.c:12:
>> drivers/staging/media/ipu3/include/uapi/intel-ipu3.h:105:8: error: redefinition of 'struct ipu3_uapi_awb_raw_buffer'
     105 | struct ipu3_uapi_awb_raw_buffer {
         |        ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/staging/media/ipu3/include/uapi/intel-ipu3.h:76:8: note: originally defined here
      76 | struct ipu3_uapi_awb_raw_buffer {
         |        ^~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/staging/media/ipu3/include/uapi/intel-ipu3.h:106:34: error: array type has incomplete element type 'struct ipu3_uapi_awb_raw_buffer'
     106 |  struct ipu3_uapi_awb_raw_buffer meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
         |                                  ^~~~~~~~~


vim +105 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h

e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   86  
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   87  /*
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   88   * The grid based data is divided into "slices" called set, each slice of setX
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   89   * refers to ipu3_uapi_grid_config width * height_per_slice.
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   90   */
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   91  #define IPU3_UAPI_AWB_MAX_SETS				60
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   92  #define AWB_PUBLIC_NUM_OF_ITEMS_IN_SET			160
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   93  /* Based on max grid height + Spare for bubbles */
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   94  #define AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER IPU3_UAPI_AWB_MAX_SETS + \
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   95  	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES)
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   96  #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31   97          AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER * AWB_PUBLIC_NUM_OF_ITEMS_IN_SET
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   98  
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06   99  /**
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  100   * struct ipu3_uapi_awb_raw_buffer - AWB raw buffer
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  101   *
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  102   * @meta_data: buffer to hold auto white balance meta data which is
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  103   *		the average values for each color channel.
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  104   */
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06 @105  struct ipu3_uapi_awb_raw_buffer {
e9de209cdb5156 drivers/staging/media/ipu3/include/uapi/intel-ipu3.h Jean-Michel Hautbois 2021-08-31 @106  	struct ipu3_uapi_awb_raw_buffer meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  107  		__attribute__((aligned(32)));
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  108  } __packed;
41158dabfd913c drivers/staging/media/ipu3/include/intel-ipu3.h      Yong Zhi             2018-12-06  109  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 65653 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout
  2021-08-31 21:33 ` Laurent Pinchart
@ 2021-09-08  8:17   ` Laurent Pinchart
  2021-09-09  2:19     ` Bingbu Cao
  0 siblings, 1 reply; 10+ messages in thread
From: Laurent Pinchart @ 2021-09-08  8:17 UTC (permalink / raw)
  To: bingbu.cao, Tian Shu Qiu, sakari.ailus
  Cc: linux-media, Jean-Michel Hautbois, Tomasz Figa

Hello,

(CC'ing Tomasz)

Gentle ping.

On Wed, Sep 01, 2021 at 12:34:00AM +0300, Laurent Pinchart wrote:
> On Tue, Aug 31, 2021 at 08:51:40PM +0200, Jean-Michel Hautbois wrote:
> > While parsing the RAW AWB metadata, the AWB layout was missing to fully
> > understand which byte corresponds to which feature. Make the field names
> > and usage explicit, as it is used by the userspace applications.
> > 
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > ---
> > This structure layout is defined in CrOs:
> > https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/awb_public.h
> > 
> > There are a few things not really understood right now:
> > - Is sat_ratio a full scale ratio (I can't get more than some values out
> >   of it, is it a ratio of 25%, 50%, 75%, 100% ?)
> > - What are the real minimum and maximum values for the grid size ? From
> >   CrOs it appears to be [16, 80] for width and [16, 60] for height while
> >   in this file it seems to be [16, 160] for width and not really defined
> >   for height AFAICT ?
> > - Same for the block_width_log2 and block_height_log2 which are [3, 7]
> >   in this file and [3, 6] in the awb_public.h header ?
> > 
> >  .../media/ipu3/include/uapi/intel-ipu3.h      | 38 ++++++++++++++-----
> >  1 file changed, 29 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
> > index fa3d6ee5adf2..83191aff2ddd 100644
> > --- a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
> > +++ b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
> > @@ -61,20 +61,40 @@ struct ipu3_uapi_grid_config {
> >  	__u16 y_end;
> >  } __packed;
> >  
> > +/**
> > + * struct ipu3_uapi_awb_raw_buffer - Memory layout for each cell in AWB
> > + *
> > + * @Gr_avg:	Green average for red lines in the cell.
> > + * @R_avg:	Red average in the cell.
> > + * @B_avg:	Blue average in the cell.
> > + * @Gb_avg:	Green average for blue lines in the cell.
> > + * @sat_ratio:  Saturation ratio in the cell.
> > + * @padding0:   Unused byte for padding.
> > + * @padding1:   Unused byte for padding.
> > + * @padding2:   Unused byte for padding.
> > + */
> > +struct ipu3_uapi_awb_raw_buffer {
> > +    unsigned char Gr_avg;
> > +    unsigned char R_avg;
> > +    unsigned char B_avg;
> > +    unsigned char Gb_avg;
> > +    unsigned char sat_ratio;
> > +    unsigned char padding0;
> > +    unsigned char padding1;
> > +    unsigned char padding2;
> > +} __packed;
> > +
> >  /*
> >   * The grid based data is divided into "slices" called set, each slice of setX
> >   * refers to ipu3_uapi_grid_config width * height_per_slice.
> >   */
> >  #define IPU3_UAPI_AWB_MAX_SETS				60
> > -/* Based on grid size 80 * 60 and cell size 16 x 16 */
> > -#define IPU3_UAPI_AWB_SET_SIZE				1280
> > -#define IPU3_UAPI_AWB_MD_ITEM_SIZE			8
> > -#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \
> > -	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
> > -	 IPU3_UAPI_AWB_MD_ITEM_SIZE)
> > +#define AWB_PUBLIC_NUM_OF_ITEMS_IN_SET			160
> > +/* Based on max grid height + Spare for bubbles */
> > +#define AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER IPU3_UAPI_AWB_MAX_SETS + \
> > +	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES)
> >  #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
> > -	(IPU3_UAPI_AWB_MAX_SETS * \
> > -	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
> > +        AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER * AWB_PUBLIC_NUM_OF_ITEMS_IN_SET
> >  
> >  /**
> >   * struct ipu3_uapi_awb_raw_buffer - AWB raw buffer
> > @@ -83,7 +103,7 @@ struct ipu3_uapi_grid_config {
> >   *		the average values for each color channel.
> >   */
> >  struct ipu3_uapi_awb_raw_buffer {
> > -	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> > +	struct ipu3_uapi_awb_raw_buffer meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> >  		__attribute__((aligned(32)));
> >  } __packed;
> >  

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout
  2021-09-08  8:17   ` Laurent Pinchart
@ 2021-09-09  2:19     ` Bingbu Cao
  2021-09-09  5:52       ` Jean-Michel Hautbois
  0 siblings, 1 reply; 10+ messages in thread
From: Bingbu Cao @ 2021-09-09  2:19 UTC (permalink / raw)
  To: Laurent Pinchart, bingbu.cao, Tian Shu Qiu, sakari.ailus
  Cc: linux-media, Jean-Michel Hautbois, Tomasz Figa

Jean-Michel,

Thanks for your patch.

On 9/8/21 4:17 PM, Laurent Pinchart wrote:
> Hello,
> 
> (CC'ing Tomasz)
> 
> Gentle ping.
> 
> On Wed, Sep 01, 2021 at 12:34:00AM +0300, Laurent Pinchart wrote:
>> On Tue, Aug 31, 2021 at 08:51:40PM +0200, Jean-Michel Hautbois wrote:
>>> While parsing the RAW AWB metadata, the AWB layout was missing to fully
>>> understand which byte corresponds to which feature. Make the field names
>>> and usage explicit, as it is used by the userspace applications.
>>>
>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>>> ---
>>> This structure layout is defined in CrOs:
>>> https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/awb_public.h
>>>
>>> There are a few things not really understood right now:
>>> - Is sat_ratio a full scale ratio (I can't get more than some values out
>>>   of it, is it a ratio of 25%, 50%, 75%, 100% ?)
>>> - What are the real minimum and maximum values for the grid size ? From
>>>   CrOs it appears to be [16, 80] for width and [16, 60] for height while
>>>   in this file it seems to be [16, 160] for width and not really defined
>>>   for height AFAICT ?
>>> - Same for the block_width_log2 and block_height_log2 which are [3, 7]
>>>   in this file and [3, 6] in the awb_public.h header ?
>>>
>>>  .../media/ipu3/include/uapi/intel-ipu3.h      | 38 ++++++++++++++-----
>>>  1 file changed, 29 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
>>> index fa3d6ee5adf2..83191aff2ddd 100644
>>> --- a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
>>> +++ b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
>>> @@ -61,20 +61,40 @@ struct ipu3_uapi_grid_config {
>>>  	__u16 y_end;
>>>  } __packed;
>>>  
>>> +/**
>>> + * struct ipu3_uapi_awb_raw_buffer - Memory layout for each cell in AWB
>>> + *
>>> + * @Gr_avg:	Green average for red lines in the cell.
>>> + * @R_avg:	Red average in the cell.
>>> + * @B_avg:	Blue average in the cell.
>>> + * @Gb_avg:	Green average for blue lines in the cell.
>>> + * @sat_ratio:  Saturation ratio in the cell.
>>> + * @padding0:   Unused byte for padding.
>>> + * @padding1:   Unused byte for padding.
>>> + * @padding2:   Unused byte for padding.
>>> + */
>>> +struct ipu3_uapi_awb_raw_buffer {
>>> +    unsigned char Gr_avg;
>>> +    unsigned char R_avg;
>>> +    unsigned char B_avg;
>>> +    unsigned char Gb_avg;
>>> +    unsigned char sat_ratio;
>>> +    unsigned char padding0;
>>> +    unsigned char padding1;
>>> +    unsigned char padding2;

It is fine for me to define and exposure the awb memory layout in uAPI.

nit: use __u8 here?

>>> +} __packed;
>>> +
>>>  /*
>>>   * The grid based data is divided into "slices" called set, each slice of setX
>>>   * refers to ipu3_uapi_grid_config width * height_per_slice.
>>>   */
>>>  #define IPU3_UAPI_AWB_MAX_SETS				60
>>> -/* Based on grid size 80 * 60 and cell size 16 x 16 */
>>> -#define IPU3_UAPI_AWB_SET_SIZE				1280
>>> -#define IPU3_UAPI_AWB_MD_ITEM_SIZE			8
>>> -#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \
>>> -	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
>>> -	 IPU3_UAPI_AWB_MD_ITEM_SIZE)
>>> +#define AWB_PUBLIC_NUM_OF_ITEMS_IN_SET			160
>>> +/* Based on max grid height + Spare for bubbles */
>>> +#define AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER IPU3_UAPI_AWB_MAX_SETS + \
>>> +	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES)
>>>  #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
>>> -	(IPU3_UAPI_AWB_MAX_SETS * \
>>> -	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
>>> +        AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER * AWB_PUBLIC_NUM_OF_ITEMS_IN_SET

It's better to update the name of 'IPU3_UAPI_AWB_MAX_BUFFER_SIZE' to align current
definition.

>>>  
>>>  /**
>>>   * struct ipu3_uapi_awb_raw_buffer - AWB raw buffer
>>> @@ -83,7 +103,7 @@ struct ipu3_uapi_grid_config {
>>>   *		the average values for each color channel.
>>>   */
>>>  struct ipu3_uapi_awb_raw_buffer {
>>> -	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
>>> +	struct ipu3_uapi_awb_raw_buffer meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
>>>  		__attribute__((aligned(32)));
>>>  } __packed;
>>>  
> 

-- 
Best regards,
Bingbu Cao

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout
  2021-09-09  2:19     ` Bingbu Cao
@ 2021-09-09  5:52       ` Jean-Michel Hautbois
  2021-09-21 11:33         ` Laurent Pinchart
  0 siblings, 1 reply; 10+ messages in thread
From: Jean-Michel Hautbois @ 2021-09-09  5:52 UTC (permalink / raw)
  To: Bingbu Cao, Laurent Pinchart, bingbu.cao, Tian Shu Qiu, sakari.ailus
  Cc: linux-media, Tomasz Figa

Hi Bingbu, thanks for your answer !

On 09/09/2021 04:19, Bingbu Cao wrote:
> Jean-Michel,
> 
> Thanks for your patch.
> 
> On 9/8/21 4:17 PM, Laurent Pinchart wrote:
>> Hello,
>>
>> (CC'ing Tomasz)
>>
>> Gentle ping.
>>
>> On Wed, Sep 01, 2021 at 12:34:00AM +0300, Laurent Pinchart wrote:
>>> On Tue, Aug 31, 2021 at 08:51:40PM +0200, Jean-Michel Hautbois wrote:
>>>> While parsing the RAW AWB metadata, the AWB layout was missing to fully
>>>> understand which byte corresponds to which feature. Make the field names
>>>> and usage explicit, as it is used by the userspace applications.
>>>>
>>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>>>> ---
>>>> This structure layout is defined in CrOs:
>>>> https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/awb_public.h
>>>>
>>>> There are a few things not really understood right now:
>>>> - Is sat_ratio a full scale ratio (I can't get more than some values out
>>>>   of it, is it a ratio of 25%, 50%, 75%, 100% ?)
>>>> - What are the real minimum and maximum values for the grid size ? From
>>>>   CrOs it appears to be [16, 80] for width and [16, 60] for height while
>>>>   in this file it seems to be [16, 160] for width and not really defined
>>>>   for height AFAICT ?
>>>> - Same for the block_width_log2 and block_height_log2 which are [3, 7]
>>>>   in this file and [3, 6] in the awb_public.h header ?

Do you have any clue for those questions please :-) ?

>>>>
>>>>  .../media/ipu3/include/uapi/intel-ipu3.h      | 38 ++++++++++++++-----
>>>>  1 file changed, 29 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
>>>> index fa3d6ee5adf2..83191aff2ddd 100644
>>>> --- a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
>>>> +++ b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
>>>> @@ -61,20 +61,40 @@ struct ipu3_uapi_grid_config {
>>>>  	__u16 y_end;
>>>>  } __packed;
>>>>  
>>>> +/**
>>>> + * struct ipu3_uapi_awb_raw_buffer - Memory layout for each cell in AWB
>>>> + *
>>>> + * @Gr_avg:	Green average for red lines in the cell.
>>>> + * @R_avg:	Red average in the cell.
>>>> + * @B_avg:	Blue average in the cell.
>>>> + * @Gb_avg:	Green average for blue lines in the cell.
>>>> + * @sat_ratio:  Saturation ratio in the cell.
>>>> + * @padding0:   Unused byte for padding.
>>>> + * @padding1:   Unused byte for padding.
>>>> + * @padding2:   Unused byte for padding.
>>>> + */
>>>> +struct ipu3_uapi_awb_raw_buffer {
>>>> +    unsigned char Gr_avg;
>>>> +    unsigned char R_avg;
>>>> +    unsigned char B_avg;
>>>> +    unsigned char Gb_avg;
>>>> +    unsigned char sat_ratio;
>>>> +    unsigned char padding0;
>>>> +    unsigned char padding1;
>>>> +    unsigned char padding2;
> 
> It is fine for me to define and exposure the awb memory layout in uAPI.
> 
> nit: use __u8 here?

Sure !

> 
>>>> +} __packed;
>>>> +
>>>>  /*
>>>>   * The grid based data is divided into "slices" called set, each slice of setX
>>>>   * refers to ipu3_uapi_grid_config width * height_per_slice.
>>>>   */
>>>>  #define IPU3_UAPI_AWB_MAX_SETS				60
>>>> -/* Based on grid size 80 * 60 and cell size 16 x 16 */
>>>> -#define IPU3_UAPI_AWB_SET_SIZE				1280
>>>> -#define IPU3_UAPI_AWB_MD_ITEM_SIZE			8
>>>> -#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \
>>>> -	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
>>>> -	 IPU3_UAPI_AWB_MD_ITEM_SIZE)
>>>> +#define AWB_PUBLIC_NUM_OF_ITEMS_IN_SET			160
>>>> +/* Based on max grid height + Spare for bubbles */
>>>> +#define AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER IPU3_UAPI_AWB_MAX_SETS + \
>>>> +	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES)
>>>>  #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
>>>> -	(IPU3_UAPI_AWB_MAX_SETS * \
>>>> -	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
>>>> +        AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER * AWB_PUBLIC_NUM_OF_ITEMS_IN_SET
> 
> It's better to update the name of 'IPU3_UAPI_AWB_MAX_BUFFER_SIZE' to align current
> definition.
> 
>>>>  
>>>>  /**
>>>>   * struct ipu3_uapi_awb_raw_buffer - AWB raw buffer
>>>> @@ -83,7 +103,7 @@ struct ipu3_uapi_grid_config {
>>>>   *		the average values for each color channel.
>>>>   */
>>>>  struct ipu3_uapi_awb_raw_buffer {
>>>> -	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
>>>> +	struct ipu3_uapi_awb_raw_buffer meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
>>>>  		__attribute__((aligned(32)));
>>>>  } __packed;
>>>>  
>>
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout
  2021-08-31 18:51 [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout Jean-Michel Hautbois
                   ` (2 preceding siblings ...)
  2021-09-01  2:35 ` kernel test robot
@ 2021-09-21 11:29 ` Laurent Pinchart
  3 siblings, 0 replies; 10+ messages in thread
From: Laurent Pinchart @ 2021-09-21 11:29 UTC (permalink / raw)
  To: Jean-Michel Hautbois; +Cc: linux-media, sakari.ailus, bingbu.cao

Hi Jean-Michel,

Another comment.

On Tue, Aug 31, 2021 at 08:51:40PM +0200, Jean-Michel Hautbois wrote:
> While parsing the RAW AWB metadata, the AWB layout was missing to fully
> understand which byte corresponds to which feature. Make the field names
> and usage explicit, as it is used by the userspace applications.
> 
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> ---
> This structure layout is defined in CrOs:
> https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/awb_public.h
> 
> There are a few things not really understood right now:
> - Is sat_ratio a full scale ratio (I can't get more than some values out
>   of it, is it a ratio of 25%, 50%, 75%, 100% ?)
> - What are the real minimum and maximum values for the grid size ? From
>   CrOs it appears to be [16, 80] for width and [16, 60] for height while
>   in this file it seems to be [16, 160] for width and not really defined
>   for height AFAICT ?
> - Same for the block_width_log2 and block_height_log2 which are [3, 7]
>   in this file and [3, 6] in the awb_public.h header ?
> 
>  .../media/ipu3/include/uapi/intel-ipu3.h      | 38 ++++++++++++++-----
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
> index fa3d6ee5adf2..83191aff2ddd 100644
> --- a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
> +++ b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
> @@ -61,20 +61,40 @@ struct ipu3_uapi_grid_config {
>  	__u16 y_end;
>  } __packed;
>  
> +/**
> + * struct ipu3_uapi_awb_raw_buffer - Memory layout for each cell in AWB
> + *
> + * @Gr_avg:	Green average for red lines in the cell.
> + * @R_avg:	Red average in the cell.
> + * @B_avg:	Blue average in the cell.
> + * @Gb_avg:	Green average for blue lines in the cell.
> + * @sat_ratio:  Saturation ratio in the cell.
> + * @padding0:   Unused byte for padding.
> + * @padding1:   Unused byte for padding.
> + * @padding2:   Unused byte for padding.
> + */
> +struct ipu3_uapi_awb_raw_buffer {
> +    unsigned char Gr_avg;
> +    unsigned char R_avg;
> +    unsigned char B_avg;
> +    unsigned char Gb_avg;
> +    unsigned char sat_ratio;
> +    unsigned char padding0;
> +    unsigned char padding1;
> +    unsigned char padding2;
> +} __packed;
> +
>  /*
>   * The grid based data is divided into "slices" called set, each slice of setX
>   * refers to ipu3_uapi_grid_config width * height_per_slice.
>   */
>  #define IPU3_UAPI_AWB_MAX_SETS				60
> -/* Based on grid size 80 * 60 and cell size 16 x 16 */
> -#define IPU3_UAPI_AWB_SET_SIZE				1280
> -#define IPU3_UAPI_AWB_MD_ITEM_SIZE			8
> -#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \
> -	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
> -	 IPU3_UAPI_AWB_MD_ITEM_SIZE)
> +#define AWB_PUBLIC_NUM_OF_ITEMS_IN_SET			160
> +/* Based on max grid height + Spare for bubbles */
> +#define AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER IPU3_UAPI_AWB_MAX_SETS + \
> +	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES)

You're missing parentheses, this won't work as expected. Look at the
usage below. The size is likely computed incorrectly.

Have you tested this ?

>  #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
> -	(IPU3_UAPI_AWB_MAX_SETS * \
> -	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
> +        AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER * AWB_PUBLIC_NUM_OF_ITEMS_IN_SET
>  
>  /**
>   * struct ipu3_uapi_awb_raw_buffer - AWB raw buffer
> @@ -83,7 +103,7 @@ struct ipu3_uapi_grid_config {
>   *		the average values for each color channel.
>   */
>  struct ipu3_uapi_awb_raw_buffer {
> -	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> +	struct ipu3_uapi_awb_raw_buffer meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
>  		__attribute__((aligned(32)));
>  } __packed;
>  

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout
  2021-09-09  5:52       ` Jean-Michel Hautbois
@ 2021-09-21 11:33         ` Laurent Pinchart
  0 siblings, 0 replies; 10+ messages in thread
From: Laurent Pinchart @ 2021-09-21 11:33 UTC (permalink / raw)
  To: Bingbu Cao, bingbu.cao
  Cc: Jean-Michel Hautbois, Tian Shu Qiu, sakari.ailus, linux-media,
	Tomasz Figa

Hello,

On Thu, Sep 09, 2021 at 07:52:46AM +0200, Jean-Michel Hautbois wrote:
> On 09/09/2021 04:19, Bingbu Cao wrote:
> > On 9/8/21 4:17 PM, Laurent Pinchart wrote:
> >> On Wed, Sep 01, 2021 at 12:34:00AM +0300, Laurent Pinchart wrote:
> >>> On Tue, Aug 31, 2021 at 08:51:40PM +0200, Jean-Michel Hautbois wrote:
> >>>> While parsing the RAW AWB metadata, the AWB layout was missing to fully
> >>>> understand which byte corresponds to which feature. Make the field names
> >>>> and usage explicit, as it is used by the userspace applications.
> >>>>
> >>>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> >>>> ---
> >>>> This structure layout is defined in CrOs:
> >>>> https://chromium.googlesource.com/chromiumos/platform/arc-camera/+/refs/heads/master/hal/intel/include/ia_imaging/awb_public.h
> >>>>
> >>>> There are a few things not really understood right now:
> >>>> - Is sat_ratio a full scale ratio (I can't get more than some values out
> >>>>   of it, is it a ratio of 25%, 50%, 75%, 100% ?)
> >>>> - What are the real minimum and maximum values for the grid size ? From
> >>>>   CrOs it appears to be [16, 80] for width and [16, 60] for height while
> >>>>   in this file it seems to be [16, 160] for width and not really defined
> >>>>   for height AFAICT ?
> >>>> - Same for the block_width_log2 and block_height_log2 which are [3, 7]
> >>>>   in this file and [3, 6] in the awb_public.h header ?
> 
> Do you have any clue for those questions please :-) ?

This is becoming a blocker, it would be really nice if we could have
answers to these questions. The grid size constraints are the most
immediate priority, but understanding the sat_ratio value will be next
veyr shortly.

> >>>>
> >>>>  .../media/ipu3/include/uapi/intel-ipu3.h      | 38 ++++++++++++++-----
> >>>>  1 file changed, 29 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
> >>>> index fa3d6ee5adf2..83191aff2ddd 100644
> >>>> --- a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
> >>>> +++ b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
> >>>> @@ -61,20 +61,40 @@ struct ipu3_uapi_grid_config {
> >>>>  	__u16 y_end;
> >>>>  } __packed;
> >>>>  
> >>>> +/**
> >>>> + * struct ipu3_uapi_awb_raw_buffer - Memory layout for each cell in AWB
> >>>> + *
> >>>> + * @Gr_avg:	Green average for red lines in the cell.
> >>>> + * @R_avg:	Red average in the cell.
> >>>> + * @B_avg:	Blue average in the cell.
> >>>> + * @Gb_avg:	Green average for blue lines in the cell.
> >>>> + * @sat_ratio:  Saturation ratio in the cell.
> >>>> + * @padding0:   Unused byte for padding.
> >>>> + * @padding1:   Unused byte for padding.
> >>>> + * @padding2:   Unused byte for padding.
> >>>> + */
> >>>> +struct ipu3_uapi_awb_raw_buffer {
> >>>> +    unsigned char Gr_avg;
> >>>> +    unsigned char R_avg;
> >>>> +    unsigned char B_avg;
> >>>> +    unsigned char Gb_avg;
> >>>> +    unsigned char sat_ratio;
> >>>> +    unsigned char padding0;
> >>>> +    unsigned char padding1;
> >>>> +    unsigned char padding2;
> > 
> > It is fine for me to define and exposure the awb memory layout in uAPI.
> > 
> > nit: use __u8 here?
> 
> Sure !
> 
> >>>> +} __packed;
> >>>> +
> >>>>  /*
> >>>>   * The grid based data is divided into "slices" called set, each slice of setX
> >>>>   * refers to ipu3_uapi_grid_config width * height_per_slice.
> >>>>   */
> >>>>  #define IPU3_UAPI_AWB_MAX_SETS				60
> >>>> -/* Based on grid size 80 * 60 and cell size 16 x 16 */
> >>>> -#define IPU3_UAPI_AWB_SET_SIZE				1280
> >>>> -#define IPU3_UAPI_AWB_MD_ITEM_SIZE			8
> >>>> -#define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \
> >>>> -	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
> >>>> -	 IPU3_UAPI_AWB_MD_ITEM_SIZE)
> >>>> +#define AWB_PUBLIC_NUM_OF_ITEMS_IN_SET			160
> >>>> +/* Based on max grid height + Spare for bubbles */
> >>>> +#define AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER IPU3_UAPI_AWB_MAX_SETS + \
> >>>> +	(IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES)
> >>>>  #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
> >>>> -	(IPU3_UAPI_AWB_MAX_SETS * \
> >>>> -	 (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
> >>>> +        AWB_PUBLIC_NUM_OF_SETS_IN_BUFFER * AWB_PUBLIC_NUM_OF_ITEMS_IN_SET
> > 
> > It's better to update the name of 'IPU3_UAPI_AWB_MAX_BUFFER_SIZE' to align current
> > definition.
> > 
> >>>>  
> >>>>  /**
> >>>>   * struct ipu3_uapi_awb_raw_buffer - AWB raw buffer
> >>>> @@ -83,7 +103,7 @@ struct ipu3_uapi_grid_config {
> >>>>   *		the average values for each color channel.
> >>>>   */
> >>>>  struct ipu3_uapi_awb_raw_buffer {
> >>>> -	__u8 meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> >>>> +	struct ipu3_uapi_awb_raw_buffer meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
> >>>>  		__attribute__((aligned(32)));
> >>>>  } __packed;
> >>>>  

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-09-21 11:34 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-31 18:51 [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout Jean-Michel Hautbois
2021-08-31 21:33 ` Laurent Pinchart
2021-09-08  8:17   ` Laurent Pinchart
2021-09-09  2:19     ` Bingbu Cao
2021-09-09  5:52       ` Jean-Michel Hautbois
2021-09-21 11:33         ` Laurent Pinchart
2021-09-01  1:00 ` kernel test robot
2021-09-01  1:00   ` kernel test robot
2021-09-01  2:35 ` kernel test robot
2021-09-21 11:29 ` Laurent Pinchart

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.