<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Sep 10, 2021 at 12:33 PM Chia-I Wu <<a href="mailto:olvaffe@gmail.com" target="_blank">olvaffe@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Sep 8, 2021 at 6:37 PM Gurchetan Singh<br>
<<a href="mailto:gurchetansingh@chromium.org" target="_blank">gurchetansingh@chromium.org</a>> wrote:<br>
><br>
> We don't want fences from different 3D contexts (virgl, gfxstream,<br>
> venus) to be on the same timeline. With explicit context creation,<br>
> we can specify the number of ring each context wants.<br>
><br>
> Execbuffer can specify which ring to use.<br>
><br>
> Signed-off-by: Gurchetan Singh <<a href="mailto:gurchetansingh@chromium.org" target="_blank">gurchetansingh@chromium.org</a>><br>
> Acked-by: Lingfeng Yang <<a href="mailto:lfy@google.com" target="_blank">lfy@google.com</a>><br>
> ---<br>
> drivers/gpu/drm/virtio/virtgpu_drv.h | 3 +++<br>
> drivers/gpu/drm/virtio/virtgpu_ioctl.c | 34 ++++++++++++++++++++++++--<br>
> 2 files changed, 35 insertions(+), 2 deletions(-)<br>
><br>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h<br>
> index a5142d60c2fa..cca9ab505deb 100644<br>
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h<br>
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h<br>
> @@ -56,6 +56,7 @@<br>
> #define STATE_ERR 2<br>
><br>
> #define MAX_CAPSET_ID 63<br>
> +#define MAX_RINGS 64<br>
><br>
> struct virtio_gpu_object_params {<br>
> unsigned long size;<br>
> @@ -263,6 +264,8 @@ struct virtio_gpu_fpriv {<br>
> uint32_t ctx_id;<br>
> uint32_t context_init;<br>
> bool context_created;<br>
> + uint32_t num_rings;<br>
> + uint64_t base_fence_ctx;<br>
> struct mutex context_lock;<br>
> };<br>
><br>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c<br>
> index f51f3393a194..262f79210283 100644<br>
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c<br>
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c<br>
> @@ -99,6 +99,11 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,<br>
> int in_fence_fd = exbuf->fence_fd;<br>
> int out_fence_fd = -1;<br>
> void *buf;<br>
> + uint64_t fence_ctx;<br>
> + uint32_t ring_idx;<br>
> +<br>
> + fence_ctx = vgdev->fence_drv.context;<br>
> + ring_idx = 0;<br>
><br>
> if (vgdev->has_virgl_3d == false)<br>
> return -ENOSYS;<br>
> @@ -106,6 +111,17 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,<br>
> if ((exbuf->flags & ~VIRTGPU_EXECBUF_FLAGS))<br>
> return -EINVAL;<br>
><br>
> + if ((exbuf->flags & VIRTGPU_EXECBUF_RING_IDX)) {<br>
> + if (exbuf->ring_idx >= vfpriv->num_rings)<br>
> + return -EINVAL;<br>
> +<br>
> + if (!vfpriv->base_fence_ctx)<br>
> + return -EINVAL;<br>
> +<br>
> + fence_ctx = vfpriv->base_fence_ctx;<br>
> + ring_idx = exbuf->ring_idx;<br>
> + }<br>
> +<br>
> exbuf->fence_fd = -1;<br>
><br>
> virtio_gpu_create_context(dev, file);<br>
> @@ -173,7 +189,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,<br>
> goto out_memdup;<br>
> }<br>
><br>
> - out_fence = virtio_gpu_fence_alloc(vgdev, vgdev->fence_drv.context, 0);<br>
> + out_fence = virtio_gpu_fence_alloc(vgdev, fence_ctx, ring_idx);<br>
> if(!out_fence) {<br>
> ret = -ENOMEM;<br>
> goto out_unresv;<br>
> @@ -691,7 +707,7 @@ static int virtio_gpu_context_init_ioctl(struct drm_device *dev,<br>
> return -EINVAL;<br>
><br>
> /* Number of unique parameters supported at this time. */<br>
> - if (num_params > 1)<br>
> + if (num_params > 2)<br>
> return -EINVAL;<br>
><br>
> ctx_set_params = memdup_user(u64_to_user_ptr(args->ctx_set_params),<br>
> @@ -731,6 +747,20 @@ static int virtio_gpu_context_init_ioctl(struct drm_device *dev,<br>
><br>
> vfpriv->context_init |= value;<br>
> break;<br>
> + case VIRTGPU_CONTEXT_PARAM_NUM_RINGS:<br>
> + if (vfpriv->base_fence_ctx) {<br>
> + ret = -EINVAL;<br>
> + goto out_unlock;<br>
> + }<br>
> +<br>
> + if (value > MAX_RINGS) {<br>
> + ret = -EINVAL;<br>
> + goto out_unlock;<br>
> + }<br>
> +<br>
> + vfpriv->base_fence_ctx = dma_fence_context_alloc(value);<br>
With multiple fence contexts, we should do something about implicit fencing.<br>
<br>
The classic example is Mesa and X server. When both use virgl and the<br>
global fence context, no dma_fence_wait is fine. But when Mesa uses<br>
venus and the ring fence context, dma_fence_wait should be inserted.<br></blockquote><div><br></div><div> If I read your comment correctly, the use case is:</div><div><br></div><div>context A (venus) </div><div><br></div><div>sharing a render target with</div><div><br></div><div>context B (Xserver backed virgl)</div><div><br></div><div>?</div><div><br></div><div>Which function do you envisage dma_fence_wait(...) to be inserted? Doesn't implicit synchronization mean there's no fence to share between contexts (only buffer objects)? </div><div><br></div><div>It may be possible to wait on the reservation object associated with a buffer object from a different context (userspace can also do DRM_IOCTL_VIRTGPU_WAIT), but not sure if that's what you're looking for.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
> + vfpriv->num_rings = value;<br>
> + break;<br>
> default:<br>
> ret = -EINVAL;<br>
> goto out_unlock;<br>
> --<br>
> 2.33.0.153.gba50c8fa24-goog<br>
><br>
><br>
> ---------------------------------------------------------------------<br>
> To unsubscribe, e-mail: <a href="mailto:virtio-dev-unsubscribe@lists.oasis-open.org" target="_blank">virtio-dev-unsubscribe@lists.oasis-open.org</a><br>
> For additional commands, e-mail: <a href="mailto:virtio-dev-help@lists.oasis-open.org" target="_blank">virtio-dev-help@lists.oasis-open.org</a><br>
><br>
</blockquote></div></div>