[virtio-comment] [RFC PATCH v2 1/2] virtio-gpu: add resource create blob

[virtio-comment] [RFC PATCH v2 1/2] virtio-gpu: add resource create blob

[Gurchetan Singh]

Blob resources are size-based containers for host, guest, or
host+guest allocations.  These resources are designed with
mulit-process 3D support in mind, but also usable in virtio-gpu 2d
with guest memory.

Many hypercalls are reused, since a image view into the blob resource
is possible.

Blob resources are both forward and backward looking.

v2: Add TRANSFER_BLOB, SET_SCANOUT_BLOB, SCANOUT_FLUSH

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 virtio-gpu.tex | 143 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 143 insertions(+)

diff --git a/virtio-gpu.tex b/virtio-gpu.tex
index e70532f..754d5a8 100644
--- a/virtio-gpu.tex
+++ b/virtio-gpu.tex
@@ -36,6 +36,7 @@ \subsection{Feature bits}\label{sec:Device Types / GPU Device / Feature bits}
 \item[VIRTIO_GPU_F_VIRGL (0)] virgl 3D mode is supported.
 \item[VIRTIO_GPU_F_EDID  (1)] EDID is supported.
 \item[VIRTIO_GPU_F_RESOURCE_UUID (2)] assigning resources UUIDs for export to other virtio devices is supported.
+\item[VIRTIO_GPU_F_RESOURCE_BLOB (3)] creating and using size-based blob resources is supported.
 \end{description}
 
 \subsection{Device configuration layout}\label{sec:Device Types / GPU Device / Device configuration layout}
