thr3ads.net - Virtualization - [PATCH v4] drm/virtio: add drm

If this information is useful, please help other people find it:
Share via:

Gerd Hoffmann

2020-Feb-11 13:58 UTC

[PATCH v4] drm/virtio: add drm_driver.release callback.

Split virtio_gpu_deinit(), move the drm shutdown and release to
virtio_gpu_release().  Drop vqs_ready variable, instead use
drm_dev_{enter,exit,unplug} to avoid touching hardware after
device removal.  Tidy up here and there.

v4: add changelog.
v3: use drm_dev_*().

Signed-off-by: Gerd Hoffmann <kraxel at redhat.com>
---
 drivers/gpu/drm/virtio/virtgpu_drv.h     |  3 ++-
 drivers/gpu/drm/virtio/virtgpu_display.c |  1 -
 drivers/gpu/drm/virtio/virtgpu_drv.c     |  6 +++++-
 drivers/gpu/drm/virtio/virtgpu_kms.c     |  7 ++++--
 drivers/gpu/drm/virtio/virtgpu_vq.c      | 27 +++++++++++++-----------
 5 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
index 7fd8361e1c9e..af9403e1cf78 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.h
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
@@ -32,6 +32,7 @@
 #include <linux/virtio_gpu.h>
 
 #include <drm/drm_atomic.h>
+#include <drm/drm_drv.h>
 #include <drm/drm_encoder.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_gem.h>
@@ -177,7 +178,6 @@ struct virtio_gpu_device {
 	struct virtio_gpu_queue ctrlq;
 	struct virtio_gpu_queue cursorq;
 	struct kmem_cache *vbufs;
-	bool vqs_ready;
 
 	bool disable_notify;
 	bool pending_notify;
@@ -219,6 +219,7 @@ extern struct drm_ioctl_desc
virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
 /* virtio_kms.c */
 int virtio_gpu_init(struct drm_device *dev);
 void virtio_gpu_deinit(struct drm_device *dev);
+void virtio_gpu_release(struct drm_device *dev);
 int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file);
 void virtio_gpu_driver_postclose(struct drm_device *dev, struct drm_file
*file);
 
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c
b/drivers/gpu/drm/virtio/virtgpu_display.c
index 7b0f0643bb2d..af953db4a0c9 100644
--- a/drivers/gpu/drm/virtio/virtgpu_display.c
+++ b/drivers/gpu/drm/virtio/virtgpu_display.c
@@ -368,6 +368,5 @@ void virtio_gpu_modeset_fini(struct virtio_gpu_device
*vgdev)
 
 	for (i = 0 ; i < vgdev->num_scanouts; ++i)
 		kfree(vgdev->outputs[i].edid);
-	drm_atomic_helper_shutdown(vgdev->ddev);
 	drm_mode_config_cleanup(vgdev->ddev);
 }
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
b/drivers/gpu/drm/virtio/virtgpu_drv.c
index 8cf27af3ad53..ab4bed78e656 100644
--- a/drivers/gpu/drm/virtio/virtgpu_drv.c
+++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
@@ -31,6 +31,7 @@
 #include <linux/pci.h>
 
 #include <drm/drm.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_file.h>
 
@@ -135,7 +136,8 @@ static void virtio_gpu_remove(struct virtio_device *vdev)
 {
 	struct drm_device *dev = vdev->priv;
 
-	drm_dev_unregister(dev);
+	drm_dev_unplug(dev);
+	drm_atomic_helper_shutdown(dev);
 	virtio_gpu_deinit(dev);
 	drm_dev_put(dev);
 }
@@ -214,4 +216,6 @@ static struct drm_driver driver = {
 	.major = DRIVER_MAJOR,
 	.minor = DRIVER_MINOR,
 	.patchlevel = DRIVER_PATCHLEVEL,
+
+	.release = virtio_gpu_release,
 };
diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c
b/drivers/gpu/drm/virtio/virtgpu_kms.c
index c1086df49816..4009c2f97d08 100644
--- a/drivers/gpu/drm/virtio/virtgpu_kms.c
+++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
@@ -199,7 +199,6 @@ int virtio_gpu_init(struct drm_device *dev)
 	virtio_gpu_modeset_init(vgdev);
 
 	virtio_device_ready(vgdev->vdev);
-	vgdev->vqs_ready = true;
 
 	if (num_capsets)
 		virtio_gpu_get_capsets(vgdev, num_capsets);
@@ -234,12 +233,16 @@ void virtio_gpu_deinit(struct drm_device *dev)
 	struct virtio_gpu_device *vgdev = dev->dev_private;
 
 	flush_work(&vgdev->obj_free_work);
