Re: [Qemu-devel] [PATCH 0 of 7] [UPDATE] DisplayState interface change

[Stefano Stabellini]

Paul Brook wrote:

>> This patch changes the DisplayState interface adding support for
>> multiple frontends at the same time (sdl and vnc) and implements most
>> of the benefit of the shared_buf patch without the added complexity.
> 
> I'm still not convinced you've got the abstraction right here.
> 
> By my reading your shared buffer code is fairly fragile. If the users 
> switches 
> VCs (which will cause qemu to reallocate the surface independently of the 
> device) then qemu will reallocate the buffer. Having both qemu and individual 
> device code allocating surfaces seems wrong. It should be one or the other.


This is something that happens even without my patches and will go away
by itself when we get rid of QEMUConsole.

 
> It also feels messy the way that allocating the surface and notifying the 
> DisplayState of the change are two separate operations.
> IMHO DisplayState should be an opaque structure as far as devices are 
> concerned. They should never have to access its members (or deal with 
> DisplaySurface) directly.


I can fix that implementing a qemu_console_resize_shared(int width, int
height, int bpp, int linesize, uint8_t *data) that does what the vga
code is now doing.
Since it is important for you I'll do it.

>> - do not use is_active_console, use is_graphic_console instead.
> 
> This is wrong. There may be multiple graphic consoles.

I thought we wanted to move to a model in which a DisplayState
corresponds to a single QEMUConsole and a single graphic device.
This is the reason for Anthony to ask me this update, if I am not mistaken.

>> This patch changes the graphical_console_init function to return an
>> allocated DisplayState instead of a QEMUConsole.
> 
> You should also remove QEMUConsole.


I removed QEMUConsole from the graphic devices, I am not going to
completely get rid of it because it is currently needed by text consoles
and to multiplex multiple consoles.
Implementing a DisplayState multiplexer is the subject of another patch
series.
Keep in mind that now (without this series) we multiplex multiple
QEMUConsole on a DisplayState. I didn't introduce it, it was already there.

 
>> +    s->ds = graphic_console_init(s->update, s->invalidate,
>> +                                 s->screen_dump, s->text_update, s);
>> +    register_displaystate(s->ds);
> 
> Why have you got separate allocate and register functions?
> 

No need. I can easily change that.