| Message ID | 1426435269-17059-12-git-send-email-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 16/03/15 03:01, Takashi Sakamoto wrote: > Digi 002/003 family uses own way to multiplex PCM samples into data > blocks in CIP payload for received stream, thus AMDTP-conformant > implementation causes noisy sound. > > This commit applies double-oh-three algorism, which discovered by Robin > Gareus and Damien Zammit in 2012. > > As long as I tested, this patch still has disorder that: > * 1st PCM channel still causes noise in 2nd PCM channel. > * At 88.2/96.0kHz, any PCM channels still causes noise in the other PCM > channels. Can you please double check, I dont think snd_dg00x_protocol_write_s32() is even being called because amdtp.c needs a small change to prevent overriding the transmit_samples function pointer. Damien
Hi Damien, On Mar 16 2015 20:39, Damien Zammit wrote: > On 16/03/15 03:01, Takashi Sakamoto wrote: >> Digi 002/003 family uses own way to multiplex PCM samples into data >> blocks in CIP payload for received stream, thus AMDTP-conformant >> implementation causes noisy sound. >> >> This commit applies double-oh-three algorism, which discovered by Robin >> Gareus and Damien Zammit in 2012. >> >> As long as I tested, this patch still has disorder that: >> * 1st PCM channel still causes noise in 2nd PCM channel. >> * At 88.2/96.0kHz, any PCM channels still causes noise in the other PCM >> channels. > > Can you please double check, I dont think snd_dg00x_protocol_write_s32() > is even being called because amdtp.c needs a small change to prevent > overriding the transmit_samples function pointer. This line overwrites the default callback function with driver-specific function every time to start streams, thus the driver-specific function is used for out-stream. On Mar 16 2015 01:01, Takashi Sakamoto wrote: > diff --git a/sound/firewire/digi00x/digi00x-stream.c b/sound/firewire/digi00x/digi00x-stream.c > index 410b971..bc4c88c 100644 > --- a/sound/firewire/digi00x/digi00x-stream.c > +++ b/sound/firewire/digi00x/digi00x-stream.c > @@ -204,6 +204,8 @@ static int keep_resources(struct snd_dg00x *dg00x, unsigned int rate) > dg00x->rx_stream.midi_position = 0; > dg00x->tx_stream.midi_position = 0; > > + /* Apply doubleOhThree algorism. */ > + dg00x->rx_stream.transfer_samples = snd_dg00x_protocol_write_s32; > dg00x->rx_stream.transfer_midi = snd_dg00x_protocol_fill_midi; > dg00x->tx_stream.transfer_midi = snd_dg00x_protocol_pull_midi; Regards Takashi Sakamoto
Hi Takashi, Many thanks for doing this! On 03/15/2015 05:01 PM, Takashi Sakamoto wrote: > + > +void snd_dg00x_protocol_write_s32(struct amdtp_stream *s, > + struct snd_pcm_substream *pcm, > + __be32 *buffer, unsigned int frames) > +{ > + struct snd_pcm_runtime *runtime = pcm->runtime; > + unsigned int channels, remaining_frames, i, c; > + const u32 *src; > + static struct dot_state state; There's no need to declare this as static. The state is per frame. > + channels = s->pcm_channels; > + src = (void *)runtime->dma_area + > + frames_to_bytes(runtime, s->pcm_buffer_pointer); > + remaining_frames = runtime->buffer_size - s->pcm_buffer_pointer; > + > + for (i = 0; i < frames; ++i) { ..it is zeroed here, anyway: > + dot_state_reset(&state); > + for (c = 0; c < channels; ++c) { > + buffer[s->pcm_positions[c]] = > + cpu_to_be32((*src >> 8) | 0x40000000); > + dot_encode_step(&state, &buffer[s->pcm_positions[c]]); > + src++; > + } > + > + buffer += s->data_block_quadlets; > + if (--remaining_frames == 0) > + src = (void *)runtime->dma_area; > + } > +} In any case, the algorithm to xor-encode the digidesign data is not yet 100% correct there. One will need to continue iterating after the last channel (feeding zero) until the state->carry (dot_scrt()) is 0x00. The current code here only works only correctly for data on the first 4 chanels (18 [total channels] - 14 [max xor-chain length]). I'll let Damien elaborate. He has access to a Digidesign interface and did the protocol-dumps. I only did the clean room reverse-engineering and maths on the other end. Cheers! robin
Hi Robin, Thanks for your comment. I have a necessity to understand your algorithm for better implementation. On Mar 16 2015 23:25, Robin Gareus wrote: >> +void snd_dg00x_protocol_write_s32(struct amdtp_stream *s, >> + struct snd_pcm_substream *pcm, >> + __be32 *buffer, unsigned int frames) >> +{ >> + struct snd_pcm_runtime *runtime = pcm->runtime; >> + unsigned int channels, remaining_frames, i, c; >> + const u32 *src; >> + static struct dot_state state; > > There's no need to declare this as static. The state is per frame. > >> + channels = s->pcm_channels; >> + src = (void *)runtime->dma_area + >> + frames_to_bytes(runtime, s->pcm_buffer_pointer); >> + remaining_frames = runtime->buffer_size - s->pcm_buffer_pointer; >> + >> + for (i = 0; i < frames; ++i) { > > ..it is zeroed here, anyway: > >> + dot_state_reset(&state); Here, I'm consider about the usage of kernel stack. But for this purpose it may be better to use the stack because it's never nested calls. >> + for (c = 0; c < channels; ++c) { >> + buffer[s->pcm_positions[c]] = >> + cpu_to_be32((*src >> 8) | 0x40000000); >> + dot_encode_step(&state, &buffer[s->pcm_positions[c]]); >> + src++; >> + } >> + >> + buffer += s->data_block_quadlets; >> + if (--remaining_frames == 0) >> + src = (void *)runtime->dma_area; >> + } >> +} > > > In any case, the algorithm to xor-encode the digidesign data is not yet > 100% correct there. One will need to continue iterating after the last > channel (feeding zero) until the state->carry (dot_scrt()) is 0x00. > > The current code here only works only correctly for data on the first 4 > chanels (18 [total channels] - 14 [max xor-chain length]). Hm. Actually, I can hear better sounds in 1/2 ch as long as I aplayback to the first 4 ch. When 5 or later channels get PCM samples, I can hear noisy sound in the 1 ch (maybe 2 or more). This is an sample of CIP packet which Windows driver transmit to Digi 002 Rack, at 96.0 kHz. The first line shows CIP header. One of the other line shows one data block. I put 24 bit PCM samples into 7th Multi Bit Linear Audio (MBLA) data channel (8th data channel). The other channels includes zero samples except for MIDI conformant data channel (first data channel). 020B0090 9004E400 80000000 40005100 40003F00 40000000 40000000 40000000 40000000 4011C5E4 4000DB00 4000E400 40001C00 80000000 40002300 4000BD00 4000A200 40000E00 40006100 4000FF00 40116AFC 4000F500 40008B00 40007400 80000000 40005C00 40003300 40004D00 4000C200 4000DE00 4000E100 4011DDE1 4000E200 40001E00 40002100 80000000 4000BF00 40000000 40000000 40000000 40000000 40000000 4012DF19 40000000 40000000 40000000 80000000 40000000 40000000 40000000 40000000 40000000 40000000 40146531 4000FB00 40008400 40007C00 80000000 40005300 40003D00 40004200 4000CE00 4000D100 4000EF00 4015B003 40000000 40000000 40000000 80000000 40000000 40000000 40000000 40000000 40000000 40000000 40162CFB 4000B300 4000AD00 40000200 80000000 40006E00 4000F100 40008F00 40000000 40000000 40000000 4015C4F8 4000DC00 4000E300 40001D00 80000000 40002200 4000BE00 4000A100 40000F00 40000000 40000000 4014EC69 40001300 40002D00 4000B200 80000000 4000AE00 40000100 40006F00 40000000 40000000 40000000 40143EE8 40004100 4000CF00 40000000 80000000 40000000 40000000 40000000 40000000 40000000 40000000 401408D0 40006700 4000F900 40008600 80000000 40007A00 40005500 40003B00 40004400 4000CC00 4000D300 4014C0B6 40000000 40000000 40000000 80000000 40000000 40000000 40000000 40000000 40000000 40000000 40146A74 4000F500 40008B00 40007400 80000000 40005C00 40003300 40004D00 4000C200 4000DE00 4000E100 40148837 40007700 40005900 40003600 80000000 40004A00 4000C500 4000DB00 4000E400 40001C00 40002300 401414CA 40002C00 4000B300 4000AD00 80000000 40000200 40006E00 4000F100 40008F00 40000000 40000000 401493B0 40009D00 40009200 40009E00 We can see the data in 7th MBLA data channel influences data in next data block (data block is represented as 'frame' in driver code). The pattern is what you discovered. In my understanding, this is the lack of my implementation. Do you mean this issue? Thanks Takashi Sakamoto
On 03/16/2015 05:25 PM, Takashi Sakamoto wrote: [..] > > We can see the data in 7th MBLA data channel influences data in next > data block (data block is represented as 'frame' in driver code). The > pattern is what you discovered. In my understanding, this is the lack of > my implementation. > > Do you mean this issue? > yes, precisely. Though from the information Damien sent me it looked like it wraps around in the current frame, rather then progress to the next.. Anyway, in this case the original code at https://github.com/x42/003amdtp is also wrong and the driver will have to allocate space for the state and retain it for subsequent calls. Using a static on the stack won't work in case someone has multiple of those devices. best, robin
Hi Robin, On May 17 2015 02:13, Robin Gareus wrote: > On 03/16/2015 05:25 PM, Takashi Sakamoto wrote: > [..] >> >> We can see the data in 7th MBLA data channel influences data in next >> data block (data block is represented as 'frame' in driver code). The >> pattern is what you discovered. In my understanding, this is the lack of >> my implementation. >> >> Do you mean this issue? >> > > yes, precisely. OK. Thanks for your confirmation. > Though from the information Damien sent me it looked like it wraps > around in the current frame, rather then progress to the next.. > > Anyway, in this case the original code at > https://github.com/x42/003amdtp is also wrong and the driver will have > to allocate space for the state and retain it for subsequent calls. This idea may solve both issues that 'per-data block' and 'per-packet'. I describe the detail later. > Using a static on the stack won't work in case someone has multiple of > those devices. Oops, exactly. I forgot this case... Thanks. This is another sample, with the last four data blocks in a CIP and the first four data blocks in next CIP. You can see the continuous value in data-block-counter (dbc) field in each CIP header. 020B00F0 9004D000 ... 80000000 4000DA00 4000E500 40001B00 40002400 4000BC00 4000A300 40F04E1C 4000C100 4000DF00 40000000 80000000 40000000 40000000 40000000 40000000 40000000 40000000 40F0530D 40003D00 40004200 4000CE00 80000000 4000D100 4000EF00 40000000 40000000 40000000 40000000 40F0831C 40007D00 40005200 40003E00 80000000 40004100 4000CF00 40000000 40000000 40000000 40000000 40F0E477 40001C00 40002300 4000BD00 020B0000 9004E400 80000000 4000A200 40000E00 40006100 4000FF00 40000000 40000000 40F15FC1 40000000 40000000 40000000 80000000 40000000 40000000 40000000 40000000 40000000 40000000 40F1C387 4000DD00 4000E200 40001E00 80000000 40002100 4000BF00 40000000 40000000 40000000 40000000 40F1DDE5 4000E200 40001E00 40002100 80000000 4000BF00 40000000 40000000 40000000 40000000 40000000 40F19742 40009900 40009600 40009A00 ... We can see the pattern is carried to the data block in next packet. But current implementation is not good for this case. Regards Takashi Sakamoto
On 03/16/2015 11:47 PM, Takashi Sakamoto wrote: Hi Takashi, > This is another sample, with the last four data blocks in a CIP and the > first four data blocks in next CIP. You can see the continuous value in > data-block-counter (dbc) field in each CIP header. > [snip] > We can see the pattern is carried to the data block in next packet. But > current implementation is not good for this case. Indeed, thanks for the heads up. I've just updated https://github.com/x42/003amdtp/ accordingly. You'll have to retain the state and reset it on open/re-start but that's part of driver integration. Cheers! robin
Hi Takashi, others, Can we include my mixer control into the driver? I think it is important to include the clock source selector switch as an alsamixer setting. Every other serious soundcard allows clock source selection. I tested 003R+ with Robin's suggestion of hacking the state to be statically initialised to zero (which won't work for multi devices), and the results were great: dead silence on channel 1 when playing into channel 18. This small hack means that Robin's update to 003amdtp should work flawlessly when integrated into the driver properly. Takashi: I am also reporting that ADAT sync, SPDIF sync both work using my clock source selector control. In fact, without the sync to external device, there are many dropouts in the 1394 streams. How can we address this issue? Regards, Damien
On 2015?03?17? 22:49, Damien Zammit wrote: > Can we include my mixer control into the driver? Depends on the reason. > I think it is important to include the clock source selector switch as > an alsamixer setting. Please explain the importance, especially the reason to include the selector into kernel code instead of userspace application. > Every other serious soundcard allows clock source selection. Over generalization. Some ALSA drivers still expect userspace applications to implement this functionality. > I tested 003R+ with Robin's suggestion of hacking the state to be > statically initialised to zero (which won't work for multi devices), > and the results were great: dead silence on channel 1 when playing into > channel 18. This small hack means that Robin's update to 003amdtp > should work flawlessly when integrated into the driver properly. What we need is to test Robin's new code thoroughly, not include extra functionality such as clock source selector unrelated to streaming functionality. > Takashi: I am also reporting that ADAT sync, SPDIF sync both work using > my clock source selector control. In fact, without the sync to external > device, there are many dropouts in the 1394 streams. > How can we address this issue? In this case, the kernel driver should return error code to userspace application. If the driver cannot handle any in-packets after starting transmitted stream, it detects timeout (500msec) and return -ETIMEDOUT. If the in-packets include discontinuity of the value in dbc field of CIP header, ALSA AMDTP implementation detects it and stop transmitted stream, then the driver also returns -ETIMEDOUT. Regards Takashi Sakamoto
On 18/03/15 12:06, Takashi Sakamoto wrote: > On 2015?03?17? 22:49, Damien Zammit wrote: >> Can we include my mixer control into the driver? > > Depends on the reason. The reason for allowing the mixer clock source control in the kernel is because it is *core* functionality to be able to decide where the sound card gets its clock from. It would be cumbersome and awkward if users had to fire up an external userspace program just to configure the clock source! The only reason some sound cards have external userspace programs to configure these extras is because no one has bothered to implement them in the kernel yet. (Well that is my opinion). >> I tested 003R+ with Robin's suggestion of hacking the state to be >> statically initialised to zero (which won't work for multi devices), >> and the results were great: dead silence on channel 1 when playing into >> channel 18. This small hack means that Robin's update to 003amdtp >> should work flawlessly when integrated into the driver properly. > > What we need is to test Robin's new code thoroughly, not include extra > functionality such as clock source selector unrelated to streaming > functionality. Takashi, I agree that my clock source mixer control is a separate issue, but please don't underestimate the work Robin and I have done with 003amdtp: We are informing you that it now works correctly for all channels, so long as it is integrated into the driver correctly. We know this because we tested it thoroughly with a bus analyser to verify that the expected patterns were being played into the device and I used my ears to listen to the pure silence on the other channels when the magic bit pattern was being played into the device, as well as using a program to monitor the levels (zero). >> Takashi: I am also reporting that ADAT sync, SPDIF sync both work using >> my clock source selector control. In fact, without the sync to external >> device, there are many dropouts in the 1394 streams. >> How can we address this issue? > In this case, the kernel driver should return error code to userspace > application. If the driver cannot handle any in-packets after starting > transmitted stream, it detects timeout (500msec) and return -ETIMEDOUT. I would not have been able to notice the difference in dropouts without my mixer control enabled, so I think from a development point of view it is useful to include the mixer control in-kernel. (another reason). > If the in-packets include discontinuity of the value in dbc field of CIP > header, ALSA AMDTP implementation detects it and stop transmitted > stream, then the driver also returns -ETIMEDOUT. Takashi, you have provided here the conditions for which -ETIMEDOUT occurs. I am asking how can we prevent the dropouts, not how it occurs. Perhaps the dbc field is irrelevant for 00x ? I don't know, please let me know if there is anything I can dump for you to help find out. Damien
On 03/18/2015 02:06 AM, Takashi Sakamoto wrote: > > > On 2015?03?17? 22:49, Damien Zammit wrote: >> Can we include my mixer control into the driver? > > Depends on the reason. > >> I think it is important to include the clock source selector switch as >> an alsamixer setting. > > Please explain the importance, especially the reason to include the > selector into kernel code instead of userspace application. Can one access the device from userspace while the kernel is using it to stream audio or midi ("Device or resource busy") ? This is one of the reasons, why USB soundcards expose all settings though the alsa mixer interface. It also offers a standardized interface for userspace apps to build on top and a method to directly link mixer to soundcard for systems with multiple soundcards. How do you envisage to handle this? 2c, robin
Robin and Damien, On Mar 19 2015 22:59, Robin Gareus wrote: > On 03/18/2015 02:06 AM, Takashi Sakamoto wrote: >> Please explain the importance, especially the reason to include the >> selector into kernel code instead of userspace application. > > Can one access the device from userspace while the kernel is using it to > stream audio or midi ("Device or resource busy") ? I can't understand what character devices you mention about. If you mention about ALSA PCM character devices (usually /dev/snd/pcmC%uD%d[p|c]), it's yes. In fact, one ALSA application can use ALSA PCM character device for one direction (playback/capture). So the access by any other applications causes -EBUSY. (here, I ignore alsa-lib's PCM direct mixing layer.) But, when you utilize kernel control functionality, the character device is not PCM one, it's Control one (/dev/snd/controlC%u). By the way, we can program any userspace application via FireWire character device (/dev/fw*). Actually, I utilize firewire-request in Linux FireWire utilities (former known as jujuutils), and write my own I/O library, libhinawa. Linux FireWire utilities https://github.com/cladisch/linux-firewire-utils libhinawa http://mailman.alsa-project.org/pipermail/alsa-devel/2015-January/086969.html For example, to control the clock selector of Digi 002/003 family, we just execute this command with firewire-request. $ ./firewire-request /dev/fw1 write 0xffffe0000118 0x0000000[0|1|2|3] We can perform to control them from userspace. > This is one of the reasons, why USB soundcards expose all settings > though the alsa mixer interface. It also offers a standardized interface > for userspace apps to build on top and a method to directly link mixer > to soundcard for systems with multiple soundcards. > > How do you envisage to handle this? These USB-connected sound devices basically tells their control capabilities by USB descriptor information. This mechanism is standardized and included in USB specification. Thus single parser has a meaning. On the other hand, IEEE 1394 bus-connected sound devices implements its own way to tell their control capabilities and model-specific way to perform control. Thus we should prepare for model-specific parsers. The idea to include these parsers into kernel-space increases maintaining efforts. (Actually, USB-connected sound devices includes vendor-specific interface. Such interfaces require own codes and snd-usb-audio includes these code. You can see such codes in sound/usb/mixer_quirks.c and the other USB Audio device class driver codes.) In an aspect of user experience, I cannot find any differences between using alsamixer (or amixer) and using specific-application. ALSA PCM character devices, ALSA Control character devices and Linux FireWire character devices are completely different and users don't mind the differences. What the users' want is to control their physical devices, to consider about to which character devices they access. Regards Takashi Sakamoto
On Mar 20 2015 07:46, Takashi Sakamoto wrote: > I can't understand what character devices you mention about. If you > mention about ALSA PCM character devices (usually > /dev/snd/pcmC%uD%d[p|c]), it's yes. In fact, one ALSA application can > use ALSA PCM character device for one direction (playback/capture). So > the access by any other applications causes -EBUSY. (here, I ignore > alsa-lib's PCM direct mixing layer.) I'll correct the 'ALSA application' with 'ALSA PCM application'. The ALSA PCM application and ALSA Control application are different because they access to different character devices and use different set of ioctl(2) interfaces or different API set given by alsa-lib. Regards Takashi Sakamoto
On 03/19/2015 11:46 PM, Takashi Sakamoto wrote: Hi Takashi Thanks for elaborating. [..] > For example, to control the clock selector of Digi 002/003 family, we > just execute this command with firewire-request. > > $ ./firewire-request /dev/fw1 write 0xffffe0000118 0x0000000[0|1|2|3] Yes, that's what I was asking about. Can one safely write raw control messages to /dev/fw* without interfering with ongoing streaming? Instead interfacing via established protocols /dev/snd/control* or rather libasound's snd_mixer_t seems like a no-brainer to me. Are there any control elements on common 1394 devices that cannot be properly exposed using existing infrastructure? > On the other hand, IEEE 1394 bus-connected sound devices implements its > own way to tell their control capabilities and model-specific way to > perform control. Thus we should prepare for model-specific parsers. The > idea to include these parsers into kernel-space increases maintaining > efforts. Agreed. Most of the heavy lifting should probably be done by libasound. > In an aspect of user experience, I cannot find any differences between > using alsamixer (or amixer) and using specific-application. Uhm. It's a huge difference. There is a whole lot of existing infrastructure: from Sys-V init.d and/or SystemD save/restore, dbus hooks, existing mixer GUIs and application integration, not to mention various language bindings (eg pyalsa). Linux Audio is already fragmented enough as it stands, adding yet another interface/toolchain won't help. One might just as well continue to use ffado. I was under the impression that the whole point of moving 1394 audio into the kernel was to allow seamless integration with the rest of ALSA. 2c, robin
Robin Gareus wrote: > On 03/19/2015 11:46 PM, Takashi Sakamoto wrote: >> For example, to control the clock selector of Digi 002/003 family, we >> just execute this command with firewire-request. >> >> $ ./firewire-request /dev/fw1 write 0xffffe0000118 0x0000000[0|1|2|3] > > Yes, that's what I was asking about. Can one safely write raw control > messages to /dev/fw* without interfering with ongoing streaming? Yes. If a device did not allow this, or if the mixer accesses would be part of the audio stream, the driver would have no choice but to implement this in the kernel. But this is not the case for most devices. > Instead interfacing via established protocols /dev/snd/control* or > rather libasound's snd_mixer_t seems like a no-brainer to me. It is possible to attach 'virtual' mixer controls to hardware sound cards. (This was originally designed for the software volume control.) The only reason that FFADO did not use this was that there was no suitable ALSA card instance to attach to. Regards, Clemens
Hi Robin, On Mar 20 2015 21:05, Robin Gareus wrote: >> $ ./firewire-request /dev/fw1 write 0xffffe0000118 0x0000000[0|1|2|3] > > Yes, that's what I was asking about. Can one safely write raw control > messages to /dev/fw* without interfering with ongoing streaming? Any userspace applications can destroy packet streaming which kernel driver starts, by transaction to streaming-related register. In current implementation of ALSA firewire stack and Linux FireWire subsystem, we cannot avoid this. For example, about Digi 002/003 family, we can destroy kernel streaming in a way below: 1.write/read PCM samples to ALSA PCM character devices (in most case done via alsa-lib PCM API) 2.write transaction with 0x00000003 for 0xffffe0000004 to /dev/fw%u. 3.Applications cannot read/write PCM samples again. In this case, usually, the process receive EIO from ALSA PCM API. > Instead interfacing via established protocols /dev/snd/control* or > rather libasound's snd_mixer_t seems like a no-brainer to me. As long as being able to send transactions via FireWire character devices, the headache remains, regardless of the place to implement such control functionality. > Are there any control elements on common 1394 devices that cannot be > properly exposed using existing infrastructure? More explaination, please. >> On the other hand, IEEE 1394 bus-connected sound devices implements its >> own way to tell their control capabilities and model-specific way to >> perform control. Thus we should prepare for model-specific parsers. The >> idea to include these parsers into kernel-space increases maintaining >> efforts. > > Agreed. Most of the heavy lifting should probably be done by libasound. I don't think it possible to argue the other ALSA developers for going to include such vendor-specific or model-specific huge codes to alsa-lib... (Except for intel HDA) >> In an aspect of user experience, I cannot find any differences between >> using alsamixer (or amixer) and using specific-application. > > Uhm. It's a huge difference. There is a whole lot of existing > infrastructure: from Sys-V init.d and/or SystemD save/restore, dbus > hooks, existing mixer GUIs and application integration, not to mention > various language bindings (eg pyalsa). Here, I mention about alsa-lib control API to add control elements from userspace and it's eventing mechanism. http://www.alsa-project.org/alsa-doc/alsa-lib/group___control.html#gad5f640f1d836b532b1c18d7604a90bad Regards Takashi Sakamoto
At Fri, 20 Mar 2015 22:25:26 +0900, Takashi Sakamoto wrote: > > >> On the other hand, IEEE 1394 bus-connected sound devices implements its > >> own way to tell their control capabilities and model-specific way to > >> perform control. Thus we should prepare for model-specific parsers. The > >> idea to include these parsers into kernel-space increases maintaining > >> efforts. > > > > Agreed. Most of the heavy lifting should probably be done by libasound. > > I don't think it possible to argue the other ALSA developers for going > to include such vendor-specific or model-specific huge codes to > alsa-lib... (Except for intel HDA) Why not implementing as a plugin? Takashi
On Mar 20 2015 22:35, Takashi Iwai wrote: >> I don't think it possible to argue the other ALSA developers for going >> to include such vendor-specific or model-specific huge codes to >> alsa-lib... (Except for intel HDA) > > Why not implementing as a plugin? As long as I know, we cannot write any configuration to load it for 'hw' node. On the other hand, when adding any nodes like 'bebob' or 'dice', they always stay in alsa-lib configuration space even if there're no actual devices connected. If my understanding is wrong, please inform it to me. Regards Takashi Sakamoto
On 21/03/15 00:25, Takashi Sakamoto wrote: > Any userspace applications can destroy packet streaming which kernel > driver starts, by transaction to streaming-related register. > > In current implementation of ALSA firewire stack and Linux FireWire > subsystem, we cannot avoid this. Does that mean the mixer control will need root permissions to execute in userspace because the /dev/fwX node is usually owned by root? Damien
Damien Zammit wrote: > On 21/03/15 00:25, Takashi Sakamoto wrote: >> Any userspace applications can destroy packet streaming which kernel >> driver starts, by transaction to streaming-related register. >> >> In current implementation of ALSA firewire stack and Linux FireWire >> subsystem, we cannot avoid this. > > Does that mean the mixer control will need root permissions to execute > in userspace because the /dev/fwX node is usually owned by root? /dev/fw* of audio devices typically get a different group to allow FFADO to run. Whatever software implements these mixer controls (whether it ends up called FFADO or not) would run with the same permissions. Regards, Clemens
At Fri, 20 Mar 2015 22:51:25 +0900, Takashi Sakamoto wrote: > > On Mar 20 2015 22:35, Takashi Iwai wrote: > >> I don't think it possible to argue the other ALSA developers for going > >> to include such vendor-specific or model-specific huge codes to > >> alsa-lib... (Except for intel HDA) > > > > Why not implementing as a plugin? > > As long as I know, we cannot write any configuration to load it for 'hw' > node. On the other hand, when adding any nodes like 'bebob' or 'dice', > they always stay in alsa-lib configuration space even if there're no > actual devices connected. > > If my understanding is wrong, please inform it to me. You seem mixing up how to use the plugin setup and how to write the plugin... The usage with a plugin might be more complex indeed, but it's more or less same no matter whether you implement in alsa-lib itself or implement as an external plugin. Takashi
On Mar 20 2015 23:13, Takashi Iwai wrote: > At Fri, 20 Mar 2015 22:51:25 +0900, > Takashi Sakamoto wrote: >> >> On Mar 20 2015 22:35, Takashi Iwai wrote: >>>> I don't think it possible to argue the other ALSA developers for going >>>> to include such vendor-specific or model-specific huge codes to >>>> alsa-lib... (Except for intel HDA) >>> >>> Why not implementing as a plugin? >> >> As long as I know, we cannot write any configuration to load it for 'hw' >> node. On the other hand, when adding any nodes like 'bebob' or 'dice', >> they always stay in alsa-lib configuration space even if there're no >> actual devices connected. >> >> If my understanding is wrong, please inform it to me. > > You seem mixing up how to use the plugin setup and how to write the > plugin... The usage with a plugin might be more complex indeed, but > it's more or less same no matter whether you implement in alsa-lib > itself or implement as an external plugin. Sorry, but I consider about one-step future. I think it possible to discuss constructively about such plugins for alsa-plugins, while its usage is not so easy for usual users of FireWire audio devices. I can imagine to receive much requests about improvements, then consider about including it to alsa-lib itself. But this idea may be hard to achieve because of the reasons I describe. I felt a bit unhappiness about your question and had a logic jump, sorry. I'm not so tough developer... Regards Takashi Sakamoto
At Fri, 20 Mar 2015 23:45:40 +0900, Takashi Sakamoto wrote: > > On Mar 20 2015 23:13, Takashi Iwai wrote: > > At Fri, 20 Mar 2015 22:51:25 +0900, > > Takashi Sakamoto wrote: > >> > >> On Mar 20 2015 22:35, Takashi Iwai wrote: > >>>> I don't think it possible to argue the other ALSA developers for going > >>>> to include such vendor-specific or model-specific huge codes to > >>>> alsa-lib... (Except for intel HDA) > >>> > >>> Why not implementing as a plugin? > >> > >> As long as I know, we cannot write any configuration to load it for 'hw' > >> node. On the other hand, when adding any nodes like 'bebob' or 'dice', > >> they always stay in alsa-lib configuration space even if there're no > >> actual devices connected. > >> > >> If my understanding is wrong, please inform it to me. > > > > You seem mixing up how to use the plugin setup and how to write the > > plugin... The usage with a plugin might be more complex indeed, but > > it's more or less same no matter whether you implement in alsa-lib > > itself or implement as an external plugin. > > Sorry, but I consider about one-step future. > > I think it possible to discuss constructively about such plugins for > alsa-plugins, while its usage is not so easy for usual users of FireWire > audio devices. I can imagine to receive much requests about > improvements, then consider about including it to alsa-lib itself. But > this idea may be hard to achieve because of the reasons I describe. OK, point taken. Then it's no matter whether plugin or not. Rather a question is whether it works out-of-the-box without any extra configuration, right? It'd be possible to achieve this in pure alsa-lib config, but it can be complicated. The alsa-lib config may choose different sub-config depending on the hardware component. The current per-card config setup is already so, and USB-audio.conf contains yet more per-driver extra config. But I don't also push this as the best solution, either. My comment was only about the implementation detail *if* we want to implement as an alsa-lib functionality. Any better solution is more welcome. However, what would be the better alternative for the mixer setup including the clock source selection? Invoking another mixer program may annoy people (as least already two people complained :) Takashi
On 21/03/15 02:01, Takashi Iwai wrote: > OK, point taken. Then it's no matter whether plugin or not. Rather a > question is whether it works out-of-the-box without any extra > configuration, right? I think it would be a nice feature if it just worked out of the box like any other alsa mixer control, and I'm sure many other people who are currently using FFADO would agree that having everything to control the sound card in ALSA would be more convenient than what they have now. Takashi S: can we perhaps focus on getting the streaming working better with 003amdtp for digi00x and revisit the issue of mixer controls later? Have you looked at the updated 003amdtp documentation? I believe Robin has made suggestions on how to integrate the latest code into the driver. <snip> #ifdef EXAMPLE // NB. this should not be static (multiple devices) // and also re-initialzed on open/close or stream-restart. // use digi_state_reset(&state); static DigiMagic digistate = {0x00, 0x00, 0}; #endif </snip> Damien
On Mar 21 2015 14:59, Damien Zammit wrote: > I think it would be a nice feature if it just worked out of the box like > any other alsa mixer control, and I'm sure many other people who are > currently using FFADO would agree that having everything to control the > sound card in ALSA would be more convenient than what they have now. Over generalization. Actually ALSA middleware cannot represent control interfaces with the same level as FFADO achieves, thus your idea can bring no advantages to FFADO users. > Takashi S: can we perhaps focus on getting the streaming working better > with 003amdtp for digi00x and revisit the issue of mixer controls later? It's what I've suggested. > Have you looked at the updated 003amdtp documentation? I believe Robin > has made suggestions on how to integrate the latest code into the driver. It's due to what I've reported to Robin. According to his response, I've judge his codes have never passed any appropriate tests with actual devices. Regards Takashi Sakamoto
On Sun, Mar 22, 2015 at 11:55:24AM +0900, Takashi Sakamoto wrote: > On Mar 21 2015 14:59, Damien Zammit wrote: > > I think it would be a nice feature if it just worked out of the box like > > any other alsa mixer control, and I'm sure many other people who are > > currently using FFADO would agree that having everything to control the > > sound card in ALSA would be more convenient than what they have now. > > Over generalization. > > Actually ALSA middleware cannot represent control interfaces with the > same level as FFADO achieves, thus your idea can bring no advantages to > FFADO users. I've been following this thread but haven't had a chance to respond until now. Speaking generally I think Takashi S's comment is a valid consideration. While there are some controls on some Firewire audio devices which could make use of the alsamixer interface, there are other situations where the alsamixer program and/or API either could not cope or would be very cumbersome to use. For these devices having a dedicated mixer-type application which communicated directly with the audio interface would make sense. Mixer controls function independently of the streaming engine in almost every case, so there is no technical reason why the kernel would need to be burdened which what could end up being a very large block of code. I don't see this as a bad thing though. Complex audio interfaces already have dedicated mixer applications in alsa-tools. I don't think the lack of certain controls via the generic alsamixer hinders the use of these devices. Regarding device control, there will always be a need to draw a line in the sand somewhere. At some point a certain device control becomes secondary to audio streaming and more to do with signal routing and conditioning in the device. The clock source control which Damien mentioned is a case in point. Assuming the presence of a clock signal on the selected clock source, streaming startup doesn't really need to be concerned with the selection of the clock source. However, it is clearly related to streaming so it could be argued that something like this does deserve to be provided with an alsa API interface of some description - even if it is only for the convenience of users. Contrast this to phantom power switches, which have nothing to do with audio streaming at all. > It does mean that alsa clients (such as jackd) don't currently provide a > way to switch clock sources on startup. Most sound cards don't have multiple clock sources, which is probably the reason why there is not a standard way to represent such a control via the alsa API. That doesn't mean that one couldn't be defined of course, and doing so would allow clients like jackd to switch clock sources on startup (something which would be useful for a number of use cases). The complicating factor in this is that while many interfaces could represent the clock source selection as a simple one-of-many selector, some devices are in reality more complicated than this. As a result, any generic clock source selector - if adopted - would have to allow for this to be done in other ways if needed. What all this comes down to is that I don't see a problem with the use of userspace model-specific mixer applications for interfaces which genuinely need it. It seems to have worked out fine for envy24control and hdspmixer for example. Whether these talk direct to the device or via some alsa API is probably a device-by-device decision. If a control/mixer program was required for a given interface I would expect it to be ultimately included in alsa-tools. Having the required tools within the alsa packages would get as close to the "works out the box" situation as possible. Regards jonathan
On Fri, Mar 20, 2015 at 01:05:16PM +0100, Robin Gareus wrote: > I was under the impression that the whole point of moving 1394 audio into > the kernel was to allow seamless integration with the rest of ALSA. Speaking as a FFADO developer, the primary reason for pushing FFADO's audio streaming code into the kernel is for performance and reliability reasons: it is difficult to maintain the necessary timing when running as a userspace client of the juju ABI. Being in kernel brings advantages in this respect. The mixer side of things is competely orthogonal to the audio streaming system in FFADO and was therefore peripheral to this. As a result the movement of device control into ALSA wasn't really discussed in great detail: regardless of whether a kernel or user-space streaming engine is used, the device control component of FFADO (ffado-mixer) will continue to be functional. At least in the first instance the thought was to concentrate on getting the streaming code into the kernel since that would resolve a vast majority of performance issues that FFADO faces. Furthermore, kernel streaming is the only manditory component needed to allow firewire interfaces to be usable from "ALSA" applications (presently a program must support JACK to make use of devices supported by FFADO). For many users of these interfaces, the fact that they might have to use something other than alsamixer to control the interface is completely irrelevant. The idea was that once in-kernel streaming was operational the longer term mixer situation could then be looked at if it made sense to change it. Whether things work out like this remains to be seen. Regards jonathan
On 03/22/2015 03:55 AM, Takashi Sakamoto wrote: [..] > It's due to what I've reported to Robin. According to his response, I've > judge his codes have never passed any appropriate tests with actual devices. We did clean-room reverse engineer the protocol and key. I do not own any digidesign devices (nor any other firewire devices for that matter) and have never tested it myself on real hardware. I can only state that the current code works correctly on all the raw bus data dumps that I have seen so far. Damien verified the code to work on real hardware and mentioned that in a prior email and also on https://github.com/takaswie/snd-firewire-improve/issues/16 Cheers! robin
diff --git a/sound/firewire/digi00x/digi00x-protocol.c b/sound/firewire/digi00x/digi00x-protocol.c index 4dd373d..ac092cb 100644 --- a/sound/firewire/digi00x/digi00x-protocol.c +++ b/sound/firewire/digi00x/digi00x-protocol.c @@ -2,12 +2,125 @@ * digi00x-protocol.c - a part of driver for Digidesign Digi 002/003 family * * Copyright (c) 2014-2015 Takashi Sakamoto + * Copyright (C) 2012 Robin Gareus <robin@gareus.org> + * Copyright (C) 2012 Damien Zammit <damien@zamaudio.com> * * Licensed under the terms of the GNU General Public License, version 2. */ #include "digi00x.h" +/* + * The double-oh-three algorism is invented by Robin Gareus and Damien Zammit + * in 2012, with reverse-engineering for Digi 003 Rack. + */ + +struct dot_state { + __u8 carry; + __u8 idx; + unsigned int off; +}; + +#define BYTE_PER_SAMPLE (4) +#define MAGIC_DOT_BYTE (2) + +#define MAGIC_BYTE_OFF(x) (((x) * BYTE_PER_SAMPLE) + MAGIC_DOT_BYTE) + +/* + * double-oh-three look up table + * + * @param idx index byte (audio-sample data) 0x00..0xff + * @param off channel offset shift + * @return salt to XOR with given data + */ +static const __u8 dot_scrt(const __u8 idx, const unsigned int off) +{ + /* + * the length of the added pattern only depends on the lower nibble + * of the last non-zero data + */ + static const __u8 len[16] = {0, 1, 3, 5, 7, 9, 11, 13, 14, + 12, 10, 8, 6, 4, 2, 0}; + + /* + * the lower nibble of the salt. Interleaved sequence. + * this is walked backwards according to len[] + */ + static const __u8 nib[15] = {0x8, 0x7, 0x9, 0x6, 0xa, 0x5, 0xb, 0x4, + 0xc, 0x3, 0xd, 0x2, 0xe, 0x1, 0xf}; + + /* circular list for the salt's hi nibble. */ + static const __u8 hir[15] = {0x0, 0x6, 0xf, 0x8, 0x7, 0x5, 0x3, 0x4, + 0xc, 0xd, 0xe, 0x1, 0x2, 0xb, 0xa}; + + /* + * start offset for upper nibble mapping. + * note: 9 is /special/. In the case where the high nibble == 0x9, + * hir[] is not used and - coincidentally - the salt's hi nibble is + * 0x09 regardless of the offset. + */ + static const __u8 hio[16] = {0, 11, 12, 6, 7, 5, 1, 4, + 3, 0x00, 14, 13, 8, 9, 10, 2}; + + const __u8 ln = idx & 0xf; + const __u8 hn = (idx >> 4) & 0xf; + const __u8 hr = (hn == 0x9) ? 0x9 : hir[(hio[hn] + off) % 15]; + + if (len[ln] < off) + return 0x00; + + return ((nib[14 + off - len[ln]]) | (hr << 4)); +} + +static inline void dot_state_reset(struct dot_state *state) +{ + state->carry = 0x00; + state->idx = 0x00; + state->off = 0; +} + +static void dot_encode_step(struct dot_state *state, __be32 *const buffer) +{ + __u8 * const data = (__u8 *) buffer; + + if (data[MAGIC_DOT_BYTE] != 0x00) { + state->off = 0; + state->idx = data[MAGIC_DOT_BYTE] ^ state->carry; + } + data[MAGIC_DOT_BYTE] ^= state->carry; + state->carry = dot_scrt(state->idx, ++(state->off)); +} + +void snd_dg00x_protocol_write_s32(struct amdtp_stream *s, + struct snd_pcm_substream *pcm, + __be32 *buffer, unsigned int frames) +{ + struct snd_pcm_runtime *runtime = pcm->runtime; + unsigned int channels, remaining_frames, i, c; + const u32 *src; + static struct dot_state state; + + channels = s->pcm_channels; + src = (void *)runtime->dma_area + + frames_to_bytes(runtime, s->pcm_buffer_pointer); + remaining_frames = runtime->buffer_size - s->pcm_buffer_pointer; + + for (i = 0; i < frames; ++i) { + dot_state_reset(&state); + + for (c = 0; c < channels; ++c) { + buffer[s->pcm_positions[c]] = + cpu_to_be32((*src >> 8) | 0x40000000); + dot_encode_step(&state, &buffer[s->pcm_positions[c]]); + src++; + } + + buffer += s->data_block_quadlets; + if (--remaining_frames == 0) + src = (void *)runtime->dma_area; + } +} + void snd_dg00x_protocol_fill_midi(struct amdtp_stream *s, __be32 *buffer, unsigned int frames) { diff --git a/sound/firewire/digi00x/digi00x-stream.c b/sound/firewire/digi00x/digi00x-stream.c index 410b971..bc4c88c 100644 --- a/sound/firewire/digi00x/digi00x-stream.c +++ b/sound/firewire/digi00x/digi00x-stream.c @@ -204,6 +204,8 @@ static int keep_resources(struct snd_dg00x *dg00x, unsigned int rate) dg00x->rx_stream.midi_position = 0; dg00x->tx_stream.midi_position = 0; + /* Apply doubleOhThree algorism. */ + dg00x->rx_stream.transfer_samples = snd_dg00x_protocol_write_s32; dg00x->rx_stream.transfer_midi = snd_dg00x_protocol_fill_midi; dg00x->tx_stream.transfer_midi = snd_dg00x_protocol_pull_midi; diff --git a/sound/firewire/digi00x/digi00x.h b/sound/firewire/digi00x/digi00x.h index a658f44..07e54fc 100644 --- a/sound/firewire/digi00x/digi00x.h +++ b/sound/firewire/digi00x/digi00x.h @@ -84,6 +84,9 @@ enum snd_dg00x_optical_mode { SND_DG00X_OPTICAL_MODE_SPDIF, }; +void snd_dg00x_protocol_write_s32(struct amdtp_stream *s, + struct snd_pcm_substream *pcm, + __be32 *buffer, unsigned int frames); void snd_dg00x_protocol_fill_midi(struct amdtp_stream *s, __be32 *buffer, unsigned int frames); void snd_dg00x_protocol_pull_midi(struct amdtp_stream *s,
Digi 002/003 family uses own way to multiplex PCM samples into data blocks in CIP payload for received stream, thus AMDTP-conformant implementation causes noisy sound. This commit applies double-oh-three algorism, which discovered by Robin Gareus and Damien Zammit in 2012. As long as I tested, this patch still has disorder that: * 1st PCM channel still causes noise in 2nd PCM channel. * At 88.2/96.0kHz, any PCM channels still causes noise in the other PCM channels. Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> --- sound/firewire/digi00x/digi00x-protocol.c | 113 ++++++++++++++++++++++++++++++ sound/firewire/digi00x/digi00x-stream.c | 2 + sound/firewire/digi00x/digi00x.h | 3 + 3 files changed, 118 insertions(+)