Message ID | 20210831185140.77400-1-jeanmichel.hautbois@ideasonboard.com (mailing list archive) |
---|---|
State | Accepted, archived |
Delegated to: | Sakari Ailus |
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1mL8rT-007aUk-5G; Tue, 31 Aug 2021 18:51:59 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239210AbhHaSws (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Tue, 31 Aug 2021 14:52:48 -0400 Received: from perceval.ideasonboard.com ([213.167.242.64]:57974 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229946AbhHaSws (ORCPT <rfc822;linux-media@vger.kernel.org>); Tue, 31 Aug 2021 14:52:48 -0400 Received: from tatooine.ideasonboard.com (unknown [IPv6:2a01:e0a:169:7140:b904:a32:6762:6e37]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 1921524F; Tue, 31 Aug 2021 20:51:51 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1630435911; bh=MTpHPcv7BpJs9Hw6l+l851YdSllFKlcRtf1S4bEHtvI=; h=From:To:Cc:Subject:Date:From; b=VXGn2YtNt+lyM/TLI2L4dj8KQ9JRckzfqnWmbeWRTnJIbOutLhjQRNyIheh29zCAf UZIkQEzicP9hio4RuorPRQaRZ5hAbhNmDj0y763pGOiBDWgApYxzRmBT8UHi30Qycl D1eWVXAw3atoA43o47erWpVzxV5ojHoH4hd075s0= From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> To: linux-media@vger.kernel.org Cc: sakari.ailus@linux.intel.com, bingbu.cao@intel.com, laurent.pinchart@ideasonboard.com, Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> Subject: [RFC PATCH] media: staging: ipu3-imgu: add the AWB memory layout Date: Tue, 31 Aug 2021 20:51:40 +0200 Message-Id: <20210831185140.77400-1-jeanmichel.hautbois@ideasonboard.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -4.8 (----) X-LSpam-Report: No, score=-4.8 required=5.0 tests=BAYES_00=-1.9,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_MED=-2.3 autolearn=ham autolearn_force=no |
Series |
[RFC] media: staging: ipu3-imgu: add the AWB memory layout
|
|
Commit Message
Jean-Michel Hautbois
Aug. 31, 2021, 6:51 p.m. UTC
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(-)
Comments
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; >
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; > >
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; >>> >
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; >>>> >> >
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; >
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; > >>>>
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;