Re: [Qemu-devel] [PATCH 3/3] ramfb enhancement

[Marcel Apfelbaum]

On 5/9/19 10:58 AM, Hou Qiming wrote:
> Write an initial resolution to the configuration space on guest reset,
> which a later BIOS / OVMF patch can take advantage of.

I like the idea of moving the ramfb configuration to the PCI 
configuration space,
do you think is possible to move all the ramfb configuration there so we 
will
not need the fw-config file?
()

> Signed-off-by: HOU Qiming <address@hidden 
> <mailto:address@hidden>>
> ---
>  hw/display/ramfb-standalone.c | 12 +++++++++++-
>  hw/display/ramfb.c            | 16 +++++++++++++++-
>  hw/vfio/display.c             |  4 ++--
>  hw/vfio/pci.c                 |  6 ++++--
>  include/hw/display/ramfb.h    |  2 +-
>  stubs/ramfb.c                 |  2 +-
>  6 files changed, 34 insertions(+), 8 deletions(-)
>
> diff --git a/hw/display/ramfb-standalone.c b/hw/display/ramfb-standalone.c
> index da3229a..6441449 100644
> --- a/hw/display/ramfb-standalone.c
> +++ b/hw/display/ramfb-standalone.c
> @@ -1,6 +1,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "hw/loader.h"
> +#include "hw/isa/isa.h"
>  #include "hw/display/ramfb.h"
>  #include "ui/console.h"
>  #include "sysemu/sysemu.h"
> @@ -11,6 +12,8 @@ typedef struct RAMFBStandaloneState {
>      SysBusDevice parent_obj;
>      QemuConsole *con;
>      RAMFBState *state;
> +    uint32_t xres;
> +    uint32_t yres;
>  } RAMFBStandaloneState;
>
>  static void display_update_wrapper(void *dev)
> @@ -33,15 +36,22 @@ static void ramfb_realizefn(DeviceState *dev, 
> Error **errp)
>      RAMFBStandaloneState *ramfb = RAMFB(dev);
>
>      ramfb->con = graphic_console_init(dev, 0, &wrapper_ops, dev);
> -    ramfb->state = ramfb_setup(errp);
> +    ramfb->state = ramfb_setup(dev, errp);
>  }
>
> +static Property ramfb_properties[] = {
> +    DEFINE_PROP_UINT32("xres", RAMFBStandaloneState, xres, 0),
> +    DEFINE_PROP_UINT32("yres", RAMFBStandaloneState, yres, 0),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
>  static void ramfb_class_initfn(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>
>      set_bit(DEVICE_CATEGORY_DISPLAY, dc->categories);
>      dc->realize = ramfb_realizefn;
> +    dc->props = ramfb_properties;
>      dc->desc = "ram framebuffer standalone device";
>      dc->user_creatable = true;
>  }
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index fa6296b..0033ac8 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -12,6 +12,7 @@
>   */
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
> +#include "qemu/option.h"
>  #include "hw/loader.h"
>  #include "hw/display/ramfb.h"
>  #include "ui/console.h"
> @@ -29,6 +30,7 @@ struct QEMU_PACKED RAMFBCfg {
>  struct RAMFBState {
>      DisplaySurface *ds;
>      uint32_t width, height;
> +    uint32_t starting_width, starting_height;
>      hwaddr addr, length;
>      struct RAMFBCfg cfg;
>      bool locked;
> @@ -120,9 +122,11 @@ static void ramfb_reset(void *opaque)
>      RAMFBState *s = (RAMFBState *)opaque;
>      s->locked = false;
>      memset(&s->cfg, 0, sizeof(s->cfg));
> +    s->cfg.width = s->starting_width;
> +    s->cfg.height = s->starting_height;
>  }
>
> -RAMFBState *ramfb_setup(Error **errp)
> +RAMFBState *ramfb_setup(DeviceState* dev, Error **errp)
>  {
>      FWCfgState *fw_cfg = fw_cfg_find();
>      RAMFBState *s;
> @@ -134,6 +138,16 @@ RAMFBState *ramfb_setup(Error **errp)
>
>      s = g_new0(RAMFBState, 1);
>
> +    const char *s_fb_width = qemu_opt_get(dev->opts, "xres");
> +    const char *s_fb_height = qemu_opt_get(dev->opts, "yres");
> +    if (s_fb_width) {
> +        s->cfg.width = atoi(s_fb_width);
> +        s->starting_width = s->cfg.width;
> +    }
> +    if (s_fb_height) {
> +        s->cfg.height = atoi(s_fb_height);
> +        s->starting_height = s->cfg.height;
> +    }
>      s->locked = false;
>
>      rom_add_vga("vgabios-ramfb.bin");
> diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> index a3d9c8f..2c2d3e5 100644
> --- a/hw/vfio/display.c
> +++ b/hw/vfio/display.c
> @@ -352,7 +352,7 @@ static int vfio_display_dmabuf_init(VFIOPCIDevice 
> *vdev, Error **errp)
> &vfio_display_dmabuf_ops,
>                                            vdev);
>      if (vdev->enable_ramfb) {
> -        vdev->dpy->ramfb = ramfb_setup(errp);
> +        vdev->dpy->ramfb = ramfb_setup(DEVICE(vdev), errp);
>      }
>      vfio_display_edid_init(vdev);
>      return 0;
> @@ -478,7 +478,7 @@ static int vfio_display_region_init(VFIOPCIDevice 
> *vdev, Error **errp)
> &vfio_display_region_ops,
>                                            vdev);
>      if (vdev->enable_ramfb) {
> -        vdev->dpy->ramfb = ramfb_setup(errp);
> +        vdev->dpy->ramfb = ramfb_setup(DEVICE(vdev), errp);
>      }
>      return 0;
>  }
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 8cecb53..5d64daa 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3080,8 +3080,10 @@ static void vfio_realize(PCIDevice *pdev, Error 
> **errp)
>              error_setg(errp, "xres and yres properties require 
> display=on");
>              goto out_teardown;
>          }
> -        if (vdev->dpy->edid_regs == NULL) {
> -            error_setg(errp, "xres and yres properties need edid 
> support");
> +        if (vdev->dpy->edid_regs == NULL && !vdev->enable_ramfb) {
> +            error_setg(errp,
> +                       "xres and yres properties need edid support"
> +                       " or ramfb=on");

Is this chunk related to this patch? If not, you may want to split it.

Thanks,
Marcel

>              goto out_teardown;
>          }
>      }
> diff --git a/include/hw/display/ramfb.h b/include/hw/display/ramfb.h
> index b33a2c4..f6c2de9 100644
> --- a/include/hw/display/ramfb.h
> +++ b/include/hw/display/ramfb.h
> @@ -4,7 +4,7 @@
>  /* ramfb.c */
>  typedef struct RAMFBState RAMFBState;
>  void ramfb_display_update(QemuConsole *con, RAMFBState *s);
> -RAMFBState *ramfb_setup(Error **errp);
> +RAMFBState *ramfb_setup(DeviceState *dev, Error **errp);
>
>  /* ramfb-standalone.c */
>  #define TYPE_RAMFB_DEVICE "ramfb"
> diff --git a/stubs/ramfb.c b/stubs/ramfb.c
> index 48143f3..0799093 100644
> --- a/stubs/ramfb.c
> +++ b/stubs/ramfb.c
> @@ -6,7 +6,7 @@ void ramfb_display_update(QemuConsole *con, RAMFBState *s)
>  {
>  }
>
> -RAMFBState *ramfb_setup(Error **errp)
> +RAMFBState *ramfb_setup(DeviceState* dev, Error **errp)
>  {
>      error_setg(errp, "ramfb support not available");
>      return NULL;
> --
> 2.17.1