| Message ID | 1372246684-12499-2-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 26 Jun 2013 13:38:04 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > This patch adds a fbdev monitor command to enable/disable > the fbdev display at runtime to both qmp and hmp. > > qmp: framebuffer-display enable=on|off scale=on|off device=/dev/fb<n> > hmp: framebuffer-display on|off > > There is also a query-framebuffer command for qmp. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > hmp-commands.hx | 14 ++++++++++++++ > hmp.c | 9 +++++++++ > hmp.h | 1 + > include/ui/console.h | 1 + > qapi-schema.json | 43 +++++++++++++++++++++++++++++++++++++++++++ > qmp-commands.hx | 12 ++++++++++++ > qmp.c | 31 +++++++++++++++++++++++++++++++ > ui/fbdev.c | 20 ++++++++++++++++++++ > 8 files changed, 131 insertions(+) > > diff --git a/hmp-commands.hx b/hmp-commands.hx > index 915b0d1..283106d 100644 > --- a/hmp-commands.hx > +++ b/hmp-commands.hx > @@ -1563,7 +1563,21 @@ STEXI > @findex qemu-io > > Executes a qemu-io command on the given block device. > +ETEXI > + > + { > + .name = "framebuffer-display", > + .args_type = "enable:b", > + .params = "on|off", > + .help = "enable/disable linux console framebuffer display", > + .mhandler.cmd = hmp_framebuffer_display, > + }, > + > +STEXI > +@item framebuffer-display on | off > +@findex framebuffer-display > > +enable/disable linux console framebuffer display. > ETEXI > > { > diff --git a/hmp.c b/hmp.c > index 494a9aa..55f195f 100644 > --- a/hmp.c > +++ b/hmp.c > @@ -1464,3 +1464,12 @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict) > > hmp_handle_error(mon, &err); > } > + > +void hmp_framebuffer_display(Monitor *mon, const QDict *qdict) > +{ > + int enable = qdict_get_bool(qdict, "enable"); > + Error *errp = NULL; > + > + qmp_framebuffer_display(enable, false, false, false, NULL, &errp); > + hmp_handle_error(mon, &errp); > +} > diff --git a/hmp.h b/hmp.h > index 56d2e92..c3a48e4 100644 > --- a/hmp.h > +++ b/hmp.h > @@ -86,5 +86,6 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict); > void hmp_chardev_add(Monitor *mon, const QDict *qdict); > void hmp_chardev_remove(Monitor *mon, const QDict *qdict); > void hmp_qemu_io(Monitor *mon, const QDict *qdict); > +void hmp_framebuffer_display(Monitor *mon, const QDict *qdict); > > #endif > diff --git a/include/ui/console.h b/include/ui/console.h > index 71b538a..5a9207d 100644 > --- a/include/ui/console.h > +++ b/include/ui/console.h > @@ -311,6 +311,7 @@ void sdl_display_init(DisplayState *ds, int full_screen, int no_frame); > /* fbdev.c */ > int fbdev_display_init(const char *device, bool scale, Error **err); > void fbdev_display_uninit(void); > +FramebufferInfo *framebuffer_info(void); > > /* cocoa.m */ > void cocoa_display_init(DisplayState *ds, int full_screen); > diff --git a/qapi-schema.json b/qapi-schema.json > index 6cc07c2..715dc1f 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -3608,3 +3608,46 @@ > '*cpuid-input-ecx': 'int', > 'cpuid-register': 'X86CPURegister32', > 'features': 'int' } } > + > +## > +# @framebuffer-display: Let me bike-shed: we're trying to make command's names verbs. So, we could call this framebuffer-display-set or maybe have two commands, framebuffer-display-enable and framebuffer-display-disable. I prefer the latter. > +# > +# Enable/disable linux console framebuffer display. > +# > +# @enable: whenever the framebuffer display should be enabled or disabled. > +# > +# @scale: #optional enables display scaling, default: off > +# > +# @device: #optional specifies framebuffer device, default: /dev/fb0 Actually, it will try to get the device name from an env variable first, which sounds too automatic for a building block API like QMP. You can do it for HMP, but QMP I guess it would be more appropriate to make the device name mandatory. > +# > +# Returns: Nothing. > +# > +# Since: 1.6 > +# > +## > +{ 'command': 'framebuffer-display', 'data': {'enable' : 'bool', > + '*scale' : 'bool', > + '*device' : 'str' } } > + > +## > +# @FramebufferInfo: > +# Missing docs. > +# Since 1.6 > +## > +{ 'type': 'FramebufferInfo', > + 'data': { 'enabled': 'bool', > + '*scale' : 'bool', > + '*device': 'str', Why is device optional? > + '*vtno' : 'int' } } > + > +## > +# @query-framebuffer: > +# > +# Query linux console framebuffer state. > +# > +# Returns: FramebufferInfo. > +# > +# Since: 1.6 > +# > +## > +{ 'command': 'query-framebuffer', 'returns': 'FramebufferInfo' } > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 8cea5e5..e0661f0 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -2997,3 +2997,15 @@ Example: > <- { "return": {} } > > EQMP > + > + { > + .name = "framebuffer-display", > + .args_type = "enable:b,scale:b?,device:s?", > + .mhandler.cmd_new = qmp_marshal_input_framebuffer_display, > + }, > + > + { > + .name = "query-framebuffer", > + .args_type = "", > + .mhandler.cmd_new = qmp_marshal_input_query_framebuffer, > + }, > diff --git a/qmp.c b/qmp.c > index 4c149b3..b6d826c 100644 > --- a/qmp.c > +++ b/qmp.c > @@ -404,6 +404,37 @@ void qmp_change(const char *device, const char *target, > } > } > > +void qmp_framebuffer_display(bool enable, > + bool has_scale, bool scale, > + bool has_device, const char *device, > + Error **errp) > +{ > +#if defined(CONFIG_FBDEV) > + if (enable) { > + if (fbdev_display_init(has_device ? device : NULL, > + has_scale ? scale : false, > + errp) != 0) { > + if (!error_is_set(errp)) { > + error_setg(errp, "fbdev initialization failed"); You should use error_propagate() in order to propagate the error information filled by fbdev_display_init(). Also, I'd move the generic error_setg() above to fbdev_display_init() and make it return void. > + } > + } > + } else { > + fbdev_display_uninit(); > + } > +#else > + error_setg(errp, "fbdev support disabled at compile time"); > +#endif > +} > + > +FramebufferInfo *qmp_query_framebuffer(Error **errp) > +{ > +#if defined(CONFIG_FBDEV) > + return framebuffer_info(); > +#else > + error_setg(errp, "fbdev support disabled at compile time"); > +#endif > +} > + > static void qom_list_types_tramp(ObjectClass *klass, void *data) > { > ObjectTypeInfoList *e, **pret = data; > diff --git a/ui/fbdev.c b/ui/fbdev.c > index 91e11e5..326bba1 100644 > --- a/ui/fbdev.c > +++ b/ui/fbdev.c > @@ -1162,3 +1162,23 @@ void fbdev_display_uninit(void) > g_free(s); > fb = NULL; > } > + > +FramebufferInfo *framebuffer_info(void) > +{ > + FramebufferInfo *info = g_new0(FramebufferInfo, 1); > + FBDevState *s = fb; > + > + if (s == NULL) { > + info->enabled = false; > + return info; > + } > + > + info->enabled = true; > + info->has_device = true; > + info->device = g_strdup(s->device); > + info->has_scale = true; > + info->scale = s->use_scale; > + info->has_vtno = true; > + info->vtno = s->vtno; > + return info; > +}
Hi, >> --- a/qapi-schema.json >> +++ b/qapi-schema.json >> @@ -3608,3 +3608,46 @@ >> '*cpuid-input-ecx': 'int', >> 'cpuid-register': 'X86CPURegister32', >> 'features': 'int' } } >> + >> +## >> +# @framebuffer-display: > > Let me bike-shed: we're trying to make command's names verbs. So, we > could call this framebuffer-display-set or maybe have two commands, > framebuffer-display-enable and framebuffer-display-disable. I prefer > the latter. Can do that. >> +# >> +# Enable/disable linux console framebuffer display. >> +# >> +# @enable: whenever the framebuffer display should be enabled or disabled. >> +# >> +# @scale: #optional enables display scaling, default: off >> +# >> +# @device: #optional specifies framebuffer device, default: /dev/fb0 > > Actually, it will try to get the device name from an env variable first, > which sounds too automatic for a building block API like QMP. The env variable is icky indeed. But a fixed default (which will be the one you need in 99% of the cases) is fine IMO. >> +# Since 1.6 >> +## >> +{ 'type': 'FramebufferInfo', >> + 'data': { 'enabled': 'bool', >> + '*scale' : 'bool', >> + '*device': 'str', > > Why is device optional? When the framebuffer is'nt active you'll get just { "enabled" : "false" }, thats why the other ones are optional. They all are filled in case the framebuffer is active. >> +void qmp_framebuffer_display(bool enable, >> + bool has_scale, bool scale, >> + bool has_device, const char *device, >> + Error **errp) >> +{ >> +#if defined(CONFIG_FBDEV) >> + if (enable) { >> + if (fbdev_display_init(has_device ? device : NULL, >> + has_scale ? scale : false, >> + errp) != 0) { >> + if (!error_is_set(errp)) { >> + error_setg(errp, "fbdev initialization failed"); > > You should use error_propagate() in order to propagate the error > information filled by fbdev_display_init(). Why? What is wrong with simply passing down errp? > Also, I'd move > the generic error_setg() above to fbdev_display_init() and make it > return void. Makes sense indeed. cheers, Gerd
On Thu, 27 Jun 2013 12:05:33 +0200 Gerd Hoffmann <kraxel@redhat.com> wrote: > Hi, > > >> --- a/qapi-schema.json > >> +++ b/qapi-schema.json > >> @@ -3608,3 +3608,46 @@ > >> '*cpuid-input-ecx': 'int', > >> 'cpuid-register': 'X86CPURegister32', > >> 'features': 'int' } } > >> + > >> +## > >> +# @framebuffer-display: > > > > Let me bike-shed: we're trying to make command's names verbs. So, we > > could call this framebuffer-display-set or maybe have two commands, > > framebuffer-display-enable and framebuffer-display-disable. I prefer > > the latter. > > Can do that. > > >> +# > >> +# Enable/disable linux console framebuffer display. > >> +# > >> +# @enable: whenever the framebuffer display should be enabled or disabled. > >> +# > >> +# @scale: #optional enables display scaling, default: off > >> +# > >> +# @device: #optional specifies framebuffer device, default: /dev/fb0 > > > > Actually, it will try to get the device name from an env variable first, > > which sounds too automatic for a building block API like QMP. > > The env variable is icky indeed. But a fixed default (which will be the > one you need in 99% of the cases) is fine IMO. I prefer having no defaults, but I'm not strong about this. > > >> +# Since 1.6 > >> +## > >> +{ 'type': 'FramebufferInfo', > >> + 'data': { 'enabled': 'bool', > >> + '*scale' : 'bool', > >> + '*device': 'str', > > > > Why is device optional? > > When the framebuffer is'nt active you'll get just > { "enabled" : "false" }, thats why the other ones are optional. They > all are filled in case the framebuffer is active. > > >> +void qmp_framebuffer_display(bool enable, > >> + bool has_scale, bool scale, > >> + bool has_device, const char *device, > >> + Error **errp) > >> +{ > >> +#if defined(CONFIG_FBDEV) > >> + if (enable) { > >> + if (fbdev_display_init(has_device ? device : NULL, > >> + has_scale ? scale : false, > >> + errp) != 0) { > >> + if (!error_is_set(errp)) { > >> + error_setg(errp, "fbdev initialization failed"); > > > > You should use error_propagate() in order to propagate the error > > information filled by fbdev_display_init(). > > Why? What is wrong with simply passing down errp? Because errp can be NULL, then you'll be unable to detect errors if fbdev_display_init() is changed to return void. But you're right you won't need to propagate errors if you drop the if () test, as you won't be checking for errors anymore. > > > Also, I'd move > > the generic error_setg() above to fbdev_display_init() and make it > > return void. > > Makes sense indeed. > > cheers, > Gerd >
On 06/26/2013 09:56 AM, Luiz Capitulino wrote: > On Wed, 26 Jun 2013 13:38:04 +0200 > Gerd Hoffmann <kraxel@redhat.com> wrote: > >> This patch adds a fbdev monitor command to enable/disable >> the fbdev display at runtime to both qmp and hmp. >> >> +## >> +# @framebuffer-display: > > Let me bike-shed: we're trying to make command's names verbs. So, we > could call this framebuffer-display-set or maybe have two commands, > framebuffer-display-enable and framebuffer-display-disable. I prefer > the latter. Having 2 commands also avoids the semantic quandary of what to do for "enable":false,"device":"/path/to/non-default" - the device parameter only makes sense when enabling the framebuffer display. >> + >> +## >> +# @FramebufferInfo: >> +# > > Missing docs. > >> +# Since 1.6 >> +## >> +{ 'type': 'FramebufferInfo', >> + 'data': { 'enabled': 'bool', >> + '*scale' : 'bool', >> + '*device': 'str', > > Why is device optional? > >> + '*vtno' : 'int' } } Also, 'vtno' isn't a word; is it worth spelling it out a bit more by naming it 'vt-number'?
On 06/27/2013 07:29 AM, Luiz Capitulino wrote: > On Thu, 27 Jun 2013 12:05:33 +0200 > Gerd Hoffmann <kraxel@redhat.com> wrote: > >>>> +# @device: #optional specifies framebuffer device, default: /dev/fb0 >>> >>> Actually, it will try to get the device name from an env variable first, >>> which sounds too automatic for a building block API like QMP. >> >> The env variable is icky indeed. But a fixed default (which will be the >> one you need in 99% of the cases) is fine IMO. > > I prefer having no defaults, but I'm not strong about this. I also think that letting HMP leave the parameter optional, with first-level fallback to the environment variable and secondary fallback to the hardcoded /dev/fv0, while making the parameter mandatory in QMP, makes more sense. > >> >>>> +# Since 1.6 >>>> +## >>>> +{ 'type': 'FramebufferInfo', >>>> + 'data': { 'enabled': 'bool', >>>> + '*scale' : 'bool', >>>> + '*device': 'str', >>> >>> Why is device optional? >> >> When the framebuffer is'nt active you'll get just >> { "enabled" : "false" }, thats why the other ones are optional. They >> all are filled in case the framebuffer is active. Would it be any simpler to just have the query command fail when the framebuffer is disabled, leave out the 'enabled' member from FramebufferInfo, and have all the other members mandatory? Also, would it ever make sense for a future enhancement to allow having more than one framebuffer in use by a single domain? Even if your initial implementation supports at most one /dev/fb* device per guest, it might be worth having the query-framebuffer command return an array of all enabled framebuffers (a 0-length array when none is enabled), rather than hard-coding it to a single FramebufferInfo return. Likewise, make sure that framebuffer-display can deal with extension if it makes sense (the suggestion to split into -enable and -disable commands might be better stated as splitting into framebuffer-display-add and framebuffer-display-delete, more like hotplugging of other devices).
On Thu, 27 Jun 2013 16:03:48 -0600 Eric Blake <eblake@redhat.com> wrote: > On 06/27/2013 07:29 AM, Luiz Capitulino wrote: > > On Thu, 27 Jun 2013 12:05:33 +0200 > > Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > >>>> +# @device: #optional specifies framebuffer device, default: /dev/fb0 > >>> > >>> Actually, it will try to get the device name from an env variable first, > >>> which sounds too automatic for a building block API like QMP. > >> > >> The env variable is icky indeed. But a fixed default (which will be the > >> one you need in 99% of the cases) is fine IMO. > > > > I prefer having no defaults, but I'm not strong about this. > > I also think that letting HMP leave the parameter optional, with > first-level fallback to the environment variable and secondary fallback > to the hardcoded /dev/fv0, while making the parameter mandatory in QMP, > makes more sense. > > > > >> > >>>> +# Since 1.6 > >>>> +## > >>>> +{ 'type': 'FramebufferInfo', > >>>> + 'data': { 'enabled': 'bool', > >>>> + '*scale' : 'bool', > >>>> + '*device': 'str', > >>> > >>> Why is device optional? > >> > >> When the framebuffer is'nt active you'll get just > >> { "enabled" : "false" }, thats why the other ones are optional. They > >> all are filled in case the framebuffer is active. > > Would it be any simpler to just have the query command fail when the > framebuffer is disabled, leave out the 'enabled' member from > FramebufferInfo, and have all the other members mandatory? I don't think this is correct. First because mngt would have to parse the error message to learn if the query command really failed or the framebuffer device is just disabled. Second because the command does succeed on returning the framebuffer status even when it's disabled. > Also, would it ever make sense for a future enhancement to allow having > more than one framebuffer in use by a single domain? Even if your > initial implementation supports at most one /dev/fb* device per guest, > it might be worth having the query-framebuffer command return an array > of all enabled framebuffers (a 0-length array when none is enabled), This is also an argument in favor of not having a default device name in QMP.
diff --git a/hmp-commands.hx b/hmp-commands.hx index 915b0d1..283106d 100644 --- a/hmp-commands.hx +++ b/hmp-commands.hx @@ -1563,7 +1563,21 @@ STEXI @findex qemu-io Executes a qemu-io command on the given block device. +ETEXI + + { + .name = "framebuffer-display", + .args_type = "enable:b", + .params = "on|off", + .help = "enable/disable linux console framebuffer display", + .mhandler.cmd = hmp_framebuffer_display, + }, + +STEXI +@item framebuffer-display on | off +@findex framebuffer-display +enable/disable linux console framebuffer display. ETEXI { diff --git a/hmp.c b/hmp.c index 494a9aa..55f195f 100644 --- a/hmp.c +++ b/hmp.c @@ -1464,3 +1464,12 @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict) hmp_handle_error(mon, &err); } + +void hmp_framebuffer_display(Monitor *mon, const QDict *qdict) +{ + int enable = qdict_get_bool(qdict, "enable"); + Error *errp = NULL; + + qmp_framebuffer_display(enable, false, false, false, NULL, &errp); + hmp_handle_error(mon, &errp); +} diff --git a/hmp.h b/hmp.h index 56d2e92..c3a48e4 100644 --- a/hmp.h +++ b/hmp.h @@ -86,5 +86,6 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict); void hmp_chardev_add(Monitor *mon, const QDict *qdict); void hmp_chardev_remove(Monitor *mon, const QDict *qdict); void hmp_qemu_io(Monitor *mon, const QDict *qdict); +void hmp_framebuffer_display(Monitor *mon, const QDict *qdict); #endif diff --git a/include/ui/console.h b/include/ui/console.h index 71b538a..5a9207d 100644 --- a/include/ui/console.h +++ b/include/ui/console.h @@ -311,6 +311,7 @@ void sdl_display_init(DisplayState *ds, int full_screen, int no_frame); /* fbdev.c */ int fbdev_display_init(const char *device, bool scale, Error **err); void fbdev_display_uninit(void); +FramebufferInfo *framebuffer_info(void); /* cocoa.m */ void cocoa_display_init(DisplayState *ds, int full_screen); diff --git a/qapi-schema.json b/qapi-schema.json index 6cc07c2..715dc1f 100644 --- a/qapi-schema.json +++ b/qapi-schema.json @@ -3608,3 +3608,46 @@ '*cpuid-input-ecx': 'int', 'cpuid-register': 'X86CPURegister32', 'features': 'int' } } + +## +# @framebuffer-display: +# +# Enable/disable linux console framebuffer display. +# +# @enable: whenever the framebuffer display should be enabled or disabled. +# +# @scale: #optional enables display scaling, default: off +# +# @device: #optional specifies framebuffer device, default: /dev/fb0 +# +# Returns: Nothing. +# +# Since: 1.6 +# +## +{ 'command': 'framebuffer-display', 'data': {'enable' : 'bool', + '*scale' : 'bool', + '*device' : 'str' } } + +## +# @FramebufferInfo: +# +# Since 1.6 +## +{ 'type': 'FramebufferInfo', + 'data': { 'enabled': 'bool', + '*scale' : 'bool', + '*device': 'str', + '*vtno' : 'int' } } + +## +# @query-framebuffer: +# +# Query linux console framebuffer state. +# +# Returns: FramebufferInfo. +# +# Since: 1.6 +# +## +{ 'command': 'query-framebuffer', 'returns': 'FramebufferInfo' } diff --git a/qmp-commands.hx b/qmp-commands.hx index 8cea5e5..e0661f0 100644 --- a/qmp-commands.hx +++ b/qmp-commands.hx @@ -2997,3 +2997,15 @@ Example: <- { "return": {} } EQMP + + { + .name = "framebuffer-display", + .args_type = "enable:b,scale:b?,device:s?", + .mhandler.cmd_new = qmp_marshal_input_framebuffer_display, + }, + + { + .name = "query-framebuffer", + .args_type = "", + .mhandler.cmd_new = qmp_marshal_input_query_framebuffer, + }, diff --git a/qmp.c b/qmp.c index 4c149b3..b6d826c 100644 --- a/qmp.c +++ b/qmp.c @@ -404,6 +404,37 @@ void qmp_change(const char *device, const char *target, } } +void qmp_framebuffer_display(bool enable, + bool has_scale, bool scale, + bool has_device, const char *device, + Error **errp) +{ +#if defined(CONFIG_FBDEV) + if (enable) { + if (fbdev_display_init(has_device ? device : NULL, + has_scale ? scale : false, + errp) != 0) { + if (!error_is_set(errp)) { + error_setg(errp, "fbdev initialization failed"); + } + } + } else { + fbdev_display_uninit(); + } +#else + error_setg(errp, "fbdev support disabled at compile time"); +#endif +} + +FramebufferInfo *qmp_query_framebuffer(Error **errp) +{ +#if defined(CONFIG_FBDEV) + return framebuffer_info(); +#else + error_setg(errp, "fbdev support disabled at compile time"); +#endif +} + static void qom_list_types_tramp(ObjectClass *klass, void *data) { ObjectTypeInfoList *e, **pret = data; diff --git a/ui/fbdev.c b/ui/fbdev.c index 91e11e5..326bba1 100644 --- a/ui/fbdev.c +++ b/ui/fbdev.c @@ -1162,3 +1162,23 @@ void fbdev_display_uninit(void) g_free(s); fb = NULL; } + +FramebufferInfo *framebuffer_info(void) +{ + FramebufferInfo *info = g_new0(FramebufferInfo, 1); + FBDevState *s = fb; + + if (s == NULL) { + info->enabled = false; + return info; + } + + info->enabled = true; + info->has_device = true; + info->device = g_strdup(s->device); + info->has_scale = true; + info->scale = s->use_scale; + info->has_vtno = true; + info->vtno = s->vtno; + return info; +}
This patch adds a fbdev monitor command to enable/disable the fbdev display at runtime to both qmp and hmp. qmp: framebuffer-display enable=on|off scale=on|off device=/dev/fb<n> hmp: framebuffer-display on|off There is also a query-framebuffer command for qmp. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hmp-commands.hx | 14 ++++++++++++++ hmp.c | 9 +++++++++ hmp.h | 1 + include/ui/console.h | 1 + qapi-schema.json | 43 +++++++++++++++++++++++++++++++++++++++++++ qmp-commands.hx | 12 ++++++++++++ qmp.c | 31 +++++++++++++++++++++++++++++++ ui/fbdev.c | 20 ++++++++++++++++++++ 8 files changed, 131 insertions(+)