Re: [PATCH v2 0/9] gfxstream + rutabaga_gfx

[Akihiko Odaki <akihiko.odaki@gmail.com>]

On 2023/08/01 10:17, Gurchetan Singh wrote:
> Latest iteration of rutabaga_gfx + gfxstream patches.  Previous versions
> available here:
> 
> https://patchew.org/QEMU/20230711025649.708-1-gurchetansingh@chromium.org/
> 
> https://patchew.org/QEMU/20230421011223.718-1-gurchetansingh@chromium.org/
> 
> Changes since v1:
> - New error callback hooked into QEMU error's handlers
> - Incorporated review feedback
> - goldfish-opengl repo is now gone: new unified repo for guest and host
> 
> How to build both rutabaga and gfxstream guest/host libs:
> 
> https://crosvm.dev/book/appendix/rutabaga_gfx.html

It's nice to see a documentation for this. The below are some comments:

Why not install dependencies in one command?
sudo apt install libdrm libglm-dev libstb-dev

The cmake command for AEMU is broken into two lines but the newline 
character between them are not escaped. It should be one line or the 
newline should be escaped.

It also gives a warning:
CMake Warning:
   Ignoring extra path from command line:

    "../"

And it's probably better to have a dedicated build directory. /build is 
in .gitignore so you may:
cmake -DAEMU_COMMON_GEN_PKGCONFIG=ON \
       -DAEMU_COMMON_BUILD_CONFIG=gfxstream \
       -DENABLE_VKCEREAL_TESTS=OFF -B build

It's also better to use the following commands to build and install it 
just in case the system has a different backend (like Ninja):
cmake --build build -j
sudo cmake --install build

The build directory of gfxstream may be named just "build" so that you 
can blindly copy the command on Arm64 or whatever and to ignore it with 
.gitignore.

You may build and install it with the following command:
meson install -C build
No need for separate build command and sudo. Meson takes care of that.

The documentation has one command for building and installing Rutabaga, 
but that may cause a problem. For example, you may have rustup 
configured for a normal user but not for the superuser. Perhaps it's 
better not to have "build" as a dependency of "install" and have two 
commands for each of the steps:
make
sudo make install

> 
> Branch containing this patch series:
> 
> https://gitlab.freedesktop.org/gurchetansingh/qemu-gfxstream/-/commits/qemu-gfxstream-v2
> 
> Next steps:
>   - Will add a v0.1.2 release "commit" after this patch series is fully
>     reviewed, but before it's merged
> 
> Antonio Caggiano (2):
>    virtio-gpu: CONTEXT_INIT feature
>    virtio-gpu: blob prep
> 
> Dr. David Alan Gilbert (1):
>    virtio: Add shared memory capability
> 
> Gerd Hoffmann (1):
>    virtio-gpu: hostmem
> 
> Gurchetan Singh (5):
>    gfxstream + rutabaga prep: added need defintions, fields, and options
>    gfxstream + rutabaga: add initial support for gfxstream
>    gfxstream + rutabaga: meson support
>    gfxstream + rutabaga: enable rutabaga
>    docs/system: add basic virtio-gpu documentation
> 
>   docs/system/device-emulation.rst     |    1 +
>   docs/system/devices/virtio-gpu.rst   |   98 +++
>   hw/display/meson.build               |   22 +
>   hw/display/virtio-gpu-base.c         |    6 +-
>   hw/display/virtio-gpu-pci-rutabaga.c |   48 ++
>   hw/display/virtio-gpu-pci.c          |   14 +
>   hw/display/virtio-gpu-rutabaga.c     | 1077 ++++++++++++++++++++++++++
>   hw/display/virtio-gpu.c              |   17 +-
>   hw/display/virtio-vga-rutabaga.c     |   52 ++
>   hw/display/virtio-vga.c              |   33 +-
>   hw/virtio/virtio-pci.c               |   18 +
>   include/hw/virtio/virtio-gpu-bswap.h |   18 +
>   include/hw/virtio/virtio-gpu.h       |   41 +
>   include/hw/virtio/virtio-pci.h       |    4 +
>   meson.build                          |    7 +
>   meson_options.txt                    |    2 +
>   scripts/meson-buildoptions.sh        |    3 +
>   softmmu/qdev-monitor.c               |    3 +
>   softmmu/vl.c                         |    1 +
>   19 files changed, 1445 insertions(+), 20 deletions(-)
>   create mode 100644 docs/system/devices/virtio-gpu.rst
>   create mode 100644 hw/display/virtio-gpu-pci-rutabaga.c
>   create mode 100644 hw/display/virtio-gpu-rutabaga.c
>   create mode 100644 hw/display/virtio-vga-rutabaga.c
>