Add device posture feature

[serhiy-katsyuba-intel]

Adds a device posture feature. The driver sends IPC4_COPIER_MODULE_CFG_PARAM_CHANNEL_MAP with a channel map to be used for swapping channels according to the device orientation.
IPC4_COPIER_MODULE_CFG_PARAM_CHANNEL_MAP is applied only for the DAI gateway.

[plbossart]

can I ask what this has to do with 'posture' in its traditional sense of extended landscape/portrait configuration?

[serhiy-katsyuba-intel]

can I ask what this has to do with 'posture' in its traditional sense of extended landscape/portrait configuration?

The PR adds support to IPC4_COPIER_MODULE_CFG_PARAM_CHANNEL_MAP IPC request. When user rotates tablet-like device by 180 degree, driver sends this IPC to gateway(s) with new channel map to switch left and right channels on speakers and microphones.

[plbossart]

Right, but this capability is usually handled in external devices - for SDCA long term I don't see anyone requesting posture changes at the Intel firmware level.

[lyakh]

should this be the "identity mapping" maybe?

[lyakh]

cannot you put those functions under the same #ifs too?

[serhiy-katsyuba-intel]

That is not that easy. For example, for remap_c16_to_c32() it would be:

#if CONFIG_PCM_CONVERTER_FORMAT_S16LE && (CONFIG_PCM_CONVERTER_FORMAT_S32LE || CONFIG_PCM_CONVERTER_FORMAT_S24_4LE || CONFIG_PCM_CONVERTER_FORMAT_S24_4LE_MSB || CONFIG_PCM_CONVERTER_FORMAT_S16_4LE)

[lyakh]

yeah, well, it's nothing critical - just thinking out loud. You could define some intermediate flags like

#define X_IS_NEEDED (CONFIG_X1 || CONFIG_X2...)

but doesn't seem too important to me, up to you basically

[lyakh]

is this because out_fmt->valid_bit_depth hasn't been set up yet or is it because it must be changed? So what we previously thought was a valid bit depth, now it's a different one? Something is a bit strange here. Your if is the same as

if (dma_buf_container_bits != out_fmt->depth || out_fmt->depth == out_fmt->valid_bit_depth)

So we can enter this branch also if dma_buf_container_bits == out_fmt->depth and then you set out_fmt->depth = dma_buf_container_bits again... Could you double-check this?

[serhiy-katsyuba-intel]

The setup of DMA buffer format and conversoin func is somewhat confusing. I wrote a big comment above this code trying to explain. The second part of the comment is about this weird logic.

DMA buffer format is set in dai-zephyr from dev->ipc_config.frame_fmt, which comes from topology. That is enum sof_ipc_frame type like SOF_IPC_FRAME_S16_LE, SOF_IPC_FRAME_S32_LE, etc. There is no way to use this value to setup DMA buffer, for example, for 16-bit valid in 32-bit container -- no such enum value. Such format is used in our CI for SSP gateway.
So before my PR we had this code to fix the problem (see line 466 in copier_dai.c in version before my PR):

/*
		 * dai_zephyr_params assigns the conversion function
		 * based on the input/output formats but does not take
		 * the valid bits into account. So change the conversion
		 * function if the valid bits are different from the
		 * container size.
		 */
		audio_stream_fmt_conversion(in_fmt->depth,
					    in_fmt->valid_bit_depth,
					    &in_bits, &in_valid_bits,
					    in_fmt->s_type);
		audio_stream_fmt_conversion(out_fmt->depth,
					    out_fmt->valid_bit_depth,
					    &out_bits, &out_valid_bits,
					    out_fmt->s_type);

		if (in_bits != in_valid_bits || out_bits != out_valid_bits)
			cd->dd[0]->process =
				cd->converter[IPC4_COPIER_GATEWAY_PIN];

The code overwrites cd->dd[0]->process previously set in dai-zephyr based on dev->ipc_config.frame_fmt and other pin format with new value based on coper input and output formats in case of if (in_bits != in_valid_bits || out_bits != out_valid_bits).
That condition looks strange to me but we lived with it for quite a sometime.

The condition in my PR (also looking very confusing) works like this:

• if dma_buf_container_bits != in_fmt->depth, then in_fmt values cannot be trusted and are ignored (format from topology takes precedence over copier's input/output formats).
• if dma_buf_container_bits == in_fmt->depth, we still prefer value from topology except for in_fmt->depth != in_fmt->valid_bit_depth. This is to handle that 16-bit valid in 32-bit container SSP case. Topology cannot specify such format, so we have to rely on copier's input/output format in this case.

Of course, it would be much simpler if we just always use copier's input/output formats -- they contain all the required info. However, on our CI we have test that actually sends different container sizes for topology format and copier input/output format. So that weird logic is required.

[lyakh]

• if dma_buf_container_bits != in_fmt->depth, then in_fmt values cannot be trusted and are ignored (format from topology takes precedence over copier's input/output formats).

• if dma_buf_container_bits == in_fmt->depth, we still prefer value from topology except for in_fmt->depth != in_fmt->valid_bit_depth. This is to handle that 16-bit valid in 32-bit container SSP case. Topology cannot specify such format, so we have to rely on copier's input/output format in this case.

Right, this logic fits well the format:

if (dma_buf_container_bits != in_fmt->depth || in_fmt->depth == in_fmt->valid_bit_depth)

which is equivalent to yours, but has 1 operation fewer and is therefore easier to read IMHO.

[lgirdwood]

Looks good after Makefile open addressed.

[serhiy-katsyuba-intel]

Rebased to fix merge conflict.