storing two data types in a single bigger data type

Question

A bit of introduction to the problem,
I tried searching about this on google/stack before I post this but most of them were unclear.
I have a cortex-a8 based board on which I'm running a bare metal RTOS, The display (framebuffer) is a bit slow because I haven't implemented DMA for my target right now but is not all that slow, however I noticed an opportunity of improvement. On my CPU and toolchain combo, 32 bit math, data access is faster than 16 bit access and the display is 16 bit rgb565 so some of the framebuffer ops are a bit slower than what they could have been (Some of them use memcpy, memmove and memset which takes care of data alignment etc..)

What I attempted was to cram two pixels into one 32 bit data type and use that to access memory (aligned as far as i remember, even if not, my cpu supports inaligned memory access in hardware so the problem shouldn't be this.) Note that I'm not talking of speed of my implementation but a weird effect I'm getting which I suspect is because How I'm cramming the two pixels into one 32 bit data type.

Here is most part of my fb_putc

if (((unsigned char)c > 32) && ((unsigned char) c < 127)) {
    check_for_scroll(49);

    // fontdata starts from ASCII 33 shifted by logarithm(base2, font_height)
    c -= 33;
    c <<= 4;

    uint16_t pallete_16[2] = {fb.fg_color, fb.tg_color};

    uint32_t y;
    uint32_t *pixel_32;
    uint32_t fb_shifter;
    uint32_t pixel_32_holder;
    uint32_t fb_bg_32 = ((pallete_16[1] << 16) | (pallete_16[1]));
    /*
     * Each pixel is 16 bits, we access them using 32 bit data type,
     * which is faster for aligned memory access. Also many architectures
     * have free bit shifts with each instruction so we use that too.
     */
    pixel_32 = (uint32_t *) fb.config->base;
    pixel_32 += ( ((fb.cursor.y * (FONT_HEIGHT * fb.config->width)) + ((fb.cursor.x * (FONT_WIDTH))))
                    / ((sizeof(uint32_t))/(sizeof(uint16_t))) );
    for (y = 0; y < 16; y++) {
        for ( unsigned x = 7; x >= 0; x -= 2 )
        {
            if (fontdata[c + y] & (1 << x)) {   
                pixel_32_holder = (pallete_16[0] << 16);
            } else {
                pixel_32_holder = (pallete_16[1] << 16);
            }
            if (fontdata[c + y] & (1 << (x -1))) {
                pixel_32_holder |= (pallete_16[0] & 0xffff);
            } else {
                pixel_32_holder |= (pallete_16[1] & 0xffff);
            }
            *pixel_32++ = pixel_32_holder;
        }
        // Panel stride = width (480) - font_width (8)
        pixel_32 += (472 / ((sizeof(uint32_t))/(sizeof(uint16_t))));
    }

    fb.cursor.x++;
}

Any help regarding where I went wrong? I'm a bit new to programming and am doing this as hobby.

Answer 1

Your idea of combining 2 pixels before writing them to memory is correct. The write buffer hardware of ARM will be used more efficiently this way and the code will run faster. I don't think mixing C and ASM in that form will produce the best results. Sticking with pure ASM will guarantee that you're using conditionally executed instructions. Also, the use of an array for your palette may cause the compiler to output very inefficient code. Here's a way to do it more efficiently in pure ASM. Unrolling the loop is a good idea. This is the code to handle each byte of bitonal font data.

@ Register usage
@ R0 = source data pointer
@ R1 = destination data pointer
@ R2 = foreground color (loaded outside of loop)
@ R3 = background color (loaded outside of loop)
@ R4,R5 = temp registers
@ Assumes that the most significant short of each 32-bit word is on the left

  ldrb r4,[r0],#1  @ source bitonal image data
@ first pair of pixels
  tst r4,#0x80
  movne r5,r5,r2,LSL #16
  moveq r5,r5,r3,LSL #16
  tst r4,#0x40
  orrne r5,r5,r2
  orreq r5,r5,r3
  str r5,[r1],#4
@ second pair of pixels
  tst r4,#0x20
  movne r5,r5,r2,LSL #16
  moveq r5,r5,r3,LSL #16
  tst r4,#0x10
  orrne r5,r5,r2
  orreq r5,r5,r3
  str r5,[r1],#4
@ third pair of pixels
  tst r4,#0x8
  movne r5,r5,r2,LSL #16
  moveq r5,r5,r3,LSL #16
  tst r4,#0x4
  orrne r5,r5,r2
  orreq r5,r5,r3
  str r5,[r1],#4
@ fourth pair of pixels
  tst r4,#0x2
  movne r5,r5,r2,LSL #16
  moveq r5,r5,r3,LSL #16
  tst r4,#0x1
  orrne r5,r5,r2
  orreq r5,r5,r3
  str r5,[r1],#4

Update slightly simpler code

Answer 2

I've fixed the storing two pixels at a time thing by using asm after the compiler eating my head for the past few hours, but now seems there's some other problem as the characters appear garbled except for few of them, I am not sure what in the world is causing this...

As for packing the pixels, here's what i finally used (just incase someone in future might need to do this)

if (((unsigned char)c > 32) && ((unsigned char) c < 127)) {

    check_for_scroll(FB_MAX_Y_UNDER);

    uint32_t pixel_32_tmp;
    uint16_t pallete[2] = { (fb.fg_color), (fb.tg_color)};
    uint32_t *pixel_32 = (uint32_t *)fb.base +((((fb.cursor.y << 13)-(fb.cursor.y << 9))+(fb.cursor.x << 3)) >> 1);

    c -= 32;
    c <<= 4;

    for (int y = 0; y < 16; y++) {
        unsigned char font_bits = fontdata[c + y];

        if (font_bits & 0x80) {
            __asm__ volatile("mov %0, %1, lsl $16" : "=r" (pixel_32_tmp) : "r" (pallete[0]));
        } else {
            __asm__ volatile("mov %0, %1, lsl $16" : "=r" (pixel_32_tmp) : "r" (pallete[1]));
        }
        if (font_bits & 0x40) {
            __asm__ volatile("orr %0, %0, %1" : "=r" (pixel_32_tmp) : "r" (pallete[0]));
        } else {
            __asm__ volatile("orr %0, %0, %1" : "=r" (pixel_32_tmp) : "r" (pallete[1]));
        }
        *pixel_32++ = pixel_32_tmp;

        if (font_bits & 0x20) {
            __asm__ volatile("mov %0, %1, lsl $16" : "=r" (pixel_32_tmp) : "r" (pallete[0]));
        } else {
            __asm__ volatile("mov %0, %1, lsl $16" : "=r" (pixel_32_tmp) : "r" (pallete[1]));
        }
        if (font_bits & 0x10) {
            __asm__ volatile("orr %0, %0, %1" : "=r" (pixel_32_tmp) : "r" (pallete[0]));
        } else {
            __asm__ volatile("orr %0, %0, %1" : "=r" (pixel_32_tmp) : "r" (pallete[1]));
        }
        *pixel_32++ = pixel_32_tmp;

        if (font_bits & 0x08) {
            __asm__ volatile("mov %0, %1, lsl $16" : "=r" (pixel_32_tmp) : "r" (pallete[0]));
        } else {
            __asm__ volatile("mov %0, %1, lsl $16" : "=r" (pixel_32_tmp) : "r" (pallete[1]));
        }
        if (font_bits & 0x04) {
            __asm__ volatile("orr %0, %0, %1" : "=r" (pixel_32_tmp) : "r" (pallete[0]));
        } else {
            __asm__ volatile("orr %0, %0, %1" : "=r" (pixel_32_tmp) : "r" (pallete[1]));
        }
        *pixel_32++ = pixel_32_tmp;

        if (font_bits & 0x02) {
            __asm__ volatile("mov %0, %1, lsl $16" : "=r" (pixel_32_tmp) : "r" (pallete[0]));
        } else {
            __asm__ volatile("mov %0, %1, lsl $16" : "=r" (pixel_32_tmp) : "r" (pallete[1]));
        }
        if (font_bits & 0x01) {
            __asm__ volatile("orr %0, %0, %1" : "=r" (pixel_32_tmp) : "r" (pallete[0]));
        } else {
            __asm__ volatile("orr %0, %0, %1" : "=r" (pixel_32_tmp) : "r" (pallete[1]));
        }
        *pixel_32++ = pixel_32_tmp;

        pixel_32 += 236;
    }
    fb.cursor.x++;
}