> From: Peter Maydell [mailto:peter.mayd...@linaro.org] > Sent: Tuesday, 1 March 2016 11:23 AM > > 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.)
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
