Skip to content
Snippets Groups Projects

vulkan renderer definitions and public headers

    • Closed Issue created by Chia-I Wu @olv

    The virgl stack has a good number of public header files that are defined and consumed by different projects:

    Untitled_drawing__1_

    Kernel/VMM or vtest in the diagram is what I call transport mechanisms. They don't parse most of the traffic between Mesa and virglrenderer.

    Examples of traffic that the transport mechanisms don't parse are

    • resource parameters (virgl_formats, VIRGL_BIND_* in virgl_hw.h)
    • virgl capsets (virgl_capsets and virgl_caps_v* in virgl_hw.h)
    • virgl protocol (VIRGL_CCMD_* in virgl_protocol.h)

    Examples of traffic that the transport mechanisms do parse are

    • blob resource types
    • blob resource flags

    These are new and are parsed because the transport mechanisms need to allocate differently and set up mappings for example.

    For vulkan renderer, I need to add some new definitions

    • context types (virgl or venus)
    • venus capset
    • venus protocol

    The least intrusive change is to

    • add context types to virgl_hw.h because they are things the transport mechanisms don't parse
    • add venus capset to virgl_hw.h where virgl capsets are defined
    • add venus protocol to a new protocol header

    But we could also use the chance to clean up virgl_hw.h such that Vulkan bits do not end up in virgl_hw.h. Thoughts?

    @gurchetansingh

    To upload designs, you'll need to enable LFS and have an admin enable hashed storage. More information

    Child items ...

    • Activity

    • Gurchetan Singh
      Gurchetan Singh @gurchetansingh ·
      Developer

      It would be nice not to have a dependency on virgl_hw.h, if we're shooting for a Zink or Vkangle + VK future.

      In terms of context types, the capsets are definitely related.  We can definitely have virgl_renderer_create_context_from_capset_id rather than virgl_renderer_create_context_from_type.  The only reason we have two capsets for virgl is due a kernel bug that's been fixed.

      We already have VIRTIO_GPU_CAPSET_VIRGL2, VIRTIO_GPU_CAPSET_VIRGL in virtio_gpu.h / QEMU.  So probably we're probably going to add VIRTIO_GPU_CAPSET_VENUS.  

      Should we add VIRTGPU_CAPSET_VIRGL2/VIRTGPU_CAPSET_VIRGL2/VIRTGPU_CAPSET_VIRGL2 too in virtgpu_drm.h?  We can of course do without it and put it into virgl_hw.h.  But there's a possibility of  VIRTIO_GPU_CAPSET_GFXSTREAM or VIRTIO_GPU_CAPSET_WAYLAND, and introducing a dependency on virgl_hw.h may not be the best for these cases. Gotta ask around more on this one...

      Also GET_CAPSET/GET_CAPSET_INFO are in the 2D group, for reasons I don't fully understand.  I'm not sure if virgl_hw.h is the only 3D thing or a 2D thing too.

      The ioctl I'm thinking of is something like VIRTGPU_SETPARAM rather than VIRTGPU_INIT currently in virtio-gpu-next (so it could be used for other things).  The flow would be like:

      setparam.setparam = VIRTGPU_DRM_CONTEXT_CAPSET
