Re: [Qemu-arm] [PATCH 3/4] bcm2835_fb: add framebuffer device for Raspberry Pi

[Peter Maydell]

On 27 February 2016 at 00:16, Andrew Baumann
<address@hidden> 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 <address@hidden>

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