| Message ID | 20191002014935.33171-2-gurchetansingh@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/virtgpu: plumb fix for virtgpu_drm.h / virtio_gpu.h discrepancy | expand |
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? 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
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.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 >
> 3) Make sure host_stride == guest_stride at allocation time > 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. Yep, for shared resources it certainly makes sense to have host and guest agree on the stride. I'd tend to not touch the current TRANSFER ioctls (and virtio commands) though, but integrate that into the blob resource support instead. We probably need blob transfer ioctls and virtio commands anyway. > > 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. Ah, right, I forgot the generic drm ioctl code does that service for us. cheers, Gerd
On Wed, Oct 2, 2019 at 5:18 PM Gurchetan Singh <gurchetansingh@chromium.org> wrote: > > 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: I don't fully get the picture, but drm_virtgpu_resource_create has stride. Can we send that down in transfers? It lacks layer_stride though...
On Fri, Feb 28, 2020 at 01:01:49PM -0800, Chia-I Wu wrote: > On Wed, Oct 2, 2019 at 5:18 PM Gurchetan Singh > <gurchetansingh@chromium.org> wrote: > > > > 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: > I don't fully get the picture, but drm_virtgpu_resource_create has > stride. Can we send that down in transfers? It's unused and suddenly caring about it has a good chance to break stuff ... cheers, Gerd
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c index 98b72dead962..c29473ac24a1 100644 --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c @@ -325,10 +325,9 @@ static int virtio_gpu_transfer_from_host_ioctl(struct drm_device *dev, goto err_unlock; } - /* 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); dma_fence_put(&fence->f); return 0; @@ -371,11 +370,10 @@ static int virtio_gpu_transfer_to_host_ioctl(struct drm_device *dev, void *data, if (!fence) goto err_unlock; - /* TODO: add the correct stride / layer_stride. */ virtio_gpu_cmd_transfer_to_host_3d (vgdev, vfpriv ? vfpriv->ctx_id : 0, offset, args->level, - 0, 0, &box, objs, fence); + args->stride, args->layer_stride, &box, objs, fence); dma_fence_put(&fence->f); } return 0; 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; }; struct drm_virtgpu_3d_transfer_from_host { @@ -124,6 +126,8 @@ struct drm_virtgpu_3d_transfer_from_host { struct drm_virtgpu_3d_box box; __u32 level; __u32 offset; + __u32 stride; + __u32 layer_stride; }; #define VIRTGPU_WAIT_NOWAIT 1 /* like it */
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 (2) modify the UAPI now, and not expose a corresponding feature (i.e, VIRTGPU_PARAM_STRIDE_FIX). This would fold this minor fix into another bigger feature (like the proposed metadata query). (3) don't modify UAPI now, wait until another feature lands. I don't have a preference either way, as long as we get something like this eventually. The corresponding userspace is: https://gitlab.freedesktop.org/mesa/mesa/merge_requests/2129 Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org> --- drivers/gpu/drm/virtio/virtgpu_ioctl.c | 8 +++----- include/uapi/drm/virtgpu_drm.h | 4 ++++ 2 files changed, 7 insertions(+), 5 deletions(-)