@@ -188,6 +189,10 @@ \subsubsection{Device Operation: Request header}\label{sec:Device Types / GPU De
         VIRTIO_GPU_CMD_GET_CAPSET,
         VIRTIO_GPU_CMD_GET_EDID,
         VIRTIO_GPU_CMD_RESOURCE_ASSIGN_UUID,
+        VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB,
+        VIRTIO_GPU_CMD_TRANSFER_BLOB,
+        VIRTIO_GPU_CMD_SCANOUT_BLOB,
+        VIRTIO_GPU_CMD_SCANOUT_FLUSH,
 
         /* 3d commands (OpenGL) */
         VIRTIO_GPU_CMD_CTX_CREATE = 0x0200,
@@ -498,6 +503,144 @@ \subsubsection{Device Operation: controlq}\label{sec:Device Types / GPU Device /
 other devices are not visible in the attached backing until they are transferred
 into the backing.
 
+\item[VIRTIO_GPU_CMD_RESOURCE_CREATE_BLOB] Creates a virtio-gpu blob
+  resource. Request data is \field{struct
+  virtio_gpu_resource_create_blob}, followed by \field{struct
+  virtio_gpu_mem_entry} entries. Response type is
+  VIRTIO_GPU_RESP_OK_NODATA. Support is optional and negotiated
+  using the VIRTIO_GPU_F_RESOURCE_BLOB feature flag.
+
+\begin{lstlisting}
+#define VIRTIO_GPU_BLOB_MEM_GUEST             0x0001
+#define VIRTIO_GPU_BLOB_MEM_HOST3D            0x0002
+#define VIRTIO_GPU_BLOB_MEM_HOST3D_GUEST      0x0003
+
+#define VIRTIO_GPU_BLOB_FLAG_USE_MAPPABLE     0x0001
+#define VIRTIO_GPU_BLOB_FLAG_USE_SHAREABLE    0x0002
+#define VIRTIO_GPU_BLOB_FLAG_USE_CROSS_DEVICE 0x0004
+
+struct virtio_gpu_resource_create_blob {
+       struct virtio_gpu_ctrl_hdr hdr;
+       le32 resource_id;
+       le32 blob_mem;
+       le32 blob_flags;
+       le32 nr_entries;
+       le64 blob_id;
+       le64 size;
+};
+
+\end{lstlisting}
+
+A blob resource is a container for:
+
+  \begin{itemize*}
+  \item a guest memory allocation (referred to as a
+  "guest-only blob resource").
+  \item a host memory allocation (referred to as a
+  "host-only blob resource").
+  \item a guest memory and host memory allocation (referred
+  to as a "default blob resource").
+  \end{itemize*}
+
+The memory properties of the blob resource MUST be described by
+\field{blob_mem}, which MUST be non-zero.
+
+For default and guest-only blob resources, \field{nents} guest
+system pages are assigned to the resource.  For default blob resources
+(i.e, when \field{blob_mem} is VIRTIO_GPU_BLOB_MEM_HOST3D_GUEST), these
+guest pages are used as a shadow buffer for the host memory.
+
+\field{blob_mem} can only be VIRTIO_GPU_BLOB_MEM_HOST3D and
+VIRTIO_GPU_BLOB_MEM_HOST3D_GUEST if VIRTIO_GPU_F_VIRGL is supported.
+VIRTIO_GPU_BLOB_MEM_GUEST is valid regardless whether VIRTIO_GPU_F_VIRGL
+is supported or not.
+
+For VIRTIO_GPU_BLOB_MEM_HOST3D and VIRTIO_GPU_BLOB_MEM_HOST3D_GUEST, the
+virtio-gpu resource MUST be created from the rendering context local object
+identified by the \field{blob_id}. The actual allocation is done via
+VIRTIO_GPU_CMD_SUBMIT_3D.
+
+The driver MUST inform the device if the blob resource is used for
+memory access, sharing between driver instances and/or sharing with
+other devices. This is done via the \field{blob_flags} field.
+
+If VIRTIO_GPU_F_VIRGL is set, both VIRTIO_GPU_CMD_TRANSFER_TO_HOST_3D
+and VIRTIO_GPU_CMD_TRANSFER_FROM_HOST_3D may be used to update the
+resource. There is no restriction on the image/buffer view the driver
+has on the blob resource.  VIRTIO_GPU_CMD_TRANSFER_BLOB (described below)
+may also be used.
+
+\item[VIRTIO_GPU_CMD_TRANSFER_BLOB] synchronizes a blob resource. Request
+  data is \field{struct virtio_gpu_resource_transfer_blob}, followed by
+  \field{struct virtio_gpu_blob_range} entries. Response type is
+  VIRTIO_GPU_RESP_OK_NODATA. Support is optional and negotiated
+  using the VIRTIO_GPU_F_RESOURCE_BLOB feature flag.
+
+\begin{lstlisting}
+#define VIRTIO_GPU_TRANSFER_BLOB_TO_HOST   0x0001
+#define VIRTIO_GPU_TRANSFER_BLOB_FROM_HOST 0x0002
+
+struct virtio_gpu_blob_range {
+       le64 offset;
+       le64 size;
+};
+
+struct virtio_gpu_resource_transfer_blob {
+       struct virtio_gpu_ctrl_hdr hdr;
+       le32 resource_id;
+       le32 flags;
+       le32 nr_ranges;
+       le32 pad;
+};
+\end{lstlisting}
+
+The default blob resources, transfer operations transfer operations
+copy the contents of the guest pages to or from the host.
+For host-only blob resources, transfer operations MAY synchronize caches.
+
+\item[VIRTIO_GPU_CMD_SET_SCANOUT_BLOB] sets scanout parameters for a
+   blob resource. Request data is
+  \field{struct virtio_gpu_set_scanout_blob}. Response type is
+  VIRTIO_GPU_RESP_OK_NODATA. Support is optional and negotiated
+  using the VIRTIO_GPU_F_RESOURCE_BLOB feature flag.
+
+\begin{lstlisting}
+struct virtio_gpu_set_scanout_blob {
+       struct virtio_gpu_ctrl_hdr hdr;
+       struct virtio_gpu_rect r;
+       le32 scanout_id;
+       le32 resource_id;
+       le32 width;
+       le32 height;
+       le32 format;
+       le32 padding;
+       le32 strides[4];
+       le32 offsets[4];
+};
+\end{lstlisting}
+
+The rectangle \field{r} represents the portion of the blob resource being
+displayed. The rest is the metadata associated with the blob resource. The
+format MUST be one of \field{enum virtio_gpu_formats}.
+
+\item[VIRTIO_GPU_CMD_SCANOUT_FLUSH] flushes a portion of the scanout surface
+to the display. The request data is \field{struct virtio_gpu_scanout_flush}.
+
+Support is optional and negotiated using the VIRTIO_GPU_F_RESOURCE_BLOB
+feature flag.
+
+\begin{lstlisting}
+struct virtio_gpu_scanout_flush {
+      struct virtio_gpu_ctrl_hdr hdr;
+      struct virtio_gpu_rect r;
+      le32 scanout_id;
+      le32 padding;
+};
+\end{lstlisting}
+
+The rectangle \field{r} represents the portion of the scanout surface that has
+damage and needs to be updated.
+
 \end{description}
 
 \subsubsection{Device Operation: controlq (3d)}\label{sec:Device Types / GPU Device / Device Operation / Device Operation: controlq (3d)}
-- 
2.24.1


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] [RFC PATCH v2 2/2] virtio-gpu: add support for mapping/unmapping blob resources

[Gurchetan Singh]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 6092 bytes --]