setparam.value = VIRTGPU_CAPSET_VENUS

      Since the available capset ids are already known by the kernel, we can have some basic error checking.  In addition, delayed context creation for older user space would use VIRTIO_GPU_CAPSET_VIRGL2. If the value isn't opaque in some instances, it makes sense to make the value not opaque for others.

      Edited by Gurchetan Singh
    • Chia-I Wu
      Chia-I Wu @olv ·
      Author Owner

      It would be nice not to have a dependency on virgl_hw.h, if we're shooting for a Zink or Vkangle + VK future.

      Ok, I can refactor virgl_hw.h. How about

      • moving virgl_caps_v* to virgl_hw_capsets.h
      • moving resource v1 defines to virgl_hw_resource.h
      • include both headers from virgl_hw.h

      venus definitions will be added to virgl_hw_venus.h.

      We already have VIRTIO_GPU_CAPSET_VIRGL2, VIRTIO_GPU_CAPSET_VIRGL in virtio_gpu.h / QEMU. So probably we're probably going to add VIRTIO_GPU_CAPSET_VENUS.

      Those defines are a bit awkward. They are not spec'ed yet.

      I think we should instead add

      void virgl_renderer_enumerate_cap_sets(uint32_t *count, uint32_t *sets);

      to enumerate all capsets that virgl_renderer_get_cap_set and virgl_renderer_fill_caps understand. qemu will report count in its virtio_gpu_virgl_get_num_capsets, and use sets for the index-to-id mapping that virgl_cmd_get_capset_info needs.

      Should we add VIRTGPU_CAPSET_VIRGL2/VIRTGPU_CAPSET_VIRGL2/VIRTGPU_CAPSET_VIRGL2 too in virtgpu_drm.h? We can of course do without it and put it into virgl_hw.h. But there's a possibility of VIRTIO_GPU_CAPSET_GFXSTREAM or VIRTIO_GPU_CAPSET_WAYLAND, and introducing a dependency on virgl_hw.h may not be the best for these cases. Gotta ask around more on this one...

      Why does the kernel need to know that the context is a gfxsteam one or wayland one?

      The ioctl I'm thinking of is something like VIRTGPU_SETPARAM rather than VIRTGPU_INIT currently in virtio-gpu-next (so it could be used for other things). The flow would be like:

      There is already GETPARAM which is a global thing. I think this new SETPARAM should at least be renamed since it is a per-file thing?

      I agree that there may be context params that can be set dynamically. What I am not sure about is if the context type is a parameter or not.

      Since the available capset ids are already known by the kernel, we can have some basic error checking. In addition, delayed context creation for older user space would use VIRTIO_GPU_CAPSET_VIRGL2. If the value isn't opaque in some instances, it makes sense to make the value not opaque for others.

      The value of VIRTGPU_DRM_CONTEXT_CAPSET must be a valid capset id, as determined by DRM_IOCTL_VIRTGPU_GET_CAPS. That is all the kernel cares and can already validate.

    • Chia-I Wu
      Chia-I Wu @olv ·
      Author Owner

      Can we move the discussion about setparam/capset to #176 (closed), and focus on header organization in this issue.

    • Gurchetan Singh
      Gurchetan Singh @gurchetansingh ·
      Developer

      venus definitions will be added to virgl_hw_venus.h.

      Will venus capsets be included virgl_hw_capsets.h?  Super-long term, I think it makes sense to even not think about virgl_caps_v2 and especially virgl_caps_v1 since it's been deprecated for 2+ years.

      What if we add enum virgl_capsets tovirgl_hw_venus.h, and keep the old definitions in virgl_hw.h, but add new definitions to virgl_hw_venus.h.  Virgl Mesa can continue using magic numbers, as we have been doing, or include virgl_hw_venus.h to not use magic numbers.

      Why does the kernel need to know that the context is a gfxsteam one or wayland one?

      Hmmm ... probably have to ask around on intentions/desires here, since I've only touched these code bases tangentially.

      I think this new SETPARAM should at least be renamed since it is a per-file thing?

      Sure.

      Edited by Gurchetan Singh
    • Chia-I Wu
      Chia-I Wu @olv ·
      Author Owner

      Will venus capsets be included virgl_hw_capsets.h? Super-long term, I think it makes sense to even not think about virgl_caps_v2 and especially virgl_caps_v1 since it's been deprecated for 2+ years.

      No, venus capsets should be in virgl_hw_venus.h.

      What if we add enum virgl_capsets tovirgl_hw_venus.h, and keep the old definitions in virgl_hw.h, but add new definitions to virgl_hw_venus.h. Virgl Mesa can continue using magic numbers, as we have been doing, or include virgl_hw_venus.h to not use magic numbers.

      I thought the intention of the reorganization was such that virgl does not use venus-specific definitions, and vice versa? Roughly,

      • virgl_hw.h for definitions that are renderer-agnostic
      • virgl_hw_virgl.h for definitions that are virgl-specific
      • virgl_hw_venus.h for definitions that are venus-specific

      But there are a bit of twists

      • v1 resource definitions are renderer-agnostic but only virgl uses them
      • virgl_hw_virgl.h has a weird name
      • virgl_hw.h should include the other two headers for backward-compatibility

      These should be easily solvable but need a consensus.

      By keeping the old definitions in virgl_hw.h and adding new definitions to virgl_hw_venus.h, and allowing virgl to include virgl_hw_venus.h, do you want to deprecate virgl_hw.h in the super-long term? In that case, I would suggest

      • virgl_hw.h for new definitions and include virgl_hw_legacy.h
      • virgl_hw_legacy.h for old definitions
      Edited by Chia-I Wu
    • Chia-I Wu mentioned in issue #176 (closed)

      mentioned in issue #176 (closed)

    • Chia-I Wu mentioned in merge request !422 (closed)

      mentioned in merge request !422 (closed)

    • Gurchetan Singh
      Gurchetan Singh @gurchetansingh ·
      Developer

      I think any v1 non-renderer agnostic definitions (needed by the guest kernel) should be moved into the virtio spec. 

      I wouldn't bother messing with virgl_hw.h too much since it's legacy -- just come with a venus_hw.h. 

      I think virglrenderer can include "venus_hw.h", and virgl Mesa probably continue using magic numbers. Or virtgpu_hw.h can include virgl_hw.h and venus_hw.h, if that is the preference. Maybe include enum virgl_capsets in venus_hw.h or virtgpu_hw.h.

      Edited by Gurchetan Singh
    • Chia-I Wu
      Chia-I Wu @olv ·
      Author Owner

      Or virtgpu_hw.h can include virgl_hw.h and venus_hw.h, if that is the preference. Maybe include enum virgl_capsets in venus_hw.h or virtgpu_hw.h.

      OK. I will call the new header virglrenderer_hw.h instead of virtgpu_hw.h. It is a companion header for virglrenderer.h and provides definitions for the opaque function parameters in virglrenderer.h.

    • Chia-I Wu mentioned in merge request !423 (merged)

      mentioned in merge request !423 (merged)

    • Yiwei Zhang closed

      closed

    Please register or sign in to reply