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.rst > > > > > > @@ -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))). -- Kind regards, Sakari Ailus sakari.ailus@xxxxxxxxxxxxxxx
