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

[Gurchetan Singh <gurchetansingh@chromium.org>]

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
>