From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:33206) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEa49-0002KN-AM for qemu-devel@nongnu.org; Thu, 20 Sep 2012 02:16:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TEa47-0004qK-HP for qemu-devel@nongnu.org; Thu, 20 Sep 2012 02:16:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:6924) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TEa47-0004pn-7z for qemu-devel@nongnu.org; Thu, 20 Sep 2012 02:16:47 -0400 Message-ID: <505AB4C9.9070809@redhat.com> Date: Thu, 20 Sep 2012 08:16:41 +0200 From: Gerd Hoffmann MIME-Version: 1.0 References: <1348053341-29212-1-git-send-email-kraxel@redhat.com> <1348053341-29212-8-git-send-email-kraxel@redhat.com> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 7/9] fbdev: move to pixman List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: "qemu-devel@nongnu.org" Hi, >> + if (surface) { >> + pixman_image_unref(surface); >> } >> + surface = pixman_from_displaystate(ds); > > Am I reading this right? Are you creating a new pixman surface in every > call to fbdev_update? No. The whole block doing this is wrapped into "if (resize_screen)" (which you don't see in the patch due to context being too small). >> @@ -916,6 +953,10 @@ static void fbdev_refresh(DisplayState *ds) >> if (redraw_screen) { >> fbdev_update(ds, 0, 0, 0, 0); >> } >> + >> + if (pixman_region_not_empty(&dirty)) { >> + fbdev_render(ds); >> + } > > Why are you using fbdev_refresh for rendering instead of fbdev_update? > From consistency with sdl and vnc as well as the semantics of these > callbacks I think it would be better to do the rendering from > fbdev_update and just call vga_hw_update here. It _does_ call vga_hw_update. The fbdev_update callbacks triggered by this collect the updated regions in the dirty variable. Finally we'll render everything in one go (assuming we got any updates). Performs better than doing the rendering in the fbdev_update callback. > Pixman or non-pixman, I still think that this could benefit from > implementing a DisplayAllocator interface: it would avoid a memcpy > whenever there is no need for scaling and pixel conversions. There is one more issue I didn't mention yet: The framebuffer memory should better be treaded as write-only memory as this is what gfx cards are optimized for. Read access works of course, but can be _very_ slow depending on the hardware. So implementing a DisplayAllocator and thereby making the vga emulation operate directly on framebuffer memory is a very bad idea IMO. Most likely it will make certain operations (like cirrus bitblits) slower even though it saves a memcpy. cheers, Gerd