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.
Best regards,
Tomasz
