Re: [Qemu-trivial] [PATCH v6 08/13] sm501: Fix hardware cursor

[Aurelien Jarno]

On 2017-04-21 12:31, BALATON Zoltan wrote:
> Rework HWC handling to simplify it and fix cursor not updating on
> screen as needed. Previously cursor was not updated because checking
> for changes in a line overrode the update flag set for the cursor but
> fixing this is not enough because the cursor should also be updated if
> its shape or location changes. Introduce hwc_invalidate() function to
> handle that similar to other display controller models.
> 
> Signed-off-by: BALATON Zoltan <address@hidden>
> Reviewed-by: Peter Maydell <address@hidden>
> ---
> 
> 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 a628ef1..dc806a3 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1339,14 +1362,16 @@ static void sm501_draw_crt(SM501State *s)
>      /* draw each line according to conditions */
>      memory_region_sync_dirty_bitmap(&s->local_mem_region);
>      for (y = 0; y < height; y++) {
> -        int update_hwc = draw_hwc_line ? within_hwc_y_range(s, y, 1) : 0;
> -        int update = full_update || update_hwc;
> +        int update, update_hwc;
>          ram_addr_t page0 = offset;
>          ram_addr_t page1 = offset + width * src_bpp - 1;
>  
> +        /* check if hardware cursor is enabled and we're within its range */
> +        update_hwc = draw_hwc_line && c_y <= y && y < c_y + SM501_HWC_HEIGHT;
> +        update = full_update || update_hwc;
>          /* check dirty flags for each line */
> -        update = memory_region_get_dirty(&s->local_mem_region, page0,
> -                                         page1 - page0, DIRTY_MEMORY_VGA);
> +        update |= memory_region_get_dirty(&s->local_mem_region, page0,
> +                                          page1 - page0, DIRTY_MEMORY_VGA);
>  
>          /* draw line and change status */
>          if (update) {
> @@ -1358,7 +1383,7 @@ static void sm501_draw_crt(SM501State *s)
>  
>              /* draw hardware cursor */
>              if (update_hwc) {
> -                draw_hwc_line(s, 1, hwc_palette, y - get_hwc_y(s, 1), d, 
> width);
> +                draw_hwc_line(d, hwc_src, width, hwc_palette, c_x, y - c_y);
>              }
>  
>              if (y_start < 0) {

The above change causes a warning with at least GCC 6 and GCC 7, which
causes a build failure when using -Werror:

|   CC      sh4-softmmu/hw/display/sm501.o
| /home/aurel32/git/qemu/hw/display/sm501.c: In function ‘sm501_update_display’:
| /home/aurel32/git/qemu/hw/display/sm501.c:1504:17: warning: ‘hwc_src’ may be 
used uninitialized in this function [-Wmaybe-uninitialized]
|                  draw_hwc_line(d, hwc_src, width, hwc_palette, c_x, y - c_y);
|                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| /home/aurel32/git/qemu/hw/display/sm501.c:1488:59: warning: ‘c_y’ may be used 
uninitialized in this function [-Wmaybe-uninitialized]
|          update_hwc = draw_hwc_line && c_y <= y && y < c_y + SM501_HWC_HEIGHT;
|                                                            ^
| /home/aurel32/git/qemu/hw/display/sm501.c:1504:17: warning: ‘c_x’ may be used 
uninitialized in this function [-Wmaybe-uninitialized]
|                  draw_hwc_line(d, hwc_src, width, hwc_palette, c_x, y - c_y);
|                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

GCC fails to notice that hwc_src, c_x and c_y are always defined if
draw_hwc_line is not NULL. This is obviously not a bug in the code, but
it might be a good idea to put a workaround for example by defining
default values for those variables.

Aurelien
-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net