diff mbox series

[v3] hw/display/tcx: Allow 64-bit accesses to framebuffer stippler and blitter

Message ID 20201024205100.3623006-1-f4bug@amsat.org
State New
Headers show
Series [v3] hw/display/tcx: Allow 64-bit accesses to framebuffer stippler and blitter | expand

Commit Message

Philippe Mathieu-Daudé Oct. 24, 2020, 8:51 p.m. UTC 
The S24/TCX datasheet is listed as "Unable to locate" on [1].

However the NetBSD revision 1.32 of the driver introduced
64-bit accesses to the stippler and blitter [2]. It is safe
to assume these memory regions are 64-bit accessible.
QEMU implementation is 32-bit, so fill the 'impl' fields.

Michael Lorenz (author of the NetBSD code [2]) provided us with more
information in [3]:

> IIRC the real hardware *requires* 64bit accesses for stipple and
> blitter operations to work. For stipples you write a 64bit word into
> STIP space, the address defines where in the framebuffer you want to
> draw, the data contain a 32bit bitmask, foreground colour and a ROP.
> BLIT space works similarly, the 64bit word contains an offset were to
> read pixels from, and how many you want to copy.
>
> One more thing since there seems to be some confusion - 64bit accesses
> on the framebuffer are fine as well. TCX/S24 is *not* an SBus device,
> even though its node says it is.
> S24 is a card that plugs into a special slot on the SS5 mainboard,
> which is shared with an SBus slot and looks a lot like a horizontal
> UPA slot. Both S24 and TCX are accessed through the Micro/TurboSPARC's
> AFX bus which is 64bit wide and intended for graphics.
> Early FFB docs even mentioned connecting to both AFX and UPA,
> no idea if that was ever realized in hardware though.

[1] http://web.archive.org/web/20111209011516/http://wikis.sun.com/display/FOSSdocs/Home
[2] http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/sbus/tcx.c.diff?r1=1.31&r2=1.32
[3] https://www.mail-archive.com/qemu-devel@nongnu.org/msg734928.html

Reported-by: Andreas Gustafsson <gson@gson.org>
Buglink: https://bugs.launchpad.net/bugs/1892540
Fixes: 55d7bfe2293 ("tcx: Implement hardware acceleration")
Tested-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Tested-by: Andreas Gustafsson <gson@gson.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Since v2:
- added Michael's memories
- added R-b/T-b tags

Since v1:
- added missing uncommitted staged changes... (tcx_blit_ops)
---
 hw/display/tcx.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Comments

Mark Cave-Ayland Oct. 25, 2020, 10:55 a.m. UTC | #1 
On 24/10/2020 21:51, Philippe Mathieu-Daudé wrote:

