All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] drm/bochs: add drm_driver.release callback.
@ 2020-02-10  9:38 ` Gerd Hoffmann
  0 siblings, 0 replies; 6+ messages in thread
From: Gerd Hoffmann @ 2020-02-10  9:38 UTC (permalink / raw)
  To: dri-devel
  Cc: daniel, Gerd Hoffmann, David Airlie,
	open list:DRM DRIVER FOR BOCHS VIRTUAL GPU, open list

Call drm_dev_unregister() first in bochs_pci_remove().  Hook
bochs_unload() into the new .release callback, to make sure cleanup
is done when all users are gone.

Add ready bool to state struct and move bochs_hw_fini() call from
bochs_unload() to bochs_pci_remove() to make sure hardware is not
touched after bochs_pci_remove returns.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/bochs/bochs.h     |  1 +
 drivers/gpu/drm/bochs/bochs_drv.c |  6 +++---
 drivers/gpu/drm/bochs/bochs_hw.c  | 14 ++++++++++++++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index 917767173ee6..f6bce8669274 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -57,6 +57,7 @@ struct bochs_device {
 	unsigned long  fb_base;
 	unsigned long  fb_size;
 	unsigned long  qext_size;
+	bool           ready;
 
 	/* mode */
 	u16 xres;
diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
index 10460878414e..60b5492739ef 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -23,7 +23,6 @@ static void bochs_unload(struct drm_device *dev)
 
 	bochs_kms_fini(bochs);
 	bochs_mm_fini(bochs);
-	bochs_hw_fini(dev);
 	kfree(bochs);
 	dev->dev_private = NULL;
 }
@@ -69,6 +68,7 @@ static struct drm_driver bochs_driver = {
 	.major			= 1,
 	.minor			= 0,
 	DRM_GEM_VRAM_DRIVER,
+	.release                = bochs_unload,
 };
 
 /* ---------------------------------------------------------------------- */
@@ -148,9 +148,9 @@ static void bochs_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
-	drm_atomic_helper_shutdown(dev);
 	drm_dev_unregister(dev);
-	bochs_unload(dev);
+	drm_atomic_helper_shutdown(dev);
+	bochs_hw_fini(dev);
 	drm_dev_put(dev);
 }
 
diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
index b615b7dfdd9d..48c1a6a8b026 100644
--- a/drivers/gpu/drm/bochs/bochs_hw.c
+++ b/drivers/gpu/drm/bochs/bochs_hw.c
@@ -168,6 +168,7 @@ int bochs_hw_init(struct drm_device *dev)
 	}
 	bochs->fb_base = addr;
 	bochs->fb_size = size;
+	bochs->ready = true;
 
 	DRM_INFO("Found bochs VGA, ID 0x%x.\n", id);
 	DRM_INFO("Framebuffer size %ld kB @ 0x%lx, %s @ 0x%lx.\n",
@@ -194,6 +195,10 @@ void bochs_hw_fini(struct drm_device *dev)
 {
 	struct bochs_device *bochs = dev->dev_private;
 
+	bochs->ready = false;
+
+	/* TODO: shot down existing vram mappings */
+
 	if (bochs->mmio)
 		iounmap(bochs->mmio);
 	if (bochs->ioports)
@@ -207,6 +212,9 @@ void bochs_hw_fini(struct drm_device *dev)
 void bochs_hw_setmode(struct bochs_device *bochs,
 		      struct drm_display_mode *mode)
 {
+	if (!bochs->ready)
+		return;
+
 	bochs->xres = mode->hdisplay;
 	bochs->yres = mode->vdisplay;
 	bochs->bpp = 32;
@@ -237,6 +245,9 @@ void bochs_hw_setmode(struct bochs_device *bochs,
 void bochs_hw_setformat(struct bochs_device *bochs,
 			const struct drm_format_info *format)
 {
+	if (!bochs->ready)
+		return;
+
 	DRM_DEBUG_DRIVER("format %c%c%c%c\n",
 			 (format->format >>  0) & 0xff,
 			 (format->format >>  8) & 0xff,
@@ -264,6 +275,9 @@ void bochs_hw_setbase(struct bochs_device *bochs,
 	unsigned long offset;
 	unsigned int vx, vy, vwidth;
 
+	if (!bochs->ready)
+		return;
+
 	bochs->stride = stride;
 	offset = (unsigned long)addr +
 		y * bochs->stride +
-- 
2.18.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH v2] drm/bochs: add drm_driver.release callback.
@ 2020-02-10  9:38 ` Gerd Hoffmann
  0 siblings, 0 replies; 6+ messages in thread
