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

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

[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 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

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

[Paul Brook]

> 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.

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.

> - do not use is_active_console, use is_graphic_console instead.

This is wrong. There may be multiple graphic consoles.

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

You should also remove QEMUConsole.

> +    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

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.

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

[Anthony Liguori]

Stefano Stabellini wrote:
> Paul Brook wrote:
>
>   
>
>>> - 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.
>   

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?

>>> 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.
>   

QEMUConsole is just a typedef right now.  Could you remove the typedef?  
It should become unused after your patch.

Regards,

Anthony Liguori

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

[Paul Brook]

> >>> - 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.
>
> 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?

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

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

[Stefano Stabellini]

Anthony Liguori wrote:

> Stefano Stabellini wrote:
>> Paul Brook wrote:
>>
>>  
>>>> - 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.
>>   
> 
> 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?


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);

>>>> 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.
>>   
> 
> 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 :)

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

[Anthony Liguori]

Stefano Stabellini wrote:
> 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.
>   

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

> Cheers,
>
> Stefano
>
>
>
>
>
>   

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

[Anthony Liguori]

Paul Brook wrote:
>
> 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.
>   

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

> Paul
>   

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

[Stefano Stabellini]

Anthony Liguori wrote:

> 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 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.

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

[Paul Brook]

> 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

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

[Stefano Stabellini]

Paul Brook wrote:

>> 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?



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.

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

[Paul Brook]

On Thursday 11 December 2008, Stefano Stabellini wrote:
> Paul Brook wrote:
> >> 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?
>
> 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

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

[Stefano Stabellini]

Paul Brook wrote:

> On Thursday 11 December 2008, Stefano Stabellini wrote:
>> Paul Brook wrote:
>>>> 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?
>> 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.

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

[Paul Brook]

On Thursday 11 December 2008, Stefano Stabellini wrote:
> Paul Brook wrote:
> > On Thursday 11 December 2008, Stefano Stabellini wrote:
> >> Paul Brook wrote:
> >>>> 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?
> >>
> >> 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.

No. We're arguing about how surface creation is managed. As I pointed out the 
two implementations are no equivalent.

Paul