[0/6] ALSA: firewire: handle several IR/IT context in callback of the IT context
mbox series

Message ID 20191018061911.24909-1-o-takashi@sakamocchi.jp
Headers show
Series
  • ALSA: firewire: handle several IR/IT context in callback of the IT context
Related show

Message

Takashi Sakamoto Oct. 18, 2019, 6:19 a.m. UTC
Hi,

This patchset is a part of my continuous work to improve ALSA IEC
61883-1/6 packet streaming engine for clock-recovery, addressed in
my previous message:
https://mailman.alsa-project.org/pipermail/alsa-devel/2019-August/153388.html

For the clock-recovery, information in the sequence of tx packet from
device should be used to generate the sequence of rx packet to the
device. In current implementation of the engine, the packets are
processed in different tasklet contexts for each IR/IT context.
This is inconvenient to bypass information between these IR/IT contexts.

In this patchset, the engine is improved to process all of IR/IT
contexts in the same domain by a tasklet context for one of IT context.
For convenience, the IT context is called as 'IRQ target'. As a result,
1394 OHCI controller is managed to generate hardware IRQ just for the
IRQ target. All of rx/tx packets are processed in tasklet for the
hardware IRQ.

Takashi Sakamoto (6):
  ALSA: firewire-lib: add irq_target member into amdtp_domain struct
  ALSA: firewire-lib: replace pointer callback to flush isoc contexts in
    AMDTP domain
  ALSA: firewire-lib: replace ack callback to flush isoc contexts in
    AMDTP domain
  ALSA: firewire-lib: cancel flushing isoc context in the laste step to
    process context callback
  ALSA: firewire-lib: handle several AMDTP streams in callback handler
    of IRQ target
  ALSA: firewire-lib: postpone to start IR context

 sound/firewire/amdtp-stream.c               | 329 +++++++++++++++-----
 sound/firewire/amdtp-stream.h               |  17 +-
 sound/firewire/bebob/bebob_pcm.c            |  18 +-
 sound/firewire/bebob/bebob_stream.c         |  10 +-
 sound/firewire/dice/dice-pcm.c              |   8 +-
 sound/firewire/dice/dice-stream.c           |   2 +-
 sound/firewire/digi00x/digi00x-pcm.c        |   8 +-
 sound/firewire/digi00x/digi00x-stream.c     |   2 +-
 sound/firewire/fireface/ff-pcm.c            |   8 +-
 sound/firewire/fireface/ff-stream.c         |  10 +-
 sound/firewire/fireworks/fireworks_pcm.c    |  10 +-
 sound/firewire/fireworks/fireworks_stream.c |   2 +-
 sound/firewire/motu/motu-pcm.c              |   8 +-
 sound/firewire/motu/motu-stream.c           |   2 +-
 sound/firewire/oxfw/oxfw-pcm.c              |   8 +-
 sound/firewire/oxfw/oxfw-stream.c           |   2 +-
 sound/firewire/tascam/tascam-pcm.c          |   8 +-
 sound/firewire/tascam/tascam-stream.c       |   2 +-
 18 files changed, 325 insertions(+), 129 deletions(-)

Comments

Takashi Iwai Oct. 19, 2019, 7:22 a.m. UTC | #1
On Fri, 18 Oct 2019 08:19:05 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> This patchset is a part of my continuous work to improve ALSA IEC
> 61883-1/6 packet streaming engine for clock-recovery, addressed in
> my previous message:
> https://mailman.alsa-project.org/pipermail/alsa-devel/2019-August/153388.html
> 
> For the clock-recovery, information in the sequence of tx packet from
> device should be used to generate the sequence of rx packet to the
> device. In current implementation of the engine, the packets are
> processed in different tasklet contexts for each IR/IT context.
> This is inconvenient to bypass information between these IR/IT contexts.
> 
> In this patchset, the engine is improved to process all of IR/IT
> contexts in the same domain by a tasklet context for one of IT context.
> For convenience, the IT context is called as 'IRQ target'. As a result,
> 1394 OHCI controller is managed to generate hardware IRQ just for the
> IRQ target. All of rx/tx packets are processed in tasklet for the
> hardware IRQ.
> 
> Takashi Sakamoto (6):
>   ALSA: firewire-lib: add irq_target member into amdtp_domain struct
>   ALSA: firewire-lib: replace pointer callback to flush isoc contexts in
>     AMDTP domain
>   ALSA: firewire-lib: replace ack callback to flush isoc contexts in
>     AMDTP domain
>   ALSA: firewire-lib: cancel flushing isoc context in the laste step to
>     process context callback
>   ALSA: firewire-lib: handle several AMDTP streams in callback handler
>     of IRQ target
>   ALSA: firewire-lib: postpone to start IR context

