On Wed, Oct 2, 2019 at 1:49 AM Gerd Hoffmann <kraxel@xxxxxxxxxx> 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 > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
