[PATCH 0/4] *** Delay enqueuing context create command ***

[PATCH 0/4] *** Delay enqueuing context create command ***

[Gurchetan Singh]

Let's delay enqueuing the context creation command until the first 3D
ioctl, as we add more context types.  This is based on kraxel's
"drm/virtio: rework batching" patch and needs that to work correctly.

Gurchetan Singh (4):
  drm/virtio: use consistent names for drm_files
  drm/virtio: factor out context create cmd
  drm/virtio: track whether or not a context has been initiated
  drm/virtio: enqueue virtio_gpu_create_context after first 3D ioctl

 drivers/gpu/drm/virtio/virtgpu_drv.h   |  3 ++
 drivers/gpu/drm/virtio/virtgpu_ioctl.c | 46 ++++++++++++++++++++------
 drivers/gpu/drm/virtio/virtgpu_kms.c   | 11 +++---
 3 files changed, 44 insertions(+), 16 deletions(-)

-- 
2.25.0.225.g125e21ebc7-goog

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

[PATCH 1/4] drm/virtio: use consistent names for drm_files

[Gurchetan Singh]

Minor cleanup, change:

- file_priv--> file,
- drm_file --> file.

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

diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 75d818d707e6..89d90e95900f 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -34,12 +34,12 @@
 #include "virtgpu_drv.h"
 
 static int virtio_gpu_map_ioctl(struct drm_device *dev, void *data,
-				struct drm_file *file_priv)
+				struct drm_file *file)
 {
 	struct virtio_gpu_device *vgdev = dev->dev_private;
 	struct drm_virtgpu_map *virtio_gpu_map = data;
 
-	return virtio_gpu_mode_dumb_mmap(file_priv, vgdev->ddev,
+	return virtio_gpu_mode_dumb_mmap(file, vgdev->ddev,
 					 virtio_gpu_map->handle,
 					 &virtio_gpu_map->offset);
 }
@@ -51,11 +51,11 @@ static int virtio_gpu_map_ioctl(struct drm_device *dev, void *data,
  * VIRTIO_GPUReleaseInfo struct (first XXX bytes)
  */
 static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
-				 struct drm_file *drm_file)
+				 struct drm_file *file)
 {
 	struct drm_virtgpu_execbuffer *exbuf = data;
 	struct virtio_gpu_device *vgdev = dev->dev_private;
-	struct virtio_gpu_fpriv *vfpriv = drm_file->driver_priv;
+	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
 	struct virtio_gpu_fence *out_fence;
 	int ret;
 	uint32_t *bo_handles = NULL;
@@ -116,7 +116,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
 			goto out_unused_fd;
 		}
 
