[Cbe-oss-dev] [patch 07/10] spufs: add kernel support for spu task

[Arnd Bergmann]

On Wednesday 29 August 2007, Sebastian Siewior wrote:
> Arnd, it worked out with
> 
> if (process_kspu_events(kctx))
>         continue;

Ah, much better now ;-)


 A few more details I just noticed:

>  
> +config KSPU
> +	bool
> +  default n
> +

default n is redundant.

> +
> +	ctx->csa.lscsa->gprs[1].slot[0] = INITIAL_STACK;
> +	spu_release(ctx);
> +	pr_debug("SPU's stack ready 0x%04x\n", INITIAL_STACK);
> +}

this needs to be spu_release_saved() instead of spu_release().

> +	ret = -ENOMEM;
> +	kctx = kzalloc(sizeof *kctx, GFP_KERNEL);
> +	if (!kctx)
> +		goto err;
> +
> +	kctx->qlen = 0;
> +	kctx->spu_code = spu_code;
> +	init_waitqueue_head(&kctx->newitem_wq);
> +	spin_lock_init(&kctx->queue_lock);
> +	INIT_LIST_HEAD(&kctx->work_queue);
> +	kctx->notify_cb_info = kzalloc(sizeof(*kctx->notify_cb_info) *
> +			(kctx->spu_code->queue_mask + 1), GFP_KERNEL);

You can do a single allocation here, to make the cleanup logic simpler.

> +	spu_acquire(ctx);
> +	ls = ctx->ops->get_ls(ctx);
> +	memcpy(ls, spu_code->code, spu_code->code_len);
> +	spu_release(ctx);
> +	setup_stack(kctx);

setup_stack immediately does an spu_acquire again. Maybe move the
memcpy in there?

> +static void check_spu_state(struct kspu_context *kctx, unsigned int *status)
> +{
> +	struct spu_context *ctx = kctx->spu_ctx;
> +	unsigned int npc;
> +
> +	if (!(*status & SPU_STATUS_RUNNING)) {
> +		/* spu is currently not running */
> +		pr_debug("SPU not running, last stop code was: %08x\n",
> +				*status >> SPU_STOP_STATUS_SHIFT);
> +		if (pending_spu_work(kctx)) {
> +			/* spu should run again */
> +			pr_debug("Activate SPU\n");
> +			kspu_fill_dummy_reqs(kctx);
> +
> +			npc = ctx->ops->npc_read(ctx);
> +			spu_release(ctx);
> +			spu_acquire_runnable(ctx, 0);
> +			spu_run_init(ctx, &npc);
> +
> +			if (unlikely(test_and_clear_bit(SPU_SCHED_NOTIFY_ACTIVE,
> +					&ctx->sched_flags)))
> +				spu_switch_notify(ctx->spu, ctx);
> +		} else {
> +			/* spu finished work */
> +			pr_debug("SPU will remain in stop state\n");
> +			spu_run_fini(ctx, &npc, status);
> +			spu_yield(ctx);
> +			spu_acquire(ctx);
> +		}
> +	} else {
> +		pr_debug("SPU is running, switch state to util user\n");
> +		spuctx_switch_state(ctx, SPU_UTIL_USER);
> +
> +		if (unlikely(test_and_clear_bit(SPU_SCHED_NOTIFY_ACTIVE,
> +						&ctx->sched_flags)))
> +			spu_switch_notify(ctx->spu, ctx);
> +	}
> +}

If you reverse the first if(), you can reduce the indentation level, like
this:

	if ((*status & SPU_STATUS_RUNNING)) {
		pr_debug("SPU is running, switch state to util user\n");
		spuctx_switch_state(ctx, SPU_UTIL_USER);
		if (unlikely(test_and_clear_bit(SPU_SCHED_NOTIFY_ACTIVE,
						&ctx->sched_flags)))
			spu_switch_notify(ctx->spu, ctx);
		return;
	}

	/* spu is currently not running */
	pr_debug("SPU not running, last stop code was: %08x\n",
			*status >> SPU_STOP_STATUS_SHIFT);
	if (pending_spu_work(kctx)) {
		...
	}


	Arnd <><