drm/virtio: implement virtio_gpu_shutdown

[PATCH] drm/virtio: implement virtio_gpu_shutdown

Posted by Gerd Hoffmann 7 months, 1 week ago

Calling drm_dev_unplug() is the drm way to say the device
is gone and can not be accessed any more.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@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)
-- 
2.49.0

Re: [PATCH] drm/virtio: implement virtio_gpu_shutdown

Posted by Dmitry Osipenko 6 months, 3 weeks ago

On 5/7/25 11:28, Gerd Hoffmann wrote:
> Calling drm_dev_unplug() is the drm way to say the device
> is gone and can not be accessed any more.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Eric Auger <eric.auger@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)

Could you please describe whether this patch is fixing a specific
problem or it's a generic improvement for avoiding potential problems on
shutdown.

-- 
Best regards,
Dmitry

Re: [PATCH] drm/virtio: implement virtio_gpu_shutdown

Posted by Maxime Ripard 7 months, 1 week ago

Hi,

On Wed, May 07, 2025 at 10:28:21AM +0200, Gerd Hoffmann wrote:
> Calling drm_dev_unplug() is the drm way to say the device
> is gone and can not be accessed any more.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Eric Auger <eric.auger@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);

I'm not necessarily opposed to using drm_dev_unplug() here, but it's
still pretty surprising to me. It's typically used in remove, not
shutdown. The typical helper to use at shutdown is
drm_atomic_helper_shutdown.

So if the latter isn't enough or wrong, we should at least document why.

Maxime

Re: [PATCH] drm/virtio: implement virtio_gpu_shutdown

Posted by Gerd Hoffmann 7 months ago

> > 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);
> 
> I'm not necessarily opposed to using drm_dev_unplug() here, but it's
> still pretty surprising to me. It's typically used in remove, not
> shutdown. The typical helper to use at shutdown is
> drm_atomic_helper_shutdown.
> 
> So if the latter isn't enough or wrong, we should at least document why.

The intention of this is to make sure the driver stops talking to the
device (as the comment already says).

There are checks in place in the virt queue functions which will make
sure the driver will not try place new requests in the queues after
drm_dev_unplug() has been called.  Which why I decided to implement it
that way.

drm_atomic_helper_shutdown() tears down all outputs according to the
documentation.  Which is something different.  I don't think calling
drm_atomic_helper_shutdown() will do what I need here.  Calling it in
addition to drm_dev_unplug() might make sense, not sure.

Suggestions are welcome.

take care,
  Gerd

Re: [PATCH] drm/virtio: implement virtio_gpu_shutdown

Posted by Michael S. Tsirkin 6 months, 4 weeks ago

On Tue, May 13, 2025 at 12:18:44PM +0200, Gerd Hoffmann wrote:
> > > 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);
> > 
> > I'm not necessarily opposed to using drm_dev_unplug() here, but it's
> > still pretty surprising to me. It's typically used in remove, not
> > shutdown. The typical helper to use at shutdown is
> > drm_atomic_helper_shutdown.
> > 
> > So if the latter isn't enough or wrong, we should at least document why.
> 
> The intention of this is to make sure the driver stops talking to the
> device (as the comment already says).
> 
> There are checks in place in the virt queue functions which will make
> sure the driver will not try place new requests in the queues after
> drm_dev_unplug() has been called.  Which why I decided to implement it
> that way.
> 
> drm_atomic_helper_shutdown() tears down all outputs according to the
> documentation.  Which is something different.  I don't think calling
> drm_atomic_helper_shutdown() will do what I need here.  Calling it in
> addition to drm_dev_unplug() might make sense, not sure.
> 
> Suggestions are welcome.
> 
> take care,
>   Gerd


I suggest adding comments in code explaining why it's approriate here.
Want to try?

-- 
MST

Re: [PATCH] drm/virtio: implement virtio_gpu_shutdown

Posted by Maxime Ripard 6 months, 4 weeks ago

On Sun, May 18, 2025 at 05:53:59PM -0400, Michael S. Tsirkin wrote:
> On Tue, May 13, 2025 at 12:18:44PM +0200, Gerd Hoffmann wrote:
> > > > 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);
> > > 
> > > I'm not necessarily opposed to using drm_dev_unplug() here, but it's
> > > still pretty surprising to me. It's typically used in remove, not
> > > shutdown. The typical helper to use at shutdown is
> > > drm_atomic_helper_shutdown.
> > > 
> > > So if the latter isn't enough or wrong, we should at least document why.
> > 
> > The intention of this is to make sure the driver stops talking to the
> > device (as the comment already says).
> > 
> > There are checks in place in the virt queue functions which will make
> > sure the driver will not try place new requests in the queues after
> > drm_dev_unplug() has been called.  Which why I decided to implement it
> > that way.
> > 
> > drm_atomic_helper_shutdown() tears down all outputs according to the
> > documentation.  Which is something different.  I don't think calling
> > drm_atomic_helper_shutdown() will do what I need here.  Calling it in
> > addition to drm_dev_unplug() might make sense, not sure.
> > 
> > Suggestions are welcome.
> > 
> > take care,
> >   Gerd
> 
> 
> I suggest adding comments in code explaining why it's approriate here.
> Want to try?

Yes, that would be great

Maxime