[Cbe-oss-dev] powerpc/ps3: ps3vram driver for accessing video RAM as MTD

[Arnd Bergmann]

On Tuesday 04 November 2008, Jim Paris wrote:
> Add ps3vram driver, which exposes unused video RAM on the PS3 as a MTD
> device suitable for storage or swap.  Fast data transfer is achieved
> using a local cache in system RAM and DMA transfers via the GPU.

The code looks alright mostly, but what made you do it as an MTD driver?
This would be much easier to do as a block device driver, similar to
axonram. This also has the advantage that you can use it with direct mapping,
e.g. wiht axfs or ext2 xip to run binaries directly from video memory,
or as a backing store for azfs so that the SPUs can use it as a fast
DMA target.

> +#define dbg(fmt, args...)	\
> +	pr_debug("%s:%d " fmt "\n", __func__, __LINE__, ## args)

Please don't define your own wrappers like this. Either use
a plain pr_debug, or the better dev_dbg().

> +struct ps3vram_priv {
> +	uint64_t memory_handle;
> +	uint64_t context_handle;
> +	uint8_t *base;
> +	uint32_t *ctrl;
> +	uint32_t *reports;
> +	uint8_t *xdr_buf;
> +
> +	uint32_t *fifo_base;
> +	uint32_t *fifo_ptr;

Most people don't use the stdint.h types in the kernel, for historic
reasons. When you have a memory address, that is normally represented
as a dma_addr_t or an unsigned long. 

> +static int ps3vram_notifier_wait(struct mtd_info *mtd, int timeout_ms)
> +{
> +	struct ps3vram_priv *priv = mtd->priv;
> +	uint32_t *notify = ps3vram_get_notifier(priv->reports, NOTIFIER);
> +
> +	timeout_ms *= 1000;
> +
> +	do {
> +		if (notify[3] == 0)
> +			return 0;
> +
> +		if (timeout_ms)
> +			udelay(1);
> +	} while (timeout_ms--);
> +
> +	return -1;
> +}

This kind of busy-looping is considered very bad. Can't you change
the code so that this is called in non-atomic context and you can do
msleep()?

> +static void ps3vram_dump_ring(struct mtd_info *mtd)
> +{
> +	struct ps3vram_priv *priv = mtd->priv;
> +	uint32_t *fifo;
> +
> +	pr_info("PUT = %08x GET = %08x\n", priv->ctrl[CTRL_PUT],
> +		priv->ctrl[CTRL_GET]);
> +	for (fifo = priv->fifo_base; fifo < priv->fifo_ptr; fifo++)
> +		pr_info("%p: %08x\n", fifo, *fifo);
> +}
> +
> +
> +static void ps3vram_dump_reports(struct mtd_info *mtd)
> +{
> +	struct ps3vram_priv *priv = mtd->priv;
> +	int i;
> +
> +	for (i = 0; i < NUM_NOTIFIERS; i++) {
> +		uint32_t *n = ps3vram_get_notifier(priv->reports, i);
> +		pr_info("%p: %08x\n", n, *n);
> +	}
> +}

If you only needed this code for debugging, maybe you should
now remove it.

> +static void ps3vram_init_ring(struct mtd_info *mtd)
> +{
> +	struct ps3vram_priv *priv = mtd->priv;
> +
> +	priv->ctrl[CTRL_PUT] = FIFO_BASE + FIFO_OFFSET;
> +	priv->ctrl[CTRL_GET] = FIFO_BASE + FIFO_OFFSET;
> +}
> +
> +static int ps3vram_wait_ring(struct mtd_info *mtd, int timeout)
> +{
> +	struct ps3vram_priv *priv = mtd->priv;
> +
> +	/* wait until setup commands are processed */
> +	timeout *= 1000;
> +	while (--timeout) {
> +		if (priv->ctrl[CTRL_PUT] == priv->ctrl[CTRL_GET])
> +			break;
> +		udelay(1);
> +	}
> +	if (timeout == 0) {
> +		pr_err("FIFO timeout (%08x/%08x/%08x)\n", priv->ctrl[CTRL_PUT],
> +		       priv->ctrl[CTRL_GET], priv->ctrl[CTRL_TOP]);
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}

Again, try avoiding udelay.

> +	priv->base = ioremap(ddr_lpar, ddr_size);
> +	if (!priv->base) {
> +		pr_err("ps3vram: ioremap failed\n");
> +		ret = -ENOMEM;
> +		goto out_free_context;
> +	}
> +
> +	priv->ctrl = ioremap(ctrl_lpar, 64 * 1024);
> +	if (!priv->ctrl) {
> +		pr_err("ps3vram: ioremap failed\n");
> +		ret = -ENOMEM;
> +		goto out_unmap_vram;
> +	}
> +
> +	priv->reports = ioremap(reports_lpar, reports_size);
> +	if (!priv->reports) {
> +		pr_err("ps3vram: ioremap failed\n");
> +		ret = -ENOMEM;
> +		goto out_unmap_ctrl;
> +	}

ioremap returns a noncacheable guarded mapping. Are you should you
want that for the memory? Normally, it should at least not be guarded,
and possibly even cached, if you don't need to keep it consistent
with the view from the GPU.

	Arnd <><