| Message ID | 20200901183452.24967-1-deller@gmx.de |
|---|---|
| Headers | show |
| Series | hppa power button support, graphics updates and firmware fixes | expand |
Hi Helge, On 9/1/20 8:34 PM, Helge Deller wrote: > Add emulation for a power button on hppa, > fix quite some bugs in seabios-hppa firmware for artist graphics card > fix boot with old Linux installation CDs. I started to review the version you sent last week and took few notes, are there big changes in this version (is it a REPOST or a v2)? (I think you just appended 2 extra patches posted separately). Thanks, Phil. > > Helge Deller (7): > seabios-hppa: Update SeaBIOS to hppa-qemu-5.2-2 tag > hw/hppa: Make number of TLB and BTLB entries configurable > hw/hppa: Store boot device in fw_cfg section > hw/hppa: Inform SeaBIOS about fw_cfg port address > hw/hppa: Add power button emulation > hw/display/artist: Fix artist screen resolution > target/hppa: Fix boot with old Linux installation CDs > > hw/display/artist.c | 37 +++++++++++++++---------- > hw/hppa/hppa_hardware.h | 3 +- > hw/hppa/machine.c | 56 +++++++++++++++++++++++++++++++++++++- > pc-bios/hppa-firmware.img | Bin 783192 -> 785696 bytes > roms/seabios-hppa | 2 +- > target/hppa/cpu.h | 5 +++- > target/hppa/insns.decode | 10 +++---- > 7 files changed, 89 insertions(+), 24 deletions(-) > > -- > 2.21.3 > >
Hi Philippe, On 01.09.20 21:37, Philippe Mathieu-Daudé wrote: > On 9/1/20 8:34 PM, Helge Deller wrote: >> Add emulation for a power button on hppa, >> fix quite some bugs in seabios-hppa firmware for artist graphics card >> fix boot with old Linux installation CDs. > > I started to review the version you sent last week and took few notes, Thanks! > are there big changes in this version (is it a REPOST or a v2)? > (I think you just appended 2 extra patches posted separately). Yes, it's a v2 version with the two patches appended. Otherwise no real changes to qemu, but some more fixes in the included Seabios. Helge > > Thanks, > > Phil. > >> >> Helge Deller (7): >> seabios-hppa: Update SeaBIOS to hppa-qemu-5.2-2 tag >> hw/hppa: Make number of TLB and BTLB entries configurable >> hw/hppa: Store boot device in fw_cfg section >> hw/hppa: Inform SeaBIOS about fw_cfg port address >> hw/hppa: Add power button emulation >> hw/display/artist: Fix artist screen resolution >> target/hppa: Fix boot with old Linux installation CDs >> >> hw/display/artist.c | 37 +++++++++++++++---------- >> hw/hppa/hppa_hardware.h | 3 +- >> hw/hppa/machine.c | 56 +++++++++++++++++++++++++++++++++++++- >> pc-bios/hppa-firmware.img | Bin 783192 -> 785696 bytes >> roms/seabios-hppa | 2 +- >> target/hppa/cpu.h | 5 +++- >> target/hppa/insns.decode | 10 +++---- >> 7 files changed, 89 insertions(+), 24 deletions(-) >> >> -- >> 2.21.3 >> >> >
On 9/1/20 11:34 AM, Helge Deller wrote: > -/* QEMU fw_cfg interface port */ > -#define QEMU_FW_CFG_IO_BASE (MEMORY_HPA + 0x80) > +#define FW_CFG_IO_BASE 0xfffa0000 Why is this value changing? r~
On 01.09.20 23:39, Richard Henderson wrote: > On 9/1/20 11:34 AM, Helge Deller wrote: >> -/* QEMU fw_cfg interface port */ >> -#define QEMU_FW_CFG_IO_BASE (MEMORY_HPA + 0x80) >> +#define FW_CFG_IO_BASE 0xfffa0000 > > Why is this value changing? Devices on hppa occupy at least 4k starting at the HPA, so MEMORY_HPA+4k is blocked (by Linux) for the memory module. I noticed this when testing the new Linux kernel patch to let the fw_cfg entries show up in Linux under /proc: https://patchwork.kernel.org/patch/11715133/ The Linux kernel driver could not allocate the region for fw_cfg. This new base address seems to not conflict. Helge
On 9/2/20 4:24 AM, Helge Deller wrote: > On 01.09.20 23:39, Richard Henderson wrote: >> On 9/1/20 11:34 AM, Helge Deller wrote: >>> -/* QEMU fw_cfg interface port */ >>> -#define QEMU_FW_CFG_IO_BASE (MEMORY_HPA + 0x80) >>> +#define FW_CFG_IO_BASE 0xfffa0000 >> >> Why is this value changing? > > Devices on hppa occupy at least 4k starting at the HPA, > so MEMORY_HPA+4k is blocked (by Linux) for the memory module. > I noticed this when testing the new Linux kernel patch to > let the fw_cfg entries show up in Linux under /proc: > https://patchwork.kernel.org/patch/11715133/ > The Linux kernel driver could not allocate the region for fw_cfg. > This new base address seems to not conflict. Then that information should be in a patch description, and the change should be a completely separate patch. r~
* Richard Henderson <richard.henderson@linaro.org>: > On 9/1/20 11:34 AM, Helge Deller wrote: > > Artist screen size is limited to 2048 x 2048 pixels and x/y coordination > > addressing needs to be done by OS via an uint32 value which is based on > > a 2048 byte line length, independend of the real screen size. > > > > Since HP-UX seems to ideally need at least 640 pixels in width, this > > patch ensures that the screen size stays between 640x480 and 2048x2048 > > pixels and fixes some pixel glitches were visible before on STI text > > consoles due to the 2048 line length limitation. > > > > Cc: Sven Schnelle <svens@stackframe.org> > > Signed-off-by: Helge Deller <deller@gmx.de> > > --- > > hw/display/artist.c | 37 +++++++++++++++++++++++-------------- > > 1 file changed, 23 insertions(+), 14 deletions(-) > > > > diff --git a/hw/display/artist.c b/hw/display/artist.c > > index 71982559c6..98bee6d61c 100644 > > --- a/hw/display/artist.c > > +++ b/hw/display/artist.c > > @@ -192,6 +192,10 @@ static const char *artist_reg_name(uint64_t addr) > > } > > #undef REG_NAME > > > > +/* artist has a fixed line length of 2048 bytes. */ > > +#define ADDR_TO_Y(addr) (((addr) >> 11) & 0x7ff) > > +#define ADDR_TO_X(addr) ((addr) & 0x7ff) > > extract32() fixed. > > @@ -366,6 +370,8 @@ static void vram_bit_write(ARTISTState *s, int posx, int posy, bool incr_x, > > if (s->cmap_bm_access) { > > offset = s->vram_pos; > > } else { > > + posx = ADDR_TO_X(s->vram_pos >> 2); > > + posy += ADDR_TO_Y(s->vram_pos >> 2); > > Do you in fact want to fold the >> 2 into the ADDR_TO_X/Y, like > #define ADDR_TO_X(POS) extract32(POS, 2, 11) Yes. Basically this block was simply moved up to inside the function. See in removed chunk below, there is the "s->vram_pos >> 2". > > @@ -881,16 +886,12 @@ static void artist_reg_write(void *opaque, hwaddr addr, uint64_t val, > > break; > > > > case VRAM_WRITE_INCR_Y: > > - posx = (s->vram_pos >> 2) & 0x7ff; > > - posy = (s->vram_pos >> 13) & 0x3ff; > ... > > case VRAM_WRITE_INCR_X: > > case VRAM_WRITE_INCR_X2: > > - posx = (s->vram_pos >> 2) & 0x7ff; > > - posy = (s->vram_pos >> 13) & 0x3ff; > ... > > - int posy = (addr >> 11) & 0x3ff; > > Is it a bug that these Y were using 0x3ff and not 0x7ff? > Because it's pretty consistent... I'm not sure if was intentional, a bug or just initial coding by Sven. The old code was limiting to 1024 lines. I assume that there doesn't exist any real hardware with > 1024 lines, so it's more theoretical. I tested the code, and it seems to work with > 1024 at least. So, I think to use 0x7ff is probably more correct - at least if we allow more lines. > You should make that a separate change, for sure. I'd prefer to keep it in one patch. It's simply moving code around and reuse a macro. > > @@ -1374,6 +1377,12 @@ static void artist_realizefn(DeviceState *dev, Error **errp) > > struct vram_buffer *buf; > > hwaddr offset = 0; > > > > + /* Screen on artist can not be greater than 2048x2048 pixels. */ > > + s->width = MAX(s->width, 640); > > + s->width = MIN(s->width, 2048); > > + s->height = MAX(s->height, 480); > > + s->height = MIN(s->height, 2048); > > Was the original values chosen by the user? Yes, can bet set by user via -global -global artist.width=800 -global artist.height=600 > Should we be giving some sort of error for out-of-range values? I had a warning there, but then removed it again. Most users will probably use the default values anyway. If you really want, I will add a warning. I'm not a fan of errors. Errors usually abort, but we can adjust and simply continue here. Updated patch below. Helge ---------------------------------- [PATCH] hw/display/artist: Fix artist screen resolution Artist screen size is limited to 2048 x 2048 pixels and x/y coordination addressing needs to be done by OS via an uint32 value which is based on a 2048 byte line length, independend of the real screen size. Since HP-UX seems to ideally need at least 640 pixels in width, this patch ensures that the screen size stays between 640x480 and 2048x2048 pixels and fixes some pixel glitches were visible before on STI text consoles due to the 2048 line length limitation. Cc: Sven Schnelle <svens@stackframe.org> Signed-off-by: Helge Deller <deller@gmx.de> diff --git a/hw/display/artist.c b/hw/display/artist.c index 71982559c6..71f527c0e1 100644 --- a/hw/display/artist.c +++ b/hw/display/artist.c @@ -192,6 +192,10 @@ static const char *artist_reg_name(uint64_t addr) } #undef REG_NAME +/* artist has a fixed line length of 2048 bytes. */ +#define ADDR_TO_Y(addr) extract32(addr, 11, 11) +#define ADDR_TO_X(addr) extract32(addr, 0, 11) + static int16_t artist_get_x(uint32_t reg) { return reg >> 16; @@ -348,13 +352,13 @@ static void artist_invalidate_cursor(ARTISTState *s) y, s->cursor_height); } -static void vram_bit_write(ARTISTState *s, int posx, int posy, bool incr_x, +static void vram_bit_write(ARTISTState *s, int posy, bool incr_x, int size, uint32_t data) { struct vram_buffer *buf; uint32_t vram_bitmask = s->vram_bitmask; int mask, i, pix_count, pix_length; - unsigned int offset, width; + unsigned int posx, offset, width; uint8_t *data8, *p; pix_count = vram_write_pix_per_transfer(s); @@ -366,6 +370,8 @@ static void vram_bit_write(ARTISTState *s, int posx, int posy, bool incr_x, if (s->cmap_bm_access) { offset = s->vram_pos; } else { + posx = ADDR_TO_X(s->vram_pos >> 2); + posy += ADDR_TO_Y(s->vram_pos >> 2); offset = posy * width + posx; } @@ -858,7 +864,6 @@ static void artist_reg_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) { ARTISTState *s = opaque; - int posx, posy; int width, height; trace_artist_reg_write(size, addr, artist_reg_name(addr & ~3ULL), val); @@ -881,16 +886,12 @@ static void artist_reg_write(void *opaque, hwaddr addr, uint64_t val, break; case VRAM_WRITE_INCR_Y: - posx = (s->vram_pos >> 2) & 0x7ff; - posy = (s->vram_pos >> 13) & 0x3ff; - vram_bit_write(s, posx, posy + s->vram_char_y++, false, size, val); + vram_bit_write(s, s->vram_char_y++, false, size, val); break; case VRAM_WRITE_INCR_X: case VRAM_WRITE_INCR_X2: - posx = (s->vram_pos >> 2) & 0x7ff; - posy = (s->vram_pos >> 13) & 0x3ff; - vram_bit_write(s, posx, posy + s->vram_char_y, true, size, val); + vram_bit_write(s, s->vram_char_y, true, size, val); break; case VRAM_IDX: @@ -1156,8 +1157,7 @@ static void artist_vram_write(void *opaque, hwaddr addr, uint64_t val, { ARTISTState *s = opaque; struct vram_buffer *buf; - int posy = (addr >> 11) & 0x3ff; - int posx = addr & 0x7ff; + unsigned int posy, posx; unsigned int offset; trace_artist_vram_write(size, addr, val); @@ -1170,6 +1170,9 @@ static void artist_vram_write(void *opaque, hwaddr addr, uint64_t val, } buf = vram_write_buffer(s); + posy = ADDR_TO_Y(addr); + posx = ADDR_TO_X(addr); + if (!buf->size) { return; } @@ -1212,7 +1215,7 @@ static uint64_t artist_vram_read(void *opaque, hwaddr addr, unsigned size) ARTISTState *s = opaque; struct vram_buffer *buf; uint64_t val; - int posy, posx; + unsigned int posy, posx; if (s->cmap_bm_access) { buf = &s->vram_buffer[ARTIST_BUFFER_CMAP]; @@ -1229,8 +1232,8 @@ static uint64_t artist_vram_read(void *opaque, hwaddr addr, unsigned size) return 0; } - posy = (addr >> 13) & 0x3ff; - posx = (addr >> 2) & 0x7ff; + posy = ADDR_TO_Y(addr); + posx = ADDR_TO_X(addr); if (posy > buf->height || posx > buf->width) { return 0; @@ -1374,6 +1377,12 @@ static void artist_realizefn(DeviceState *dev, Error **errp) struct vram_buffer *buf; hwaddr offset = 0; + /* Screen on artist can not be greater than 2048x2048 pixels. */ + s->width = MAX(s->width, 640); + s->width = MIN(s->width, 2048); + s->height = MAX(s->height, 480); + s->height = MIN(s->height, 2048); + memory_region_init(&s->mem_as_root, OBJECT(dev), "artist", ~0ull); address_space_init(&s->as, &s->mem_as_root, "artist");
New patch #2: From b0f0d24563df504b8221fbc934d25c5a896e0a49 Mon Sep 17 00:00:00 2001 From: Helge Deller <deller@gmx.de> Subject: [PATCH] hw/display/artist: Refactor x/y coordination extraction Simplify the code by using new introduced ADDR_TO_Y() and ADDR_TO_X() macros. Those macros extract the x/y-coordinate from the given uint32. As further simplification the extraction of the x/y coordinates for VRAM_WRITE_INCR_Y and VRAM_WRITE_INCR_X can be done centrally in vram_bit_write(), so move this code up into the function. ADDR_TO_Y() is still limited to 10 bits which allow to address up to of 1024 lines - this will be increased in a follow-up patch. Signed-off-by: Helge Deller <deller@gmx.de> diff --git a/hw/display/artist.c b/hw/display/artist.c index ff1532fdc1..16d85c65f8 100644 --- a/hw/display/artist.c +++ b/hw/display/artist.c @@ -192,6 +192,10 @@ static const char *artist_reg_name(uint64_t addr) } #undef REG_NAME +/* artist has a fixed line length of 2048 bytes. */ +#define ADDR_TO_Y(addr) extract32(addr, 11, 10) +#define ADDR_TO_X(addr) extract32(addr, 0, 11) + static int16_t artist_get_x(uint32_t reg) { return reg >> 16; @@ -348,13 +352,13 @@ static void artist_invalidate_cursor(ARTISTState *s) y, s->cursor_height); } -static void vram_bit_write(ARTISTState *s, int posx, int posy, bool incr_x, +static void vram_bit_write(ARTISTState *s, int posy, bool incr_x, int size, uint32_t data) { struct vram_buffer *buf; uint32_t vram_bitmask = s->vram_bitmask; int mask, i, pix_count, pix_length; - unsigned int offset, width; + unsigned int posx, offset, width; uint8_t *data8, *p; pix_count = vram_write_pix_per_transfer(s); @@ -366,6 +370,8 @@ static void vram_bit_write(ARTISTState *s, int posx, int posy, bool incr_x, if (s->cmap_bm_access) { offset = s->vram_pos; } else { + posx = ADDR_TO_X(s->vram_pos >> 2); + posy += ADDR_TO_Y(s->vram_pos >> 2); offset = posy * width + posx; } @@ -858,7 +864,6 @@ static void artist_reg_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) { ARTISTState *s = opaque; - int posx, posy; int width, height; trace_artist_reg_write(size, addr, artist_reg_name(addr & ~3ULL), val); @@ -881,16 +886,12 @@ static void artist_reg_write(void *opaque, hwaddr addr, uint64_t val, break; case VRAM_WRITE_INCR_Y: - posx = (s->vram_pos >> 2) & 0x7ff; - posy = (s->vram_pos >> 13) & 0x3ff; - vram_bit_write(s, posx, posy + s->vram_char_y++, false, size, val); + vram_bit_write(s, s->vram_char_y++, false, size, val); break; case VRAM_WRITE_INCR_X: case VRAM_WRITE_INCR_X2: - posx = (s->vram_pos >> 2) & 0x7ff; - posy = (s->vram_pos >> 13) & 0x3ff; - vram_bit_write(s, posx, posy + s->vram_char_y, true, size, val); + vram_bit_write(s, s->vram_char_y, true, size, val); break; case VRAM_IDX: @@ -1156,8 +1157,7 @@ static void artist_vram_write(void *opaque, hwaddr addr, uint64_t val, { ARTISTState *s = opaque; struct vram_buffer *buf; - int posy = (addr >> 11) & 0x3ff; - int posx = addr & 0x7ff; + unsigned int posy, posx; unsigned int offset; trace_artist_vram_write(size, addr, val); @@ -1170,6 +1170,9 @@ static void artist_vram_write(void *opaque, hwaddr addr, uint64_t val, } buf = vram_write_buffer(s); + posy = ADDR_TO_Y(addr); + posx = ADDR_TO_X(addr); + if (!buf->size) { return; } @@ -1212,7 +1215,7 @@ static uint64_t artist_vram_read(void *opaque, hwaddr addr, unsigned size) ARTISTState *s = opaque; struct vram_buffer *buf; uint64_t val; - int posy, posx; + unsigned int posy, posx; if (s->cmap_bm_access) { buf = &s->vram_buffer[ARTIST_BUFFER_CMAP]; @@ -1229,8 +1232,8 @@ static uint64_t artist_vram_read(void *opaque, hwaddr addr, unsigned size) return 0; } - posy = (addr >> 13) & 0x3ff; - posx = (addr >> 2) & 0x7ff; + posy = ADDR_TO_Y(addr); + posx = ADDR_TO_X(addr); if (posy > buf->height || posx > buf->width) { return 0;