[Cbe-oss-dev] [RFC 4/5] spufs: add kernel support for spu task

[Jeremy Kerr]

Hi Sebastian,

> Utilisation of SPUs by the kernel, main implementation.
> The idea behind this implemtation is that there are single jobs that
> are executed asynchronous on the SPU. The user queues jobs with
> enqueue_for_spu() and gets a callback once the job is completed. The
> function itself does not block, it returns -EINPROGRESS.

As Ben said, just return 0 instead.

Also, I'm a little confused about the queue API - the user queues a 
kspu_work_item with this function, and it later gets an 'enqueue' 
callback, where it may put an entry on the ringbuffer, right?

> +config SPU_KERNEL_SUPPORT
> +	tristate "Support for utilisation of SPU by the kernel"
> +	default m
> +	depends on SPU_FS

Maybe this should depend on CONFIG_EXPERIMENTAL?

> +choice
> +	prompt "Slots in the ringbuffer"
> +	depends on SPU_KERNEL_SUPPORT
> +	default SPU_RINB_SLOTS_16
> +	help
> +	  This option allows you to determine the number of slots in the
> +	  ringbuffer. More slots means that more work may be queued on
> +	  the SPU before waiting for completion. The number of slots must
> +	  be power of two. Every slot takes approx 128 bytes of memory.

Does this need to be a config option?

(if so, can we make this tied to the KSPU options, eg KSPU_RING_SLOTS ?)

>  SPU_CFLAGS	:= -O2 -Wall -I$(srctree)/include \
> -		   -I$(objtree)/include2 -D__KERNEL__
> +		   -I$(objtree)/include2 -D__KERNEL__ -ffreestanding

What happens without -ffreestanding?

> +
> +struct kspu_code single_spu_code = {
> +	.code = (unsigned char*) spu_kspu_code,

Would be good to keep the .code member the same type as the symbol here.

> --- /dev/null
> +++ b/arch/powerpc/platforms/cell/spufs/kspu_helper.c
> @@ -0,0 +1,574 @@
> +/*
> + * Helper function for dealing with kspus
> + */

I'd say that these aren't just 'helper' functions, they're the core API. 
Might be better to call this arch/powerpc/platforms/cell/spusf/kspu.c

> +	if (!ret)
> +		WARN_ON(1);
> +	else {
> +		kfree(kctx->notify_cb_info);
> +		kfree(kctx);
> +	}

Minor one, but keep the bracketing consistent within one if/else block.


> +static void set_reg32(struct spu_lscsa *lscsa, int reg, unsigned int 
p) +{
> +	lscsa->gprs[reg].slot[0] = p;
> +}

Would this be useful for other (non-KSPU) purposes? should we make this 
a generic SPU function?

> +	 * |      آ�      |

I get some whacky character here that puts my mailer in right-to-left 
text mode.

> +#define BACKCHAIN (kctx->spu_code->data_offset - 16)

This makes me think that the data_offset member is misnamed, as it has 
nothing to do with the 'data' section of the KSPU program.

> +#define STACK_GAP 176
> +#define INITIAL_STACK (BACKCHAIN - STACK_GAP)
> +
> +	/*
> +	 * if kspu_data_offset is too large it will overflow not allocated
> +	 * data will be modofied
> +	 */
> +	if (INITIAL_STACK > KSPU_LS_SIZE)
> +		BUG();

BUG_ON ?

> +	lscsa = ctx->csa.lscsa;
> +	set_reg32(lscsa, 1, (unsigned int) (INITIAL_STACK));

we probably don't need the 'lscsa' variable for only one usage.

> +	spu_release(ctx);
> +	printk("Stack ready 0x%x08\n", INITIAL_STACK);

0x%08x? Also, if you're going to keep these printk()s in a later patch, 
they'd probably be better as pr_debug()s.

> +	ret = -EFBIG;
> +	printk("SPU's queue: %d elemets, %d bytes each (%d bytes total)\n",
> +			spu_code->queue_mask+1, spu_code->queue_entr_size,
> +			(spu_code->queue_mask+1) * spu_code->queue_entr_size);
> +	ring_space = (spu_code->queue_mask+1) * spu_code->queue_entr_size;
> +	ring_space += sizeof(struct kspu_ring_data);
> +	if (spu_code->data_offset != KSPU_LS_SIZE - ring_space) {
> +		printk("data offset: %04x, ring space: %04x\n",
> spu_code->data_offset, ring_space);
> +		printk("data expect: %04x\n", KSPU_LS_SIZE - ring_space);
> +	} 
> +
> +	if (spu_code->code_len > KSPU_LS_SIZE)
> +		goto err;

Put the 'ret = -EFBIG' closer to the error check.


> +	kctx->notify_cb_info = kzalloc(sizeof(void*) *

              sizeof(*kctx->notify_cb_info) ----^   ?

> +
> +	do {
> +		ret = spufs_run_spu(ctx, npc, event);
> +		if (ret & SPU_STATUS_STOPPED_BY_STOP) {
> +			if (ret >> SPU_STOP_STATUS_SHIFT == 0x1234) {

We'll need to formalise this stopcode somehow, a #define should do the 
trick.


> +
> +	printk("Entering main loop\n");
> +
> +	do {
> +		fastpath = 0;

What conditions can we take the fast path? can we rename the variable to 
reflect this?

> +static struct kspu_context *kspu_ctx;

As ben mentioned, we could have one per physical SPE, or even make this 
configurable? or create contexts as required?


> Index: b/arch/powerpc/platforms/cell/spufs/spu_main.c
> ===================================================================
> --- /dev/null
> +++ b/arch/powerpc/platforms/cell/spufs/spu_main.c
> @@ -0,0 +1,35 @@
> +/*
> + * 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
> + */
> +#include <spu_mfcio.h>
> +
> +#include <asm/kspu/merged_code.h>
> +
> +spu_operation spu_ops[TOTAL_SPU_FUNCS] __attribute__((aligned(16)))

ops vs. funcs ?

> = { +
> +};
> +
> +#define DMA_TAG 1
> +void _start(void) __attribute__((noreturn));
> +void _start(void)
> +{
> +	struct kernel_spu_data *spu_data;
> +
> +	mfc_write_tag_mask(1<< DMA_TAG);

Why do you set the tag here ?

> +	spu_data = (struct kernel_spu_data*) KERNEL_SPU_DATA_OFFSET;
> +
> +	while (42) {
> +
> +		unsigned int consumed = spu_data->kspu_ring_data.consumed;
> +		unsigned int outstanding = spu_data->kspu_ring_data.outstanding;
> +
> +		if (consumed != outstanding) {
> +			int cons = consumed & (RB_SLOTS-1);
> +
> +			spu_ops[spu_data->work_item[cons].operation](cons);
> +		} else
> +			spu_stop(0x99);

We should probably #define this.


> Index: b/include/asm-powerpc/kspu/merged_code.h
> ===================================================================
> --- /dev/null
> +++ b/include/asm-powerpc/kspu/merged_code.h
> @@ -0,0 +1,46 @@
> +#ifndef KSPU_MERGED_CODE_H
> +#define KSPU_MERGED_CODE_H
> +#include <linux/autoconf.h>
> +
> +#define KSPU_LS_SIZE 0x40000

This is defined in spu.h:

include/asm-powerpc/spu.h:#define LS_SIZE (256 * 1024)

Can we somehow share this with the SPE-application header?

Cheers,


Jeremy