Re: [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

Re: [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Adrian McMenamin]

On Thu, 2006-06-01 at 23:31 +0100, Adrian McMenamin wrote:
> This adds sound for the Yamaha AICA "Super Intelligent Sound
> Processor" (PCM) device on the SEGA Dreamcast
> 
> Signed off by Adrian McMenamin <adrian@mcmen.demon.co.uk>
> 
I've had no comments back on this - I am thinking of committing to the
linux-sh cvs, though it really belongs in ALSA.

Any reason why I shouldn't?

Re: [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Lee Revell]

On Sat, 2006-06-03 at 12:39 +0100, Adrian McMenamin wrote:
> On Thu, 2006-06-01 at 23:31 +0100, Adrian McMenamin wrote:
> > This adds sound for the Yamaha AICA "Super Intelligent Sound
> > Processor" (PCM) device on the SEGA Dreamcast
> > 
> > Signed off by Adrian McMenamin <adrian@mcmen.demon.co.uk>
> > 
> I've had no comments back on this - I am thinking of committing to the
> linux-sh cvs, though it really belongs in ALSA.
> 
> Any reason why I shouldn't?
> 

Did you fix all of the issues that were raised by Paul and Takashi-san?

Lee

Re: [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Adrian McMenamin]

On Sat, 2006-06-03 at 11:16 -0400, Lee Revell wrote:
> On Sat, 2006-06-03 at 12:39 +0100, Adrian McMenamin wrote:
> > On Thu, 2006-06-01 at 23:31 +0100, Adrian McMenamin wrote:
> > > This adds sound for the Yamaha AICA "Super Intelligent Sound
> > > Processor" (PCM) device on the SEGA Dreamcast
> > > 
> > > Signed off by Adrian McMenamin <adrian@mcmen.demon.co.uk>
> > > 
> > I've had no comments back on this - I am thinking of committing to the
> > linux-sh cvs, though it really belongs in ALSA.
> > 
> > Any reason why I shouldn't?
> > 
> 
> Did you fix all of the issues that were raised by Paul and Takashi-san?
> 
> Lee
Those ones that were fixable, yes. At least I think so :)

Re: [Alsa-devel] [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Takashi Iwai]

At Sat, 03 Jun 2006 12:39:48 +0100,
Adrian McMenamin wrote:
> 
> On Thu, 2006-06-01 at 23:31 +0100, Adrian McMenamin wrote:
> > This adds sound for the Yamaha AICA "Super Intelligent Sound
> > Processor" (PCM) device on the SEGA Dreamcast
> > 
> > Signed off by Adrian McMenamin <adrian@mcmen.demon.co.uk>
> > 
> I've had no comments back on this - I am thinking of committing to the
> linux-sh cvs, though it really belongs in ALSA.

Sorry for the delay.  I've had a long weekend.

> Any reason why I shouldn't?

As Paul already pointed, the platform_device things must be fixed.
Also, better to clean up the code directly accessing hardcoded
addresses.

Another big concern is that spu_dma_work is initialized/rewritten
dynamically in spu_begin_dma() and aica_period_elapsed() via
INIT_WORK() and PREPARE_WOR().  This looks pretty strange and may be
racy.


Takashi

Re: [linuxsh-dev] [Alsa-devel] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Adrian McMenamin]

On Tue, 2006-06-06 at 12:25 +0200, Takashi Iwai wrote:

> 
> As Paul already pointed, the platform_device things must be fixed.
> Also, better to clean up the code directly accessing hardcoded
> addresses.

Working on that, some new code in my personal CVS now - but I suspect it
will be the weekend before that gets fully fixed.


> 
> Another big concern is that spu_dma_work is initialized/rewritten
> dynamically in spu_begin_dma() and aica_period_elapsed() via
> INIT_WORK() and PREPARE_WOR().  This looks pretty strange and may be
> racy.

Actually, the two macros INIT_WORK and PREPARE_WORK use the same work
queue but ask it to schedule the execution of two different (if very
similar) functions start_spu_dma() - which does the initial transfer and
more_spu_dma - which tops up the dma transfers.

So I think I've got that right.

Re: [linuxsh-dev] [Alsa-devel] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Takashi Iwai]

At Wed, 07 Jun 2006 00:10:48 +0100,
Adrian McMenamin wrote:
> 
> On Tue, 2006-06-06 at 12:25 +0200, Takashi Iwai wrote:
> 
> > Another big concern is that spu_dma_work is initialized/rewritten
> > dynamically in spu_begin_dma() and aica_period_elapsed() via
> > INIT_WORK() and PREPARE_WOR().  This looks pretty strange and may be
> > racy.
> 
> Actually, the two macros INIT_WORK and PREPARE_WORK use the same work
> queue but ask it to schedule the execution of two different (if very
> similar) functions start_spu_dma() - which does the initial transfer and
> more_spu_dma - which tops up the dma transfers.
> 
> So I think I've got that right.

What's wrong with using two individual work struct so that you
initialize them only once?
I wonder it because you already have unused fields work and work2...


