Move final cleanups from cirrus_pci_remove() to the new callback.
Add drm_atomic_helper_shutdown() call to cirrus_pci_remove().
Use drm_dev_{enter,exit,unplug} to avoid touching hardware after
device removal.
v4: add changelog.
v3: use drm_dev*.
v2: stop touching hardware after pci_remove().
Signed-off-by: Gerd Hoffmann <[email protected]>
---
drivers/gpu/drm/cirrus/cirrus.c | 43 ++++++++++++++++++++++++++++-----
1 file changed, 37 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/cirrus/cirrus.c b/drivers/gpu/drm/cirrus/cirrus.c
index a91fb0d7282c..d2ff63ce8eaf 100644
--- a/drivers/gpu/drm/cirrus/cirrus.c
+++ b/drivers/gpu/drm/cirrus/cirrus.c
@@ -151,9 +151,13 @@ static int cirrus_pitch(struct drm_framebuffer *fb)
static void cirrus_set_start_address(struct cirrus_device *cirrus, u32 offset)
{
+ int idx;
u32 addr;
u8 tmp;
+ if (!drm_dev_enter(&cirrus->dev, &idx))
+ return;
+
addr = offset >> 2;
wreg_crt(cirrus, 0x0c, (u8)((addr >> 8) & 0xff));
wreg_crt(cirrus, 0x0d, (u8)(addr & 0xff));
@@ -168,6 +172,8 @@ static void cirrus_set_start_address(struct cirrus_device *cirrus, u32 offset)
tmp &= 0x7f;
tmp |= (addr >> 12) & 0x80;
wreg_crt(cirrus, 0x1d, tmp);
+
+ drm_dev_exit(idx);
}
static int cirrus_mode_set(struct cirrus_device *cirrus,
@@ -176,9 +182,12 @@ static int cirrus_mode_set(struct cirrus_device *cirrus,
{
int hsyncstart, hsyncend, htotal, hdispend;
int vtotal, vdispend;
- int tmp;
+ int tmp, idx;
int sr07 = 0, hdr = 0;
+ if (!drm_dev_enter(&cirrus->dev, &idx))
+ return -1;
+
htotal = mode->htotal / 8;
hsyncend = mode->hsync_end / 8;
hsyncstart = mode->hsync_start / 8;
@@ -264,6 +273,7 @@ static int cirrus_mode_set(struct cirrus_device *cirrus,
hdr = 0xc5;
break;
default:
+ drm_dev_exit(idx);
return -1;
}
@@ -292,6 +302,8 @@ static int cirrus_mode_set(struct cirrus_device *cirrus,
/* Unblank (needed on S3 resume, vgabios doesn't do it then) */
outb(0x20, 0x3c0);
+
+ drm_dev_exit(idx);
return 0;
}
@@ -300,10 +312,16 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb,
{
struct cirrus_device *cirrus = fb->dev->dev_private;
void *vmap;
+ int idx, ret;
+ ret = -ENODEV;
+ if (!drm_dev_enter(&cirrus->dev, &idx))
+ goto out;
+
+ ret = -ENOMEM;
vmap = drm_gem_shmem_vmap(fb->obj[0]);
if (!vmap)
- return -ENOMEM;
+ goto out_dev_exit;
if (cirrus->cpp == fb->format->cpp[0])
drm_fb_memcpy_dstclip(cirrus->vram,
@@ -323,7 +341,12 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb,
WARN_ON_ONCE("cpp mismatch");
drm_gem_shmem_vunmap(fb->obj[0], vmap);
- return 0;
+ ret = 0;
+
+out_dev_exit:
+ drm_dev_exit(idx);
+out:
+ return ret;
}
static int cirrus_fb_blit_fullscreen(struct drm_framebuffer *fb)
@@ -502,6 +525,14 @@ static void cirrus_mode_config_init(struct cirrus_device *cirrus)
/* ------------------------------------------------------------------ */
+static void cirrus_release(struct drm_device *dev)
+{
+ struct cirrus_device *cirrus = dev->dev_private;
+
+ drm_mode_config_cleanup(dev);
+ kfree(cirrus);
+}
+
DEFINE_DRM_GEM_FOPS(cirrus_fops);
static struct drm_driver cirrus_driver = {
@@ -515,6 +546,7 @@ static struct drm_driver cirrus_driver = {
.fops = &cirrus_fops,
DRM_GEM_SHMEM_DRIVER_OPS,
+ .release = cirrus_release,
};
static int cirrus_pci_probe(struct pci_dev *pdev,
@@ -598,12 +630,11 @@ static void cirrus_pci_remove(struct pci_dev *pdev)
struct drm_device *dev = pci_get_drvdata(pdev);
struct cirrus_device *cirrus = dev->dev_private;
- drm_dev_unregister(dev);
- drm_mode_config_cleanup(dev);
+ drm_dev_unplug(dev);
+ drm_atomic_helper_shutdown(dev);
iounmap(cirrus->mmio);
iounmap(cirrus->vram);
drm_dev_put(dev);
- kfree(cirrus);
pci_release_regions(pdev);
}
--
2.18.2
On Tue, Feb 11, 2020 at 02:55:22PM +0100, Gerd Hoffmann wrote:
> Move final cleanups from cirrus_pci_remove() to the new callback.
> Add drm_atomic_helper_shutdown() call to cirrus_pci_remove().
>
> Use drm_dev_{enter,exit,unplug} to avoid touching hardware after
> device removal.
>
> v4: add changelog.
> v3: use drm_dev*.
> v2: stop touching hardware after pci_remove().
>
> Signed-off-by: Gerd Hoffmann <[email protected]>
I think you got them all with drm_dev_enter/exit.
Reviewed-by: Daniel Vetter <[email protected]>
Aside: everyone ignores the return value of cirrus_fb_blit_rect and
cirrus_mode_set (with atomic those shouldn't be able to fail anymore),
could ditch those.
-Daniel
> ---
> drivers/gpu/drm/cirrus/cirrus.c | 43 ++++++++++++++++++++++++++++-----
> 1 file changed, 37 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/cirrus/cirrus.c b/drivers/gpu/drm/cirrus/cirrus.c
> index a91fb0d7282c..d2ff63ce8eaf 100644
> --- a/drivers/gpu/drm/cirrus/cirrus.c
> +++ b/drivers/gpu/drm/cirrus/cirrus.c
> @@ -151,9 +151,13 @@ static int cirrus_pitch(struct drm_framebuffer *fb)
>
> static void cirrus_set_start_address(struct cirrus_device *cirrus, u32 offset)
> {
> + int idx;
> u32 addr;
> u8 tmp;
>
> + if (!drm_dev_enter(&cirrus->dev, &idx))
> + return;
> +
> addr = offset >> 2;
> wreg_crt(cirrus, 0x0c, (u8)((addr >> 8) & 0xff));
> wreg_crt(cirrus, 0x0d, (u8)(addr & 0xff));
> @@ -168,6 +172,8 @@ static void cirrus_set_start_address(struct cirrus_device *cirrus, u32 offset)
> tmp &= 0x7f;
> tmp |= (addr >> 12) & 0x80;
> wreg_crt(cirrus, 0x1d, tmp);
> +
> + drm_dev_exit(idx);
> }
>
> static int cirrus_mode_set(struct cirrus_device *cirrus,
> @@ -176,9 +182,12 @@ static int cirrus_mode_set(struct cirrus_device *cirrus,
> {
> int hsyncstart, hsyncend, htotal, hdispend;
> int vtotal, vdispend;
> - int tmp;
> + int tmp, idx;
> int sr07 = 0, hdr = 0;
>
> + if (!drm_dev_enter(&cirrus->dev, &idx))
> + return -1;
> +
> htotal = mode->htotal / 8;
> hsyncend = mode->hsync_end / 8;
> hsyncstart = mode->hsync_start / 8;
> @@ -264,6 +273,7 @@ static int cirrus_mode_set(struct cirrus_device *cirrus,
> hdr = 0xc5;
> break;
> default:
> + drm_dev_exit(idx);
> return -1;
> }
>
> @@ -292,6 +302,8 @@ static int cirrus_mode_set(struct cirrus_device *cirrus,
>
> /* Unblank (needed on S3 resume, vgabios doesn't do it then) */
> outb(0x20, 0x3c0);
> +
> + drm_dev_exit(idx);
> return 0;
> }
>
> @@ -300,10 +312,16 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb,
> {
> struct cirrus_device *cirrus = fb->dev->dev_private;
> void *vmap;
> + int idx, ret;
>
> + ret = -ENODEV;
> + if (!drm_dev_enter(&cirrus->dev, &idx))
> + goto out;
> +
> + ret = -ENOMEM;
> vmap = drm_gem_shmem_vmap(fb->obj[0]);
> if (!vmap)
> - return -ENOMEM;
> + goto out_dev_exit;
>
> if (cirrus->cpp == fb->format->cpp[0])
> drm_fb_memcpy_dstclip(cirrus->vram,
> @@ -323,7 +341,12 @@ static int cirrus_fb_blit_rect(struct drm_framebuffer *fb,
> WARN_ON_ONCE("cpp mismatch");
>
> drm_gem_shmem_vunmap(fb->obj[0], vmap);
> - return 0;
> + ret = 0;
> +
> +out_dev_exit:
> + drm_dev_exit(idx);
> +out:
> + return ret;
> }
>
> static int cirrus_fb_blit_fullscreen(struct drm_framebuffer *fb)
> @@ -502,6 +525,14 @@ static void cirrus_mode_config_init(struct cirrus_device *cirrus)
>
> /* ------------------------------------------------------------------ */
>
> +static void cirrus_release(struct drm_device *dev)
> +{
> + struct cirrus_device *cirrus = dev->dev_private;
> +
> + drm_mode_config_cleanup(dev);
> + kfree(cirrus);
> +}
> +
> DEFINE_DRM_GEM_FOPS(cirrus_fops);
>
> static struct drm_driver cirrus_driver = {
> @@ -515,6 +546,7 @@ static struct drm_driver cirrus_driver = {
>
> .fops = &cirrus_fops,
> DRM_GEM_SHMEM_DRIVER_OPS,
> + .release = cirrus_release,
> };
>
> static int cirrus_pci_probe(struct pci_dev *pdev,
> @@ -598,12 +630,11 @@ static void cirrus_pci_remove(struct pci_dev *pdev)
> struct drm_device *dev = pci_get_drvdata(pdev);
> struct cirrus_device *cirrus = dev->dev_private;
>
> - drm_dev_unregister(dev);
> - drm_mode_config_cleanup(dev);
> + drm_dev_unplug(dev);
> + drm_atomic_helper_shutdown(dev);
> iounmap(cirrus->mmio);
> iounmap(cirrus->vram);
> drm_dev_put(dev);
> - kfree(cirrus);
> pci_release_regions(pdev);
> }
>
> --
> 2.18.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch