Reply by jim November 13, 20072007-11-13
Andrew;

Thank you for the very detailed response. It has been helpful. Removing the
interrupt modifier in fact seems to have solved one of my mystery problems;
my interrupts are no longer vanishing into the weeds.

I know that the c64cc_pci.c code I started with didn't have that modifier; it
was one of the things I was exploring as I sort this environment out.
However, I had totally forgotten that I myself had added that, and it was
biting me as I get this straightened out, and I didn't realize it.

On Monday 12 November 2007 15:27:17 you wrote:
> Hi Jim,
>
> The last question first: no there is nothing else to correctly set up
> interrupts. On the startup before the application tasks are created and
> the BIOS started I would check and clear any spurious interrupts, but
> the ISR handles them anyway.
>
> This problem relates to robustness of the code (altogether BIOS and
> CSL and the application). Under any circumstances PCE1 should always
> be in the .text section plus whatever executable subsections are defined
> in the BIOS. If it is off, there is a bug either in pointers or in the
> way of handling interrupts.
>
> When the CPU start servicing an interrupt, it saves (among other things)
> the current PCE1 value in the IRP (interrupt return pointer) and jumps
> to the corresponding IST entry, which is ISTP+INT_NO*32, where the branch
> to the actual ISR is usually taken. The ISR uses B IRP to return, instead
> of the regular B B3 return, so there is no chances for the ISR to go
> astray, if it keeps the IRP intact.
>
> This is what is going on the CPU side of the game. The OS adds to this
> more complexity: as there is practically no difference from the CPU side
> in which exactly context (interrupt (i.e. a "privileged" one) or "user")
> it currently is, the OS should make it explicit. This is required for the
> OS in order to do not preform a context switch right in the interrupt mode
> - which is possible provided there is a semaphore post call. So the OS does
> prevent itself from doing such things at the cost that a C interrupt
> handler now is defined without the interrupt modifier! :) See Chpt 4 of
> SPRU423. You need to remove the interrupt modifier from the mb_isr()
> definition (in fact it was not there originally).
>
> The simple algorithm is as follows:
>
> Interrupts occurs
> CPU branches to the IST
> IST branches to ISR
> ISR saves context
> ISR completes its actions
> ISR restores context
> ISR branches back via B IRP
>
> What BIOS does:
>
> Before interrupts are enabled, BIOS stores the address of the ISR by an
> HWI_dispatchPlug() call, start tasks, etc. BIOS starts.
>
> Interrupt occurs
> CPU branches to the IST
> IST branches to the BIOS dispatcher
> Dispatcher saves context
> Dispatcher INCREMENTS NESTED INTERRUPTS COUNTER (hwi.h62)
> Dispatcher calls ISR as a regular C subroutine
> ISR does its actions and possibly calls BIOS service that may
> lead to task rescheduling. BIOS in response checks the interrupt
> counter and postpones task switch until after the counter is 0
> ISR returns
> Dispatcher DECREMENTS COUNTER, if counter is 0, calls the scheduler and
> restores not the previously saved context, but the context returned
> by the scheduler.
> Dispatcher branches to the restored context.
>
> Other suggestions:
>
> One potential place for wrong pointers is in the call of the application
> callback function, that is called in the ISR before return. There are a
> couple of things to check:
>
> * is the pointer to the callback routine does passed correctly?
> * does the declaration of the callback function matches to what the BIOS
> expects? That might corrupt the stack which is common to the ISR and the
> callback. It might be useful to compare SP before and after the callback
> call. Did you step over the callback and make sure that it returns to the
> ISR?
>
Actually, it turned out that the demo code I started with wasn't the original
TI demo code. The copy I was provided by my client had been used/modified by
someone else before the job came to me. This deceived me for awhile; I had
thought I was starting with working TI code when in fact I was starting with
non-working incorrectly modified TI code. By the time a discussion with my
client sorted this out for me, I was well into my mods and decided to just
continue with it, but it meant that all my earlier assumptions that "this
must be right" went out the window.

Specifically it turns out that the callback function was never being invoked,
and my asynchronous application was free running when I had thought it was
being synchronized. After working on the interrupt structure, I figured out
that the callback wasn't being called, traced the logic, and fixed it. It is
now being called and appears to be working correctly.

