[RFC PATCH 0/8] *** Per context fencing ***

[RFC PATCH 0/8] *** Per context fencing ***

[Gurchetan Singh]

We don't want fences from different 3D contexts/processes (GL, VK) to
be on the same timeline. Sending this out as a RFC to solicit feedback
on the general approach.

Gurchetan Singh (8):
  drm/virtio: use fence_id when processing fences
  drm/virtio: allocate a fence context for every 3D context
  drm/virtio: plumb virtio_gpu_fpriv to virtio_gpu_fence_alloc
  drm/virtio: rename sync_seq and last_seq
  drm/virtio: track fence_id in virtio_gpu_fence
  virtio/drm: rework virtio_fence_signaled
  drm/virtio: check context when signaling
  drm/virtio: enable per context fencing

 drivers/gpu/drm/virtio/virtgpu_debugfs.c |  4 +-
 drivers/gpu/drm/virtio/virtgpu_drv.h     | 12 +++--
 drivers/gpu/drm/virtio/virtgpu_fence.c   | 66 ++++++++++++++++--------
 drivers/gpu/drm/virtio/virtgpu_ioctl.c   |  9 ++--
 drivers/gpu/drm/virtio/virtgpu_kms.c     |  1 +
 drivers/gpu/drm/virtio/virtgpu_plane.c   |  2 +-
 drivers/gpu/drm/virtio/virtgpu_vq.c      |  4 +-
 7 files changed, 62 insertions(+), 36 deletions(-)

-- 
2.25.1.481.gfbce0eb801-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 1/8] drm/virtio: use fence_id when processing fences

[Gurchetan Singh]

Currently, the fence ID, which can be used to identify a
virtgpu fence, is the same as the fence sequence number.

They could be the same, but not necessarily so. Let's differentiate
them.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   | 2 +-
 drivers/gpu/drm/virtio/virtgpu_fence.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index c1824bdf2418..9627cd08f5be 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -360,7 +360,7 @@ void virtio_gpu_fence_emit(struct virtio_gpu_device *vgdev,
 			  struct virtio_gpu_ctrl_hdr *cmd_hdr,
 			  struct virtio_gpu_fence *fence);
 void virtio_gpu_fence_event_process(struct virtio_gpu_device *vdev,
-				    u64 last_seq);
+				    u64 fence_id);
 
 /* virtio_gpu_object */
 void virtio_gpu_cleanup_object(struct virtio_gpu_object *bo);
diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
index 5b2a4146c5bd..2fe9c7ebcbd4 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -112,16 +112,16 @@ void virtio_gpu_fence_emit(struct virtio_gpu_device *vgdev,
 }
 
 void virtio_gpu_fence_event_process(struct virtio_gpu_device *vgdev,
-				    u64 last_seq)
+				    u64 fence_id)
 {
 	struct virtio_gpu_fence_driver *drv = &vgdev->fence_drv;
 	struct virtio_gpu_fence *fence, *tmp;
 	unsigned long irq_flags;
 
 	spin_lock_irqsave(&drv->lock, irq_flags);
-	atomic64_set(&vgdev->fence_drv.last_seq, last_seq);
+	atomic64_set(&vgdev->fence_drv.last_seq, fence_id);
 	list_for_each_entry_safe(fence, tmp, &drv->fences, node) {
-		if (last_seq < fence->f.seqno)
+		if (fence_id < fence->f.seqno)
 			continue;
 		dma_fence_signal_locked(&fence->f);
 		list_del(&fence->node);
-- 
2.25.1.481.gfbce0eb801-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 2/8] drm/virtio: allocate a fence context for every 3D context

[Gurchetan Singh]

We don't want fences from different 3D contexts (GL, VK) to
be on the same timeline.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h | 1 +
 drivers/gpu/drm/virtio/virtgpu_kms.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 9627cd08f5be..b4f85c5fedb9 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -215,6 +215,7 @@ struct virtio_gpu_device {
 struct virtio_gpu_fpriv {
 	uint32_t ctx_id;
 	bool context_created;
+	uint64_t fence_context;
 	struct mutex context_lock;
 };
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 023a030ca7b9..76b0f18e6691 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -267,6 +267,7 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
 	}
 
 	vfpriv->ctx_id = handle + 1;
+	vfpriv->fence_context = dma_fence_context_alloc(1);
 	file->driver_priv = vfpriv;
 	return 0;
 }
-- 
2.25.1.481.gfbce0eb801-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 3/8] drm/virtio: plumb virtio_gpu_fpriv to virtio_gpu_fence_alloc

[Gurchetan Singh]

We'll need it when initiating a dma-fence.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   | 4 ++--
 drivers/gpu/drm/virtio/virtgpu_fence.c | 3 ++-
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 9 +++++----
 drivers/gpu/drm/virtio/virtgpu_plane.c | 2 +-
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index b4f85c5fedb9..76223e2aa68d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -355,8 +355,8 @@ struct drm_plane *virtio_gpu_plane_init(struct virtio_gpu_device *vgdev,
 					int index);
 
 /* virtio_gpu_fence.c */
-struct virtio_gpu_fence *virtio_gpu_fence_alloc(
-	struct virtio_gpu_device *vgdev);
+struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device *vgdev,
+						struct virtio_gpu_fpriv *fpriv);
 void virtio_gpu_fence_emit(struct virtio_gpu_device *vgdev,
 			  struct virtio_gpu_ctrl_hdr *cmd_hdr,
 			  struct virtio_gpu_fence *fence);
diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
index 2fe9c7ebcbd4..f0a7ef80e484 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -73,7 +73,8 @@ static const struct dma_fence_ops virtio_fence_ops = {
 	.timeline_value_str  = virtio_timeline_value_str,
 };
 
-struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device *vgdev)
+struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device *vgdev,
+						struct virtio_gpu_fpriv *fpriv)
 {
 	struct virtio_gpu_fence_driver *drv = &vgdev->fence_drv;
 	struct virtio_gpu_fence *fence = kzalloc(sizeof(struct virtio_gpu_fence),
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 336cc9143205..4860dcb3b3bb 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -160,7 +160,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
 			goto out_memdup;
 	}
 
-	out_fence = virtio_gpu_fence_alloc(vgdev);
+	out_fence = virtio_gpu_fence_alloc(vgdev, vfpriv);
 	if(!out_fence) {
 		ret = -ENOMEM;
 		goto out_unresv;
@@ -226,6 +226,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 					    struct drm_file *file)
 {
 	struct virtio_gpu_device *vgdev = dev->dev_private;
+	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
 	struct drm_virtgpu_resource_create *rc = data;
 	struct virtio_gpu_fence *fence;
 	int ret;
@@ -265,7 +266,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 	if (params.size == 0)
 		params.size = PAGE_SIZE;
 
-	fence = virtio_gpu_fence_alloc(vgdev);
+	fence = virtio_gpu_fence_alloc(vgdev, vfpriv);
 	if (!fence)
 		return -ENOMEM;
 	ret = virtio_gpu_object_create(vgdev, &params, &qobj, fence);
@@ -329,7 +330,7 @@ static int virtio_gpu_transfer_from_host_ioctl(struct drm_device *dev,
 	if (ret != 0)
 		goto err_put_free;
 
-	fence = virtio_gpu_fence_alloc(vgdev);
+	fence = virtio_gpu_fence_alloc(vgdev, vfpriv);
 	if (!fence) {
 		ret = -ENOMEM;
 		goto err_unlock;
@@ -375,7 +376,7 @@ static int virtio_gpu_transfer_to_host_ioctl(struct drm_device *dev, void *data,
 			goto err_put_free;
 
 		ret = -ENOMEM;
-		fence = virtio_gpu_fence_alloc(vgdev);
+		fence = virtio_gpu_fence_alloc(vgdev, vfpriv);
 		if (!fence)
 			goto err_unlock;
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c b/drivers/gpu/drm/virtio/virtgpu_plane.c
index 52d24179bcec..0cf43936abb3 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -202,7 +202,7 @@ static int virtio_gpu_cursor_prepare_fb(struct drm_plane *plane,
 	vgfb = to_virtio_gpu_framebuffer(new_state->fb);
 	bo = gem_to_virtio_gpu_obj(vgfb->base.obj[0]);
 	if (bo && bo->dumb && (plane->state->fb != new_state->fb)) {
-		vgfb->fence = virtio_gpu_fence_alloc(vgdev);
+		vgfb->fence = virtio_gpu_fence_alloc(vgdev, NULL);
 		if (!vgfb->fence)
 			return -ENOMEM;
 	}
-- 
2.25.1.481.gfbce0eb801-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 4/8] drm/virtio: rename sync_seq and last_seq

[Gurchetan Singh]

To be clearer about our intentions to differentiate per-context
sequence numbers and fence IDs, let's rename these variables.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/gpu/drm/virtio/virtgpu_debugfs.c | 4 ++--
 drivers/gpu/drm/virtio/virtgpu_drv.h     | 4 ++--
 drivers/gpu/drm/virtio/virtgpu_fence.c   | 9 +++++----
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_debugfs.c b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
index e27120d512b0..807fe7e9e4d9 100644
--- a/drivers/gpu/drm/virtio/virtgpu_debugfs.c
+++ b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
@@ -60,8 +60,8 @@ virtio_gpu_debugfs_irq_info(struct seq_file *m, void *data)
 	struct virtio_gpu_device *vgdev = node->minor->dev->dev_private;
 
 	seq_printf(m, "fence %llu %lld\n",
-		   (u64)atomic64_read(&vgdev->fence_drv.last_seq),
-		   vgdev->fence_drv.sync_seq);
+		   (u64)atomic64_read(&vgdev->fence_drv.last_fence_id),
+		   vgdev->fence_drv.current_fence_id);
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 76223e2aa68d..7d96d0fdcbac 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -98,8 +98,8 @@ typedef void (*virtio_gpu_resp_cb)(struct virtio_gpu_device *vgdev,
 				   struct virtio_gpu_vbuffer *vbuf);
 
 struct virtio_gpu_fence_driver {
-	atomic64_t       last_seq;
-	uint64_t         sync_seq;
+	atomic64_t       last_fence_id;
+	uint64_t         current_fence_id;
 	uint64_t         context;
 	struct list_head fences;
 	spinlock_t       lock;
diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
index f0a7ef80e484..0c32f3f72737 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -48,7 +48,7 @@ static bool virtio_fence_signaled(struct dma_fence *f)
 		/* leaked fence outside driver before completing
 		 * initialization with virtio_gpu_fence_emit */
 		return false;
-	if (atomic64_read(&fence->drv->last_seq) >= fence->f.seqno)
+	if (atomic64_read(&fence->drv->last_fence_id) >= fence->f.seqno)
 		return true;
 	return false;
 }
@@ -62,7 +62,8 @@ static void virtio_timeline_value_str(struct dma_fence *f, char *str, int size)
 {
 	struct virtio_gpu_fence *fence = to_virtio_fence(f);
 
-	snprintf(str, size, "%llu", (u64)atomic64_read(&fence->drv->last_seq));
+	snprintf(str, size, "%llu",
+		(u64)atomic64_read(&fence->drv->last_fence_id));
 }
 
 static const struct dma_fence_ops virtio_fence_ops = {
@@ -101,7 +102,7 @@ void virtio_gpu_fence_emit(struct virtio_gpu_device *vgdev,
 	unsigned long irq_flags;
 
 	spin_lock_irqsave(&drv->lock, irq_flags);
-	fence->f.seqno = ++drv->sync_seq;
+	fence->f.seqno = ++drv->current_fence_id;
 	dma_fence_get(&fence->f);
 	list_add_tail(&fence->node, &drv->fences);
 	spin_unlock_irqrestore(&drv->lock, irq_flags);
@@ -120,7 +121,7 @@ void virtio_gpu_fence_event_process(struct virtio_gpu_device *vgdev,
 	unsigned long irq_flags;
 
 	spin_lock_irqsave(&drv->lock, irq_flags);
-	atomic64_set(&vgdev->fence_drv.last_seq, fence_id);
+	atomic64_set(&vgdev->fence_drv.last_fence_id, fence_id);
 	list_for_each_entry_safe(fence, tmp, &drv->fences, node) {
 		if (fence_id < fence->f.seqno)
 			continue;
-- 
2.25.1.481.gfbce0eb801-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 5/8] drm/virtio: track fence_id in virtio_gpu_fence

[Gurchetan Singh]

Each fence should be associated with a [fence ID, context ID,
seqno].

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   | 1 +
 drivers/gpu/drm/virtio/virtgpu_fence.c | 5 +++--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 7d96d0fdcbac..e98d1a4cbda9 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -107,6 +107,7 @@ struct virtio_gpu_fence_driver {
 
 struct virtio_gpu_fence {
 	struct dma_fence f;
+	uint64_t fence_id;
 	struct virtio_gpu_fence_driver *drv;
 	struct list_head node;
 };
diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
index 0c32f3f72737..d63848178a58 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -102,7 +102,8 @@ void virtio_gpu_fence_emit(struct virtio_gpu_device *vgdev,
 	unsigned long irq_flags;
 
 	spin_lock_irqsave(&drv->lock, irq_flags);
-	fence->f.seqno = ++drv->current_fence_id;
+	fence->fence_id = ++drv->current_fence_id;
+	fence->f.seqno = fence->fence_id;
 	dma_fence_get(&fence->f);
 	list_add_tail(&fence->node, &drv->fences);
 	spin_unlock_irqrestore(&drv->lock, irq_flags);
@@ -110,7 +111,7 @@ void virtio_gpu_fence_emit(struct virtio_gpu_device *vgdev,
 	trace_dma_fence_emit(&fence->f);
 
 	cmd_hdr->flags |= cpu_to_le32(VIRTIO_GPU_FLAG_FENCE);
-	cmd_hdr->fence_id = cpu_to_le64(fence->f.seqno);
+	cmd_hdr->fence_id = cpu_to_le64(fence->fence_id);
 }
 
 void virtio_gpu_fence_event_process(struct virtio_gpu_device *vgdev,
-- 
2.25.1.481.gfbce0eb801-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 6/8] virtio/drm: rework virtio_fence_signaled

[Gurchetan Singh]

We signal the fences ourselves in virtio_gpu_fence_event_process,
shortly after we set last_fence_id. The window of time is small
enough such that it may be possible to return false in this optional
callback and rely on DMA_FENCE_FLAG_SIGNALED_BIT.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/gpu/drm/virtio/virtgpu_fence.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
index d63848178a58..a63a383347c4 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -43,13 +43,10 @@ static const char *virtio_get_timeline_name(struct dma_fence *f)
 static bool virtio_fence_signaled(struct dma_fence *f)
 {
 	struct virtio_gpu_fence *fence = to_virtio_fence(f);
-
-	if (WARN_ON_ONCE(fence->f.seqno == 0))
-		/* leaked fence outside driver before completing
-		 * initialization with virtio_gpu_fence_emit */
-		return false;
-	if (atomic64_read(&fence->drv->last_fence_id) >= fence->f.seqno)
-		return true;
+	/* leaked fence outside driver before completing
+	 * initialization with virtio_gpu_fence_emit
+	 */
+	WARN_ON_ONCE(fence->fence_id == 0);
 	return false;
 }
 
-- 
2.25.1.481.gfbce0eb801-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 7/8] drm/virtio: check context when signaling

[Gurchetan Singh]

This change:
	- Lookups virtgpu_fence given a fence_id
	- Signals all prior fences in a given context
	- Signals current fence

No functional changes yet, since all fences are initialized from
context 0.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/gpu/drm/virtio/virtgpu_fence.c | 27 ++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
index a63a383347c4..a6c6f498e79e 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -115,17 +115,32 @@ void virtio_gpu_fence_event_process(struct virtio_gpu_device *vgdev,
 				    u64 fence_id)
 {
 	struct virtio_gpu_fence_driver *drv = &vgdev->fence_drv;
-	struct virtio_gpu_fence *fence, *tmp;
+	struct virtio_gpu_fence *signaled, *curr, *tmp;
 	unsigned long irq_flags;
 
 	spin_lock_irqsave(&drv->lock, irq_flags);
 	atomic64_set(&vgdev->fence_drv.last_fence_id, fence_id);
-	list_for_each_entry_safe(fence, tmp, &drv->fences, node) {
-		if (fence_id < fence->f.seqno)
+	list_for_each_entry_safe(curr, tmp, &drv->fences, node) {
+		if (fence_id != curr->fence_id)
 			continue;
-		dma_fence_signal_locked(&fence->f);
-		list_del(&fence->node);
-		dma_fence_put(&fence->f);
+
+		signaled = curr;
+		list_for_each_entry_safe(curr, tmp, &drv->fences, node) {
+			if (signaled->f.context != curr->f.context)
+				continue;
+
+			if (!dma_fence_is_later(&signaled->f, &curr->f))
+				continue;
+
+			dma_fence_signal_locked(&curr->f);
+			list_del(&curr->node);
+			dma_fence_put(&curr->f);
+		}
+
+		dma_fence_signal_locked(&signaled->f);
+		list_del(&signaled->node);
+		dma_fence_put(&signaled->f);
+		break;
 	}
 	spin_unlock_irqrestore(&drv->lock, irq_flags);
 }
-- 
2.25.1.481.gfbce0eb801-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC PATCH 8/8] drm/virtio: enable per context fencing

[Gurchetan Singh]

If there's a 3D context available, initialize the fence using
the 3D context's fence context.

We can't process just the largest fence id anymore, since it's
not the same as the sequence number now.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/gpu/drm/virtio/virtgpu_fence.c | 15 ++++++++++++---
 drivers/gpu/drm/virtio/virtgpu_vq.c    |  4 +---
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
index a6c6f498e79e..bfcb41bcaa68 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -30,6 +30,8 @@
 #define to_virtio_fence(x) \
 	container_of(x, struct virtio_gpu_fence, f)
 
+static atomic_t fence_seq = ATOMIC_INIT(0);
+
 static const char *virtio_get_driver_name(struct dma_fence *f)
 {
 	return "virtio_gpu";
@@ -52,7 +54,7 @@ static bool virtio_fence_signaled(struct dma_fence *f)
 
 static void virtio_fence_value_str(struct dma_fence *f, char *str, int size)
 {
-	snprintf(str, size, "%llu", f->seqno);
+	snprintf(str, size, "[%llu, %llu]", f->context, f->seqno);
 }
 
 static void virtio_timeline_value_str(struct dma_fence *f, char *str, int size)
@@ -74,9 +76,11 @@ static const struct dma_fence_ops virtio_fence_ops = {
 struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device *vgdev,
 						struct virtio_gpu_fpriv *fpriv)
 {
+	uint64_t context;
 	struct virtio_gpu_fence_driver *drv = &vgdev->fence_drv;
 	struct virtio_gpu_fence *fence = kzalloc(sizeof(struct virtio_gpu_fence),
 							GFP_KERNEL);
+
 	if (!fence)
 		return fence;
 
@@ -86,7 +90,13 @@ struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device *vgdev,
 	 * unknown yet.  The fence must not be used outside of the driver
 	 * until virtio_gpu_fence_emit is called.
 	 */
-	dma_fence_init(&fence->f, &virtio_fence_ops, &drv->lock, drv->context, 0);
+	if (fpriv)
+		context = fpriv->fence_context;
+	else
+		context = drv->context;
+
+	dma_fence_init(&fence->f, &virtio_fence_ops, &drv->lock, context,
+		       atomic_inc_return(&fence_seq));
 
 	return fence;
 }
@@ -100,7 +110,6 @@ void virtio_gpu_fence_emit(struct virtio_gpu_device *vgdev,
 
 	spin_lock_irqsave(&drv->lock, irq_flags);
 	fence->fence_id = ++drv->current_fence_id;
-	fence->f.seqno = fence->fence_id;
 	dma_fence_get(&fence->f);
 	list_add_tail(&fence->node, &drv->fences);
 	spin_unlock_irqrestore(&drv->lock, irq_flags);
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 73854915ec34..e7d8b2398628 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -239,6 +239,7 @@ void virtio_gpu_dequeue_ctrl_func(struct work_struct *work)
 					  __func__, fence_id, f);
 			} else {
 				fence_id = f;
+				virtio_gpu_fence_event_process(vgdev, fence_id);
 			}
 		}
 		if (entry->resp_cb)
@@ -246,9 +247,6 @@ void virtio_gpu_dequeue_ctrl_func(struct work_struct *work)
 	}
 	wake_up(&vgdev->ctrlq.ack_queue);
 
-	if (fence_id)
-		virtio_gpu_fence_event_process(vgdev, fence_id);
-
 	list_for_each_entry_safe(entry, tmp, &reclaim_list, list) {
 		if (entry->objs)
 			virtio_gpu_array_put_free_delayed(vgdev, entry->objs);
-- 
2.25.1.481.gfbce0eb801-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/8] *** Per context fencing ***

[Gerd Hoffmann]

On Mon, Mar 09, 2020 at 06:08:10PM -0700, Gurchetan Singh wrote:
> We don't want fences from different 3D contexts/processes (GL, VK) to
> be on the same timeline. Sending this out as a RFC to solicit feedback
> on the general approach.

NACK.

virtio fences are global, period.  You can't simply change fence
semantics like this.  At least not without a virtio protocol update
because guest and host need to be on the same page here.  Just look at
virgl_renderer_callbacks->write_fences() and how it doesn't consider
contexts at all.

So one way out would be to add a virtio feature flag for this, so guest
& host can negotiate whenever fences are global or per-context.

Another approach would be to go multiqueue, with each virtqueue being
roughly the same as a rendering pipeline on physical hardware, then have
per-virtqueue fences.

When going multiqueue we might also rethink the cursor queue approach.
I think it makes sense to simply allow sending cursor commands to any
queue then.  A separate cursor queue continues to be an option for the
guest then, but I'm not sure how useful that actually is in practice
given that cursor image updates are regular resource updates and have to
go through the control queue, so virtio_gpu_cursor_plane_update() has to
wait for the resource update finish before it can queue the cursor
command.  I suspect the idea to fast-track cursor updates with a
separate queue doesn't work that well in practice because of that.

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/8] *** Per context fencing ***

[Gurchetan Singh]

[-- Attachment #1.1: Type: text/plain, Size: 3339 bytes --]

On Tue, Mar 10, 2020 at 12:43 AM Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Mon, Mar 09, 2020 at 06:08:10PM -0700, Gurchetan Singh wrote:
> > We don't want fences from different 3D contexts/processes (GL, VK) to
> > be on the same timeline. Sending this out as a RFC to solicit feedback
> > on the general approach.
>
> NACK.
>
> virtio fences are global, period.  You can't simply change fence
> semantics like this.  At least not without a virtio protocol update
> because guest and host need to be on the same page here.


I should've been more clear -- this is an internal cleanup/preparation and
the per-context changes are invisible to host userspace.

Just look at
> virgl_renderer_callbacks->write_fences() and how it doesn't consider
> contexts at all.
>
> So one way out would be to add a virtio feature flag for this, so guest
> & host can negotiate whenever fences are global or per-context.
>

Yeah, we'll need something like VIRTIO_GPU_FLAG_FENCE_V2 eventually, which
means fences virgl_write_fence can not assume fence_id is the global
sequence number.  It's a bit tricky, and we have to consider a few cases:

1) Current kernel/current host userspace.  Everything works.

2) Newer kernel (with this series) on current host userspace.  For
that, fence_id needs to be a monotonically increasing value, which it
remains to be.  I ran the standard test apps (Unigine Valley, dEQP, etc.)
with this change and things seem fine.

3) Current kernel on newer host userspace.  New host won't see
VIRTIO_GPU_FLAG_FENCE_V2, everything should work as usual.

4) Newer kernel on new host host userspace.  virgl_write_fence signals
fences only in a specific context (or possibly only one fence at a time).
The guest kernel processing based on {fence_id, fence_context} will make a
difference in a multi-process environment.

If I have things right (and again, it's a bit tricky), so the virtio
protocol update will be required at (4).  It would be nice to get in
refactorings to avoid mega-changes if we agree on the general approach..

Side note:

Fences do have an associated context ID in virglrenderer [1], though we
don't pass down the correct ctx ID just yet [2].

[1]
https://gitlab.freedesktop.org/virgl/virglrenderer/-/blob/master/src/virglrenderer.h#L204
[2] https://github.com/qemu/qemu/blob/master/hw/display/virtio-gpu-3d.c#L490

Another approach would be to go multiqueue, with each virtqueue being
> roughly the same as a rendering pipeline on physical hardware, then have
> per-virtqueue fences.
>

Multi-queue sounds very interesting indeed, especially with VK
multi-threaded command submission.  That to me is V3 rather than V2.. let's
start easy!

When going multiqueue we might also rethink the cursor queue approach.
> I think it makes sense to simply allow sending cursor commands to any
> queue then.  A separate cursor queue continues to be an option for the
> guest then, but I'm not sure how useful that actually is in practice
> given that cursor image updates are regular resource updates and have to
> go through the control queue, so virtio_gpu_cursor_plane_update() has to
> wait for the resource update finish before it can queue the cursor
> command.  I suspect the idea to fast-track cursor updates with a
> separate queue doesn't work that well in practice because of that.
>
> cheers,
>   Gerd
>
>

[-- Attachment #1.2: Type: text/html, Size: 4798 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/8] *** Per context fencing ***

[Gerd Hoffmann]

  Hi,

> I should've been more clear -- this is an internal cleanup/preparation and
> the per-context changes are invisible to host userspace.

Ok, it wasn't clear that you don't flip the switch yet.  In general the
commit messages could be a bit more verbose ...

I'm wondering though why we need the new fence_id in the first place.
Isn't it enough to have per-context (instead of global) last_seq?

> Multi-queue sounds very interesting indeed, especially with VK
> multi-threaded command submission.  That to me is V3 rather than V2.. let's
> start easy!

Having v2 if we plan to obsolete it with v3 soon doesn't look like a
good plan to me.  It'll make backward compatibility more complex for
no good reason ...

Also: Does virglrenderer render different contexts in parallel today?
Only in case it does we'll actually get benefits from per-context
fences.  But I think it doesn't, so there is no need to rush.

I think we should better have a rough plan for parallel rendering first,
then go start implementing the pieces needed.

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/8] *** Per context fencing ***

[Chia-I Wu]

On Wed, Mar 11, 2020 at 3:36 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > I should've been more clear -- this is an internal cleanup/preparation and
> > the per-context changes are invisible to host userspace.
>
> Ok, it wasn't clear that you don't flip the switch yet.  In general the
> commit messages could be a bit more verbose ...
>
> I'm wondering though why we need the new fence_id in the first place.
> Isn't it enough to have per-context (instead of global) last_seq?
>
> > Multi-queue sounds very interesting indeed, especially with VK
> > multi-threaded command submission.  That to me is V3 rather than V2.. let's
> > start easy!
>
> Having v2 if we plan to obsolete it with v3 soon doesn't look like a
> good plan to me.  It'll make backward compatibility more complex for
> no good reason ...
I agree we want to study multi-queue a little bit before doing v2.  If
we do decide that multi-queue will be v3, we should at least design v2
in a forward-compatible way.

Every VK context (or GL context if we go multi-process GL) is
isolated.  I think there will need to be at least one virtqueue for
each VK context.  Can virtqueues be added dynamically?  Or can we have
fixed but enough (e.g., 64) virtqueues?

Multi-threaded command submission is not helped by multi-queue unless
we go with one virtqueue for each VKQueue in a VK context.  Otherwise,
multi-queue only makes context scheduling easier, which is not a
priority yet IMO.


>
> Also: Does virglrenderer render different contexts in parallel today?
> Only in case it does we'll actually get benefits from per-context
> fences.  But I think it doesn't, so there is no need to rush.
>
> I think we should better have a rough plan for parallel rendering first,
> then go start implementing the pieces needed.
It will be soon.  Each VK context will be rendered by a different
renderer process.  Besides, VK contexts and GL contexts are not on the
same timeline.  We don't want one to delay another by presenting a
unified timeline.


>
> cheers,
>   Gerd
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/8] *** Per context fencing ***

[Gurchetan Singh]

[-- Attachment #1.1: Type: text/plain, Size: 1581 bytes --]

On Wed, Mar 11, 2020 at 3:36 AM Gerd Hoffmann <kraxel@redhat.com> wrote:

>   Hi,
>
> > I should've been more clear -- this is an internal cleanup/preparation
> and
> > the per-context changes are invisible to host userspace.
>
> Ok, it wasn't clear that you don't flip the switch yet.  In general the
> commit messages could be a bit more verbose ...
>
> I'm wondering though why we need the new fence_id in the first place.
> Isn't it enough to have per-context (instead of global) last_seq?
>

Heh, that was to leave open the possibility of multiple timelines per
context.  Roughly speaking,

V2 -- multiple processes
V3 -- multiple processes and multiple threads (due to VK multi-threaded
command buffers)

I think we all agree on V2.  It seems we still have to discuss V3
(multi-queue, thread pools, a fence context associated with each thread) a
bit more before we start landing pieces.


> > Multi-queue sounds very interesting indeed, especially with VK
> > multi-threaded command submission.  That to me is V3 rather than V2..
> let's
> > start easy!
>
> Having v2 if we plan to obsolete it with v3 soon doesn't look like a
> good plan to me.  It'll make backward compatibility more complex for
> no good reason ...
>
> Also: Does virglrenderer render different contexts in parallel today?
> Only in case it does we'll actually get benefits from per-context
> fences.  But I think it doesn't, so there is no need to rush.
>
> I think we should better have a rough plan for parallel rendering first,
> then go start implementing the pieces needed.
>
> cheers,
>   Gerd
>
>

[-- Attachment #1.2: Type: text/html, Size: 2270 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/8] *** Per context fencing ***

[Chia-I Wu]

On Wed, Mar 11, 2020 at 4:36 PM Gurchetan Singh
<gurchetansingh@chromium.org> wrote:
>
>
>
> On Wed, Mar 11, 2020 at 3:36 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>>
>>   Hi,
>>
>> > I should've been more clear -- this is an internal cleanup/preparation and
>> > the per-context changes are invisible to host userspace.
>>
>> Ok, it wasn't clear that you don't flip the switch yet.  In general the
>> commit messages could be a bit more verbose ...
>>
>> I'm wondering though why we need the new fence_id in the first place.
>> Isn't it enough to have per-context (instead of global) last_seq?
>
>
> Heh, that was to leave open the possibility of multiple timelines per context.  Roughly speaking,
Yeah, I think we will need multiple timelines per context.

>
> V2 -- multiple processes
> V3 -- multiple processes and multiple threads (due to VK multi-threaded command buffers)
>
> I think we all agree on V2.  It seems we still have to discuss V3 (multi-queue, thread pools, a fence context associated with each thread) a bit more before we start landing pieces.
In addition to multiple threads, we should also consider multiple VkQueues.

I will start with... how many timelines do we want to expose per
context?  In my mind, it goes like

V1: 1 timeline per virtqueue (there is one timeline for ctrlq right now)
V2: 1 timeline per context (VK and GL on different timelines)
V3: N timelines per context (each VkQueue in a VK context gets a timeline?)
V4: N+M timelines per context (each CPU thread also gets a timeline?!?!)

I certainly don't know if V4 is a good idea or not...



>
>>
>> > Multi-queue sounds very interesting indeed, especially with VK
>> > multi-threaded command submission.  That to me is V3 rather than V2.. let's
>> > start easy!
>>
>> Having v2 if we plan to obsolete it with v3 soon doesn't look like a
>> good plan to me.  It'll make backward compatibility more complex for
>> no good reason ...
>>
>> Also: Does virglrenderer render different contexts in parallel today?
>> Only in case it does we'll actually get benefits from per-context
>> fences.  But I think it doesn't, so there is no need to rush.
>>
>> I think we should better have a rough plan for parallel rendering first,
>> then go start implementing the pieces needed.
>>
>> cheers,
>>   Gerd
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/8] *** Per context fencing ***

[Gerd Hoffmann]

  Hi,

> Can virtqueues be added dynamically?

No.

> Or can we have
> fixed but enough (e.g., 64) virtqueues?

Well, I wouldn't bet that any specific number is enough.  When gtk
starts rendering using vulkan by default the number of contexts of a
standard gnome desktop will be pretty high I guess ...

We can have a host-configurable number of virtqueues.  Typically the
guest can figure the number of available queues from config space.  One
common usage pattern (seen in block/net devices) is to have the number
of virtqueues equal the number of vcpus, so each vcpu has dedicated
virtqueue.

For virtio-gpu it is probably more useful to bind contexts to
virtqueues.  As long as we have enough virtqueues each context can
have its own.  Once we run out of virtqueues we have to start sharing.

On the host side it probably makes sense to have one process per
virtqueue.

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/8] *** Per context fencing ***

[Gerd Hoffmann]

  Hi,

> I will start with... how many timelines do we want to expose per
> context?  In my mind, it goes like
> 
> V1: 1 timeline per virtqueue (there is one timeline for ctrlq right now)
> V2: 1 timeline per context (VK and GL on different timelines)
> V3: N timelines per context (each VkQueue in a VK context gets a timeline?)
> V4: N+M timelines per context (each CPU thread also gets a timeline?!?!)
> 
> I certainly don't know if V4 is a good idea or not...

I'd expect apps use one VkQueue per thread, so v3 makes sense but v4 not
so much I think ...

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/8] *** Per context fencing ***

[Gerd Hoffmann]

On Wed, Mar 11, 2020 at 04:36:16PM -0700, Gurchetan Singh wrote:
> On Wed, Mar 11, 2020 at 3:36 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> 
> >   Hi,
> >
> > > I should've been more clear -- this is an internal cleanup/preparation
> > and
> > > the per-context changes are invisible to host userspace.
> >
> > Ok, it wasn't clear that you don't flip the switch yet.  In general the
> > commit messages could be a bit more verbose ...
> >
> > I'm wondering though why we need the new fence_id in the first place.
> > Isn't it enough to have per-context (instead of global) last_seq?
> >
> 
> Heh, that was to leave open the possibility of multiple timelines per
> context.  Roughly speaking,
> 
> V2 -- multiple processes
> V3 -- multiple processes and multiple threads (due to VK multi-threaded
> command buffers)
> 
> I think we all agree on V2.  It seems we still have to discuss V3
> (multi-queue, thread pools, a fence context associated with each thread) a
> bit more before we start landing pieces.

While thinking about the whole thing a bit more ...
Do we need virtio_gpu_ctrl_hdr->fence_id at all?

At virtio level it is pretty simple:  The host completes the SUBMIT_3D
virtio command when it finished rendering, period.

On the guest side we don't need the fence_id.  The completion callback
gets passed the virtio_gpu_vbuffer, so it can figure which command did
actually complete without looking at virtio_gpu_ctrl_hdr->fence_id.

On the host side we depend on the fence_id right now, but only because
that is the way the virgl_renderer_callbacks->write_fence interface is
designed.  We have to change that anyway for per-context (or whatever)
fences, so it should not be a problem to drop the fence_id dependency
too and just pass around an opaque pointer instead.

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/8] *** Per context fencing ***

