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 <[email protected]>
---
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
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 <[email protected]>
Looks reasonable I think.
Reviewed-by: Daniel Vetter <[email protected]>
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
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 <[email protected]>
>
> Looks reasonable I think.
>
> Reviewed-by: Daniel Vetter <[email protected]>
>
> 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