[Cbe-oss-dev] [RFC 4/4] spufs: SPE side implementation of kspu

[Sebastian Siewior]

* Geoff Levand | 2007-07-31 12:12:58 [-0700]:

>> +struct kspu_context {
>> +	struct spu_context *spu_ctx;
>> +	wait_queue_head_t newitem_wq;
>> +	void **notify_cb_info;
>> +	unsigned int last_notified;
>> +	struct kspu_code *spu_code;
>> +	struct task_struct *thread;
>> +	/* spinlock protects qlen + work_queue */
>> +	spinlock_t queue_lock;
>> +	unsigned int qlen;
>> +	struct list_head work_queue;
>> +};
>
>
>Since this is where struct kspu_context is defined, shouldn't this patch
>be applied earlier in your series, or put the kspu_util.h hunks in an
>earlier patch?

Yep, sounds good.

>> --- /dev/null
>> +++ b/arch/powerpc/platforms/cell/spufs/spu_main.c
>> @@ -0,0 +1,122 @@
>> +/*
>> + * This code can be considered as crt0.S
>> + * Compile with -O[123S] and make sure that here is only one function
>> + * that starts at 0x0
>> + * Author: Sebastian Siewior <sebastian at breakpoint.cc>
>> + * License: GPLv2
>> + */
>> +#include <asm/kspu/merged_code.h>
>> +#include <spu_mfcio.h>
>> +#include "spu_runtime.h"
>> +
>> +#define barrier() __asm__ __volatile__("": : :"memory")
>> +
>> +static spu_operation spu_funcs[TOTAL_SPU_FUNCS] __attribute__((aligned(16))) = {
>> +	[SPU_FUNC_nop] = spu_nop,
>> +};
>> +
>> +static unsigned char kspu_buff[DMA_BUFFERS][DMA_MAX_TRANS_SIZE];
>> +
>> +void _start(void) __attribute__((noreturn));
>> +void _start(void)
>> +{
>> +	struct kernel_spu_data *spu_data;
>> +
>> +	spu_data = (struct kernel_spu_data *) KERNEL_SPU_DATA_OFFSET;
>> +
>> +	while (37) {
>
>
>This is a bit strange, convention is to use while(1), which you use elsewhere.

I've watched too much Kevin Smith movies. I could change this for consistency
if it is a must.

>
>> +		struct kspu_job *kjob;
>> +		void *dma_buff;
>> +		unsigned int consumed;
>> +		unsigned int outstanding;
>> +		unsigned int cur_req;
>> +		unsigned int cur_item;
>> +		unsigned int cur_buf;
>> +		unsigned int i;
>> +
>> +		spu_stop(1);
>> +		/*
>> +		 * Once started, it is guaranteed that atleast DMA_BUFFERS *2
>> +		 * requests are in ring buffer. The work order is:
>> +		 * 1. request DMA_BUFFERS transfers, every in a seperate buffer
>> +		 *    with its own tag.
>> +		 * 2. process those buffers and request new ones.
>> +		 * 3. if more than (DMA_BUFFERS *2) are available, than the
>> +		 *    main loop begins:
>> +		 *   - wait for tag to finish transfers
>> +		 *   - notify done work
>> +		 *   - process request
>> +		 *   - write back
>> +		 * 4. if no more request are available, process the last
>> +		 *    DMA_BUFFERS request that are left, write them back and
>> +		 *    wait until that transfers completes and spu_stop()
>> +		 */
>> +
>...
>> --- /dev/null
>> +++ b/include/asm-powerpc/kspu/merged_code.h
>> @@ -0,0 +1,43 @@
>> +#ifndef KSPU_MERGED_CODE_H
>> +#define KSPU_MERGED_CODE_H
>> +
>> +#define KSPU_LS_SIZE 0x40000
>> +
>> +#define RB_SLOTS 256
>> +#define RB_MASK (RB_SLOTS-1)
>> +
>> +#define DMA_MAX_TRANS_SIZE (16 * 1024)
>> +#define DMA_BUFFERS   2
>> +#define DMA_BUFF_MASK (DMA_BUFFERS-1)
>> +#define ALL_DMA_BUFFS ((1 << DMA_BUFFERS)-1)
>> +
>> +enum SPU_FUNCTIONS {
>> +	SPU_FUNC_nop,
>> +
>> +	TOTAL_SPU_FUNCS,
>> +};
>> +
>> +struct kspu_job {
>> +	enum SPU_FUNCTIONS operation __attribute__((aligned(16)));
>> +	unsigned long long in __attribute__((aligned(16)));
>> +	unsigned int in_size __attribute__((aligned(16)));
>> +	union {
>> +	} __attribute__((aligned(16)));
>> +};
>
>What is this empty union for?

The "offloaded" SPU functions will fill the union.

>> +
>> +typedef void (*spu_operation)(struct kspu_job *kjob, void *buffer,
>> +		unsigned int buf_num);
>
>
>Would this type be better as spu_operation_t?
Yes.


Thanks review.

Sebastian