diff mbox series

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

Message ID 20200901183452.24967-7-deller@gmx.de
State New
Headers show
Series hppa power button support, graphics updates and firmware fixes | expand

Commit Message

Helge Deller Sept. 1, 2020, 6:34 p.m. UTC
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(-)

--
2.21.3

Comments

Richard Henderson Sept. 1, 2020, 9:49 p.m. UTC | #1
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()

> +
>  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);

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)

?

> @@ -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...

You should make that a separate change, for sure.

> @@ -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?  Should we be giving some sort of
error for out-of-range values?


r~
Helge Deller Sept. 2, 2020, 9:48 p.m. UTC | #2
* 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");
Richard Henderson Sept. 2, 2020, 10:09 p.m. UTC | #3
On 9/2/20 2:48 PM, Helge Deller wrote:
>> 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.

It's more than that, it's a behaviour change.

Code refactoring should be separate from feature changes, when at all possible.
 And here, its certainly possible.

>> 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.

It's exactly the same error as -smp 1234, when only 8 cpus are supported.  An
error is more than appropriate.


r~
Helge Deller Sept. 3, 2020, 6:07 a.m. UTC | #4
New patch #1:

From 80ad61c2ba25ca608b7c1c1cf701ae8164219cf6 Mon Sep 17 00:00:00 2001
From: Helge Deller <deller@gmx.de>
Subject: [PATCH] hw/display/artist: Verify artist screen resolution

Artist hardware is limited to 2048 x 2048 pixels.
STI ROMs allow at minimum 640 x 480 pixels.

Qemu users can adjust the screen size on the command line with:
 -global artist.width=800 -global artist.height=600
but we need to ensure that the screen size stays inside the given
boundaries, otherwise print an error message and adjust.

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/hw/display/artist.c b/hw/display/artist.c
index 71982559c6..ff1532fdc1 100644
--- a/hw/display/artist.c
+++ b/hw/display/artist.c
@@ -1374,6 +1374,18 @@ static void artist_realizefn(DeviceState *dev, Error **errp)
     struct vram_buffer *buf;
     hwaddr offset = 0;

+    if (s->width > 2048 || s->height > 2048) {
+        error_report("artist: screen size can not exceed 2048 x 2048 pixel.");
+        s->width = MIN(s->width, 2048);
+        s->height = MIN(s->height, 2048);
+    }
+
+    if (s->width < 640 || s->height < 480) {
+        error_report("artist: minimum screen size is 640 x 480 pixel.");
+        s->width = MAX(s->width, 640);
+        s->height = MAX(s->height, 480);
+    }
+
     memory_region_init(&s->mem_as_root, OBJECT(dev), "artist", ~0ull);
     address_space_init(&s->as, &s->mem_as_root, "artist");
Helge Deller Sept. 3, 2020, 6:08 a.m. UTC | #5
New patch #2:

From b0f0d24563df504b8221fbc934d25c5a896e0a49 Mon Sep 17 00:00:00 2001
From: Helge Deller <deller@gmx.de>
Subject: [PATCH] hw/display/artist: Refactor x/y coordination extraction

Simplify the code by using new introduced ADDR_TO_Y() and ADDR_TO_X()
macros. Those macros extract the x/y-coordinate from the given uint32.

As further simplification the extraction of the x/y coordinates for
VRAM_WRITE_INCR_Y and VRAM_WRITE_INCR_X can be done centrally in
vram_bit_write(), so move this code up into the function.

ADDR_TO_Y() is still limited to 10 bits which allow to address up to of
1024 lines - this will be increased in a follow-up patch.

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/hw/display/artist.c b/hw/display/artist.c
index ff1532fdc1..16d85c65f8 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, 10)
+#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;
Helge Deller Sept. 3, 2020, 6:09 a.m. UTC | #6
New patch #3/3:

From 476aeb9b832ae172a9d6a28aa9e43300dedd419b Mon Sep 17 00:00:00 2001
Subject: [PATCH] hw/display/artist: Allow screen size up to 2048 lines

Adjust the ADDR_TO_Y() macro to extract 11 bits, which allows userspace
to address screen sizes up to 2048 lines (instead of 1024 before).

Signed-off-by: Helge Deller <deller@gmx.de>

diff --git a/hw/display/artist.c b/hw/display/artist.c
index 16d85c65f8..955296d3d8 100644
--- a/hw/display/artist.c
+++ b/hw/display/artist.c
@@ -193,7 +193,7 @@ 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, 10)
+#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)
Richard Henderson Sept. 3, 2020, 3:09 p.m. UTC | #7
On 9/2/20 11:09 PM, Helge Deller wrote:
> New patch #3/3:
> 
> From 476aeb9b832ae172a9d6a28aa9e43300dedd419b Mon Sep 17 00:00:00 2001
> Subject: [PATCH] hw/display/artist: Allow screen size up to 2048 lines
> 
> Adjust the ADDR_TO_Y() macro to extract 11 bits, which allows userspace
> to address screen sizes up to 2048 lines (instead of 1024 before).
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> 
> diff --git a/hw/display/artist.c b/hw/display/artist.c
> index 16d85c65f8..955296d3d8 100644
> --- a/hw/display/artist.c
> +++ b/hw/display/artist.c
> @@ -193,7 +193,7 @@ 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, 10)
> +#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)
> 

Thanks.  All 3 can have a
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~
diff mbox series

Patch

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)
+
 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");