All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: Controlling the Tascam FW-1884
       [not found] <9cec059e1ff1a558f21a3f0729c5a69a3d506573.camel@suse.com>
@ 2018-09-08  2:53 ` Takashi Sakamoto
  2018-09-08 11:21   ` Scott Bahling
  0 siblings, 1 reply; 22+ messages in thread
From: Takashi Sakamoto @ 2018-09-08  2:53 UTC (permalink / raw)
  To: sbahling; +Cc: alsa-devel, ffado-devel

Hi Scott,
(C.Ced to alsa-devel, ffado-devel)

On Sep 7 2018 23:01, Scott Bahling wrote:
> I recently got my hands on a FW-1884 and would like to get it working in
> Linux. I'm mostly interested in using the control interface which should
> support several midi control interface protocols like Mackie Control or
> Mackie HUI. From the user manual it appears that the host computer needs to
> set the registers on the FW-1884 to switch modes (they reference Windows
> applications).
> 
> I see that you have recently been working on user space support asynchronous
> commands to such devices. The hinawa_utils code has some beginnings of
> support for setting options on the Tascam devices and I have just started
> playing with that.

Thanks for your suggestion.

At present, the most of features on control surface of FW-1884 are not
available, especially for fader, knob and start/stop buttons.

In data transmission between system and devices on IEEE 1394 bus,
both of asynchronous transactions and isochronous packet streams are
available. In a case of TASCAM FireWire series, messages via MIDI
physical interface is bypassed to asynchronous transaction, while
messages generated on control surface are bypassed to isochronous
packets, multiplexed with PCM frames.

When enabling multiplexing messages to isochronous packet,
demultiplexer should be implemented to ALSA firewire-tascam driver,
but not yet. You can see my initial RFC in alsa-devel[1]. This patch
enables userspace applications to perform the demultiplexing, however
this idea easily brings cache-coherency problems due to page mapping
between kernel/userspace and impossible to be applied. We need to
investigate another idea to achieve the demultiplexing in kernel land.

As another option, according to page 26 in 'TASCAM FW-1884 Owner's
Manual', when programming Keys to port 1-4, system could receive MIDI
messages from control surface of FW-1884. However, I've not investigated
it.

> How are you determining the registers and flags? Do you have documentation,
> or are you reverse engineering? Do you know the registers and flags for
> setting the control protocol?
> 
> If you have any hints on how figure out the proper registers and flags I'm
> happy to contribute to the work you have started and help support more
> functions of the FW-1884.

User manuals are documents to which we can refer. The most vendors
licenses their drivers/software with EULA and in most cases it can't
allow users to binary-based reverse engineering.

All of my work are done with external protocol analyzer unit on
IEEE 1394 bus. When working to implement drivers, I
read/classified/parsed content of many packets sniffed by the analyzer.
An initial idea of libhinawa/hinawa-utils is to assist this my work.

I heard FFADO developers uses some ways for this type of work:
  - Usage of host controller of TI PCILynx and driver module (nosy.ko)
  - Windows application; bushound[2]

However, this type of work takes you long time, so I don't necessarily
recommend you. It's 2018 and I think you can find alternative control
surfaces on USB. If you take long time to make devices available on
GNU/Linux, it's better to select devices on 'live' buses instead of
'legacy' buses such as IEEE 1394 bus, IMO. In a case of USB, wireshark
with usbcap is the easiest way for us to sniff packets on Windows.

[1] 
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-July/094817.html
[2] http://perisoft.net/bushound/

Regards

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-09-08  2:53 ` Controlling the Tascam FW-1884 Takashi Sakamoto
@ 2018-09-08 11:21   ` Scott Bahling
  2018-09-10  7:59     ` Takashi Sakamoto
  0 siblings, 1 reply; 22+ messages in thread
From: Scott Bahling @ 2018-09-08 11:21 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, ffado-devel


[-- Attachment #1.1: Type: text/plain, Size: 3740 bytes --]

On Sat, 2018-09-08 at 11:53 +0900, Takashi Sakamoto wrote:
> Hi Scott,
> (C.Ced to alsa-devel, ffado-devel)
> 
<snip>

> 
> When enabling multiplexing messages to isochronous packet,
> demultiplexer should be implemented to ALSA firewire-tascam driver,
> but not yet. You can see my initial RFC in alsa-devel[1]. This patch
> enables userspace applications to perform the demultiplexing, however
> this idea easily brings cache-coherency problems due to page mapping
> between kernel/userspace and impossible to be applied. We need to
> investigate another idea to achieve the demultiplexing in kernel
> land.

Thanks for the information! I read the RFC and your initial code. Why
did you remove the virtual MIDI port code that was part of the RFC code
(I don't see it upstream)? Did it not work? What I have discovered is
that in "computer control" mode the unit sends MIDI messages via the
virtual midi port (control port). The Tascam Native protocol as well as
the other emulations like Mackie are routed via that virtual midi port
and never touch the physical midi ports.

So the secret might be enabling the virtual midi ports - but it's still
not clear to me if those ports are created/mapped within the FW-1884,
or need to be created on the host OS based on the isochronous packet
data. The latter would be more work.

> As another option, according to page 26 in 'TASCAM FW-1884 Owner's
> Manual', when programming Keys to port 1-4, system could receive MIDI
> messages from control surface of FW-1884. However, I've not
> investigated
> it.

I tested this using a supported OS to program the FW-1884 controls to
midi. It works, and the settings are nonvolatile. With this, all
controls can be assigned, but it's one way control from the FW-1884
outward. There appears to be no way to control the motorized faders or
panel indicators from the application towards the FW-1884 in this mode.

<snip>

> User manuals are documents to which we can refer. The most vendors
> licenses their drivers/software with EULA and in most cases it can't
> allow users to binary-based reverse engineering.
> 
> All of my work are done with external protocol analyzer unit on
> IEEE 1394 bus. When working to implement drivers, I
> read/classified/parsed content of many packets sniffed by the analyzer.
> An initial idea of libhinawa/hinawa-utils is to assist this my work.

Right. That's what I meant by reverse engineering - analyzing the
traffic and device behavior to understand the protocols.

> I heard FFADO developers uses some ways for this type of work:
>   - Usage of host controller of TI PCILynx and driver module
> (nosy.ko)
>   - Windows application; bushound[2]

Yep. Read about all that already ;) It sounds like I could take your
RFC code, build a custom kernel, and start analyzing the status and
control messages to get a better understanding of what is happening at
that layer.

> However, this type of work takes you long time, so I don't necessarily
> recommend you. It's 2018 and I think you can find alternative control
> surfaces on USB. If you take long time to make devices available on
> GNU/Linux, it's better to select devices on 'live' buses instead of
> 'legacy' buses such as IEEE 1394 bus, IMO. In a case of USB, wireshark
> with usbcap is the easiest way for us to sniff packets on Windows.

Understood, but hacking is part of the fun, and this is looking like a
really nice piece of equipment (better than modern control surfaces I
have looked at in my price range). Legacy buses don't scare me. I'm
still running my main audio capture card on legacy PCI bus.

Thanks for the ground work you have already done!

Regards,

Scott


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-09-08 11:21   ` Scott Bahling
@ 2018-09-10  7:59     ` Takashi Sakamoto
  2018-09-12  7:18       ` Scott Bahling
  0 siblings, 1 reply; 22+ messages in thread
From: Takashi Sakamoto @ 2018-09-10  7:59 UTC (permalink / raw)
  To: Scott Bahling; +Cc: alsa-devel, ffado-devel

Hi Scott,