[Gurchetan Singh]

[-- Attachment #1.1: Type: text/plain, Size: 2406 bytes --]

On Thu, Mar 12, 2020 at 2:29 AM Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Wed, Mar 11, 2020 at 04:36:16PM -0700, Gurchetan Singh wrote:
> > On Wed, Mar 11, 2020 at 3:36 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > >   Hi,
> > >
> > > > I should've been more clear -- this is an internal
> cleanup/preparation
> > > and
> > > > the per-context changes are invisible to host userspace.
> > >
> > > Ok, it wasn't clear that you don't flip the switch yet.  In general the
> > > commit messages could be a bit more verbose ...
> > >
> > > I'm wondering though why we need the new fence_id in the first place.
> > > Isn't it enough to have per-context (instead of global) last_seq?
> > >
> >
> > Heh, that was to leave open the possibility of multiple timelines per
> > context.  Roughly speaking,
> >
> > V2 -- multiple processes
> > V3 -- multiple processes and multiple threads (due to VK multi-threaded
> > command buffers)
> >
> > I think we all agree on V2.  It seems we still have to discuss V3
> > (multi-queue, thread pools, a fence context associated with each thread)
> a
> > bit more before we start landing pieces.
>
> While thinking about the whole thing a bit more ...
> Do we need virtio_gpu_ctrl_hdr->fence_id at all?
>

A fence ID could be useful for sharing fences across virtio devices.  Think
FENCE_ASSIGN_UUID, akin to  RESOURCE_ASSIGN_UUID (+dstevens@).


> At virtio level it is pretty simple:  The host completes the SUBMIT_3D
> virtio command when it finished rendering, period.


> On the guest side we don't need the fence_id.  The completion callback
> gets passed the virtio_gpu_vbuffer, so it can figure which command did
> actually complete without looking at virtio_gpu_ctrl_hdr->fence_id.
>
> On the host side we depend on the fence_id right now, but only because
> that is the way the virgl_renderer_callbacks->write_fence interface is
> designed.  We have to change that anyway for per-context (or whatever)
> fences, so it should not be a problem to drop the fence_id dependency
> too and just pass around an opaque pointer instead.
>

For multiple GPU timelines per context, the (process local) sync object
handle looks interesting:

https://patchwork.kernel.org/patch/9758565/

Some have extended EXECBUFFER to support this flow:

https://patchwork.freedesktop.org/patch/msgid/1499289202-25441-1-git-send-email-jason.ekstrand@intel.com


> cheers,
>   Gerd
>
>

[-- Attachment #1.2: Type: text/html, Size: 3841 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/8] *** Per context fencing ***

[Chia-I Wu]

On Thu, Mar 12, 2020 at 4:08 PM Gurchetan Singh
<gurchetansingh@chromium.org> wrote:
>
>
>
> On Thu, Mar 12, 2020 at 2:29 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>>
>> On Wed, Mar 11, 2020 at 04:36:16PM -0700, Gurchetan Singh wrote:
>> > On Wed, Mar 11, 2020 at 3:36 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
>> >
>> > >   Hi,
>> > >
>> > > > I should've been more clear -- this is an internal cleanup/preparation
>> > > and
>> > > > the per-context changes are invisible to host userspace.
>> > >
>> > > Ok, it wasn't clear that you don't flip the switch yet.  In general the
>> > > commit messages could be a bit more verbose ...
>> > >
>> > > I'm wondering though why we need the new fence_id in the first place.
>> > > Isn't it enough to have per-context (instead of global) last_seq?
>> > >
>> >
>> > Heh, that was to leave open the possibility of multiple timelines per
>> > context.  Roughly speaking,
>> >
>> > V2 -- multiple processes
>> > V3 -- multiple processes and multiple threads (due to VK multi-threaded
>> > command buffers)
>> >
>> > I think we all agree on V2.  It seems we still have to discuss V3
>> > (multi-queue, thread pools, a fence context associated with each thread) a
>> > bit more before we start landing pieces.
>>
>> While thinking about the whole thing a bit more ...
>> Do we need virtio_gpu_ctrl_hdr->fence_id at all?
>
>
> A fence ID could be useful for sharing fences across virtio devices.  Think FENCE_ASSIGN_UUID, akin to  RESOURCE_ASSIGN_UUID (+dstevens@).
>
>>
>> At virtio level it is pretty simple:  The host completes the SUBMIT_3D
>> virtio command when it finished rendering, period.
>>
>>
>> On the guest side we don't need the fence_id.  The completion callback
>> gets passed the virtio_gpu_vbuffer, so it can figure which command did
>> actually complete without looking at virtio_gpu_ctrl_hdr->fence_id.
>>
>> On the host side we depend on the fence_id right now, but only because
>> that is the way the virgl_renderer_callbacks->write_fence interface is
>> designed.  We have to change that anyway for per-context (or whatever)
>> fences, so it should not be a problem to drop the fence_id dependency
>> too and just pass around an opaque pointer instead.

I am still catching up, but IIUC, indeed I don't think the host needs
to depend on fence_id.  We should be able to repurpose fence_id.  On
the other hand, the VIRTIO_GPU_FLAG_FENCE flag is interesting, and it
indicates that the vbuf is on the host GPU timeline instead of the
host CPU timeline.

>
>
> For multiple GPU timelines per context, the (process local) sync object handle looks interesting:
>
> https://patchwork.kernel.org/patch/9758565/
>
> Some have extended EXECBUFFER to support this flow:
>
> https://patchwork.freedesktop.org/patch/msgid/1499289202-25441-1-git-send-email-jason.ekstrand@intel.com

I think this only affects the kernel/userspace interface?  I know
there were works being done to support VK_KHR_timeline semaphore,
which is something we definitely want.  I don't know if it is the only
way for the userspace to gain the extension support.  I need to do my
homework...



>
>>
>> cheers,
>>   Gerd
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/8] *** Per context fencing ***

[Gerd Hoffmann]

  Hi,

> >> At virtio level it is pretty simple:  The host completes the SUBMIT_3D
> >> virtio command when it finished rendering, period.
> >>
> >>
> >> On the guest side we don't need the fence_id.  The completion callback
> >> gets passed the virtio_gpu_vbuffer, so it can figure which command did
> >> actually complete without looking at virtio_gpu_ctrl_hdr->fence_id.
> >>
> >> On the host side we depend on the fence_id right now, but only because
> >> that is the way the virgl_renderer_callbacks->write_fence interface is
> >> designed.  We have to change that anyway for per-context (or whatever)
> >> fences, so it should not be a problem to drop the fence_id dependency
> >> too and just pass around an opaque pointer instead.
> 
> I am still catching up, but IIUC, indeed I don't think the host needs
> to depend on fence_id.  We should be able to repurpose fence_id.

I'd rather ignore it altogether for FENCE_V2 (or whatever we call the
feature flag).