-	vgdev->vqs_ready = false;
 	flush_work(&vgdev->ctrlq.dequeue_work);
 	flush_work(&vgdev->cursorq.dequeue_work);
 	flush_work(&vgdev->config_changed_work);
 	vgdev->vdev->config->reset(vgdev->vdev);
 	vgdev->vdev->config->del_vqs(vgdev->vdev);
+}
+
+void virtio_gpu_release(struct drm_device *dev)
+{
+	struct virtio_gpu_device *vgdev = dev->dev_private;
 
 	virtio_gpu_modeset_fini(vgdev);
 	virtio_gpu_free_vbufs(vgdev);
diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c
b/drivers/gpu/drm/virtio/virtgpu_vq.c
index a682c2fcbe9a..cfe9c54f87a3 100644
--- a/drivers/gpu/drm/virtio/virtgpu_vq.c
+++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
@@ -330,7 +330,14 @@ static void virtio_gpu_queue_ctrl_sgs(struct
virtio_gpu_device *vgdev,
 {
 	struct virtqueue *vq = vgdev->ctrlq.vq;
 	bool notify = false;
-	int ret;
+	int ret, idx;
+
+	if (!drm_dev_enter(vgdev->ddev, &idx)) {
+		if (fence && vbuf->objs)
+			virtio_gpu_array_unlock_resv(vbuf->objs);
+		free_vbuf(vgdev, vbuf);
+		return;
+	}
 
 	if (vgdev->has_indirect)
 		elemcnt = 1;
@@ -338,14 +345,6 @@ static void virtio_gpu_queue_ctrl_sgs(struct
virtio_gpu_device *vgdev,
 again:
 	spin_lock(&vgdev->ctrlq.qlock);
 
-	if (!vgdev->vqs_ready) {
-		spin_unlock(&vgdev->ctrlq.qlock);
-
-		if (fence && vbuf->objs)
-			virtio_gpu_array_unlock_resv(vbuf->objs);
-		return;
-	}
-
 	if (vq->num_free < elemcnt) {
 		spin_unlock(&vgdev->ctrlq.qlock);
 		wait_event(vgdev->ctrlq.ack_queue, vq->num_free >= elemcnt);
@@ -379,6 +378,7 @@ static void virtio_gpu_queue_ctrl_sgs(struct
virtio_gpu_device *vgdev,
 		else
 			virtqueue_notify(vq);
 	}
+	drm_dev_exit(idx);
 }
 
 static void virtio_gpu_queue_fenced_ctrl_buffer(struct virtio_gpu_device
*vgdev,
@@ -460,12 +460,13 @@ static void virtio_gpu_queue_cursor(struct
virtio_gpu_device *vgdev,
 {
 	struct virtqueue *vq = vgdev->cursorq.vq;
 	struct scatterlist *sgs[1], ccmd;
+	int idx, ret, outcnt;
 	bool notify;
-	int ret;
-	int outcnt;
 
-	if (!vgdev->vqs_ready)
+	if (!drm_dev_enter(vgdev->ddev, &idx)) {
+		free_vbuf(vgdev, vbuf);
 		return;
+	}
 
 	sg_init_one(&ccmd, vbuf->buf, vbuf->size);
 	sgs[0] = &ccmd;
@@ -490,6 +491,8 @@ static void virtio_gpu_queue_cursor(struct virtio_gpu_device
*vgdev,
 
 	if (notify)
 		virtqueue_notify(vq);
+
+	drm_dev_exit(idx);
 }
 
 /* just create gem objects for userspace and long lived objects,
-- 
2.18.2

Daniel Vetter

2020-Feb-11 14:27 UTC

head link

[PATCH v4] drm/virtio: add drm_driver.release callback.

On Tue, Feb 11, 2020 at 02:58:04PM +0100, Gerd Hoffmann
wrote:> Split virtio_gpu_deinit(), move the drm shutdown and release to
> virtio_gpu_release().  Drop vqs_ready variable, instead use
> drm_dev_{enter,exit,unplug} to avoid touching hardware after
> device removal.  Tidy up here and there.
> 
> v4: add changelog.
> v3: use drm_dev_*().
> 
> Signed-off-by: Gerd Hoffmann <kraxel at redhat.com>
Looks reasonable I think.

Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>

I didn't review whether you need more drm_dev_enter/exit pairs, virtio is
a bit more complex for that and I have no idea how exactly it works. Maybe
for these more complex drivers we need a drm_dev_assert_entered() or so
that uses the right srcu lockdep annotations to make sure we do this
right. Sprinkling that check into a few low-level hw functions (touching
registers or whatever) should catch most issues.
-Daniel
> ---
>  drivers/gpu/drm/virtio/virtgpu_drv.h     |  3 ++-
>  drivers/gpu/drm/virtio/virtgpu_display.c |  1 -
>  drivers/gpu/drm/virtio/virtgpu_drv.c     |  6 +++++-
>  drivers/gpu/drm/virtio/virtgpu_kms.c     |  7 ++++--
>  drivers/gpu/drm/virtio/virtgpu_vq.c      | 27 +++++++++++++-----------
>  5 files changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 7fd8361e1c9e..af9403e1cf78 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -32,6 +32,7 @@
>  #include <linux/virtio_gpu.h>
>  
>  #include <drm/drm_atomic.h>
> +#include <drm/drm_drv.h>
>  #include <drm/drm_encoder.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_gem.h>
> @@ -177,7 +178,6 @@ struct virtio_gpu_device {
>  	struct virtio_gpu_queue ctrlq;
>  	struct virtio_gpu_queue cursorq;
>  	struct kmem_cache *vbufs;
> -	bool vqs_ready;
>  
>  	bool disable_notify;
>  	bool pending_notify;
> @@ -219,6 +219,7 @@ extern struct drm_ioctl_desc
virtio_gpu_ioctls[DRM_VIRTIO_NUM_IOCTLS];
>  /* virtio_kms.c */
>  int virtio_gpu_init(struct drm_device *dev);
>  void virtio_gpu_deinit(struct drm_device *dev);
> +void virtio_gpu_release(struct drm_device *dev);
>  int virtio_gpu_driver_open(struct drm_device *dev, struct drm_file *file);
>  void virtio_gpu_driver_postclose(struct drm_device *dev, struct drm_file
*file);
>  
> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c
b/drivers/gpu/drm/virtio/virtgpu_display.c
> index 7b0f0643bb2d..af953db4a0c9 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_display.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c
> @@ -368,6 +368,5 @@ void virtio_gpu_modeset_fini(struct virtio_gpu_device
*vgdev)
>  
>  	for (i = 0 ; i < vgdev->num_scanouts; ++i)
>  		kfree(vgdev->outputs[i].edid);
> -	drm_atomic_helper_shutdown(vgdev->ddev);
>  	drm_mode_config_cleanup(vgdev->ddev);
>  }
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c
b/drivers/gpu/drm/virtio/virtgpu_drv.c
> index 8cf27af3ad53..ab4bed78e656 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c
> @@ -31,6 +31,7 @@
>  #include <linux/pci.h>
>  
>  #include <drm/drm.h>
> +#include <drm/drm_atomic_helper.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_file.h>
>  
> @@ -135,7 +136,8 @@ static void virtio_gpu_remove(struct virtio_device
*vdev)
>  {
>  	struct drm_device *dev = vdev->priv;
>  
> -	drm_dev_unregister(dev);
> +	drm_dev_unplug(dev);
> +	drm_atomic_helper_shutdown(dev);
>  	virtio_gpu_deinit(dev);
>  	drm_dev_put(dev);
>  }
> @@ -214,4 +216,6 @@ static struct drm_driver driver = {
>  	.major = DRIVER_MAJOR,
>  	.minor = DRIVER_MINOR,
>  	.patchlevel = DRIVER_PATCHLEVEL,
> +
> +	.release = virtio_gpu_release,
>  };
> diff --git a/drivers/gpu/drm/virtio/virtgpu_kms.c
b/drivers/gpu/drm/virtio/virtgpu_kms.c
> index c1086df49816..4009c2f97d08 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_kms.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_kms.c
> @@ -199,7 +199,6 @@ int virtio_gpu_init(struct drm_device *dev)
>  	virtio_gpu_modeset_init(vgdev);
>  
>  	virtio_device_ready(vgdev->vdev);
> -	vgdev->vqs_ready = true;
>  
>  	if (num_capsets)
>  		virtio_gpu_get_capsets(vgdev, num_capsets);
> @@ -234,12 +233,16 @@ void virtio_gpu_deinit(struct drm_device *dev)
>  	struct virtio_gpu_device *vgdev = dev->dev_private;
>  
>  	flush_work(&vgdev->obj_free_work);
> -	vgdev->vqs_ready = false;
>  	flush_work(&vgdev->ctrlq.dequeue_work);
>  	flush_work(&vgdev->cursorq.dequeue_work);
>  	flush_work(&vgdev->config_changed_work);
>  	vgdev->vdev->config->reset(vgdev->vdev);
>  	vgdev->vdev->config->del_vqs(vgdev->vdev);
> +}
> +
> +void virtio_gpu_release(struct drm_device *dev)
> +{
> +	struct virtio_gpu_device *vgdev = dev->dev_private;
>  
>  	virtio_gpu_modeset_fini(vgdev);
>  	virtio_gpu_free_vbufs(vgdev);
> diff --git a/drivers/gpu/drm/virtio/virtgpu_vq.c
b/drivers/gpu/drm/virtio/virtgpu_vq.c
> index a682c2fcbe9a..cfe9c54f87a3 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_vq.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_vq.c
> @@ -330,7 +330,14 @@ static void virtio_gpu_queue_ctrl_sgs(struct
virtio_gpu_device *vgdev,
>  {
>  	struct virtqueue *vq = vgdev->ctrlq.vq;
>  	bool notify = false;
> -	int ret;
> +	int ret, idx;
> +
> +	if (!drm_dev_enter(vgdev->ddev, &idx)) {
> +		if (fence && vbuf->objs)
> +			virtio_gpu_array_unlock_resv(vbuf->objs);
> +		free_vbuf(vgdev, vbuf);
> +		return;
> +	}
>  
>  	if (vgdev->has_indirect)
>  		elemcnt = 1;
> @@ -338,14 +345,6 @@ static void virtio_gpu_queue_ctrl_sgs(struct
virtio_gpu_device *vgdev,
>  again:
>  	spin_lock(&vgdev->ctrlq.qlock);
>  
> -	if (!vgdev->vqs_ready) {
> -		spin_unlock(&vgdev->ctrlq.qlock);
> -
> -		if (fence && vbuf->objs)
> -			virtio_gpu_array_unlock_resv(vbuf->objs);
> -		return;
> -	}
> -
>  	if (vq->num_free < elemcnt) {
>  		spin_unlock(&vgdev->ctrlq.qlock);
>  		wait_event(vgdev->ctrlq.ack_queue, vq->num_free >= elemcnt);
> @@ -379,6 +378,7 @@ static void virtio_gpu_queue_ctrl_sgs(struct
virtio_gpu_device *vgdev,
>  		else
>  			virtqueue_notify(vq);
>  	}
> +	drm_dev_exit(idx);
>  }
>  
>  static void virtio_gpu_queue_fenced_ctrl_buffer(struct virtio_gpu_device
*vgdev,
> @@ -460,12 +460,13 @@ static void virtio_gpu_queue_cursor(struct
virtio_gpu_device *vgdev,
>  {
>  	struct virtqueue *vq = vgdev->cursorq.vq;
>  	struct scatterlist *sgs[1], ccmd;
> +	int idx, ret, outcnt;
>  	bool notify;
> -	int ret;
> -	int outcnt;
>  
> -	if (!vgdev->vqs_ready)
> +	if (!drm_dev_enter(vgdev->ddev, &idx)) {
> +		free_vbuf(vgdev, vbuf);
>  		return;
> +	}
>  
>  	sg_init_one(&ccmd, vbuf->buf, vbuf->size);
>  	sgs[0] = &ccmd;
> @@ -490,6 +491,8 @@ static void virtio_gpu_queue_cursor(struct
virtio_gpu_device *vgdev,
>  
>  	if (notify)
>  		virtqueue_notify(vq);
> +
> +	drm_dev_exit(idx);
>  }
>  
>  /* just create gem objects for userspace and long lived objects,
> -- 
> 2.18.2
> 
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Gerd Hoffmann

2020-Feb-12 09:35 UTC

head link

[PATCH v4] drm/virtio: add drm_driver.release callback.

On Tue, Feb 11, 2020 at 03:27:11PM +0100, Daniel Vetter
wrote:> On Tue, Feb 11, 2020 at 02:58:04PM +0100, Gerd Hoffmann wrote:
> > Split virtio_gpu_deinit(), move the drm shutdown and release to
> > virtio_gpu_release().  Drop vqs_ready variable, instead use
> > drm_dev_{enter,exit,unplug} to avoid touching hardware after
> > device removal.  Tidy up here and there.
> > 
> > v4: add changelog.
> > v3: use drm_dev_*().
> > 
> > Signed-off-by: Gerd Hoffmann <kraxel at redhat.com>
> 
> Looks reasonable I think.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> 
> I didn't review whether you need more drm_dev_enter/exit pairs, virtio
is
> a bit more complex for that and I have no idea how exactly it works.
virtio uses two rings to send commands to the device, one to move the
cursor and one for everything else.  So pretty much everything ends up
calling either this ...
> > @@ -330,7 +330,14 @@ static void virtio_gpu_queue_ctrl_sgs(struct
virtio_gpu_device *vgdev,
... or this ...
> > @@ -460,12 +460,13 @@ static void virtio_gpu_queue_cursor(struct
virtio_gpu_device *vgdev,
... to submit some request to the (virtual) hardware.  Therefore we
don't need many drm_dev_enter/exit pairs to cover everything ;)

cheers,
  Gerd

Possibly Parallel Threads

Search for more seemingly similar threads

Virtualization - Feb 2020 - [PATCH v4] drm/virtio: add drm_driver.release callback.

[PATCH v4] drm/virtio: add drm_driver.release callback.

[PATCH v4] drm/virtio: add drm_driver.release callback.

[PATCH v4] drm/virtio: add drm_driver.release callback.

Possibly Parallel Threads