[PATCH 0/4] macfb: A/UX fixes for colour LUT

[PATCH 0/4] macfb: A/UX fixes for colour LUT

[Mark Cave-Ayland]

This fixes an issue discovered in A/UX whereby attempts to read the colour
LUT from the macfb device returned zero (black) for all colours. The problem
was that the DAFB_RESET register is actually used to set the current colour
palette array index: fixing that and allowing larger access sizes allows the
test case to work.

This series implements these changes, along with renaming the DAFB_RESET
register to DAFB_LUT_INDEX which better describes its purpose.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk


Mark Cave-Ayland (4):
  macfb: don't clear interrupts when writing to DAFB_RESET
  macfb: rename DAFB_RESET to DAFB_LUT_INDEX
  macfb: allow larger write accesses to the DAFB_LUT register
  macfb: allow reads from the DAFB_LUT register

 hw/display/macfb.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

-- 
2.39.2

[PATCH 1/4] macfb: don't clear interrupts when writing to DAFB_RESET

[Mark Cave-Ayland]

Traces from A/UX suggest that this register is only used to reset the framebuffer
LUT (colour lookup table) and not any other device state.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 2f8e016566..28db2e9f24 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -585,8 +585,6 @@ static void macfb_ctrl_write(void *opaque,
         break;
     case DAFB_RESET:
         s->palette_current = 0;
-        s->regs[DAFB_INTR_STAT >> 2] &= ~DAFB_INTR_VBL;
-        macfb_update_irq(s);
         break;
     case DAFB_LUT:
         s->color_palette[s->palette_current] = val;
-- 
2.39.2

Re: [PATCH 1/4] macfb: don't clear interrupts when writing to DAFB_RESET

[Laurent Vivier]

Le 26/10/2023 à 10:56, Mark Cave-Ayland a écrit :
> Traces from A/UX suggest that this register is only used to reset the framebuffer
> LUT (colour lookup table) and not any other device state.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/display/macfb.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 2f8e016566..28db2e9f24 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -585,8 +585,6 @@ static void macfb_ctrl_write(void *opaque,
>           break;
>       case DAFB_RESET:
>           s->palette_current = 0;
> -        s->regs[DAFB_INTR_STAT >> 2] &= ~DAFB_INTR_VBL;
> -        macfb_update_irq(s);
>           break;
>       case DAFB_LUT:
>           s->color_palette[s->palette_current] = val;

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[PATCH 2/4] macfb: rename DAFB_RESET to DAFB_LUT_INDEX

[Mark Cave-Ayland]

When A/UX uses the MacOS Device Manager Status (GetEntries) call to read the
contents of the CLUT, it is easy to see that the requested index is written to
the DAFB_RESET register. Update the palette_current index with the requested
value, and rename it to DAFB_LUT_INDEX to reflect its true purpose.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 28db2e9f24..eb4ce6b824 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -36,7 +36,7 @@
 #define DAFB_INTR_MASK      0x104
 #define DAFB_INTR_STAT      0x108
 #define DAFB_INTR_CLEAR     0x10c
-#define DAFB_RESET          0x200
+#define DAFB_LUT_INDEX      0x200
 #define DAFB_LUT            0x213
 
 #define DAFB_INTR_VBL   0x4
@@ -583,8 +583,8 @@ static void macfb_ctrl_write(void *opaque,
         s->regs[DAFB_INTR_STAT >> 2] &= ~DAFB_INTR_VBL;
         macfb_update_irq(s);
         break;
-    case DAFB_RESET:
-        s->palette_current = 0;
+    case DAFB_LUT_INDEX:
+        s->palette_current = (val & 0xff) * 3;
         break;
     case DAFB_LUT:
         s->color_palette[s->palette_current] = val;
-- 
2.39.2

Re: [PATCH 2/4] macfb: rename DAFB_RESET to DAFB_LUT_INDEX

[Laurent Vivier]

Le 26/10/2023 à 10:56, Mark Cave-Ayland a écrit :
> When A/UX uses the MacOS Device Manager Status (GetEntries) call to read the
> contents of the CLUT, it is easy to see that the requested index is written to
> the DAFB_RESET register. Update the palette_current index with the requested
> value, and rename it to DAFB_LUT_INDEX to reflect its true purpose.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/display/macfb.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 28db2e9f24..eb4ce6b824 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -36,7 +36,7 @@
>   #define DAFB_INTR_MASK      0x104
>   #define DAFB_INTR_STAT      0x108
>   #define DAFB_INTR_CLEAR     0x10c
> -#define DAFB_RESET          0x200
> +#define DAFB_LUT_INDEX      0x200
>   #define DAFB_LUT            0x213
>   
>   #define DAFB_INTR_VBL   0x4
> @@ -583,8 +583,8 @@ static void macfb_ctrl_write(void *opaque,
>           s->regs[DAFB_INTR_STAT >> 2] &= ~DAFB_INTR_VBL;
>           macfb_update_irq(s);
>           break;
> -    case DAFB_RESET:
> -        s->palette_current = 0;
> +    case DAFB_LUT_INDEX:
> +        s->palette_current = (val & 0xff) * 3;
>           break;
>       case DAFB_LUT:
>           s->color_palette[s->palette_current] = val;

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[PATCH 3/4] macfb: allow larger write accesses to the DAFB_LUT register

[Mark Cave-Ayland]

The original tests with MacOS showed that only the bottom 8 bits of the DAFB_LUT
register were used when writing to the LUT, however A/UX performs some of its
writes using 4 byte accesses. Expand the address range for the DAFB_LUT register
so that different size accesses write the correct value to the color_palette
array.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index eb4ce6b824..4a1c75d572 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -37,7 +37,7 @@
 #define DAFB_INTR_STAT      0x108
 #define DAFB_INTR_CLEAR     0x10c
 #define DAFB_LUT_INDEX      0x200
-#define DAFB_LUT            0x213
+#define DAFB_LUT            0x210
 
 #define DAFB_INTR_VBL   0x4
 
@@ -586,8 +586,8 @@ static void macfb_ctrl_write(void *opaque,
     case DAFB_LUT_INDEX:
         s->palette_current = (val & 0xff) * 3;
         break;
-    case DAFB_LUT:
-        s->color_palette[s->palette_current] = val;
+    case DAFB_LUT ... DAFB_LUT + 3:
+        s->color_palette[s->palette_current] = val & 0xff;
         s->palette_current = (s->palette_current + 1) %
                              ARRAY_SIZE(s->color_palette);
         if (s->palette_current % 3) {
-- 
2.39.2

Re: [PATCH 3/4] macfb: allow larger write accesses to the DAFB_LUT register

[Laurent Vivier]

Le 26/10/2023 à 10:56, Mark Cave-Ayland a écrit :
> The original tests with MacOS showed that only the bottom 8 bits of the DAFB_LUT
> register were used when writing to the LUT, however A/UX performs some of its
> writes using 4 byte accesses. Expand the address range for the DAFB_LUT register
> so that different size accesses write the correct value to the color_palette
> array.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/display/macfb.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index eb4ce6b824..4a1c75d572 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -37,7 +37,7 @@
>   #define DAFB_INTR_STAT      0x108
>   #define DAFB_INTR_CLEAR     0x10c
>   #define DAFB_LUT_INDEX      0x200
> -#define DAFB_LUT            0x213
> +#define DAFB_LUT            0x210
>   
>   #define DAFB_INTR_VBL   0x4
>   
> @@ -586,8 +586,8 @@ static void macfb_ctrl_write(void *opaque,
>       case DAFB_LUT_INDEX:
>           s->palette_current = (val & 0xff) * 3;
>           break;
> -    case DAFB_LUT:
> -        s->color_palette[s->palette_current] = val;
> +    case DAFB_LUT ... DAFB_LUT + 3:
> +        s->color_palette[s->palette_current] = val & 0xff;
>           s->palette_current = (s->palette_current + 1) %
>                                ARRAY_SIZE(s->color_palette);
>           if (s->palette_current % 3) {

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[PATCH 4/4] macfb: allow reads from the DAFB_LUT register

[Mark Cave-Ayland]

This enables A/UX to correctly retrieve the LUT entries when used with
applications that use the MacOS Device Manager Status (GetEntries) call.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/display/macfb.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/display/macfb.c b/hw/display/macfb.c
index 4a1c75d572..d61541ccb5 100644
--- a/hw/display/macfb.c
+++ b/hw/display/macfb.c
@@ -537,6 +537,11 @@ static uint64_t macfb_ctrl_read(void *opaque,
     case DAFB_MODE_SENSE:
         val = macfb_sense_read(s);
         break;
+    case DAFB_LUT ... DAFB_LUT + 3:
+        val = s->color_palette[s->palette_current];
+        s->palette_current = (s->palette_current + 1) %
+                             ARRAY_SIZE(s->color_palette);
+        break;
     default:
         if (addr < MACFB_CTRL_TOPADDR) {
             val = s->regs[addr >> 2];
-- 
2.39.2

Re: [PATCH 4/4] macfb: allow reads from the DAFB_LUT register

[Laurent Vivier]

Le 26/10/2023 à 10:56, Mark Cave-Ayland a écrit :
> This enables A/UX to correctly retrieve the LUT entries when used with
> applications that use the MacOS Device Manager Status (GetEntries) call.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/display/macfb.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/hw/display/macfb.c b/hw/display/macfb.c
> index 4a1c75d572..d61541ccb5 100644
> --- a/hw/display/macfb.c
> +++ b/hw/display/macfb.c
> @@ -537,6 +537,11 @@ static uint64_t macfb_ctrl_read(void *opaque,
>       case DAFB_MODE_SENSE:
>           val = macfb_sense_read(s);
>           break;
> +    case DAFB_LUT ... DAFB_LUT + 3:
> +        val = s->color_palette[s->palette_current];
> +        s->palette_current = (s->palette_current + 1) %
> +                             ARRAY_SIZE(s->color_palette);
> +        break;
>       default:
>           if (addr < MACFB_CTRL_TOPADDR) {
>               val = s->regs[addr >> 2];

Reviewed-by: Laurent Vivier <laurent@vivier.eu>