| Message ID | 20180919123114.64267-1-marcus@mc.pp.se |
|---|---|
| State | New |
| Headers | show |
| Series | sm501: Adjust endianness of pixel value in rectangle fill | expand |
On Wed, 19 Sep 2018, Marcus Comstedt wrote: > The value from twoD_foreground (which is in host endian format) must > be converted to the endianness of the framebuffer (currently always > little endian) before it can be used to perform the fill operation. > > Signed-off-by: Marcus Comstedt <marcus@mc.pp.se> Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> But also cc-ing Peter Maydell who reviewed endianness fixes before for this device model in case he has something to add. Eventually I plan to rewrite this to make it more optimised and maybe use pixman routines for these but likely I can't get to that until later (maybe in next dev cycle) so until then this fix would help. Thanks for testing on big endian host and fixing this bug. Have you also tried KVM? That would be interesting but I think it may have problems currently and maybe only PR KVM would work as it need to emulate PPC440 on server CPU but I don't know much about KVM. Regards, BALATON Zoltan > --- > > Hi. > > I noticed when running AmigaOS 4.1 as a guest that the rectangle fill > function in SM501 does not work correctly if the host is big endian > (tested on a Talos II). This can be observed easily by starting a > shell in 16-bit mode, where the background turns purple instead of gray. > > After applying this patch rendering was ok on both big and little > endian hosts. > > > // Marcus > > > hw/display/sm501.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hw/display/sm501.c b/hw/display/sm501.c > index 874260a143..4a8686f0f5 100644 > --- a/hw/display/sm501.c > +++ b/hw/display/sm501.c > @@ -39,6 +39,7 @@ > #include "hw/i2c/i2c-ddc.h" > #include "qemu/range.h" > #include "ui/pixel_ops.h" > +#include "qemu/bswap.h" > > /* > * Status: 2010/05/07 > @@ -812,9 +813,11 @@ static void sm501_2d_operation(SM501State *s) > FILL_RECT(1, uint8_t); > break; > case 1: > + color = cpu_to_le16(color); > FILL_RECT(2, uint16_t); > break; > case 2: > + color = cpu_to_le32(color); > FILL_RECT(4, uint32_t); > break; > } >
Hi, BALATON Zoltan <balaton@eik.bme.hu> writes: > Thanks for testing on big endian host and fixing this bug. Have you > also tried KVM? That would be interesting but I think it may have > problems currently and maybe only PR KVM would work as it need to > emulate PPC440 on server CPU but I don't know much about KVM. I did a quick attempt with HV KVM but it did not work. I'll try again with PR KVM once I figure out how to load the kvm_pr kernel module (right now modprobe gives me "Input/output error"...). // Marcus
On 19 September 2018 at 07:46, BALATON Zoltan <balaton@eik.bme.hu> wrote: > On Wed, 19 Sep 2018, Marcus Comstedt wrote: >> >> The value from twoD_foreground (which is in host endian format) must >> be converted to the endianness of the framebuffer (currently always >> little endian) before it can be used to perform the fill operation. >> >> Signed-off-by: Marcus Comstedt <marcus@mc.pp.se> > > > Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> > > But also cc-ing Peter Maydell who reviewed endianness fixes before for this > device model in case he has something to add. There are probably other ways to fix that, but this looks OK to me. We have a pixel value in a host-endianness register field, and we're trying to write it into a little-endian framebuffer with a plain host store of a 32/16/8 bit type. So cpu-to-le is the right thing. (The other approach would be to use the stl_le_p()/stw_le_p()/stb_p() functions to do the store directly in the correct endianness. But I don't think that ends up with significantly cleaner code.) Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On Wed, Sep 19, 2018 at 06:23:14PM +0200, Marcus Comstedt wrote: > > Hi, > > BALATON Zoltan <balaton@eik.bme.hu> writes: > > > Thanks for testing on big endian host and fixing this bug. Have you > > also tried KVM? That would be interesting but I think it may have > > problems currently and maybe only PR KVM would work as it need to > > emulate PPC440 on server CPU but I don't know much about KVM. > > I did a quick attempt with HV KVM but it did not work. I'll try again > with PR KVM once I figure out how to load the kvm_pr kernel module > (right now modprobe gives me "Input/output error"...). Yeah, that definitely can't work with HV KVM. HV KVM can only emulate a PAPR (machine type "pseries") guest. And the guest cpu has to be (pretty close to) the same as the host cpu.
On Wed, Sep 19, 2018 at 09:29:31AM -0700, Peter Maydell wrote: > On 19 September 2018 at 07:46, BALATON Zoltan <balaton@eik.bme.hu> wrote: > > On Wed, 19 Sep 2018, Marcus Comstedt wrote: > >> > >> The value from twoD_foreground (which is in host endian format) must > >> be converted to the endianness of the framebuffer (currently always > >> little endian) before it can be used to perform the fill operation. > >> > >> Signed-off-by: Marcus Comstedt <marcus@mc.pp.se> > > > > > > Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu> > > > > But also cc-ing Peter Maydell who reviewed endianness fixes before for this > > device model in case he has something to add. > > There are probably other ways to fix that, but this looks OK to me. > We have a pixel value in a host-endianness register field, and we're > trying to write it into a little-endian framebuffer with a plain > host store of a 32/16/8 bit type. So cpu-to-le is the right thing. > (The other approach would be to use the stl_le_p()/stw_le_p()/stb_p() > functions to do the store directly in the correct endianness. But > I don't think that ends up with significantly cleaner code.) > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> Applied to ppc-for-3.1, thanks.
On Wed, 19 Sep 2018, Marcus Comstedt wrote: > BALATON Zoltan <balaton@eik.bme.hu> writes: >> Thanks for testing on big endian host and fixing this bug. Have you >> also tried KVM? That would be interesting but I think it may have >> problems currently and maybe only PR KVM would work as it need to >> emulate PPC440 on server CPU but I don't know much about KVM. > > I did a quick attempt with HV KVM but it did not work. I'll try again > with PR KVM once I figure out how to load the kvm_pr kernel module > (right now modprobe gives me "Input/output error"...). Not sure if you're aware of this: http://tenfourfox.blogspot.com/2018/06/another-weekend-on-new-computer-or.html but it may have some relevant info on KVM on Talos II. Regards, BALATON Zoltan
diff --git a/hw/display/sm501.c b/hw/display/sm501.c index 874260a143..4a8686f0f5 100644 --- a/hw/display/sm501.c +++ b/hw/display/sm501.c @@ -39,6 +39,7 @@ #include "hw/i2c/i2c-ddc.h" #include "qemu/range.h" #include "ui/pixel_ops.h" +#include "qemu/bswap.h" /* * Status: 2010/05/07 @@ -812,9 +813,11 @@ static void sm501_2d_operation(SM501State *s) FILL_RECT(1, uint8_t); break; case 1: + color = cpu_to_le16(color); FILL_RECT(2, uint16_t); break; case 2: + color = cpu_to_le32(color); FILL_RECT(4, uint32_t); break; }
The value from twoD_foreground (which is in host endian format) must be converted to the endianness of the framebuffer (currently always little endian) before it can be used to perform the fill operation. Signed-off-by: Marcus Comstedt <marcus@mc.pp.se> --- Hi. I noticed when running AmigaOS 4.1 as a guest that the rectangle fill function in SM501 does not work correctly if the host is big endian (tested on a Talos II). This can be observed easily by starting a shell in 16-bit mode, where the background turns purple instead of gray. After applying this patch rendering was ok on both big and little endian hosts. // Marcus hw/display/sm501.c | 3 +++ 1 file changed, 3 insertions(+)