From: Tomasz Figa <tfiga@chromium.org>
To: Jacob Chen <jacob-chen@iotwrt.com>,
Shunqian Zheng <zhengsq@rock-chips.com>
Cc: "open list:ARM/Rockchip SoC..."
<linux-rockchip@lists.infradead.org>,
"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
"list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>,
Joerg Roedel <joro@8bytes.org>,"
<linux-arm-kernel@lists.infradead.org>,
"Mauro Carvalho Chehab" <mchehab@kernel.org>,
"Linux Media Mailing List" <linux-media@vger.kernel.org>,
"Sakari Ailus" <sakari.ailus@linux.intel.com>,
"Hans Verkuil" <hans.verkuil@cisco.com>,
"Laurent Pinchart" <laurent.pinchart@ideasonboard.com>,
钟以崇 <zyc@rock-chips.com>, "Eddie Cai" <eddie.cai.linux@gmail.com>,
Jeffy <jeffy.chen@rock-chips.com>,
devicetree@vger.kernel.org, "Heiko Stübner" <heiko@sntech.de>,
"Chen Jacob" <jacob2.chen@rock-chips.com>
Subject: Re: [PATCH v6 03/17] media: rkisp1: Add user space ABI definitions
Date: Tue, 24 Apr 2018 04:28:49 +0000 [thread overview]
Message-ID: <CAAFQd5APomi1tx0X=agbSQfsX3SLBwqsXyqGhWzS4mdDcS+N9g@mail.gmail.com> (raw)
In-Reply-To: <20180308094807.9443-4-jacob-chen@iotwrt.com>
On Thu, Mar 8, 2018 at 6:48 PM Jacob Chen <jacob-chen@iotwrt.com> wrote:
[snip]
> +/**
> + * struct cifisp_lsc_config - Configuration used by Lens shading
correction
> + *
> + * refer to REF_01 for details
> + */
> +struct cifisp_lsc_config {
> + __u32 r_data_tbl[CIFISP_LSC_DATA_TBL_SIZE];
> + __u32 gr_data_tbl[CIFISP_LSC_DATA_TBL_SIZE];
> + __u32 gb_data_tbl[CIFISP_LSC_DATA_TBL_SIZE];
> + __u32 b_data_tbl[CIFISP_LSC_DATA_TBL_SIZE];
> +
> + __u32 x_grad_tbl[CIFISP_LSC_GRAD_TBL_SIZE];
> + __u32 y_grad_tbl[CIFISP_LSC_GRAD_TBL_SIZE];
> +
> + __u32 x_size_tbl[CIFISP_LSC_SIZE_TBL_SIZE];
> + __u32 y_size_tbl[CIFISP_LSC_SIZE_TBL_SIZE];
Looking at the code, we only need 12 bits of each, so perhaps it could make
sense to make those __u16? Also, the natural layout for these seems to be
two-dimensional, i.e. [CIFISP_LSC_NUM_SECTORS][CIFISP_LSC_NUM_SECTORS]. I
think it wouldn't be a problem to define it this way for UAPI too.
> + __u16 config_width;
> + __u16 config_height;
These 2 seem unused. Just making sure. If they are part of hardware LSC
configuration, then we should keep them.
[snip]
> +struct cifisp_awb_meas_config {
> + /*
> + * Note: currently the h and v offsets are mapped to grid offsets
> + */
Perhaps should be part of the kerneldoc comment above? Also, I don't seem
to understand what this means.
> + struct cifisp_window awb_wnd;
> + __u32 awb_mode;
> + __u8 max_y;
> + __u8 min_y;
> + __u8 max_csum;
> + __u8 min_c;
> + __u8 frames;
> + __u8 awb_ref_cr;
> + __u8 awb_ref_cb;
> + __u8 enable_ymax_cmp;
> +} __attribute__ ((packed));
> +
> +/**
> + * struct cifisp_awb_gain_config - Configuration used by auto white
balance gain
> + *
> + * out_data_x = ( AWB_GEAIN_X * in_data + 128) >> 8
typo: AWB_GAIN?
> + */
> +struct cifisp_awb_gain_config {
> + __u16 gain_red;
> + __u16 gain_green_r;
> + __u16 gain_blue;
> + __u16 gain_green_b;
> +} __attribute__ ((packed));
> +
> +/**
> + * struct cifisp_flt_config - Configuration used by ISP filtering
> + *
> + * @mode: ISP_FILT_MODE register fields (from enum cifisp_flt_mode)
> + * @grn_stage1: ISP_FILT_MODE register fields
> + * @chr_h_mode: ISP_FILT_MODE register fields
> + * @chr_v_mode: ISP_FILT_MODE register fields
Missing documentation for remaining fields.
> + *
> + * refer to REF_01 for details.
> + */
> +
> +struct cifisp_flt_config {
> + __u32 mode;
> + __u8 grn_stage1;
> + __u8 chr_h_mode;
> + __u8 chr_v_mode;
Maybe we could move u8 below u32 to optimize the alignment?
[snip]
> +/**
> + * struct cifisp_hst_config - Configuration used by Histogram
> + *
> + * @mode: histogram mode (from enum cifisp_histogram_mode)
> + * @histogram_predivider: process every stepsize pixel, all other pixels
are skipped
> + * @meas_window: coordinates of the measure window
> + * @hist_weight: weighting factor for sub-windows
> + */
> +struct cifisp_hst_config {
> + __u32 mode;
> + __u8 histogram_predivider;
> + struct cifisp_window meas_window;
Perhaps could swap the two above for better alignment?
> + __u8 hist_weight[CIFISP_HISTOGRAM_WEIGHT_GRIDS_SIZE];
> +} __attribute__ ((packed));
> +
> +/**
> + * struct cifisp_aec_config - Configuration used by Auto Exposure Control
> + *
> + * @mode: Exposure measure mode (from enum cifisp_exp_meas_mode)
> + * @autostop: stop mode (from enum cifisp_exp_ctrl_autostop)
> + * @meas_window: coordinates of the measure window
> + */
> +struct cifisp_aec_config {
> + __u32 mode;
> + __u32 autostop;
> + struct cifisp_window meas_window;
> +} __attribute__ ((packed));
> +
> +/**
> + * struct cifisp_afc_config - Configuration used by Auto Focus Control
> + *
> + * @num_afm_win: max CIFISP_AFM_MAX_WINDOWS
> + * @afm_win: coordinates of the meas window
> + * @thres: threshold used for minimizing the influence of noise
> + * @var_shift: the number of bits for the shift operation at the end of
the calculation chain.
> + */
> +struct cifisp_afc_config {
> + __u8 num_afm_win;
> + struct cifisp_window afm_win[CIFISP_AFM_MAX_WINDOWS];
> + __u32 thres;
> + __u32 var_shift;
Perhaps could put afm_win[] and then num_afm_win here, for better alignment?
Best regards,
Tomasz
next prev parent reply other threads:[~2018-04-24 4:29 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-08 9:47 [PATCH v6 00/17] Rockchip ISP1 Driver Jacob Chen
2018-03-08 9:47 ` [PATCH v6 01/17] media: videodev2.h, v4l2-ioctl: add rkisp1 meta buffer format Jacob Chen
2019-04-26 20:34 ` Nicolas Dufresne
2018-03-08 9:47 ` [PATCH v6 02/17] media: doc: add document for " Jacob Chen
2018-03-09 13:37 ` Hans Verkuil
2018-03-08 9:47 ` [PATCH v6 03/17] media: rkisp1: Add user space ABI definitions Jacob Chen
2018-03-09 13:34 ` Hans Verkuil
2018-04-24 4:28 ` Tomasz Figa [this message]
2018-03-08 9:47 ` [PATCH v6 04/17] media: rkisp1: add Rockchip MIPI Synopsys DPHY driver Jacob Chen
2018-05-16 5:20 ` Laurent Pinchart
2018-05-16 14:39 ` Jacob Chen
2018-05-16 14:53 ` Jacob Chen
2018-05-16 15:15 ` Tomasz Figa
2019-03-10 17:49 ` Laurent Pinchart
2019-03-11 9:37 ` Tomasz Figa
2018-03-08 9:47 ` [PATCH v6 05/17] media: rkisp1: add Rockchip ISP1 subdev driver Jacob Chen
2018-03-09 13:57 ` Hans Verkuil
2018-05-03 9:09 ` Baruch Siach
2018-05-07 6:13 ` Tomasz Figa
2018-05-07 6:38 ` Baruch Siach
2018-05-07 6:41 ` Tomasz Figa
2018-05-24 11:30 ` Baruch Siach
2018-05-25 4:35 ` Tomasz Figa
2018-03-08 9:47 ` [PATCH v6 06/17] media: rkisp1: add ISP1 statistics driver Jacob Chen
2018-03-09 13:59 ` Hans Verkuil
2018-03-08 9:47 ` [PATCH v6 07/17] media: rkisp1: add ISP1 params driver Jacob Chen
2018-03-09 14:03 ` Hans Verkuil
2018-03-08 9:47 ` [PATCH v6 08/17] media: rkisp1: add capture device driver Jacob Chen
2018-03-09 14:07 ` Hans Verkuil
2018-04-17 8:44 ` Tomasz Figa
2018-03-08 9:47 ` [PATCH v6 09/17] media: rkisp1: add rockchip isp1 core driver Jacob Chen
2018-03-11 6:58 ` Baruch Siach
2018-05-16 8:08 ` Tomasz Figa
2018-03-08 9:48 ` [PATCH v6 10/17] dt-bindings: Document the Rockchip ISP1 bindings Jacob Chen
2018-03-08 9:48 ` [PATCH v6 11/17] dt-bindings: Document the Rockchip MIPI RX D-PHY bindings Jacob Chen
2018-03-08 9:48 ` [PATCH v6 12/17] ARM: dts: rockchip: add isp node for rk3288 Jacob Chen
2018-03-08 9:48 ` [PATCH v6 13/17] ARM: dts: rockchip: add rx0 mipi-phy " Jacob Chen
2018-03-08 9:48 ` [PATCH v6 14/17] ARM: dts: rockchip: Add dts mipi-dphy TXRX1 node " Jacob Chen
2018-03-08 9:48 ` [PATCH v6 15/17] arm64: dts: rockchip: add isp0 node for rk3399 Jacob Chen
2018-03-12 3:49 ` [PATCH v6-1,15/17] " Shunqian Zheng
2018-03-08 9:48 ` [PATCH v6 16/17] arm64: dts: rockchip: add rx0 mipi-phy " Jacob Chen
2018-03-08 9:48 ` [PATCH v6 17/17] MAINTAINERS: add entry for Rockchip ISP1 driver Jacob Chen
2018-03-08 10:29 ` [PATCH v6 00/17] Rockchip ISP1 Driver Tomasz Figa
2018-03-08 12:02 ` Baruch Siach
2018-03-09 0:53 ` Jacob Chen
2018-03-09 4:09 ` Baruch Siach
2018-03-09 5:05 ` Jacob Chen
2018-03-09 5:54 ` Baruch Siach
2018-03-09 14:12 ` Hans Verkuil
2018-03-12 3:37 ` [v6-1,15/17] arm64: dts: rockchip: add isp0 node for rk3399 Shunqian Zheng
2018-03-12 3:51 ` Shunqian Zheng
2018-09-03 13:07 ` [PATCH v6 00/17] Rockchip ISP1 Driver Hans Verkuil
2018-09-03 13:50 ` Tomasz Figa
2018-09-03 14:15 ` Heiko Stuebner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAAFQd5APomi1tx0X=agbSQfsX3SLBwqsXyqGhWzS4mdDcS+N9g@mail.gmail.com' \
--to=tfiga@chromium.org \
--cc=devicetree@vger.kernel.org \
--cc=eddie.cai.linux@gmail.com \
--cc=hans.verkuil@cisco.com \
--cc=heiko@sntech.de \
--cc=jacob-chen@iotwrt.com \
--cc=jacob2.chen@rock-chips.com \
--cc=jeffy.chen@rock-chips.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=mchehab@kernel.org \
--cc=sakari.ailus@linux.intel.com \
--cc=zhengsq@rock-chips.com \
--cc=zyc@rock-chips.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).