This defines a virtgpu shared memory region, with the possibilty
of more in the future.  This is required to implement VK/GL coherent
memory semantics, among other things.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 virtio-gpu.tex | 72 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 68 insertions(+), 4 deletions(-)

diff --git a/virtio-gpu.tex b/virtio-gpu.tex
index 754d5a8..2bfebff 100644
--- a/virtio-gpu.tex
+++ b/virtio-gpu.tex
@@ -88,13 +88,31 @@ \subsubsection{Events}
 information is available or all displays are disabled the driver MAY
 choose to use a fallback, such as 1024x768 at display 0.
 
+The driver SHOULD query all shared memory regions supported by the device.
+If the device supports shared memory, the \field{shmid} of a region MUST
+(see \ref{sec:Basic Facilities of a Virtio Device /
+Shared Memory Regions}~\nameref{sec:Basic Facilities of a Virtio Device /
+Shared Memory Regions}) be one of the following:
+
+\begin{lstlisting}
+enum virtio_gpu_shm_id {
+        VIRTIO_GPU_SHM_ID_UNDEFINED = 0,
+        VIRTIO_GPU_SHM_ID_HOST_VISIBLE = 1,
+};
+\end{lstlisting}
+
+The shared memory region with VIRTIO_GPU_SHM_ID_HOST_VISIBLE is referred as
+the "host visible memory region".  The device MUST support the
+VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB and VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB
+if the host visible memory region is available.
+
 \subsection{Device Operation}\label{sec:Device Types / GPU Device / Device Operation}
 
 The virtio-gpu is based around the concept of resources private to the
-host, the guest must DMA transfer into these resources. This is a
-design requirement in order to interface with future 3D rendering. In
-the unaccelerated 2D mode there is no support for DMA transfers from
-resources, just to them.
+host.  The guest must DMA transfer into these resources, unless shared memory
+regions are supported. This is a design requirement in order to interface with
+future 3D rendering. In the unaccelerated 2D mode there is no support for DMA
+transfers from resources, just to them.
 
 Resources are initially simple 2D resources, consisting of a width,
 height and format along with an identifier. The guest must then attach
