DisplayState interface change
PATCH 1/1 DisplayState interface change
Hi all,
this is the third update of the "DisplayState interface change" series.
The series is now of made of 7 patches:
1) accessors again
some other substitutions in hw/sm501.c;
2) remove bgr
the new DisplayState interface does not contain any host specific display detail, it is an abstraction of the backend display, hence we don't need to memorize the bgr flag in DisplayState.
The frontend must be able to handle a bgr display by itself, in fact sdl is perfectly capable of that;
3) DisplayState interface change
this is the big patch that actually changes the interface;
4) vnc improvements
this patch introduces DisplaySurfaces in vnc, simplifying the code.
5) graphical_console_init change
this is the patch that changes the graphical_console_init function to return an allocated DisplayState instead of a QEMUConsole, as Anthony suggested.
This patch does *not* include any required changes to any devices, these changes come with the two following patches.
6) machine changes
this patch changes the QEMUMachine init functions of all the machine types not to take a DisplayState as an argument because is not needed any more;
7) graphic device changes
this patch updates the graphic device code to use the new graphical_console_init function.
Patch number 5 requires also 6 and 7 to compile.
More comments on single patches.
Cheers,
Stefano
Quoted 3 lines
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_bufpatch 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.
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.
Quoted 1 line
- do not use
is_active_console, useis_graphic_consoleinstead.
This is wrong. There may be multiple graphic consoles.
Quoted 2 lines
This patch changes the
graphical_console_initfunction to return an allocated DisplayState instead of a QEMUConsole.
You should also remove QEMUConsole.
Review
Quoted 3 lines
(quoted diff) +3
-0+ 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?
Paul
Quoted 10 lines · Paul Brook
Quoted 3 lines
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_bufpatch 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.
Quoted 5 lines
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.
Quoted 3 lines
Quoted 1 line
- do not use
is_active_console, useis_graphic_consoleinstead.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.
Quoted 4 lines
Quoted 2 lines
This patch changes the
graphical_console_initfunction 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.
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +3
-0+ 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.
Quoted 4 lines · Stefano Stabellini
Paul Brook wrote:
Quoted 9 lines
Quoted 4 lines
Quoted 2 lines
- do not use
is_active_console, useis_graphic_consoleinstead.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.
I need to look more closely at the patches (and I will tomorrow), but to me, the use of is_active_console or is_graphics_console is a red herring. Nothing should never need to know whether it's the "active" console. Such a concept simply shouldn't exist.
I think this is Paul's point about multiple graphics consoles. If you have two graphics consoles, than the concept of active console doesn't make very much sense. Is that what you were thinking Paul?
Quoted 10 lines
Quoted 5 lines
Quoted 3 lines
This patch changes the
graphical_console_initfunction 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.
QEMUConsole is just a typedef right now. Could you remove the typedef? It should become unused after your patch.
Regards,
Anthony Liguori
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
- do not use
is_active_console, useis_graphic_consoleinstead.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.
Quoted 8 lines
I need to look more closely at the patches (and I will tomorrow), but to me, the use of
is_active_consoleoris_graphics_consoleis a red herring. Nothing should never need to know whether it's the "active" console. Such a concept simply shouldn't exist.I think this is Paul's point about multiple graphics consoles. If you have two graphics consoles, than the concept of active console doesn't make very much sense. Is that what you were thinking Paul?
Something like that, yes.
My understanding was that the plan is to have one DisplayState per image source. At that point the device doesn't care whether it's "active" or not (inactive DisplayStates will be the equivalent of /dev/null).
If we still have a single DisplayState shared by multiple sources then is_graphics_console is not a sufficient check to tell whether the VGA device is active.
I'm not sure that removing QEMUConsole without adding multiple DisplayStates is feasible.
Paul
Quoted 8 lines · Paul Brook
Something like that, yes.
My understanding was that the plan is to have one DisplayState per image source. At that point the device doesn't care whether it's "active" or not (inactive DisplayStates will be the equivalent of
/dev/null). If we still have a single DisplayState shared by multiple sources thenis_graphics_consoleis not a sufficient check to tell whether the VGA device is active.I'm not sure that removing QEMUConsole without adding multiple DisplayStates is feasible.
I think this patch series is a big step in this direction. The is_graphics_console is an artifact. What we need to follow up this series with, is adding keyboard/mouse input to DisplayState, and pushing all the virtual terminal switching into TextConsole and cleaning TextConsole up to be a more clean virtual KVM.
If noone else takes this on, I'll add it to my queue. I think it's a follow up series though to this one.
Regards,
Anthony Liguori
Quoted 2 lines
Paul
Quoted 6 lines · Anthony Liguori
I think this patch series is a big step in this direction. The
is_graphics_consoleis an artifact. What we need to follow up this series with, is adding keyboard/mouse input to DisplayState, and pushing all the virtual terminal switching into TextConsole and cleaning TextConsole up to be a more clean virtual KVM.
if it is any better I can move the is_graphic_console artifact from vga.c to console.c:
void qemu_console_resize(DisplayState *ds, int width, int height, int bpp,
int linesize, uint8_t *data)
{
TextConsole *s = get_graphic_console();
s->g_width = width;
s->g_height = height;
if (is_graphic_console()) {
if (data && (bpp == 16 || bpp == 32)) {
qemu_freeDisplaySurface(ds->surface);
ds->surface = qemu_createDisplaySurfaceFrom(width, height, bpp, linesize, data);
} else {
ds->surface = qemu_resizeDisplaySurface(ds->surface, width, height, 32, 4 * width);
}
dpy_resize(ds);
}
}of course this means changing every single call to qemu_console_resize in qemu because the number of arguments is changed.
Quoted 16 lines
void qemu_console_resize(DisplayState *ds, int width, int height, int bpp,
int linesize,uint8_t*data){ TextConsole *s = get_graphic_console(); s->g_width = width; s->g_height = height; if (is_graphic_console()) { if (data && (bpp == 16 || bpp == 32)) { qemu_freeDisplaySurface(ds->surface); ds->surface = qemu_createDisplaySurfaceFrom(width, height, bpp, linesize, data); } else { ds->surface = qemu_resizeDisplaySurface(ds->surface, width, height, 32, 4 * width); } dpy_resize(ds); } }
It feels wrong to be modifying the surface here. We already have to recreate the surface when we switch consoles, so why can't we use the same code for a resize?
Paul
Quoted 17 lines · Paul Brook
Quoted 16 lines
void qemu_console_resize(DisplayState *ds, int width, int height, int bpp,
int linesize,uint8_t*data){ TextConsole *s = get_graphic_console(); s->g_width = width; s->g_height = height; if (is_graphic_console()) { if (data && (bpp == 16 || bpp == 32)) { qemu_freeDisplaySurface(ds->surface); ds->surface = qemu_createDisplaySurfaceFrom(width, height, bpp, linesize, data); } else { ds->surface = qemu_resizeDisplaySurface(ds->surface, width, height, 32, 4 * width); } dpy_resize(ds); } }
Quoted 3 lines
It feels wrong to be modifying the surface here. We already have to recreate the surface when we switch consoles, so why can't we use the same code for a resize?
We use mostly the same code already.
The following are the implementations of console_select and qemu_console_resize in the last patch series I sent:
void console_select(unsigned int index)
{
TextConsole *s;
if (index >= MAX_CONSOLES)
return;
active_console->g_width = ds_get_width(active_console->ds);
active_console->g_height = ds_get_height(active_console->ds);
s = consoles[index];
if (s) {
DisplayState *ds = s->ds;
active_console = s;
ds->surface = qemu_resizeDisplaySurface(ds->surface, s->g_width,
s->g_height, 32, 4 * s->g_width);
dpy_resize(s->ds);
vga_hw_invalidate();
}
}void qemu_console_resize(QEMUConsole *console, int width, int height)
{
console->g_width = width;
console->g_height = height;
if (active_console == console) {
DisplayState *ds = console->ds;
ds->surface = qemu_resizeDisplaySurface(ds->surface, width, height, 32, 4 * width);
dpy_resize(console->ds);
}
}as you can see they both call qemu_resizeDisplaySurface and dpy_resize.
I was just saying that we could change qemu_console_resize to:
void qemu_console_resize(DisplayState *ds, int width, int height, int bpp,
int linesize, uint8_t *data)
in order to move the "artifact" away from vga.c, I am not sure if this is desiderable.
Quoted 18 lines · Stefano Stabellini
Quoted 17 lines · Paul Brook
Quoted 16 lines
void
qemu_console_resize(DisplayState *ds, int width, int height, int bpp, int linesize, uint8_t *data){ TextConsole *s = get_graphic_console(); s->g_width = width; s->g_height = height; if (is_graphic_console()) { if (data && (bpp == 16 || bpp == 32)) { qemu_freeDisplaySurface(ds->surface); ds->surface = qemu_createDisplaySurfaceFrom(width, height, bpp, linesize, data); } else { ds->surface = qemu_resizeDisplaySurface(ds->surface, width, height, 32, 4 * width); } dpy_resize(ds); } }
Quoted 5 lines
Quoted 3 lines
It feels wrong to be modifying the surface here. We already have to recreate the surface when we switch consoles, so why can't we use the same code for a resize?
We use mostly the same code already.
Why only mostly?
IIUC if a console is resized while active you get different behavior to a console that is resized when inactive, then activated. This is wrong.
Paul
Quoted 21 lines · Paul Brook
Quoted 20 lines · Stefano Stabellini
Quoted 19 lines · Paul Brook
Quoted 16 lines
void
qemu_console_resize(DisplayState *ds, int width, int height, int bpp, int linesize, uint8_t *data){ TextConsole *s = get_graphic_console(); s->g_width = width; s->g_height = height; if (is_graphic_console()) { if (data && (bpp == 16 || bpp == 32)) { qemu_freeDisplaySurface(ds->surface); ds->surface = qemu_createDisplaySurfaceFrom(width, height, bpp, linesize, data); } else { ds->surface = qemu_resizeDisplaySurface(ds->surface, width, height, 32, 4 * width); } dpy_resize(ds); } }It feels wrong to be modifying the surface here. We already have to recreate the surface when we switch consoles, so why can't we use the same code for a resize?
Quoted 7 lines
Quoted 1 line
We use mostly the same code already.
Why only mostly?
IIUC if a console is resized while active you get different behavior to a console that is resized when inactive, then activated. This is wrong.
Are we arguing about where to put two lines of code?
I can use the same function for both even if the code is already the same.
Quoted 26 lines · Stefano Stabellini
Quoted 25 lines · Paul Brook
Quoted 23 lines · Stefano Stabellini
Quoted 17 lines · Paul Brook
Quoted 16 lines
void
qemu_console_resize(DisplayState *ds, int width, int height, int bpp, int linesize, uint8_t *data){ TextConsole *s = get_graphic_console(); s->g_width = width; s->g_height = height; if (is_graphic_console()) { if (data && (bpp == 16 || bpp == 32)) { qemu_freeDisplaySurface(ds->surface); ds->surface = qemu_createDisplaySurfaceFrom(width, height, bpp, linesize, data); } else { ds->surface = qemu_resizeDisplaySurface(ds->surface, width, height, 32, 4 * width); } dpy_resize(ds); } }Quoted 3 lines
It feels wrong to be modifying the surface here. We already have to recreate the surface when we switch consoles, so why can't we use the same code for a resize?
We use mostly the same code already.
Quoted 7 lines
Quoted 4 lines
Why only mostly?
IIUC if a console is resized while active you get different behavior to a console that is resized when inactive, then activated. This is wrong.
Are we arguing about where to put two lines of code?
I can use the same function for both even if the code is already the same.
No. We're arguing about how surface creation is managed. As I pointed out the two implementations are no equivalent.
Paul
Quoted 15 lines · Anthony Liguori
Quoted 5 lines · Stefano Stabellini
Quoted 2 lines · Paul Brook
Quoted 2 lines
- do not use
is_active_console, useis_graphic_consoleinstead.Quoted 8 lines
Quoted 2 lines
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.
Quoted 8 lines
I need to look more closely at the patches (and I will tomorrow), but to me, the use of
is_active_consoleoris_graphics_consoleis a red herring. Nothing should never need to know whether it's the "active" console. Such a concept simply shouldn't exist.I think this is Paul's point about multiple graphics consoles. If you have two graphics consoles, than the concept of active console doesn't make very much sense. Is that what you were thinking Paul?
I can easily remove it, at the price of changing qemu_console_resize in:
void qemu_console_resize(DisplayState *ds, int width, int height, int bpp,
int linesize, uint8_t *data);
Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
Quoted 3 lines
This patch changes the
graphical_console_initfunction to return an allocated DisplayState instead of a QEMUConsole.You should also remove QEMUConsole.
Quoted 8 lines
Quoted 4 lines
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.
QEMUConsole is just a typedef right now. Could you remove the typedef? It should become unused after your patch.
I think I could remove it now, without even a single compile warning :)
Quoted 33 lines · Stefano Stabellini
Hi all,
this is the third update of the "DisplayState interface change" series.The series is now of made of 7 patches:
1) accessors again
some other substitutions inhw/sm501.c;2) remove bgr
the new DisplayState interface does not contain any host specific display detail, it is an abstraction of the backend display, hence we don't need to memorize the bgr flag in DisplayState.
The frontend must be able to handle a bgr display by itself, in fact sdl is perfectly capable of that;3) DisplayState interface change
this is the big patch that actually changes the interface;4) vnc improvements
this patch introduces DisplaySurfaces in vnc, simplifying the code.5)
graphical_console_initchange
this is the patch that changes thegraphical_console_initfunction to return an allocated DisplayState instead of a QEMUConsole, as Anthony suggested.
This patch does *not* include any required changes to any devices, these changes come with the two following patches.6) machine changes
this patch changes the QEMUMachine init functions of all the machine types not to take a DisplayState as an argument because is not needed any more;
Quoted 8 lines
7) graphic device changes
this patch updates the graphic device code to use the newgraphical_console_initfunction.Patch number 5 requires also 6 and 7 to compile.
More comments on single patches.
Except for the few comments I've made, I think this series looks good. Once you fix the problems I've pointed out, I'll do a thorough test run through each patch and unless there are major objections, push the series.
Regards,
Anthony Liguori
Quoted 9 lines
Cheers,
Stefano