On Mon, Oct 11, 2021 at 12:30 PM Cao, Bingbu <bingbu.cao@xxxxxxxxx> wrote:
>
> All,
>
> ________________________
> BRs,
> Bingbu Cao
>
> > -----Original Message-----
> > From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> > Sent: Wednesday, October 6, 2021 4:54 AM
> > To: Jean-Michel Hautbois <jeanmichel.hautbois@xxxxxxxxxxxxxxxx>
> > Cc: linux-media@xxxxxxxxxxxxxxx; sakari.ailus@xxxxxxxxxxxxxxx; Cao,
> > Bingbu <bingbu.cao@xxxxxxxxx>; tfiga@xxxxxxxxxx; Qiu, Tian Shu
> > <tian.shu.qiu@xxxxxxxxx>
> > Subject: Re: [PATCH v2] media: staging: ipu3-imgu: add the AWB memory
> > layout
> >
> > Hi Jean-Michel,
> >
> > Thank you for the patch.
> >
> > On Tue, Oct 05, 2021 at 10:20:19PM +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@xxxxxxxxxxxxxxxx>
> > > ---
> > > .../media/ipu3/include/uapi/intel-ipu3.h | 32 ++++++++++++++++---
> > > 1 file changed, 27 insertions(+), 5 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..ad116a912e44 100644
> > > --- a/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
> > > +++ b/drivers/staging/media/ipu3/include/uapi/intel-ipu3.h
> > > @@ -61,17 +61,39 @@ 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: Percentage of pixels over a given threshold set in
> >
> > s/over a given threshold set in/over the thresholds specified in/
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> >
> > I would still like a review from someone knowledgeable with the hardware
> > and firmware, as we're partly guessing here.
>
> Reviewed-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>
> I have no concern on this patch, I think it has minor impact on user-space
> implementation from this change. Tomasz, do you have any comment on this?
>
No, looks fine to me as well. Thanks!
(We'll have to update our userspace when we pick this, but it should
be trivial.)
Reviewed-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
Best regards,
Tomasz
>
> >
> > > + * ipu3_uapi_awb_config_s, coded from 0 to 255.
> > > + * @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.
> > > */
> > > #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)) @@
> > > -83,7 +105,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;
> > >
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