Takashi

Re: [Alsa-devel] [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Adrian McMenamin]

On Wed, 2006-06-07 at 11:51 +0200, Takashi Iwai wrote:
> At Wed, 07 Jun 2006 00:10:48 +0100,
> Adrian McMenamin wrote:
> > 
> > On Tue, 2006-06-06 at 12:25 +0200, Takashi Iwai wrote:
> > 
> > > Another big concern is that spu_dma_work is initialized/rewritten
> > > dynamically in spu_begin_dma() and aica_period_elapsed() via
> > > INIT_WORK() and PREPARE_WOR().  This looks pretty strange and may be
> > > racy.
> > 
> > Actually, the two macros INIT_WORK and PREPARE_WORK use the same work
> > queue but ask it to schedule the execution of two different (if very
> > similar) functions start_spu_dma() - which does the initial transfer and
> > more_spu_dma - which tops up the dma transfers.
> > 
> > So I think I've got that right.
> 
> What's wrong with using two individual work struct so that you
> initialize them only once?
> I wonder it because you already have unused fields work and work2...
> 
They've gone actually.

I need to initialise them because every call is a discrete processing
job - ie I send one thing to go, then when a period has elapsed I
schedule another transfer to run on the kernel thread.

Isn't this the way it is meant to work?

Re: [Alsa-devel] [linuxsh-dev] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Takashi Iwai]

At Wed, 07 Jun 2006 19:16:42 +0100,
Adrian McMenamin wrote:
> 
> On Wed, 2006-06-07 at 11:51 +0200, Takashi Iwai wrote:
> > At Wed, 07 Jun 2006 00:10:48 +0100,
> > Adrian McMenamin wrote:
> > > 
> > > On Tue, 2006-06-06 at 12:25 +0200, Takashi Iwai wrote:
> > > 
> > > > Another big concern is that spu_dma_work is initialized/rewritten
> > > > dynamically in spu_begin_dma() and aica_period_elapsed() via
> > > > INIT_WORK() and PREPARE_WOR().  This looks pretty strange and may be
> > > > racy.
> > > 
> > > Actually, the two macros INIT_WORK and PREPARE_WORK use the same work
> > > queue but ask it to schedule the execution of two different (if very
> > > similar) functions start_spu_dma() - which does the initial transfer and
> > > more_spu_dma - which tops up the dma transfers.
> > > 
> > > So I think I've got that right.
> > 
> > What's wrong with using two individual work struct so that you
> > initialize them only once?
> > I wonder it because you already have unused fields work and work2...
> > 
> They've gone actually.
> 
> I need to initialise them because every call is a discrete processing
> job - ie I send one thing to go, then when a period has elapsed I
> schedule another transfer to run on the kernel thread.
> 
> Isn't this the way it is meant to work?

It's uncoventional that a work struct is re-initialized on the fly.

IMO, it's better to introduce a flag indicating the stream is already
running and use a single function/work struct.  For example,

	static void do_first_event(struct stream *str)
	{
		...
		str->running = 1;
	}

	static void do_more_event(struct stream *str)
	{
		...
	}

	static void work_event(void *data)
	{
		struct stream *str = data;

		if (str->running)
			do_more_event(str);
		else
			do_first_event(str);
	}

	trigger_start()
	{
		queue_work(queue, &this_work);
	}

	interrupt_handler()
	{
		...
		queue_work(queue, &this_work);
		...
	}

	initialization()
	{
		...
		queue = create_workqueue("xxx");
		INIT_WORK(&this_work, work_event, str);
		...
	}	

Re: [linuxsh-dev] [Alsa-devel] [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Adrian McMenamin]

On Thu, 2006-06-08 at 12:35 +0200, Takashi Iwai wrote:

> 
> It's uncoventional that a work struct is re-initialized on the fly.
> 
> IMO, it's better to introduce a flag indicating the stream is already
> running and use a single function/work struct.  For example,

Yes, I've did this sort of thing last night :)

Re: [PATCH] Add support for Yamaha AICA sound on SEGA Dreamcast

[Adrian McMenamin]

On Mon, 2006-06-05 at 12:53 +0300, Paul Mundt wrote:

> > +static void spu_init(void)
> 
> This can probably be inlined..
> 
> > +/* aica_chn_start - write to spu to start playback */
> > +static void aica_chn_start(void)
> > +{
> > +	spu_write_wait();
> > +	writel(AICA_CMD_KICK | AICA_CMD_START,
> > +	       (uint32_t *) AICA_CONTROL_POINT);
> > +}
> > +
> > +/* aica_chn_halt - write to spu to halt playback */
> > +static void aica_chn_halt(void)
> > +{
> > +	spu_write_wait();
> > +	writel(AICA_CMD_KICK | AICA_CMD_STOP,
> > +	       (uint32_t *) AICA_CONTROL_POINT);
> > +}
> > +
> These too.
> 

No point in inlining these as they all wait on a FIFO to clear - ie the
benefit of inling will be tiny. I notice a stray inline still lurking
from the code that can go...