On Sep 8 2018 20:21, Scott Bahling wrote:
>> When enabling multiplexing messages to isochronous packet,
>> demultiplexer should be implemented to ALSA firewire-tascam driver,
>> but not yet. You can see my initial RFC in alsa-devel[1]. This patch
>> enables userspace applications to perform the demultiplexing, however
>> this idea easily brings cache-coherency problems due to page mapping
>> between kernel/userspace and impossible to be applied. We need to
>> investigate another idea to achieve the demultiplexing in kernel
>> land.
> 
> Thanks for the information! I read the RFC and your initial code. Why
> did you remove the virtual MIDI port code that was part of the RFC code
> (I don't see it upstream)? Did it not work? What I have discovered is
> that in "computer control" mode the unit sends MIDI messages via the
> virtual midi port (control port). The Tascam Native protocol as well as
> the other emulations like Mackie are routed via that virtual midi port
> and never touch the physical midi ports.
> 
> So the secret might be enabling the virtual midi ports - but it's still
> not clear to me if those ports are created/mapped within the FW-1884,
> or need to be created on the host OS based on the isochronous packet
> data. The latter would be more work.

I depict an overview relevant to messages from control surface of FW-1884.

           IEEE 1394 bus
  +-------+            +--------+
  |       |   isoc     |        |          +->PCM frames
  |FW-1884| ---------> |1394OHCI| -(demux)-+
  |       |   packet   |        |          +->control message
  +-------+            +--------+                   |
                                                (convert)
                                                    v
                                            Mackie control/HUI
                                                    v
                                               applications

The position of control message in isochronous packet is fixed and
unable to change. Conversion of control message to Mackie control or
HUI is done by software running on operating system.

Below is an actual sample of isochronous packets from the unit.

packet 1:
   CIP header: 0x001400EA,0x9E000000,
   datablock1: 0xE63AE63A,(..PCM frames..),0x00000000,
   datablock2: 0xE63BE63B,(..PCM frames..),0x00000001,
   datablock3: 0xE63CE63C,(..PCM frames..),0x00000000,
   datablock4: 0xE63DE63D,(..PCM frames..),0x00000000,
   datablock5: 0xE63EE63E,(..PCM frames..),0x00000000,
   datablock6: 0xE63FE63F,(..PCM frames..),0x00000000,
packet 2:
   CIP header: 0x001400F0,0x9E000000,
   datablock1: 0xE640E640,(..PCM frames..),0x00020000,
   datablock2: 0xE641E641,(..PCM frames..),0x000B0002,
   datablock3: 0xE642E642,(..PCM frames..),0x00000000,
   datablock4: 0xE643E643,(..PCM frames..),0x00000000,
   datablock5: 0xE644E644,(..PCM frames..),0x00000000,
packet 3:
   CIP header: 0x001400F5,0x9E000000,
   datablock1: 0xE645E645,(..PCM frames..),0xFFFF00C9,
   datablock2: 0xE646E646,(..PCM frames..),0xFFFFFFFF,
   datablock3: 0xE647E647,(..PCM frames..),0xFFFFFFFF,
   datablock4: 0xE648E648,(..PCM frames..),0xFFFFFFFF,
   datablock5: 0xE649E649,(..PCM frames..),0xFFFFFFFF,
   datablock6: 0xE64AE64A,(..PCM frames..),0x00000000,
packet 4:
   CIP header: 0x001400FB,0x9E000000,
   datablock1: 0xE64BE64B,(..PCM frames..),0x00000000,
   datablock2: 0xE64CE64C,(..PCM frames..),0x00000000,
   datablock3: 0xE64DE64D,(..PCM frames..),0x00000000,
   datablock4: 0xE64EE64E,(..PCM frames..),0x00000000,
   datablock5: 0xE64FE64F,(..PCM frames..),0x00000000,

The first quadlet in each data block represents the count of transferred
data blocks: 0x00000000,0x00010001,...,0xfffefffe,0xffffffff(back to
0x00000000). The last quadlet in each data block represents control and
status data. 64 quadlets of the data consists of one image of control
and status.

For example, the second data block of packet 3 can be parsed:
  * __be32 image[64];
  * pos = (0xE646E645 % 64)
  * image[pos] = 0xFFFF00C9

The above sample includes a part of the image, below.
   image[58] = 0x00000000
   image[59] = 0x00000001
   image[60] = 0x00000000
   image[61] = 0x00000000
   image[62] = 0x00000000
   image[63] = 0x00000000
   image[00] = 0x00020000
   image[01] = 0x000B0002
   image[02] = 0x00000000
   image[03] = 0x00000000
   image[04] = 0x00000000
   image[05] = 0xFFFF00C9
   image[06] = 0xFFFFFFFF
   image[07] = 0xFFFFFFFF
   image[08] = 0xFFFFFFFF
   image[09] = 0xFFFFFFFF
   image[10] = 0x00000000
   image[11] = 0x00000000
   image[12] = 0x00000000
   image[13] = 0x00000000
   image[14] = 0x00000000
   image[15] = 0x00000000

The frequency to update this image depends on the number of data blocks
per second(=current sampling transmission, sampling rate):
  - 44.1kHz: 1.45 msec (=64/44100)
  - 48.0kHz: 1.33 msec (=64/48000)
  - 88.2kHz: 0.72 msec (=64/88200)
  - 96.0kHz: 0.66 msec (=64/96000)

However, in current implementation of packet streaming engine for
IEC 61882-1/6 of ALSA firewire stack, 16 packets are handled in one 
interrupt (INTERRUPT_INTERVAL=16). Therefore, a period to handle a batch
of packet is 7.8125 msec (=16/8000, 8000 is the number of isochronous
cycles in IEEE 1394 bus).

As long as I know, the image consists of below information:
  - pos  0-15: control messages from physical control surface
  - pos 16-23: analog input level
  - pos 24-31: digital ADAT input level
  - pos 32-33: digital S/PDIF input level
  - pos 34-35: (not cleared yet)
  - pos 36-43: analog output level
  - pos 44-64: (not cleared yet)

When parsing the image, a parser should generate several types of
events. I prefer to implementing this complicated work in user space
instead of kernel space. Expecially for pos 16-23/24-31/32-33/36-43, the
parser should always generate events to notify each of the levels.

What we should do is to parsing the image and generate events with
enough consideration of task scheduling and eventing dencity. The parser
could be implemented as an application of ALSA sequencer.


Regards

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-09-10  7:59     ` Takashi Sakamoto
@ 2018-09-12  7:18       ` Scott Bahling
  2018-09-17 14:36         ` Takashi Sakamoto
  0 siblings, 1 reply; 22+ messages in thread
From: Scott Bahling @ 2018-09-12  7:18 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, ffado-devel


[-- Attachment #1.1: Type: text/plain, Size: 1455 bytes --]

Hi Takashi,

On Mon, 2018-09-10 at 16:59 +0900, Takashi Sakamoto wrote:

<snip>

> As long as I know, the image consists of below information:
>   - pos  0-15: control messages from physical control surface
>   - pos 16-23: analog input level
>   - pos 24-31: digital ADAT input level
>   - pos 32-33: digital S/PDIF input level
>   - pos 34-35: (not cleared yet)
>   - pos 36-43: analog output level
>   - pos 44-64: (not cleared yet)
> 
> When parsing the image, a parser should generate several types of
> events. I prefer to implementing this complicated work in user space
> instead of kernel space. Expecially for pos 16-23/24-31/32-33/36-43, the
> parser should always generate events to notify each of the levels.
> 
> What we should do is to parsing the image and generate events with
> enough consideration of task scheduling and eventing dencity. The parser
> could be implemented as an application of ALSA sequencer.

Thanks for the details. I can work on deciphering the raw control messages.
My current idea is to set up a netlink socket and stream the control data to
user space just for analysis. I will look into ALSA sequencer to see if I
can understand how that could be used.

I noticed that we are able to control the LEDs from the host via the
asynchronous link. Do you you think the faders are also controlled that way
or would that also go via isochronous packets to the FW-1884?

Regards,

Scott

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-09-12  7:18       ` Scott Bahling
@ 2018-09-17 14:36         ` Takashi Sakamoto
  2018-09-24  9:32           ` Scott Bahling
  0 siblings, 1 reply; 22+ messages in thread
From: Takashi Sakamoto @ 2018-09-17 14:36 UTC (permalink / raw)
  To: sbahling; +Cc: alsa-devel, ffado-devel

Hi,

On Sep 12 2018 16:18, Scott Bahling wrote:
>> As long as I know, the image consists of below information:
>>    - pos  0-15: control messages from physical control surface
>>    - pos 16-23: analog input level
>>    - pos 24-31: digital ADAT input level
>>    - pos 32-33: digital S/PDIF input level
>>    - pos 34-35: (not cleared yet)
>>    - pos 36-43: analog output level
>>    - pos 44-64: (not cleared yet)
>>
>> When parsing the image, a parser should generate several types of
>> events. I prefer to implementing this complicated work in user space
>> instead of kernel space. Expecially for pos 16-23/24-31/32-33/36-43, the
>> parser should always generate events to notify each of the levels.
>>
>> What we should do is to parsing the image and generate events with
>> enough consideration of task scheduling and eventing dencity. The parser
>> could be implemented as an application of ALSA sequencer.
> 
> Thanks for the details. I can work on deciphering the raw control messages.
> My current idea is to set up a netlink socket and stream the control data to
> user space just for analysis. I will look into ALSA sequencer to see if I
> can understand how that could be used.

I prepare branches in two remote repositories:
  - 
https://github.com/takaswie/snd-firewire-improve/tree/topic/tascam-userspace 
(a384019c0f78)
  - https://github.com/takaswie/libhinawa/tree/topic/tascam-userspace 
(a5994ec2165f)

Installing the patched driver, you can read the status and control
message in userspace by mmap(2).

Patched libhinawa produces HinawaSndTscm GObject class. This class is
also available with gobject introspection. For example with PyGobject:

```
#/usr/bin/env python3
from time import sleep
import gi
gi.require_version('Hinawa', '2.0')
from gi.repository import Hinawa
unit = Hinawa.SndTscm()
# I assume card number 1 is assigned. Take care of file permission.
unit.open('/dev/snd/hwC1D0')
unit.listen()
while (True):
   for i, frame in enumerate(unit.get_status()):
     print('{0:02d}: {1:08x}'.format(i, frame))
   sleep(0.1)
```

This code print the message to stdout, but not start packet streaming.
You need to start it by ALSA PCM/rawMIDI interfaces, like:
$ aplay -Dplughw:1,0 /dev/urandom

I notice that the branches include patches I introduced[1], with some
minor optimizations to Linux kernel v4.17 or later. The patches are
written just to satisfy investigation work and really ad-hoc ones.

> I noticed that we are able to control the LEDs from the host via the
> asynchronous link. Do you you think the faders are also controlled that way
> or would that also go via isochronous packets to the FW-1884?

The rx isochronous packets from system to the unit include no data to
control the unit[2]. If the faders are movable from system software,
it should be achieved by asynchronous transactions, like blighting
LEDs.

[1] 
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-July/094817.html
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/sound/firewire/tascam/amdtp-tascam.c?h=for-next#n29


Thanks

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-09-17 14:36         ` Takashi Sakamoto
@ 2018-09-24  9:32           ` Scott Bahling
  2018-09-28  3:44             ` Takashi Sakamoto
  0 siblings, 1 reply; 22+ messages in thread
From: Scott Bahling @ 2018-09-24  9:32 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, ffado-devel


[-- Attachment #1.1: Type: text/plain, Size: 2573 bytes --]

On Mon, 2018-09-17 at 23:36 +0900, Takashi Sakamoto wrote:
> Hi,


> I prepare branches in two remote repositories:
>   - 
> 
https://github.com/takaswie/snd-firewire-improve/tree/topic/tascam-userspace
>  
> (a384019c0f78)
>   - https://github.com/takaswie/libhinawa/tree/topic/tascam-userspace 
> (a5994ec2165f)
> 
> Installing the patched driver, you can read the status and control
> message in userspace by mmap(2).
> 
> Patched libhinawa produces HinawaSndTscm GObject class. This class is
> also available with gobject introspection. For example with PyGobject:
> 
> ```
> #/usr/bin/env python3
> from time import sleep
> import gi
> gi.require_version('Hinawa', '2.0')
> from gi.repository import Hinawa
> unit = Hinawa.SndTscm()
> # I assume card number 1 is assigned. Take care of file permission.
> unit.open('/dev/snd/hwC1D0')
> unit.listen()
> while (True):
>    for i, frame in enumerate(unit.get_status()):
>      print('{0:02d}: {1:08x}'.format(i, frame))
>    sleep(0.1)
> ```
> 
> This code print the message to stdout, but not start packet streaming.
> You need to start it by ALSA PCM/rawMIDI interfaces, like:
> $ aplay -Dplughw:1,0 /dev/urandom
> 
> I notice that the branches include patches I introduced[1], with some
> minor optimizations to Linux kernel v4.17 or later. The patches are
> written just to satisfy investigation work and really ad-hoc ones.

Thanks! I finally had some time to try it out.

My test system is running a 4.12 kernel from openSUSE Leap 15.0. I
backported the patches but had to remove your GFP_KERNEL patch for it to
work on my kernel. With the GFP_KERNEL patch, user space was not seeing
updates to the data stream. With it removed, I had a constant update.

The kernel was unstable and the system hard locked frequently with the
patches enabled (with and without the GFP_KERNEL patch). But I was able
to map out all the controller functions to the bits in the first 16
quadlets. I will write up my findings and send them later.

> > I noticed that we are able to control the LEDs from the host via the
> > asynchronous link. Do you you think the faders are also controlled
> > that way
> > or would that also go via isochronous packets to the FW-1884?
> 
> The rx isochronous packets from system to the unit include no data to
> control the unit[2]. If the faders are movable from system software,
> it should be achieved by asynchronous transactions, like blighting
> LEDs.

I guess without an analyzer that will be difficult to track down.

-Scott

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-09-24  9:32           ` Scott Bahling
@ 2018-09-28  3:44             ` Takashi Sakamoto
  2018-09-28 15:28               ` Scott Bahling
  2018-10-03 19:31               ` Scott Bahling
  0 siblings, 2 replies; 22+ messages in thread
From: Takashi Sakamoto @ 2018-09-28  3:44 UTC (permalink / raw)
  To: sbahling; +Cc: alsa-devel, ffado-devel

Hi Scott,

On Sep 24 2018 18:32, Scott Bahling wrote:
> On Mon, 2018-09-17 at 23:36 +0900, Takashi Sakamoto wrote:
>> I prepare branches in two remote repositories:
>>    -
>>
> https://github.com/takaswie/snd-firewire-improve/tree/topic/tascam-userspace
>>   
>> (a384019c0f78)
>>    - https://github.com/takaswie/libhinawa/tree/topic/tascam-userspace
>> (a5994ec2165f)
>>
>> Installing the patched driver, you can read the status and control
>> message in userspace by mmap(2).
>>
>> Patched libhinawa produces HinawaSndTscm GObject class. This class is
>> also available with gobject introspection. For example with PyGobject:
>>
> ...
> Thanks! I finally had some time to try it out.
> 
> My test system is running a 4.12 kernel from openSUSE Leap 15.0. I
> backported the patches but had to remove your GFP_KERNEL patch for it to
> work on my kernel. With the GFP_KERNEL patch, user space was not seeing
> updates to the data stream. With it removed, I had a constant update.
> 
> The kernel was unstable and the system hard locked frequently with the
> patches enabled (with and without the GFP_KERNEL patch). But I was able
> to map out all the controller functions to the bits in the first 16
> quadlets. I will write up my findings and send them later.

I'm sorry but I have applied wrong changes to the remote branch for
kernel patch. It includes three issues:

1. call vfree() to memory object allocated by kmalloc()
2. bring kernel oops due to double free corruption of 'tscm->status'
3. invalid page mapping of 'tscm->status' to process' VMA

I pushed additional three patches. Would you please test with them?

  - 1777454 firewire-tascam: fix kernel oops to call vfree() to memory 
object allocated by kmalloc
  - e11d941 firewire-tascam: fix double free corruption
  - 5e7fef9 firewire-tascam: use one page for mmap(2)ed data

Espeially, when allocating in kernel logical space instead of kernel
virtual space, I should have kept one page for 'tscm->status' because
kmalloc() allocates memory object unaligned to page boundary by kernel
SLAB/SLOB/SLUB allocaters. This is a cause which you cannot see updated
data. A page mapped to process' VMA doesn't include region of
'tscm->status' data correctly.

Again, I'm sorry to lost your time due to this kind of my stupid
codes...

>>> I noticed that we are able to control the LEDs from the host via the
>>> asynchronous link. Do you you think the faders are also controlled
>>> that way
>>> or would that also go via isochronous packets to the FW-1884?
>>
>> The rx isochronous packets from system to the unit include no data to
>> control the unit[2]. If the faders are movable from system software,
>> it should be achieved by asynchronous transactions, like blighting
>> LEDs.
> 
> I guess without an analyzer that will be difficult to track down.

Thanks

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-09-28  3:44             ` Takashi Sakamoto
@ 2018-09-28 15:28               ` Scott Bahling
  2018-10-02  3:16                 ` Takashi Sakamoto
  2018-10-03 19:31               ` Scott Bahling
  1 sibling, 1 reply; 22+ messages in thread
From: Scott Bahling @ 2018-09-28 15:28 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, ffado-devel


[-- Attachment #1.1: Type: text/plain, Size: 2045 bytes --]

Hi Takashi,

On Fri, 2018-09-28 at 12:44 +0900, Takashi Sakamoto wrote:
> Hi Scott,
> 
> On Sep 24 2018 18:32, Scott Bahling wrote:

> > 
> > The kernel was unstable and the system hard locked frequently with the
> > patches enabled (with and without the GFP_KERNEL patch). But I was able
> > to map out all the controller functions to the bits in the first 16
> > quadlets. I will write up my findings and send them later.
> 
> I'm sorry but I have applied wrong changes to the remote branch for
> kernel patch. It includes three issues:
> 
> 1. call vfree() to memory object allocated by kmalloc()
> 2. bring kernel oops due to double free corruption of 'tscm->status'
> 3. invalid page mapping of 'tscm->status' to process' VMA
> 
> I pushed additional three patches. Would you please test with them?
> 
>   - 1777454 firewire-tascam: fix kernel oops to call vfree() to memory 
> object allocated by kmalloc
>   - e11d941 firewire-tascam: fix double free corruption
>   - 5e7fef9 firewire-tascam: use one page for mmap(2)ed data
> 
> Espeially, when allocating in kernel logical space instead of kernel
> virtual space, I should have kept one page for 'tscm->status' because
> kmalloc() allocates memory object unaligned to page boundary by kernel
> SLAB/SLOB/SLUB allocaters. This is a cause which you cannot see updated
> data. A page mapped to process' VMA doesn't include region of
> 'tscm->status' data correctly.
> 
> Again, I'm sorry to lost your time due to this kind of my stupid
> codes...

No worries! It did not waste too much time at all. I assumed it's just
quick test code anyway. I was able to get the button press events and
control levels mapped to the quadlet bits which was my goal (I'm still
documenting my findings).

I will test your new patches this weekend and let you know if it works
better.

Are you still thinking to try to make mmap the solution to pass the
control data to userspace, or do we need to find another method in the
long run?

Regards,

Scott
> 

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-09-28 15:28               ` Scott Bahling
@ 2018-10-02  3:16                 ` Takashi Sakamoto
  2018-10-03 20:37                   ` Scott Bahling
  2018-10-07 14:11                   ` Takashi Sakamoto
  0 siblings, 2 replies; 22+ messages in thread
From: Takashi Sakamoto @ 2018-10-02  3:16 UTC (permalink / raw)
  To: sbahling; +Cc: alsa-devel, ffado-devel

Hi Scott,

On Sep 29 2018 00:28, Scott Bahling wrote:
> On Fri, 2018-09-28 at 12:44 +0900, Takashi Sakamoto wrote:
> Are you still thinking to try to make mmap the solution to pass the
> control data to userspace, or do we need to find another method in the
> long run?

The latter is better.

The patchset to map page frame for status and control image is just for
our work to investigate. Practically, it includes several issues:

1. Due to scheduling granularity of user processes against interval to
update the image, applications in the processes can fail to pick up
events on the image.

2. Recently, bare embedded board with ARM cores becomes pupular. Some
of them have PCIe buses. It's better for users to use units on
IEEE 1394 bus, however ARM is known as cache incoherent architecture.
In the architecture, page frame mapping can bring unexpected results.
(See a conditional macro[1].)

For these reasons, we need to find another solution based on
synchronous procedure such like read(2)/ioctl(2). For this direction,
we need to care several items:

1. The status and control image includes several types of data;
bitflags for physical controls of device surface, level meters
and so on. Although it's acceptable to fail picking up values for
level meters, it's not for physical controls.

2. TASCAM FireWire series includes below models and the layout of
bitflags differs depending on them:
  - FW-1884
  - FW-1804
  - FW-1082
(FE-8 has not been investigated yet.)
It cost expensive to handle such differences by kernel land in
development/maintenance POV.

3. In current implementation of ALSA firewire-tascam driver,
demultiplexing of OHCI 1394 isochronous packet runs on
software IRQ context. Total time of software IRQ context is
shared in system wide, thus it's better to reduce load as
much as possible even if software IRQ run on schedulable tasks
(threadedirq).

I have an idea to invervene them:

1. For control events, in kernel land, driver module detects
changes of set of bitflags for physical controls, then queue
events to tell the change to userspace applications
(e.g. poll(2)). The queued events include information about
changed bitflags (e.g. a shape of u32 data). Userspace
applications execute read(2) then get the bitflags, then parse
it and emit userspace event by ports in ALSA sequencer
subsystem.
The driver and userspace application should pay enough
attention to share the queue. The driver can drop the oldest
queued events if the queue is full.

2. For level meter, in kernel land, driver module caches the
recent value. Userspace applications execute ioctl(2) with
unique command (You can see this kind of commands in
'include/uapi/sound/firwire.h').

However, as long as I note[2], the purpose of some quadlets in the
image are not still identified:

"Quadlet 00-15 show control messages. Quadlet 16-23 show analog input
level. Quadlet 24-31 shows digital ADAT input level. Quadlet 32-33
shows digital S/PDIF input level. Quadlet 34-35 is unknown. Quadlet
36-43 shows analog output level. The other quadlets are unknown."

We need further investigation to clear the unknown fields as much as
possible to add more codes in ALSA kernel land.

[1] 
https://github.com/takaswie/snd-firewire-improve/blob/topic/tascam-userspace/sound/firewire/tascam/tascam-hwdep.c#L192
[2] 
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-July/094817.html


Thanks

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-09-28  3:44             ` Takashi Sakamoto
  2018-09-28 15:28               ` Scott Bahling
@ 2018-10-03 19:31               ` Scott Bahling
  1 sibling, 0 replies; 22+ messages in thread
From: Scott Bahling @ 2018-10-03 19:31 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, ffado-devel

Hi Takashi,

On Fri, 2018-09-28 at 12:44 +0900, Takashi Sakamoto wrote:
> Hi Scott,
> 
> On Sep 24 2018 18:32, Scott Bahling wrote:

...

> > 
> > The kernel was unstable and the system hard locked frequently with the
> > patches enabled (with and without the GFP_KERNEL patch). But I was able
> > to map out all the controller functions to the bits in the first 16
> > quadlets. I will write up my findings and send them later.
> 
> I'm sorry but I have applied wrong changes to the remote branch for
> kernel patch. It includes three issues:
> 
> 1. call vfree() to memory object allocated by kmalloc()
> 2. bring kernel oops due to double free corruption of 'tscm->status'
> 3. invalid page mapping of 'tscm->status' to process' VMA
> 
> I pushed additional three patches. Would you please test with them?

MMAP works fine and stable with the three additional patches.

-Scott

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-10-02  3:16                 ` Takashi Sakamoto
@ 2018-10-03 20:37                   ` Scott Bahling
  2018-10-06  9:07                     ` Takashi Sakamoto
  2018-10-07 14:11                   ` Takashi Sakamoto
  1 sibling, 1 reply; 22+ messages in thread
From: Scott Bahling @ 2018-10-03 20:37 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, ffado-devel

On Tue, 2018-10-02 at 12:16 +0900, Takashi Sakamoto wrote:
> Hi Scott,
> 

...

> I have an idea to invervene them:
> 
> 1. For control events, in kernel land, driver module detects
> changes of set of bitflags for physical controls, then queue
> events to tell the change to userspace applications
> (e.g. poll(2)). The queued events include information about
> changed bitflags (e.g. a shape of u32 data). Userspace
> applications execute read(2) then get the bitflags, then parse
> it and emit userspace event by ports in ALSA sequencer
> subsystem.
> The driver and userspace application should pay enough
> attention to share the queue. The driver can drop the oldest
> queued events if the queue is full.
> 
> 2. For level meter, in kernel land, driver module caches the
> recent value. Userspace applications execute ioctl(2) with
> unique command (You can see this kind of commands in
> 'include/uapi/sound/firwire.h').
> 
> However, as long as I note[2], the purpose of some quadlets in the
> image are not still identified:
> 
> "Quadlet 00-15 show control messages. Quadlet 16-23 show analog input
> level. Quadlet 24-31 shows digital ADAT input level. Quadlet 32-33
> shows digital S/PDIF input level. Quadlet 34-35 is unknown.

On my FW-1884 34/35 is analog level of output 1/2. The "Monitor" level
pot (as well as the master fader if enabled) control the level so if
they are set at 0, then you will not see any signal on the 34/35.

> Quadlet
> 36-43 shows analog output level.

36-41 = analog outputs 3-8

> The other quadlets are unknown."

52: shows the current sample rate setting
    010101xx = 44.1k
    020102xx = 48k
    810181xx = 88.2k
    820182xx = 96k

52: Byte 1 shows the current clock source
    0x01 = Internal
    0x02 = Word Clock
    0x03 = Digital In
    0x04 = ADAT

54-55 appears to be the level of the internal "Monitor Mix".
57-58 appears to be the level of the stereo mix of the analog inputs

59: Byte 1 indicates what is routed to the Monitor Mix
    0x01 = PCM Stream from computer
    0x02 = Analog Inputs
    0x03 = Both

> We need further investigation to clear the unknown fields as much as
> possible to add more codes in ALSA kernel land.

Above is as far as I have gotten with the unknown fields.

On the asynchronous side, I have mapped out the LED codes to the LEDs
on the FW-1884 and discovered the registers for controlling the faders.

-Scott

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-10-03 20:37                   ` Scott Bahling
@ 2018-10-06  9:07                     ` Takashi Sakamoto
  2018-10-07 11:32                       ` Scott Bahling
  0 siblings, 1 reply; 22+ messages in thread
From: Takashi Sakamoto @ 2018-10-06  9:07 UTC (permalink / raw)
  To: Scott Bahling; +Cc: alsa-devel, ffado-devel

Hi Scott,

On Oct 4 2018 05:37, Scott Bahling wrote:
> On Tue, 2018-10-02 at 12:16 +0900, Takashi Sakamoto wrote:
>> I have an idea to invervene them:
>>
>> 1. For control events, in kernel land, driver module detects
>> changes of set of bitflags for physical controls, then queue
>> events to tell the change to userspace applications
>> (e.g. poll(2)). The queued events include information about
>> changed bitflags (e.g. a shape of u32 data). Userspace
>> applications execute read(2) then get the bitflags, then parse
>> it and emit userspace event by ports in ALSA sequencer
>> subsystem.
>> The driver and userspace application should pay enough
>> attention to share the queue. The driver can drop the oldest
>> queued events if the queue is full.
>>
>> 2. For level meter, in kernel land, driver module caches the
>> recent value. Userspace applications execute ioctl(2) with
>> unique command (You can see this kind of commands in
>> 'include/uapi/sound/firwire.h').
>>
>> However, as long as I note[2], the purpose of some quadlets in the
>> image are not still identified:
>>
>> "Quadlet 00-15 show control messages. Quadlet 16-23 show analog input
>> level. Quadlet 24-31 shows digital ADAT input level. Quadlet 32-33
>> shows digital S/PDIF input level. Quadlet 34-35 is unknown.
> 
> On my FW-1884 34/35 is analog level of output 1/2. The "Monitor" level
> pot (as well as the master fader if enabled) control the level so if
> they are set at 0, then you will not see any signal on the 34/35.
> 
>> Quadlet
>> 36-43 shows analog output level.
> 
> 36-41 = analog outputs 3-8
> 
>> The other quadlets are unknown."
> 
> 52: shows the current sample rate setting
>      010101xx = 44.1k
>      020102xx = 48k
>      810181xx = 88.2k
>      820182xx = 96k
> 
> 52: Byte 1 shows the current clock source
>      0x01 = Internal
>      0x02 = Word Clock
>      0x03 = Digital In
>      0x04 = ADAT
> 
> 54-55 appears to be the level of the internal "Monitor Mix".
> 57-58 appears to be the level of the stereo mix of the analog inputs
> 
> 59: Byte 1 indicates what is routed to the Monitor Mix
>      0x01 = PCM Stream from computer
>      0x02 = Analog Inputs
>      0x03 = Both
> 
>> We need further investigation to clear the unknown fields as much as
>> possible to add more codes in ALSA kernel land.
> 
> Above is as far as I have gotten with the unknown fields.
> 
> On the asynchronous side, I have mapped out the LED codes to the LEDs
> on the FW-1884 and discovered the registers for controlling the faders.

Thanks for your report. I have additional information from today
investigation. In detail, please read the end of this message.

I think it reasonable to take kernel-land driver emits events for
quadlet 05-15 to userspace applications, then let SndTscm object in
libhinawa. (Extra care is required for the value of monitor-knob).
Additionally, SndTscm object produces API to retrieve current value
of fader, knob and so on.

Anyway, initial value should be reported to userspace, mmm...

======== 8< --------

00: fader2 fader1
01: fader4 fader3
02: fader6 fader5
03: fader8 fader7
04: solo-knob fader9(=master)

The value of fader is between 0x0000 and 0x03ff (2byte).
The value of solo-knob (FW-1884 only) is between 0x0000 and
0x03ff (2byte).

05: op-mode+fader-sense monitor-knob

op-mode is 7 bits in MSB side.
  - 0x00: computer mode
  - 0xfe: midi-ctl/mon-mix modes

The value of fader-sense consists of bitflags in which
a bit becomes zero during user touches corresponding fader.
This is next 9 bits in MSB side.
  - 0x0100: for fader 9 (=master)
  - 0x0080: for fader 8
    ...
  - 0x0001: for fader 1

The value of monitor-knob is between 0x0000 and 0x03ff.
But the lowest bits becomes frequently without handy operation.

06: bitflags
07: bitflags
08: bitflags
09: bitflags

These bitflags consists two states:
  - During corresponding button is pressed, bit becomes zero.
  - some sets of 2 bits represent current value of corresponding
    knob.
The position is largely different depending on FW-1884/FW-1082.

10: unknown unknown
11: unknown unknown
12: unknown unknown
13: unknown unknown
14: unknown unknown

15: dial-value unknown

The value of dial is between 0x0000 and 0xffff, accumulated clockwise.
At overflow it resets to 0x0000.

16: analog-in-1
17: analog-in-2
18: analog-in-3
19: analog-in-4
20: analog-in-5
21: analog-in-6
22: analog-in-7
23: analog-in-8
24: adat-in-1
25: adat-in-2
26: adat-in-3
27: adat-in-4
28: adat-in-5
29: adat-in-6
30: adat-in-7
31: adat-in-8
32: s/pdif-in-1
33: s/pdif-in-2
34: analog-out-1
35: analog-out-2
36: analog-out-3
37: analog-out-4
38: analog-out-5
39: analog-out-6
40: analog-out-7
41: analog-out-8
42: adat-out-1
43: adat-out-2
44: adat-out-3
45: adat-out-4
46: adat-out-5
47: adat-out-6
48: adat-out-7
49: adat-out-8

The value of level is between 0x00000000 and 0x7fffff00.

50: (unknown)
51: (unknown)

I expect them for spdif-out-1/2 but actually they're not.

52: clock-status clock-config

As Scott investigated, but configuration of clock source
is not necessarily effective in bits of clock-status.

53: (unknown)
54: monitor-mix-1 (enabled at INPUT/BOTH modes)
55: monitor-mix-2 (enabled at INPUT/BOTH modes)
56: (unknown)
57: analog-in-mix-1
58: analog-in-mix-2

59: 0:COMPUTER, 1:INPUTS, 2:BOTH

At monitor mix mode, the above selections are available.

60: (unknown)
61: (unknown)
62: (unknown)
63: (unknown)

======== 8< --------

Thanks

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-10-06  9:07                     ` Takashi Sakamoto
@ 2018-10-07 11:32                       ` Scott Bahling
  0 siblings, 0 replies; 22+ messages in thread
From: Scott Bahling @ 2018-10-07 11:32 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, ffado-devel

Hi Takashi,

On Sat, 2018-10-06 at 18:07 +0900, Takashi Sakamoto wrote:
> Hi Scott,

> 
> Thanks for your report. I have additional information from today
> investigation. In detail, please read the end of this message.

Your findings match mine.

> I think it reasonable to take kernel-land driver emits events for
> quadlet 05-15 to userspace applications, then let SndTscm object in
> libhinawa. (Extra care is required for the value of monitor-knob).
> Additionally, SndTscm object produces API to retrieve current value
> of fader, knob and so on.
> 
> Anyway, initial value should be reported to userspace, mmm...
> 
> ======== 8< --------
> 
> 00: fader2 fader1
> 01: fader4 fader3
> 02: fader6 fader5
> 03: fader8 fader7
> 04: solo-knob fader9(=master)
> 
> The value of fader is between 0x0000 and 0x03ff (2byte).
> The value of solo-knob (FW-1884 only) is between 0x0000 and
> 0x03ff (2byte).
> 
> 05: op-mode+fader-sense monitor-knob
> 
> op-mode is 7 bits in MSB side.
>   - 0x00: computer mode
>   - 0xfe: midi-ctl/mon-mix modes
> 
> The value of fader-sense consists of bitflags in which
> a bit becomes zero during user touches corresponding fader.
> This is next 9 bits in MSB side.
>   - 0x0100: for fader 9 (=master)
>   - 0x0080: for fader 8
>     ...
>   - 0x0001: for fader 1
> 
> The value of monitor-knob is between 0x0000 and 0x03ff.
> But the lowest bits becomes frequently without handy operation.

I notice the same unstable low bits with the Solo knob as well.

> 06: bitflags
> 07: bitflags
> 08: bitflags
> 09: bitflags
> 
> These bitflags consists two states:
>   - During corresponding button is pressed, bit becomes zero.
>   - some sets of 2 bits represent current value of corresponding
>     knob.

The 2 bits are actually the raw encoder bits from the knobs. This is a
"quadrature output" [1] showing the velocity and direction of the
encoder as it spins.  This bits can also be unstable and fluctuate when
there is no movement depending on the position of the encoder knob. If
used, some form of filtering would need to be done to avoid triggering
false updates.

The FW-1884 also tracks the absolute position of the encoders as 16 bit
values similar to the dial-value. They are tracked at quadlets 10-15.

> The position is largely different depending on FW-1884/FW-1082.

I have all the buttons of the FW-1884 mapped out to the bitflags. See
the end of this email.

> 10: unknown unknown
> 11: unknown unknown
> 12: unknown unknown
> 13: unknown unknown
> 14: unknown unknown

10: encoder2 encoder1
11: encoder4 encoder3
12: encoder6 encoder5
13: encoder8 encoder7
14: Freq     Gain

> 15: dial-value unknown

15: dial-value Q

> The value of dial is between 0x0000 and 0xffff, accumulated clockwise.
> At overflow it resets to 0x0000.
> 
> 16: analog-in-1
> 17: analog-in-2
> 18: analog-in-3
> 19: analog-in-4
> 20: analog-in-5
> 21: analog-in-6
> 22: analog-in-7
> 23: analog-in-8
> 24: adat-in-1
> 25: adat-in-2
> 26: adat-in-3
> 27: adat-in-4
> 28: adat-in-5
> 29: adat-in-6
> 30: adat-in-7
> 31: adat-in-8
> 32: s/pdif-in-1
> 33: s/pdif-in-2
> 34: analog-out-1
> 35: analog-out-2
> 36: analog-out-3
> 37: analog-out-4
> 38: analog-out-5
> 39: analog-out-6
> 40: analog-out-7
> 41: analog-out-8
> 42: adat-out-1
> 43: adat-out-2
> 44: adat-out-3
> 45: adat-out-4
> 46: adat-out-5
> 47: adat-out-6
> 48: adat-out-7
> 49: adat-out-8
> 
> The value of level is between 0x00000000 and 0x7fffff00.
> 
> 50: (unknown)
> 51: (unknown)
> 
> I expect them for spdif-out-1/2 but actually they're not.
> 
> 52: clock-status clock-config
> 
> As Scott investigated, but configuration of clock source
> is not necessarily effective in bits of clock-status.
> 
> 53: (unknown)
> 54: monitor-mix-1 (enabled at INPUT/BOTH modes)
> 55: monitor-mix-2 (enabled at INPUT/BOTH modes)
> 56: (unknown)
> 57: analog-in-mix-1
> 58: analog-in-mix-2
> 
> 59: 0:COMPUTER, 1:INPUTS, 2:BOTH
> 
> At monitor mix mode, the above selections are available.
> 
> 60: (unknown)
> 61: (unknown)
> 62: (unknown)
> 63: (unknown)
> 
> ======== 8< --------

Button Mapping:

Quadlet 6
=========

Bit Button
--- --------------
  1 unknown
  2 unknown 
  3 unknown
  4 unknown

  5 unknown
  6 unknown
  7 unknown
  8 unknown

  9 unknown
 10 unknown
 11 unknown
 12 unknown

 13 unknown
 14 unknown
 15 unknown
 16 unknown

 17 Sel Channel 1
 18 Sel Channel 2
 19 Sel Channel 3
 20 Sel Channel 4

 21 Sel Channel 5
 22 Sel Channel 6
 23 Sel Channel 7
 24 Sel Channel 8

 25 Solo Channel 1
 26 Solo Channel 2
 27 Solo Channel 3
 28 Solo Channel 4

 29 Solo Channel 5
 30 Solo Channel 6
 31 Solo Channel 7
 32 Solo Channel 8


Quadlet 7
=========

Bit Button
--- --------------
  1 Mute Channel 1
  2 Mute Channel 2
  3 Mute Channel 3
  4 Mute Channel 4

  5 Mute Channel 5
  6 Mute Channel 6
  7 Mute Channel 7
  8 Mute Channel 8

  9 Aux5
 10 Aux7
 11 Aux6
 12 Aux8

 13 unknown
 14 unknown
 15 unknown
 16 unknown

 17 Flip
 18 Aux1
 19 Aux3
 20 Pan

 21 Aux2
 22 Aux4
 23 unknown 
 24 unknown 

 25 Control Panel
 26 Save
 27 All Save
 28 Marker

 29 Cut
 30 Copy
 31 Alt
 32 Shift


Quadlet 8
=========

Bit Button
--- --------------
  1 Revert
  2 Clr Solo
  3 Loop
  4 Del

  5 Paste
  6 Undo
  7 Ctrl
  8 unknown

  9 Foot Switch
 10 unknown
 11 unknown
 12 unknown

 13 unknown
 14 unknown
 15 unknown
 16 unknown

 17 unknown
 18 unknown
 19 unknown
 20 unknown

 21 unknown
 22 unknown
 23 unknown
 24 unknown 

 25 unknown
 26 unknown
 27 unknown
 28 unknown

 29 Clock
 30 Route
 31 unknown
 32 unknown


Quadlet 9
=========

Bit Button
--- --------------
  1 F7
  2 F8
  3 F9
  4 F10

  5 Read
  6 Wrt
  7 Tch
  8 Latch

  9 High
 10 Hi-Mid
 11 Hi-Low
 12 Low

 13 Up
 14 Left
 15 Down
 16 Right

 17 Rec
 18 Nudge Left
 19 Nudge Right
 20 Bank Left

 21 Bank Right
 22 Locate Left
 23 Locate Right
 24 Shtl

 25 Set
 26 In
 27 Out
 28 REW

 29 F.FWD
 30 STOP
 31 PLAY
 32 REC

===============


[1] https://en.wikipedia.org/wiki/Incremental_encoder

-Scott

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-10-02  3:16                 ` Takashi Sakamoto
  2018-10-03 20:37                   ` Scott Bahling
@ 2018-10-07 14:11                   ` Takashi Sakamoto
  2018-10-12  8:12                     ` Scott Bahling
  1 sibling, 1 reply; 22+ messages in thread
From: Takashi Sakamoto @ 2018-10-07 14:11 UTC (permalink / raw)
  To: sbahling; +Cc: alsa-devel, ffado-devel

Hi Scott,

On Oct 2 2018 12:16, Takashi Sakamoto wrote:
> I have an idea to invervene them:
> 
> 1. For control events, in kernel land, driver module detects
> changes of set of bitflags for physical controls, then queue
> events to tell the change to userspace applications
> (e.g. poll(2)). The queued events include information about
> changed bitflags (e.g. a shape of u32 data). Userspace
> applications execute read(2) then get the bitflags, then parse
> it and emit userspace event by ports in ALSA sequencer
> subsystem.
> The driver and userspace application should pay enough
> attention to share the queue. The driver can drop the oldest
> queued events if the queue is full.
> 
> 2. For level meter, in kernel land, driver module caches the
> recent value. Userspace applications execute ioctl(2) with
> unique command (You can see this kind of commands in
> 'include/uapi/sound/firwire.h').

I prepare two remote branch as 'topic/tascam-userspace-take2' for kernel
driver[1] and libhinawa[2].

The patches for kernel driver adds two features below to firewire-tascam 
module:
  - SNDRV_FIREWIRE_IOCTL_TASCAM_STATUS ioctl command
  - 'struct snd_firewire_event_tascam_ctl' event notification

The patches for libhinawa adds two features below for g-i modules:
  - hinawa_snd_tscm_get_status() method
  - 'control' GObject signal

For example[3]:

```
$ cat test.py
#!/usr/bin/env python3

from sys import exit
from time import sleep

import gi
gi.require_version('Hinawa', '2.0')
from gi.repository import Hinawa

unit = Hinawa.SndTscm()
unit.open('/dev/snd/hwC2D0')
unit.listen()

req = Hinawa.FwReq()

def handle_control(self, index, flags):
     print('{0:02d}: {1:08x}'.format(index, flags))
     # data = bytearray(4)
     # You can check the flags and bright LED with proper values
     # in the data array.
     # print(req.write(self, 0xffff00000404, data))

unit.connect('control', handle_control)

while (True):
   msgs = unit.get_status()
   for i in range(len(msgs) // 2):
       left = i
       right = i + len(msgs) // 2
       print('{0:02d}: {1:08x}, {2:02d}: {3:08x}'.format(left, 
msgs[left], right, msgs[right]))
   sleep(0.1)
```

When running any scripts with your local build of libhinawa,
it's useful to set LD_LIBRARY_PATH/GI_TYPELIB_PATH properly.

```
$ export 
LD_LIBRARY_PATH="/home/username/git/libhinawa/build/src:${LD_LIBRARY_PATH}"
$ export 
GI_TYPELIB_PATH="/home/username/git/libhinawa/build/src:${GI_TYPELIB_PATH}"
$ ./sample.py
...
```

When starting packet streaming and operating control surface, you can
see dump with index and bitflags additionally to status dump.

```
...
06: fffeffff
06: ffffffff
06: feffffff
...
```

As I told, kernel driver emits events corresponding to quadlet 05-15.
If there's insufficiency, please inform it to me. If all things look
well, I'll submit patches for next merge window but .

[1] 
https://github.com/takaswie/snd-firewire-improve/tree/topic/tascam-userspace-take2
[2] https://github.com/takaswie/libhinawa/tree/topic/tascam-userspace-take2
[3] https://gist.github.com/takaswie/669fd4254c7b9fd631762fc61ac931f5


Thanks

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-10-07 14:11                   ` Takashi Sakamoto
@ 2018-10-12  8:12                     ` Scott Bahling
  2018-10-13 10:40                       ` Takashi Sakamoto
  0 siblings, 1 reply; 22+ messages in thread
From: Scott Bahling @ 2018-10-12  8:12 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, ffado-devel

Hi Takashi,

On Sun, 2018-10-07 at 23:11 +0900, Takashi Sakamoto wrote:
> Hi Scott,
> 
> On Oct 2 2018 12:16, Takashi Sakamoto wrote:
> > I have an idea to invervene them:
> > 
> > 1. For control events, in kernel land, driver module detects
> > changes of set of bitflags for physical controls, then queue
> > events to tell the change to userspace applications
> > (e.g. poll(2)). The queued events include information about
> > changed bitflags (e.g. a shape of u32 data). Userspace
> > applications execute read(2) then get the bitflags, then parse
> > it and emit userspace event by ports in ALSA sequencer
> > subsystem.
> > The driver and userspace application should pay enough
> > attention to share the queue. The driver can drop the oldest
> > queued events if the queue is full.
> > 
> > 2. For level meter, in kernel land, driver module caches the
> > recent value. Userspace applications execute ioctl(2) with
> > unique command (You can see this kind of commands in
> > 'include/uapi/sound/firwire.h').
> 
> I prepare two remote branch as 'topic/tascam-userspace-take2' for
> kernel
> driver[1] and libhinawa[2].
> 
> The patches for kernel driver adds two features below to firewire-tascam 
> module:
>   - SNDRV_FIREWIRE_IOCTL_TASCAM_STATUS ioctl command
>   - 'struct snd_firewire_event_tascam_ctl' event notification
> 
> The patches for libhinawa adds two features below for g-i modules:
>   - hinawa_snd_tscm_get_status() method
>   - 'control' GObject signal
> 
> For example[3]:
> 
> ```
> $ cat test.py
> #!/usr/bin/env python3
> 
> from sys import exit
> from time import sleep
> 
> import gi
> gi.require_version('Hinawa', '2.0')
> from gi.repository import Hinawa
> 
> unit = Hinawa.SndTscm()
> unit.open('/dev/snd/hwC2D0')
> unit.listen()
> 
> req = Hinawa.FwReq()
> 
> def handle_control(self, index, flags):
>      print('{0:02d}: {1:08x}'.format(index, flags))
>      # data = bytearray(4)
>      # You can check the flags and bright LED with proper values
>      # in the data array.
>      # print(req.write(self, 0xffff00000404, data))
> 
> unit.connect('control', handle_control)
> 
> while (True):
>    msgs = unit.get_status()
>    for i in range(len(msgs) // 2):
>        left = i
>        right = i + len(msgs) // 2
>        print('{0:02d}: {1:08x}, {2:02d}: {3:08x}'.format(left, 
> msgs[left], right, msgs[right]))
>    sleep(0.1)
> ```
> 
> When running any scripts with your local build of libhinawa,
> it's useful to set LD_LIBRARY_PATH/GI_TYPELIB_PATH properly.
> 
> ```
> $ export 
> LD_LIBRARY_PATH="/home/username/git/libhinawa/build/src:${LD_LIBRARY_PATH}"
> $ export 
> GI_TYPELIB_PATH="/home/username/git/libhinawa/build/src:${GI_TYPELIB_PATH}"
> $ ./sample.py
> ...
> ```
> 
> When starting packet streaming and operating control surface, you can
> see dump with index and bitflags additionally to status dump.
> 
> ```
> ...
> 06: fffeffff
> 06: ffffffff
> 06: feffffff
> ...
> ```
> 
> As I told, kernel driver emits events corresponding to quadlet 05-15.
> If there's insufficiency, please inform it to me. If all things look
> well, I'll submit patches for next merge window but .


I have tested the branches and it works well. I was able to create
daemon in python to interface the FW-1884 with Ardour via the OSC
protocol. That's working reliably.

I had to enable quadlets 00-04 since they contain the fader control
values. I masked out the solo control values in the same way as the
monitor control values since they continuously fluctuate in the same
way.

diff -Nrup a/sound/firewire/tascam/amdtp-tascam.c b/sound/firewire/tascam/amdtp-tascam.c
--- a/sound/firewire/tascam/amdtp-tascam.c	2018-10-09 09:49:54.693197299 +0200
+++ b/sound/firewire/tascam/amdtp-tascam.c	2018-10-11 10:55:14.865475143 +0200
@@ -130,8 +130,10 @@ static void read_status_messages(struct
 		index = be32_to_cpu(buffer[0]) % SNDRV_FIREWIRE_TASCAM_STATUS_COUNT;
 		quad = buffer[s->data_block_quadlets - 1];
 
-		if (index >= 5 && index <= 15) {
-			if (index == 5)
+		if (index <= 15) {
+			if (index == 4)
+				mask = 0xffff0000;
+			else if (index == 5)
 				mask = 0x0000ffff;
 			else
 				mask = 0xffffffff;


While testing, noticed that the encoder bits on quadlet 06 do not get
updated reliably if the encoder knob is moved quickly. The transitions
on those bits happen quickly and some of the bit state changes get
lost. I switched to using the absolute encoder values on quadlets 10-15 
which works reliably.

Thanks!

Scott

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-10-12  8:12                     ` Scott Bahling
@ 2018-10-13 10:40                       ` Takashi Sakamoto
  2018-10-14 19:09                         ` Scott Bahling
  0 siblings, 1 reply; 22+ messages in thread
From: Takashi Sakamoto @ 2018-10-13 10:40 UTC (permalink / raw)
  To: Scott Bahling; +Cc: alsa-devel, ffado-devel

Hi Scott,

On Oct 12 2018 17:12, Scott Bahling wrote:
> I have tested the branches and it works well. I was able to create
> daemon in python to interface the FW-1884 with Ardour via the OSC
> protocol. That's working reliably.

Good to hear ;)

> I had to enable quadlets 00-04 since they contain the fader control
> values. I masked out the solo control values in the same way as the
> monitor control values since they continuously fluctuate in the same
> way.

For FW-1082 these quadlets includes value of movable fader as well,
however unlike FW-1884 they have fluctuate quirk. So I think it
reasonable for us to program this module to ignore them.

Instead, let us program applications so that they call
'hinawa_snd_tscm_get_status()' periodically to get current value of
these faders between touch and untouch event on quadlet 05? This
take applications to consume CPU time more efficiently than handling
many events.

> While testing, noticed that the encoder bits on quadlet 06 do not get
> updated reliably if the encoder knob is moved quickly. The transitions
> on those bits happen quickly and some of the bit state changes get
> lost. I switched to using the absolute encoder values on quadlets 10-15
> which works reliably.

Yep. We need to ignore some bits in quadlet 06 and 08 for value of the
knobs.


Well, as a result, ALSA firewire-tascam driver handles 'edge-trigge'
events except for jog wheel and knobs. For this kind of event, it's
useful to notify before/after value when emitting notification. So I'd
like to change structure passed to UAPI so that:

  struct snd_firewire_tascam_control {
         unsigned int index;
-       __u32 flags;
+       __u32 before;
+       __u32 after;
  };

Would I request your opinion? You can see patches here:
  - 
https://github.com/takaswie/snd-firewire-improve/tree/topic/tascam-userspace-take3
  - https://github.com/takaswie/libhinawa/tree/topic/tascam-userspace-take3

You can receive 'control' event of 'HinawaSndTscm' GObject with these
two values:

```
def handle_control(self, index, before, after):
     print('{0:02d}: {1:08x} -> {2:08x}, {3:08x}'.format(index, before, 
after, after ^ before))
```

idx before      after     XOR
09: ffffffff -> 7fffffff, 80000000
09: 7fffffff -> ffffffff, 80000000
06: ffffffff -> ff7fffff, 00800000
06: ff7fffff -> ffffffff, 00800000
07: ffffffff -> ffffff7f, 00000080
07: ffffff7f -> ffffffff, 00000080
10: 00000000 -> 00000001, 00000001 (knob)
10: 00000001 -> 00000002, 00000003 (knob)
15: 003f000c -> 003f000b, 00000007 (jog wheel)
15: 003f000b -> 003f000a, 00000001 (job wheel)
14: 00100005 -> 00110005, 00010000 (knob)
09: ffffffff -> fffffbff, 00000400
09: fffffbff -> ffffffff, 00000400
05: 01ff00a6 -> 017f00a6, 00800000
05: 017f00a6 -> 01ff00a6, 00800000


Thanks

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-10-13 10:40                       ` Takashi Sakamoto
@ 2018-10-14 19:09                         ` Scott Bahling
  2018-10-22 11:47                           ` Scott Bahling
  0 siblings, 1 reply; 22+ messages in thread
From: Scott Bahling @ 2018-10-14 19:09 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, ffado-devel

On Sat, 2018-10-13 at 19:40 +0900, Takashi Sakamoto wrote:

> For FW-1082 these quadlets includes value of movable fader as well,
> however unlike FW-1884 they have fluctuate quirk. So I think it
> reasonable for us to program this module to ignore them.

Ah, understood.

> Instead, let us program applications so that they call
> 'hinawa_snd_tscm_get_status()' periodically to get current value of
> these faders between touch and untouch event on quadlet 05? This
> take applications to consume CPU time more efficiently than handling
> many events.

That should work.

...
> Well, as a result, ALSA firewire-tascam driver handles 'edge-trigge'
> events except for jog wheel and knobs. For this kind of event, it's
> useful to notify before/after value when emitting notification. So I'd
> like to change structure passed to UAPI so that:
> 
>   struct snd_firewire_tascam_control {
>          unsigned int index;
> -       __u32 flags;
> +       __u32 before;
> +       __u32 after;
>   };
> 
> Would I request your opinion? You can see patches here:
>   - 
> https://github.com/takaswie/snd-firewire-improve/tree/topic/tascam-userspace-take3
>   - https://github.com/takaswie/libhinawa/tree/topic/tascam-userspace-take3
> 
> You can receive 'control' event of 'HinawaSndTscm' GObject with these
> two values:
> 
...

Sounds reasonable. For state change detection, I currently store the
"before" state of each control in my application. Your new driver
implementation will mean not needing to maintain my own state
structures for that purpose.

I will try it out in the next few days.

Thanks!

Scott

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-10-14 19:09                         ` Scott Bahling
@ 2018-10-22 11:47                           ` Scott Bahling
  2018-10-30  9:34                             ` Takashi Sakamoto
  0 siblings, 1 reply; 22+ messages in thread
From: Scott Bahling @ 2018-10-22 11:47 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, ffado-devel

Hi Takashi,

On Sun, 2018-10-14 at 21:09 +0200, Scott Bahling wrote:
> On Sat, 2018-10-13 at 19:40 +0900, Takashi Sakamoto wrote:
> 
> > For FW-1082 these quadlets includes value of movable fader as well,
> > however unlike FW-1884 they have fluctuate quirk. So I think it
> > reasonable for us to program this module to ignore them.
> 
> Ah, understood.
> 
> > Instead, let us program applications so that they call
> > 'hinawa_snd_tscm_get_status()' periodically to get current value of
> > these faders between touch and untouch event on quadlet 05? This
> > take applications to consume CPU time more efficiently than handling
> > many events.
> 
> That should work.

I have tried this, but the endianness of the status bits passed via this
method seems to be wrong.

I noticed in your latest kernel code[1], that you don't convert the firewire
data to local CPU endian when storing in the "after" variable which ends up
getting pushed into the status structure as big endian:


    after = buffer[s->data_block_quadlets - 1];
...
...
...
    tscm->status[index] = after;


Shouldn't that be:

    after = be32_to_cpu(buffer[s->data_block_quadlets - 1]);

which also would remove the need for later conversions in that block of
code?

I have a tested patch which I will send as a pull request once github is
functioning again.

> ...
> > Well, as a result, ALSA firewire-tascam driver handles 'edge-trigge'
> > events except for jog wheel and knobs. For this kind of event, it's
> > useful to notify before/after value when emitting notification. So I'd
> > like to change structure passed to UAPI so that:
> > 
> >   struct snd_firewire_tascam_control {
> >          unsigned int index;
> > -       __u32 flags;
> > +       __u32 before;
> > +       __u32 after;
> >   };
> > 
> > Would I request your opinion? You can see patches here:
> >   - 
> > https://github.com/takaswie/snd-firewire-improve/tree/topic/tascam-userspace-take3
> >   - https://github.com/takaswie/libhinawa/tree/topic/tascam-userspace-take3
> > 
> > You can receive 'control' event of 'HinawaSndTscm' GObject with these
> > two values:
> > 
> 
> ...
> 
> Sounds reasonable. For state change detection, I currently store the
> "before" state of each control in my application. Your new driver
> implementation will mean not needing to maintain my own state
> structures for that purpose.
> 
> I will try it out in the next few days.

I have only done a small amount of testing, but so far the control event
setup is working fine.

-Scott


[1] https://github.com/takaswie/snd-firewire-improve/blob/topic/tascam-userspace-take3/sound/firewire/tascam/amdtp-tascam.c#L132

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-10-22 11:47                           ` Scott Bahling
@ 2018-10-30  9:34                             ` Takashi Sakamoto
  2018-11-02  9:26                               ` Scott Bahling
  0 siblings, 1 reply; 22+ messages in thread
From: Takashi Sakamoto @ 2018-10-30  9:34 UTC (permalink / raw)
  To: sbahling; +Cc: alsa-devel, ffado-devel

Hi Scott,

Sorry to be late for reply, but I was a bit busy with travel for Audio
miniconference 2018 and tests for v4.20 merge window.

On 2018/10/22 20:47, Scott Bahling wrote:
> I have tried this, but the endianness of the status bits passed via this
> method seems to be wrong.
> 
> I noticed in your latest kernel code[1], that you don't convert the firewire
> data to local CPU endian when storing in the "after" variable which ends up
> getting pushed into the status structure as big endian:
> 
> 
>      after = buffer[s->data_block_quadlets - 1];
> ...
> ...
> ...
>      tscm->status[index] = after;
> 
> 
> Shouldn't that be:
> 
>      after = be32_to_cpu(buffer[s->data_block_quadlets - 1]);
> 
> which also would remove the need for later conversions in that block of
> code?
> 
> I have a tested patch which I will send as a pull request once github is
> functioning again.

Ah, indeed. I completely overlooked it, sorry...

At first place, I just focused on reduction of CPU usage in
packet processing, then cache big-endianness values from packets.
Queued events have converted host-endiannness value from the cache.
On the other hand, data returned by SNDRV_FIREWIRE_IOCTL_TASCAM_STATUS
ioctl is just copied from the cache, then it's still big-endiannness,
as you found.

Thanks for your PR[1] and it works well. But here I suggest that
we can judge the frequency to call SNDRV_FIREWIRE_IOCTL_TASCAM_STATUS
ioctl is surely less than the frequency on packet processing. If we
attempt to keep low CPU usage in the packet processing, current
implementation can be kept as is. Instead, a patch for handler of the
ioctl is worth for commit.

For example, This patch will fix the issue.

```
$ git diff
diff --git a/sound/firewire/tascam/tascam-hwdep.c 
b/sound/firewire/tascam/tascam-hwdep.c
index ad164ad..dff7dc4 100644
--- a/sound/firewire/tascam/tascam-hwdep.c
+++ b/sound/firewire/tascam/tascam-hwdep.c
@@ -189,10 +189,23 @@ static int hwdep_unlock(struct snd_tscm *tscm)

  static int tscm_hwdep_status(struct snd_tscm *tscm, void __user *arg)
  {
-       if (copy_to_user(arg, tscm->status, sizeof(tscm->status)))
-               return -EFAULT;
+       struct snd_firewire_tascam_status *status;
+       int i;
+       int err = 0;

-       return 0;
+       status = kmalloc(sizeof(*status), GFP_KERNEL);
+       if (!status)
+               return -ENOMEM;
+
+       for (i = 0; i < SNDRV_FIREWIRE_TASCAM_STATUS_COUNT; ++i)
+               status->data[i] = be32_to_cpu(tscm->status[i]);
+
+       if (copy_to_user(arg, status, sizeof(*status)))
+               err = -EFAULT;
+
+       kfree(status);
+
+       return err;
  }
```

I'd like you to test with this patch, before granting your PR.

[1] https://github.com/takaswie/snd-firewire-improve/pull/23


Thanks

Takashi Sakamoto

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-10-30  9:34                             ` Takashi Sakamoto
@ 2018-11-02  9:26                               ` Scott Bahling
  2018-11-02 12:05                                 ` Takashi Sakamoto
  0 siblings, 1 reply; 22+ messages in thread
From: Scott Bahling @ 2018-11-02  9:26 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, ffado-devel


[-- Attachment #1.1: Type: text/plain, Size: 3153 bytes --]

On Tue, 2018-10-30 at 18:34 +0900, Takashi Sakamoto wrote:
> Hi Scott,
> 
> Sorry to be late for reply, but I was a bit busy with travel for
> Audio
> miniconference 2018 and tests for v4.20 merge window.
> 
> On 2018/10/22 20:47, Scott Bahling wrote:
> > I have tried this, but the endianness of the status bits passed via
> > this
> > method seems to be wrong.
> > 
> > I noticed in your latest kernel code[1], that you don't convert the
> > firewire
> > data to local CPU endian when storing in the "after" variable which
> > ends up
> > getting pushed into the status structure as big endian:
> > 
> > 
> >      after = buffer[s->data_block_quadlets - 1];
> > ...
> > ...
> > ...
> >      tscm->status[index] = after;
> > 
> > 
> > Shouldn't that be:
> > 
> >      after = be32_to_cpu(buffer[s->data_block_quadlets - 1]);
> > 
> > which also would remove the need for later conversions in that
> > block of
> > code?
> > 
> > I have a tested patch which I will send as a pull request once
> > github is
> > functioning again.
> 
> Ah, indeed. I completely overlooked it, sorry...
> 
> At first place, I just focused on reduction of CPU usage in
> packet processing, then cache big-endianness values from packets.
> Queued events have converted host-endiannness value from the cache.
> On the other hand, data returned by
> SNDRV_FIREWIRE_IOCTL_TASCAM_STATUS
> ioctl is just copied from the cache, then it's still big-endiannness,
> as you found.
> 
> Thanks for your PR[1] and it works well. But here I suggest that
> we can judge the frequency to call SNDRV_FIREWIRE_IOCTL_TASCAM_STATUS
> ioctl is surely less than the frequency on packet processing. If we
> attempt to keep low CPU usage in the packet processing, current
> implementation can be kept as is. Instead, a patch for handler of the
> ioctl is worth for commit.

Makes perfect sense.

> For example, This patch will fix the issue.
> 
> ```
> $ git diff
> diff --git a/sound/firewire/tascam/tascam-hwdep.c 
> b/sound/firewire/tascam/tascam-hwdep.c
> index ad164ad..dff7dc4 100644
> --- a/sound/firewire/tascam/tascam-hwdep.c
> +++ b/sound/firewire/tascam/tascam-hwdep.c
> @@ -189,10 +189,23 @@ static int hwdep_unlock(struct snd_tscm *tscm)
> 
>   static int tscm_hwdep_status(struct snd_tscm *tscm, void __user
> *arg)
>   {
> -       if (copy_to_user(arg, tscm->status, sizeof(tscm->status)))
> -               return -EFAULT;
> +       struct snd_firewire_tascam_status *status;
> +       int i;
> +       int err = 0;
> 
> -       return 0;
> +       status = kmalloc(sizeof(*status), GFP_KERNEL);
> +       if (!status)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < SNDRV_FIREWIRE_TASCAM_STATUS_COUNT; ++i)
> +               status->data[i] = be32_to_cpu(tscm->status[i]);
> +
> +       if (copy_to_user(arg, status, sizeof(*status)))
> +               err = -EFAULT;
> +
> +       kfree(status);
> +
> +       return err;
>   }
> ```
> 
> I'd like you to test with this patch, before granting your PR.

I have tested and the patch above works as expected.

-Scott

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-11-02  9:26                               ` Scott Bahling
@ 2018-11-02 12:05                                 ` Takashi Sakamoto
  2018-11-16 17:37                                   ` Scott Bahling
  0 siblings, 1 reply; 22+ messages in thread
From: Takashi Sakamoto @ 2018-11-02 12:05 UTC (permalink / raw)
  To: Scott Bahling; +Cc: alsa-devel, ffado-devel

Hi Scott,

On 2018/11/02 18:26, Scott Bahling wrote:
>> Thanks for your PR[1] and it works well. But here I suggest that
>> we can judge the frequency to call SNDRV_FIREWIRE_IOCTL_TASCAM_STATUS
>> ioctl is surely less than the frequency on packet processing. If we
>> attempt to keep low CPU usage in the packet processing, current
>> implementation can be kept as is. Instead, a patch for handler of the
>> ioctl is worth for commit.
> 
> Makes perfect sense.
> 
>> For example, This patch will fix the issue.
>>
>> ```
>> $ git diff
>> diff --git a/sound/firewire/tascam/tascam-hwdep.c
>> b/sound/firewire/tascam/tascam-hwdep.c
>> index ad164ad..dff7dc4 100644
>> --- a/sound/firewire/tascam/tascam-hwdep.c
>> +++ b/sound/firewire/tascam/tascam-hwdep.c
>> @@ -189,10 +189,23 @@ static int hwdep_unlock(struct snd_tscm *tscm)
>>
>>    static int tscm_hwdep_status(struct snd_tscm *tscm, void __user
>> *arg)
>>    {
>> -       if (copy_to_user(arg, tscm->status, sizeof(tscm->status)))
>> -               return -EFAULT;
>> +       struct snd_firewire_tascam_status *status;
>> +       int i;
>> +       int err = 0;
>>
>> -       return 0;
>> +       status = kmalloc(sizeof(*status), GFP_KERNEL);
>> +       if (!status)
>> +               return -ENOMEM;
>> +
>> +       for (i = 0; i < SNDRV_FIREWIRE_TASCAM_STATUS_COUNT; ++i)
>> +               status->data[i] = be32_to_cpu(tscm->status[i]);
>> +
>> +       if (copy_to_user(arg, status, sizeof(*status)))
>> +               err = -EFAULT;
>> +
>> +       kfree(status);
>> +
>> +       return err;
>>    }
>> ```
>>
>> I'd like you to test with this patch, before granting your PR.
> 
> I have tested and the patch above works as expected.

Thanks for your check. I pushed the above patch into my remote branch.

Well, soon, merge window for v4.20 (or v5.0) kernel will be closed, then
we start working for next v4.21 (or v5.1) kernel. I have a plan to
include changes required for the status and control message in this
development period (not prepare yet) and the changes are the same in
the remote branch. I'll add you to C.C list when posting patches for it.

If you require more functionality, please inform it to me in this
development period.


Thanks

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: Controlling the Tascam FW-1884
  2018-11-02 12:05                                 ` Takashi Sakamoto
@ 2018-11-16 17:37                                   ` Scott Bahling
  0 siblings, 0 replies; 22+ messages in thread
From: Scott Bahling @ 2018-11-16 17:37 UTC (permalink / raw)
  To: Takashi Sakamoto; +Cc: alsa-devel, ffado-devel


[-- Attachment #1.1: Type: text/plain, Size: 1528 bytes --]

Hi Takashi,

On Fri, 2018-11-02 at 21:05 +0900, Takashi Sakamoto wrote:
> Hi Scott,
> 
> On 2018/11/02 18:26, Scott Bahling wrote:

> > 
> > I have tested and the patch above works as expected.
> 
> Thanks for your check. I pushed the above patch into my remote branch.
> 
> Well, soon, merge window for v4.20 (or v5.0) kernel will be closed, then
> we start working for next v4.21 (or v5.1) kernel. I have a plan to
> include changes required for the status and control message in this
> development period (not prepare yet) and the changes are the same in
> the remote branch. I'll add you to C.C list when posting patches for it.

Thanks!

> If you require more functionality, please inform it to me in this
> development period.

I have been testing the patches for a while now and they work without
issue. I'm also testing the libhinawa and hinawa-utils code in the
process. I haven't tested the channel level values from the isochronous
packets as I haven't a need for that so far. I see that the data
changes as expected, but I have not verified if the values are
accurate.

But my primary need was the control surface. Between what you already
had hinawa-utils and the kernel patches to pass the isochronous data to
userspace, I'm able to react on all input from the controls as well as
set the fader positions and light the LEDs. I will submit a pull
request to hinawa-utils for controlling the LEDs and Fader positions
for the FW-1884.

Thanks for your help!

Regards,

Scott

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2018-11-16 17:37 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <9cec059e1ff1a558f21a3f0729c5a69a3d506573.camel@suse.com>
2018-09-08  2:53 ` Controlling the Tascam FW-1884 Takashi Sakamoto
2018-09-08 11:21   ` Scott Bahling
2018-09-10  7:59     ` Takashi Sakamoto
2018-09-12  7:18       ` Scott Bahling
2018-09-17 14:36         ` Takashi Sakamoto
2018-09-24  9:32           ` Scott Bahling
2018-09-28  3:44             ` Takashi Sakamoto
2018-09-28 15:28               ` Scott Bahling
2018-10-02  3:16                 ` Takashi Sakamoto
2018-10-03 20:37                   ` Scott Bahling
2018-10-06  9:07                     ` Takashi Sakamoto
2018-10-07 11:32                       ` Scott Bahling
2018-10-07 14:11                   ` Takashi Sakamoto
2018-10-12  8:12                     ` Scott Bahling
2018-10-13 10:40                       ` Takashi Sakamoto
2018-10-14 19:09                         ` Scott Bahling
2018-10-22 11:47                           ` Scott Bahling
2018-10-30  9:34                             ` Takashi Sakamoto
2018-11-02  9:26                               ` Scott Bahling
2018-11-02 12:05                                 ` Takashi Sakamoto
2018-11-16 17:37                                   ` Scott Bahling
2018-10-03 19:31               ` Scott Bahling

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.