[Cbe-oss-dev] [RFC/PATCH] ps3vram: new driver for accessing PS3 video RAM as MTD

[Geert Uytterhoeven]

	Hi Jim, Vivien,

On Wed, 23 Apr 2008, Jim Paris wrote:
> Attached is a new driver, ps3vram, that allows you to access the extra
> ~240MB of DDR video RAM on the PS3 as a MTD block device.  It can then
> be used as temporary storage or swap.  It's faster than other storage
> and is very useful for installers and live CDs that have no swap media.

Nice, thx!

> This version creates a cache in XDR RAM and uses the GPU to do DMA
> between the cache and DDR RAM, so it's reasonably fast:

> Known issues:
> - Currently it only works if ps3fb is loaded first, due to some
>   problem with GPU state.
> - lv1_gpu_memory_allocate gives us memory that overlaps the existing
>   framebuffer, so we ignore the first 16MB to avoid stepping on ps3fb.

You can look at ps3fb_videomemory.size to know exactly how much memory ps3fb
uses.

Alternatively, if ps3vram depends on ps3fb anyway, we may allocate all memory
in ps3fb and export to ps3vram how much it can use?

> - The XDR cache needs to be contiguous for DMA, so the driver won't
>   load if your memory is too fragmented.

Currently your cache is 7 x 256 KiB, allocated as one big block? Ps3fb and
ps3flash faced a similar problem, and preallocate memory in
arch/powerpc/platforms/ps3/setup.c.

You could reduce memory pressure by allocating individuals blocks of 256 KiB
instead.

Is there a special reason you chose 256 KiB for the cache blocks, e.g.
performance benchmarking?

Would it be possible to have this memory non-contiguous and use vmalloc(), i.e.
can the RSX 2D accel engine do scatter/gather, or can the copy be split in
multiple commands that each copy one or more 4 KiB pages?

> Comments welcome..

Global coding style comment: please use scripts/checkpatch.pl.

> --- /dev/null
> +++ b/drivers/mtd/devices/ps3vram.c
> @@ -0,0 +1,774 @@

> +#define BEGIN_RING(chan, tag, size) \
> +	OUT_RING(((size) << 18) | ((chan) << 13) | (tag))
> +#define OUT_RING(data) *(priv->fifo_ptr)++ = (data)
> +#define FIRE_RING() ps3vram_fire_ring(mtd)

Please use static inline functions instead of macros.

> +struct ps3vram_cache {

> +	int hit;
> +	int miss;

unsigned int?

But you removed the proc interface to the statistics, so it's no longer in use?

> +	uint32_t reserved;

I think this one needs a comment.

> +	struct mutex lock;

Same here.

> +/* XXX: reserved space in GDDR RAM, used by framebuffer */
> +#define SKIP_SIZE (16 * 1024 * 1024)

ps3fb_videomemory.size

> +static void ps3vram_notifier_reset(struct mtd_info *mtd)
> +{
> +	struct ps3vram_priv *priv = mtd->priv;
> +	uint32_t *notify = (uint32_t *) ((void *) priv->reports +
> +		DMA_NOTIFIER_OFFSET_BASE + DMA_NOTIFIER_SIZE * NOTIFIER);

This cast-laden construct is used several times, so you may want to hide it in
a static inline function.

> +	priv->ctrl[CTRL_PUT] = FIFO_BASE + FIFO_OFFSET +
> +		(priv->fifo_ptr - priv->fifo_base) * 4;
                                                     ^
						     sizeof(uint32_t)?

> +	if ((priv->fifo_ptr - priv->fifo_base) * 4 > FIFO_SIZE - 1024) {
						 ^               ^^^^
						 sizeof(uint32_t)?

Where does the 1024 come from? Perhaps DMA_NOTIFIER_OFFSET_BASE?

> +	if (cache->tags[entry].flags & CACHE_PAGE_DIRTY) {
> +		dbg("flushing %d : 0x%08x",
> +		    entry, (unsigned int) cache->tags[entry].address);
                           ^^^^^^^^^^^^^^
			   superfluous cast

> +static int ps3vram_read(struct mtd_info *mtd, loff_t from, size_t len,
> +                size_t *retlen, u_char *buf)
> +{
> +	struct ps3vram_priv *priv = mtd->priv;
> +	unsigned int cached, count;
> +
> +	dbg("from = 0x%08x len = 0x%x",
                                   ^^
				   %zx
> +	    (unsigned int) from, (unsigned int) len);
                                 ^^^^^^^^^^^^^^
				 and kill the cast

> +		dbg("from=%08x cached=%08x offset=%08x avail=%08x count=%08x",
> +		    (unsigned ) from, cached, offset, avail, (unsigned) count);
                                                             ^^^^^^^^^^
							     superfluous cast

> +		dbg("to=%08x cached=%08x offset=%08x avail=%08x count=%08x",
> +		    (unsigned ) to, cached, offset, avail, (unsigned) count);
                                                           ^^^^^^^^^^
							   superfluous cast
> +	ddr_size = (ddr_size + 1024 * 1024 - 1) & ~(1024 * 1024 - 1);

ddr_size = ALIGN(ddr_size, 1024*1024);

> +	printk(KERN_INFO "ps3vram: allocated %dMB of DDR memory\n",
                                             ^^^^
					     %u MiB

> +static int __init init_ps3vram(void)
> +{
> +	return register_device();

Why not inline register_device() here?

> +}
> +
> +static void __exit cleanup_ps3vram(void)
> +{
> +	unregister_device();

Why not inline unregister_device() here?

> +}

Looks really promising, and gave a nice performance boost to my PS3 desktop.

Thanks again!

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven at sonycom.com
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · BIC GEBABEBB08A · IBAN BE39001382358619