> On the other hand, the VIRTIO_GPU_FLAG_FENCE flag is interesting, and
> it indicates that the vbuf is on the host GPU timeline instead of the
> host CPU timeline.

Yep, we have to keep that (unless we do command completion on GPU
timeline unconditionally with FENCE_V2).

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/8] *** Per context fencing ***

[Chia-I Wu]

On Mon, Mar 16, 2020 at 3:44 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> > >> At virtio level it is pretty simple:  The host completes the SUBMIT_3D
> > >> virtio command when it finished rendering, period.
> > >>
> > >>
> > >> On the guest side we don't need the fence_id.  The completion callback
> > >> gets passed the virtio_gpu_vbuffer, so it can figure which command did
> > >> actually complete without looking at virtio_gpu_ctrl_hdr->fence_id.
> > >>
> > >> On the host side we depend on the fence_id right now, but only because
> > >> that is the way the virgl_renderer_callbacks->write_fence interface is
> > >> designed.  We have to change that anyway for per-context (or whatever)
> > >> fences, so it should not be a problem to drop the fence_id dependency
> > >> too and just pass around an opaque pointer instead.
> >
> > I am still catching up, but IIUC, indeed I don't think the host needs
> > to depend on fence_id.  We should be able to repurpose fence_id.
>
> I'd rather ignore it altogether for FENCE_V2 (or whatever we call the
> feature flag).

