[PATCH 1/8] drm/virtio: use virtio_gpu_object->hw_res_handle in virtio_gpufb_create()

[PATCH 1/8] drm/virtio: use virtio_gpu_object->hw_res_handle in virtio_gpufb_create()

[Gerd Hoffmann]

Drop pointless resid variable in virtio_gpufb_create(), just use
the hw_res_handle field in virtio_gpu_object directly.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_fb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_fb.c b/drivers/gpu/drm/virtio/virtgpu_fb.c
index cea749f4ec..b1f37cba76 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fb.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fb.c
@@ -214,7 +214,7 @@ static int virtio_gpufb_create(struct drm_fb_helper *helper,
 	struct drm_framebuffer *fb;
 	struct drm_mode_fb_cmd2 mode_cmd = {};
 	struct virtio_gpu_object *obj;
-	uint32_t resid, format, size;
+	uint32_t format, size;
 	int ret;
 
 	mode_cmd.width = sizes->surface_width;
@@ -231,8 +231,8 @@ static int virtio_gpufb_create(struct drm_fb_helper *helper,
 	if (IS_ERR(obj))
 		return PTR_ERR(obj);
 
-	virtio_gpu_resource_id_get(vgdev, &resid);
-	virtio_gpu_cmd_create_resource(vgdev, resid, format,
+	virtio_gpu_resource_id_get(vgdev, &obj->hw_res_handle);
+	virtio_gpu_cmd_create_resource(vgdev, obj->hw_res_handle, format,
 				       mode_cmd.width, mode_cmd.height);
 
 	ret = virtio_gpu_object_kmap(obj);
@@ -242,7 +242,7 @@ static int virtio_gpufb_create(struct drm_fb_helper *helper,
 	}
 
 	/* attach the object to the resource */
-	ret = virtio_gpu_object_attach(vgdev, obj, resid, NULL);
+	ret = virtio_gpu_object_attach(vgdev, obj, obj->hw_res_handle, NULL);
 	if (ret)
 		goto err_obj_attach;
 
-- 
2.9.3

[PATCH 2/8] drm/virtio: use virtio_gpu_object->hw_res_handle in virtio_gpu_mode_dumb_create()

[Gerd Hoffmann]

Drop pointless resid variable in virtio_gpu_mode_dumb_create(), just use
the hw_res_handle field in virtio_gpu_object directly.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_gem.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index 82c817f37c..2492f5a2e9 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -87,7 +87,6 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 	struct virtio_gpu_object *obj;
 	int ret;
 	uint32_t pitch;
-	uint32_t resid;
 	uint32_t format;
 
 	if (args->bpp != 32)
@@ -103,13 +102,13 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 		goto fail;
 
 	format = virtio_gpu_translate_format(DRM_FORMAT_HOST_XRGB8888);
-	virtio_gpu_resource_id_get(vgdev, &resid);
-	virtio_gpu_cmd_create_resource(vgdev, resid, format,
+	obj = gem_to_virtio_gpu_obj(gobj);
+	virtio_gpu_resource_id_get(vgdev, &obj->hw_res_handle);
+	virtio_gpu_cmd_create_resource(vgdev, obj->hw_res_handle, format,
 				       args->width, args->height);
 
 	/* attach the object to the resource */
-	obj = gem_to_virtio_gpu_obj(gobj);
-	ret = virtio_gpu_object_attach(vgdev, obj, resid, NULL);
+	ret = virtio_gpu_object_attach(vgdev, obj, obj->hw_res_handle, NULL);
 	if (ret)
 		goto fail;
 
-- 
2.9.3

[PATCH 3/8] drm/virtio: use virtio_gpu_object->hw_res_handle in virtio_gpu_resource_create_ioctl()

[Gerd Hoffmann]

Drop pointless res_id variable in virtio_gpu_resource_create_ioctl(),
just use the hw_res_handle field in virtio_gpu_object directly.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index f16b875d6a..4b76a0ec1a 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -217,7 +217,6 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 	struct virtio_gpu_device *vgdev = dev->dev_private;
 	struct drm_virtgpu_resource_create *rc = data;
 	int ret;
-	uint32_t res_id;
 	struct virtio_gpu_object *qobj;
 	struct drm_gem_object *obj;
 	uint32_t handle = 0;
@@ -244,8 +243,6 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 	INIT_LIST_HEAD(&validate_list);
 	memset(&mainbuf, 0, sizeof(struct ttm_validate_buffer));
 
-	virtio_gpu_resource_id_get(vgdev, &res_id);
-
 	size = rc->size;
 
 	/* allocate a single page size object */
@@ -253,17 +250,16 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 		size = PAGE_SIZE;
 
 	qobj = virtio_gpu_alloc_object(dev, size, false, false);
-	if (IS_ERR(qobj)) {
-		ret = PTR_ERR(qobj);
-		goto fail_id;
-	}
+	if (IS_ERR(qobj))
+		return PTR_ERR(qobj);
 	obj = &qobj->gem_base;
+	virtio_gpu_resource_id_get(vgdev, &qobj->hw_res_handle);
 
 	if (!vgdev->has_virgl_3d) {
-		virtio_gpu_cmd_create_resource(vgdev, res_id, rc->format,
+		virtio_gpu_cmd_create_resource(vgdev, qobj->hw_res_handle, rc->format,
 					       rc->width, rc->height);
 
-		ret = virtio_gpu_object_attach(vgdev, qobj, res_id, NULL);
+		ret = virtio_gpu_object_attach(vgdev, qobj, qobj->hw_res_handle, NULL);
 	} else {
 		/* use a gem reference since unref list undoes them */
 		drm_gem_object_get(&qobj->gem_base);
@@ -276,7 +272,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 			goto fail_unref;
 		}
 
-		rc_3d.resource_id = cpu_to_le32(res_id);
+		rc_3d.resource_id = cpu_to_le32(qobj->hw_res_handle);
 		rc_3d.target = cpu_to_le32(rc->target);
 		rc_3d.format = cpu_to_le32(rc->format);
 		rc_3d.bind = cpu_to_le32(rc->bind);
@@ -289,7 +285,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 		rc_3d.flags = cpu_to_le32(rc->flags);
 
 		virtio_gpu_cmd_resource_create_3d(vgdev, &rc_3d, NULL);
-		ret = virtio_gpu_object_attach(vgdev, qobj, res_id, &fence);
+		ret = virtio_gpu_object_attach(vgdev, qobj, qobj->hw_res_handle, &fence);
 		if (ret) {
 			ttm_eu_backoff_reservation(&ticket, &validate_list);
 			goto fail_unref;
@@ -297,8 +293,6 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 		ttm_eu_fence_buffer_objects(&ticket, &validate_list, &fence->f);
 	}
 
-	qobj->hw_res_handle = res_id;
-
 	ret = drm_gem_handle_create(file_priv, obj, &handle);
 	if (ret) {
 
@@ -311,7 +305,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 	}
 	drm_gem_object_put_unlocked(obj);
 
-	rc->res_handle = res_id; /* similiar to a VM address */
+	rc->res_handle = qobj->hw_res_handle; /* similiar to a VM address */
 	rc->bo_handle = handle;
 
 	if (vgdev->has_virgl_3d) {
@@ -326,8 +320,6 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 	}
 //fail_obj:
 //	drm_gem_object_handle_unreference_unlocked(obj);
-fail_id:
-	virtio_gpu_resource_id_put(vgdev, res_id);
 	return ret;
 }
 
-- 
2.9.3

[PATCH 4/8] drm/virtio: drop resource_id argument from virtio_gpu_object_attach()

[Gerd Hoffmann]

We pass the obj anyway, so obj->hw_res_handle can be used instead.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h   | 1 -
 drivers/gpu/drm/virtio/virtgpu_fb.c    | 2 +-
 drivers/gpu/drm/virtio/virtgpu_gem.c   | 2 +-
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 4 ++--
 drivers/gpu/drm/virtio/virtgpu_ttm.c   | 3 +--
 drivers/gpu/drm/virtio/virtgpu_vq.c    | 4 +---
 6 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index d29f0c7c76..9b26e8ee84 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -285,7 +285,6 @@ void virtio_gpu_cmd_set_scanout(struct virtio_gpu_device *vgdev,
 				uint32_t x, uint32_t y);
 int virtio_gpu_object_attach(struct virtio_gpu_device *vgdev,
 			     struct virtio_gpu_object *obj,
-			     uint32_t resource_id,
 			     struct virtio_gpu_fence **fence);
 void virtio_gpu_object_detach(struct virtio_gpu_device *vgdev,
 			      struct virtio_gpu_object *obj);
diff --git a/drivers/gpu/drm/virtio/virtgpu_fb.c b/drivers/gpu/drm/virtio/virtgpu_fb.c
index b1f37cba76..b16c62c4d8 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fb.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fb.c
@@ -242,7 +242,7 @@ static int virtio_gpufb_create(struct drm_fb_helper *helper,
 	}
 
 	/* attach the object to the resource */
-	ret = virtio_gpu_object_attach(vgdev, obj, obj->hw_res_handle, NULL);
+	ret = virtio_gpu_object_attach(vgdev, obj, NULL);
 	if (ret)
 		goto err_obj_attach;
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index 2492f5a2e9..5450f7ab5b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -108,7 +108,7 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 				       args->width, args->height);
 
 	/* attach the object to the resource */
-	ret = virtio_gpu_object_attach(vgdev, obj, obj->hw_res_handle, NULL);
+	ret = virtio_gpu_object_attach(vgdev, obj, NULL);
 	if (ret)
 		goto fail;
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 4b76a0ec1a..44c9160c14 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -259,7 +259,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 		virtio_gpu_cmd_create_resource(vgdev, qobj->hw_res_handle, rc->format,
 					       rc->width, rc->height);
 
-		ret = virtio_gpu_object_attach(vgdev, qobj, qobj->hw_res_handle, NULL);
+		ret = virtio_gpu_object_attach(vgdev, qobj, NULL);
 	} else {
 		/* use a gem reference since unref list undoes them */
 		drm_gem_object_get(&qobj->gem_base);
@@ -285,7 +285,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 		rc_3d.flags = cpu_to_le32(rc->flags);
 
 		virtio_gpu_cmd_resource_create_3d(vgdev, &rc_3d, NULL);
-		ret = virtio_gpu_object_attach(vgdev, qobj, qobj->hw_res_handle, &fence);
+		ret = virtio_gpu_object_attach(vgdev, qobj, &fence);
 		if (ret) {
 			ttm_eu_backoff_reservation(&ticket, &validate_list);
 			goto fail_unref;
diff --git a/drivers/gpu/drm/virtio/virtgpu_ttm.c b/drivers/gpu/drm/virtio/virtgpu_ttm.c
index e3152d45c5..cd63dffa6d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ttm.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ttm.c
@@ -347,8 +347,7 @@ static void virtio_gpu_bo_move_notify(struct ttm_buffer_object *tbo,
 
 	} else if (new_mem->placement & TTM_PL_FLAG_TT) {
 		if (bo->hw_res_handle) {
-			virtio_gpu_object_attach(vgdev, bo, bo->hw_res_handle,
-						 NULL);
+			virtio_gpu_object_attach(vgdev, bo, NULL);
 		}
 	}
 }
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 4e2e037aed..dd4464ccb1 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -861,7 +861,6 @@ void virtio_gpu_cmd_submit(struct virtio_gpu_device *vgdev,
 
 int virtio_gpu_object_attach(struct virtio_gpu_device *vgdev,
 			     struct virtio_gpu_object *obj,
-			     uint32_t resource_id,
 			     struct virtio_gpu_fence **fence)
 {
 	bool use_dma_api = !virtio_has_iommu_quirk(vgdev->vdev);
@@ -902,10 +901,9 @@ int virtio_gpu_object_attach(struct virtio_gpu_device *vgdev,
 		ents[si].padding = 0;
 	}
 
-	virtio_gpu_cmd_resource_attach_backing(vgdev, resource_id,
+	virtio_gpu_cmd_resource_attach_backing(vgdev, obj->hw_res_handle,
 					       ents, nents,
 					       fence);
-	obj->hw_res_handle = resource_id;
 	return 0;
 }
 
-- 
2.9.3

[PATCH 5/8] drm/virtio: track created object state

[Gerd Hoffmann]

Track whenever the virtio_gpu_object is already created (i.e. host knows
about it) in a new variable.  Add checks to virtio_gpu_object_attach()
to do nothing on objects not created yet.

Make virtio_gpu_ttm_bo_destroy() use the new variable too, instead of
expecting hw_res_handle indicating the object state.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h    | 3 ++-
 drivers/gpu/drm/virtio/virtgpu_fb.c     | 2 +-
 drivers/gpu/drm/virtio/virtgpu_gem.c    | 2 +-
 drivers/gpu/drm/virtio/virtgpu_ioctl.c  | 3 ++-
 drivers/gpu/drm/virtio/virtgpu_object.c | 2 +-
 drivers/gpu/drm/virtio/virtgpu_vq.c     | 8 ++++++--
 6 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 9b26e8ee84..4a39877ce6 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -65,6 +65,7 @@ struct virtio_gpu_object {
 	struct ttm_placement		placement;
 	struct ttm_buffer_object	tbo;
 	struct ttm_bo_kmap_obj		kmap;
+	bool created;
 };
 #define gem_to_virtio_gpu_obj(gobj) \
 	container_of((gobj), struct virtio_gpu_object, gem_base)
@@ -263,7 +264,7 @@ void virtio_gpu_resource_id_get(struct virtio_gpu_device *vgdev,
 			       uint32_t *resid);
 void virtio_gpu_resource_id_put(struct virtio_gpu_device *vgdev, uint32_t id);
 void virtio_gpu_cmd_create_resource(struct virtio_gpu_device *vgdev,
-				    uint32_t resource_id,
+				    struct virtio_gpu_object *bo,
 				    uint32_t format,
 				    uint32_t width,
 				    uint32_t height);
diff --git a/drivers/gpu/drm/virtio/virtgpu_fb.c b/drivers/gpu/drm/virtio/virtgpu_fb.c
index b16c62c4d8..c22a8246b6 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fb.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fb.c
@@ -232,7 +232,7 @@ static int virtio_gpufb_create(struct drm_fb_helper *helper,
 		return PTR_ERR(obj);
 
 	virtio_gpu_resource_id_get(vgdev, &obj->hw_res_handle);
-	virtio_gpu_cmd_create_resource(vgdev, obj->hw_res_handle, format,
+	virtio_gpu_cmd_create_resource(vgdev, obj, format,
 				       mode_cmd.width, mode_cmd.height);
 
 	ret = virtio_gpu_object_kmap(obj);
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index 5450f7ab5b..665d18a49d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -104,7 +104,7 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 	format = virtio_gpu_translate_format(DRM_FORMAT_HOST_XRGB8888);
 	obj = gem_to_virtio_gpu_obj(gobj);
 	virtio_gpu_resource_id_get(vgdev, &obj->hw_res_handle);
-	virtio_gpu_cmd_create_resource(vgdev, obj->hw_res_handle, format,
+	virtio_gpu_cmd_create_resource(vgdev, obj, format,
 				       args->width, args->height);
 
 	/* attach the object to the resource */
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 44c9160c14..f9c55ecfca 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -256,7 +256,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 	virtio_gpu_resource_id_get(vgdev, &qobj->hw_res_handle);
 
 	if (!vgdev->has_virgl_3d) {
-		virtio_gpu_cmd_create_resource(vgdev, qobj->hw_res_handle, rc->format,
+		virtio_gpu_cmd_create_resource(vgdev, qobj, rc->format,
 					       rc->width, rc->height);
 
 		ret = virtio_gpu_object_attach(vgdev, qobj, NULL);
@@ -285,6 +285,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 		rc_3d.flags = cpu_to_le32(rc->flags);
 
 		virtio_gpu_cmd_resource_create_3d(vgdev, &rc_3d, NULL);
+		qobj->created = true;
 		ret = virtio_gpu_object_attach(vgdev, qobj, &fence);
 		if (ret) {
 			ttm_eu_backoff_reservation(&ticket, &validate_list);
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index eca7655374..6611c487d7 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -33,7 +33,7 @@ static void virtio_gpu_ttm_bo_destroy(struct ttm_buffer_object *tbo)
 	bo = container_of(tbo, struct virtio_gpu_object, tbo);
 	vgdev = (struct virtio_gpu_device *)bo->gem_base.dev->dev_private;
 
-	if (bo->hw_res_handle)
+	if (bo->created)
 		virtio_gpu_cmd_unref_resource(vgdev, bo->hw_res_handle);
 	if (bo->pages)
 		virtio_gpu_object_free_sg_table(bo);
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index dd4464ccb1..45da3c87b6 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -388,7 +388,7 @@ static int virtio_gpu_queue_cursor(struct virtio_gpu_device *vgdev,
 
 /* create a basic resource */
 void virtio_gpu_cmd_create_resource(struct virtio_gpu_device *vgdev,
-				    uint32_t resource_id,
+				    struct virtio_gpu_object *bo,
 				    uint32_t format,
 				    uint32_t width,
 				    uint32_t height)
@@ -400,12 +400,13 @@ void virtio_gpu_cmd_create_resource(struct virtio_gpu_device *vgdev,
 	memset(cmd_p, 0, sizeof(*cmd_p));
 
 	cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_RESOURCE_CREATE_2D);
-	cmd_p->resource_id = cpu_to_le32(resource_id);
+	cmd_p->resource_id = cpu_to_le32(bo->hw_res_handle);
 	cmd_p->format = cpu_to_le32(format);
 	cmd_p->width = cpu_to_le32(width);
 	cmd_p->height = cpu_to_le32(height);
 
 	virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
+	bo->created = true;
 }
 
 void virtio_gpu_cmd_unref_resource(struct virtio_gpu_device *vgdev,
@@ -868,6 +869,9 @@ int virtio_gpu_object_attach(struct virtio_gpu_device *vgdev,
 	struct scatterlist *sg;
 	int si, nents;
 
+	if (!obj->created)
+		return 0;
+
 	if (!obj->pages) {
 		int ret;
 
-- 
2.9.3

Re: [PATCH 5/8] drm/virtio: track created object state

[Dave Airlie]

On Mon, 1 Oct 2018 at 20:33, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Track whenever the virtio_gpu_object is already created (i.e. host knows
> about it) in a new variable.  Add checks to virtio_gpu_object_attach()
> to do nothing on objects not created yet.
>
> Make virtio_gpu_ttm_bo_destroy() use the new variable too, instead of
> expecting hw_res_handle indicating the object state.

Is there a potential patch ordering issue here? If this patch changes
the code to avoid using hw_res_handle,
is after patches that start filling in hw_res_handle in places we
haven't filled it in before.

Maybe this patch should happen earlier.

Dave.

>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_drv.h    | 3 ++-
>  drivers/gpu/drm/virtio/virtgpu_fb.c     | 2 +-
>  drivers/gpu/drm/virtio/virtgpu_gem.c    | 2 +-
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c  | 3 ++-
>  drivers/gpu/drm/virtio/virtgpu_object.c | 2 +-
>  drivers/gpu/drm/virtio/virtgpu_vq.c     | 8 ++++++--
>  6 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 9b26e8ee84..4a39877ce6 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -65,6 +65,7 @@ struct virtio_gpu_object {
>         struct ttm_placement            placement;
>         struct ttm_buffer_object        tbo;
>         struct ttm_bo_kmap_obj          kmap;
> +       bool created;
>  };
>  #define gem_to_virtio_gpu_obj(gobj) \
>         container_of((gobj), struct virtio_gpu_object, gem_base)
> @@ -263,7 +264,7 @@ void virtio_gpu_resource_id_get(struct virtio_gpu_device *vgdev,
>                                uint32_t *resid);
>  void virtio_gpu_resource_id_put(struct virtio_gpu_device *vgdev, uint32_t id);
>  void virtio_gpu_cmd_create_resource(struct virtio_gpu_device *vgdev,
> -                                   uint32_t resource_id,
> +                                   struct virtio_gpu_object *bo,
>                                     uint32_t format,
>                                     uint32_t width,
>                                     uint32_t height);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_fb.c b/drivers/gpu/drm/virtio/virtgpu_fb.c
> index b16c62c4d8..c22a8246b6 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_fb.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_fb.c
> @@ -232,7 +232,7 @@ static int virtio_gpufb_create(struct drm_fb_helper *helper,
>                 return PTR_ERR(obj);
>
>         virtio_gpu_resource_id_get(vgdev, &obj->hw_res_handle);
> -       virtio_gpu_cmd_create_resource(vgdev, obj->hw_res_handle, format,
> +       virtio_gpu_cmd_create_resource(vgdev, obj, format,
>                                        mode_cmd.width, mode_cmd.height);
>
>         ret = virtio_gpu_object_kmap(obj);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
> index 5450f7ab5b..665d18a49d 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_gem.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
> @@ -104,7 +104,7 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
>         format = virtio_gpu_translate_format(DRM_FORMAT_HOST_XRGB8888);
>         obj = gem_to_virtio_gpu_obj(gobj);
>         virtio_gpu_resource_id_get(vgdev, &obj->hw_res_handle);
> -       virtio_gpu_cmd_create_resource(vgdev, obj->hw_res_handle, format,
> +       virtio_gpu_cmd_create_resource(vgdev, obj, format,
>                                        args->width, args->height);
>
>         /* attach the object to the resource */
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index 44c9160c14..f9c55ecfca 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -256,7 +256,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
>         virtio_gpu_resource_id_get(vgdev, &qobj->hw_res_handle);
>
>         if (!vgdev->has_virgl_3d) {
> -               virtio_gpu_cmd_create_resource(vgdev, qobj->hw_res_handle, rc->format,
> +               virtio_gpu_cmd_create_resource(vgdev, qobj, rc->format,
>                                                rc->width, rc->height);
>
>                 ret = virtio_gpu_object_attach(vgdev, qobj, NULL);
> @@ -285,6 +285,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
>                 rc_3d.flags = cpu_to_le32(rc->flags);
>
>                 virtio_gpu_cmd_resource_create_3d(vgdev, &rc_3d, NULL);
> +               qobj->created = true;
>                 ret = virtio_gpu_object_attach(vgdev, qobj, &fence);
>                 if (ret) {
>                         ttm_eu_backoff_reservation(&ticket, &validate_list);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
> index eca7655374..6611c487d7 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -33,7 +33,7 @@ static void virtio_gpu_ttm_bo_destroy(struct ttm_buffer_object *tbo)
>         bo = container_of(tbo, struct virtio_gpu_object, tbo);
>         vgdev = (struct virtio_gpu_device *)bo->gem_base.dev->dev_private;
>
> -       if (bo->hw_res_handle)
> +       if (bo->created)
>                 virtio_gpu_cmd_unref_resource(vgdev, bo->hw_res_handle);
>         if (bo->pages)
>                 virtio_gpu_object_free_sg_table(bo);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index dd4464ccb1..45da3c87b6 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -388,7 +388,7 @@ static int virtio_gpu_queue_cursor(struct virtio_gpu_device *vgdev,
>
>  /* create a basic resource */
>  void virtio_gpu_cmd_create_resource(struct virtio_gpu_device *vgdev,
> -                                   uint32_t resource_id,
> +                                   struct virtio_gpu_object *bo,
>                                     uint32_t format,
>                                     uint32_t width,
>                                     uint32_t height)
> @@ -400,12 +400,13 @@ void virtio_gpu_cmd_create_resource(struct virtio_gpu_device *vgdev,
>         memset(cmd_p, 0, sizeof(*cmd_p));
>
>         cmd_p->hdr.type = cpu_to_le32(VIRTIO_GPU_CMD_RESOURCE_CREATE_2D);
> -       cmd_p->resource_id = cpu_to_le32(resource_id);
> +       cmd_p->resource_id = cpu_to_le32(bo->hw_res_handle);
>         cmd_p->format = cpu_to_le32(format);
>         cmd_p->width = cpu_to_le32(width);
>         cmd_p->height = cpu_to_le32(height);
>
>         virtio_gpu_queue_ctrl_buffer(vgdev, vbuf);
> +       bo->created = true;
>  }
>
>  void virtio_gpu_cmd_unref_resource(struct virtio_gpu_device *vgdev,
> @@ -868,6 +869,9 @@ int virtio_gpu_object_attach(struct virtio_gpu_device *vgdev,
>         struct scatterlist *sg;
>         int si, nents;
>
> +       if (!obj->created)
> +               return 0;
> +
>         if (!obj->pages) {
>                 int ret;
>
> --
> 2.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/8] drm/virtio: track created object state

[Gerd Hoffmann]

On Thu, Oct 18, 2018 at 11:25:22AM +1000, Dave Airlie wrote:
> On Mon, 1 Oct 2018 at 20:33, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > Track whenever the virtio_gpu_object is already created (i.e. host knows
> > about it) in a new variable.  Add checks to virtio_gpu_object_attach()
> > to do nothing on objects not created yet.
> >
> > Make virtio_gpu_ttm_bo_destroy() use the new variable too, instead of
> > expecting hw_res_handle indicating the object state.
> 
> Is there a potential patch ordering issue here? If this patch changes
> the code to avoid using hw_res_handle,
> is after patches that start filling in hw_res_handle in places we
> haven't filled it in before.
> Maybe this patch should happen earlier.

Didn't run into trouble in testing, but yes, it might be an issue in
error paths.  I'll reorder the patches.

cheers,
  Gerd

[PATCH 6/8] drm/virtio: fix resource id handling

[Gerd Hoffmann]

Move virtio_gpu_resource_id_{get,put} to virtgpu_object.c and make them
static.  Allocate and free the id on creation and destroy, drop all
other calls.  That way objects have a valid handle for the whole
lifetime of the object.

Also fixes ids leaking.  Worst offender are dumb buffers, and I think
some error paths too.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h    |  3 ---
 drivers/gpu/drm/virtio/virtgpu_fb.c     |  1 -
 drivers/gpu/drm/virtio/virtgpu_gem.c    |  1 -
 drivers/gpu/drm/virtio/virtgpu_ioctl.c  |  1 -
 drivers/gpu/drm/virtio/virtgpu_object.c | 23 +++++++++++++++++++++++
 drivers/gpu/drm/virtio/virtgpu_vq.c     | 20 --------------------
 6 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 4a39877ce6..1c321e484d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -260,9 +260,6 @@ int virtio_gpu_surface_dirty(struct virtio_gpu_framebuffer *qfb,
 /* virtio vg */
 int virtio_gpu_alloc_vbufs(struct virtio_gpu_device *vgdev);
 void virtio_gpu_free_vbufs(struct virtio_gpu_device *vgdev);
-void virtio_gpu_resource_id_get(struct virtio_gpu_device *vgdev,
-			       uint32_t *resid);
-void virtio_gpu_resource_id_put(struct virtio_gpu_device *vgdev, uint32_t id);
 void virtio_gpu_cmd_create_resource(struct virtio_gpu_device *vgdev,
 				    struct virtio_gpu_object *bo,
 				    uint32_t format,
diff --git a/drivers/gpu/drm/virtio/virtgpu_fb.c b/drivers/gpu/drm/virtio/virtgpu_fb.c
index c22a8246b6..fb1cc8b2f1 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fb.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fb.c
@@ -231,7 +231,6 @@ static int virtio_gpufb_create(struct drm_fb_helper *helper,
 	if (IS_ERR(obj))
 		return PTR_ERR(obj);
 
-	virtio_gpu_resource_id_get(vgdev, &obj->hw_res_handle);
 	virtio_gpu_cmd_create_resource(vgdev, obj, format,
 				       mode_cmd.width, mode_cmd.height);
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index 665d18a49d..f065863939 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -103,7 +103,6 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 
 	format = virtio_gpu_translate_format(DRM_FORMAT_HOST_XRGB8888);
 	obj = gem_to_virtio_gpu_obj(gobj);
-	virtio_gpu_resource_id_get(vgdev, &obj->hw_res_handle);
 	virtio_gpu_cmd_create_resource(vgdev, obj, format,
 				       args->width, args->height);
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index f9c55ecfca..681edd9c92 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -253,7 +253,6 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 	if (IS_ERR(qobj))
 		return PTR_ERR(qobj);
 	obj = &qobj->gem_base;
-	virtio_gpu_resource_id_get(vgdev, &qobj->hw_res_handle);
 
 	if (!vgdev->has_virgl_3d) {
 		virtio_gpu_cmd_create_resource(vgdev, qobj, rc->format,
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index 6611c487d7..8bd1ebe13b 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -25,6 +25,26 @@
 
 #include "virtgpu_drv.h"
 
+static void virtio_gpu_resource_id_get(struct virtio_gpu_device *vgdev,
+				       uint32_t *resid)
+{
+	int handle;
+
+	idr_preload(GFP_KERNEL);
+	spin_lock(&vgdev->resource_idr_lock);
+	handle = idr_alloc(&vgdev->resource_idr, NULL, 1, 0, GFP_NOWAIT);
+	spin_unlock(&vgdev->resource_idr_lock);
+	idr_preload_end();
+	*resid = handle;
+}
+
+static void virtio_gpu_resource_id_put(struct virtio_gpu_device *vgdev, uint32_t id)
+{
+	spin_lock(&vgdev->resource_idr_lock);
+	idr_remove(&vgdev->resource_idr, id);
+	spin_unlock(&vgdev->resource_idr_lock);
+}
+
 static void virtio_gpu_ttm_bo_destroy(struct ttm_buffer_object *tbo)
 {
 	struct virtio_gpu_object *bo;
@@ -40,6 +60,7 @@ static void virtio_gpu_ttm_bo_destroy(struct ttm_buffer_object *tbo)
 	if (bo->vmap)
 		virtio_gpu_object_kunmap(bo);
 	drm_gem_object_release(&bo->gem_base);
+	virtio_gpu_resource_id_put(vgdev, bo->hw_res_handle);
 	kfree(bo);
 }
 
@@ -81,9 +102,11 @@ int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
 	bo = kzalloc(sizeof(struct virtio_gpu_object), GFP_KERNEL);
 	if (bo == NULL)
 		return -ENOMEM;
+	virtio_gpu_resource_id_get(vgdev, &bo->hw_res_handle);
 	size = roundup(size, PAGE_SIZE);
 	ret = drm_gem_object_init(vgdev->ddev, &bo->gem_base, size);
 	if (ret != 0) {
+		virtio_gpu_resource_id_put(vgdev, bo->hw_res_handle);
 		kfree(bo);
 		return ret;
 	}
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c b/drivers/gpu/drm/virtio/virtgpu_vq.c
index 45da3c87b6..f6e875cdcd 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -38,26 +38,6 @@
 			       + MAX_INLINE_CMD_SIZE		 \
 			       + MAX_INLINE_RESP_SIZE)
 
-void virtio_gpu_resource_id_get(struct virtio_gpu_device *vgdev,
-				uint32_t *resid)
-{
-	int handle;
-
-	idr_preload(GFP_KERNEL);
-	spin_lock(&vgdev->resource_idr_lock);
-	handle = idr_alloc(&vgdev->resource_idr, NULL, 1, 0, GFP_NOWAIT);
-	spin_unlock(&vgdev->resource_idr_lock);
-	idr_preload_end();
-	*resid = handle;
-}
-
-void virtio_gpu_resource_id_put(struct virtio_gpu_device *vgdev, uint32_t id)
-{
-	spin_lock(&vgdev->resource_idr_lock);
-	idr_remove(&vgdev->resource_idr, id);
-	spin_unlock(&vgdev->resource_idr_lock);
-}
-
 void virtio_gpu_ctrl_ack(struct virtqueue *vq)
 {
 	struct drm_device *dev = vq->vdev->priv;
-- 
2.9.3

[PATCH 7/8] drm/virtio: move virtio_gpu_object_{attach,detach} calls.

[Gerd Hoffmann]

Remove the virtio_gpu_object_{attach,detach} calls from move_notify()
callback.  Add them to the ttm_tt_{populate,unpopulate} callbacks, which
is the correct place to handle this.

The new ttm_tt_{populate,unpopulate} callbacks call the
ttm_pool_populate()/unpopulate() functions (which are the default
implementation in case the callbacks not present) for the actual ttm
work.  Additionally virtio_gpu_object_{attach,detach} is called to
update the state on the host.

With that in place the move and move_notify callbacks are not needed
any more, so drop them.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_ttm.c | 70 +++++++++++-------------------------
 1 file changed, 21 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_ttm.c b/drivers/gpu/drm/virtio/virtgpu_ttm.c
index cd63dffa6d..96fb17e0fc 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ttm.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ttm.c
@@ -250,33 +250,24 @@ static void virtio_gpu_ttm_io_mem_free(struct ttm_bo_device *bdev,
  */
 struct virtio_gpu_ttm_tt {
 	struct ttm_dma_tt		ttm;
-	struct virtio_gpu_device	*vgdev;
-	u64				offset;
+	struct virtio_gpu_object        *obj;
 };
 
 static int virtio_gpu_ttm_backend_bind(struct ttm_tt *ttm,
 				       struct ttm_mem_reg *bo_mem)
 {
-	struct virtio_gpu_ttm_tt *gtt = (void *)ttm;
-
-	gtt->offset = (unsigned long)(bo_mem->start << PAGE_SHIFT);
-	if (!ttm->num_pages)
-		WARN(1, "nothing to bind %lu pages for mreg %p back %p!\n",
-		     ttm->num_pages, bo_mem, ttm);
-
-	/* Not implemented */
 	return 0;
 }
 
 static int virtio_gpu_ttm_backend_unbind(struct ttm_tt *ttm)
 {
-	/* Not implemented */
 	return 0;
 }
 
 static void virtio_gpu_ttm_backend_destroy(struct ttm_tt *ttm)
 {
-	struct virtio_gpu_ttm_tt *gtt = (void *)ttm;
+	struct virtio_gpu_ttm_tt *gtt =
+		container_of(ttm, struct virtio_gpu_ttm_tt, ttm.ttm);
 
 	ttm_dma_tt_fini(&gtt->ttm);
 	kfree(gtt);
@@ -299,7 +290,7 @@ static struct ttm_tt *virtio_gpu_ttm_tt_create(struct ttm_buffer_object *bo,
 	if (gtt == NULL)
 		return NULL;
 	gtt->ttm.ttm.func = &virtio_gpu_backend_func;
-	gtt->vgdev = vgdev;
+	gtt->obj = container_of(bo, struct virtio_gpu_object, tbo);
 	if (ttm_dma_tt_init(&gtt->ttm, bo, page_flags)) {
 		kfree(gtt);
 		return NULL;
@@ -307,49 +298,30 @@ static struct ttm_tt *virtio_gpu_ttm_tt_create(struct ttm_buffer_object *bo,
 	return &gtt->ttm.ttm;
 }
 
-static void virtio_gpu_move_null(struct ttm_buffer_object *bo,
-				 struct ttm_mem_reg *new_mem)
-{
-	struct ttm_mem_reg *old_mem = &bo->mem;
-
-	BUG_ON(old_mem->mm_node != NULL);
-	*old_mem = *new_mem;
-	new_mem->mm_node = NULL;
-}
-
-static int virtio_gpu_bo_move(struct ttm_buffer_object *bo, bool evict,
-			      struct ttm_operation_ctx *ctx,
-			      struct ttm_mem_reg *new_mem)
+static int virtio_gpu_ttm_tt_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
 {
+	struct virtio_gpu_ttm_tt *gtt =
+		container_of(ttm, struct virtio_gpu_ttm_tt, ttm.ttm);
+	struct virtio_gpu_device *vgdev =
+		(struct virtio_gpu_device *)gtt->obj->gem_base.dev->dev_private;
 	int ret;
 
-	ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);
+	ret = ttm_pool_populate(ttm, ctx);
 	if (ret)
 		return ret;
-
-	virtio_gpu_move_null(bo, new_mem);
-	return 0;
+	virtio_gpu_object_attach(vgdev, gtt->obj, NULL);
+	return ret;
 }
 
-static void virtio_gpu_bo_move_notify(struct ttm_buffer_object *tbo,
-				      bool evict,
-				      struct ttm_mem_reg *new_mem)
+static void virtio_gpu_ttm_tt_unpopulate(struct ttm_tt *ttm)
 {
-	struct virtio_gpu_object *bo;
-	struct virtio_gpu_device *vgdev;
+	struct virtio_gpu_ttm_tt *gtt =
+		container_of(ttm, struct virtio_gpu_ttm_tt, ttm.ttm);
+	struct virtio_gpu_device *vgdev =
+		(struct virtio_gpu_device *)gtt->obj->gem_base.dev->dev_private;
 
-	bo = container_of(tbo, struct virtio_gpu_object, tbo);
-	vgdev = (struct virtio_gpu_device *)bo->gem_base.dev->dev_private;
-
-	if (!new_mem || (new_mem->placement & TTM_PL_FLAG_SYSTEM)) {
-		if (bo->hw_res_handle)
-			virtio_gpu_object_detach(vgdev, bo);
-
-	} else if (new_mem->placement & TTM_PL_FLAG_TT) {
-		if (bo->hw_res_handle) {
-			virtio_gpu_object_attach(vgdev, bo, NULL);
-		}
-	}
+	virtio_gpu_object_detach(vgdev, gtt->obj);
+	ttm_pool_unpopulate(ttm);
 }
 
 static void virtio_gpu_bo_swap_notify(struct ttm_buffer_object *tbo)
@@ -366,15 +338,15 @@ static void virtio_gpu_bo_swap_notify(struct ttm_buffer_object *tbo)
 
 static struct ttm_bo_driver virtio_gpu_bo_driver = {
 	.ttm_tt_create = &virtio_gpu_ttm_tt_create,
+	.ttm_tt_populate = &virtio_gpu_ttm_tt_populate,
+	.ttm_tt_unpopulate = &virtio_gpu_ttm_tt_unpopulate,
 	.invalidate_caches = &virtio_gpu_invalidate_caches,
 	.init_mem_type = &virtio_gpu_init_mem_type,
 	.eviction_valuable = ttm_bo_eviction_valuable,
 	.evict_flags = &virtio_gpu_evict_flags,
-	.move = &virtio_gpu_bo_move,
 	.verify_access = &virtio_gpu_verify_access,
 	.io_mem_reserve = &virtio_gpu_ttm_io_mem_reserve,
 	.io_mem_free = &virtio_gpu_ttm_io_mem_free,
-	.move_notify = &virtio_gpu_bo_move_notify,
 	.swap_notify = &virtio_gpu_bo_swap_notify,
 };
 
-- 
2.9.3

Re: [PATCH 7/8] drm/virtio: move virtio_gpu_object_{attach, detach} calls.

[Dave Airlie]

On Mon, 1 Oct 2018 at 20:33, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Remove the virtio_gpu_object_{attach,detach} calls from move_notify()
> callback.  Add them to the ttm_tt_{populate,unpopulate} callbacks, which
> is the correct place to handle this.
>
> The new ttm_tt_{populate,unpopulate} callbacks call the
> ttm_pool_populate()/unpopulate() functions (which are the default
> implementation in case the callbacks not present) for the actual ttm
> work.  Additionally virtio_gpu_object_{attach,detach} is called to
> update the state on the host.

This to me feels more like a bind/unbind operation rather than a
populate/unpopulate operation,

bind is " Bind the backend pages into the aperture in the location"

whereas populate is

allocate pages for a ttm.

Dave.

>
> With that in place the move and move_notify callbacks are not needed
> any more, so drop them.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/virtio/virtgpu_ttm.c | 70 +++++++++++-------------------------
>  1 file changed, 21 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ttm.c b/drivers/gpu/drm/virtio/virtgpu_ttm.c
> index cd63dffa6d..96fb17e0fc 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ttm.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ttm.c
> @@ -250,33 +250,24 @@ static void virtio_gpu_ttm_io_mem_free(struct ttm_bo_device *bdev,
>   */
>  struct virtio_gpu_ttm_tt {
>         struct ttm_dma_tt               ttm;
> -       struct virtio_gpu_device        *vgdev;
> -       u64                             offset;
> +       struct virtio_gpu_object        *obj;
>  };
>
>  static int virtio_gpu_ttm_backend_bind(struct ttm_tt *ttm,
>                                        struct ttm_mem_reg *bo_mem)
>  {
> -       struct virtio_gpu_ttm_tt *gtt = (void *)ttm;
> -
> -       gtt->offset = (unsigned long)(bo_mem->start << PAGE_SHIFT);
> -       if (!ttm->num_pages)
> -               WARN(1, "nothing to bind %lu pages for mreg %p back %p!\n",
> -                    ttm->num_pages, bo_mem, ttm);
> -
> -       /* Not implemented */
>         return 0;
>  }
>
>  static int virtio_gpu_ttm_backend_unbind(struct ttm_tt *ttm)
>  {
> -       /* Not implemented */
>         return 0;
>  }
>
>  static void virtio_gpu_ttm_backend_destroy(struct ttm_tt *ttm)
>  {
> -       struct virtio_gpu_ttm_tt *gtt = (void *)ttm;
> +       struct virtio_gpu_ttm_tt *gtt =
> +               container_of(ttm, struct virtio_gpu_ttm_tt, ttm.ttm);
>
>         ttm_dma_tt_fini(&gtt->ttm);
>         kfree(gtt);
> @@ -299,7 +290,7 @@ static struct ttm_tt *virtio_gpu_ttm_tt_create(struct ttm_buffer_object *bo,
>         if (gtt == NULL)
>                 return NULL;
>         gtt->ttm.ttm.func = &virtio_gpu_backend_func;
> -       gtt->vgdev = vgdev;
> +       gtt->obj = container_of(bo, struct virtio_gpu_object, tbo);
>         if (ttm_dma_tt_init(&gtt->ttm, bo, page_flags)) {
>                 kfree(gtt);
>                 return NULL;
> @@ -307,49 +298,30 @@ static struct ttm_tt *virtio_gpu_ttm_tt_create(struct ttm_buffer_object *bo,
>         return &gtt->ttm.ttm;
>  }
>
> -static void virtio_gpu_move_null(struct ttm_buffer_object *bo,
> -                                struct ttm_mem_reg *new_mem)
> -{
> -       struct ttm_mem_reg *old_mem = &bo->mem;
> -
> -       BUG_ON(old_mem->mm_node != NULL);
> -       *old_mem = *new_mem;
> -       new_mem->mm_node = NULL;
> -}
> -
> -static int virtio_gpu_bo_move(struct ttm_buffer_object *bo, bool evict,
> -                             struct ttm_operation_ctx *ctx,
> -                             struct ttm_mem_reg *new_mem)
> +static int virtio_gpu_ttm_tt_populate(struct ttm_tt *ttm, struct ttm_operation_ctx *ctx)
>  {
> +       struct virtio_gpu_ttm_tt *gtt =
> +               container_of(ttm, struct virtio_gpu_ttm_tt, ttm.ttm);
> +       struct virtio_gpu_device *vgdev =
> +               (struct virtio_gpu_device *)gtt->obj->gem_base.dev->dev_private;
>         int ret;
>
> -       ret = ttm_bo_wait(bo, ctx->interruptible, ctx->no_wait_gpu);
> +       ret = ttm_pool_populate(ttm, ctx);
>         if (ret)
>                 return ret;
> -
> -       virtio_gpu_move_null(bo, new_mem);
> -       return 0;
> +       virtio_gpu_object_attach(vgdev, gtt->obj, NULL);
> +       return ret;
>  }
>
> -static void virtio_gpu_bo_move_notify(struct ttm_buffer_object *tbo,
> -                                     bool evict,
> -                                     struct ttm_mem_reg *new_mem)
> +static void virtio_gpu_ttm_tt_unpopulate(struct ttm_tt *ttm)
>  {
> -       struct virtio_gpu_object *bo;
> -       struct virtio_gpu_device *vgdev;
> +       struct virtio_gpu_ttm_tt *gtt =
> +               container_of(ttm, struct virtio_gpu_ttm_tt, ttm.ttm);
> +       struct virtio_gpu_device *vgdev =
> +               (struct virtio_gpu_device *)gtt->obj->gem_base.dev->dev_private;
>
> -       bo = container_of(tbo, struct virtio_gpu_object, tbo);
> -       vgdev = (struct virtio_gpu_device *)bo->gem_base.dev->dev_private;
> -
> -       if (!new_mem || (new_mem->placement & TTM_PL_FLAG_SYSTEM)) {
> -               if (bo->hw_res_handle)
> -                       virtio_gpu_object_detach(vgdev, bo);
> -
> -       } else if (new_mem->placement & TTM_PL_FLAG_TT) {
> -               if (bo->hw_res_handle) {
> -                       virtio_gpu_object_attach(vgdev, bo, NULL);
> -               }
> -       }
> +       virtio_gpu_object_detach(vgdev, gtt->obj);
> +       ttm_pool_unpopulate(ttm);
>  }
>
>  static void virtio_gpu_bo_swap_notify(struct ttm_buffer_object *tbo)
> @@ -366,15 +338,15 @@ static void virtio_gpu_bo_swap_notify(struct ttm_buffer_object *tbo)
>
>  static struct ttm_bo_driver virtio_gpu_bo_driver = {
>         .ttm_tt_create = &virtio_gpu_ttm_tt_create,
> +       .ttm_tt_populate = &virtio_gpu_ttm_tt_populate,
> +       .ttm_tt_unpopulate = &virtio_gpu_ttm_tt_unpopulate,
>         .invalidate_caches = &virtio_gpu_invalidate_caches,
>         .init_mem_type = &virtio_gpu_init_mem_type,
>         .eviction_valuable = ttm_bo_eviction_valuable,
>         .evict_flags = &virtio_gpu_evict_flags,
> -       .move = &virtio_gpu_bo_move,
>         .verify_access = &virtio_gpu_verify_access,
>         .io_mem_reserve = &virtio_gpu_ttm_io_mem_reserve,
>         .io_mem_free = &virtio_gpu_ttm_io_mem_free,
> -       .move_notify = &virtio_gpu_bo_move_notify,
>         .swap_notify = &virtio_gpu_bo_swap_notify,
>  };
>
> --
> 2.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/8] drm/virtio: move virtio_gpu_object_{attach, detach} calls.

[Gerd Hoffmann]

On Thu, Oct 18, 2018 at 11:41:52AM +1000, Dave Airlie wrote:
> On Mon, 1 Oct 2018 at 20:33, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > Remove the virtio_gpu_object_{attach,detach} calls from move_notify()
> > callback.  Add them to the ttm_tt_{populate,unpopulate} callbacks, which
> > is the correct place to handle this.
> >
> > The new ttm_tt_{populate,unpopulate} callbacks call the
> > ttm_pool_populate()/unpopulate() functions (which are the default
> > implementation in case the callbacks not present) for the actual ttm
> > work.  Additionally virtio_gpu_object_{attach,detach} is called to
> > update the state on the host.
> 
> This to me feels more like a bind/unbind operation rather than a
> populate/unpopulate operation,
> 
> bind is " Bind the backend pages into the aperture in the location"
> 
> whereas populate is
> 
> allocate pages for a ttm.

I ran into that trap too ;)

My first attempt was to map this to bind/unbind.  But this is not
correct and therefore didn't work very well.

virtio_gpu_object_attach() will send a scatter list of the pages
allocated for the object to the host (so the host knows where to
copy from/to when processing the transfer_from/to calls).  So IMO
it should be done on population not when binding.

cheers,
  Gerd

Re: [PATCH 7/8] drm/virtio: move virtio_gpu_object_{attach, detach} calls.

[Dave Airlie]

On Thu, 18 Oct 2018 at 16:11, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> On Thu, Oct 18, 2018 at 11:41:52AM +1000, Dave Airlie wrote:
> > On Mon, 1 Oct 2018 at 20:33, Gerd Hoffmann <kraxel@redhat.com> wrote:
> > >
> > > Remove the virtio_gpu_object_{attach,detach} calls from move_notify()
> > > callback.  Add them to the ttm_tt_{populate,unpopulate} callbacks, which
> > > is the correct place to handle this.
> > >
> > > The new ttm_tt_{populate,unpopulate} callbacks call the
> > > ttm_pool_populate()/unpopulate() functions (which are the default
> > > implementation in case the callbacks not present) for the actual ttm
> > > work.  Additionally virtio_gpu_object_{attach,detach} is called to
> > > update the state on the host.
> >
> > This to me feels more like a bind/unbind operation rather than a
> > populate/unpopulate operation,
> >
> > bind is " Bind the backend pages into the aperture in the location"
> >
> > whereas populate is
> >
> > allocate pages for a ttm.
>
> I ran into that trap too ;)
>
> My first attempt was to map this to bind/unbind.  But this is not
> correct and therefore didn't work very well.
>
> virtio_gpu_object_attach() will send a scatter list of the pages
> allocated for the object to the host (so the host knows where to
> copy from/to when processing the transfer_from/to calls).  So IMO
> it should be done on population not when binding.

Well bind on AGP is the same thing, we'd fill the AGP GART table on
bind, so that the AGP GPU could access the pages.

So I'm interested in why using bind/unbind failed if you have some more info?

Dave.

Re: [PATCH 7/8] drm/virtio: move virtio_gpu_object_{attach, detach} calls.

[Gerd Hoffmann]

> > > This to me feels more like a bind/unbind operation rather than a
> > > populate/unpopulate operation,
> > >
> > > bind is " Bind the backend pages into the aperture in the location"
> > >
> > > whereas populate is
> > >
> > > allocate pages for a ttm.
> >
> > I ran into that trap too ;)
> >
> > My first attempt was to map this to bind/unbind.  But this is not
> > correct and therefore didn't work very well.
> >
> > virtio_gpu_object_attach() will send a scatter list of the pages
> > allocated for the object to the host (so the host knows where to
> > copy from/to when processing the transfer_from/to calls).  So IMO
> > it should be done on population not when binding.
> 
> Well bind on AGP is the same thing, we'd fill the AGP GART table on
> bind, so that the AGP GPU could access the pages.

> So I'm interested in why using bind/unbind failed if you have some more info?

Need to try again to be sure, but IIRC I saw multiple bind/unbind calls
for the same object.  ttm probably does it to not waste AGB GART address
space for objects not in use.  But for virtio it is pointless overhead.
But maybe it is just a matter of taking a reference and keeping it for
the whole lifetime of the object to make the binding permanent ...

cheers,
  Gerd

Re: [PATCH 7/8] drm/virtio: move virtio_gpu_object_{attach, detach} calls.

[Dave Airlie]

On Thu, 18 Oct 2018 at 17:00, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> > > > This to me feels more like a bind/unbind operation rather than a
> > > > populate/unpopulate operation,
> > > >
> > > > bind is " Bind the backend pages into the aperture in the location"
> > > >
> > > > whereas populate is
> > > >
> > > > allocate pages for a ttm.
> > >
> > > I ran into that trap too ;)
> > >
> > > My first attempt was to map this to bind/unbind.  But this is not
> > > correct and therefore didn't work very well.
> > >
> > > virtio_gpu_object_attach() will send a scatter list of the pages
> > > allocated for the object to the host (so the host knows where to
> > > copy from/to when processing the transfer_from/to calls).  So IMO
> > > it should be done on population not when binding.
> >
> > Well bind on AGP is the same thing, we'd fill the AGP GART table on
> > bind, so that the AGP GPU could access the pages.
>
> > So I'm interested in why using bind/unbind failed if you have some more info?
>
> Need to try again to be sure, but IIRC I saw multiple bind/unbind calls
> for the same object.  ttm probably does it to not waste AGB GART address
> space for objects not in use.  But for virtio it is pointless overhead.
> But maybe it is just a matter of taking a reference and keeping it for
> the whole lifetime of the object to make the binding permanent ...

Hmm maybe for the bind/unbind, not sure what would cause unbind except
for the object being moved back out of the TT space and into system,
it might be worth confirming what happens here, as I really do feel
bind/unbind is the correct interface to use here.

Dave.

[PATCH 8/8] drm/virtio: move objects to TTM_PL_TT after creation

[Gerd Hoffmann]

Initialize objects with TTM_PL_FLAG_SYSTEM placement, move them over to
TTM_PL_FLAG_TT after they are created (i.e. registered in the host).
That way the ttm backend will handle attach/detach just fine and we
don't need the extra virtio_gpu_object_attach() calls.

TODO: check ioctl fence.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h    |  1 +
 drivers/gpu/drm/virtio/virtgpu_fb.c     |  5 ++---
 drivers/gpu/drm/virtio/virtgpu_gem.c    |  4 ++--
 drivers/gpu/drm/virtio/virtgpu_ioctl.c  | 13 +++++++++---
 drivers/gpu/drm/virtio/virtgpu_object.c | 35 ++++++++++++++++++++++++++++++++-
 5 files changed, 49 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 1c321e484d..554d887c6d 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -360,6 +360,7 @@ void virtio_gpu_fence_event_process(struct virtio_gpu_device *vdev,
 int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
 			     unsigned long size, bool kernel, bool pinned,
 			     struct virtio_gpu_object **bo_ptr);
+int virtio_gpu_object_move(struct virtio_gpu_object *vgbo);
 void virtio_gpu_object_kunmap(struct virtio_gpu_object *bo);
 int virtio_gpu_object_kmap(struct virtio_gpu_object *bo);
 int virtio_gpu_object_get_sg_table(struct virtio_gpu_device *qdev,
diff --git a/drivers/gpu/drm/virtio/virtgpu_fb.c b/drivers/gpu/drm/virtio/virtgpu_fb.c
index fb1cc8b2f1..4dd01520be 100644
--- a/drivers/gpu/drm/virtio/virtgpu_fb.c
+++ b/drivers/gpu/drm/virtio/virtgpu_fb.c
@@ -240,8 +240,8 @@ static int virtio_gpufb_create(struct drm_fb_helper *helper,
 		goto err_obj_vmap;
 	}
 
-	/* attach the object to the resource */
-	ret = virtio_gpu_object_attach(vgdev, obj, NULL);
+	/* attach the object to the resource (via ttm backend) */
+	ret = virtio_gpu_object_move(obj);
 	if (ret)
 		goto err_obj_attach;
 
@@ -277,7 +277,6 @@ static int virtio_gpufb_create(struct drm_fb_helper *helper,
 	return 0;
 
 err_fb_alloc:
-	virtio_gpu_object_detach(vgdev, obj);
 err_obj_attach:
 err_obj_vmap:
 	virtio_gpu_gem_free_object(&obj->gem_base);
diff --git a/drivers/gpu/drm/virtio/virtgpu_gem.c b/drivers/gpu/drm/virtio/virtgpu_gem.c
index f065863939..e3f0c46cbf 100644
--- a/drivers/gpu/drm/virtio/virtgpu_gem.c
+++ b/drivers/gpu/drm/virtio/virtgpu_gem.c
@@ -106,8 +106,8 @@ int virtio_gpu_mode_dumb_create(struct drm_file *file_priv,
 	virtio_gpu_cmd_create_resource(vgdev, obj, format,
 				       args->width, args->height);
 
-	/* attach the object to the resource */
-	ret = virtio_gpu_object_attach(vgdev, obj, NULL);
+	/* attach the object to the resource (via ttm backend) */
+	ret = virtio_gpu_object_move(obj);
 	if (ret)
 		goto fail;
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 681edd9c92..55baad8e90 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -258,7 +258,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 		virtio_gpu_cmd_create_resource(vgdev, qobj, rc->format,
 					       rc->width, rc->height);
 
-		ret = virtio_gpu_object_attach(vgdev, qobj, NULL);
+		ret = virtio_gpu_object_move(qobj);
 	} else {
 		/* use a gem reference since unref list undoes them */
 		drm_gem_object_get(&qobj->gem_base);
@@ -285,12 +285,17 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 
 		virtio_gpu_cmd_resource_create_3d(vgdev, &rc_3d, NULL);
 		qobj->created = true;
+#if 1
+		ret = virtio_gpu_object_move(qobj);
+		ttm_eu_backoff_reservation(&ticket, &validate_list);
+#else
 		ret = virtio_gpu_object_attach(vgdev, qobj, &fence);
 		if (ret) {
 			ttm_eu_backoff_reservation(&ticket, &validate_list);
 			goto fail_unref;
 		}
 		ttm_eu_fence_buffer_objects(&ticket, &validate_list, &fence->f);
+#endif
 	}
 
 	ret = drm_gem_handle_create(file_priv, obj, &handle);
@@ -299,7 +304,8 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 		drm_gem_object_release(obj);
 		if (vgdev->has_virgl_3d) {
 			virtio_gpu_unref_list(&validate_list);
-			dma_fence_put(&fence->f);
+			if (fence)
+				dma_fence_put(&fence->f);
 		}
 		return ret;
 	}
@@ -310,7 +316,8 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 
 	if (vgdev->has_virgl_3d) {
 		virtio_gpu_unref_list(&validate_list);
-		dma_fence_put(&fence->f);
+		if (fence)
+			dma_fence_put(&fence->f);
 	}
 	return 0;
 fail_unref:
diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c b/drivers/gpu/drm/virtio/virtgpu_object.c
index 8bd1ebe13b..0c2fc10859 100644
--- a/drivers/gpu/drm/virtio/virtgpu_object.c
+++ b/drivers/gpu/drm/virtio/virtgpu_object.c
@@ -75,12 +75,45 @@ static void virtio_gpu_init_ttm_placement(struct virtio_gpu_object *vgbo,
 	vgbo->placement_code.fpfn = 0;
 	vgbo->placement_code.lpfn = 0;
 	vgbo->placement_code.flags =
-		TTM_PL_MASK_CACHING | TTM_PL_FLAG_TT | pflag;
+		TTM_PL_MASK_CACHING | TTM_PL_FLAG_SYSTEM | pflag;
 	vgbo->placement.num_placement = c;
 	vgbo->placement.num_busy_placement = c;
 
 }
 
+int virtio_gpu_object_move(struct virtio_gpu_object *vgbo)
+{
+	struct ttm_operation_ctx ctx = { true, true };
+	struct ttm_place placement_memtype = {
+		.fpfn = 0,
+		.lpfn = 0,
+		.flags = vgbo->placement_code.flags,
+	};
+	struct ttm_placement placement;
+	struct ttm_mem_reg new_reg = { 0 };
+	int ret;
+
+	placement_memtype.flags &= ~TTM_PL_MASK_MEM;
+	placement_memtype.flags |= TTM_PL_FLAG_TT;
+
+	placement.num_placement = placement.num_busy_placement = 1;
+	placement.placement = placement.busy_placement = &placement_memtype;
+
+	ret = ttm_bo_mem_space(&vgbo->tbo, &placement, &new_reg, &ctx);
+	if (ret) {
+		DRM_INFO("%s: ttm_bo_mem_space: %d\n", __func__, ret);
+		return ret;
+	}
+
+	ret = ttm_bo_move_ttm(&vgbo->tbo, &ctx, &new_reg);
+	if (ret) {
+		DRM_INFO("%s: ttm_bo_move_ttm: %d\n", __func__, ret);
+		ttm_bo_mem_put(&vgbo->tbo, &new_reg);
+	}
+
+	return ret;
+}
+
 int virtio_gpu_object_create(struct virtio_gpu_device *vgdev,
 			     unsigned long size, bool kernel, bool pinned,
 			     struct virtio_gpu_object **bo_ptr)
-- 
2.9.3