On 27 February 2016 at 00:16, Andrew Baumann
<andrew.baum...@microsoft.com> wrote:
> The framebuffer occupies the upper portion of memory (64MiB by
> default), but it can only be controlled/configured via a system
> mailbox or property channel (to be added by a subsequent patch).
>
> Signed-off-by: Andrew Baumann <andrew.baum...@microsoft.com>
Mostly looks ok, but a couple of points:
> +static void draw_line_src16(void *opaque, uint8_t *dst, const uint8_t *src,
> + int width, int deststep)
> +{
> + BCM2835FBState *s = opaque;
> + uint16_t rgb565;
> + uint32_t rgb888;
> + uint8_t r, g, b;
> + DisplaySurface *surface = qemu_console_surface(s->con);
> + int bpp = surface_bits_per_pixel(surface);
> +
> + while (width--) {
> + switch (s->bpp) {
> + case 8:
> + rgb888 = ldl_phys(&s->dma_as, s->vcram_base + (*src << 2));
Don't use ldl_phys() in device model code, please. It means "do
a load with the endianness of the CPU's bus", and generally
device behaviour doesn't depend on the what the CPU happens to
be doing. If you do a grep for 'ld[a-z]*_phys' in hw/ you'll find
that (apart from a few spurious matches) the only things doing this
are some bcm2835 code that you should fix and the spapr hypercall
device (which really is legitimately cpu-behaviour-dependent).
You need to determine what endianness the device uses to do
its DMA accesses, and use either ldl_le_phys() or ldl_be_phys()
as appropriate. (Or address_space_ldl_le/be if you care about
memory attributes.)
More interestingly, why can't you just read from the source
pointer you're passed in here? The framebuffer_update_display()
code should have obtained it by looking up the location of the
host RAM where the guest video RAM is, so if you ignore it and
do a complete physical-address-to-memory-region lookup for every
pixel it's going to make redraws unnecessarily slow.
> + r = (rgb888 >> 0) & 0xff;
> + g = (rgb888 >> 8) & 0xff;
> + b = (rgb888 >> 16) & 0xff;
> + src++;
> + break;
> + case 16:
> + rgb565 = lduw_p(src);
> + r = ((rgb565 >> 11) & 0x1f) << 3;
> + g = ((rgb565 >> 5) & 0x3f) << 2;
> + b = ((rgb565 >> 0) & 0x1f) << 3;
> + src += 2;
> + break;
> + case 24:
> + rgb888 = ldl_p(src);
> + r = (rgb888 >> 0) & 0xff;
> + g = (rgb888 >> 8) & 0xff;
> + b = (rgb888 >> 16) & 0xff;
> + src += 3;
> + break;
> + case 32:
> + rgb888 = ldl_p(src);
> + r = (rgb888 >> 0) & 0xff;
> + g = (rgb888 >> 8) & 0xff;
> + b = (rgb888 >> 16) & 0xff;
> + src += 4;
> + break;
> + default:
> + r = 0;
> + g = 0;
> + b = 0;
> + break;
> +static void bcm2835_fb_mbox_push(BCM2835FBState *s, uint32_t value)
> +{
> + value &= ~0xf;
> +
> + s->lock = true;
> +
> + s->xres = ldl_phys(&s->dma_as, value);
> + s->yres = ldl_phys(&s->dma_as, value + 4);
> + s->xres_virtual = ldl_phys(&s->dma_as, value + 8);
> + s->yres_virtual = ldl_phys(&s->dma_as, value + 12);
> + s->bpp = ldl_phys(&s->dma_as, value + 20);
> + s->xoffset = ldl_phys(&s->dma_as, value + 24);
> + s->yoffset = ldl_phys(&s->dma_as, value + 28);
> +
> + s->base = s->vcram_base | (value & 0xc0000000);
> + s->base += BCM2835_FB_OFFSET;
> +
> + /* TODO - Manage properly virtual resolution */
> +
> + s->pitch = s->xres * (s->bpp >> 3);
> + s->size = s->yres * s->pitch;
> +
> + stl_phys(&s->dma_as, value + 16, s->pitch);
> + stl_phys(&s->dma_as, value + 32, s->base);
> + stl_phys(&s->dma_as, value + 36, s->size);
These should all be specifying which endianness to write
explicitly too.
> +
> + s->invalidate = true;
> + qemu_console_resize(s->con, s->xres, s->yres);
> + s->lock = false;
> +}
thanks
-- PMM