From: Gerd Hoffmann @ 2020-02-10  9:38 UTC (permalink / raw)
  To: dri-devel
  Cc: daniel, Gerd Hoffmann, David Airlie,
	open list:DRM DRIVER FOR BOCHS VIRTUAL GPU, open list

Call drm_dev_unregister() first in bochs_pci_remove().  Hook
bochs_unload() into the new .release callback, to make sure cleanup
is done when all users are gone.

Add ready bool to state struct and move bochs_hw_fini() call from
bochs_unload() to bochs_pci_remove() to make sure hardware is not
touched after bochs_pci_remove returns.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/bochs/bochs.h     |  1 +
 drivers/gpu/drm/bochs/bochs_drv.c |  6 +++---
 drivers/gpu/drm/bochs/bochs_hw.c  | 14 ++++++++++++++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index 917767173ee6..f6bce8669274 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -57,6 +57,7 @@ struct bochs_device {
 	unsigned long  fb_base;
 	unsigned long  fb_size;
 	unsigned long  qext_size;
+	bool           ready;
 
 	/* mode */
 	u16 xres;
diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
index 10460878414e..60b5492739ef 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -23,7 +23,6 @@ static void bochs_unload(struct drm_device *dev)
 
 	bochs_kms_fini(bochs);
 	bochs_mm_fini(bochs);
-	bochs_hw_fini(dev);
 	kfree(bochs);
 	dev->dev_private = NULL;
 }
@@ -69,6 +68,7 @@ static struct drm_driver bochs_driver = {
 	.major			= 1,
 	.minor			= 0,
 	DRM_GEM_VRAM_DRIVER,
+	.release                = bochs_unload,
 };
 
 /* ---------------------------------------------------------------------- */
@@ -148,9 +148,9 @@ static void bochs_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
-	drm_atomic_helper_shutdown(dev);
 	drm_dev_unregister(dev);
-	bochs_unload(dev);
+	drm_atomic_helper_shutdown(dev);
+	bochs_hw_fini(dev);
 	drm_dev_put(dev);
 }
 
diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
index b615b7dfdd9d..48c1a6a8b026 100644
--- a/drivers/gpu/drm/bochs/bochs_hw.c
+++ b/drivers/gpu/drm/bochs/bochs_hw.c
@@ -168,6 +168,7 @@ int bochs_hw_init(struct drm_device *dev)
 	}
 	bochs->fb_base = addr;
 	bochs->fb_size = size;
