Hi Laurent,
On 04/10/2021 23:58, Laurent Pinchart wrote:
> Hi Jean-Michel,
>
> Thank you for the patch.
>
> On Thu, Sep 30, 2021 at 11:20:21AM +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.
>
> I would have mentioned how the hardware (or maybe firmware) generates
> the statistics instead of how applications consume them, as it's the
> IPU3 dictating the format, but it doesn't matter too much.
>
>> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@xxxxxxxxxxxxxxxx>
>> ---
>> .../media/ipu3/include/uapi/intel-ipu3.h | 30 +++++++++++++++++--
>> 1 file changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
>> index 585f55981c86..fdda9d0a30af 100644
>> --- a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
>> +++ b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
>> @@ -61,6 +61,29 @@ struct ipu3_uapi_grid_config {
>> __u16 y_end;
>> } __packed;
>>
>> +/**
>> + * struct ipu3_uapi_awb_set_item - 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.
>
> Do we have more information about this field ? We can add it later if we
> don't.
Indeed, I have made tests on my side, and I am now even able to make
this field "visible" :-).
The sat_ratio is a 0-100% ratio coded as 0-255 values. It is controlled
by the rgbs_thr_* fields in the ipu3_uapi_awb_config_s structure.
>
>> + * @padding0: Unused byte for padding.
>> + * @padding1: Unused byte for padding.
>> + * @padding2: Unused byte for padding.
>> + */
>> +struct ipu3_uapi_awb_set_item {
>> + __u8 Gr_avg;
>> + __u8 R_avg;
>> + __u8 B_avg;
>> + __u8 Gb_avg;
>> + __u8 sat_ratio;
>> + __u8 padding0;
>> + __u8 padding1;
>> + __u8 padding2;
>> +} __attribute__((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.
>> @@ -73,8 +96,9 @@ struct ipu3_uapi_grid_config {
>> (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
>> IPU3_UAPI_AWB_MD_ITEM_SIZE)
>> #define IPU3_UAPI_AWB_MAX_BUFFER_SIZE \
>> - (IPU3_UAPI_AWB_MAX_SETS * \
>> - (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES))
>> + ((IPU3_UAPI_AWB_MAX_SETS * \
>> + (IPU3_UAPI_AWB_SET_SIZE + IPU3_UAPI_AWB_SPARE_FOR_BUBBLES)) / \
>> + sizeof(struct ipu3_uapi_awb_set_item))
>
> We'll really have to figure out what the bubbles are... Not in this
> patch though.
>
> Given that IPU3_UAPI_AWB_MD_ITEM_SIZE is equal to the size of one item,
> how about this ?
>
> diff --git a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
> index ee0e6d0e4b2c..a18e9228ed07 100644
> --- a/include/linux/intel-ipu3.h
> +++ b/include/linux/intel-ipu3.h
> @@ -65,11 +65,9 @@ struct ipu3_uapi_grid_config {
> */
> #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_SET_SIZE 160
> #define IPU3_UAPI_AWB_SPARE_FOR_BUBBLES \
> - (IPU3_UAPI_MAX_BUBBLE_SIZE * IPU3_UAPI_MAX_STRIPES * \
> - IPU3_UAPI_AWB_MD_ITEM_SIZE)
> + (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))
>
Yes, it is better :-), thanks :-).
>>
>> /**
>> * struct ipu3_uapi_awb_raw_buffer - AWB raw buffer
>> @@ -83,7 +107,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_set_item meta_data[IPU3_UAPI_AWB_MAX_BUFFER_SIZE]
>> __attribute__((aligned(32)));
>> } __packed;
>>
>