Applied all patches now.

Although the preempt handling in AMDTP looks a bit suspicious, it
should be OK as long as the code has been tested, so I took as is.


thanks,

Takashi
Takashi Sakamoto Oct. 20, 2019, 8:37 a.m. UTC | #2
On Sat, Oct 19, 2019 at 09:22:16AM +0200, Takashi Iwai wrote:
> On Fri, 18 Oct 2019 08:19:05 +0200,
> Takashi Sakamoto wrote:
> > 
> > Hi,
> > 
> > This patchset is a part of my continuous work to improve ALSA IEC
> > 61883-1/6 packet streaming engine for clock-recovery, addressed in
> > my previous message:
> > https://mailman.alsa-project.org/pipermail/alsa-devel/2019-August/153388.html
> > 
> > For the clock-recovery, information in the sequence of tx packet from
> > device should be used to generate the sequence of rx packet to the
> > device. In current implementation of the engine, the packets are
> > processed in different tasklet contexts for each IR/IT context.
> > This is inconvenient to bypass information between these IR/IT contexts.
> > 
> > In this patchset, the engine is improved to process all of IR/IT
> > contexts in the same domain by a tasklet context for one of IT context.
> > For convenience, the IT context is called as 'IRQ target'. As a result,
> > 1394 OHCI controller is managed to generate hardware IRQ just for the
> > IRQ target. All of rx/tx packets are processed in tasklet for the
> > hardware IRQ.
> > 
> > Takashi Sakamoto (6):
> >   ALSA: firewire-lib: add irq_target member into amdtp_domain struct
> >   ALSA: firewire-lib: replace pointer callback to flush isoc contexts in
> >     AMDTP domain
> >   ALSA: firewire-lib: replace ack callback to flush isoc contexts in
> >     AMDTP domain
> >   ALSA: firewire-lib: cancel flushing isoc context in the laste step to
> >     process context callback
> >   ALSA: firewire-lib: handle several AMDTP streams in callback handler
> >     of IRQ target
> >   ALSA: firewire-lib: postpone to start IR context
> 
> Applied all patches now.
> 
> Although the preempt handling in AMDTP looks a bit suspicious, it
> should be OK as long as the code has been tested, so I took as is.

I can understand your concern but it works well as long as I tested.
In this time I use preemption-disable during processing queued packet in
process context but before I had used irq-disable instead.

I depict a call graph to process isoc packet in process context below:

On pcm.ack() or pcm.pointer() callbacks:
fw_iso_context_flush_completions()
->struct fw_card_driver.flush_iso_completions()
= ohci_flush_iso_completions()
  ->tasklet_disable()
  ->context_tasklet()
    (read registers on 1394 OHCI controller to get info for queued packet)
  ->flush_iso_completions()
    ->struct fw_iso_context.callback.sc()
    = amdtp_master_callback()
      ->out_stream_callback() (for irq-target)
      ->fw_iso_context_flush_completions()
        ->...
          ->out_stream_callback() or in_stream_callback() (for non irq-target)
  ->tasklet_enable()

The tasklet of IT context for irq-target AMDTP stream is temporarily
disabled when flushing isoc packets queued for the context. The tasklet
doesn't run even if it's queued in hw IRQ context. In a point to
processing queued packet for several isoc contexts in the same domain,
I have no concern without any irq-flag handling for local cpu.

1394 OHCI controller still continue isoc cycle during the above call
graph (= 125 usec interval according to 24.576 MHz clock). Actually the
number of queued packets for non-irq-target AMDTP stream can be slightly
different from the number for irq-target AMDTP stream by one or two
packets without any interruption. In a case that any interruption occurs
after processing queued packets for the irq-target stream, it's likely to
process largely different number of queued packets for the rest of AMDTP
streams in the same domain after resumed. It's desirable not to make such
count gap between streams in the same domain and I leave it for my future
work.

