[alsa-devel] [PATCH] ALSA AICA sound on SEGA Dreamcast - fix behaviour in poor resource conditions
Patch against code in alsa-kernel - tested with patched up 2.6.21-rc7 and working well Submitted by: Adrian McMenamin <adrian@mcmen.demon.co.uk> Signed-off by: Adrian McMenamin <adrian@mcmen.demon.co.uk> diff -ruN aicaold/aica.c aicanew/aica.c --- aicaold/aica.c 2007-07-05 23:01:39.000000000 +0100 +++ aicanew/aica.c 2007-07-05 23:02:02.000000000 +0100 @@ -65,10 +65,6 @@ /* Use workqueue */ -static struct spu_work_holder { - struct work_struct spu_dma_work; - void *sspointer; -} spu_working; static struct workqueue_struct *aica_queue; @@ -252,18 +248,15 @@ static void run_spu_dma(struct work_struct *work) { int buffer_size; - struct snd_pcm_substream *substream; struct snd_pcm_runtime *runtime; struct snd_card_aica *dreamcastcard; - struct spu_work_holder *holder = container_of(work, struct spu_work_holder, spu_dma_work); - substream = holder-> sspointer; - dreamcastcard = substream->pcm->private_data; - runtime = substream->runtime; + dreamcastcard = container_of(work, struct snd_card_aica, spu_dma_work); + runtime = dreamcastcard->substream->runtime; if (unlikely(dreamcastcard->dma_check == 0)) { buffer_size = frames_to_bytes(runtime, runtime->buffer_size); if (runtime->channels > 1) dreamcastcard->channel->flags |= 0x01; - aica_dma_transfer(runtime->channels, buffer_size, substream); + aica_dma_transfer(runtime->channels, buffer_size, dreamcastcard->substream); startup_aica(dreamcastcard); dreamcastcard->clicks = buffer_size / (AICA_PERIOD_SIZE * runtime->channels); @@ -271,7 +264,7 @@ } else { aica_dma_transfer(runtime->channels, AICA_PERIOD_SIZE * runtime->channels, - substream); + dreamcastcard->substream); snd_pcm_period_elapsed(dreamcastcard->substream); dreamcastcard->clicks++; if (unlikely(dreamcastcard->clicks >= AICA_PERIOD_NUMBER)) @@ -307,7 +300,8 @@ dreamcastcard->current_period = play_period; if (unlikely(dreamcastcard->dma_check == 0)) dreamcastcard->dma_check = 1; - queue_work(aica_queue, &(spu_working.spu_dma_work)); + queue_work(aica_queue, &(dreamcastcard->spu_dma_work)); + } static void spu_begin_dma(struct snd_pcm_substream *substream) @@ -317,10 +311,8 @@ struct snd_pcm_runtime *runtime; runtime = substream->runtime; dreamcastcard = substream->pcm->private_data; - /* Use queue to do the heavy lifting */ - spu_working.sspointer = substream; - INIT_WORK(&(spu_working.spu_dma_work), run_spu_dma); - queue_work(aica_queue, &(spu_working.spu_dma_work)); + //get the queue to do the work + queue_work(aica_queue, &(dreamcastcard->spu_dma_work)); /* Timer may already be running */ if (unlikely(dreamcastcard->timer.data)) { mod_timer(&dreamcastcard->timer, jiffies + 4); @@ -366,7 +358,9 @@ *substream) { struct snd_card_aica *dreamcastcard = substream->pcm->private_data; + flush_workqueue(aica_queue); del_timer(&dreamcastcard->timer); + aica_chn_halt(); kfree(dreamcastcard->channel); spu_disable(); return 0; @@ -402,17 +396,10 @@ static int snd_aicapcm_pcm_trigger(struct snd_pcm_substream *substream, int cmd) { - struct snd_card_aica *dreamcastcard; switch (cmd) { case SNDRV_PCM_TRIGGER_START: spu_begin_dma(substream); break; - case SNDRV_PCM_TRIGGER_STOP: - dreamcastcard = substream->pcm->private_data; - if (dreamcastcard->timer.data) - del_timer(&dreamcastcard->timer); - aica_chn_halt(); - break; default: return -EINVAL; } @@ -610,6 +597,8 @@ strcpy(dreamcastcard->card->shortname, SND_AICA_DRIVER); strcpy(dreamcastcard->card->longname, "Yamaha AICA Super Intelligent Sound Processor for SEGA Dreamcast"); + //start the worker thread + INIT_WORK(&(dreamcastcard->spu_dma_work), run_spu_dma); /* Load the PCM 'chip' */ err = snd_aicapcmchip(dreamcastcard, 0); if (unlikely(err < 0)) diff -ruN aicaold/aica.h aicanew/aica.h --- aicaold/aica.h 2007-07-05 23:01:39.000000000 +0100 +++ aicanew/aica.h 2007-07-05 23:02:02.000000000 +0100 @@ -69,6 +69,7 @@ }; struct snd_card_aica { + struct work_struct spu_dma_work; struct snd_card *card; struct aica_channel *channel; struct snd_pcm_substream *substream;
On 05/07/07, Adrian McMenamin <adrianmcmenamin@gmail.com> wrote:
Patch against code in alsa-kernel - tested with patched up 2.6.21-rc7 and working well
Apologies - obviously I mean 2.6.22-rc7 patched with the aica driver now in alsa-kernel
At Thu, 5 Jul 2007 23:07:44 +0100, Adrian McMenamin wrote:
@@ -402,17 +396,10 @@ static int snd_aicapcm_pcm_trigger(struct snd_pcm_substream *substream, int cmd) { - struct snd_card_aica *dreamcastcard; switch (cmd) { case SNDRV_PCM_TRIGGER_START: spu_begin_dma(substream); break; - case SNDRV_PCM_TRIGGER_STOP: - dreamcastcard = substream->pcm->private_data; - if (dreamcastcard->timer.data) - del_timer(&dreamcastcard->timer); - aica_chn_halt(); - break; default: return -EINVAL; }
Is this a correct change? Then you'd have no control for stopping the stream. (BTW, your embedded patch was malformed...) thanks, Takashi
On 06/07/07, Takashi Iwai <tiwai@suse.de> wrote:
At Thu, 5 Jul 2007 23:07:44 +0100, Adrian McMenamin wrote:
@@ -402,17 +396,10 @@ static int snd_aicapcm_pcm_trigger(struct snd_pcm_substream *substream, int cmd) { - struct snd_card_aica *dreamcastcard; switch (cmd) { case SNDRV_PCM_TRIGGER_START: spu_begin_dma(substream); break; - case SNDRV_PCM_TRIGGER_STOP: - dreamcastcard = substream->pcm->private_data; - if (dreamcastcard->timer.data) - del_timer(&dreamcastcard->timer); - aica_chn_halt(); - break; default: return -EINVAL; }
Is this a correct change? Then you'd have no control for stopping the stream.
The shutdown is all done in the close() now, partly because snd_aicapcm_pcm_trigger cannot sleep and close() can. Is there a pressing reason to put code in snd_aicapcm_pcm_trigger?
At Fri, 6 Jul 2007 10:46:59 +0100, Adrian McMenamin wrote:
On 06/07/07, Takashi Iwai <tiwai@suse.de> wrote:
At Thu, 5 Jul 2007 23:07:44 +0100, Adrian McMenamin wrote:
@@ -402,17 +396,10 @@ static int snd_aicapcm_pcm_trigger(struct snd_pcm_substream *substream, int cmd) { - struct snd_card_aica *dreamcastcard; switch (cmd) { case SNDRV_PCM_TRIGGER_START: spu_begin_dma(substream); break; - case SNDRV_PCM_TRIGGER_STOP: - dreamcastcard = substream->pcm->private_data; - if (dreamcastcard->timer.data) - del_timer(&dreamcastcard->timer); - aica_chn_halt(); - break; default: return -EINVAL; }
Is this a correct change? Then you'd have no control for stopping the stream.
The shutdown is all done in the close() now, partly because snd_aicapcm_pcm_trigger cannot sleep and close() can. Is there a pressing reason to put code in snd_aicapcm_pcm_trigger?
I guess you shoud call del_timer() there at least. Otherwise the timer handler (aica_period_elapsed()) might be called even if the stream is not really running. The below is some coding style fixes, BTW. I'm not sure whether queue_work() is intended in the block if "if (dma_check == 0)" or not. Takashi --- aica.c.old 2007-07-06 11:51:18.000000000 +0200 +++ aica.c 2007-07-06 11:52:10.000000000 +0200 @@ -256,7 +256,8 @@ buffer_size = frames_to_bytes(runtime, runtime->buffer_size); if (runtime->channels > 1) dreamcastcard->channel->flags |= 0x01; - aica_dma_transfer(runtime->channels, buffer_size, dreamcastcard->substream); + aica_dma_transfer(runtime->channels, buffer_size, + dreamcastcard->substream); startup_aica(dreamcastcard); dreamcastcard->clicks = buffer_size / (AICA_PERIOD_SIZE * runtime->channels); @@ -268,9 +269,7 @@ snd_pcm_period_elapsed(dreamcastcard->substream); dreamcastcard->clicks++; if (unlikely(dreamcastcard->clicks >= AICA_PERIOD_NUMBER)) - { dreamcastcard->clicks %= AICA_PERIOD_NUMBER; - } mod_timer(&dreamcastcard->timer, jiffies + 1); } } @@ -300,8 +299,7 @@ dreamcastcard->current_period = play_period; if (unlikely(dreamcastcard->dma_check == 0)) dreamcastcard->dma_check = 1; - queue_work(aica_queue, &(dreamcastcard->spu_dma_work)); - + queue_work(aica_queue, &(dreamcastcard->spu_dma_work)); } static void spu_begin_dma(struct snd_pcm_substream *substream) @@ -311,7 +309,7 @@ struct snd_pcm_runtime *runtime; runtime = substream->runtime; dreamcastcard = substream->pcm->private_data; - //get the queue to do the work + /* get the queue to do the work */ queue_work(aica_queue, &(dreamcastcard->spu_dma_work)); /* Timer may already be running */ if (unlikely(dreamcastcard->timer.data)) { @@ -597,7 +595,7 @@ strcpy(dreamcastcard->card->shortname, SND_AICA_DRIVER); strcpy(dreamcastcard->card->longname, "Yamaha AICA Super Intelligent Sound Processor for SEGA Dreamcast"); - //start the worker thread + /* start the worker thread */ INIT_WORK(&(dreamcastcard->spu_dma_work), run_spu_dma); /* Load the PCM 'chip' */ err = snd_aicapcmchip(dreamcastcard, 0); @@ -631,7 +629,8 @@ .probe = snd_aica_probe, .remove = snd_aica_remove, .driver = { - .name = SND_AICA_DRIVER}, + .name = SND_AICA_DRIVER + }, }; static int __init aica_init(void)
On 06/07/07, Takashi Iwai <tiwai@suse.de> wrote:
I guess you shoud call del_timer() there at least. Otherwise the timer handler (aica_period_elapsed()) might be called even if the stream is not really running.
Fair enough. I'll fix that and repost later.
The below is some coding style fixes, BTW. I'm not sure whether queue_work() is intended in the block if "if (dma_check == 0)" or not.
It's just an aberrant indent - I think there was an independent block there in earlier code, it got removed but the tab stayed :)
On 06/07/07, Adrian McMenamin <adrianmcmenamin@gmail.com> wrote:
On 06/07/07, Takashi Iwai <tiwai@suse.de> wrote:
I guess you shoud call del_timer() there at least. Otherwise the timer handler (aica_period_elapsed()) might be called even if the stream is not really running.
Fair enough. I'll fix that and repost later.
Attached is a new patch, tidied with indent and with SNDRV_PCM_TRIGGER_STOP being used. Submitted by: Adrian McMenamin <adrian@mcmen.demon.co.uk> Signed-off by: Adrian McMenamin <adrian@mcmen.demon.co.uk>
At Fri, 6 Jul 2007 21:42:27 +0100, Adrian McMenamin wrote:
On 06/07/07, Adrian McMenamin <adrianmcmenamin@gmail.com> wrote:
On 06/07/07, Takashi Iwai <tiwai@suse.de> wrote:
I guess you shoud call del_timer() there at least. Otherwise the timer handler (aica_period_elapsed()) might be called even if the stream is not really running.
Fair enough. I'll fix that and repost later.
Attached is a new patch, tidied with indent and with SNDRV_PCM_TRIGGER_STOP being used.
Submitted by: Adrian McMenamin <adrian@mcmen.demon.co.uk> Signed-off by: Adrian McMenamin <adrian@mcmen.demon.co.uk>
Thanks, committed to ALSA tree now. Takashi
participants (2)
-
Adrian McMenamin -
Takashi Iwai