+	bochs->ready = true;
 
 	DRM_INFO("Found bochs VGA, ID 0x%x.\n", id);
 	DRM_INFO("Framebuffer size %ld kB @ 0x%lx, %s @ 0x%lx.\n",
@@ -194,6 +195,10 @@ void bochs_hw_fini(struct drm_device *dev)
 {
 	struct bochs_device *bochs = dev->dev_private;
 
+	bochs->ready = false;
+
+	/* TODO: shot down existing vram mappings */
+
 	if (bochs->mmio)
 		iounmap(bochs->mmio);
 	if (bochs->ioports)
@@ -207,6 +212,9 @@ void bochs_hw_fini(struct drm_device *dev)
 void bochs_hw_setmode(struct bochs_device *bochs,
 		      struct drm_display_mode *mode)
 {
+	if (!bochs->ready)
+		return;
+
 	bochs->xres = mode->hdisplay;
 	bochs->yres = mode->vdisplay;
 	bochs->bpp = 32;
@@ -237,6 +245,9 @@ void bochs_hw_setmode(struct bochs_device *bochs,
 void bochs_hw_setformat(struct bochs_device *bochs,
 			const struct drm_format_info *format)
 {
+	if (!bochs->ready)
+		return;
+
 	DRM_DEBUG_DRIVER("format %c%c%c%c\n",
 			 (format->format >>  0) & 0xff,
 			 (format->format >>  8) & 0xff,
@@ -264,6 +275,9 @@ void bochs_hw_setbase(struct bochs_device *bochs,
 	unsigned long offset;
 	unsigned int vx, vy, vwidth;
 
+	if (!bochs->ready)
+		return;
+
 	bochs->stride = stride;
 	offset = (unsigned long)addr +
 		y * bochs->stride +
-- 
2.18.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH v2] drm/bochs: add drm_driver.release callback.
@ 2020-02-10  9:38 ` Gerd Hoffmann
  0 siblings, 0 replies; 6+ messages in thread
From: Gerd Hoffmann @ 2020-02-10  9:38 UTC (permalink / raw)
  To: dri-devel
  Cc: David Airlie, open list, Gerd Hoffmann,
	open list:DRM DRIVER FOR BOCHS VIRTUAL GPU

Call drm_dev_unregister() first in bochs_pci_remove().  Hook
bochs_unload() into the new .release callback, to make sure cleanup
is done when all users are gone.

Add ready bool to state struct and move bochs_hw_fini() call from
bochs_unload() to bochs_pci_remove() to make sure hardware is not
touched after bochs_pci_remove returns.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 drivers/gpu/drm/bochs/bochs.h     |  1 +
 drivers/gpu/drm/bochs/bochs_drv.c |  6 +++---
 drivers/gpu/drm/bochs/bochs_hw.c  | 14 ++++++++++++++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
index 917767173ee6..f6bce8669274 100644
--- a/drivers/gpu/drm/bochs/bochs.h
+++ b/drivers/gpu/drm/bochs/bochs.h
@@ -57,6 +57,7 @@ struct bochs_device {
 	unsigned long  fb_base;
 	unsigned long  fb_size;
 	unsigned long  qext_size;
+	bool           ready;
 
 	/* mode */
 	u16 xres;
diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
index 10460878414e..60b5492739ef 100644
--- a/drivers/gpu/drm/bochs/bochs_drv.c
+++ b/drivers/gpu/drm/bochs/bochs_drv.c
@@ -23,7 +23,6 @@ static void bochs_unload(struct drm_device *dev)
 
 	bochs_kms_fini(bochs);
 	bochs_mm_fini(bochs);
-	bochs_hw_fini(dev);
 	kfree(bochs);
 	dev->dev_private = NULL;
 }
@@ -69,6 +68,7 @@ static struct drm_driver bochs_driver = {
 	.major			= 1,
 	.minor			= 0,
 	DRM_GEM_VRAM_DRIVER,
+	.release                = bochs_unload,
 };
 
 /* ---------------------------------------------------------------------- */
@@ -148,9 +148,9 @@ static void bochs_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
-	drm_atomic_helper_shutdown(dev);
 	drm_dev_unregister(dev);
-	bochs_unload(dev);
+	drm_atomic_helper_shutdown(dev);
+	bochs_hw_fini(dev);
 	drm_dev_put(dev);
 }
 
diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
index b615b7dfdd9d..48c1a6a8b026 100644
--- a/drivers/gpu/drm/bochs/bochs_hw.c
+++ b/drivers/gpu/drm/bochs/bochs_hw.c
@@ -168,6 +168,7 @@ int bochs_hw_init(struct drm_device *dev)
 	}
 	bochs->fb_base = addr;
 	bochs->fb_size = size;
+	bochs->ready = true;
 
 	DRM_INFO("Found bochs VGA, ID 0x%x.\n", id);
 	DRM_INFO("Framebuffer size %ld kB @ 0x%lx, %s @ 0x%lx.\n",
@@ -194,6 +195,10 @@ void bochs_hw_fini(struct drm_device *dev)
 {
 	struct bochs_device *bochs = dev->dev_private;
 
+	bochs->ready = false;
+
+	/* TODO: shot down existing vram mappings */
+
 	if (bochs->mmio)
 		iounmap(bochs->mmio);
 	if (bochs->ioports)
@@ -207,6 +212,9 @@ void bochs_hw_fini(struct drm_device *dev)
 void bochs_hw_setmode(struct bochs_device *bochs,
 		      struct drm_display_mode *mode)
 {
+	if (!bochs->ready)
+		return;
+
 	bochs->xres = mode->hdisplay;
 	bochs->yres = mode->vdisplay;
 	bochs->bpp = 32;
@@ -237,6 +245,9 @@ void bochs_hw_setmode(struct bochs_device *bochs,
 void bochs_hw_setformat(struct bochs_device *bochs,
 			const struct drm_format_info *format)
 {
+	if (!bochs->ready)
+		return;
+
 	DRM_DEBUG_DRIVER("format %c%c%c%c\n",
 			 (format->format >>  0) & 0xff,
 			 (format->format >>  8) & 0xff,
@@ -264,6 +275,9 @@ void bochs_hw_setbase(struct bochs_device *bochs,
 	unsigned long offset;
 	unsigned int vx, vy, vwidth;
 
+	if (!bochs->ready)
+		return;
+
 	bochs->stride = stride;
 	offset = (unsigned long)addr +
 		y * bochs->stride +
-- 
2.18.1

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/bochs: add drm_driver.release callback.
  2020-02-10  9:38 ` Gerd Hoffmann
  (?)
@ 2020-02-10 14:55   ` Daniel Vetter
  -1 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2020-02-10 14:55 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: dri-devel, daniel, David Airlie,
	open list:DRM DRIVER FOR BOCHS VIRTUAL GPU, open list

On Mon, Feb 10, 2020 at 10:38:01AM +0100, Gerd Hoffmann wrote:
> Call drm_dev_unregister() first in bochs_pci_remove().  Hook
> bochs_unload() into the new .release callback, to make sure cleanup
> is done when all users are gone.
> 
> Add ready bool to state struct and move bochs_hw_fini() call from
> bochs_unload() to bochs_pci_remove() to make sure hardware is not
> touched after bochs_pci_remove returns.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/bochs/bochs.h     |  1 +
>  drivers/gpu/drm/bochs/bochs_drv.c |  6 +++---
>  drivers/gpu/drm/bochs/bochs_hw.c  | 14 ++++++++++++++
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
> index 917767173ee6..f6bce8669274 100644
> --- a/drivers/gpu/drm/bochs/bochs.h
> +++ b/drivers/gpu/drm/bochs/bochs.h
> @@ -57,6 +57,7 @@ struct bochs_device {
>  	unsigned long  fb_base;
>  	unsigned long  fb_size;
>  	unsigned long  qext_size;
> +	bool           ready;
>  
>  	/* mode */
>  	u16 xres;
> diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
> index 10460878414e..60b5492739ef 100644
> --- a/drivers/gpu/drm/bochs/bochs_drv.c
> +++ b/drivers/gpu/drm/bochs/bochs_drv.c
> @@ -23,7 +23,6 @@ static void bochs_unload(struct drm_device *dev)
>  
>  	bochs_kms_fini(bochs);
>  	bochs_mm_fini(bochs);
> -	bochs_hw_fini(dev);
>  	kfree(bochs);
>  	dev->dev_private = NULL;
>  }
> @@ -69,6 +68,7 @@ static struct drm_driver bochs_driver = {
>  	.major			= 1,
>  	.minor			= 0,
>  	DRM_GEM_VRAM_DRIVER,
> +	.release                = bochs_unload,
>  };
>  
>  /* ---------------------------------------------------------------------- */
> @@ -148,9 +148,9 @@ static void bochs_pci_remove(struct pci_dev *pdev)
>  {
>  	struct drm_device *dev = pci_get_drvdata(pdev);
>  
> -	drm_atomic_helper_shutdown(dev);
>  	drm_dev_unregister(dev);
> -	bochs_unload(dev);
> +	drm_atomic_helper_shutdown(dev);
> +	bochs_hw_fini(dev);
>  	drm_dev_put(dev);
>  }
>  
> diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
> index b615b7dfdd9d..48c1a6a8b026 100644
> --- a/drivers/gpu/drm/bochs/bochs_hw.c
> +++ b/drivers/gpu/drm/bochs/bochs_hw.c
> @@ -168,6 +168,7 @@ int bochs_hw_init(struct drm_device *dev)
>  	}
>  	bochs->fb_base = addr;
>  	bochs->fb_size = size;
> +	bochs->ready = true;
>  
>  	DRM_INFO("Found bochs VGA, ID 0x%x.\n", id);
>  	DRM_INFO("Framebuffer size %ld kB @ 0x%lx, %s @ 0x%lx.\n",
> @@ -194,6 +195,10 @@ void bochs_hw_fini(struct drm_device *dev)
>  {
>  	struct bochs_device *bochs = dev->dev_private;
>  
> +	bochs->ready = false;
> +
> +	/* TODO: shot down existing vram mappings */

Aside: I'm mildly hopefull that we could do this with a generic helper,
both punching out all current ptes and replacing them with something
dummy. Since replacing them with nothing and refusing to fault stuff is
probably not going to work out well - userspace will crash&burn too much.

> +
>  	if (bochs->mmio)
>  		iounmap(bochs->mmio);
>  	if (bochs->ioports)
> @@ -207,6 +212,9 @@ void bochs_hw_fini(struct drm_device *dev)
>  void bochs_hw_setmode(struct bochs_device *bochs,
>  		      struct drm_display_mode *mode)
>  {
> +	if (!bochs->ready)
> +		return;

drm_dev_enter/exit is the primitive you're looking for I think. Don't hand
roll your own racy version of this. btw changelog in the patch missing.
Personally I'd split out the drm_dev_enter/exit in a 2nd patch, but up to
you.

The remove/release split looks correct to me now.
-Daniel


> +
>  	bochs->xres = mode->hdisplay;
>  	bochs->yres = mode->vdisplay;
>  	bochs->bpp = 32;
> @@ -237,6 +245,9 @@ void bochs_hw_setmode(struct bochs_device *bochs,
>  void bochs_hw_setformat(struct bochs_device *bochs,
>  			const struct drm_format_info *format)
>  {
> +	if (!bochs->ready)
> +		return;
> +
>  	DRM_DEBUG_DRIVER("format %c%c%c%c\n",
>  			 (format->format >>  0) & 0xff,
>  			 (format->format >>  8) & 0xff,
> @@ -264,6 +275,9 @@ void bochs_hw_setbase(struct bochs_device *bochs,
>  	unsigned long offset;
>  	unsigned int vx, vy, vwidth;
>  
> +	if (!bochs->ready)
> +		return;
> +
>  	bochs->stride = stride;
>  	offset = (unsigned long)addr +
>  		y * bochs->stride +
> -- 
> 2.18.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/bochs: add drm_driver.release callback.
@ 2020-02-10 14:55   ` Daniel Vetter
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2020-02-10 14:55 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: dri-devel, daniel, David Airlie,
	open list:DRM DRIVER FOR BOCHS VIRTUAL GPU, open list

On Mon, Feb 10, 2020 at 10:38:01AM +0100, Gerd Hoffmann wrote:
> Call drm_dev_unregister() first in bochs_pci_remove().  Hook
> bochs_unload() into the new .release callback, to make sure cleanup
> is done when all users are gone.
> 
> Add ready bool to state struct and move bochs_hw_fini() call from
> bochs_unload() to bochs_pci_remove() to make sure hardware is not
> touched after bochs_pci_remove returns.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/bochs/bochs.h     |  1 +
>  drivers/gpu/drm/bochs/bochs_drv.c |  6 +++---
>  drivers/gpu/drm/bochs/bochs_hw.c  | 14 ++++++++++++++
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
> index 917767173ee6..f6bce8669274 100644
> --- a/drivers/gpu/drm/bochs/bochs.h
> +++ b/drivers/gpu/drm/bochs/bochs.h
> @@ -57,6 +57,7 @@ struct bochs_device {
>  	unsigned long  fb_base;
>  	unsigned long  fb_size;
>  	unsigned long  qext_size;
> +	bool           ready;
>  
>  	/* mode */
>  	u16 xres;
> diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
> index 10460878414e..60b5492739ef 100644
> --- a/drivers/gpu/drm/bochs/bochs_drv.c
> +++ b/drivers/gpu/drm/bochs/bochs_drv.c
> @@ -23,7 +23,6 @@ static void bochs_unload(struct drm_device *dev)
>  
>  	bochs_kms_fini(bochs);
>  	bochs_mm_fini(bochs);
> -	bochs_hw_fini(dev);
>  	kfree(bochs);
>  	dev->dev_private = NULL;
>  }
> @@ -69,6 +68,7 @@ static struct drm_driver bochs_driver = {
>  	.major			= 1,
>  	.minor			= 0,
>  	DRM_GEM_VRAM_DRIVER,
> +	.release                = bochs_unload,
>  };
>  
>  /* ---------------------------------------------------------------------- */
> @@ -148,9 +148,9 @@ static void bochs_pci_remove(struct pci_dev *pdev)
>  {
>  	struct drm_device *dev = pci_get_drvdata(pdev);
>  
> -	drm_atomic_helper_shutdown(dev);
>  	drm_dev_unregister(dev);
> -	bochs_unload(dev);
> +	drm_atomic_helper_shutdown(dev);
> +	bochs_hw_fini(dev);
>  	drm_dev_put(dev);
>  }
>  
> diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
> index b615b7dfdd9d..48c1a6a8b026 100644
> --- a/drivers/gpu/drm/bochs/bochs_hw.c
> +++ b/drivers/gpu/drm/bochs/bochs_hw.c
> @@ -168,6 +168,7 @@ int bochs_hw_init(struct drm_device *dev)
>  	}
>  	bochs->fb_base = addr;
>  	bochs->fb_size = size;
> +	bochs->ready = true;
>  
>  	DRM_INFO("Found bochs VGA, ID 0x%x.\n", id);
>  	DRM_INFO("Framebuffer size %ld kB @ 0x%lx, %s @ 0x%lx.\n",
> @@ -194,6 +195,10 @@ void bochs_hw_fini(struct drm_device *dev)
>  {
>  	struct bochs_device *bochs = dev->dev_private;
>  
> +	bochs->ready = false;
> +
> +	/* TODO: shot down existing vram mappings */

Aside: I'm mildly hopefull that we could do this with a generic helper,
both punching out all current ptes and replacing them with something
dummy. Since replacing them with nothing and refusing to fault stuff is
probably not going to work out well - userspace will crash&burn too much.

> +
>  	if (bochs->mmio)
>  		iounmap(bochs->mmio);
>  	if (bochs->ioports)
> @@ -207,6 +212,9 @@ void bochs_hw_fini(struct drm_device *dev)
>  void bochs_hw_setmode(struct bochs_device *bochs,
>  		      struct drm_display_mode *mode)
>  {
> +	if (!bochs->ready)
> +		return;

drm_dev_enter/exit is the primitive you're looking for I think. Don't hand
roll your own racy version of this. btw changelog in the patch missing.
Personally I'd split out the drm_dev_enter/exit in a 2nd patch, but up to
you.

The remove/release split looks correct to me now.
-Daniel


> +
>  	bochs->xres = mode->hdisplay;
>  	bochs->yres = mode->vdisplay;
>  	bochs->bpp = 32;
> @@ -237,6 +245,9 @@ void bochs_hw_setmode(struct bochs_device *bochs,
>  void bochs_hw_setformat(struct bochs_device *bochs,
>  			const struct drm_format_info *format)
>  {
> +	if (!bochs->ready)
> +		return;
> +
>  	DRM_DEBUG_DRIVER("format %c%c%c%c\n",
>  			 (format->format >>  0) & 0xff,
>  			 (format->format >>  8) & 0xff,
> @@ -264,6 +275,9 @@ void bochs_hw_setbase(struct bochs_device *bochs,
>  	unsigned long offset;
>  	unsigned int vx, vy, vwidth;
>  
> +	if (!bochs->ready)
> +		return;
> +
>  	bochs->stride = stride;
>  	offset = (unsigned long)addr +
>  		y * bochs->stride +
> -- 
> 2.18.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/bochs: add drm_driver.release callback.
@ 2020-02-10 14:55   ` Daniel Vetter
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2020-02-10 14:55 UTC (permalink / raw)
  To: Gerd Hoffmann
  Cc: David Airlie, open list, dri-devel,
	open list:DRM DRIVER FOR BOCHS VIRTUAL GPU

On Mon, Feb 10, 2020 at 10:38:01AM +0100, Gerd Hoffmann wrote:
> Call drm_dev_unregister() first in bochs_pci_remove().  Hook
> bochs_unload() into the new .release callback, to make sure cleanup
> is done when all users are gone.
> 
> Add ready bool to state struct and move bochs_hw_fini() call from
> bochs_unload() to bochs_pci_remove() to make sure hardware is not
> touched after bochs_pci_remove returns.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  drivers/gpu/drm/bochs/bochs.h     |  1 +
>  drivers/gpu/drm/bochs/bochs_drv.c |  6 +++---
>  drivers/gpu/drm/bochs/bochs_hw.c  | 14 ++++++++++++++
>  3 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bochs/bochs.h b/drivers/gpu/drm/bochs/bochs.h
> index 917767173ee6..f6bce8669274 100644
> --- a/drivers/gpu/drm/bochs/bochs.h
> +++ b/drivers/gpu/drm/bochs/bochs.h
> @@ -57,6 +57,7 @@ struct bochs_device {
>  	unsigned long  fb_base;
>  	unsigned long  fb_size;
>  	unsigned long  qext_size;
> +	bool           ready;
>  
>  	/* mode */
>  	u16 xres;
> diff --git a/drivers/gpu/drm/bochs/bochs_drv.c b/drivers/gpu/drm/bochs/bochs_drv.c
> index 10460878414e..60b5492739ef 100644
> --- a/drivers/gpu/drm/bochs/bochs_drv.c
> +++ b/drivers/gpu/drm/bochs/bochs_drv.c
> @@ -23,7 +23,6 @@ static void bochs_unload(struct drm_device *dev)
>  
>  	bochs_kms_fini(bochs);
>  	bochs_mm_fini(bochs);
> -	bochs_hw_fini(dev);
>  	kfree(bochs);
>  	dev->dev_private = NULL;
>  }
> @@ -69,6 +68,7 @@ static struct drm_driver bochs_driver = {
>  	.major			= 1,
>  	.minor			= 0,
>  	DRM_GEM_VRAM_DRIVER,
> +	.release                = bochs_unload,
>  };
>  
>  /* ---------------------------------------------------------------------- */
> @@ -148,9 +148,9 @@ static void bochs_pci_remove(struct pci_dev *pdev)
>  {
>  	struct drm_device *dev = pci_get_drvdata(pdev);
>  
> -	drm_atomic_helper_shutdown(dev);
>  	drm_dev_unregister(dev);
> -	bochs_unload(dev);
> +	drm_atomic_helper_shutdown(dev);
> +	bochs_hw_fini(dev);
>  	drm_dev_put(dev);
>  }
>  
> diff --git a/drivers/gpu/drm/bochs/bochs_hw.c b/drivers/gpu/drm/bochs/bochs_hw.c
> index b615b7dfdd9d..48c1a6a8b026 100644
> --- a/drivers/gpu/drm/bochs/bochs_hw.c
> +++ b/drivers/gpu/drm/bochs/bochs_hw.c
> @@ -168,6 +168,7 @@ int bochs_hw_init(struct drm_device *dev)
>  	}
>  	bochs->fb_base = addr;
>  	bochs->fb_size = size;
> +	bochs->ready = true;
>  
>  	DRM_INFO("Found bochs VGA, ID 0x%x.\n", id);
>  	DRM_INFO("Framebuffer size %ld kB @ 0x%lx, %s @ 0x%lx.\n",
> @@ -194,6 +195,10 @@ void bochs_hw_fini(struct drm_device *dev)
>  {
>  	struct bochs_device *bochs = dev->dev_private;
>  
> +	bochs->ready = false;
> +
> +	/* TODO: shot down existing vram mappings */

Aside: I'm mildly hopefull that we could do this with a generic helper,
both punching out all current ptes and replacing them with something
dummy. Since replacing them with nothing and refusing to fault stuff is
probably not going to work out well - userspace will crash&burn too much.

> +
>  	if (bochs->mmio)
>  		iounmap(bochs->mmio);
>  	if (bochs->ioports)
> @@ -207,6 +212,9 @@ void bochs_hw_fini(struct drm_device *dev)
>  void bochs_hw_setmode(struct bochs_device *bochs,
>  		      struct drm_display_mode *mode)
>  {
> +	if (!bochs->ready)
> +		return;

drm_dev_enter/exit is the primitive you're looking for I think. Don't hand
roll your own racy version of this. btw changelog in the patch missing.
Personally I'd split out the drm_dev_enter/exit in a 2nd patch, but up to
you.

The remove/release split looks correct to me now.
-Daniel


> +
>  	bochs->xres = mode->hdisplay;
>  	bochs->yres = mode->vdisplay;
>  	bochs->bpp = 32;
> @@ -237,6 +245,9 @@ void bochs_hw_setmode(struct bochs_device *bochs,
>  void bochs_hw_setformat(struct bochs_device *bochs,
>  			const struct drm_format_info *format)
>  {
> +	if (!bochs->ready)
> +		return;
> +
>  	DRM_DEBUG_DRIVER("format %c%c%c%c\n",
>  			 (format->format >>  0) & 0xff,
>  			 (format->format >>  8) & 0xff,
> @@ -264,6 +275,9 @@ void bochs_hw_setbase(struct bochs_device *bochs,
>  	unsigned long offset;
>  	unsigned int vx, vy, vwidth;
>  
> +	if (!bochs->ready)
> +		return;
> +
>  	bochs->stride = stride;
>  	offset = (unsigned long)addr +
>  		y * bochs->stride +
> -- 
> 2.18.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-02-10 14:55 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-10  9:38 [PATCH v2] drm/bochs: add drm_driver.release callback Gerd Hoffmann
2020-02-10  9:38 ` Gerd Hoffmann
2020-02-10  9:38 ` Gerd Hoffmann
2020-02-10 14:55 ` Daniel Vetter
2020-02-10 14:55   ` Daniel Vetter
2020-02-10 14:55   ` Daniel Vetter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.