[PATCH 1/3] drm/virtio: virtio_{blah} --> virtio_gpu_{blah}

[PATCH 1/3] drm/virtio: virtio_{blah} --> virtio_gpu_{blah}

[Gurchetan Singh]

virtio_gpu typically uses the prefix virtio_gpu, but there are
a few places where the virtio prefix is used.  Modify this for
consistency.

Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
---
 drivers/gpu/drm/virtio/virtgpu_debugfs.c | 24 ++++++++++--------
 drivers/gpu/drm/virtio/virtgpu_fence.c   | 32 +++++++++++++-----------
 2 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_debugfs.c b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
index 5fefc88d47e4..c2b20e0ee030 100644
--- a/drivers/gpu/drm/virtio/virtgpu_debugfs.c
+++ b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
@@ -28,14 +28,13 @@
 
 #include "virtgpu_drv.h"
 
-static void virtio_add_bool(struct seq_file *m, const char *name,
-				    bool value)
+static void virtio_gpu_add_bool(struct seq_file *m, const char *name,
+				bool value)
 {
 	seq_printf(m, "%-16s : %s\n", name, value ? "yes" : "no");
 }
 
-static void virtio_add_int(struct seq_file *m, const char *name,
-				   int value)
+static void virtio_gpu_add_int(struct seq_file *m, const char *name, int value)
 {
 	seq_printf(m, "%-16s : %d\n", name, value);
 }
@@ -45,13 +44,16 @@ static int virtio_gpu_features(struct seq_file *m, void *data)
 	struct drm_info_node *node = (struct drm_info_node *)m->private;
 	struct virtio_gpu_device *vgdev = node->minor->dev->dev_private;
 