@@ -203,6 +221,8 @@ \subsubsection{Device Operation: Request header}\label{sec:Device Types / GPU De
         VIRTIO_GPU_CMD_TRANSFER_TO_HOST_3D,
         VIRTIO_GPU_CMD_TRANSFER_FROM_HOST_3D,
         VIRTIO_GPU_CMD_SUBMIT_3D,
+        VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB,
+        VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB,
 
         /* cursor commands */
         VIRTIO_GPU_CMD_UPDATE_CURSOR = 0x0300,
@@ -215,6 +235,7 @@ \subsubsection{Device Operation: Request header}\label{sec:Device Types / GPU De
         VIRTIO_GPU_RESP_OK_CAPSET,
         VIRTIO_GPU_RESP_OK_EDID,
         VIRTIO_GPU_RESP_OK_RESOURCE_UUID,
+        VIRTIO_GPU_RESP_OK_MAP_INFO,
 
         /* error responses */
         VIRTIO_GPU_RESP_ERR_UNSPEC = 0x1200,
@@ -641,6 +662,7 @@ \subsubsection{Device Operation: controlq}\label{sec:Device Types / GPU Device /
 The rectangle \field{r} represents the portion of the scanout surface that has
 damage and needs to be updated.
 
+
 \end{description}
 
 \subsubsection{Device Operation: controlq (3d)}\label{sec:Device Types / GPU Device / Device Operation / Device Operation: controlq (3d)}
@@ -666,6 +688,48 @@ \subsubsection{Device Operation: controlq (3d)}\label{sec:Device Types / GPU Dev
 \item[VIRTIO_GPU_CMD_SUBMIT_3D]
   Submit rendering commands (mesa gallium command stream).
 
+\item[VIRTIO_GPU_CMD_RESOURCE_MAP_BLOB] maps a host-only
+  blob resource into an offset in the host visible memory region. Request
+  data is \field{struct virtio_gpu_resource_map_blob}.  Response type
+  is VIRTIO_GPU_RESP_OK_MAP_INFO. Support is optional and negotiated
+  using the VIRTIO_GPU_F_RESOURCE_BLOB feature flag and checking for
+  the presence of the host visible memory region.
+
+\begin{lstlisting}
+struct virtio_gpu_resource_map_blob {
+        struct virtio_gpu_ctrl_hdr hdr;
+        le32 resource_id;
+        le32 padding;
+        le64 offset;
+};
+
+#define VIRTIO_GPU_MAP_CACHE_MASK      0x0f
+#define VIRTIO_GPU_MAP_CACHE_NONE      0x00
+#define VIRTIO_GPU_MAP_CACHE_CACHED    0x01
+#define VIRTIO_GPU_MAP_CACHE_UNCACHED  0x02
+#define VIRTIO_GPU_MAP_CACHE_WC        0x03
+struct virtio_gpu_resp_map_info {
+        struct virtio_gpu_ctrl_hdr hdr;
+        u32 map_info;
+        u32 padding;
+};
+\end{lstlisting}
+
+\item[VIRTIO_GPU_CMD_RESOURCE_UNMAP_BLOB] unmaps a
+  host-only blob resource from the host visible memory region. Request data
+  is \field{struct virtio_gpu_resource_unmap_blob}.  Response type is
+  VIRTIO_GPU_RESP_OK_NODATA.  Support is optional and negotiated
+  using the VIRTIO_GPU_F_RESOURCE_BLOB feature flag and checking for
+  the presence of the host visible memory region.
+
+\begin{lstlisting}
+struct virtio_gpu_resource_unmap_blob {
+        struct virtio_gpu_ctrl_hdr hdr;
+        le32 resource_id;
+        le32 padding;
+};
+\end{lstlisting}
+
 \end{description}
 
 \subsubsection{Device Operation: cursorq}\label{sec:Device Types / GPU Device / Device Operation / Device Operation: cursorq}
-- 
2.24.1


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [RFC PATCH v2 1/2] virtio-gpu: add resource create blob

[Gerd Hoffmann]

> +\item[VIRTIO_GPU_CMD_SET_SCANOUT_BLOB] sets scanout parameters for a
> +   blob resource. Request data is
> +  \field{struct virtio_gpu_set_scanout_blob}. Response type is
> +  VIRTIO_GPU_RESP_OK_NODATA. Support is optional and negotiated
> +  using the VIRTIO_GPU_F_RESOURCE_BLOB feature flag.
> +
> +\begin{lstlisting}
> +struct virtio_gpu_set_scanout_blob {
> +       struct virtio_gpu_ctrl_hdr hdr;
> +       struct virtio_gpu_rect r;
> +       le32 scanout_id;
> +       le32 resource_id;
> +       le32 width;
> +       le32 height;
> +       le32 format;
> +       le32 padding;
> +       le32 strides[4];
> +       le32 offsets[4];
> +};
> +\end{lstlisting}
> +
> +The rectangle \field{r} represents the portion of the blob resource being
> +displayed. The rest is the metadata associated with the blob resource. The
> +format MUST be one of \field{enum virtio_gpu_formats}.

So you've changed strides & offsets to arrays, for planar formats.
We don't have any planar formats in virtio_gpu_formats though ...

So add a note that this is for future planar format support?
Or do we want add planar formats now?

Also: should resource_id an array too?  So we have the option to store
each plane in a different resource (simliar to drm_framebuffer in the
linux kernel)?

cheers,
  Gerd


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [RFC PATCH v2 1/2] virtio-gpu: add resource create blob

[Gurchetan Singh]

On Tue, May 12, 2020 at 5:24 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > +\item[VIRTIO_GPU_CMD_SET_SCANOUT_BLOB] sets scanout parameters for a
> > +   blob resource. Request data is
> > +  \field{struct virtio_gpu_set_scanout_blob}. Response type is
> > +  VIRTIO_GPU_RESP_OK_NODATA. Support is optional and negotiated
> > +  using the VIRTIO_GPU_F_RESOURCE_BLOB feature flag.
> > +
> > +\begin{lstlisting}
> > +struct virtio_gpu_set_scanout_blob {
> > +       struct virtio_gpu_ctrl_hdr hdr;
> > +       struct virtio_gpu_rect r;
> > +       le32 scanout_id;
> > +       le32 resource_id;
> > +       le32 width;
> > +       le32 height;
> > +       le32 format;
> > +       le32 padding;
> > +       le32 strides[4];
> > +       le32 offsets[4];
> > +};
> > +\end{lstlisting}
> > +
> > +The rectangle \field{r} represents the portion of the blob resource being
> > +displayed. The rest is the metadata associated with the blob resource. The
> > +format MUST be one of \field{enum virtio_gpu_formats}.
>
> So you've changed strides & offsets to arrays, for planar formats.
> We don't have any planar formats in virtio_gpu_formats though ...
>
> So add a note that this is for future planar format support?
> Or do we want add planar formats now?

No YUV support for now.  I was mostly thinking about compressed
standard formats (AB24, AR24) with a header + data plane
(I915_FORMAT_MOD_Y_TILED_CCS etc.).  The modifier can be queried from
virglrenderer in the distant future to avoid modifiers in the guest
(which is another can of worms).  Will write a note about that.

>
> Also: should resource_id an array too?  So we have the option to store
> each plane in a different resource (simliar to drm_framebuffer in the
> linux kernel)?

It's a cost vs. level of future-proofing tradeoff.

Pros:
+ To mirror the KMS/wayland APIs, a resource_id array +
VIRTIO_GPU_CMD_SET_SCANOUT_BLOBS makes sense.
Cons
- The trend seems to be one buffer.  I've only encountered disjoint
YUV images on deprecated platforms, and reviewing the modifiers in
<drm_fourcc.h> seem to be one buffer too.
- We can probably reuse RESOURCE_FLUSH (no need for SCANOUT_FLUSH?) if
we only allow one blob resource per drm framebuffer.  Display update
would be SET_SCANOUT_BLOB (which attaches a format to a resource) +
RESOURCE_FLUSH.  Kind of like VHOST_USER_GPU_DMABUF_SCANOUT +
VHOST_USER_GPU_DMABUF_UPDATE.  With SET_SCANOUT_BLOBS, we'll require
SCANOUT_FLUSH.

No multi-plane blob works for us too -- CrOS doesn't really use the
scanout blob api, as we rely on virtio-wl for display integration.
It's up to you/other virtio-gpu kms consumers.

>
> cheers,
>   Gerd
>

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [RFC PATCH v2 1/2] virtio-gpu: add resource create blob

[Gerd Hoffmann]

  Hi,

> > So add a note that this is for future planar format support?
> > Or do we want add planar formats now?
> 
> No YUV support for now.  I was mostly thinking about compressed
> standard formats (AB24, AR24) with a header + data plane
> (I915_FORMAT_MOD_Y_TILED_CCS etc.).  The modifier can be queried from
> virglrenderer in the distant future to avoid modifiers in the guest
> (which is another can of worms).  Will write a note about that.

Ok, good.

> > Also: should resource_id an array too?  So we have the option to store
> > each plane in a different resource (simliar to drm_framebuffer in the
> > linux kernel)?
> 
> It's a cost vs. level of future-proofing tradeoff.
> 
> Pros:
> + To mirror the KMS/wayland APIs, a resource_id array +
> VIRTIO_GPU_CMD_SET_SCANOUT_BLOBS makes sense.
> Cons
> - The trend seems to be one buffer.  I've only encountered disjoint
> YUV images on deprecated platforms, and reviewing the modifiers in
> <drm_fourcc.h> seem to be one buffer too.

Ok.  I guess it makes sense to stick to one buffer then.  An API which
isn't used in practice is a maintenance burden we better avoid ...

take care,
  Gerd


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [RFC PATCH v2 1/2] virtio-gpu: add resource create blob

[Gurchetan Singh]

On Wed, May 13, 2020 at 12:03 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > > So add a note that this is for future planar format support?
> > > Or do we want add planar formats now?
> >
> > No YUV support for now.  I was mostly thinking about compressed
> > standard formats (AB24, AR24) with a header + data plane
> > (I915_FORMAT_MOD_Y_TILED_CCS etc.).  The modifier can be queried from
> > virglrenderer in the distant future to avoid modifiers in the guest
> > (which is another can of worms).  Will write a note about that.
>
> Ok, good.
>
> > > Also: should resource_id an array too?  So we have the option to store
> > > each plane in a different resource (simliar to drm_framebuffer in the
> > > linux kernel)?
> >
> > It's a cost vs. level of future-proofing tradeoff.
> >
> > Pros:
> > + To mirror the KMS/wayland APIs, a resource_id array +
> > VIRTIO_GPU_CMD_SET_SCANOUT_BLOBS makes sense.
> > Cons
> > - The trend seems to be one buffer.  I've only encountered disjoint
> > YUV images on deprecated platforms, and reviewing the modifiers in
> > <drm_fourcc.h> seem to be one buffer too.
>
> Ok.  I guess it makes sense to stick to one buffer then.  An API which
> isn't used in practice is a maintenance burden we better avoid ...

Cool.  I left out SCANOUT_FLUSH in v3 since AFAICT we'll only have one
blob resource per scanout, and that blob resource will encompass the
entire window (since we set_scanout when display parameters change).
And to update the scanout blob resource, we can just call
RESOURCE_FLUSH.

Though SCANOUT_FLUSH does make sense if you want multiple blob
resources per scanout surface, so if there's a desire for some
additional functionality, LMK.

Also, TRANSFER_BLOB is also another maintenance vs. future proofing
tradeoff -- for example, emulated coherent memory[1] (for platforms
without the host-visible region or external memory extensions or
shared guest) may benefit from such an API.  However, the
implementation of that doesn't exist and may not for a while (focusing
on true zero copy for now), and the virglrenderer impl of the
hypercall will likely just be a stub for now.  So if that's
undesirable, LMK.

[1] https://lists.freedesktop.org/archives/dri-devel/2019-April/215731.htm

>
> take care,
>   Gerd
>

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [RFC PATCH v2 1/2] virtio-gpu: add resource create blob

[Gerd Hoffmann]

  Hi,

> Cool.  I left out SCANOUT_FLUSH in v3 since AFAICT we'll only have one
> blob resource per scanout, and that blob resource will encompass the
> entire window (since we set_scanout when display parameters change).
> And to update the scanout blob resource, we can just call
> RESOURCE_FLUSH.

Hmm, right, using the SET_SCANOUT parameters in case of BLOB resources
should work.

> Though SCANOUT_FLUSH does make sense if you want multiple blob
> resources per scanout surface, so if there's a desire for some
> additional functionality, LMK.

Other way around: one big framebuffer/resource backing multiple
scanouts.

xorg used to handle multihead setups that way, but these days it is
rather uncommon.  Typically userspace creates one drm framebuffer
per drm crtc.  So, yes, I guess we don't have to worry much.

> Also, TRANSFER_BLOB is also another maintenance vs. future proofing
> tradeoff -- for example, emulated coherent memory[1] (for platforms
> without the host-visible region or external memory extensions or
> shared guest) may benefit from such an API.  However, the
> implementation of that doesn't exist and may not for a while (focusing
> on true zero copy for now), and the virglrenderer impl of the
> hypercall will likely just be a stub for now.  So if that's
> undesirable, LMK.

Implies the linux kernel can't use blob resources for dumb BOs in
case shared memory isn't available, same goes for dma-buf imports
(inside the guest).

cheers,
  Gerd


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [RFC PATCH v2 1/2] virtio-gpu: add resource create blob

[Gurchetan Singh]

On Thu, May 14, 2020 at 1:24 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > Cool.  I left out SCANOUT_FLUSH in v3 since AFAICT we'll only have one
> > blob resource per scanout, and that blob resource will encompass the
> > entire window (since we set_scanout when display parameters change).
> > And to update the scanout blob resource, we can just call
> > RESOURCE_FLUSH.
>
> Hmm, right, using the SET_SCANOUT parameters in case of BLOB resources
> should work.

Ok, so removing SCANOUT_FLUSH makes sense.

>
> > Though SCANOUT_FLUSH does make sense if you want multiple blob
> > resources per scanout surface, so if there's a desire for some
> > additional functionality, LMK.
>
> Other way around: one big framebuffer/resource backing multiple
> scanouts.
>
> xorg used to handle multihead setups that way, but these days it is
> rather uncommon.  Typically userspace creates one drm framebuffer
> per drm crtc.  So, yes, I guess we don't have to worry much.
>
> > Also, TRANSFER_BLOB is also another maintenance vs. future proofing
> > tradeoff -- for example, emulated coherent memory[1] (for platforms
> > without the host-visible region or external memory extensions or
> > shared guest) may benefit from such an API.  However, the
> > implementation of that doesn't exist and may not for a while (focusing
> > on true zero copy for now), and the virglrenderer impl of the
> > hypercall will likely just be a stub for now.  So if that's
> > undesirable, LMK.
>
> Implies the linux kernel can't use blob resources for dumb BOs in
> case shared memory isn't available,

v3 only allows VIRTIO_GPU_BLOB_MEM_GUEST for dumb blob resources,
which implies we won't really use TRANSFER_BLOB.

Currently, the path seems to be:

SET_SCANOUT
TRANSFER_TO_HOST_2D --> copies from iovecs to host private resource
RESOURCE_FLUSH_BLOB --> copies from host private resource to framebuffer

A theoretical display update path would be for dumb BOs without shared
mappings would be:

SET_SCANOUT_BLOB
RESOURCE_FLUSH_BLOB --> copies from iovecs to framebuffer

That should work with crosvm, you might want to verify if it's doable with QEMU.

QEMU seems to have multiple dumb display paths --
qemu_spice_display_update, sdl2_2d_update.  One possiblity is to only
use dumb blob resources in virtio-gpu kms when shared guest is
available rather than refactoring that code, or maybe you have
something else in mind?

> same goes for dma-buf imports (inside the guest).

dma-buf import from another virtio driver is very interesting, but
we'll probably need a some import UUID hypercall for that?  Maybe a
bit too expansive for F_RESOURCE_BLOB at this point.

Though TRANSFER_BLOB can be useful.  I can see the following use cases:

- implementing guest kernel dma-buf mmap and synchronization
(begin_cpu_access/end_cpu_access).  Currently, it doesn't work but no
guest user-space relies on it, so no one has noticed/complained.
- emulated coherent memory

It sounds like you think TRANSFER_BLOB is worth the maintenance vs.
future proofing tradeoff, which is fair.  I just want to make sure
we're all on the same page in terms of use cases.

>
> cheers,
>   Gerd
>

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [RFC PATCH v2 1/2] virtio-gpu: add resource create blob

[Gerd Hoffmann]

> v3 only allows VIRTIO_GPU_BLOB_MEM_GUEST for dumb blob resources,
> which implies we won't really use TRANSFER_BLOB.
> 
> Currently, the path seems to be:
> 
> SET_SCANOUT
> TRANSFER_TO_HOST_2D --> copies from iovecs to host private resource
> RESOURCE_FLUSH_BLOB --> copies from host private resource to framebuffer

Yes.

> A theoretical display update path would be for dumb BOs without shared
> mappings would be:
> 
> SET_SCANOUT_BLOB
> RESOURCE_FLUSH_BLOB --> copies from iovecs to framebuffer
> 
> That should work with crosvm, you might want to verify if it's doable with QEMU.

I think qemu can handle it.  With udmabuf qemu can simply mmap() the
resource, create a pixman image, then run with it.  Without udmabuf
it'll be a bit more complicated because we can't offload most work to
pixman then, but should still be doable.  Qemu could also choose to
allow blob resources only in case udmabuf is available to avoid handling
that.  So no blockers here.

> One possiblity is to only use dumb blob resources in virtio-gpu kms
> when shared guest is available rather than refactoring that code,

I think this makes sense.

> > same goes for dma-buf imports (inside the guest).
> 
> dma-buf import from another virtio driver is very interesting, but
> we'll probably need a some import UUID hypercall for that?

No, just dma-buf import from somewhere, say a gpu passed to the guest
via pci pass-through does the rendering to a dma-buf and that dmabuf
gets imported into virtio-gpu for display in a host window.

That'll requires BLOB resources to work, because we have to create a
guest bo (and virtio resource) without knowning what format the guest
wants to use to scanout the thing.

We may likewise allow that only in case shared guest is available.

> Though TRANSFER_BLOB can be useful.  I can see the following use cases:
> 
> - implementing guest kernel dma-buf mmap and synchronization
> (begin_cpu_access/end_cpu_access).  Currently, it doesn't work but no
> guest user-space relies on it, so no one has noticed/complained.
> - emulated coherent memory
> 
> It sounds like you think TRANSFER_BLOB is worth the maintenance vs.
> future proofing tradeoff, which is fair.

We can leave it out for now and see if we'll need it at some point in
the future.  One point of adding blob resources is to allow shared
mappings, so maybe it wouldn't be used. There is also the option to
fallback to traditional resources ...

cheers,
  Gerd


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [RFC PATCH v2 1/2] virtio-gpu: add resource create blob

[Gurchetan Singh]

On Fri, May 15, 2020 at 3:38 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > v3 only allows VIRTIO_GPU_BLOB_MEM_GUEST for dumb blob resources,
> > which implies we won't really use TRANSFER_BLOB.
> >
> > Currently, the path seems to be:
> >
> > SET_SCANOUT
> > TRANSFER_TO_HOST_2D --> copies from iovecs to host private resource
> > RESOURCE_FLUSH_BLOB --> copies from host private resource to framebuffer
>
> Yes.
>
> > A theoretical display update path would be for dumb BOs without shared
> > mappings would be:
> >
> > SET_SCANOUT_BLOB
> > RESOURCE_FLUSH_BLOB --> copies from iovecs to framebuffer
> >
> > That should work with crosvm, you might want to verify if it's doable with QEMU.
>
> I think qemu can handle it.  With udmabuf qemu can simply mmap() the
> resource, create a pixman image, then run with it.  Without udmabuf
> it'll be a bit more complicated because we can't offload most work to
> pixman then, but should still be doable.  Qemu could also choose to
> allow blob resources only in case udmabuf is available to avoid handling
> that.  So no blockers here.
>
> > One possiblity is to only use dumb blob resources in virtio-gpu kms
> > when shared guest is available rather than refactoring that code,
>
> I think this makes sense.
>
> > > same goes for dma-buf imports (inside the guest).
> >
> > dma-buf import from another virtio driver is very interesting, but
> > we'll probably need a some import UUID hypercall for that?
>
> No, just dma-buf import from somewhere, say a gpu passed to the guest
> via pci pass-through does the rendering to a dma-buf and that dmabuf
> gets imported into virtio-gpu for display in a host window.

I'm unfamiliar with that use case ... is there a chance the dma-buf is
compressed/tiled?  If so, we won't be able to query virglrenderer for
the modifier and may need it in SCANOUT_BLOB unless there's another
way to get it host-side.

>
> That'll requires BLOB resources to work, because we have to create a
> guest bo (and virtio resource) without knowning what format the guest
> wants to use to scanout the thing.
>
> We may likewise allow that only in case shared guest is available.
>
> > Though TRANSFER_BLOB can be useful.  I can see the following use cases:
> >
> > - implementing guest kernel dma-buf mmap and synchronization
> > (begin_cpu_access/end_cpu_access).  Currently, it doesn't work but no
> > guest user-space relies on it, so no one has noticed/complained.
> > - emulated coherent memory
> >
> > It sounds like you think TRANSFER_BLOB is worth the maintenance vs.
> > future proofing tradeoff, which is fair.
>
> We can leave it out for now and see if we'll need it at some point in
> the future.  One point of adding blob resources is to allow shared
> mappings, so maybe it wouldn't be used. There is also the option to
> fallback to traditional resources ...

Ok, will leave out TRANSFER_BLOB in v4 for now.

>
> cheers,
>   Gerd
>

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [RFC PATCH v2 1/2] virtio-gpu: add resource create blob

[Gerd Hoffmann]

> > > > same goes for dma-buf imports (inside the guest).
> > >
> > > dma-buf import from another virtio driver is very interesting, but
> > > we'll probably need a some import UUID hypercall for that?
> >
> > No, just dma-buf import from somewhere, say a gpu passed to the guest
> > via pci pass-through does the rendering to a dma-buf and that dmabuf
> > gets imported into virtio-gpu for display in a host window.
> 
> I'm unfamiliar with that use case ... is there a chance the dma-buf is
> compressed/tiled?

Hmm, not fully sure but probably not.  I'd expect gpu drivers would
untile the BO when exporting to another driver because tiling modes
are typically hard-ware specific ...

> If so, we won't be able to query virglrenderer for the modifier and
> may need it in SCANOUT_BLOB unless there's another way to get it
> host-side.

No, there is no other magic way.  Userspace has to tell virtio-gpu what
format the dma-buf data has if it wants virtio-gpu do something with it,
the dma-buf itself is just a blob.

take care,
  Gerd


This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/

[virtio-comment] Re: [RFC PATCH v2 1/2] virtio-gpu: add resource create blob

[Gurchetan Singh]

On Mon, May 18, 2020 at 12:41 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > > > > same goes for dma-buf imports (inside the guest).
> > > >
> > > > dma-buf import from another virtio driver is very interesting, but
> > > > we'll probably need a some import UUID hypercall for that?
> > >
> > > No, just dma-buf import from somewhere, say a gpu passed to the guest
> > > via pci pass-through does the rendering to a dma-buf and that dmabuf
> > > gets imported into virtio-gpu for display in a host window.
> >
> > I'm unfamiliar with that use case ... is there a chance the dma-buf is
> > compressed/tiled?
>
> Hmm, not fully sure but probably not.  I'd expect gpu drivers would
> untile the BO when exporting to another driver because tiling modes
> are typically hard-ware specific ...

Okay, kept the modifier out of SCANOUT_BLOB in v4 as we expect it will
not be used for the pass through case.

>
> > If so, we won't be able to query virglrenderer for the modifier and
> > may need it in SCANOUT_BLOB unless there's another way to get it
> > host-side.
>
> No, there is no other magic way.  Userspace has to tell virtio-gpu what
> format the dma-buf data has if it wants virtio-gpu do something with it,
> the dma-buf itself is just a blob.
>
> take care,
>   Gerd
>

This publicly archived list offers a means to provide input to the
OASIS Virtual I/O Device (VIRTIO) TC.

In order to verify user consent to the Feedback License terms and
to minimize spam in the list archive, subscription is required
before posting.

Subscribe: virtio-comment-subscribe@lists.oasis-open.org
Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
List help: virtio-comment-help@lists.oasis-open.org
List archive: https://lists.oasis-open.org/archives/virtio-comment/
Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
Committee: https://www.oasis-open.org/committees/virtio/
Join OASIS: https://www.oasis-open.org/join/