-		buflist = virtio_gpu_array_from_handles(drm_file, bo_handles,
+		buflist = virtio_gpu_array_from_handles(file, bo_handles,
 							exbuf->num_bo_handles);
 		if (!buflist) {
 			ret = -ENOENT;
@@ -178,7 +178,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
 }
 
 static int virtio_gpu_getparam_ioctl(struct drm_device *dev, void *data,
-				     struct drm_file *file_priv)
+				     struct drm_file *file)
 {
 	struct virtio_gpu_device *vgdev = dev->dev_private;
 	struct drm_virtgpu_getparam *param = data;
@@ -201,7 +201,7 @@ static int virtio_gpu_getparam_ioctl(struct drm_device *dev, void *data,
 }
 
 static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
-					    struct drm_file *file_priv)
+					    struct drm_file *file)
 {
 	struct virtio_gpu_device *vgdev = dev->dev_private;
 	struct drm_virtgpu_resource_create *rc = data;
@@ -252,7 +252,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 		return ret;
 	obj = &qobj->base.base;
 
-	ret = drm_gem_handle_create(file_priv, obj, &handle);
+	ret = drm_gem_handle_create(file, obj, &handle);
 	if (ret) {
 		drm_gem_object_release(obj);
 		return ret;
@@ -265,13 +265,13 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 }
 
 static int virtio_gpu_resource_info_ioctl(struct drm_device *dev, void *data,
-					  struct drm_file *file_priv)
+					  struct drm_file *file)
 {
 	struct drm_virtgpu_resource_info *ri = data;
 	struct drm_gem_object *gobj = NULL;
 	struct virtio_gpu_object *qobj = NULL;
 
-	gobj = drm_gem_object_lookup(file_priv, ri->bo_handle);
+	gobj = drm_gem_object_lookup(file, ri->bo_handle);
 	if (gobj == NULL)
 		return -ENOENT;
 
-- 
2.25.0.225.g125e21ebc7-goog

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

[PATCH 2/4] drm/virtio: factor out context create cmd

[Gurchetan Singh]

We currently do it when open the DRM fd, let's delay it. First step,
remove the hyercall from initialization.

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

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 28aeac8717e1..edaa7b8224a8 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -214,6 +214,8 @@ struct virtio_gpu_fpriv {
 /* virtio_ioctl.c */
 #define DRM_VIRTIO_NUM_IOCTLS 10
 extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
+void virtio_gpu_create_context(struct drm_device *dev,
+			       struct drm_file *file);
 
 /* virtio_kms.c */
 int virtio_gpu_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 89d90e95900f..f1afabaa3a08 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -33,6 +33,22 @@
 
 #include "virtgpu_drv.h"
 
+void virtio_gpu_create_context(struct drm_device *dev,
+			       struct drm_file *file)
+{
+	struct virtio_gpu_device *vgdev = dev->dev_private;
+	struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
+	char dbgname[TASK_COMM_LEN];
+
+	/* can't create contexts without 3d renderer */
+	if (!vgdev->has_virgl_3d)
+		return;
+
+	get_task_comm(dbgname, current);
+	virtio_gpu_cmd_context_create(vgdev, vfpriv->ctx_id,
+				      strlen(dbgname), dbgname);
+}
+
 static int virtio_gpu_map_ioctl(struct drm_device *dev, void *data,
 				struct drm_file *file)
 {
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 44e4c07d0162..e1e1c0821a35 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -52,15 +52,13 @@ static void virtio_gpu_config_changed_work_func(struct work_struct *work)
 		      events_clear, &events_clear);
 }
 
-static int virtio_gpu_context_create(struct virtio_gpu_device *vgdev,
-				      uint32_t nlen, const char *name)
+static int virtio_gpu_context_handle(struct virtio_gpu_device *vgdev)
 {
 	int handle = ida_alloc(&vgdev->ctx_id_ida, GFP_KERNEL);
 
 	if (handle < 0)
 		return handle;
 	handle += 1;
-	virtio_gpu_cmd_context_create(vgdev, handle, nlen, name);
 	return handle;
 }
 
@@ -256,7 +254,6 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
 	struct virtio_gpu_device *vgdev = dev->dev_private;
 	struct virtio_gpu_fpriv *vfpriv;
 	int id;
-	char dbgname[TASK_COMM_LEN];
 
 	/* can't create contexts without 3d renderer */
 	if (!vgdev->has_virgl_3d)
@@ -267,8 +264,7 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
 	if (!vfpriv)
 		return -ENOMEM;
 
-	get_task_comm(dbgname, current);
-	id = virtio_gpu_context_create(vgdev, strlen(dbgname), dbgname);
+	id = virtio_gpu_context_handle(vgdev);
 	if (id < 0) {
 		kfree(vfpriv);
 		return id;
@@ -276,6 +272,8 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
 
 	vfpriv->ctx_id = id;
 	file->driver_priv = vfpriv;
+	virtio_gpu_create_context(dev, file);
+
 	return 0;
 }
 
-- 
2.25.0.225.g125e21ebc7-goog

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

[PATCH 3/4] drm/virtio: track whether or not a context has been initiated

[Gurchetan Singh]

We only want create a new virglrenderer context after the first
3D ioctl.

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

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
index edaa7b8224a8..93ce69c0d9be 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -209,6 +209,7 @@ struct virtio_gpu_device {
 
 struct virtio_gpu_fpriv {
 	uint32_t ctx_id;
+	atomic_t context_initiated;
 };
 
 /* virtio_ioctl.c */
diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index f1afabaa3a08..858ee153fb18 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -44,9 +44,14 @@ void virtio_gpu_create_context(struct drm_device *dev,
 	if (!vgdev->has_virgl_3d)
 		return;
 
+	if (atomic_read(&vfpriv->context_initiated))
+		return;
+
 	get_task_comm(dbgname, current);
 	virtio_gpu_cmd_context_create(vgdev, vfpriv->ctx_id,
 				      strlen(dbgname), dbgname);
+
+	atomic_inc(&vfpriv->context_initiated);
 }
 
 static int virtio_gpu_map_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index e1e1c0821a35..64fe5fcbedfd 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -270,7 +270,9 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
 		return id;
 	}
 
+
 	vfpriv->ctx_id = id;
+	atomic_set(&vfpriv->context_initiated, 0);
 	file->driver_priv = vfpriv;
 	virtio_gpu_create_context(dev, file);
 
-- 
2.25.0.225.g125e21ebc7-goog

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

[PATCH 4/4] drm/virtio: enqueue virtio_gpu_create_context after first 3D ioctl

[Gurchetan Singh]

With virtio_gpu_notify(..), the create context command now is batched
with the first 3D hypercall.

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

diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
index 858ee153fb18..32f9af4bcb3a 100644
--- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
+++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
@@ -95,6 +95,7 @@ static int virtio_gpu_execbuffer_ioctl(struct drm_device *dev, void *data,
 
 	exbuf->fence_fd = -1;
 
+	virtio_gpu_create_context(dev, file);
 	if (exbuf->flags & VIRTGPU_EXECBUF_FENCE_FD_IN) {
 		struct dma_fence *in_fence;
 
@@ -246,6 +247,7 @@ static int virtio_gpu_resource_create_ioctl(struct drm_device *dev, void *data,
 			return -EINVAL;
 	}
 
+	virtio_gpu_create_context(dev, file);
 	params.format = rc->format;
 	params.width = rc->width;
 	params.height = rc->height;
@@ -319,6 +321,7 @@ static int virtio_gpu_transfer_from_host_ioctl(struct drm_device *dev,
 	if (vgdev->has_virgl_3d == false)
 		return -ENOSYS;
 
+	virtio_gpu_create_context(dev, file);
 	objs = virtio_gpu_array_from_handles(file, &args->bo_handle, 1);
 	if (objs == NULL)
 		return -ENOENT;
@@ -367,6 +370,7 @@ static int virtio_gpu_transfer_to_host_ioctl(struct drm_device *dev, void *data,
 			 args->box.w, args->box.h, args->box.x, args->box.y,
 			 objs, NULL);
 	} else {
+		virtio_gpu_create_context(dev, file);
 		ret = virtio_gpu_array_lock_resv(objs);
 		if (ret != 0)
 			goto err_put_free;
@@ -467,6 +471,7 @@ static int virtio_gpu_get_caps_ioctl(struct drm_device *dev,
 	spin_unlock(&vgdev->display_info_lock);
 
 	/* not in cache - need to talk to hw */
+	virtio_gpu_create_context(dev, file);
 	virtio_gpu_cmd_get_capset(vgdev, found_valid, args->cap_set_ver,
 				  &cache_ent);
 	virtio_gpu_notify(vgdev);
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
index 64fe5fcbedfd..277c76de8e9c 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -274,7 +274,6 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
 	vfpriv->ctx_id = id;
 	atomic_set(&vfpriv->context_initiated, 0);
 	file->driver_priv = vfpriv;
-	virtio_gpu_create_context(dev, file);
 
 	return 0;
 }
-- 
2.25.0.225.g125e21ebc7-goog

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

Re: [PATCH 3/4] drm/virtio: track whether or not a context has been initiated

[Gerd Hoffmann]

  Hi,

> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -270,7 +270,9 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
>  		return id;
>  	}
>  
> +
>  	vfpriv->ctx_id = id;

checkpatch warning here:

-:52: CHECK:LINE_SPACING: Please don't use multiple blank lines
#52: FILE: drivers/gpu/drm/virtio/virtgpu_kms.c:276:

cheers,
  Gerd

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

Re: [PATCH 2/4] drm/virtio: factor out context create cmd

[Chia-I Wu]

On Tue, Feb 11, 2020 at 3:56 PM Gurchetan Singh
<gurchetansingh@chromium.org> wrote:
>
> We currently do it when open the DRM fd, let's delay it. First step,
> remove the hyercall from initialization.
>
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> ---
>  drivers/gpu/drm/virtio/virtgpu_drv.h   |  2 ++
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 16 ++++++++++++++++
>  drivers/gpu/drm/virtio/virtgpu_kms.c   | 10 ++++------
>  3 files changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 28aeac8717e1..edaa7b8224a8 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -214,6 +214,8 @@ struct virtio_gpu_fpriv {
>  /* virtio_ioctl.c */
>  #define DRM_VIRTIO_NUM_IOCTLS 10
>  extern struct drm_ioctl_desc virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
> +void virtio_gpu_create_context(struct drm_device *dev,
> +                              struct drm_file *file);
>
>  /* virtio_kms.c */
>  int virtio_gpu_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index 89d90e95900f..f1afabaa3a08 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -33,6 +33,22 @@
>
>  #include "virtgpu_drv.h"
>
> +void virtio_gpu_create_context(struct drm_device *dev,
> +                              struct drm_file *file)
> +{
> +       struct virtio_gpu_device *vgdev = dev->dev_private;
> +       struct virtio_gpu_fpriv *vfpriv = file->driver_priv;
> +       char dbgname[TASK_COMM_LEN];
> +
> +       /* can't create contexts without 3d renderer */
> +       if (!vgdev->has_virgl_3d)
> +               return;
> +
> +       get_task_comm(dbgname, current);
> +       virtio_gpu_cmd_context_create(vgdev, vfpriv->ctx_id,
> +                                     strlen(dbgname), dbgname);
> +}
> +
>  static int virtio_gpu_map_ioctl(struct drm_device *dev, void *data,
>                                 struct drm_file *file)
>  {
> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
> index 44e4c07d0162..e1e1c0821a35 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -52,15 +52,13 @@ static void virtio_gpu_config_changed_work_func(struct work_struct *work)
>                       events_clear, &events_clear);
>  }
>
> -static int virtio_gpu_context_create(struct virtio_gpu_device *vgdev,
> -                                     uint32_t nlen, const char *name)
> +static int virtio_gpu_context_handle(struct virtio_gpu_device *vgdev)
virtio_gpu_context_id_get, to mirror virtio_gpu_resource_id_get?


>  {
>         int handle = ida_alloc(&vgdev->ctx_id_ida, GFP_KERNEL);
>
>         if (handle < 0)
>                 return handle;
>         handle += 1;
> -       virtio_gpu_cmd_context_create(vgdev, handle, nlen, name);
>         return handle;
>  }
>
> @@ -256,7 +254,6 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
>         struct virtio_gpu_device *vgdev = dev->dev_private;
>         struct virtio_gpu_fpriv *vfpriv;
>         int id;
> -       char dbgname[TASK_COMM_LEN];
>
>         /* can't create contexts without 3d renderer */
>         if (!vgdev->has_virgl_3d)
> @@ -267,8 +264,7 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
>         if (!vfpriv)
>                 return -ENOMEM;
>
> -       get_task_comm(dbgname, current);
> -       id = virtio_gpu_context_create(vgdev, strlen(dbgname), dbgname);
> +       id = virtio_gpu_context_handle(vgdev);
>         if (id < 0) {
>                 kfree(vfpriv);
>                 return id;
> @@ -276,6 +272,8 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
>
>         vfpriv->ctx_id = id;
>         file->driver_priv = vfpriv;
> +       virtio_gpu_create_context(dev, file);
> +
>         return 0;
>  }
>
> --
> 2.25.0.225.g125e21ebc7-goog
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/virtio: track whether or not a context has been initiated

[Chia-I Wu]

On Tue, Feb 11, 2020 at 3:56 PM Gurchetan Singh
<gurchetansingh@chromium.org> wrote:
>
> We only want create a new virglrenderer context after the first
> 3D ioctl.
>
> Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> ---
>  drivers/gpu/drm/virtio/virtgpu_drv.h   | 1 +
>  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 5 +++++
>  drivers/gpu/drm/virtio/virtgpu_kms.c   | 2 ++
>  3 files changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index edaa7b8224a8..93ce69c0d9be 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -209,6 +209,7 @@ struct virtio_gpu_device {
>
>  struct virtio_gpu_fpriv {
>         uint32_t ctx_id;
> +       atomic_t context_initiated;
>  };
>  /* virtio_ioctl.c */
> diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> index f1afabaa3a08..858ee153fb18 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> @@ -44,9 +44,14 @@ void virtio_gpu_create_context(struct drm_device *dev,
>         if (!vgdev->has_virgl_3d)
>                 return;
>
> +       if (atomic_read(&vfpriv->context_initiated))
> +               return;
> +
>         get_task_comm(dbgname, current);
>         virtio_gpu_cmd_context_create(vgdev, vfpriv->ctx_id,
>                                       strlen(dbgname), dbgname);
> +
> +       atomic_inc(&vfpriv->context_initiated);
>  }
This needs to be protected with a lock.  ctx_id can probably be
generated here as well.


>
>  static int virtio_gpu_map_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
> index e1e1c0821a35..64fe5fcbedfd 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -270,7 +270,9 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
>                 return id;
>         }
>
> +
>         vfpriv->ctx_id = id;
> +       atomic_set(&vfpriv->context_initiated, 0);
>         file->driver_priv = vfpriv;
>         virtio_gpu_create_context(dev, file);
>
> --
> 2.25.0.225.g125e21ebc7-goog
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/virtio: track whether or not a context has been initiated

[Gurchetan Singh]

On Wed, Feb 12, 2020 at 10:50 AM Chia-I Wu <olvaffe@gmail.com> wrote:
>
> On Tue, Feb 11, 2020 at 3:56 PM Gurchetan Singh
> <gurchetansingh@chromium.org> wrote:
> >
> > We only want create a new virglrenderer context after the first
> > 3D ioctl.
> >
> > Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> > ---
> >  drivers/gpu/drm/virtio/virtgpu_drv.h   | 1 +
> >  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 5 +++++
> >  drivers/gpu/drm/virtio/virtgpu_kms.c   | 2 ++
> >  3 files changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> > index edaa7b8224a8..93ce69c0d9be 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> > @@ -209,6 +209,7 @@ struct virtio_gpu_device {
> >
> >  struct virtio_gpu_fpriv {
> >         uint32_t ctx_id;
> > +       atomic_t context_initiated;
> >  };
> >  /* virtio_ioctl.c */
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > index f1afabaa3a08..858ee153fb18 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > @@ -44,9 +44,14 @@ void virtio_gpu_create_context(struct drm_device *dev,
> >         if (!vgdev->has_virgl_3d)
> >                 return;
> >
> > +       if (atomic_read(&vfpriv->context_initiated))
> > +               return;
> > +
> >         get_task_comm(dbgname, current);
> >         virtio_gpu_cmd_context_create(vgdev, vfpriv->ctx_id,
> >                                       strlen(dbgname), dbgname);
> > +
> > +       atomic_inc(&vfpriv->context_initiated);
> >  }
> This needs to be protected with a lock.

What do you think about atomic_add_unless(&vfpriv->context_initiated, 1, 1)?

> ctx_id can probably be
> generated here as well.

If we generate the context id in virtio_gpu_create_context, we'll have
to add error checking in all of the ioctl call-sites (since ida_alloc
may fail).

int virtio_gpu_create_context(..)
ret = virtio_gpu_create_context(dev, file);
if (ret)
    return -EINVAL

vs.

void virtio_gpu_create_context(..)
virtio_gpu_create_context(..)

Any strong opinions here?

>
> >
> >  static int virtio_gpu_map_ioctl(struct drm_device *dev, void *data,
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
> > index e1e1c0821a35..64fe5fcbedfd 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> > @@ -270,7 +270,9 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
> >                 return id;
> >         }
> >
> > +
> >         vfpriv->ctx_id = id;
> > +       atomic_set(&vfpriv->context_initiated, 0);
> >         file->driver_priv = vfpriv;
> >         virtio_gpu_create_context(dev, file);
> >
> > --
> > 2.25.0.225.g125e21ebc7-goog
> >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/virtio: track whether or not a context has been initiated

[Chia-I Wu]

On Wed, Feb 12, 2020 at 5:54 PM Gurchetan Singh
<gurchetansingh@chromium.org> wrote:
>
> On Wed, Feb 12, 2020 at 10:50 AM Chia-I Wu <olvaffe@gmail.com> wrote:
> >
> > On Tue, Feb 11, 2020 at 3:56 PM Gurchetan Singh
> > <gurchetansingh@chromium.org> wrote:
> > >
> > > We only want create a new virglrenderer context after the first
> > > 3D ioctl.
> > >
> > > Signed-off-by: Gurchetan Singh <gurchetansingh@chromium.org>
> > > ---
> > >  drivers/gpu/drm/virtio/virtgpu_drv.h   | 1 +
> > >  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 5 +++++
> > >  drivers/gpu/drm/virtio/virtgpu_kms.c   | 2 ++
> > >  3 files changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h
> > > index edaa7b8224a8..93ce69c0d9be 100644
> > > --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> > > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> > > @@ -209,6 +209,7 @@ struct virtio_gpu_device {
> > >
> > >  struct virtio_gpu_fpriv {
> > >         uint32_t ctx_id;
> > > +       atomic_t context_initiated;
> > >  };
> > >  /* virtio_ioctl.c */
> > > diff --git a/drivers/gpu/drm/virtio/virtgpu_ioctl.c b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > > index f1afabaa3a08..858ee153fb18 100644
> > > --- a/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > > +++ b/drivers/gpu/drm/virtio/virtgpu_ioctl.c
> > > @@ -44,9 +44,14 @@ void virtio_gpu_create_context(struct drm_device *dev,
> > >         if (!vgdev->has_virgl_3d)
> > >                 return;
> > >
> > > +       if (atomic_read(&vfpriv->context_initiated))
> > > +               return;
> > > +
> > >         get_task_comm(dbgname, current);
> > >         virtio_gpu_cmd_context_create(vgdev, vfpriv->ctx_id,
> > >                                       strlen(dbgname), dbgname);
> > > +
> > > +       atomic_inc(&vfpriv->context_initiated);
> > >  }
> > This needs to be protected with a lock.
>
> What do you think about atomic_add_unless(&vfpriv->context_initiated, 1, 1)?
Does that prevent virtio_gpu_cmd_context_create from being called more
than once when two threads call this function at the same time?


>
> > ctx_id can probably be
> > generated here as well.
>
> If we generate the context id in virtio_gpu_create_context, we'll have
> to add error checking in all of the ioctl call-sites (since ida_alloc
> may fail).
>
> int virtio_gpu_create_context(..)
> ret = virtio_gpu_create_context(dev, file);
> if (ret)
>     return -EINVAL
>
> vs.
>
> void virtio_gpu_create_context(..)
> virtio_gpu_create_context(..)
>
> Any strong opinions here?
Yeah, the current way should be better... unless we decide to make
virtio_gpu_cmd_context_create synchronous and check for errors.

>
> >
> > >
> > >  static int virtio_gpu_map_ioctl(struct drm_device *dev, void *data,
> > > diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c b/drivers/gpu/drm/virtio/virtgpu_kms.c
> > > index e1e1c0821a35..64fe5fcbedfd 100644
> > > --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> > > +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> > > @@ -270,7 +270,9 @@ int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file)
> > >                 return id;
> > >         }
> > >
> > > +
> > >         vfpriv->ctx_id = id;
> > > +       atomic_set(&vfpriv->context_initiated, 0);
> > >         file->driver_priv = vfpriv;
> > >         virtio_gpu_create_context(dev, file);
> > >
> > > --
> > > 2.25.0.225.g125e21ebc7-goog
> > >
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel