Re: [PATCH 6/7] hw/display/artist: Fix artist screen resolution

[Helge Deller]

* Richard Henderson <richard.henderson@linaro.org>:
> On 9/1/20 11:34 AM, Helge Deller wrote:
> > Artist screen size is limited to 2048 x 2048 pixels and x/y coordination
> > addressing needs to be done by OS via an uint32 value which is based on
> > a 2048 byte line length, independend of the real screen size.
> >
> > Since HP-UX seems to ideally need at least 640 pixels in width, this
> > patch ensures that the screen size stays between 640x480 and 2048x2048
> > pixels and fixes some pixel glitches were visible before on STI text
> > consoles due to the 2048 line length limitation.
> >
> > Cc: Sven Schnelle <svens@stackframe.org>
> > Signed-off-by: Helge Deller <deller@gmx.de>
> > ---
> >  hw/display/artist.c | 37 +++++++++++++++++++++++--------------
> >  1 file changed, 23 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/display/artist.c b/hw/display/artist.c
> > index 71982559c6..98bee6d61c 100644
> > --- a/hw/display/artist.c
> > +++ b/hw/display/artist.c
> > @@ -192,6 +192,10 @@ static const char *artist_reg_name(uint64_t addr)
> >  }
> >  #undef REG_NAME
> >
> > +/* artist has a fixed line length of 2048 bytes. */
> > +#define ADDR_TO_Y(addr) (((addr) >> 11) & 0x7ff)
> > +#define ADDR_TO_X(addr) ((addr) & 0x7ff)
>
> extract32()

fixed.

