[PATCH 2/2] [RFC] drm/virtgpu: modify uapi with stride/layer_stride fix

[Gurchetan Singh]

On Wed, Oct 2, 2019 at 1:49 AM Gerd Hoffmann <kraxel at redhat.com> wrote:
>
> On Tue, Oct 01, 2019 at 06:49:35PM -0700, Gurchetan Singh wrote:
> > This doesn't really break userspace, since it always passes down
> > 0 for stride/layer_stride currently. We could:
> >
> > (1) modify UAPI now and add a VIRTGPU_PARAM_STRIDE_FIX feature
>
> This I think.
> But IMO it's not a fix, it is an added feature ...
>
> Also missing the big picture here.  Why do we need this?

Two reasons:

a) wayland-proxing.  Generally, host_stride != guest_stride and
aligning to 64 is insufficient (for example, Intel x-tiled buffers).
There are generally three places we can make an adjustment:

1) In the wayland guest proxy, before crossing the VM boundary
2) In the wayland host proxy, before sending to the server
3) Make sure host_stride == guest_stride at allocation time

For (1), we'd have to extend drm_virtgpu_resource_info to actually
return the stride.  This raises questions about strides of 1D buffers,
cubemaps, etc..

For (2), somebody actually needs to write a wayland host proxy.  It's
too much work just for this bug.

For (3), since we have to do something like
VIRTIO_GPU_CMD_METADATA_QUERY (or whatever we call it) for Vulkan and
zero-copy anyways, this seemed like the most natural choice.
Consequently, when guest_stride != bpp * width, we'll have to make
some fixes in Mesa/virtio-gpu.  The only tricky part is modifiers -- I
suspect we'll keep a mapping of virtualized modifiers to host
modifiers.

It could make some sense to wait for VIRTIO_GPU_CMD_METADATA_QUERY to
land.  If we agree (3) is a practical solution to this, I recommend we
just land the first patch as a statement of purpose to save some
feature bits.  We can modify the uapi later.

b) We currently have hacks for YUV we can remove [i][ii].  This is
related to the restriction imposed by Android guest userspace that the
stride must be aligned to a certain amount of bytes.

[i] https://gitlab.freedesktop.org/virgl/virglrenderer/blob/master/src/virgl_gbm.c#L329
[ii] https://chromium.googlesource.com/chromiumos/platform/minigbm/+/refs/heads/master/virtio_gpu.c#278

> I don't think we can simply use the args here without checking we
actually got something from userspace ...

Correct me if I'm wrong, but doesn't drm_ioctl(..) already make sure
that the pointer is the intersection of the kernel and userspace
sizes, so we can safely append data?  i.e, layer_stride and stride
will be zero for old user space + a new kernel.




>
> For guest object we don't have strides (virtio_gpu_resource_create_3d
> doesn't allow this).
>
> For host objects the host should know the strides.
>
> Which I think is the reason why the stride and layer_stride fields in
> the transfer commands are effectively unused ...
>
> > -     /* TODO: add the correct stride / layer_stride. */
> >       virtio_gpu_cmd_transfer_from_host_3d
> > -             (vgdev, vfpriv->ctx_id, offset, args->level, 0, 0,
> > -              &box, objs, fence);
> > +             (vgdev, vfpriv->ctx_id, offset, args->level, args->stride,
> > +              args->layer_stride, &box, objs, fence);
>
> What happens with old userspace running on a new kernel?
>
> I don't think we can simply use the args here without checking we
> actually got something from userspace ...
>
> > diff --git a/include/uapi/drm/virtgpu_drm.h b/include/uapi/drm/virtgpu_drm.h
> > index f06a789f34cd..b2fc92c3d2be 100644
> > --- a/include/uapi/drm/virtgpu_drm.h
> > +++ b/include/uapi/drm/virtgpu_drm.h
> > @@ -117,6 +117,8 @@ struct drm_virtgpu_3d_transfer_to_host {
> >       struct drm_virtgpu_3d_box box;
> >       __u32 level;
> >       __u32 offset;
> > +     __u32 stride;
> > +     __u32 layer_stride;
> >  };
>
> cheers,
>   Gerd
>