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

[Andrew Baumann]

> From: Peter Maydell [mailto:address@hidden
> Sent: Tuesday, 1 March 2016 11:23 AM
> 
> 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.)

Thanks for pointing this out. I'll get the other instances fixed (they are all 
le).

> 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.

That is what's happening for the 16-, 24- and 32-bit modes. For 8-bit, it 
appears to be doing a palette lookup (using the 8-bit value at the pointer as 
an index into the 256-colour palette starting at vcram_base). Your point that 
this is inefficient stands, but the translation afforded by the existing 
framebuffer code doesn't help. I'm tempted to replace it with ldl_le_phys as a 
quick fix.

Regards,
Andrew