Re: [Qemu-devel] [PATCH] SM501 emulation for R2D-SH4

[Shin-ichiro KAWASAKI <kawasaki@juno.dti.ne.jp>]

Thank you for your review!

>> I'm not sure about following two points.
>>
>>  - Register definitions were copied from Linux : include/linux/sm501-regs.h
> 
> I'd try to suck it into the .c file because the definitions are not
> going to be used anywhere else in qemu.

OK.  I'll include it in sm501.c.


>> +#include "vga_int.h"
> 
> I think <vga_int.h> isn't needed?

Right.  Will be deleted.


>> +static void sm501_draw_crt(SM501State * s)
>> +{
>> +    int x, y;
>> +    uint32_t crt_width = (s->dc_crt_h_total & 0x00000FFF) + 1;
>> +    uint32_t crt_height = (s->dc_crt_v_total & 0x00000FFF) + 1;
>> +    uint8_t  * buf = s->local_mem;
>> +    uint32_t * palette = (uint32_t *)s->dc_crt_palette;
>> +
>> +    /* adjust console size */
>> +    if (s->last_width != crt_width || s->last_height != crt_height) {
>> +       qemu_console_resize(s->console, crt_width, crt_height);
>> +       s->last_width = crt_width;
>> +       s->last_height = crt_height;
>> +    }
>> +
>> +    switch (s->dc_crt_control & 3) {
>> +    case SM501_DC_CRT_CONTROL_8BPP:
>> +       for (y = 0; y < crt_height; y++) {
>> +           for (x = 0; x < crt_width; x++) {
>> +               int i = (y * crt_width + x) * 4;
>> +               *(uint32_t *)&s->ds->data[i] = palette[*buf];
>> +               buf++;
>> +           }
>> +       }
>> +       break;
>> +    case SM501_DC_CRT_CONTROL_16BPP:
>> +       for (y = 0; y < crt_height; y++) {
>> +           for (x = 0; x < crt_width; x++) {
>> +               int i = (y * crt_width + x) * 4;
>> +               uint32_t rgb565 = *(uint16_t*)buf;
>> +               int r = ((rgb565 >> 11) & 0x1f) << 3;
>> +               int g = ((rgb565 >>  5) & 0x3f) << 2;
>> +               int b = ((rgb565 >>  0) & 0x1f) << 3;
>> +               s->ds->data[i + 0] = b;
>> +               s->ds->data[i + 1] = g;
>> +               s->ds->data[i + 2] = r;
>> +               s->ds->data[i + 3] = 0;
>> +               buf += 2;
>> +           }
>> +       }
>> +       break;
>> +    case SM501_DC_CRT_CONTROL_32BPP:
>> +       for (y = 0; y < crt_height; y++) {
>> +           for (x = 0; x < crt_width; x++) {
>> +               int i = (y * crt_width + x) * 4;
>> +               *(uint32_t *)&s->ds->data[i] = *(uint32_t*)buf;
>> +               buf += 4;
>> +           }
>> +       }
>> +       break;
> 
> All the cases assume the host is using 32 bpp colours, which is rare I
> think.  Because s->ds->depth is not checked, it will likely segfault.

Right.  I will introduce templates like hw/vga_template.h to fit
s->ds->depth variety.

Thanks!

Shin-ichiro KAWASAKI