On 6/17/20 10:23 PM, Peter Maydell wrote:
> On Mon, 15 Jun 2020 at 22:23, Allan Peramaki <apera...@pp1.inet.fi> wrote:
>>
>> 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 <apera...@pp1.inet.fi>
>
> This patch is quite difficult to read because it mixes some
> whitespace only changes with some actual changes of
> behaviour.
In such cases the author might add a comment in the commit
description "this patch is easier to review with git-diff -w"
or other options.
Allan, can we get the patch with only the changed lines?
See:
---
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
@@ -32,7 +32,7 @@
#define GUSregb(position) (*(gusptr + (position)))
#define GUSregw(position) (*(uint16_t *)(gusptr + (position)))
-#define GUSregd(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
@@ -28,7 +28,7 @@
#define GUSregb(position) (*(gusptr + (position)))
#define GUSregw(position) (*(uint16_t *)(gusptr + (position)))
-#define GUSregd(position) (*(uint16_t *)(gusptr+(position)))
+#define GUSregd(position) (*(uint32_t *)(gusptr + (position)))
#define GUSvoice(position) (*(uint16_t *)(voiceptr + (position)))
---
>
>> -#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)))
>
> 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?
>
>> -#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.
>
> thanks
> -- PMM
>
