Re: [PATCH v5 14/16] media: uapi: Add parameters structs to mali-c55-config.h

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dan,

On Fri, May 31, 2024 at 08:30:21AM +0100, Daniel Scally wrote:
> On 31/05/2024 01:09, Laurent Pinchart wrote:
> > 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 ? :-)
> 
> Hah - sure. I'll switch it to V1.
>
> >> + */
> >> +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 ?
> 
> **Need** to no, but it's convenient. I don't particularly expect the histogram configuration to be 
> changed past the first frame, but changing the metering mode for example would require a new weights 
> array and separating them avoids re-writing the config unnecessarily.

Fine with me.

> >> + * @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.
> 
> They're used by the separate algorithms yes.
> 
> >> + * @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.
> 
> Hmm...just in the documentary comment? Or something more strict?

Just in the comments. As the mapping isn't 1:1 (here are structures used
by multiple block types), documenting the mapping explicitly could avoid
confusion and mistakes.

> >> + */
> >> +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.
> 
> OK.
> 
> >> + *
> >> + * 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.
> 
> It's part of the header, so it's available to all blocks...they can all be disabled somehow though 
> the exact mechanism differs.
> 
> > 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 ?
> 
> We don't pass blocks where the values are unchanged (so only a couple go in each frame - white 
> balance and digital gain IIRC), but if we want to transition a block from enabled to disabled for 
> whatever reason the driver would need to see it

When userspace wants to disable a block, it will include the block in
the params buffer with .enabeld set to false. That's not an issue. What
I was wondering is if the rest of the block (after the header) was
needed in that case, or if we could include the header only in that
case.

> >> + * @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.
> 
> Ack
> 
> >> +};
> >> +
> >> +/**
> >> + * 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.
> 
> Err...you mean "off"? It's from the documentation - I can spell it properly though.

I mean "offsets" vs. "offsets". But now that you mention it, the
shortened "off" spelling in the structure name can be a bit confusing.

> >> + * 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.
> 
> Sure, ok.
> 
> > On a side note,
> > at some point turning that diagram into SVG may make sense.
> 
> That diagram is incomplete...it's only supposed to highlight the tap points and the blocks that 
> surround them. So we'll need to extend it to accommodate that (which probably will be easier as an SVG)

Agreed.

> >> + *
> >> + * @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 ?
> 
> Horizontally, yes. So the maximum pixel count would be (width / 2) x height.
> 
> >> +
> >> +/**
> >> + * 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.
> 
> Plane is from the documentation yes.
> 
> >> + * @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 ?
> 
> To pixels, so particular configurations of the skipping and the offsets can cause some of the colour 
> component histograms to be empty.

Does it mean that in modes other than MALI_C55_AEXP_HIST_COMBINED the
user should pick a /3, /5 or /9 downsampling ? I think it would be
useful to explain this a bit better.

> >> + */
> >> +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 ?
>
> It's effectively a boolean field to decide whether to skip the first column/row or not.

That should be mentionedThat should be mentioned..

> >> + * @scale_bottom:	scale of bottom half of range: 0=1x ,1=2x, 2=4x, 4=8x, 4=16x
> >
> > Could you elaborate on this ?
> 
> Will do
> 
> >> + * @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 ?
> 
> That's my understanding yes
> 
> > What happens if the number of zones is 0 ?
> 
> I don't know; haven't tried. I'll test it out.

I hope it wont cause the blue smoke to come out of the chip :-)

> >> + * @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 ? :-)
> 
> The gainNN comes from the register field names; I've used them for everything...does doing that 
> consistently have value? Or should I rename them?

I'm OK keeping the current names.

> >> + *
> >> + * 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.
> 
> I'll add an explanatory comment
> 
> >> +	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 //
> 
> Yeah wrong word...I can't think of a better one - I'll just remove it.
> 
> >> + * 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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux