Re: [Qemu-devel] [PATCH v3 08/13] sm501: Fix hardware cursor

[Peter Maydell]

On 4 December 2016 at 18:01, BALATON Zoltan  wrote:
> Signed-off-by: BALATON Zoltan 
> ---
>
> v3: simplify return expression in get_bpp

In my review on v2 I asked for the commit message to
say clearly what the bugs being fixed here are. It's
a lot easier to review a patch if I know what it's supposed
to be doing rather than having to figure it out from scratch.

thanks
-- PMM

[Qemu-devel] [PATCH v3 08/13] sm501: Fix hardware cursor

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan 
---

v3: simplify return expression in get_bpp

 hw/display/sm501.c  | 169 +---
 hw/display/sm501_template.h |  25 +++
 2 files changed, 107 insertions(+), 87 deletions(-)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index 021b605..de9f3e7 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -554,6 +554,24 @@ static uint32_t get_local_mem_size_index(uint32_t size)
 return index;
 }
 
+static inline int get_width(SM501State *s, int crt)
+{
+int width = crt ? s->dc_crt_h_total : s->dc_panel_h_total;
+return (width & 0x0FFF) + 1;
+}
+
+static inline int get_height(SM501State *s, int crt)
+{
+int height = crt ? s->dc_crt_v_total : s->dc_panel_v_total;
+return (height & 0x0FFF) + 1;
+}
+
+static inline int get_bpp(SM501State *s, int crt)
+{
+int bpp = crt ? s->dc_crt_control : s->dc_panel_control;
+return 1 << (bpp & 3);
+}
+
 /**
  * Check the availability of hardware cursor.
  * @param crt  0 for PANEL, 1 for CRT.
@@ -568,10 +586,10 @@ static inline int is_hwc_enabled(SM501State *state, int 
crt)
  * Get the address which holds cursor pattern data.
  * @param crt  0 for PANEL, 1 for CRT.
  */
-static inline uint32_t get_hwc_address(SM501State *state, int crt)
+static inline uint8_t *get_hwc_address(SM501State *state, int crt)
 {
 uint32_t addr = crt ? state->dc_crt_hwc_addr : state->dc_panel_hwc_addr;
-return (addr & 0x03F0)/* >> 4*/;
+return state->local_mem + (addr & 0x03F0);
 }
 
 /**
@@ -597,50 +615,48 @@ static inline uint32_t get_hwc_x(SM501State *state, int 
crt)
 }
 
 /**
- * Get the cursor position in x coordinate.
+ * Get the hardware cursor palette.
  * @param crt  0 for PANEL, 1 for CRT.
- * @param index  0, 1, 2 or 3 which specifies color of corsor dot.
+ * @param palette  pointer to a [3 * 3] array to store color values in
  */
-static inline uint16_t get_hwc_color(SM501State *state, int crt, int index)
+static inline void get_hwc_palette(SM501State *state, int crt, uint8_t 
*palette)
 {
-uint32_t color_reg = 0;
-uint16_t color_565 = 0;
-
-if (index == 0) {
-return 0;
-}
-
-switch (index) {
-case 1:
-case 2:
-color_reg = crt ? state->dc_crt_hwc_color_1_2
-: state->dc_panel_hwc_color_1_2;
-break;
-case 3:
-color_reg = crt ? state->dc_crt_hwc_color_3
-: state->dc_panel_hwc_color_3;
-break;
-default:
-printf("invalid hw cursor color.\n");
-abort();
-}
+int i;
+uint32_t color_reg;
+uint16_t rgb565;
+
+for (i = 0; i < 3; i++) {
+if (i + 1 == 3) {
+color_reg = crt ? state->dc_crt_hwc_color_3
+: state->dc_panel_hwc_color_3;
+} else {
+color_reg = crt ? state->dc_crt_hwc_color_1_2
+: state->dc_panel_hwc_color_1_2;
+}
 
-switch (index) {
-case 1:
-case 3:
-color_565 = (uint16_t)(color_reg & 0x);
-break;
-case 2:
-color_565 = (uint16_t)((color_reg >> 16) & 0x);
-break;
+if (i + 1 == 2) {
+rgb565 = (color_reg >> 16) & 0x;
+} else {
+rgb565 = color_reg & 0x;
+}
+palette[i * 3 + 0] = (rgb565 << 3) & 0xf8; /* red */
+palette[i * 3 + 1] = (rgb565 >> 3) & 0xfc; /* green */
+palette[i * 3 + 2] = (rgb565 >> 8) & 0xf8; /* blue */
 }
-return color_565;
 }
 
-static int within_hwc_y_range(SM501State *state, int y, int crt)
+static inline void hwc_invalidate(SM501State *s, int crt)
 {
-int hwc_y = get_hwc_y(state, crt);
-return (hwc_y <= y && y < hwc_y + SM501_HWC_HEIGHT);
+int w = get_width(s, crt);
+int h = get_height(s, crt);
+int bpp = get_bpp(s, crt);
+int start = get_hwc_y(s, crt);
+int end = MIN(h, start + SM501_HWC_HEIGHT) + 1;
+
+start *= w * bpp;
+end *= w * bpp;
+
+memory_region_set_dirty(>local_mem_region, start, end - start);
 }
 
 static void sm501_2d_operation(SM501State *s)
@@ -1021,10 +1037,18 @@ static void sm501_disp_ctrl_write(void *opaque, hwaddr 
addr,
 break;
 
 case SM501_DC_PANEL_HWC_ADDR:
-s->dc_panel_hwc_addr = value & 0x8FF0;
+value &= 0x8FF0;
+if (value != s->dc_panel_hwc_addr) {
+hwc_invalidate(s, 0);
+s->dc_panel_hwc_addr = value;
+}
 break;
 case SM501_DC_PANEL_HWC_LOC:
-s->dc_panel_hwc_location = value & 0x0FFF0FFF;
+value &= 0x0FFF0FFF;
+if (value != s->dc_panel_hwc_location) {
+hwc_invalidate(s, 0);
+s->dc_panel_hwc_location = value;
+}
 break;
 case SM501_DC_PANEL_HWC_COLOR_1_2:
 s->dc_panel_hwc_color_1_2 = value;
@@ -1056,10 +1080,18 @@ static void sm501_disp_ctrl_write(void *opaque, hwaddr 
addr,