diff mbox

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

Message ID 4C1A3139.5020905@citrix.com
State New
Headers show

Commit Message

Julian Pidancet June 17, 2010, 2:29 p.m. UTC 
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

Comments

Julian Pidancet June 17, 2010, 4:25 p.m. UTC | #1 
On 06/17/2010 03:29 PM, Julian Pidancet wrote:
> 
> Hi,
> 
> Thanks for spotting these errors. Here is a respin of my patch to address you concerns.
> (The munmap call is included).
> 
> Cheers,
> 
> Julian
> 

Oh, I actually tested the last patch only with the -nographic switch. There's still a segfault when starting qemu with vnc.
You can fix it by adding a call to dpy_resize(ds) after the dcl = NULL; line in fbdev_display_uninit().

For some reason, the display is extremely slow when using vnc and fbdev at the same time.

Julian
Gerd Hoffmann June 18, 2010, 7:32 a.m. UTC | #2 
Hi,

> For some reason, the display is extremely slow when using vnc and
> fbdev at the same time.

Gotcha.  Didn't notice, but it probably depends on the hardware.  Very 
likely the reason is that graphic cards usually are optimized for write 
access and reads might be slow as hell.   vnc must read though.

Hmm.  No easy way out.

cheers,
   Gerd
Julian Pidancet June 18, 2010, noon UTC | #3 
On 06/18/2010 08:32 AM, Gerd Hoffmann wrote:
>    Hi,
> 
>> For some reason, the display is extremely slow when using vnc and
>> fbdev at the same time.
> 
> Gotcha.  Didn't notice, but it probably depends on the hardware.  Very 
> likely the reason is that graphic cards usually are optimized for write 
> access and reads might be slow as hell.   vnc must read though.
> 

Access to the framebuffer are cached Write-Combining by default with fbdev, which is probably causing this latency.

One solution would be to disable the display allocator when vnc is present, and let it read from a software surface instead of reading from the framebuffer (like in your initial patch). It would probably decrease display performance, but not as much as it is now if we let the vnc driver read from the hardware framebuffer.

We can easily implement a surface usage counter in the display allocator code, so the fbdev driver can know whether or not the surface is read by other drivers at the same time.
diff mbox

Patch

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