Hi Yong,
On Thu, Nov 29, 2018 at 11:06:23PM +, Zhi, Yong wrote:
> Hi, Sakari,
>
> > -Original Message-
> > From: Sakari Ailus [mailto:sakari.ai...@linux.intel.com]
> > Sent: Thursday, November 29, 2018 4:46 PM
> > To: Zhi, Yong
> > Cc: linux-media@vger.kernel.org; tf...@chromium.org;
> > mche...@kernel.org; hans.verk...@cisco.com;
> > laurent.pinch...@ideasonboard.com; Mani, Rajmohan
> > ; Zheng, Jian Xu ; Hu,
> > Jerry W ; Toivonen, Tuukka
> > ; Qiu, Tian Shu ; Cao,
> > Bingbu ; Li, Chao C
> > Subject: Re: [PATCH v7 03/16] v4l: Add Intel IPU3 meta data uAPI
> >
> > Hi Yong,
> >
> > On Fri, Nov 16, 2018 at 10:37:00PM +, Zhi, Yong wrote:
> > ...
> > > > > +/**
> > > > > + * struct ipu3_uapi_shd_grid_config - Bayer shading(darkening)
> > > > > +correction
> > > > > + *
> > > > > + * @width: Grid horizontal dimensions, u8, [8, 128], default 73
> > > > > + * @height: Grid vertical dimensions, u8, [8, 128], default 56
> > > > > + * @block_width_log2:Log2 of the width of the grid cell in
> > > > > pixel
> > > > count
> > > > > + * u4, [0, 15], default value 5.
> > > > > + * @__reserved0: reserved
> > > > > + * @block_height_log2: Log2 of the height of the grid cell in
> > > > > pixel
> > > > count
> > > > > + * u4, [0, 15], default value 6.
> > > > > + * @__reserved1: reserved
> > > > > + * @grid_height_per_slice: SHD_MAX_CELLS_PER_SET/width.
> > > > > + * (with SHD_MAX_CELLS_PER_SET =
> > 146).
> > > > > + * @x_start: X value of top left corner of sensor relative to ROI
> > > > > + * u12, [-4096, 0]. default 0, only negative values.
> > > > > + * @y_start: Y value of top left corner of sensor relative to ROI
> > > > > + * u12, [-4096, 0]. default 0, only negative values.
> > > >
> > > > I suppose u12 is incorrect here, if the value is signed --- and
> > > > negative (sign bit) if not 0?
> > > >
> > >
> > > The value will be written to 13 bit register, should use s12.0.
> >
> > If you have s12, that means the most significant bit is the sign bit. So if
> > the
> > smallest value is -4096, you'd need s13.
> >
> > But where is the sign bit, i.e. is this either s13 or s16?
> >
>
> The notation of s12.0 means 13 bit with fraction bit as 0 right?
In s12.0, bit 11 is the sign bit, and bits 10--0 are the integer part. The
smallest number that can be represented is thus -2048 (not -4096).
>
> > >
> > > > > + */
> > > > > +struct ipu3_uapi_shd_grid_config {
> > > > > + /* reg 0 */
> > > > > + __u8 width;
> > > > > + __u8 height;
> > > > > + __u8 block_width_log2:3;
> > > > > + __u8 __reserved0:1;
> > > > > + __u8 block_height_log2:3;
> > > > > + __u8 __reserved1:1;
> > > > > + __u8 grid_height_per_slice;
> > > > > + /* reg 1 */
> > > > > + __s16 x_start;
> > > > > + __s16 y_start;
> > > > > +} __packed;
> >
> > ...
> >
> > > > > +/**
> > > > > + * struct ipu3_uapi_iefd_cux2_1 - Calculate power of non-directed
> > denoise
> > > > > + * element apply.
> > > > > + * @x0: X0 point of Config Unit, u9.0, default 0.
> > > > > + * @x1: X1 point of Config Unit, u9.0, default 0.
> > > > > + * @a01: Slope A of Config Unit, s4.4, default 0.
> > > >
> > > > The field is marked unsigned below. Which one is correct?
> > > >
> > >
> > > They are both correct, however, s4.4 is the internal representation
> > > used by CU, the inputs are unsigned, I will add a note in v8, same
> > > applies to the few other places as you commented.
> >
> > I still find this rather confusing. Is there a sign bit or is there not?
> >
>
> It's unsigned number from driver perspective, all CU inputs are unsigned,
> however, they will be "converted" to signed for FW/HW to use. I have to
> consult FW expert if more clarification is needed.
I think that would be good to have; if you somehow convert an unsigned
integer to a negative number, there's more than just the type cast there.
--
Kind regards,
Sakari Ailus
sakari.ai...@linux.intel.com
