* [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
2021-09-21 11:29 ` Laurent Pinchart
0 siblings, 2 replies; 7+ 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] 7+ 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-21 11:29 ` Laurent Pinchart
1 sibling, 1 reply; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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; 7+ 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] 7+ 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-21 11:29 ` Laurent Pinchart
1 sibling, 0 replies; 7+ 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] 7+ 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; 7+ 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] 7+ messages in thread
end of thread, other threads:[~2021-09-21 11:34 UTC | newest]
Thread overview: 7+ 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-21 11:29 ` Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).