That's fine when one assumes each virtqueue has one host GPU timeline.
But when there are multiple (e.g., multiplexing multiple contexts over
one virtqueue, or multiple VkQueues), fence_id can be repurposed as
the host timeline id.

>
> > On the other hand, the VIRTIO_GPU_FLAG_FENCE flag is interesting, and
> > it indicates that the vbuf is on the host GPU timeline instead of the
> > host CPU timeline.
>
> Yep, we have to keep that (unless we do command completion on GPU
> timeline unconditionally with FENCE_V2).

I think it will be useful when EXECBUFFER is used for metadata query
and write the metadata directly to a guest BO's sg list.  We want the
query to be on the CPU timeline.




> cheers,
>   Gerd
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC PATCH 0/8] *** Per context fencing ***

[Gerd Hoffmann]

  Hi,

> > > I am still catching up, but IIUC, indeed I don't think the host needs
> > > to depend on fence_id.  We should be able to repurpose fence_id.
> >
> > I'd rather ignore it altogether for FENCE_V2 (or whatever we call the
> > feature flag).
> 
> That's fine when one assumes each virtqueue has one host GPU timeline.
> But when there are multiple (e.g., multiplexing multiple contexts over
> one virtqueue, or multiple VkQueues), fence_id can be repurposed as
> the host timeline id.

Why do you need an id for that?  You can simply keep track of the
submit_3d commands instead.

cheers,
  Gerd

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel