[Bug 1873769] Re: SB16 audio playback freezes emulation in Windows 95 guest

[Allan Peramaki]

This is with GTK UI? Do you still have the same problem if you use Spice
and remote-viewer instead?

(GTK UI and Sound Blaster 16 emulation don't play well together. GTK UI
does screen updates only when the main event loop becomes idle, but it
never becomes idle when SB16 audio is playing due to the way
hw/dma/i8257 works. The combination of GTK UI screen updates + SB16 DMA
transfer additionally causes i8257_dma_run() getting called at a very
rapid rate.)

-- 
You received this bug notification because you are a member of qemu-
devel-ml, which is subscribed to QEMU.
https://bugs.launchpad.net/bugs/1873769

Title:
  SB16 audio playback freezes emulation in Windows 95 guest

Status in QEMU:
  New

Bug description:
  - QEMU 4.2.93 (v5.0.0-rc3) built from latest git master
  20038cd7a8412feeb49c01f6ede89e36c8995472 using MSYS2 on Windows 10 and
  launched on same Windows 10

  - Launched using "qemu-system-i386.exe -drive format=raw,file=hdd-
  2gb.img -soundhw pcspk,sb16 -m 16 -cpu pentium -vga std -cdrom
  Windows_95.iso -boot c"

  - I have attached video screen capture of the issue

  ---

  I decided to make my first ever QEMU build after encountering the
  dsound issues using the latest 4.2.0 binary from
  https://qemu.weilnetz.de/w64/. In my 5.0.0-rc3 build the sound
  playback is working correctly, however the whole Windows 95 UI freezes
  while sound is playing.

To manage notifications about this bug go to:
https://bugs.launchpad.net/qemu/+bug/1873769/+subscriptions

Re: [PATCH] hw/audio/gus: Fix registers 32-bit access

[Allan Peramaki]

On 18/06/2020 01:25, Allan Peramaki wrote:

On 17/06/2020 23:23, Peter Maydell wrote:

Are these accesses all guaranteed to be correctly aligned
to be 16 or 32 bit loads/stores ? Otherwise it would be
better to use the ldl_p/stl_p/ldw_p/stw_p/etc accessors,
which correctly handle possibly misaligned pointers.


Yes (assuming compiler aligns struct fields properly.) It is not obvious 
though (and easy to break), so I suppose refactoring much of the GUS 
code would be nice. For example, the few places where GUSregd(position) 
is used, position is 2 (mod 4). On the other hand, gusptr is also 2 (mod 
4) bytes (making gusptr+position = 0 (mod 4)), because we have the struct


It is aligned, but my reasoning had a mistake. Both gusptr and position 
(GUSDRAMPOS24bit and voicewavetableirq) are 0 (mod 4). (mixbuf is a 
pointer.) Sorry.




typedef struct GUSState {
     ISADevice dev;
     GUSEmuState emu;
     QEMUSoundCard card;
     uint32_t freq;
     uint32_t port;
     int pos, left, shift, irqs;
     int16_t *mixbuf;
     uint8_t himem[1024 * 1024 + 32 + 4096];
     int samples;
     SWVoiceOut *voice;
     int64_t last_ticks;
     qemu_irq pic;
     IsaDma *isa_dma;
     PortioList portio_list1;
     PortioList portio_list2;
} GUSState;




Best regards,
Allan

Re: [PATCH] hw/audio/gus: Fix registers 32-bit access

[Allan Peramaki]

On 17/06/2020 23:23, Peter Maydell wrote:


This patch is quite difficult to read because it mixes some
whitespace only changes with some actual changes of
behaviour.


Sorry about that. I had to put some whitespace in the two lines I 
modified because of checkpatch.pl, but then the nearby lines would have 
had inconsistent style if left unmodified. On the other hand, QEMU wiki 
said it is ok to fix style issues in the immediate area.



So, I think the actual bugfix change here is just the changing
of uint16_t to uint32_t in the GUSregd definition...


-#define GUSregb(position)  (*(gusptr+(position)))
-#define GUSregw(position)  (*(uint16_t *) (gusptr+(position)))
-#define GUSregd(position)  (*(uint16_t *)(gusptr+(position)))
+#define GUSregb(position)  (*(gusptr + (position)))
+#define GUSregw(position)  (*(uint16_t *)(gusptr + (position)))
+#define GUSregd(position)  (*(uint32_t *)(gusptr + (position)))


