[BUG][IPC4] wrong 24bit variant selected

IPC4 interface allows to specify two variants of 24bit sample encoding, SOF_IPC4_MSB_INTEGER and SOF_IPC4_LSB_INTEGER.

Currently driver does not set this field explicitly, so it is set as 0 (MSB_INTEGER). This is however not correct as

valid_bits=24, bit_depth=32, MSB_INTEGER -> SNDRV_PCM_FMTBIT_S32_LE (msbits=24)
valid_bits=24 ,bit_depth=32, LSB_INTEGER -> SNDRV_PCM_FMTBIT_S24_LE
For reference, the third variant in ALSA for 24bti sampoles
valid_bits=24, bit_depth=24, SNDRV_PCM_FMTBIT_S24_3LE

The documentation is also wrong in kernel header.h

This has not been noticed as currnet SOF FW for IPC4 behaves as SOF IPC3 used to behave (like LSB_INTEGER=1), but this needs to be fixed.

what is wrong in the kernel header.h

enum sof_ipc4_sample_type {
	SOF_IPC4_MSB_INTEGER, /* integer with Most Significant Byte first */
	SOF_IPC4_LSB_INTEGER, /* integer with Least Significant Byte first */
};

that's the only reference to the MSB/LSB.

@plbossart

@plbossart wrote:

what is wrong in the kernel header.h
enum sof_ipc4_sample_type {
	SOF_IPC4_MSB_INTEGER, /* integer with Most Significant Byte first */
	SOF_IPC4_LSB_INTEGER, /* integer with Least Significant Byte first */
};
that's the only reference to the MSB/LSB.

It's the doxygen comment "integer with Most/Least Significant Byte first". This does not match the semantic of this field, or at least is very, very misleading as this hints that this field controls the byte-order of the sample word. In the IPC4 reference, this field is NOT about byte order at all. E.g. if LSB_INTEGER is set as sample_type, 24bit samples are expected to be encoded like in ALSA SNDRV_PCM_FMTBIT_S24_LE, so 0x00aabbcc (byte-order is little-endian always, so 0xcc is first in memory). If MSB_INTEGER is set, 24bit samples are expected to be in ALSA SNDDRV_PCM_FMTBIT_S32_LE encoding, so 0xaabbcc00. Now the first byte is 00 (as it's still little-endian) so documentation is definitely wrong.

@serhiy-katsyuba-intel to double-check what I write above.

So currently driver incorrectly sets MSB_INTEGER as type when it wants the DSP to consume/produce SNDRV_PCM_FMTBIT_S24_LE . This is the case now as this was the interface in IPC3, and the current SOF mainline actually has a mirror bug so actual audio data is consumed/produced correctly currently. But I think we have to fix it as otherwise a FW client that uses the interface correctly, will get incorrectly consumed/produced 24bit audio data.

@kv2019i

Ah yes, it's the comment that's wrong, this should read "MSB aligned" or 'LSB aligned" or something to this effect. thanks for the clarification @kv2019i

Let's wait a bit on the kernel side change. There is still some opens w.r.t. the FW side implementation we need to cover first in thesofproject/sof#7697 .

@RanderWang

This is trending to be an invalid bug (thesofproject/sof#7697). The host should set MSB_INTEGER for all current 16/24/32bit integer cases and if valid bit count is smaller than sample containter bit count, bits are packed to least signifcant bits (which is misleading as we set MSB_INTEGER, but that's how it is in all drivers and tests we've surveyed so far with @RanderWang ). Still not sure what to write in the kernel header documentation....

yes, I checked all the drivers and don't find any driver set LSB_INTEGER and use MSB_INTEGER by default. I have an idea that: Currently according to HDAduio spec, MSB is used, so it is ok for all intel drivers to use MSB. Although we don't find the usage case to set LSB in driver, we still need to process it in cSOF to completely support IPC4. One windows validation test set LSB and check the result.

@kv2019i

@kv2019i I can't recall what the conclusion is for this?

@plbossart

@plbossart No final conclusion. Rander is now going piecemeal addressing the issue (like in thesofproject/sof#7959) and trying to keep us compatible with reference firmware behaviour. For now, host can't really use this field and it just needs to be set as MSB_INTEGER. The FW will in practise ignore this and override this based on DAI type, based on DAI properties.

@plbossart

@plbossart @kv2019i When I am working on the real code: thesofproject/sof#7959, I have a deeper understanding about MSB & LSB when I get some issues with this PR. Now MSB or LSB type only affects 24 bits valid sample / 32 bit container (ignore other type like 16/32, we can support it later).

Currently Windows driver & AVS driver use the default MSB type ( valued 0) and don't set it to LSB type, so does our SOF kernel driver. I find some issues:
(1) We can deal with MSB type with DAI type or DAI properties in FW, but it is only for gateway copier. We can't do it for other modules. And the code is not elegant after reviewing it a few times.

(2) Most modules like volume, gain works with S24_LE (LSB type), but now driver use default MSB type, so it is inconsistent. We don't have trouble with them since these modules are not use the sample type now and still go with the old IPC3 path. This behavior may be change in future.

(3) We found some windows test cases use different 24/32 format compared with Linux that they use MSB but Linux uses LSB (S24_LE although Linux also set MSB sample type). FW has trouble to deal with this case if both Windows & Linux use default MSB type.

I have an idea that SOF should set sample type based on Linux audio and no need to follow other drivers. We don't need to change Linux kernel driver since the sample type is defined by topology.
(1) Topology set sample type to LSB for most modules except gateway copier for DAI

        in_sample_type          $SAMPLE_TYPE_MSB_INTEGER
        out_rate                48000
        out_bit_depth           16
        out_valid_bit_depth     16
        out_channels            2
        out_interleaving_style  "interleaved"
        out_sample_type        SAMPLE_TYPE_MSB_INTEGER

(2) FW can check the sample type without any special process for module type. This also makes FW to deal with windows & Linux easier

Thanks for suggestion!

@kv2019i

@kv2019i @RanderWang ping - FW parts merged.

@kv2019i

@kv2019i I will close it on Linux side since we only need to change topology, not the Linux kernel. The PR was merged.

@RanderWang

Ack @RanderWang , the fixes are in FW side (and topology).

I do think the misleading comments in the kernel header for LSB/MSB should be fixed, but that's not really th emain topic of this bug, can be done outside this bug.

FYI to @crojewsk-intel @amadeuszslawinski-intel who are addressing this for avs upstream (and improving msbit support in the app interface) -> #4539 (plus sent upstream)

Navigation Menu

Repository navigation

[BUG][IPC4] wrong 24bit variant selected #4383

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[BUG][IPC4] wrong 24bit variant selected #4383

Description

Activity

plbossart commented on May 26, 2023

kv2019i commented on May 30, 2023

plbossart commented on May 30, 2023

kv2019i commented on May 30, 2023

kv2019i commented on Jun 29, 2023

RanderWang commented on Jun 30, 2023

plbossart commented on Aug 14, 2023

kv2019i commented on Aug 15, 2023

RanderWang commented on Aug 17, 2023

lgirdwood commented on Aug 22, 2023

RanderWang commented on Aug 23, 2023

kv2019i commented on Aug 23, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions