| Message ID | 8490dbeb6f79ed039e6c11d121002618972538a3.1744293540.git.mst@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] virtgpu: don't reset on shutdown | expand |
Hi, > +static void virtio_gpu_shutdown(struct virtio_device *vdev) > +{ > + /* > + * drm does its own synchronization on shutdown. > + * Do nothing here, opt out of device reset. > + */ I think a call to 'drm_dev_unplug()' is what you need here. take care, Gerd
On Tue, Apr 15, 2025 at 01:16:32PM +0200, Gerd Hoffmann wrote: > Hi, > > > +static void virtio_gpu_shutdown(struct virtio_device *vdev) > > +{ > > + /* > > + * drm does its own synchronization on shutdown. > > + * Do nothing here, opt out of device reset. > > + */ > > I think a call to 'drm_dev_unplug()' is what you need here. > > take care, > Gerd My patch reverts the behaviour back to what it was, so pls go ahead and send a patch on top? I won't be able to explain what it does and why it's needed.
On Tue, Apr 15, 2025 at 10:00:48AM -0400, Michael S. Tsirkin wrote: > On Tue, Apr 15, 2025 at 01:16:32PM +0200, Gerd Hoffmann wrote: > > Hi, > > > > > +static void virtio_gpu_shutdown(struct virtio_device *vdev) > > > +{ > > > + /* > > > + * drm does its own synchronization on shutdown. > > > + * Do nothing here, opt out of device reset. > > > + */ > > > > I think a call to 'drm_dev_unplug()' is what you need here. > > > > take care, > > Gerd > > My patch reverts the behaviour back to what it was, so pls go > ahead and send a patch on top? I won't be able to explain > what it does and why it's needed. See below. Untested. Eric, can you give this a spin? thanks, Gerd ----------------------- cut here ------------------------------- From f3051dd52cb2004232941e6d2cbc0c694e290534 Mon Sep 17 00:00:00 2001 From: Gerd Hoffmann <kraxel@redhat.com> Date: Wed, 16 Apr 2025 15:53:04 +0200 Subject: [PATCH] drm/virtio: implement virtio_gpu_shutdown Calling drm_dev_unplug() is the drm way to say the device is gone and can not be accessed any more. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- drivers/gpu/drm/virtio/virtgpu_drv.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c index e32e680c7197..71c6ccad4b99 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.c +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c @@ -130,10 +130,10 @@ static void virtio_gpu_remove(struct virtio_device *vdev) static void virtio_gpu_shutdown(struct virtio_device *vdev) { - /* - * drm does its own synchronization on shutdown. - * Do nothing here, opt out of device reset. - */ + struct drm_device *dev = vdev->priv; + + /* stop talking to the device */ + drm_dev_unplug(dev); } static void virtio_gpu_config_changed(struct virtio_device *vdev)
Hi Am 16.04.25 um 15:57 schrieb Gerd Hoffmann: > On Tue, Apr 15, 2025 at 10:00:48AM -0400, Michael S. Tsirkin wrote: >> On Tue, Apr 15, 2025 at 01:16:32PM +0200, Gerd Hoffmann wrote: >>> Hi, >>> >>>> +static void virtio_gpu_shutdown(struct virtio_device *vdev) >>>> +{ >>>> + /* >>>> + * drm does its own synchronization on shutdown. >>>> + * Do nothing here, opt out of device reset. >>>> + */ >>> I think a call to 'drm_dev_unplug()' is what you need here. >>> >>> take care, >>> Gerd >> My patch reverts the behaviour back to what it was, so pls go >> ahead and send a patch on top? I won't be able to explain >> what it does and why it's needed. > See below. Untested. > > Eric, can you give this a spin? > > thanks, > Gerd > > ----------------------- cut here ------------------------------- > From f3051dd52cb2004232941e6d2cbc0c694e290534 Mon Sep 17 00:00:00 2001 > From: Gerd Hoffmann <kraxel@redhat.com> > Date: Wed, 16 Apr 2025 15:53:04 +0200 > Subject: [PATCH] drm/virtio: implement virtio_gpu_shutdown > > Calling drm_dev_unplug() is the drm way to say the device > is gone and can not be accessed any more. > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > drivers/gpu/drm/virtio/virtgpu_drv.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c > index e32e680c7197..71c6ccad4b99 100644 > --- a/drivers/gpu/drm/virtio/virtgpu_drv.c > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c > @@ -130,10 +130,10 @@ static void virtio_gpu_remove(struct virtio_device *vdev) > > static void virtio_gpu_shutdown(struct virtio_device *vdev) > { > - /* > - * drm does its own synchronization on shutdown. > - * Do nothing here, opt out of device reset. > - */ > + struct drm_device *dev = vdev->priv; > + > + /* stop talking to the device */ > + drm_dev_unplug(dev); > } It's the correct approach but also requires drm_dev_enter() and drm_dev_exit() around all of the driver's hardware access. Best regards Thomas > > static void virtio_gpu_config_changed(struct virtio_device *vdev)
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c index 2d88e390feb4..e32e680c7197 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.c +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c @@ -128,6 +128,14 @@ static void virtio_gpu_remove(struct virtio_device *vdev) drm_dev_put(dev); } +static void virtio_gpu_shutdown(struct virtio_device *vdev) +{ + /* + * drm does its own synchronization on shutdown. + * Do nothing here, opt out of device reset. + */ +} + static void virtio_gpu_config_changed(struct virtio_device *vdev) { struct drm_device *dev = vdev->priv; @@ -162,6 +170,7 @@ static struct virtio_driver virtio_gpu_driver = { .id_table = id_table, .probe = virtio_gpu_probe, .remove = virtio_gpu_remove, + .shutdown = virtio_gpu_shutdown, .config_changed = virtio_gpu_config_changed }; diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c index 150753c3b578..95d5d7993e5b 100644 --- a/drivers/virtio/virtio.c +++ b/drivers/virtio/virtio.c @@ -407,6 +407,12 @@ static void virtio_dev_shutdown(struct device *_d) if (!drv) return; + /* If the driver has its own shutdown method, use that. */ + if (drv->shutdown) { + drv->shutdown(dev); + return; + } + /* * Some devices get wedged if you kick them after they are * reset. Mark all vqs as broken to make sure we don't. diff --git a/include/linux/virtio.h b/include/linux/virtio.h index 4d16c13d0df5..64cb4b04be7a 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -220,6 +220,8 @@ size_t virtio_max_dma_size(const struct virtio_device *vdev); * occurs. * @reset_done: optional function to call after transport specific reset * operation has finished. + * @shutdown: synchronize with the device on shutdown. If provided, replaces + * the virtio core implementation. */ struct virtio_driver { struct device_driver driver; @@ -237,6 +239,7 @@ struct virtio_driver { int (*restore)(struct virtio_device *dev); int (*reset_prepare)(struct virtio_device *dev); int (*reset_done)(struct virtio_device *dev); + void (*shutdown)(struct virtio_device *dev); }; #define drv_to_virtio(__drv) container_of_const(__drv, struct virtio_driver, driver)