Also, it seems that the demo code spawned tasks more or less the way I am
doing it now, and the ISR would run through all of the queued-up gio objects
when it was invoked until there were no more. The problem with this was that
these objects were defined prior to the ASYNC_write that specified what to do
with them, and if the interrupt started through the list before all the
channel tasks had run, the result was to feed garbage onto the PCI bus, crash
the host computer, and hang the DSP interrupt handler (or deadlock the PCI
bus and therefore deadlock the host and part of the DSP board...I didn't
bother to sort that part out). I just sorted that one out a couple of hours
ago, and it explains yet another part of my mystery interrupt service routine
problems.

This didn't mess things up with the demo program with which I started because
it was never invoking that ISR.

> I also thought that a few other places in the code (based on your next
> to the last revision) need to be checked again:
>
> The transmitters tasks before exit (via implicit TSK_exit() call) go to
> sleep for 2 ticks, which is redundant and can be removed.
>
> The WaitForDMA() calls PCI_dspIntReqClear(), therefore it is called twice
> before the PCI_dspIntReqSet() call, also redundant.
>
> Next, the WaitForDMA() resets the semaphore, which might already have been
> set, this would miss one host to dsp interrupt. Actually, I would better
> use SEM_post/pend+Binary calls to make it a mutex, which it is supposed to
> be. A very close issue is the use of the validxfer variable. Currently it
> mimics the semaphore, but is not protected, I would avoid using unprotected
> objects.
>
There are a number of issues like that in the code. I'll clean them up when I
have something that is working reliably. Right now I am not concerned about
some of the redundancies; in the past some of those appeared necessary, and
it is my earnest hope that as this thing comes into shape, those things which
I previously had to do to get it to work will be removed.

> The last thing that is yet unclear to me is the logic of posting an
> interrupt to host. So far, the interrupt is sent (by calling
> PCI_dspIntReqSet() ) as soon as all (or part of) the 64K chunk transfers
> have been queued, but might not been actually transferred via the PCI, this
> is due to the ASYNC_write() operation.

Actually, the task queues all the transfers, then sleeps. The priority
structure is such that, at the present time, that task should not get called
again until all of the transmitter tasks have run. It is supposed to run
again last, and tear down all the tasks. There is a bunch of commented out
code right after the sleep; this is code that was causing problems and was
therefore removed (at least temporarily). Ultimately, though, this task will
tear down and free all the resources, then branch back to sleep on the
semaphore awaiting another transfer request.

I think I will probably make that task sleep on a semaphore while waiting for
the transmitter tasks to complete and when all the transmitter tasks have
run, I'll wake it up somehow. For now, I have been having bigger problems.

> Does the host logic really expects
> this? I guess that here the place where race condition might occur. What
> would happen if the buildall_tsk() would sleep in its infinite loop before
> setting an interrupt to the host for a second. This will certainly make all
> the outstanding transfers to complete.
>
> I also wonder if it is absolutely necessary to create a task to handle a
> 64K chunk, an async chan_submit call would postpone in a queue transfers
> that are not possible to start instantly, therefore a single task can call
> ASYNC_write() in a loop similar to what is used to create separate tasks.
> The order of transactions might be different, but the overall result is the
> same :)
>
I am creating tasks for this because the demo program I started with created
tasks for this. There is a commented out line in the program where at one
point I had changed the task creation into a straight subroutine call. I had
problems so I went back to independent tasks. I was just reconsidering that
this afternoon; I have figured a lot of this out and I am thinking that
queueing the transfers without spawning extra tasks might work better, so I
might comment out the task creation line, uncomment the subroutine line, and
try it again.

For now, this continues to be a learning exercise for me.

> By the way, currently the CHAN_NUM constant is 1, which means that only one
> transmitter task is created, no matter what the transfer size is.
>
Yes. I set it simple to try to get a simple case to work.

> Did you find out how to assign the mb_isr() to INT13 instead of default
> INT4?
>
I am letting the c64xx_pci.c mini driver set it. That driver is setting it to
6, based upon a definition in the .h file.

> Hope this helps,
>
> Andrew
>
You have been a big help, and I definitely appreciate it.

-------------------