In this time the count gap is allowed. I use kernel preemption to avoid
the interruption but to catch hw IRQ from 1394 OHCI controller (and the
other hardware).


In another point, there's a race condition against flushing queued packet
in runtime between several PCM substreams for AMDTP streams in the same
domain. ALSA PCM core executes pcm.pointer and pcm.ack callback under spin
lock of runtime of PCM substream, thus the race is controlled for operations
to single PCM substream. However some PCM substreams are associated to the
same domain via AMDTP streams. At present I never add any solution for this
race.


Thanks

Takashi Sakamoto
Takashi Sakamoto Oct. 21, 2019, 2:40 p.m. UTC | #3
Hi,

On Sun, Oct 20, 2019 at 05:37:19PM +0900, Takashi Sakamoto wrote:
> On Sat, Oct 19, 2019 at 09:22:16AM +0200, Takashi Iwai wrote:
> > Although the preempt handling in AMDTP looks a bit suspicious, it
> > should be OK as long as the code has been tested, so I took as is.
> 
> I can understand your concern but it works well as long as I tested.
> In this time I use preemption-disable during processing queued packet in
> process context but before I had used irq-disable instead.
> 
> I depict a call graph to process isoc packet in process context below:
> 
> On pcm.ack() or pcm.pointer() callbacks:
> fw_iso_context_flush_completions()
> ->struct fw_card_driver.flush_iso_completions()
> = ohci_flush_iso_completions()
>   ->tasklet_disable()
>   ->context_tasklet()
>     (read registers on 1394 OHCI controller to get info for queued packet)
>   ->flush_iso_completions()
>     ->struct fw_iso_context.callback.sc()
>     = amdtp_master_callback()
>       ->out_stream_callback() (for irq-target)
>       ->fw_iso_context_flush_completions()
>         ->...
>           ->out_stream_callback() or in_stream_callback() (for non irq-target)
>   ->tasklet_enable()
> 
> The tasklet of IT context for irq-target AMDTP stream is temporarily
> disabled when flushing isoc packets queued for the context. The tasklet
> doesn't run even if it's queued in hw IRQ context. In a point to
> processing queued packet for several isoc contexts in the same domain,
> I have no concern without any irq-flag handling for local cpu.
> 
> 1394 OHCI controller still continue isoc cycle during the above call
> graph (= 125 usec interval according to 24.576 MHz clock). Actually the
> number of queued packets for non-irq-target AMDTP stream can be slightly
> different from the number for irq-target AMDTP stream by one or two
> packets without any interruption. In a case that any interruption occurs
> after processing queued packets for the irq-target stream, it's likely to
> process largely different number of queued packets for the rest of AMDTP
> streams in the same domain after resumed. It's desirable not to make such
> count gap between streams in the same domain and I leave it for my future
> work.
> 
> In this time the count gap is allowed. I use kernel preemption to avoid
> the interruption but to catch hw IRQ from 1394 OHCI controller (and the
> other hardware).
> 
> 
> In another point, there's a race condition against flushing queued packet
> in runtime between several PCM substreams for AMDTP streams in the same
> domain. ALSA PCM core executes pcm.pointer and pcm.ack callback under spin
> lock of runtime of PCM substream, thus the race is controlled for operations
> to single PCM substream. However some PCM substreams are associated to the
> same domain via AMDTP streams. At present I never add any solution for this
> race.

I realize that this race is managed as well, by a call of
test_and_set_bit_lock(). When operations for several PCM substream call
pcm.pointer or pcm.ack simultaneously, one of them can flush queued
packets. I refine the above call graph:

On pcm.ack() or pcm.pointer() callbacks:
fw_iso_context_flush_completions()
->struct fw_card_driver.flush_iso_completions()
= ohci_flush_iso_completions()
  ->tasklet_disable()
  if (!test_and_set_bit_lock())
    ->context_tasklet()
      (read registers on 1394 OHCI controller to get info for queued packet)
    ->flush_iso_completions()
      ->struct fw_iso_context.callback.sc()
      = amdtp_master_callback()
        ->out_stream_callback() (for irq-target)
        ->fw_iso_context_flush_completions()
          ->...
            ->out_stream_callback() or in_stream_callback() (for non irq-target)
    ->clear_bit_unlock()
  ->tasklet_enable()


Regards

Takashi Sakamoto