...and similarly here, and all the other changes are purely
cleaning up the spaces. Is that right?


That's right. The only real change is uint16_t -> uint32_t in GUSregd. 
This reverses what seems to be an accident in commit 135f5ae1974c, where 
GUSdword (defined as "unsigned int" or "uint32_t" depending on compiler) 
was replaced with uint16_t. That change broke applications that 
read/write DRAM above 64k via I/O ports (in lieu of using DMA.)





-#define GUSvoice(position) (*(uint16_t *)(voiceptr+(position)))
+#define GUSvoice(position) (*(uint16_t *)(voiceptr + (position)))


Are these accesses all guaranteed to be correctly aligned
to be 16 or 32 bit loads/stores ? Otherwise it would be
better to use the ldl_p/stl_p/ldw_p/stw_p/etc accessors,
which correctly handle possibly misaligned pointers.


Yes (assuming compiler aligns struct fields properly.) It is not obvious 
though (and easy to break), so I suppose refactoring much of the GUS 
code would be nice. For example, the few places where GUSregd(position) 
is used, position is 2 (mod 4). On the other hand, gusptr is also 2 (mod 
4) bytes (making gusptr+position = 0 (mod 4)), because we have the struct


typedef struct GUSState {
ISADevice dev;
GUSEmuState emu;
QEMUSoundCard card;
uint32_t freq;
uint32_t port;
int pos, left, shift, irqs;
int16_t *mixbuf;
uint8_t himem[1024 * 1024 + 32 + 4096];
int samples;
SWVoiceOut *voice;
int64_t last_ticks;
qemu_irq pic;
IsaDma *isa_dma;
PortioList portio_list1;
PortioList portio_list2;
} GUSState;

where himem is 2 (mod 4). When GUSregd() is used, gusptr is (himem + 
1024 * 1024 + 32), so gusptr is also 2 (mod 4). Similar applies to 
GUSvoice(position).


Best regards,
Allan

[PATCH] hw/audio/gus: Fix registers 32-bit access

[Allan Peramaki]

Fix audio on software that accesses DRAM above 64k via register peek/poke
and some cases when more than 16 voices are used.

Fixes: 135f5ae1974c ("audio: GUSsample is int16_t")
Signed-off-by: Allan Peramaki 
---
 hw/audio/gusemu_hal.c   | 6 +++---
 hw/audio/gusemu_mixer.c | 8 
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/audio/gusemu_hal.c b/hw/audio/gusemu_hal.c
index ae40ca341c..e35e941926 100644
--- a/hw/audio/gusemu_hal.c
+++ b/hw/audio/gusemu_hal.c
@@ -30,9 +30,9 @@
 #include "gustate.h"
 #include "gusemu.h"
 
-#define GUSregb(position) (*(gusptr+(position)))
-#define GUSregw(position) (*(uint16_t *) (gusptr+(position)))
-#define GUSregd(position) (*(uint16_t *)(gusptr+(position)))
+#define GUSregb(position) (*(gusptr + (position)))
+#define GUSregw(position) (*(uint16_t *)(gusptr + (position)))
+#define GUSregd(position) (*(uint32_t *)(gusptr + (position)))
 
 /* size given in bytes */
 unsigned int gus_read(GUSEmuState * state, int port, int size)
diff --git a/hw/audio/gusemu_mixer.c b/hw/audio/gusemu_mixer.c
index 00b9861b92..3b39254518 100644
--- a/hw/audio/gusemu_mixer.c
+++ b/hw/audio/gusemu_mixer.c
@@ -26,11 +26,11 @@
 #include "gusemu.h"
 #include "gustate.h"
 
-#define GUSregb(position)  (*(gusptr+(position)))
-#define GUSregw(position)  (*(uint16_t *) (gusptr+(position)))
-#define GUSregd(position)  (*(uint16_t *)(gusptr+(position)))
+#define GUSregb(position)  (*(gusptr + (position)))
+#define GUSregw(position)  (*(uint16_t *)(gusptr + (position)))
+#define GUSregd(position)  (*(uint32_t *)(gusptr + (position)))
 
-#define GUSvoice(position) (*(uint16_t *)(voiceptr+(position)))
+#define GUSvoice(position) (*(uint16_t *)(voiceptr + (position)))
 
 /* samples are always 16bit stereo (4 bytes each, first right then left 
interleaved) */
 void gus_mixvoices(GUSEmuState * state, unsigned int playback_freq, unsigned 
int numsamples,
-- 
2.20.1