Hi Dan,
Thank you for the patch.
On Wed, May 29, 2024 at 04:28:56PM +0100, Daniel Scally wrote:
> Add structures describing the ISP parameters to mali-c55-config.h
>
> Acked-by: Nayden Kanchev <nayden.kanchev@xxxxxxx>
> Co-developed-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
> ---
> Changes in v5:
>
> - New patch
>
> .../uapi/linux/media/arm/mali-c55-config.h | 669 ++++++++++++++++++
> 1 file changed, 669 insertions(+)
>
> diff --git a/include/uapi/linux/media/arm/mali-c55-config.h b/include/uapi/linux/media/arm/mali-c55-config.h
> index 8fb89af6c874..fce14bc74f4a 100644
> --- a/include/uapi/linux/media/arm/mali-c55-config.h
> +++ b/include/uapi/linux/media/arm/mali-c55-config.h
> @@ -179,4 +179,673 @@ struct mali_c55_stats_buffer {
> __u32 reserved3[15];
> } __attribute__((packed));
>
> +/**
> + * enum mali_c55_param_buffer_version - Mali-C55 parameters block versioning
> + *
> + * @MALI_C55_PARAM_BUFFER_V0: First version of Mali-C55 parameters block
So you like versions to be 0-based ? :-)
> + */
> +enum mali_c55_param_buffer_version {
> + MALI_C55_PARAM_BUFFER_V0,
> +};
> +
> +/**
> + * enum mali_c55_param_block_type - Enumeration of Mali-C55 parameter blocks
> + *
> + * This enumeration defines the types of Mali-C55 parameters block. Each block
> + * configures a specific processing block of the Mali-C55 ISP. The block
> + * type allows the driver to correctly interpret the parameters block data.
> + *
> + * It is the responsibility of userspace to correctly set the type of each
> + * parameters block.
> + *
> + * @MALI_C55_PARAM_BLOCK_SENSOR_OFFS: Sensor pre-shading black level offset
> + * @MALI_C55_PARAM_BLOCK_AEXP_HIST: Auto-exposure 1024-bin histogram
> + * configuration
> + * @MALI_C55_PARAM_BLOCK_AEXP_IHIST: Post-Iridix auto-exposure 1024-bin
> + * histogram configuration
> + * @MALI_C55_PARAM_BLOCK_AEXP_HIST_WEIGHTS: Auto-exposure 1024-bin histogram
> + * weighting
> + * @MALI_C55_PARAM_BLOCK_AEXP_IHIST_WEIGHTS: Post-Iridix auto-exposure 1024-bin
> + * histogram weighting
Do you expect applications to need to set the histogram configuration
and weights separately ?
> + * @MALI_C55_PARAM_BLOCK_DIGITAL_GAIN: Digital gain
> + * @MALI_C55_PARAM_BLOCK_AWB_GAINS: Auto-white balance gains
I was thinking that those two could be combined, but if the former is
used by AEC and the latter by AWB, maybe keeping them separate would
make like easier for userspace.
> + * @MALI_C55_PARAM_BLOCK_AWB_CONFIG: Auto-white balance statistics config
> + * @MALI_C55_PARAM_BLOCK_AWB_GAINS_AEXP: Auto-white balance gains for AEXP-0 tap
> + * @MALI_C55_PARAM_MESH_SHADING_CONFIG : Mesh shading tables configuration
> + * @MALI_C55_PARAM_MESH_SHADING_SELECTION: Mesh shading table selection
> + * @MALI_C55_PARAM_BLOCK_SENTINEL: First non-valid block index
You should indicate somewhere the correspondance between the block type
and the block data structure. It could be done in the definition of each
data structure for instance.
> + */
> +enum mali_c55_param_block_type {
> + MALI_C55_PARAM_BLOCK_SENSOR_OFFS,
> + MALI_C55_PARAM_BLOCK_AEXP_HIST,
> + MALI_C55_PARAM_BLOCK_AEXP_IHIST,
> + MALI_C55_PARAM_BLOCK_AEXP_HIST_WEIGHTS,
> + MALI_C55_PARAM_BLOCK_AEXP_IHIST_WEIGHTS,
> + MALI_C55_PARAM_BLOCK_DIGITAL_GAIN,
> + MALI_C55_PARAM_BLOCK_AWB_GAINS,
> + MALI_C55_PARAM_BLOCK_AWB_CONFIG,
> + MALI_C55_PARAM_BLOCK_AWB_GAINS_AEXP,
> + MALI_C55_PARAM_MESH_SHADING_CONFIG,
> + MALI_C55_PARAM_MESH_SHADING_SELECTION,
> + MALI_C55_PARAM_BLOCK_SENTINEL,
> +};
> +
> +/**
> + * struct mali_c55_params_block_header - Mali-C55 parameter block header
> + *
> + * This structure represents the common part of all the ISP configuration
> + * blocks. Each parameters block shall embed an instance of this structure type
s/shall embed/embeds/
> + * as its first member, followed by the block-specific configuration data. The
> + * driver inspects this common header to discern the block type and its size and
> + * properly handle the block content by casting it to the correct block-specific
> + * type.
> + *
> + * The @type field is one of the values enumerated by
> + * :c:type:`mali_c55_param_block_type` and specifies how the data should be
> + * interpreted by the driver. The @size field specifies the size of the
> + * parameters block and is used by the driver for validation purposes. The
> + * @enabled field specifies if the ISP block should be enabled (and configured
> + * according to the provided parameters) or disabled.
> + *
> + * .. code-block:: c
> + *
> + * struct mali_c55_params_block_header *block = ...;
> + *
> + * switch (block->type) {
> + * case MALI_C55_PARAM_BLOCK_SENSOR_OFFS:
> + * struct mali_c55_params_sensor_off_preshading *sensor_offs =
> + * (struct mali_c55_params_sensor_off_preshading *)block;
> + *
> + * if (block->size !=
> + * sizeof(struct mali_c55_params_sensor_off_preshading))
> + * return -EINVAL;
> + *
> + * handle_sensor_offs(sensor_offs);
> + * break;
> + *
> + * case MALI_C55_PARAM_BLOCK_AEXP_HIST:
> + * struct mali_c55_params_aexp_hist *aexp_hist =
> + * (struct mali_c55_params_aexp_hist)block;
> + *
> + * if (block->size != sizeof(mali_c55_params_aexp_hist))
> + * return -EINVAL;
> + *
> + * handle_aexp_hist(aesp_hist);
> + * break;
> + *
> + * ...
> + *
> + * }
I would probably skip this. The kernel-side of the implementation can be
found in the driver. I would document the UAPI here with examples
focussing on the userspace side only.
> + *
> + * Userspace is responsible for correctly populating the parameters block header
> + * fields (@type, @enabled and @size) and correctly populate the block-specific
> + * parameters.
> + *
> + * For example:
> + *
> + * .. code-block:: c
> + *
> + * void populate_sensor_offs(struct mali_c55_params_block_header *block) {
> + * block->type = MALI_C55_PARAM_BLOCK_SENSOR_OFFS;
> + * block->enabled = true;
> + * block->size = sizeof(struct mali_c55_params_sensor_off_preshading);
> + *
> + * struct mali_c55_params_sensor_off_preshading *sensor_offs =
> + * (struct mali_c55_params_sensor_off_preshading *)block;
> + *
> + * sensor_offs->chan00 = offset00;
> + * sensor_offs->chan01 = offset01;
> + * sensor_offs->chan10 = offset10;
> + * sensor_offs->chan11 = offset11;
> + * }
> + *
> + * @type: The parameters block type (enum mali_c55_param_block_type)
> + * @enabled: Block enabled/disabled flag
Does this flag apply to all blocks ? If not, it would be good to
indicate which blocks it applies to.
Also, I think we discussed previously that, when a block is disabled,
its configuration parameters are not applicable anymore, and could then
be skipped. Is that something useful to do ?
> + * @size: Size (in bytes) of the parameters block
> + */
> +struct mali_c55_params_block_header {
> + enum mali_c55_param_block_type type;
> + bool enabled;
> + size_t size;
enums, bool and size_t shouldn't be used in a UAPI, as they size may
vary between 32- and 64-bit architectures, making 32-bit userspace on a
64-bit kernel require compat handling in the driver. Use types with
well-defined sizes. Same below.
> +};
> +
> +/**
> + * struct mali_c55_params_sensor_off_preshading - offset subtraction for each
> + * color channel
> + *
> + * Provides removal of the sensor black level from the sensor data. Separate
> + * offsets are provided for each of the four Bayer component color channels
That's a weird spelling of offsets.
> + * which are defaulted to R, Gr, Gb, B.
I think it would be useful to indicate, for each structure, what block
it corresponds to in the ascii diagram in patch 12/16. On a side note,
at some point turning that diagram into SVG may make sense.
> + *
> + * @header: The Mali-C55 parameters block header
> + * @chan00: Offset for color channel 00 (default: R)
> + * @chan01: Offset for color channel 01 (default: Gr)
> + * @chan10: Offset for color channel 10 (default: Gb)
> + * @chan11: Offset for color channel 11 (default: B)
> + */
> +struct mali_c55_params_sensor_off_preshading {
> + struct mali_c55_params_block_header header;
> + __u32 chan00;
> + __u32 chan01;
> + __u32 chan10;
> + __u32 chan11;
> +};
> +
> +/**
> + * enum mali_c55_aexp_hist_tap_points - Tap points for the AEXP histogram
> + * @MALI_C55_AEXP_HIST_TAP_WB: After static white balance
> + * @MALI_C55_AEXP_HIST_TAP_FS: After WDR Frame Stitch
> + * @MALI_C55_AEXP_HIST_TAP_TPG: After the test pattern generator
> + */
> +enum mali_c55_aexp_hist_tap_points {
> + MALI_C55_AEXP_HIST_TAP_WB = 0,
> + MALI_C55_AEXP_HIST_TAP_FS,
> + MALI_C55_AEXP_HIST_TAP_TPG,
> +};
> +
> +/**
> + * enum mali_c55_aexp_skip_x - Horizontal pixel skipping
> + * @MALI_C55_AEXP_SKIP_X_EVERY_2ND: Collect every 2nd pixel horizontally
> + * @MALI_C55_AEXP_SKIP_X_EVERY_3RD: Collect every 3rd pixel horizontally
> + * @MALI_C55_AEXP_SKIP_X_EVERY_4TH: Collect every 4th pixel horizontally
> + * @MALI_C55_AEXP_SKIP_X_EVERY_5TH: Collect every 5th pixel horizontally
> + * @MALI_C55_AEXP_SKIP_X_EVERY_8TH: Collect every 8th pixel horizontally
> + * @MALI_C55_AEXP_SKIP_X_EVERY_9TH: Collect every 9th pixel horizontally
> + */
> +enum mali_c55_aexp_skip_x {
> + MALI_C55_AEXP_SKIP_X_EVERY_2ND,
> + MALI_C55_AEXP_SKIP_X_EVERY_3RD,
> + MALI_C55_AEXP_SKIP_X_EVERY_4TH,
> + MALI_C55_AEXP_SKIP_X_EVERY_5TH,
> + MALI_C55_AEXP_SKIP_X_EVERY_8TH,
> + MALI_C55_AEXP_SKIP_X_EVERY_9TH
> +};
Does this mean that that the histogram can't operate on every pixels,
but will always skip at least every other pixel ?
> +
> +/**
> + * enum mali_c55_aexp_skip_y - Vertical pixel skipping
> + * @MALI_C55_AEXP_SKIP_Y_ALL: Collect every single pixel vertically
> + * @MALI_C55_AEXP_SKIP_Y_EVERY_2ND: Collect every 2nd pixel vertically
> + * @MALI_C55_AEXP_SKIP_Y_EVERY_3RD: Collect every 3rd pixel vertically
> + * @MALI_C55_AEXP_SKIP_Y_EVERY_4TH: Collect every 4th pixel vertically
> + * @MALI_C55_AEXP_SKIP_Y_EVERY_5TH: Collect every 5th pixel vertically
> + * @MALI_C55_AEXP_SKIP_Y_EVERY_8TH: Collect every 8th pixel vertically
> + * @MALI_C55_AEXP_SKIP_Y_EVERY_9TH: Collect every 9th pixel vertically
> + */
> +enum mali_c55_aexp_skip_y {
> + MALI_C55_AEXP_SKIP_Y_ALL,
> + MALI_C55_AEXP_SKIP_Y_EVERY_2ND,
> + MALI_C55_AEXP_SKIP_Y_EVERY_3RD,
> + MALI_C55_AEXP_SKIP_Y_EVERY_4TH,
> + MALI_C55_AEXP_SKIP_Y_EVERY_5TH,
> + MALI_C55_AEXP_SKIP_Y_EVERY_8TH,
> + MALI_C55_AEXP_SKIP_Y_EVERY_9TH
> +};
> +
> +/**
> + * enum mali_c55_aexp_row_column_offset - Start from the first or second row or
> + * column
> + * @MALI_C55_AEXP_FIRST_ROW_OR_COL: Start from the first row / column
> + * @MALI_C55_AEXP_SECOND_ROW_OR_COL: Start from the second row / column
> + */
> +enum mali_c55_aexp_row_column_offset {
> + MALI_C55_AEXP_FIRST_ROW_OR_COL = 1,
> + MALI_C55_AEXP_SECOND_ROW_OR_COL = 2,
> +};
> +
> +/**
> + * enum mali_c55_aexp_hist_plane_mode - Mode for the AEXP Histograms
As histograms are computed on bayer data, I'd talk about "component"
instead of "plane" here, unless the word plane matches documentation.
> + * @MALI_C55_AEXP_HIST_COMBINED: All color planes in one 1024-bin histogram
> + * @MALI_C55_AEXP_HIST_SEPARATE: Each color plane in one 256-bin histogram with a bin width of 16
> + * @MALI_C55_AEXP_HIST_FOCUS_00: Top left plane in the first bank, rest in second bank
> + * @MALI_C55_AEXP_HIST_FOCUS_01: Top right plane in the first bank, rest in second bank
> + * @MALI_C55_AEXP_HIST_FOCUS_10: Bottom left plane in the first bank, rest in second bank
> + * @MALI_C55_AEXP_HIST_FOCUS_11: Bottom right plane in the first bank, rest in second bank
> + *
> + * In the "focus" modes statistics are collected into two 512-bin histograms
> + * with a bin width of 8. One colour plane is in the first histogram with the
> + * remainder combined into the second. The four options represent which of the
> + * four positions in a bayer pattern are the focused plane.
How does that work with x/y skipping ? Is skipping then applied to 2x2
blocks, or still to pixels ?
> + */
> +enum mali_c55_aexp_hist_plane_mode {
> + MALI_C55_AEXP_HIST_COMBINED = 0,
> + MALI_C55_AEXP_HIST_SEPARATE = 1,
> + MALI_C55_AEXP_HIST_FOCUS_00 = 4,
> + MALI_C55_AEXP_HIST_FOCUS_01 = 5,
> + MALI_C55_AEXP_HIST_FOCUS_10 = 6,
> + MALI_C55_AEXP_HIST_FOCUS_11 = 7,
> +};
> +
> +/**
> + * struct mali_c55_params_aexp_hist - configuration for AEXP metering hists
> + *
> + * This struct allows users to configure the 1024-bin AEXP histograms. Broadly
> + * speaking the parameters allow you to mask particular regions of the image and
> + * to select different kinds of histogram.
> + *
> + * @header: The Mali-C55 parameters block header
> + * @skip_x: Horizontal decimation. See enum mali_c55_aexp_skip_x
> + * @offset_x: Column to start from. See enum mali_c55_aexp_row_column_offset
> + * @skip_y: Vertical decimation. See enum mali_c55_aexp_skip_y
> + * @offset_y: Row to start from. See enum mali_c55_aexp_row_column_offset
Do the offsets need to be multiples of 2 ?
> + * @scale_bottom: scale of bottom half of range: 0=1x ,1=2x, 2=4x, 4=8x, 4=16x
Could you elaborate on this ?
> + * @scale_top: scale of top half of range: 0=1x ,1=2x, 2=4x, 4=8x, 4=16x
> + * @plane_mode: Plane separation mode. See enum mali_c55_aexp_hist_plane_mode
> + * @tap_point: Tap point for histogram from enum mali_c55_aexp_hist_tap_points.
> + * This parameter is unused for the post-Iridix Histogram
> + */
> +struct mali_c55_params_aexp_hist {
> + struct mali_c55_params_block_header header;
> + __u8 skip_x;
> + __u8 offset_x;
> + __u8 skip_y;
> + __u8 offset_y;
> + __u8 scale_bottom;
> + __u8 scale_top;
> + __u8 plane_mode;
> + __u8 tap_point;
> +};
> +
> +/**
> + * struct mali_c55_params_aexp_weights - Array of weights for AEXP metering
> + *
> + * This struct allows users to configure the weighting for both of the 1024-bin
> + * AEXP histograms. The pixel data collected for each zone is multiplied by the
> + * corresponding weight from this array, which may be zero if the intention is
> + * to mask off the zone entirely.
> + *
> + * @header: The Mali-C55 parameters block header
> + * @nodes_used_horiz: Number of active zones horizontally [0..15]
> + * @nodes_used_vert: Number of active zones vertically [0..15]
Is the image automatically split in the number of zones with a uniform
distribution ?
What happens if the number of zones is 0 ?
> + * @zone_weights: Zone weighting. Index is row*col where 0,0 is the top
> + * left zone continuing in raster order. Each zone can be
> + * weighted in the range [0..15]. The number of rows and
> + * columns is defined by @nodes_used_vert and
> + * @nodes_used_horiz
> + */
> +struct mali_c55_params_aexp_weights {
> + struct mali_c55_params_block_header header;
> + __u8 nodes_used_horiz;
> + __u8 nodes_used_vert;
> + __u8 zone_weights[MALI_C55_MAX_ZONES];
> +};
> +
> +/**
> + * struct mali_c55_params_digital_gain - Digital gain value
> + *
> + * This struct carries a digital gain value to set in the ISP
> + *
> + * @header: The Mali-C55 parameters block header
> + * @gain: The digital gain value to apply, in Q5.8 format.
> + */
> +struct mali_c55_params_digital_gain {
> + struct mali_c55_params_block_header header;
> + __u16 gain;
> +};
> +
> +/**
> + * enum mali_c55_awb_stats_mode - Statistics mode for AWB
> + * @MALI_C55_AWB_MODE_GRBR: Statistics collected as Green/Red and Blue/Red ratios
> + * @MALI_C55_AWB_MODE_RGBG: Statistics collected as Red/Green and Blue/Green ratios
> + */
> +enum mali_c55_awb_stats_mode {
> + MALI_C55_AWB_MODE_GRBR = 0,
> + MALI_C55_AWB_MODE_RGBG,
> +};
> +
> +/**
> + * struct mali_c55_params_awb_gains - Gain settings for auto white balance
> + *
> + * This struct allows users to configure the gains for auto-white balance. There
> + * are four gain settings corresponding to each colour channel in the bayer
> + * domain. Although named generically, the association between the gain applied
> + * and the colour channel is done automatically within the ISP depending on the
> + * input format, and so the following mapping always holds true::
> + *
> + * gain00 = R
> + * gain01 = Gr
> + * gain10 = Gb
> + * gain11 = B
How about naming them accordingly then ? :-)
> + *
> + * All of the gains are stored in Q4.8 format.
> + *
> + * @header: The Mali-C55 parameters block header
> + * @gain00: Multiplier for colour channel 00
> + * @gain01: Multiplier for colour channel 01
> + * @gain10: Multiplier for colour channel 10
> + * @gain11: Multiplier for colour channel 11
> + */
> +struct mali_c55_params_awb_gains {
> + struct mali_c55_params_block_header header;
> + __u16 gain00;
> + __u16 gain01;
> + __u16 gain10;
> + __u16 gain11;
> +};
> +
> +/**
> + * enum mali_c55_params_awb_tap_points - Tap points for the AWB statistics
> + * @MALI_C55_AWB_STATS_TAP_PF: Immediately after the Purple Fringe block
> + * @MALI_C55_AWB_STATS_TAP_CNR: Immediately after the CNR block
> + */
> +enum mali_c55_params_awb_tap_points {
> + MALI_C55_AWB_STATS_TAP_PF = 0,
> + MALI_C55_AWB_STATS_TAP_CNR,
> +};
> +
> +/**
> + * struct mali_c55_params_awb_config - Stats settings for auto-white balance
> + *
> + * This struct allows the configuration of the statistics generated for auto
> + * white balance. Pixel intensity limits can be set to exclude overly bright or
> + * dark regions of an image from the statistics entirely. Colour ratio minima
> + * and maxima can be set to discount pixels who's ratios fall outside the
> + * defined boundaries; there are two sets of registers to do this - the
> + * "min/max" ratios which bound a region and the "high/low" ratios which further
> + * trim the upper and lower ratios. For example with the boundaries configured
> + * as follows, only pixels whos colour ratios falls into the region marked "A"
> + * would be counted::
> + *
> + * cr_high
> + * 2.0 | |
> + * | cb_max --> _________________________v_____
> + * 1.8 | | \ |
> + * | | \ |
> + * 1.6 | | \ |
> + * | | \ |
> + * c 1.4 | cb_low -->|\ A \|<-- cb_high
> + * b | | \ |
> + * 1.2 | | \ |
> + * r | | \ |
> + * a 1.0 | cb_min --> |____\_________________________|
> + * t | ^ ^ ^
> + * i 0.8 | | | |
> + * o | cr_min | cr_max
> + * s 0.6 | |
> + * | cr_low
> + * 0.4 |
> + * |
> + * 0.2 |
> + * |
> + * 0.0 |_______________________________________________________________
> + * 0.0 0.2 0.4 0.6 0.8 1.0 1.2 1.4 1.6 1.8 2.0
> + * cr ratios
> + *
> + * @header: The Mali-C55 parameters block header
> + * @tap_point: The tap point from enum mali_c55_params_awb_tap_points
> + * @stats_mode: AWB statistics collection mode, see :c:type:`mali_c55_awb_stats_mode`
> + * @white_level: Upper pixel intensity (I.E. raw pixel values) limit
> + * @black_level: Lower pixel intensity (I.E. raw pixel values) limit
> + * @cr_max: Maximum R/G ratio (Q4.8 format)
> + * @cr_min: Minimum R/G ratio (Q4.8 format)
> + * @cb_max: Maximum B/G ratio (Q4.8 format)
> + * @cb_min: Minimum B/G ratio (Q4.8 format)
> + * @nodes_used_horiz: Number of active zones horizontally [0..15]
> + * @nodes_used_vert: Number of active zones vertically [0..15]
> + * @cr_high: R/G ratio trim high (Q4.8 format)
> + * @cr_low: R/G ratio trim low (Q4.8 format)
> + * @cb_high: B/G ratio trim high (Q4.8 format)
> + * @cb_low: B/G ratio trim low (Q4.8 format)
> + */
> +struct mali_c55_params_awb_config {
> + struct mali_c55_params_block_header header;
> + __u8 tap_point;
> + __u8 stats_mode;
> + __u16 white_level;
> + __u16 black_level;
> + __u16 cr_max;
> + __u16 cr_min;
> + __u16 cb_max;
> + __u16 cb_min;
> + __u8 nodes_used_horiz;
> + __u8 nodes_used_vert;
> + __u16 cr_high;
> + __u16 cr_low;
> + __u16 cb_high;
> + __u16 cb_low;
> +};
> +
> +#define MALI_C55_NUM_MESH_SHADING_ELEMENTS 3072
> +
> +/**
> + * struct mali_c55_params_mesh_shading_config - Mesh shading configuration
> + *
> + * The mesh shading correction module allows programming a separate table of
> + * either 16x16 or 32x32 node coefficients for 3 different light sources. The
> + * final correction coefficients applied are computed by blending the
> + * coefficients from two tables together.
> + *
> + * A page of 1024 32-bit integers is associated to each colour channel, with
> + * pages stored consecutively in memory. Each 32-bit integer packs 3 8-bit
> + * correction coefficients for a single node, one for each of the three light
> + * sources. The 8 most significant bits are unused. The following table
> + * describes the layout::
> + *
> + * +----------- Page (Colour Plane) 0 -------------+
> + * | @mesh[i] | Mesh Point | Bits | Light Source |
> + * +-----------+------------+-------+--------------+
> + * | 0 | 0,0 | 16,23 | LS2 |
> + * | | | 08-15 | LS1 |
> + * | | | 00-07 | LS0 |
> + * +-----------+------------+-------+--------------+
> + * | 1 | 0,1 | 16,23 | LS2 |
> + * | | | 08-15 | LS1 |
> + * | | | 00-07 | LS0 |
> + * +-----------+------------+-------+--------------+
> + * | ... | ... | ... | ... |
> + * +-----------+------------+-------+--------------+
> + * | 1023 | 31,31 | 16,23 | LS2 |
> + * | | | 08-15 | LS1 |
> + * | | | 00-07 | LS0 |
> + * +----------- Page (Colour Plane) 1 -------------+
> + * | @mesh[i] | Mesh Point | Bits | Light Source |
> + * +-----------+------------+-------+--------------+
> + * | 1024 | 0,0 | 16,23 | LS2 |
> + * | | | 08-15 | LS1 |
> + * | | | 00-07 | LS0 |
> + * +-----------+------------+-------+--------------+
> + * | 1025 | 0,1 | 16,23 | LS2 |
> + * | | | 08-15 | LS1 |
> + * | | | 00-07 | LS0 |
> + * +-----------+------------+-------+--------------+
> + * | ... | ... | ... | ... |
> + * +-----------+------------+-------+--------------+
> + * | 2047 | 31,31 | 16,23 | LS2 |
> + * | | | 08-15 | LS1 |
> + * | | | 00-07 | LS0 |
> + * +----------- Page (Colour Plane) 2 -------------+
> + * | @mesh[i] | Mesh Point | Bits | Light Source |
> + * +-----------+------------+-------+--------------+
> + * | 2048 | 0,0 | 16,23 | LS2 |
> + * | | | 08-15 | LS1 |
> + * | | | 00-07 | LS0 |
> + * +-----------+------------+-------+--------------+
> + * | 2049 | 0,1 | 16,23 | LS2 |
> + * | | | 08-15 | LS1 |
> + * | | | 00-07 | LS0 |
> + * +-----------+------------+-------+--------------+
> + * | ... | ... | ... | ... |
> + * +-----------+------------+-------+--------------+
> + * | 3071 | 31,31 | 16,23 | LS2 |
> + * | | | 08-15 | LS1 |
> + * | | | 00-07 | LS0 |
> + * +-----------+------------+-------+--------------+
> + *
> + * The @mesh_scale member determines the precision and minimum and maximum gain.
> + * For example if @mesh_scale is 0 and therefore selects 0 - 2x gain, a value of
> + * 0 in a coefficient means 0.0 gain, a value of 128 means 1.0 gain and 255
> + * means 2.0 gain.
> + *
> + * @header: The Mali-C55 parameters block header
> + * @mesh_show: Output the mesh data rather than image data
> + * @mesh_scale: Set the precision and maximum gain range of mesh shading
> + * - 0 = 0-2x gain
> + * - 1 = 0-4x gain
> + * - 2 = 0-8x gain
> + * - 3 = 0-16x gain
> + * - 4 = 1-2x gain
> + * - 5 = 1-3x gain
> + * - 6 = 1-5x gain
> + * - 7 = 1-9x gain
> + * @mesh_page_r: Mesh page select for red colour plane [0..2]
> + * @mesh_page_g: Mesh page select for green colour plane [0..2]
> + * @mesh_page_b: Mesh page select for blue colour plane [0..2]
> + * @mesh_width: Number of horizontal nodes minus 1 [15,31]
> + * @mesh_height: Number of vertical nodes minus 1 [15,31]
> + * @mesh: Mesh shading correction tables
> + */
> +struct mali_c55_params_mesh_shading_config {
> + struct mali_c55_params_block_header header;
> + bool mesh_show;
> + __u8 mesh_scale;
> + __u8 mesh_page_r;
> + __u8 mesh_page_g;
> + __u8 mesh_page_b;
> + __u8 mesh_width;
> + __u8 mesh_height;
> + __u32 mesh[MALI_C55_NUM_MESH_SHADING_ELEMENTS];
> +};
> +
> +/** enum mali_c55_params_mesh_alpha_bank - Mesh shading table bank selection
> + * @MALI_C55_MESH_ALPHA_BANK_LS0_AND_LS1 - Select Light Sources 0 and 1
> + * @MALI_C55_MESH_ALPHA_BANK_LS1_AND_LS2 - Select Light Sources 1 and 2
> + * @MALI_C55_MESH_ALPHA_BANK_LS0_AND_LS2 - Select Light Sources 0 and 2
> + */
> +enum mali_c55_params_mesh_alpha_bank {
> + MALI_C55_MESH_ALPHA_BANK_LS0_AND_LS1 = 0,
> + MALI_C55_MESH_ALPHA_BANK_LS1_AND_LS2 = 1,
> + MALI_C55_MESH_ALPHA_BANK_LS0_AND_LS2 = 4
> +};
> +
> +/**
> + * struct mali_c55_params_mesh_shading_selection - Mesh table selection
> + *
> + * The module computes the final correction coefficients by blending the ones
> + * from two light source tables, which are selected (independently for each
> + * colour channel) by the @mesh_alpha_bank_r/g/b fields.
> + *
> + * The final blended coefficients for each node are calculated using the
> + * following equation:
> + *
> + * Final coefficient = (a x LS\ :sub:`b`\ + (256 - a) x LS\ :sub:`a`\) / 256
> + *
> + * Where a is the @mesh_alpha_r/g/b value, and LS\ :sub:`a`\ and LS\ :sub:`b`\
> + * are the node cofficients for the two tables selected by the
> + * @mesh_alpha_bank_r/g/b value.
> + *
> + * The scale of the applied correction may also be controlled by tuning the
> + * @mesh_strength member. This is a modifier to the final coefficients which can
> + * be used to globally reduce the gains applied.
> + *
> + * @header: The Mali-C55 parameters block header
> + * @mesh_alpha_bank_r: Red mesh table select (c:type:`enum mali_c55_params_mesh_alpha_bank`)
> + * @mesh_alpha_bank_g: Green mesh table select (c:type:`enum mali_c55_params_mesh_alpha_bank`)
> + * @mesh_alpha_bank_b: Blue mesh table select (c:type:`enum mali_c55_params_mesh_alpha_bank`)
> + * @mesh_alpha_r: Blend coefficient for R [0..255]
> + * @mesh_alpha_g: Blend coefficient for G [0..255]
> + * @mesh_alpha_b: Blend coefficient for B [0..255]
> + * @mesh_strength: Mesh strength in Q4.12 format [0..4096]
> + */
> +struct mali_c55_params_mesh_shading_selection {
> + struct mali_c55_params_block_header header;
> + __u8 mesh_alpha_bank_r;
> + __u8 mesh_alpha_bank_g;
> + __u8 mesh_alpha_bank_b;
> + __u8 mesh_alpha_r;
> + __u8 mesh_alpha_g;
> + __u8 mesh_alpha_b;
> + __u16 mesh_strength;
> +};
> +
> +/**
> + * define MALI_C55_PARAMS_MAX_SIZE - Maximum size of all Mali C55 Parameters
> + *
> + * Though the parameters for the Mali-C55 are passed as optional blocks, the
> + * driver still needs to know the absolute maximum size so that it can allocate
> + * a buffer sized appropriately to accomodate userspace attempting to set all
> + * possible parameters in a single frame.
> + */
> +#define MALI_C55_PARAMS_MAX_SIZE \
> + sizeof(struct mali_c55_params_sensor_off_preshading) + \
> + sizeof(struct mali_c55_params_aexp_hist) + \
> + sizeof(struct mali_c55_params_aexp_weights) + \
> + sizeof(struct mali_c55_params_aexp_hist) + \
> + sizeof(struct mali_c55_params_aexp_weights) + \
> + sizeof(struct mali_c55_params_digital_gain) + \
> + sizeof(struct mali_c55_params_awb_gains) + \
> + sizeof(struct mali_c55_params_awb_config) + \
> + sizeof(struct mali_c55_params_awb_gains) + \
Some data structures are duplicated, please explain why somewhere, or it
may appear to be a bug.
> + sizeof(struct mali_c55_params_mesh_shading_config) + \
> + sizeof(struct mali_c55_params_mesh_shading_selection)
I think this will work fine now, but as soon as you add a block that
contains a 64-bit field, you'll have issues. Many CPUs expect 64-bit
accesses to be aligned. If some structures have a size that are not
multiples of 64-bit, you'll end up having unaligned blocks.
I would recommend aligning the size of each data structure to 64 bits. A
simple way to do so would be to align the size of the header to 64 bits.
Please test the result to make sure I'm right though :-)
> +
> +/**
> + * struct mali_c55_params_buffer - 3A configuration parameters
> + *
> + * This struct contains the configuration parameters of the Mali-C55 ISP
> + * algorithms, serialized by userspace into an opaque data buffer. Each
It's not opaque, you've documented it fully :-)
s/opaque //
> + * configuration parameter block is represented by a block-specific structure
> + * which contains a :c:type:`mali_c55_params_block_header` entry as first
> + * member. Userspace populates the @data buffer with configuration parameters
> + * for the blocks that it intends to configure. As a consequence, the data
> + * buffer effective size changes according to the number of ISP blocks that
> + * userspace intends to configure.
> + *
> + * The parameters buffer is versioned by the @version field to allow modifying
> + * and extending its definition. Userspace should populate the @version field to
s/should/shall/
> + * inform the driver about the version it intends to use. The driver will parse
> + * and handle the @data buffer according to the data layout specific to the
> + * indicated revision and return an error if the desired revision is not
s/revision/version/g
> + * supported.
> + *
> + * For each ISP block that userspace wants to configure, a block-specific
> + * structure is appended to the @data buffer, one after the other without gaps
> + * in between nor overlaps. Userspace shall populate the @total_size field with
> + * the effective size, in bytes, of the @data buffer.
> + *
> + * The expected memory layout of the parameters buffer is::
> + *
> + * +-------------------- struct mali_c55_params_buffer ------------------+
> + * | version = MALI_C55_PARAM_BUFFER_V0; |
> + * | total_size = sizeof(struct mali_c55_params_sensor_off_preshading) |
> + * | sizeof(struct mali_c55_params_aexp_hist); |
> + * | +------------------------- data ---------------------------------+ |
> + * | | +--------- struct mali_c55_params_sensor_off_preshading ------+ | |
> + * | | | +-------- struct mali_c55_params_block_header header -----+ | | |
> + * | | | | type = MALI_C55_PARAM_BLOCK_SENSOR_OFFS; | | | |
> + * | | | | enabled = 1; | | | |
> + * | | | | size = | | | |
> + * | | | | sizeof(struct mali_c55_params_sensor_off_preshading);| | | |
> + * | | | +---------------------------------------------------------+ | | |
> + * | | | chan00 = ...; | | |
> + * | | | chan01 = ...; | | |
> + * | | | chan10 = ...; | | |
> + * | | | chan11 = ...; | | |
> + * | | +------------ struct mali_c55_params_aexp_hist ---------------+ | |
> + * | | | +-------- struct mali_c55_params_block_header header -----+ | | |
> + * | | | | type = MALI_C55_PARAM_BLOCK_AEXP_HIST; | | | |
> + * | | | | enabled = 1; | | | |
> + * | | | | size = sizeof(struct mali_c55_params_aexp_hist); | | | |
> + * | | | +---------------------------------------------------------+ | | |
> + * | | | skip_x = ...; | | |
> + * | | | offset_x = ...; | | |
> + * | | | skip_y = ...; | | |
> + * | | | offset_y = ...; | | |
> + * | | | scale_bottom = ...; | | |
> + * | | | scale_top = ...; | | |
> + * | | | plane_mode = ...; | | |
> + * | | | tap_point = ...; | | |
> + * | | +-------------------------------------------------------------+ | |
> + * | +-----------------------------------------------------------------+ |
> + * +---------------------------------------------------------------------+
> + *
> + * @version: The Mali-C55 parameters buffer version
> + * @total_size: The Mali-C55 configuration data effective size, excluding this
> + * header
> + * @data: The Mali-C55 configuration blocks data
> + */
> +struct mali_c55_params_buffer {
> + enum mali_c55_param_buffer_version version;
> + size_t total_size;
> + __u8 data[MALI_C55_PARAMS_MAX_SIZE];
> +};
> +
> #endif /* __UAPI_MALI_C55_CONFIG_H */
--
Regards,
Laurent Pinchart