> > @@ -366,6 +370,8 @@ static void vram_bit_write(ARTISTState *s, int posx, 
> > int posy, bool incr_x,
> >      if (s->cmap_bm_access) {
> >          offset = s->vram_pos;
> >      } else {
> > +        posx = ADDR_TO_X(s->vram_pos >> 2);
> > +        posy += ADDR_TO_Y(s->vram_pos >> 2);
>
> Do you in fact want to fold the >> 2 into the ADDR_TO_X/Y, like
> #define ADDR_TO_X(POS)  extract32(POS, 2, 11)

Yes.
Basically this block was simply moved up to inside the function.
See in removed chunk below, there is the "s->vram_pos >> 2".

> > @@ -881,16 +886,12 @@ static void artist_reg_write(void *opaque, hwaddr 
> > addr, uint64_t val,
> >          break;
> >
> >      case VRAM_WRITE_INCR_Y:
> > -        posx = (s->vram_pos >> 2) & 0x7ff;
> > -        posy = (s->vram_pos >> 13) & 0x3ff;
> ...
> >      case VRAM_WRITE_INCR_X:
> >      case VRAM_WRITE_INCR_X2:
> > -        posx = (s->vram_pos >> 2) & 0x7ff;
> > -        posy = (s->vram_pos >> 13) & 0x3ff;
> ...
> > -    int posy = (addr >> 11) & 0x3ff;
>
> Is it a bug that these Y were using 0x3ff and not 0x7ff?
> Because it's pretty consistent...

I'm not sure if was intentional, a bug or just initial coding by Sven.
The old code was limiting to 1024 lines.
I assume that there doesn't exist any real hardware with > 1024 lines,
so it's more theoretical.
I tested the code, and it seems to work with > 1024 at least.
So, I think to use 0x7ff is probably more correct - at least if we
allow more lines.

> You should make that a separate change, for sure.

I'd prefer to keep it in one patch.
It's simply moving code around and reuse a macro.

> > @@ -1374,6 +1377,12 @@ static void artist_realizefn(DeviceState *dev, Error 
> > **errp)
> >      struct vram_buffer *buf;
> >      hwaddr offset = 0;
> >
> > +    /* Screen on artist can not be greater than 2048x2048 pixels. */
> > +    s->width = MAX(s->width, 640);
> > +    s->width = MIN(s->width, 2048);
> > +    s->height = MAX(s->height, 480);
> > +    s->height = MIN(s->height, 2048);
>
> Was the original values chosen by the user?

Yes, can bet set by user via
-global -global artist.width=800 -global artist.height=600

> Should we be giving some sort of error for out-of-range values?

I had a warning there, but then removed it again. Most users will
probably use the default values anyway. If you really want, I will add a
warning. I'm not a fan of errors. Errors usually abort, but we can
adjust and simply continue here.

Updated patch below.

Helge

----------------------------------

[PATCH] hw/display/artist: Fix artist screen resolution

Artist screen size is limited to 2048 x 2048 pixels and x/y coordination
addressing needs to be done by OS via an uint32 value which is based on
a 2048 byte line length, independend of the real screen size.

Since HP-UX seems to ideally need at least 640 pixels in width, this
patch ensures that the screen size stays between 640x480 and 2048x2048
pixels and fixes some pixel glitches were visible before on STI text
consoles due to the 2048 line length limitation.

Cc: Sven Schnelle <svens@stackframe.org>
Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/hw/display/artist.c b/hw/display/artist.c
index 71982559c6..71f527c0e1 100644
--- a/hw/display/artist.c
+++ b/hw/display/artist.c
@@ -192,6 +192,10 @@ static const char *artist_reg_name(uint64_t addr)
 }
 #undef REG_NAME

+/* artist has a fixed line length of 2048 bytes. */
+#define ADDR_TO_Y(addr) extract32(addr, 11, 11)
+#define ADDR_TO_X(addr) extract32(addr, 0, 11)
+
 static int16_t artist_get_x(uint32_t reg)
 {
     return reg >> 16;
@@ -348,13 +352,13 @@ static void artist_invalidate_cursor(ARTISTState *s)
                             y, s->cursor_height);
 }

-static void vram_bit_write(ARTISTState *s, int posx, int posy, bool incr_x,
+static void vram_bit_write(ARTISTState *s, int posy, bool incr_x,
                            int size, uint32_t data)
 {
     struct vram_buffer *buf;
     uint32_t vram_bitmask = s->vram_bitmask;
     int mask, i, pix_count, pix_length;
-    unsigned int offset, width;
+    unsigned int posx, offset, width;
     uint8_t *data8, *p;

     pix_count = vram_write_pix_per_transfer(s);
@@ -366,6 +370,8 @@ static void vram_bit_write(ARTISTState *s, int posx, int 
posy, bool incr_x,
     if (s->cmap_bm_access) {
         offset = s->vram_pos;
     } else {
+        posx = ADDR_TO_X(s->vram_pos >> 2);
+        posy += ADDR_TO_Y(s->vram_pos >> 2);
         offset = posy * width + posx;
     }

@@ -858,7 +864,6 @@ static void artist_reg_write(void *opaque, hwaddr addr, 
uint64_t val,
                              unsigned size)
 {
     ARTISTState *s = opaque;
-    int posx, posy;
     int width, height;

     trace_artist_reg_write(size, addr, artist_reg_name(addr & ~3ULL), val);
@@ -881,16 +886,12 @@ static void artist_reg_write(void *opaque, hwaddr addr, 
uint64_t val,
         break;

     case VRAM_WRITE_INCR_Y:
-        posx = (s->vram_pos >> 2) & 0x7ff;
-        posy = (s->vram_pos >> 13) & 0x3ff;
-        vram_bit_write(s, posx, posy + s->vram_char_y++, false, size, val);
+        vram_bit_write(s, s->vram_char_y++, false, size, val);
         break;

     case VRAM_WRITE_INCR_X:
     case VRAM_WRITE_INCR_X2:
-        posx = (s->vram_pos >> 2) & 0x7ff;
-        posy = (s->vram_pos >> 13) & 0x3ff;
-        vram_bit_write(s, posx, posy + s->vram_char_y, true, size, val);
+        vram_bit_write(s, s->vram_char_y, true, size, val);
         break;

     case VRAM_IDX:
@@ -1156,8 +1157,7 @@ static void artist_vram_write(void *opaque, hwaddr addr, 
uint64_t val,
 {
     ARTISTState *s = opaque;
     struct vram_buffer *buf;
-    int posy = (addr >> 11) & 0x3ff;
-    int posx = addr & 0x7ff;
+    unsigned int posy, posx;
     unsigned int offset;
     trace_artist_vram_write(size, addr, val);

@@ -1170,6 +1170,9 @@ static void artist_vram_write(void *opaque, hwaddr addr, 
uint64_t val,
     }

     buf = vram_write_buffer(s);
+    posy = ADDR_TO_Y(addr);
+    posx = ADDR_TO_X(addr);
+
     if (!buf->size) {
         return;
     }
@@ -1212,7 +1215,7 @@ static uint64_t artist_vram_read(void *opaque, hwaddr 
addr, unsigned size)
     ARTISTState *s = opaque;
     struct vram_buffer *buf;
     uint64_t val;
-    int posy, posx;
+    unsigned int posy, posx;

     if (s->cmap_bm_access) {
         buf = &s->vram_buffer[ARTIST_BUFFER_CMAP];
@@ -1229,8 +1232,8 @@ static uint64_t artist_vram_read(void *opaque, hwaddr 
addr, unsigned size)
         return 0;
     }

-    posy = (addr >> 13) & 0x3ff;
-    posx = (addr >> 2) & 0x7ff;
+    posy = ADDR_TO_Y(addr);
+    posx = ADDR_TO_X(addr);

     if (posy > buf->height || posx > buf->width) {
         return 0;
@@ -1374,6 +1377,12 @@ static void artist_realizefn(DeviceState *dev, Error 
**errp)
     struct vram_buffer *buf;
     hwaddr offset = 0;

+    /* Screen on artist can not be greater than 2048x2048 pixels. */
+    s->width = MAX(s->width, 640);
+    s->width = MIN(s->width, 2048);
+    s->height = MAX(s->height, 480);
+    s->height = MIN(s->height, 2048);
+
     memory_region_init(&s->mem_as_root, OBJECT(dev), "artist", ~0ull);
     address_space_init(&s->as, &s->mem_as_root, "artist");