Hi Sakari, > -----Original Message----- > From: Sakari Ailus [mailto:sakari.ailus@xxxxxxxxxxxxxxx] > Sent: Thursday, September 13, 2018 12:37 AM > To: Mani, Rajmohan <rajmohan.mani@xxxxxxxxx> > Cc: Tomasz Figa <tfiga@xxxxxxxxxxxx>; Mauro Carvalho Chehab > <mchehab+samsung@xxxxxxxxxx>; Hans Verkuil <hverkuil@xxxxxxxxx>; Zhi, > Yong <yong.zhi@xxxxxxxxx>; Linux Media Mailing List <linux- > media@xxxxxxxxxxxxxxx>; Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>; > Hans Verkuil <hans.verkuil@xxxxxxxxx>; Laurent Pinchart > <laurent.pinchart@xxxxxxxxxxxxxxxx>; Zheng, Jian Xu > <jian.xu.zheng@xxxxxxxxx>; Hu, Jerry W <jerry.w.hu@xxxxxxxxx>; Li, Chao C > <chao.c.li@xxxxxxxxx>; Qiu, Tian Shu <tian.shu.qiu@xxxxxxxxx> > Subject: Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI > > Hi Raj, Mauro, > > On Fri, Aug 31, 2018 at 10:40:22PM +0000, Mani, Rajmohan wrote: > > Hi Sakari, Mauro, > > > > > -----Original Message----- > > > From: Sakari Ailus [mailto:sakari.ailus@xxxxxxxxxxxxxxx] > > > Sent: Tuesday, August 28, 2018 2:16 AM > > > To: Tomasz Figa <tfiga@xxxxxxxxxxxx> > > > Cc: Mauro Carvalho Chehab <mchehab+samsung@xxxxxxxxxx>; Hans > Verkuil > > > <hverkuil@xxxxxxxxx>; Mani, Rajmohan <rajmohan.mani@xxxxxxxxx>; Zhi, > > > Yong <yong.zhi@xxxxxxxxx>; Linux Media Mailing List <linux- > > > media@xxxxxxxxxxxxxxx>; Mauro Carvalho Chehab > <mchehab@xxxxxxxxxx>; > > > Hans Verkuil <hans.verkuil@xxxxxxxxx>; Laurent Pinchart > > > <laurent.pinchart@xxxxxxxxxxxxxxxx>; Zheng, Jian Xu > > > <jian.xu.zheng@xxxxxxxxx>; Hu, Jerry W <jerry.w.hu@xxxxxxxxx>; Li, > > > Chao C <chao.c.li@xxxxxxxxx>; Qiu, Tian Shu <tian.shu.qiu@xxxxxxxxx> > > > Subject: Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI > > > > > > Hi Tomasz, > > > > > > On Tue, Aug 28, 2018 at 05:56:37PM +0900, Tomasz Figa wrote: > > > > On Tue, Aug 14, 2018 at 5:50 AM Mauro Carvalho Chehab > > > > <mchehab+samsung@xxxxxxxxxx> wrote: > > > > > > > > > > Em Mon, 13 Aug 2018 15:42:34 +0200 Hans Verkuil > > > > > <hverkuil@xxxxxxxxx> escreveu: > > > > > > > > > > > On 15/06/18 05:29, Yong Zhi wrote: > > > > > > > These meta formats are used on Intel IPU3 ImgU video queues > > > > > > > to carry 3A statistics and ISP pipeline parameters. > > > > > > > > > > > > > > V4L2_META_FMT_IPU3_3A > > > > > > > V4L2_META_FMT_IPU3_PARAMS > > > > > > > > > > > > > > Signed-off-by: Yong Zhi <yong.zhi@xxxxxxxxx> > > > > > > > Signed-off-by: Chao C Li <chao.c.li@xxxxxxxxx> > > > > > > > Signed-off-by: Rajmohan Mani <rajmohan.mani@xxxxxxxxx> > > > > > > > --- > > > > > > > Documentation/media/uapi/v4l/meta-formats.rst | 1 + > > > > > > > .../media/uapi/v4l/pixfmt-meta-intel-ipu3.rst | 174 ++ > > > > > > > include/uapi/linux/intel-ipu3.h | 2816 > > > ++++++++++++++++++++ > > > > > > > 3 files changed, 2991 insertions(+) create mode 100644 > > > > > > > Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst > > > > > > > create mode 100644 include/uapi/linux/intel-ipu3.h > > > > > > > > > > > > > > diff --git a/Documentation/media/uapi/v4l/meta-formats.rst > > > > > > > b/Documentation/media/uapi/v4l/meta-formats.rst > > > > > > > index 0c4e1ec..b887fca 100644 > > > > > > > --- a/Documentation/media/uapi/v4l/meta-formats.rst > > > > > > > +++ b/Documentation/media/uapi/v4l/meta-formats.rst > > > > > > > @@ -12,6 +12,7 @@ These formats are used for the > > > > > > > :ref:`metadata` > > > interface only. > > > > > > > .. toctree:: > > > > > > > :maxdepth: 1 > > > > > > > > > > > > > > + pixfmt-meta-intel-ipu3 > > > > > > > pixfmt-meta-uvc > > > > > > > pixfmt-meta-vsp1-hgo > > > > > > > pixfmt-meta-vsp1-hgt > > > > > > > diff --git > > > > > > > a/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst > > > > > > > b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rst > > > > > > > new file mode 100644 > > > > > > > index 0000000..5c050e6 > > > > > > > --- /dev/null > > > > > > > +++ b/Documentation/media/uapi/v4l/pixfmt-meta-intel-ipu3.rs > > > > > > > +++ t > > > > > > > @@ -0,0 +1,174 @@ > > > > > > > +.. -*- coding: utf-8; mode: rst -*- > > > > > > > + > > > > > > > +.. _intel-ipu3: > > > > > > > + > > > > > > > > > > > +*************************************************************** > > > > > > > +*** V4L2_META_FMT_IPU3_PARAMS ('ip3p'), > > > V4L2_META_FMT_IPU3_3A > > > > > > > +('ip3s') > > > > > > > > > > > +*************************************************************** > > > > > > > +*** > > > > > > > + > > > > > > > +.. c:type:: ipu3_uapi_stats_3a > > > > > > > + > > > > > > > +3A statistics > > > > > > > +============= > > > > > > > + > > > > > > > +For IPU3 ImgU, the 3A statistics accelerators collect > > > > > > > +different statistics over an input bayer frame. Those > > > > > > > +statistics, defined in data struct > > > > > > > +:c:type:`ipu3_uapi_stats_3a`, are meta output obtained > > > from "ipu3-imgu 3a stat" > > > > > > > +video node, which are then passed to user space for > > > > > > > +statistics analysis using :c:type:`v4l2_meta_format` interface. > > > > > > > + > > > > > > > +The statistics collected are AWB (Auto-white balance) RGBS > > > > > > > +cells, AWB filter > > > > > > > > > > Just like you did with AWB, AF and AE, please place the full > > > > > name in parenthesis for RGBS and AWB. > > > > > > > > > > > > +response, AF (Auto-focus) filter response, and AE > > > > > > > +(Auto-exposure) > > > histogram. > > > > > > > + > > > > > > > +struct :c:type:`ipu3_uapi_4a_config` saves configurable > > > > > > > +parameters for > > > all above. > > > > > > > + > > > > > > > + > > > > > > > +.. code-block:: c > > > > > > > + > > > > > > > + > > > > > > > + struct ipu3_uapi_stats_3a { > > > > > > > + IPU3_ALIGN struct ipu3_uapi_awb_raw_buffer > > > > > > > + awb_raw_buffer; > > > > > > > > > > > > IPU3_ALIGN? What's that? > > > > > > > > > > > > OK, after reading the header I see what it does, but I think > > > > > > you should drop it in the documentation since it doesn't help the > reader. > > > > > > > > > > Yeah, that IPU3_ALIGN is confusing. > > > > > > > > > > Yet, instead of just dropping, I would replace it by a comment > > > > > to explain that the struct is 32-bytes aligned. > > > > > > > > > > On a separate (but related) comment, you're declaring it as: > > > > > > > > > > #define IPU3_ALIGN > > > __attribute__((aligned(IPU3_UAPI_ISP_WORD_BYTES))) > > > > > > > > > > This is a gcc-specific dialect. Better to use, instead, > > > > > __aligned(x) which is defined as: > > > > > > > > > > #define __aligned(x) __attribute__((aligned(x))) > > > > > > > > > > > > > Note that this is an uapi/ header. Is the __aligned() macro okay > > > > to use in uapi headers? I couldn't find any header there using it > > > > and we had problems with our user space compiling with it. > > > > > > > > By the way, I wonder if this is the right approach for controlling > > > > the layout of ABI structs. I don't see many headers using any > > > > alignment in uapi/ in general. Perhaps explicit padding bytes > > > > would be more appropriate? They are also less tricky when one > > > > structure needs to be embedded inside two or more different > > > > structures with different alignments, which can't be done easily > > > > if you specify __aligned() on the child struct. > > > > > > One of the reasons there are not so many are probably what you just > > > elaborated above. That said, there are a few points to note here: > > > > > > - the alignment is generally the same here as it's due to DMA word size > > > AFAIK, > > > > > > - the device can be only found in an Intel SoC which limits the > > > architectures where the driver can actually be used to x86, 64- or > > > 32-bit. > > > > > > Together these should in theory make if fairly safe. Padding in > > > principle would be more explicit way to force struct memory layout > > > without relying so much on the compiler doing the right thing but > > > it'll lead to a *lot* of reserved fields, which I think is likely > > > one of the reasons why it didn't catch up back then --- I've suggested it > earlier. > > > > > > FWIW, the rest of the uAPI headers appear to be using > > > __attribute__((aligned(x))). > > > > > > > There are no compilation errors when using __attribute__((aligned(x))) > > in the user space code. > > > > When using __aligned(x) (as Mauro recommended) as Tomasz pointed, > > these compilation errors pop up (since __aligned(x) is defined nowhere). > > > > The definition of __aligned(x) is not available under include/uapi/*. > > This is available in the kernel header include/linux/compiler_types.h > > > > There are other header files under include/uapi that use this format > > (__attribute__((aligned(x))). > > > > Should we stick with __attribute__((aligned(x))) or is there a way out? > > My suggestion is to stick with __attribute__((aligned(x))). Ack > > -- > Kind regards, > > Sakari Ailus > sakari.ailus@xxxxxxxxxxxxxxx
