Hi Mauro,
Thanks for the reviews.
> Subject: Re: [PATCH v1 2/2] v4l: Document Intel IPU3 meta data uAPI
>
> 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.
>
Ack
> > > +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.
>
Ack
> 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)))
>
As discussed in the other thread, the following will be used directly,
as applicable for required data structures.
__attribute__((aligned(32)))
[snip]
> > > +
> > > +#define IPU3_UAPI_ISP_WORD_BYTES 32
> > > +#define IMGU_ABI_PAD __aligned(IPU3_UAPI_ISP_WORD_BYTES)
> > > +#define IPU3_ALIGN
> __attribute__((aligned(IPU3_UAPI_ISP_WORD_BYTES)))
> >
> > That's weird. Isn't __aligned identical to __attribute__((aligned( ?
> > If so, then these last two defines are also identical.
>
> Yes, AFAIKT they're identical and __aligned() is the preferred form.
>
As discussed in the other thread, the following will be used directly,
as applicable for required data structures.
__attribute__((aligned(x)))
> >
> > Why is it aligned to a 32 bytes? Is it a hardware requirement?
> >
> > > +
> > > +/******************* ipu3_uapi_stats_3a *******************/
> > > +
> > > +#define IPU3_UAPI_MAX_STRIPES 2
> > > +#define IPU3_UAPI_MAX_BUBBLE_SIZE 10
> > > +
> > > +#define IPU3_UAPI_GRID_START_MASK (BIT(12) - 1)
> > > +#define IPU3_UAPI_GRID_Y_START_EN BIT(15)
> > > +
> > > +/* controls generation of meta_data (like FF enable/disable) */
> > > +#define IPU3_UAPI_AWB_RGBS_THR_B_EN BIT(14)
> > > +#define IPU3_UAPI_AWB_RGBS_THR_B_INCL_SAT BIT(15)
> > > +
> > > +/**
> > > + * struct ipu3_uapi_grid_config - Grid plane config
> > > + *
> > > + * @width: Grid horizontal dimensions, in number of grid blocks(cells).
> > > + * @height: Grid vertical dimensions, in number of grid cells.
> > > + * @block_width_log2: Log2 of the width of each cell in pixels.
> > > + * for (2^3, 2^4, 2^5, 2^6, 2^7), values [3, 7].
>
> What are you meaning by [3, 7] here?
All integers from 3 through 7, including 3 and 7.
> From some comment you had later,
> I guess you're meaning that only 3 or 7 are the valid values.
>
> Yet, you're listing from 2^3 to 2^7, and that's confusing. Perhaps
> you want to say, instead, that the valid values are at the 3..7 range?
> If so, please use something like "values at the [3..7] range".
>
As Sakari pointed / preferred in the other thread, we will use the format
[3, 7] to represent all integers between 3 and 7, including 3 and 7.
> > > + * @block_height_log2: Log2 of the height of each cell in pixels.
> > > + * for (2^3, 2^4, 2^5, 2^6, 2^7), values [3, 7].
>
> Same here.
>
Ack.
[3, 7] format will be used uniformly.
[snip]
> > > + * @width: Grid horizontal dimensions. Value: [16,32], default 16.
>
> if [16,32] is a range, please use [16..32]. If only 16 or 32 are
> valid values, please say something like:
>
> Value: 16 or 32. Defaults to 16.
>
> Same to other places.
>
> > > + * @height: Grid vertical dimensions. Value: [16,24], default 16.
>
> Same here.
>
[16, 32] is a range.
(As Sakari pointed [16, 32] is the same as [16..32]. We will use the former
format uniformly).
[snip]
> > > +/**
> > > + * struct ipu3_uapi_af_meta_data - AF meta data
> > > + *
> > > + * @y_table: Each color component will be convolved separately
> with filter1
> > > + * and filter2 and the result will be summed out and averaged for
> > > + * each cell.
> > > + */
> > > +struct ipu3_uapi_af_meta_data {
> > > + __u8 y_table[IPU3_UAPI_AF_Y_TABLE_MAX_SIZE] IPU3_ALIGN;
> >
> > Here IPU3_ALIGN is put at the end...
> >
> > > +} __packed;
> > > +
> > > +/**
> > > + * struct ipu3_uapi_af_raw_buffer - AF raw buffer
> > > + *
> > > + * @meta_data: raw buffer &ipu3_uapi_af_meta_data for auto focus
> meta data.
> > > + */
> > > +struct ipu3_uapi_af_raw_buffer {
> > > + IPU3_ALIGN struct ipu3_uapi_af_meta_data meta_data;
> >
> > ... and here at the start. Is that due to the difference between an array and a
> struct?
>
No.
This was done to work around the Sphinx / kerneldoc generation as
discussed in the other thread.
> If placing it at the end works (I guess it works for structs), I would
> prefer it always at the end.
>
> Btw, IMO, I would, instead be doing something like:
>
> #define IPU3_ALIGN 32
>
> (e. g. some short name - IPU3_UAPI_ISP_WORD_BYTES is too big to my taste :-)
> )
>
> and replace all occurrences of it by:
>
> IPU3_ALIGN -> __aligned(IPU3_ALIGN)
>
> That would make it a way easier to read and review.
>
Ack.
Since Sphinx / kernel doc generation has warnings / errors, we will stick with
the following format.
__attribute__((aligned(32)))
[snip]
> > > + * @ci: Intensity coefficient for threshold calculation. u3.2, range
> [0..7.75]
>
> hmm... 7.75??? ci is an _u32 with 5 bits... it can't be a floating point value!
>
> also, what do you mean by u3.2?
>
By u3.2, we mean an unsigned integer with 5 bits, with 3 most significant
bits used for the decimal number and the 2 least significant bits used for
the fractional part.
So, 7.75, will be represented as 11111b, with 111b giving 7, 11b resulting
in 0.75 (3 times 0.25), as each unit of fractional part has a value of 0.25.
Will fix the documentation accordingly.
[snip]
> > > + * All above has precision u0.4, range [0..0xF].
>
> again, what do you mean by u0.4?
unsigned integer with 0 bits used for representing whole number,
with 4 least significant bits used to represent the fractional part.
> For hexadecimal values (here and
> everywhere)
> please prefer lowecase.
>
Ack
[snip]
>
> For coherency along the header, please represent ranges
> the same way. On other places, you used [min..max].
>
Ack
This will be represented as [min, max] uniformly.
[snip]
> > > + * @lut: 256 tabulated values of the gamma function. LUT[1].. LUT[256]
> > > + * format u13.0, range (0, 8191).
>
> Same note about ranges here... Use just one type of convention...
>
Ack
[snip]
> > > + * @coeff_b3: Bias 3x1 coefficients, s13,0 range [-8191, 8181], default
> 0.
>
> and here.
>
Ack
[snip]
> > > + * @ds_c12: range 0, 3
> > > + * @ds_c13: range 0, 3
>
> and here and similar places below.
>
Ack
[snip]
> > > +struct ipu3_uapi_anr_stitch_config {
> > > + __u32 anr_stitch_en;
> > > + __u8 __reserved[44];
>
> Better to use __aligned() here and on other similar places where you
> added __reserved fields.
>
__reserved fields are used in places, for future needs.
__aligned is used for cases where 32 byte alignment is required by IPU3 hw.
[snip]
> > > + IPU3_ALIGN struct ipu3_uapi_yuvp1_yds_config yds;
> > > + IPU3_ALIGN struct ipu3_uapi_yuvp1_chnr_config chnr;
> > > + IPU3_ALIGN __u8 __reserved1[516];
>
> Wow! lots of empty space here! I would, instead, do something like:
>
> struct ipu3_uapi_yuvp1_chnr_config chnr __aligned(IPU3_ALIGN * 517);
>
These __reservedX fields were left as such to have backward compatibility
with user space code (at the time of this patch submission).
Will clean this up in the next patch set.
> > > + IPU3_ALIGN struct ipu3_uapi_yuvp1_yds_config yds2;
> > > + IPU3_ALIGN struct ipu3_uapi_yuvp2_tcc_static_config tcc;
> > > + __u8 __reserved2[1152];
>
> and something similar here.
>
Ditto
[snip]
> > >
> >
> > So the level of documentation looks good to me.
>
> Same for me. See above for my comments.
>
Good to know.
> > The use of IPU3_ALIGN, padding and reserved fields
> > seems a bit of a mix to me, it's not always clear why a certain approach is
> taken.
> >
> > Did you check if the layout of all these structs is the same between 32 and 64
> bit code?
> > I would recommend that you automate this (e.g. abi-dumper might be useful
> for this).
> > I do need to have confirmation that they are indeed identical.
>
> Yeah, adding a 32 bits compat code here would be really painful. Let's
> avoid it if possible.
>
This is taken care as we have verified the size and layout of the structs
remain the same across 32 bit and 64 bit builds.
Thanks
Raj
