I assume this is a step towards VNC accessible video output. There are some issues, most importantly (for me) the integration of the generic bitmap stuff. This would correspond to the integration of generic VNC stuff eventually, and that wouldn't do me any good with x86. There are lots of style bugs that need to be fixed.

src/dev/arm/RealView.py (Diff revision 1)

It seems pointless to align this Param with the one below it but none of the others. I think aligning them generally is pointless, so you might just pull this one in.

src/dev/arm/RealView.py (Diff revision 1)

Inconsistent alignment

src/dev/arm/RealView.py (Diff revision 1)

Inconsistent alignment again.

src/dev/arm/amba_device.cc (Diff revision 1)

Since this occurs twice and is consistent, it should probably be a constant with a name. If it's only meaninful locally it would be fine to leave it in the .cc.

src/dev/arm/pl111.hh (Diff revision 1)

I'd like these to be an enum, but more I'd really like these to not look like macros. LCD_TIMING0 => LcdTiming0, etc.

src/dev/arm/pl111.hh (Diff revision 1)

These are types, not values. They should not use all caps.

src/dev/arm/pl111.hh (Diff revision 1)

You need spaces around the operators.

src/dev/arm/pl111.hh (Diff revision 1)

ditto

src/dev/arm/pl111.hh (Diff revision 1)

If this is a generic utility class (and I think it is) it should go in some other file for others to use. Maybe in base. You have an errant space after class it looks like.

src/dev/arm/pl111.hh (Diff revision 1)

The formatting of this comment seems fairly mangled. It also looks copied from somewhere. What did this come from originally? Do we have license to use it?

src/dev/arm/pl111.cc (Diff revision 1)

These offsets seem to be basically sequential, less their 4 byte size. Perhaps this would be better implemented as an array with an index instead of a switch?

src/dev/arm/pl111.cc (Diff revision 1)

Forgive my laziness, but is this normally how DMAs are done? It seems fairly manual. Don't we have a base class for this?

src/dev/arm/pl111.cc (Diff revision 1)

space before {, not after dmaSize. I won't mark all the style problems, but there are others.

src/dev/arm/pl111.cc (Diff revision 1)

{ goes on the line with the if, space after the if, space after the ==.

src/dev/arm/pl111.cc (Diff revision 1)

tear, not tare. Double buffering doesn't really make sense here since we don't have asynchronous screen updates. Just send out the bitmap whenever it's done. As a matter of fact that looks like what's already happening. You're doing a memcpy and then just immediately using the memcpy. Nothing's going to happen to the original in the mean time.

src/dev/arm/pl111.cc (Diff revision 1)

This needs endianness handling. Also, if the framebuffer doesn't -mean- anything as 32 bit integers, ie its not interpretted, just shuttled in from writes, out with reads, and dumped to a file, why not just store it as chars in the first place?

src/dev/arm/pl111.cc (Diff revision 1)

The 32 bit value is RGBA format. Currently that is the only thing that we support in the lcd controller so it can conveniently be written directly to the file. If the lcd controller wanted to do something else it would need to convert. Either way we assume that we get RGBA in little endian format, that might be what the lcd controller gets or it might not, but the lcd controller will do the conversion. The code is really temporary until the vnc server gets cleaned up and can be committed. I don't expect the bmp code to stick around for long.