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

[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()

> +
>  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~