> The S24/TCX datasheet is listed as "Unable to locate" on [1].
> 
> However the NetBSD revision 1.32 of the driver introduced
> 64-bit accesses to the stippler and blitter [2]. It is safe
> to assume these memory regions are 64-bit accessible.
> QEMU implementation is 32-bit, so fill the 'impl' fields.
> 
> Michael Lorenz (author of the NetBSD code [2]) provided us with more
> information in [3]:
> 
>> IIRC the real hardware *requires* 64bit accesses for stipple and
>> blitter operations to work. For stipples you write a 64bit word into
>> STIP space, the address defines where in the framebuffer you want to
>> draw, the data contain a 32bit bitmask, foreground colour and a ROP.
>> BLIT space works similarly, the 64bit word contains an offset were to
>> read pixels from, and how many you want to copy.
>>
>> One more thing since there seems to be some confusion - 64bit accesses
>> on the framebuffer are fine as well. TCX/S24 is *not* an SBus device,
>> even though its node says it is.
>> S24 is a card that plugs into a special slot on the SS5 mainboard,
>> which is shared with an SBus slot and looks a lot like a horizontal
>> UPA slot. Both S24 and TCX are accessed through the Micro/TurboSPARC's
>> AFX bus which is 64bit wide and intended for graphics.
>> Early FFB docs even mentioned connecting to both AFX and UPA,
>> no idea if that was ever realized in hardware though.
> 
> [1] http://web.archive.org/web/20111209011516/http://wikis.sun.com/display/FOSSdocs/Home
> [2] http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/sbus/tcx.c.diff?r1=1.31&r2=1.32
> [3] https://www.mail-archive.com/qemu-devel@nongnu.org/msg734928.html
> 
> Reported-by: Andreas Gustafsson <gson@gson.org>
> Buglink: https://bugs.launchpad.net/bugs/1892540
> Fixes: 55d7bfe2293 ("tcx: Implement hardware acceleration")
> Tested-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Tested-by: Andreas Gustafsson <gson@gson.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Since v2:
> - added Michael's memories
> - added R-b/T-b tags
> 
> Since v1:
> - added missing uncommitted staged changes... (tcx_blit_ops)
> ---
>   hw/display/tcx.c | 18 +++++++++++++++---
>   1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index c9d5e45cd1f..878ecc8c506 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -549,20 +549,28 @@ static const MemoryRegionOps tcx_stip_ops = {
>       .read = tcx_stip_readl,
>       .write = tcx_stip_writel,
>       .endianness = DEVICE_NATIVE_ENDIAN,
> -    .valid = {
> +    .impl = {
>           .min_access_size = 4,
>           .max_access_size = 4,
>       },
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 8,
> +    },
>   };
>   
>   static const MemoryRegionOps tcx_rstip_ops = {
>       .read = tcx_stip_readl,
>       .write = tcx_rstip_writel,
>       .endianness = DEVICE_NATIVE_ENDIAN,
> -    .valid = {
> +    .impl = {
>           .min_access_size = 4,
>           .max_access_size = 4,
>       },
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 8,
> +    },
>   };
>   
>   static uint64_t tcx_blit_readl(void *opaque, hwaddr addr,
> @@ -651,10 +659,14 @@ static const MemoryRegionOps tcx_rblit_ops = {
>       .read = tcx_blit_readl,
>       .write = tcx_rblit_writel,
>       .endianness = DEVICE_NATIVE_ENDIAN,
> -    .valid = {
> +    .impl = {
>           .min_access_size = 4,
>           .max_access_size = 4,
>       },
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 8,
> +    },
>   };
>   
>   static void tcx_invalidate_cursor_position(TCXState *s)

I'd already queued v2 of this patch (see my earlier email) with the intent to send a 
PR today, however I'll replace it with this v3 instead.


ATB,

Mark.
Mark Cave-Ayland Oct. 25, 2020, 11:42 a.m. UTC | #2 
On 10/25/20 11:55 AM, Mark Cave-Ayland wrote:
> On 24/10/2020 21:51, Philippe Mathieu-Daudé wrote:
> 
>> The S24/TCX datasheet is listed as "Unable to locate" on [1].
>>
>> However the NetBSD revision 1.32 of the driver introduced
>> 64-bit accesses to the stippler and blitter [2]. It is safe
>> to assume these memory regions are 64-bit accessible.
>> QEMU implementation is 32-bit, so fill the 'impl' fields.
>>
>> Michael Lorenz (author of the NetBSD code [2]) provided us with more
>> information in [3]:
>>
>>> IIRC the real hardware *requires* 64bit accesses for stipple and
>>> blitter operations to work. For stipples you write a 64bit word into
>>> STIP space, the address defines where in the framebuffer you want to
>>> draw, the data contain a 32bit bitmask, foreground colour and a ROP.
>>> BLIT space works similarly, the 64bit word contains an offset were to
>>> read pixels from, and how many you want to copy.
>>>
>>> One more thing since there seems to be some confusion - 64bit accesses
>>> on the framebuffer are fine as well. TCX/S24 is *not* an SBus device,
>>> even though its node says it is.
>>> S24 is a card that plugs into a special slot on the SS5 mainboard,
>>> which is shared with an SBus slot and looks a lot like a horizontal
>>> UPA slot. Both S24 and TCX are accessed through the Micro/TurboSPARC's
>>> AFX bus which is 64bit wide and intended for graphics.
>>> Early FFB docs even mentioned connecting to both AFX and UPA,
>>> no idea if that was ever realized in hardware though.
>>
>> [1] 
>> http://web.archive.org/web/20111209011516/http://wikis.sun.com/display/FOSSdocs/Home 
>>
>> [2] 
>> http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/sbus/tcx.c.diff?r1=1.31&r2=1.32 
>>
>> [3] https://www.mail-archive.com/qemu-devel@nongnu.org/msg734928.html
>>
>> Reported-by: Andreas Gustafsson <gson@gson.org>
>> Buglink: https://bugs.launchpad.net/bugs/1892540
>> Fixes: 55d7bfe2293 ("tcx: Implement hardware acceleration")
>> Tested-by: Michael S. Tsirkin <mst@redhat.com>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Tested-by: Andreas Gustafsson <gson@gson.org>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> Since v2:
>> - added Michael's memories
>> - added R-b/T-b tags
>>
>> Since v1:
>> - added missing uncommitted staged changes... (tcx_blit_ops)
>> ---
>>   hw/display/tcx.c | 18 +++++++++++++++---
>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
>> index c9d5e45cd1f..878ecc8c506 100644
>> --- a/hw/display/tcx.c
>> +++ b/hw/display/tcx.c
>> @@ -549,20 +549,28 @@ static const MemoryRegionOps tcx_stip_ops = {
>>       .read = tcx_stip_readl,
>>       .write = tcx_stip_writel,
>>       .endianness = DEVICE_NATIVE_ENDIAN,
>> -    .valid = {
>> +    .impl = {
>>           .min_access_size = 4,
>>           .max_access_size = 4,
>>       },
>> +    .valid = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 8,
>> +    },
>>   };
>>   static const MemoryRegionOps tcx_rstip_ops = {
>>       .read = tcx_stip_readl,
>>       .write = tcx_rstip_writel,
>>       .endianness = DEVICE_NATIVE_ENDIAN,
>> -    .valid = {
>> +    .impl = {
>>           .min_access_size = 4,
>>           .max_access_size = 4,
>>       },
>> +    .valid = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 8,
>> +    },
>>   };
>>   static uint64_t tcx_blit_readl(void *opaque, hwaddr addr,
>> @@ -651,10 +659,14 @@ static const MemoryRegionOps tcx_rblit_ops = {
>>       .read = tcx_blit_readl,
>>       .write = tcx_rblit_writel,
>>       .endianness = DEVICE_NATIVE_ENDIAN,
>> -    .valid = {
>> +    .impl = {
>>           .min_access_size = 4,
>>           .max_access_size = 4,
>>       },
>> +    .valid = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 8,
>> +    },
>>   };
>>   static void tcx_invalidate_cursor_position(TCXState *s)
> 
> I'd already queued v2 of this patch (see my earlier email) with the 
> intent to send a PR today, however I'll replace it with this v3 instead.

Thanks! Since there is no code change with v2, I assumed it wouldn't be
a problem to replace it, without having to re-run your tests.

> 
> 
> ATB,
> 
> Mark.
>
diff mbox series

Patch

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index c9d5e45cd1f..878ecc8c506 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -549,20 +549,28 @@  static const MemoryRegionOps tcx_stip_ops = {
     .read = tcx_stip_readl,
     .write = tcx_stip_writel,
     .endianness = DEVICE_NATIVE_ENDIAN,
-    .valid = {
+    .impl = {
         .min_access_size = 4,
         .max_access_size = 4,
     },
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 8,
+    },
 };
 
 static const MemoryRegionOps tcx_rstip_ops = {
     .read = tcx_stip_readl,
     .write = tcx_rstip_writel,
     .endianness = DEVICE_NATIVE_ENDIAN,
-    .valid = {
+    .impl = {
         .min_access_size = 4,
         .max_access_size = 4,
     },
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 8,
+    },
 };
 
 static uint64_t tcx_blit_readl(void *opaque, hwaddr addr,
@@ -651,10 +659,14 @@  static const MemoryRegionOps tcx_rblit_ops = {
     .read = tcx_blit_readl,
     .write = tcx_rblit_writel,
     .endianness = DEVICE_NATIVE_ENDIAN,
-    .valid = {
+    .impl = {
         .min_access_size = 4,
         .max_access_size = 4,
     },
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 8,
+    },
 };
 
 static void tcx_invalidate_cursor_position(TCXState *s)