| Message ID | 1410332428-16571-1-git-send-email-gtmkramer@xs4all.nl (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Takashi Iwai |
| Headers | show |
Hi,
Sorry for catching up late on this patch...
On 10.09.2014 10:00, Jurgen Kramer wrote:
> + FORMATD(DSD_U32_LE, "Direct Stream Digital, 4-byte (x32), little endian, oldest bits in MSB"),
There is bug in this patch. The spec for these formats is, as above says
"little endian, oldest bits in MSB", however with the iFi iDSD Nano this
doesn't seem to be the case. Oldest bits are in MSB of the byte, but
byte order is big endian. IOW, following alone will produce incorrect
output:
size_t N = 32;
uint32_t x = 0;
while (N--)
{
x <<= 1;
x |= nextbit;
}
But if you add __builtin_byteswap32(x) then the output is correct.
I guess this has not been caught because of just memcpy()'ing the
oldest-bit-in-MSB byte stream to the sample buffer instead of
constructing the words...
Best regards,
- Jussi
At Wed, 19 Nov 2014 21:50:30 +0200, Jussi Laako wrote: > > Hi, > > Sorry for catching up late on this patch... > > On 10.09.2014 10:00, Jurgen Kramer wrote: > > + FORMATD(DSD_U32_LE, "Direct Stream Digital, 4-byte (x32), little endian, oldest bits in MSB"), > > There is bug in this patch. The spec for these formats is, as above says > "little endian, oldest bits in MSB", however with the iFi iDSD Nano this > doesn't seem to be the case. Oldest bits are in MSB of the byte, but > byte order is big endian. Is it a bug of spec, or a bug of device? In the latter case, we'd need to introduce DSD_U32_BE format and apply a quirk, for example. Takashi > IOW, following alone will produce incorrect > output: > > size_t N = 32; > uint32_t x = 0; > while (N--) > { > x <<= 1; > x |= nextbit; > } > > But if you add __builtin_byteswap32(x) then the output is correct. > > I guess this has not been caught because of just memcpy()'ing the > oldest-bit-in-MSB byte stream to the sample buffer instead of > constructing the words... > > > Best regards, > > - Jussi > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
On Thu, 2014-11-20 at 10:58 +0100, Takashi Iwai wrote: > At Wed, 19 Nov 2014 21:50:30 +0200, > Jussi Laako wrote: > > > > Hi, > > > > Sorry for catching up late on this patch... > > > > On 10.09.2014 10:00, Jurgen Kramer wrote: > > > + FORMATD(DSD_U32_LE, "Direct Stream Digital, 4-byte (x32), little endian, oldest bits in MSB"), > > > > There is bug in this patch. The spec for these formats is, as above says > > "little endian, oldest bits in MSB", however with the iFi iDSD Nano this > > doesn't seem to be the case. Oldest bits are in MSB of the byte, but > > byte order is big endian. > > Is it a bug of spec, or a bug of device? > In the latter case, we'd need to introduce DSD_U32_BE format and apply > a quirk, for example. DSD is a bit odd wrt LE/BE. Both formats are allowed and used. The device (XMOS based/Marantz+Denon) want the following order of samples: L1 L2 L3 L4 R1 R2 R3 R4. Where Lx/Rx are 8-bit DSD samples, L1-L4 are packed to 1 32-bit sample. I do think real BE will not work. Jurgen > > Takashi > > > IOW, following alone will produce incorrect > > output: > > > > size_t N = 32; > > uint32_t x = 0; > > while (N--) > > { > > x <<= 1; > > x |= nextbit; > > } > > > > But if you add __builtin_byteswap32(x) then the output is correct. > > > > I guess this has not been caught because of just memcpy()'ing the > > oldest-bit-in-MSB byte stream to the sample buffer instead of > > constructing the words... > > > > > > Best regards, > > > > - Jussi > > _______________________________________________ > > Alsa-devel mailing list > > Alsa-devel@alsa-project.org > > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel > > > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
On 20.11.2014 11:58, Takashi Iwai wrote: >> On 10.09.2014 10:00, Jurgen Kramer wrote: >>> + FORMATD(DSD_U32_LE, "Direct Stream Digital, 4-byte (x32), little endian, oldest bits in MSB"), >> >> There is bug in this patch. The spec for these formats is, as above says >> "little endian, oldest bits in MSB", however with the iFi iDSD Nano this >> doesn't seem to be the case. Oldest bits are in MSB of the byte, but >> byte order is big endian. > > Is it a bug of spec, or a bug of device? > In the latter case, we'd need to introduce DSD_U32_BE format and apply > a quirk, for example. The device should really be flagged as DSD_U32_BE instead. So I would propose to add DSD_U16_BE and DSD_U32_BE formats. The DSD_U16_BE just to make the format list look tidy, so that when eventually it is needed, the formats are not in random order... - Jussi
On 20.11.2014 18:34, Jurgen Kramer wrote: > DSD is a bit odd wrt LE/BE. Both formats are allowed and used. > The device (XMOS based/Marantz+Denon) want the following order of > samples: > L1 L2 L3 L4 R1 R2 R3 R4. Where Lx/Rx are 8-bit DSD samples, L1-L4 are > packed to 1 32-bit sample. What you described is precisely interleaved big endian U32 sample format. L1 is the MSB byte and L4 is the LSB byte of big-endian U32. So "L1 L2 L3 L4" is equal to DSD_U32_BE. The currently advertised DSD_U32_LE would be "L4 L3 L2 L1". > I do think real BE will not work. Why? Your description precisely matches U32_BE. And seems to work correctly for me... Constructing the U32 by having oldest bit in MSB and newest bit in LSB and sending it out as little-endian results in incorrect output. Adding byteswap to big-endian format results in correct output (either using __builtin_byteswap32() or htonl()). - Jussi
Hi, I sent out two patches that make the XMOS implementation work correctly for me. While doing the change, I spotted another bug in snd_pcm_format_little_endian() where it was claiming the _LE formats to be big-endian ones. I fixed this too. Having this bug may have accidentally made some software work correctly if they do byte reversal based on snd_pcm_format_little_endian() instead of the sample format type... Best regards, - Jussi
At Fri, 21 Nov 2014 11:11:22 +0200, Jussi Laako wrote: > > Hi, > > I sent out two patches that make the XMOS implementation work correctly > for me. I haven't seen them yet. Could you put Cc to me? Takashi
diff --git a/include/pcm.h b/include/pcm.h index 11e9f0d..db88ad5 100644 --- a/include/pcm.h +++ b/include/pcm.h @@ -211,7 +211,9 @@ typedef enum _snd_pcm_format { SND_PCM_FORMAT_DSD_U8, /* Direct Stream Digital (DSD) in 2-byte samples (x16) */ SND_PCM_FORMAT_DSD_U16_LE, - SND_PCM_FORMAT_LAST = SND_PCM_FORMAT_DSD_U16_LE, + /* Direct Stream Digital (DSD) in 4-byte samples (x32) */ + SND_PCM_FORMAT_DSD_U32_LE, + SND_PCM_FORMAT_LAST = SND_PCM_FORMAT_DSD_U32_LE, #if __BYTE_ORDER == __LITTLE_ENDIAN /** Signed 16 bit CPU endian */ diff --git a/include/sound/asound.h b/include/sound/asound.h index 32168f7..6ee5867 100644 --- a/include/sound/asound.h +++ b/include/sound/asound.h @@ -219,7 +219,8 @@ typedef int __bitwise snd_pcm_format_t; #define SNDRV_PCM_FORMAT_G723_40_1B ((__force snd_pcm_format_t) 47) /* 1 sample in 1 byte */ #define SNDRV_PCM_FORMAT_DSD_U8 ((__force snd_pcm_format_t) 48) /* DSD, 1-byte samples DSD (x8) */ #define SNDRV_PCM_FORMAT_DSD_U16_LE ((__force snd_pcm_format_t) 49) /* DSD, 2-byte samples DSD (x16), little endian */ -#define SNDRV_PCM_FORMAT_LAST SNDRV_PCM_FORMAT_DSD_U16_LE +#define SNDRV_PCM_FORMAT_DSD_U32_LE ((__force snd_pcm_format_t) 50) /* DSD, 4-byte samples DSD (x32), little endian */ +#define SNDRV_PCM_FORMAT_LAST SNDRV_PCM_FORMAT_DSD_U32_LE #ifdef SNDRV_LITTLE_ENDIAN #define SNDRV_PCM_FORMAT_S16 SNDRV_PCM_FORMAT_S16_LE diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c index 1399a5b..2e24338 100644 --- a/src/pcm/pcm.c +++ b/src/pcm/pcm.c @@ -1565,6 +1565,7 @@ static const char *const snd_pcm_format_names[] = { FORMAT(G723_40_1B), FORMAT(DSD_U8), FORMAT(DSD_U16_LE), + FORMAT(DSD_U32_LE), }; static const char *const snd_pcm_format_aliases[SND_PCM_FORMAT_LAST+1] = { @@ -1624,6 +1625,7 @@ static const char *const snd_pcm_format_descriptions[] = { FORMATD(G723_40_1B, "G.723 (ADPCM) 40 kbit/s, 1 sample in 1 byte"), FORMATD(DSD_U8, "Direct Stream Digital, 1-byte (x8), oldest bit in MSB"), FORMATD(DSD_U16_LE, "Direct Stream Digital, 2-byte (x16), little endian, oldest bits in MSB"), + FORMATD(DSD_U32_LE, "Direct Stream Digital, 4-byte (x32), little endian, oldest bits in MSB"), }; static const char *const snd_pcm_type_names[] = { diff --git a/src/pcm/pcm_misc.c b/src/pcm/pcm_misc.c index 46fc771..9272179 100644 --- a/src/pcm/pcm_misc.c +++ b/src/pcm/pcm_misc.c @@ -64,6 +64,7 @@ int snd_pcm_format_signed(snd_pcm_format_t format) case SNDRV_PCM_FORMAT_U18_3BE: case SNDRV_PCM_FORMAT_DSD_U8: case SNDRV_PCM_FORMAT_DSD_U16_LE: + case SNDRV_PCM_FORMAT_DSD_U32_LE: return 0; default: return -EINVAL; @@ -154,6 +155,7 @@ int snd_pcm_format_little_endian(snd_pcm_format_t format) case SNDRV_PCM_FORMAT_U18_3BE: case SNDRV_PCM_FORMAT_DSD_U8: case SNDRV_PCM_FORMAT_DSD_U16_LE: + case SNDRV_PCM_FORMAT_DSD_U32_LE: return 0; default: return -EINVAL; @@ -232,6 +234,7 @@ int snd_pcm_format_width(snd_pcm_format_t format) case SNDRV_PCM_FORMAT_U32_BE: case SNDRV_PCM_FORMAT_FLOAT_LE: case SNDRV_PCM_FORMAT_FLOAT_BE: + case SNDRV_PCM_FORMAT_DSD_U32_LE: return 32; case SNDRV_PCM_FORMAT_FLOAT64_LE: case SNDRV_PCM_FORMAT_FLOAT64_BE: @@ -292,6 +295,7 @@ int snd_pcm_format_physical_width(snd_pcm_format_t format) case SNDRV_PCM_FORMAT_FLOAT_BE: case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE: case SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE: + case SNDRV_PCM_FORMAT_DSD_U32_LE: return 32; case SNDRV_PCM_FORMAT_FLOAT64_LE: case SNDRV_PCM_FORMAT_FLOAT64_BE: @@ -348,6 +352,7 @@ ssize_t snd_pcm_format_size(snd_pcm_format_t format, size_t samples) case SNDRV_PCM_FORMAT_U32_BE: case SNDRV_PCM_FORMAT_FLOAT_LE: case SNDRV_PCM_FORMAT_FLOAT_BE: + case SNDRV_PCM_FORMAT_DSD_U32_LE: return samples * 4; case SNDRV_PCM_FORMAT_FLOAT64_LE: case SNDRV_PCM_FORMAT_FLOAT64_BE: @@ -394,6 +399,7 @@ u_int64_t snd_pcm_format_silence_64(snd_pcm_format_t format) return 0x8080808080808080ULL; case SNDRV_PCM_FORMAT_DSD_U8: case SNDRV_PCM_FORMAT_DSD_U16_LE: + case SNDRV_PCM_FORMAT_DSD_U32_LE: return 0x6969696969696969ULL; #ifdef SNDRV_LITTLE_ENDIAN case SNDRV_PCM_FORMAT_U16_LE:
Add the new DSD_U32_LE sample format to alsa-lib. NB include/pcm.h and include/sound/asound.h are updated so a new sync with the kernel headers is not needed Signed-off-by: Jurgen Kramer <gtmkramer@xs4all.nl> --- include/pcm.h | 4 +++- include/sound/asound.h | 3 ++- src/pcm/pcm.c | 2 ++ src/pcm/pcm_misc.c | 6 ++++++ 4 files changed, 13 insertions(+), 2 deletions(-)