[Qemu-devel] Re: [PATCH 5/5] linux fbdev display driver.

[Julian Pidancet <julian.pidancet@citrix.com>]

On 06/17/2010 11:43 AM, Gerd Hoffmann wrote:
>    Hi,
> 
> You register the display allocator, but don't unregister in 
> fbdev_display_uninit().
> 
> You are just lucky that fbdev_cleanup() forgets to unmap the framebuffer.
> 
> Apply the attached fix, start qemu with vnc, then do "change fbdev on" 
> and "change fbdev off" in the monitor and watch qemu segfault.
> 
> Also after "change fbdev on" the guest screen isn't rendered correctly.
> 
> cheers,
>    Gerd
> 

Hi,

Thanks for spotting these errors. Here is a respin of my patch to address you concerns.
(The munmap call is included).

Cheers,

Julian

diff --git a/console.c b/console.c
index 698bc10..12ce215 100644
--- a/console.c
+++ b/console.c
@@ -1376,6 +1376,16 @@ DisplayAllocator *register_displayallocator(DisplayState *ds, DisplayAllocator *
     return ds->allocator;
 }
 
+void unregister_displayallocator(DisplayState *ds)
+{
+    if (ds->allocator != &default_allocator) {
+        ds->allocator->free_displaysurface(ds->surface);
+        ds->surface = defaultallocator_create_displaysurface(ds_get_width(ds),
+                                                             ds_get_height(ds));
+        ds->allocator = &default_allocator;
+    }
+}
+
 DisplayState *graphic_console_init(vga_hw_update_ptr update,
                                    vga_hw_invalidate_ptr invalidate,
                                    vga_hw_screen_dump_ptr screen_dump,
diff --git a/console.h b/console.h
index 124a22b..40bd927 100644
--- a/console.h
+++ b/console.h
@@ -192,6 +192,7 @@ PixelFormat qemu_different_endianness_pixelformat(int bpp);
 PixelFormat qemu_default_pixelformat(int bpp);
 
 DisplayAllocator *register_displayallocator(DisplayState *ds, DisplayAllocator *da);
+void unregister_displayallocator(DisplayState *ds);
 
 static inline DisplaySurface* qemu_create_displaysurface(DisplayState *ds, int width, int height)
 {
@@ -371,7 +372,7 @@ void sdl_display_init(DisplayState *ds, int full_screen, int no_frame);
 
 /* fbdev.c */
 void fbdev_display_init(DisplayState *ds, const char *device);
-void fbdev_display_uninit(void);
+void fbdev_display_uninit(DisplayState *ds);
 
 /* cocoa.m */
 void cocoa_display_init(DisplayState *ds, int full_screen);
diff --git a/fbdev.c b/fbdev.c
index 54f2381..8ea1838 100644
--- a/fbdev.c
+++ b/fbdev.c
@@ -67,13 +67,13 @@ static int fb_switch_state = FB_ACTIVE;
 
 /* qdev windup */
 static DisplayChangeListener      *dcl;
+static DisplayAllocator           *da;
 static QemuPfConv                 *conv;
 static PixelFormat                fbpf;
-static int                        resize_screen;
-static int                        redraw_screen;
 static int                        cx, cy, cw, ch;
 static int                        debug = 0;
 static Notifier                   exit_notifier;
+uint8_t                           *guest_surface;
 
 /* fwd decls */
 static int fbdev_activate_vt(int tty, int vtno, bool wait);
@@ -519,6 +519,10 @@ static void fbdev_cleanup(void)
         fprintf(stderr, "%s\n", __FUNCTION__);
 
     /* restore console */
+    if (fb_mem != NULL) {
+        munmap(fb_mem, fb_fix.smem_len + fb_mem_offset);
+        fb_mem = NULL;
+    }
     if (fb != -1) {
         if (ioctl(fb,FBIOPUT_VSCREENINFO, &fb_ovar) < 0)
             perror("ioctl FBIOPUT_VSCREENINFO");
@@ -786,10 +790,10 @@ static void fbdev_render(DisplayState *ds, int x, int y, int w, int h)
     uint8_t *src;
     int line;
 
-    if (!conv)
+    if (!conv || !guest_surface)
         return;
 
-    src = ds_get_data(ds) + y * ds_get_linesize(ds)
+    src = guest_surface + y * ds_get_linesize(ds)
         + x * ds_get_bytes_per_pixel(ds);
     dst = fb_mem + y * fb_fix.line_length
         + x * fbpf.bytes_per_pixel;
@@ -819,46 +823,50 @@ static void fbdev_update(DisplayState *ds, int x, int y, int w, int h)
     if (fb_switch_state != FB_ACTIVE)
         return;
 
-    if (resize_screen) {
-        if (debug)
-            fprintf(stderr, "%s: handle resize\n", __FUNCTION__);
-        resize_screen = 0;
-        cx = 0; cy = 0;
-        cw = ds_get_width(ds);
-        ch = ds_get_height(ds);
-        if (ds_get_width(ds) < fb_var.xres) {
-            cx = (fb_var.xres - ds_get_width(ds)) / 2;
-        }
-        if (ds_get_height(ds) < fb_var.yres) {
-            cy = (fb_var.yres - ds_get_height(ds)) / 2;
-        }
+    if (guest_surface != NULL) {
+        fbdev_render(ds, x, y, w, h);
+    }
+}
 
-        if (conv) {
-            qemu_pf_conv_put(conv);
-        }
-        conv = qemu_pf_conv_get(&fbpf, &ds->surface->pf);
-        if (conv == NULL) {
-            fprintf(stderr, "fbdev: unsupported PixelFormat conversion\n");
-        }
+static void fbdev_setdata(DisplayState *ds)
+{
+    if (conv) {
+        qemu_pf_conv_put(conv);
     }
 
-    if (redraw_screen) {
-        if (debug)
-            fprintf(stderr, "%s: handle redraw\n", __FUNCTION__);
-        redraw_screen = 0;
-        fbdev_cls();
-        x = 0; y = 0; w = ds_get_width(ds); h = ds_get_height(ds);
+    conv = qemu_pf_conv_get(&fbpf, &ds->surface->pf);
+    if (conv == NULL) {
+        fprintf(stderr, "fbdev: unsupported PixelFormat conversion\n");
     }
 
-    fbdev_render(ds, x, y, w, h);
+    guest_surface = ds_get_data(ds);
 }
 
 static void fbdev_resize(DisplayState *ds)
 {
-    if (debug)
-        fprintf(stderr, "%s: request resize+redraw\n", __FUNCTION__);
-    resize_screen++;
-    redraw_screen++;
+    int is_video_ptr;
+
+    if (fb_switch_state == FB_ACTIVE) {
+        fbdev_cls();
+    }
+
+    is_video_ptr = ds_get_data(ds) >= fb_mem + fb_mem_offset &&
+                   ds_get_data(ds) < fb_mem + fb_fix.smem_len + fb_mem_offset;
+
+    if (ds_get_bits_per_pixel(ds) != fbpf.bits_per_pixel ||
+        ds_get_linesize(ds) != fb_fix.line_length ||
+        !is_video_ptr) {
+        cx = 0; cy = 0;
+        if (ds_get_width(ds) < fb_var.xres) {
+            cx = (fb_var.xres - ds_get_width(ds)) / 2;
+        }
+        if (ds_get_height(ds) < fb_var.yres) {
+            cy = (fb_var.yres - ds_get_height(ds)) / 2;
+        }
+        fbdev_setdata(ds);
+    } else {
+        guest_surface = NULL;
+    }
 }
 
 static void fbdev_refresh(DisplayState *ds)
@@ -866,21 +874,17 @@ static void fbdev_refresh(DisplayState *ds)
     switch (fb_switch_state) {
     case FB_REL_REQ:
         fbdev_switch_release();
+        vga_hw_invalidate();
     case FB_INACTIVE:
         return;
     case FB_ACQ_REQ:
         fbdev_switch_acquire();
-        redraw_screen++;
-        if (debug)
-            fprintf(stderr, "%s: request redraw\n", __FUNCTION__);
+        vga_hw_invalidate();
     case FB_ACTIVE:
         break;
     }
 
     vga_hw_update();
-    if (redraw_screen) {
-        fbdev_update(ds, 0, 0, 0, 0);
-    }
 }
 
 static void fbdev_exit_notifier(Notifier *notifier)
@@ -888,6 +892,52 @@ static void fbdev_exit_notifier(Notifier *notifier)
     fbdev_cleanup();
 }
 
+static DisplaySurface *fbdev_create_displaysurface(int width, int height)
+{
+    DisplaySurface *surface = qemu_mallocz(sizeof (DisplaySurface));
+
+    surface->width = width;
+    surface->height = height;
+
+    surface->pf = fbpf;
+    surface->linesize = fb_fix.line_length;
+
+    if (fb_switch_state == FB_INACTIVE) {
+        surface->flags = QEMU_ALLOCATED_FLAG;
+        surface->data = qemu_mallocz(surface->linesize * surface->height);
+    } else {
+        surface->flags = QEMU_REALPIXELS_FLAG;
+        surface->data = fb_mem;
+
+        if (width < fb_var.xres)
+            surface->data += ((fb_var.xres - width) / 2) * fbpf.bytes_per_pixel;
+        if (height < fb_var.yres)
+            surface->data += ((fb_var.yres - height) / 2) * fb_fix.line_length;
+    }
+
+    return surface;
+}
+
+static void fbdev_free_displaysurface(DisplaySurface *surface)
+{
+    if (surface == NULL)
+        return;
+
+    if (surface->flags & QEMU_ALLOCATED_FLAG) {
+        qemu_free(surface->data);
+    }
+
+    qemu_free(surface);
+}
+
+static DisplaySurface *fbdev_resize_displaysurface(DisplaySurface *surface,
+                                                   int width,
+                                                   int height)
+{
+    fbdev_free_displaysurface(surface);
+    return fbdev_create_displaysurface(width, height);
+}
+
 void fbdev_display_init(DisplayState *ds, const char *device)
 {
     if (dcl != NULL) {
@@ -905,15 +955,24 @@ void fbdev_display_init(DisplayState *ds, const char *device)
     fbdev_catch_exit_signals();
     init_mouse();
 
-    fbdev_resize(ds);
     dcl = qemu_mallocz(sizeof(DisplayChangeListener));
     dcl->dpy_update  = fbdev_update;
     dcl->dpy_resize  = fbdev_resize;
     dcl->dpy_refresh = fbdev_refresh;
+    dcl->dpy_setdata = fbdev_setdata;
     register_displaychangelistener(ds, dcl);
+
+    da = qemu_mallocz(sizeof (DisplayAllocator));
+    da->create_displaysurface = fbdev_create_displaysurface;
+    da->resize_displaysurface = fbdev_resize_displaysurface;
+    da->free_displaysurface = fbdev_free_displaysurface;
+
+    if (register_displayallocator(ds, da) == da) {
+        vga_hw_invalidate();
+    }
 }
 
-void fbdev_display_uninit(void)
+void fbdev_display_uninit(DisplayState *ds)
 {
     if (dcl == NULL) {
         if (debug)
@@ -921,6 +980,9 @@ void fbdev_display_uninit(void)
         return;
     }
 
+    unregister_displayallocator(ds);
+    qemu_free(da);
+
     unregister_displaychangelistener(dcl);
     qemu_free(dcl);
     dcl = NULL;
diff --git a/monitor.c b/monitor.c
index 7442f1a..5d6efb4 100644
--- a/monitor.c
+++ b/monitor.c
@@ -965,7 +965,7 @@ static int do_change_fbdev(Monitor *mon, const char *target)
     if (strcmp(target, "on") == 0) {
         fbdev_display_init(get_displaystate(), NULL);
     } else {
-        fbdev_display_uninit();
+        fbdev_display_uninit(get_displaystate());
     }
     return 0;
 #endif