On Wed, Oct 2, 2019 at 1:49 AM Gerd Hoffmann kraxel@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... [ii] https://chromium.googlesource.com/chromiumos/platform/minigbm/+/refs/heads/m...
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