-	virtio_add_bool(m, "virgl", vgdev->has_virgl_3d);
-	virtio_add_bool(m, "edid", vgdev->has_edid);
-	virtio_add_bool(m, "indirect", vgdev->has_indirect);
-	virtio_add_bool(m, "resource uuid", vgdev->has_resource_assign_uuid);
-	virtio_add_bool(m, "blob resources", vgdev->has_resource_blob);
-	virtio_add_int(m, "cap sets", vgdev->num_capsets);
-	virtio_add_int(m, "scanouts", vgdev->num_scanouts);
+	virtio_gpu_add_bool(m, "virgl", vgdev->has_virgl_3d);
+	virtio_gpu_add_bool(m, "edid", vgdev->has_edid);
+	virtio_gpu_add_bool(m, "indirect", vgdev->has_indirect);
+
+	virtio_gpu_add_bool(m, "resource uuid",
+			    vgdev->has_resource_assign_uuid);
+
+	virtio_gpu_add_bool(m, "blob resources", vgdev->has_resource_blob);
+	virtio_gpu_add_int(m, "cap sets", vgdev->num_capsets);
+	virtio_gpu_add_int(m, "scanouts", vgdev->num_scanouts);
 	if (vgdev->host_visible_region.len) {
 		seq_printf(m, "%-16s : 0x%lx +0x%lx\n", "host visible region",
 			   (unsigned long)vgdev->host_visible_region.addr,
diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
index 728ca36f6327..586034c90587 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -27,22 +27,22 @@
 
 #include "virtgpu_drv.h"
 
-#define to_virtio_fence(x) \
+#define to_virtio_gpu_fence(x) \
 	container_of(x, struct virtio_gpu_fence, f)
 
-static const char *virtio_get_driver_name(struct dma_fence *f)
+static const char *virtio_gpu_get_driver_name(struct dma_fence *f)
 {
 	return "virtio_gpu";
 }
 
-static const char *virtio_get_timeline_name(struct dma_fence *f)
+static const char *virtio_gpu_get_timeline_name(struct dma_fence *f)
 {
 	return "controlq";
 }
 
-static bool virtio_fence_signaled(struct dma_fence *f)
+static bool virtio_gpu_fence_signaled(struct dma_fence *f)
 {
-	struct virtio_gpu_fence *fence = to_virtio_fence(f);
+	struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
 
 	if (WARN_ON_ONCE(fence->f.seqno == 0))
 		/* leaked fence outside driver before completing
@@ -53,25 +53,26 @@ static bool virtio_fence_signaled(struct dma_fence *f)
 	return false;
 }
 
-static void virtio_fence_value_str(struct dma_fence *f, char *str, int size)
+static void virtio_gpu_fence_value_str(struct dma_fence *f, char *str, int size)
 {
 	snprintf(str, size, "%llu", f->seqno);
 }
 
-static void virtio_timeline_value_str(struct dma_fence *f, char *str, int size)
+static void virtio_gpu_timeline_value_str(struct dma_fence *f, char *str,
+					  int size)
 {
-	struct virtio_gpu_fence *fence = to_virtio_fence(f);
+	struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
 
 	snprintf(str, size, "%llu",
 		 (u64)atomic64_read(&fence->drv->last_fence_id));
 }
 
-static const struct dma_fence_ops virtio_fence_ops = {
-	.get_driver_name     = virtio_get_driver_name,
-	.get_timeline_name   = virtio_get_timeline_name,
-	.signaled            = virtio_fence_signaled,
-	.fence_value_str     = virtio_fence_value_str,
-	.timeline_value_str  = virtio_timeline_value_str,
+static const struct dma_fence_ops virtio_gpu_fence_ops = {
+	.get_driver_name     = virtio_gpu_get_driver_name,
+	.get_timeline_name   = virtio_gpu_get_timeline_name,
+	.signaled            = virtio_gpu_fence_signaled,
+	.fence_value_str     = virtio_gpu_fence_value_str,
+	.timeline_value_str  = virtio_gpu_timeline_value_str,
 };
 
 struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device *vgdev)
@@ -88,7 +89,8 @@ 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);
+	dma_fence_init(&fence->f, &virtio_gpu_fence_ops, &drv->lock, drv->context,
+		       0);
 
 	return fence;
 }
-- 
2.29.2.454.gaff20da3a2-goog

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

[PATCH 2/3] drm/virtio: rework virtio_fence_signaled

[Gurchetan Singh]

virtio_gpu_fence_event_process sets the last_fence_id and
subsequently calls dma_fence_signal_locked(..).

dma_fence_signal_locked(..) sets DMA_FENCE_FLAG_SIGNALED_BIT,
which is actually checked before &dma_fence_ops.(*signaled) is
called.

The check for last_fence_id is therefore a bit redundant, and
it will not be sufficient to check the last_fence_id for multiple
synchronization timelines.  Remove it.

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

diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
index 586034c90587..b35fcd1d02d7 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -42,14 +42,10 @@ static const char *virtio_gpu_get_timeline_name(struct dma_fence *f)
 
 static bool virtio_gpu_fence_signaled(struct dma_fence *f)
 {
-	struct virtio_gpu_fence *fence = to_virtio_gpu_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(f->seqno == 0);
 	return false;
 }
 
-- 
2.29.2.454.gaff20da3a2-goog

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

[PATCH 3/3] drm/virtio: consider dma-fence context when signaling

[Gurchetan Singh]

This an incremental refactor towards multiple dma-fence contexts
in virtio-gpu.  Since all fences are still allocated using
&virtio_gpu_fence_driver.context, nothing should break and every
processed fence will be signaled.

The overall idea is every 3D context can allocate a number of
dma-fence contexts.  Each dma-fence context refers to it's own
timeline.

For example, consider the following case where virgl submits
commands to the GPU (fence ids 1, 3) and does a metadata query with
the CPU (fence id 5).  In a different process, gfxstream submits
commands to the GPU (fence ids 2, 4).

fence_id (&dma_fence.seqno)       | 1 2 3 4 5
----------------------------------|-----------
fence_ctx 0 (virgl gpu)           | 1   3
fence_ctx 1 (virgl metadata query)|         5
fence_ctx 2 (gfxstream gpu)       |   2   4

With multiple fence contexts, we can wait for the metadata query
to finish without waiting for the virgl gpu to finish.  virgl gpu
does not have to wait for gfxstream gpu.  The fence id still is the
monotonically increasing sequence number, but it's only revelant to
the specific dma-fence context.

To fully enable this feature, we'll need to:
  - have each 3d context allocate a number of fence contexts. Not
    too hard with explicit context initialization on the horizon.
  - have guest userspace specify fence context when performing
    ioctls.
  - tag each fence emitted to the host with the fence context
    information.  virtio_gpu_ctrl_hdr has padding + flags available,
    so that should be easy.

This change goes in the direction specified above, by:
  - looking up the virtgpu_fence given a fence_id
  - signalling all prior fences in a given context
  - signalling current fence

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

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 6a232553c99b..d9dbc4f258f3 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -136,6 +136,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 b35fcd1d02d7..3de09d78dada 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -51,7 +51,7 @@ static bool virtio_gpu_fence_signaled(struct dma_fence *f)
 
 static void virtio_gpu_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_gpu_timeline_value_str(struct dma_fence *f, char *str,
@@ -99,7 +99,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->current_fence_id;
+	fence->fence_id = 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);
@@ -107,24 +107,45 @@ 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,
 				    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;
+
+		/*
+		 * Signal any fences with a strictly smaller sequence number
+		 * the current signaled fence.
+		 */
+		list_for_each_entry_safe(curr, tmp, &drv->fences, node) {
+			/* dma-fence contexts must match */
+			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.29.2.454.gaff20da3a2-goog

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

[PATCH 3/3] drm/virtio: consider dma-fence context when signaling

[Gurchetan Singh]

This an incremental refactor towards multiple dma-fence contexts
in virtio-gpu.  Since all fences are still allocated using
&virtio_gpu_fence_driver.context, nothing should break and every
processed fence will be signaled.

The overall idea is every 3D context can allocate a number of
dma-fence contexts.  Each dma-fence context refers to it's own
timeline.

For example, consider the following case where virgl submits
commands to the GPU (fence ids 1, 3) and does a metadata query with
the CPU (fence id 5).  In a different process, gfxstream submits
commands to the GPU (fence ids 2, 4).

fence_id (&dma_fence.seqno)       | 1 2 3 4 5
----------------------------------|-----------
fence_ctx 0 (virgl gpu)           | 1   3
fence_ctx 1 (virgl metadata query)|         5
fence_ctx 2 (gfxstream gpu)       |   2   4

With multiple fence contexts, we can wait for the metadata query
to finish without waiting for the virgl gpu to finish.  virgl gpu
does not have to wait for gfxstream gpu.  The fence id still is the
monotonically increasing sequence number, but it's only revelant to
the specific dma-fence context.

To fully enable this feature, we'll need to:
  - have each 3d context allocate a number of fence contexts. Not
    too hard with explicit context initialization on the horizon.
  - have guest userspace specify fence context when performing
    ioctls.
  - tag each fence emitted to the host with the fence context
    information.  virtio_gpu_ctrl_hdr has padding + flags available,
    so that should be easy.

This change goes in the direction specified above, by:
  - looking up the virtgpu_fence given a fence_id
  - signalling all prior fences in a given context
  - signalling current fence

v2: fix grammar in comment

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

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 6a232553c99b..d9dbc4f258f3 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -136,6 +136,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 b35fcd1d02d7..d28e25e8409b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
@@ -51,7 +51,7 @@ static bool virtio_gpu_fence_signaled(struct dma_fence *f)
 
 static void virtio_gpu_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_gpu_timeline_value_str(struct dma_fence *f, char *str,
@@ -99,7 +99,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->current_fence_id;
+	fence->fence_id = 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);
@@ -107,24 +107,45 @@ 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,
 				    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;
+
+		/*
+		 * Signal any fences with a strictly smaller sequence number
+		 * than the current signaled fence.
+		 */
+		list_for_each_entry_safe(curr, tmp, &drv->fences, node) {
+			/* dma-fence contexts must match */
+			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.29.2.454.gaff20da3a2-goog

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

Re: [PATCH 1/3] drm/virtio: virtio_{blah} --> virtio_gpu_{blah}

[Anthoine Bourgeois]

On Mon, Nov 23, 2020 at 06:19:00PM -0800, Gurchetan Singh wrote:
>virtio_gpu typically uses the prefix virtio_gpu, but there are
>a few places where the virtio prefix is used.  Modify this for
>consistency.
>
>Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: Anthoine Bourgeois <anthoine.bourgeois@gmail.com>
>---
> drivers/gpu/drm/virtio/virtgpu_debugfs.c | 24 ++++++++++--------
> drivers/gpu/drm/virtio/virtgpu_fence.c   | 32 +++++++++++++-----------
> 2 files changed, 30 insertions(+), 26 deletions(-)
>
>diff --git a/drivers/gpu/drm/virtio/virtgpu_debugfs.c b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
>index 5fefc88d47e4..c2b20e0ee030 100644
>--- a/drivers/gpu/drm/virtio/virtgpu_debugfs.c
>+++ b/drivers/gpu/drm/virtio/virtgpu_debugfs.c
>@@ -28,14 +28,13 @@
>
> #include "virtgpu_drv.h"
>
>-static void virtio_add_bool(struct seq_file *m, const char *name,
>-				    bool value)
>+static void virtio_gpu_add_bool(struct seq_file *m, const char *name,
>+				bool value)
> {
> 	seq_printf(m, "%-16s : %s\n", name, value ? "yes" : "no");
> }
>
>-static void virtio_add_int(struct seq_file *m, const char *name,
>-				   int value)
>+static void virtio_gpu_add_int(struct seq_file *m, const char *name, int value)
> {
> 	seq_printf(m, "%-16s : %d\n", name, value);
> }
>@@ -45,13 +44,16 @@ static int virtio_gpu_features(struct seq_file *m, void *data)
> 	struct drm_info_node *node = (struct drm_info_node *)m->private;
> 	struct virtio_gpu_device *vgdev = node->minor->dev->dev_private;
>
>-	virtio_add_bool(m, "virgl", vgdev->has_virgl_3d);
>-	virtio_add_bool(m, "edid", vgdev->has_edid);
>-	virtio_add_bool(m, "indirect", vgdev->has_indirect);
>-	virtio_add_bool(m, "resource uuid", vgdev->has_resource_assign_uuid);
>-	virtio_add_bool(m, "blob resources", vgdev->has_resource_blob);
>-	virtio_add_int(m, "cap sets", vgdev->num_capsets);
>-	virtio_add_int(m, "scanouts", vgdev->num_scanouts);
>+	virtio_gpu_add_bool(m, "virgl", vgdev->has_virgl_3d);
>+	virtio_gpu_add_bool(m, "edid", vgdev->has_edid);
>+	virtio_gpu_add_bool(m, "indirect", vgdev->has_indirect);
>+
>+	virtio_gpu_add_bool(m, "resource uuid",
>+			    vgdev->has_resource_assign_uuid);
>+
>+	virtio_gpu_add_bool(m, "blob resources", vgdev->has_resource_blob);
>+	virtio_gpu_add_int(m, "cap sets", vgdev->num_capsets);
>+	virtio_gpu_add_int(m, "scanouts", vgdev->num_scanouts);
> 	if (vgdev->host_visible_region.len) {
> 		seq_printf(m, "%-16s : 0x%lx +0x%lx\n", "host visible region",
> 			   (unsigned long)vgdev->host_visible_region.addr,
>diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
>index 728ca36f6327..586034c90587 100644
>--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
>+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
>@@ -27,22 +27,22 @@
>
> #include "virtgpu_drv.h"
>
>-#define to_virtio_fence(x) \
>+#define to_virtio_gpu_fence(x) \
> 	container_of(x, struct virtio_gpu_fence, f)
>
>-static const char *virtio_get_driver_name(struct dma_fence *f)
>+static const char *virtio_gpu_get_driver_name(struct dma_fence *f)
> {
> 	return "virtio_gpu";
> }
>
>-static const char *virtio_get_timeline_name(struct dma_fence *f)
>+static const char *virtio_gpu_get_timeline_name(struct dma_fence *f)
> {
> 	return "controlq";
> }
>
>-static bool virtio_fence_signaled(struct dma_fence *f)
>+static bool virtio_gpu_fence_signaled(struct dma_fence *f)
> {
>-	struct virtio_gpu_fence *fence = to_virtio_fence(f);
>+	struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
>
> 	if (WARN_ON_ONCE(fence->f.seqno == 0))
> 		/* leaked fence outside driver before completing
>@@ -53,25 +53,26 @@ static bool virtio_fence_signaled(struct dma_fence *f)
> 	return false;
> }
>
>-static void virtio_fence_value_str(struct dma_fence *f, char *str, int size)
>+static void virtio_gpu_fence_value_str(struct dma_fence *f, char *str, int size)
> {
> 	snprintf(str, size, "%llu", f->seqno);
> }
>
>-static void virtio_timeline_value_str(struct dma_fence *f, char *str, int size)
>+static void virtio_gpu_timeline_value_str(struct dma_fence *f, char *str,
>+					  int size)
> {
>-	struct virtio_gpu_fence *fence = to_virtio_fence(f);
>+	struct virtio_gpu_fence *fence = to_virtio_gpu_fence(f);
>
> 	snprintf(str, size, "%llu",
> 		 (u64)atomic64_read(&fence->drv->last_fence_id));
> }
>
>-static const struct dma_fence_ops virtio_fence_ops = {
>-	.get_driver_name     = virtio_get_driver_name,
>-	.get_timeline_name   = virtio_get_timeline_name,
>-	.signaled            = virtio_fence_signaled,
>-	.fence_value_str     = virtio_fence_value_str,
>-	.timeline_value_str  = virtio_timeline_value_str,
>+static const struct dma_fence_ops virtio_gpu_fence_ops = {
>+	.get_driver_name     = virtio_gpu_get_driver_name,
>+	.get_timeline_name   = virtio_gpu_get_timeline_name,
>+	.signaled            = virtio_gpu_fence_signaled,
>+	.fence_value_str     = virtio_gpu_fence_value_str,
>+	.timeline_value_str  = virtio_gpu_timeline_value_str,
> };
>
> struct virtio_gpu_fence *virtio_gpu_fence_alloc(struct virtio_gpu_device *vgdev)
>@@ -88,7 +89,8 @@ 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);
>+	dma_fence_init(&fence->f, &virtio_gpu_fence_ops, &drv->lock, drv->context,
>+		       0);
>
> 	return fence;
> }
>-- 
>2.29.2.454.gaff20da3a2-goog
>
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/virtio: rework virtio_fence_signaled

[Anthoine Bourgeois]

On Mon, Nov 23, 2020 at 06:19:01PM -0800, Gurchetan Singh wrote:
>virtio_gpu_fence_event_process sets the last_fence_id and
>subsequently calls dma_fence_signal_locked(..).
>
>dma_fence_signal_locked(..) sets DMA_FENCE_FLAG_SIGNALED_BIT,
>which is actually checked before &dma_fence_ops.(*signaled) is
>called.
>
>The check for last_fence_id is therefore a bit redundant, and
>it will not be sufficient to check the last_fence_id for multiple
>synchronization timelines.  Remove it.
>
>Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: Anthoine Bourgeois <anthoine.bourgeois@gmail.com>
>---
> drivers/gpu/drm/virtio/virtgpu_fence.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
>diff --git a/drivers/gpu/drm/virtio/virtgpu_fence.c b/drivers/gpu/drm/virtio/virtgpu_fence.c
>index 586034c90587..b35fcd1d02d7 100644
>--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
>+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
>@@ -42,14 +42,10 @@ static const char *virtio_gpu_get_timeline_name(struct dma_fence *f)
>
> static bool virtio_gpu_fence_signaled(struct dma_fence *f)
> {
>-	struct virtio_gpu_fence *fence = to_virtio_gpu_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(f->seqno == 0);
> 	return false;
> }
>
>-- 
>2.29.2.454.gaff20da3a2-goog
>
>_______________________________________________
>dri-devel mailing list
>dri-devel@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/3] drm/virtio: consider dma-fence context when signaling

[Anthoine Bourgeois]

On Mon, Nov 23, 2020 at 06:28:17PM -0800, Gurchetan Singh wrote:
>This an incremental refactor towards multiple dma-fence contexts
>in virtio-gpu.  Since all fences are still allocated using
>&virtio_gpu_fence_driver.context, nothing should break and every
>processed fence will be signaled.
>
>The overall idea is every 3D context can allocate a number of
>dma-fence contexts.  Each dma-fence context refers to it's own
>timeline.
>
>For example, consider the following case where virgl submits
>commands to the GPU (fence ids 1, 3) and does a metadata query with
>the CPU (fence id 5).  In a different process, gfxstream submits
>commands to the GPU (fence ids 2, 4).
>
>fence_id (&dma_fence.seqno)       | 1 2 3 4 5
>----------------------------------|-----------
>fence_ctx 0 (virgl gpu)           | 1   3
>fence_ctx 1 (virgl metadata query)|         5
>fence_ctx 2 (gfxstream gpu)       |   2   4
>
>With multiple fence contexts, we can wait for the metadata query
>to finish without waiting for the virgl gpu to finish.  virgl gpu
>does not have to wait for gfxstream gpu.  The fence id still is the
>monotonically increasing sequence number, but it's only revelant to
>the specific dma-fence context.
>
>To fully enable this feature, we'll need to:
>  - have each 3d context allocate a number of fence contexts. Not
>    too hard with explicit context initialization on the horizon.
>  - have guest userspace specify fence context when performing
>    ioctls.
>  - tag each fence emitted to the host with the fence context
>    information.  virtio_gpu_ctrl_hdr has padding + flags available,
>    so that should be easy.
>
>This change goes in the direction specified above, by:
>  - looking up the virtgpu_fence given a fence_id
>  - signalling all prior fences in a given context
>  - signalling current fence
>
>v2: fix grammar in comment
>
>Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
Reviewed-by: Anthoine Bourgeois <anthoine.bourgeois@gmail.com>
>---
> drivers/gpu/drm/virtio/virtgpu_drv.h   |  1 +
> drivers/gpu/drm/virtio/virtgpu_fence.c | 39 ++++++++++++++++++++------
> 2 files changed, 31 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
>index 6a232553c99b..d9dbc4f258f3 100644
>--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
>+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
>@@ -136,6 +136,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 b35fcd1d02d7..d28e25e8409b 100644
>--- a/drivers/gpu/drm/virtio/virtgpu_fence.c
>+++ b/drivers/gpu/drm/virtio/virtgpu_fence.c
>@@ -51,7 +51,7 @@ static bool virtio_gpu_fence_signaled(struct dma_fence *f)
>
> static void virtio_gpu_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_gpu_timeline_value_str(struct dma_fence *f, char *str,
>@@ -99,7 +99,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->current_fence_id;
>+	fence->fence_id = 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);
>@@ -107,24 +107,45 @@ 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,
> 				    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;
>+
>+		/*
>+		 * Signal any fences with a strictly smaller sequence number
>+		 * than the current signaled fence.
>+		 */
>+		list_for_each_entry_safe(curr, tmp, &drv->fences, node) {
>+			/* dma-fence contexts must match */
>+			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.29.2.454.gaff20da3a2-goog
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel