Re: [Qemu-devel] [PATCH 2/2] fbdev: add monitor commands to enable/disable/query

[Luiz Capitulino <lcapitulino@redhat.com>]

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