Re: [Qemu-devel] [PATCH 7/9] fbdev: move to pixman

[Gerd Hoffmann <kraxel@redhat.com>]

  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