[2/3] drm/radeon: Inline drm_get_pci_dev
diff mbox series

Message ID 20200222175433.2259158-2-daniel.vetter@ffwll.ch
State New
Headers show
Series
  • [1/3] drm/amdgpu: Drop DRIVER_USE_AGP
Related show

Commit Message

Daniel Vetter Feb. 22, 2020, 5:54 p.m. UTC
It's the last user, and more importantly, it's the last non-legacy
user of anything in drm_pci.c.

The only tricky bit is the agp initialization. But a close look shows
that radeon does not use the drm_agp midlayer (the main use of that is
drm_bufs for legacy drivers), and instead could use the agp subsystem
directly (like nouveau does already). Hence we can just pull this in
too.

A further step would be to entirely drop the use of drm_device->agp,
but feels like too much churn just for this patch.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Christian König" <christian.koenig@amd.com>
Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
Cc: amd-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/radeon/radeon_drv.c | 43 +++++++++++++++++++++++++++--
 drivers/gpu/drm/radeon/radeon_kms.c |  6 ++++
 2 files changed, 47 insertions(+), 2 deletions(-)

Comments

Emil Velikov Feb. 24, 2020, 4:31 p.m. UTC | #1
On Sat, 22 Feb 2020 at 17:54, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> It's the last user, and more importantly, it's the last non-legacy
> user of anything in drm_pci.c.
>
> The only tricky bit is the agp initialization. But a close look shows
> that radeon does not use the drm_agp midlayer (the main use of that is
> drm_bufs for legacy drivers), and instead could use the agp subsystem
> directly (like nouveau does already). Hence we can just pull this in
> too.
>
> A further step would be to entirely drop the use of drm_device->agp,
> but feels like too much churn just for this patch.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
> Cc: amd-gfx@lists.freedesktop.org
> ---
>  drivers/gpu/drm/radeon/radeon_drv.c | 43 +++++++++++++++++++++++++++--
>  drivers/gpu/drm/radeon/radeon_kms.c |  6 ++++
>  2 files changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index 49ce2e7d5f9e..59f8186a2415 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -37,6 +37,7 @@
>  #include <linux/vga_switcheroo.h>
>  #include <linux/mmu_notifier.h>
>
> +#include <drm/drm_agpsupport.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_fb_helper.h>
> @@ -322,6 +323,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
>                             const struct pci_device_id *ent)
>  {
>         unsigned long flags = 0;
> +       struct drm_device *dev;
>         int ret;
>
>         if (!ent)
> @@ -362,7 +364,44 @@ static int radeon_pci_probe(struct pci_dev *pdev,
>         if (ret)
>                 return ret;
>
> -       return drm_get_pci_dev(pdev, ent, &kms_driver);
> +       dev = drm_dev_alloc(&kms_driver, &pdev->dev);
> +       if (IS_ERR(dev))
> +               return PTR_ERR(dev);
> +
> +       ret = pci_enable_device(pdev);
> +       if (ret)
> +               goto err_free;
> +
> +       dev->pdev = pdev;
> +#ifdef __alpha__
> +       dev->hose = pdev->sysdata;
> +#endif
> +
> +       pci_set_drvdata(pdev, dev);
> +
> +       if (pci_find_capability(dev->pdev, PCI_CAP_ID_AGP))
> +               dev->agp = drm_agp_init(dev);
> +       if (dev->agp) {
> +               dev->agp->agp_mtrr = arch_phys_wc_add(
> +                       dev->agp->agp_info.aper_base,
> +                       dev->agp->agp_info.aper_size *
> +                       1024 * 1024);
> +       }
> +
IMHO a better solution is kill off the drm_agpsupport.c dependency all together.
As-is it's still used, making the legacy vs not line really moot.

Especially, since the AGP ioctl (in the legacy code) can manipulate
the underlying state.

Off the top of my head, in radeon_agp_init():
 - at the top agp_backend_acquire() + agp_copy_info()
 - followed up by existing mode magic
 - opencode the enable - agp_enable() + acquired = true;
 - mtrr = arch_phys_wc_add() and the rest

In radeon_agp_fini()
 - if !acquired { agp_backend_release(); acquired = false }


Something like ^^ should result in a net diffstat of around zero.
All thanks to the interesting layer that drm_agp is ;-)

With this in place we can make move drm_device::agp and
DRM_IOCTL_AGP_INFO behind CONFIG_DRM_LEGACY.

-Emil
P.S. Watch out for radeon_ttm.c warnings/errors
Daniel Vetter Feb. 24, 2020, 8:46 p.m. UTC | #2
On Mon, Feb 24, 2020 at 5:31 PM Emil Velikov <emil.l.velikov@gmail.com> wrote:
>
> On Sat, 22 Feb 2020 at 17:54, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > It's the last user, and more importantly, it's the last non-legacy
> > user of anything in drm_pci.c.
> >
> > The only tricky bit is the agp initialization. But a close look shows
> > that radeon does not use the drm_agp midlayer (the main use of that is
> > drm_bufs for legacy drivers), and instead could use the agp subsystem
> > directly (like nouveau does already). Hence we can just pull this in
> > too.
> >
> > A further step would be to entirely drop the use of drm_device->agp,
> > but feels like too much churn just for this patch.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: "Christian König" <christian.koenig@amd.com>
> > Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
> > Cc: amd-gfx@lists.freedesktop.org
> > ---
> >  drivers/gpu/drm/radeon/radeon_drv.c | 43 +++++++++++++++++++++++++++--
> >  drivers/gpu/drm/radeon/radeon_kms.c |  6 ++++
> >  2 files changed, 47 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> > index 49ce2e7d5f9e..59f8186a2415 100644
> > --- a/drivers/gpu/drm/radeon/radeon_drv.c
> > +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> > @@ -37,6 +37,7 @@
> >  #include <linux/vga_switcheroo.h>
> >  #include <linux/mmu_notifier.h>
> >
> > +#include <drm/drm_agpsupport.h>
> >  #include <drm/drm_crtc_helper.h>
> >  #include <drm/drm_drv.h>
> >  #include <drm/drm_fb_helper.h>
> > @@ -322,6 +323,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
> >                             const struct pci_device_id *ent)
> >  {
> >         unsigned long flags = 0;
> > +       struct drm_device *dev;
> >         int ret;
> >
> >         if (!ent)
> > @@ -362,7 +364,44 @@ static int radeon_pci_probe(struct pci_dev *pdev,
> >         if (ret)
> >                 return ret;
> >
> > -       return drm_get_pci_dev(pdev, ent, &kms_driver);
> > +       dev = drm_dev_alloc(&kms_driver, &pdev->dev);
> > +       if (IS_ERR(dev))
> > +               return PTR_ERR(dev);
> > +
> > +       ret = pci_enable_device(pdev);
> > +       if (ret)
> > +               goto err_free;
> > +
> > +       dev->pdev = pdev;
> > +#ifdef __alpha__
> > +       dev->hose = pdev->sysdata;
> > +#endif
> > +
> > +       pci_set_drvdata(pdev, dev);
> > +
> > +       if (pci_find_capability(dev->pdev, PCI_CAP_ID_AGP))
> > +               dev->agp = drm_agp_init(dev);
> > +       if (dev->agp) {
> > +               dev->agp->agp_mtrr = arch_phys_wc_add(
> > +                       dev->agp->agp_info.aper_base,
> > +                       dev->agp->agp_info.aper_size *
> > +                       1024 * 1024);
> > +       }
> > +
> IMHO a better solution is kill off the drm_agpsupport.c dependency all together.
> As-is it's still used, making the legacy vs not line really moot.
>
> Especially, since the AGP ioctl (in the legacy code) can manipulate
> the underlying state.
>
> Off the top of my head, in radeon_agp_init():
>  - at the top agp_backend_acquire() + agp_copy_info()
>  - followed up by existing mode magic
>  - opencode the enable - agp_enable() + acquired = true;
>  - mtrr = arch_phys_wc_add() and the rest
>
> In radeon_agp_fini()
>  - if !acquired { agp_backend_release(); acquired = false }
>
>
> Something like ^^ should result in a net diffstat of around zero.
> All thanks to the interesting layer that drm_agp is ;-)
>
> With this in place we can make move drm_device::agp and
> DRM_IOCTL_AGP_INFO behind CONFIG_DRM_LEGACY.

Yeah could be done, but I was more out to get the drm_pci.c stuff, not
the agp stuff. But I did realize that nouveau also just directly
accesses the agp bridge stuff, so we could indeed nuke the midlayer
here. The legacy agp stuff ioctl stuff should be behind DRIVER_LEGACY
checks already, so no way to cause harm that way (I hope at least, if
there's a gap we'd need to close it).
-Daniel
Daniel Vetter Feb. 24, 2020, 8:49 p.m. UTC | #3
On Mon, Feb 24, 2020 at 9:46 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> On Mon, Feb 24, 2020 at 5:31 PM Emil Velikov <emil.l.velikov@gmail.com> wrote:
> >
> > On Sat, 22 Feb 2020 at 17:54, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > >
> > > It's the last user, and more importantly, it's the last non-legacy
> > > user of anything in drm_pci.c.
> > >
> > > The only tricky bit is the agp initialization. But a close look shows
> > > that radeon does not use the drm_agp midlayer (the main use of that is
> > > drm_bufs for legacy drivers), and instead could use the agp subsystem
> > > directly (like nouveau does already). Hence we can just pull this in
> > > too.
> > >
> > > A further step would be to entirely drop the use of drm_device->agp,
> > > but feels like too much churn just for this patch.
> > >
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > Cc: "Christian König" <christian.koenig@amd.com>
> > > Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
> > > Cc: amd-gfx@lists.freedesktop.org
> > > ---
> > >  drivers/gpu/drm/radeon/radeon_drv.c | 43 +++++++++++++++++++++++++++--
> > >  drivers/gpu/drm/radeon/radeon_kms.c |  6 ++++
> > >  2 files changed, 47 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> > > index 49ce2e7d5f9e..59f8186a2415 100644
> > > --- a/drivers/gpu/drm/radeon/radeon_drv.c
> > > +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> > > @@ -37,6 +37,7 @@
> > >  #include <linux/vga_switcheroo.h>
> > >  #include <linux/mmu_notifier.h>
> > >
> > > +#include <drm/drm_agpsupport.h>
> > >  #include <drm/drm_crtc_helper.h>
> > >  #include <drm/drm_drv.h>
> > >  #include <drm/drm_fb_helper.h>
> > > @@ -322,6 +323,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
> > >                             const struct pci_device_id *ent)
> > >  {
> > >         unsigned long flags = 0;
> > > +       struct drm_device *dev;
> > >         int ret;
> > >
> > >         if (!ent)
> > > @@ -362,7 +364,44 @@ static int radeon_pci_probe(struct pci_dev *pdev,
> > >         if (ret)
> > >                 return ret;
> > >
> > > -       return drm_get_pci_dev(pdev, ent, &kms_driver);
> > > +       dev = drm_dev_alloc(&kms_driver, &pdev->dev);
> > > +       if (IS_ERR(dev))
> > > +               return PTR_ERR(dev);
> > > +
> > > +       ret = pci_enable_device(pdev);
> > > +       if (ret)
> > > +               goto err_free;
> > > +
> > > +       dev->pdev = pdev;
> > > +#ifdef __alpha__
> > > +       dev->hose = pdev->sysdata;
> > > +#endif
> > > +
> > > +       pci_set_drvdata(pdev, dev);
> > > +
> > > +       if (pci_find_capability(dev->pdev, PCI_CAP_ID_AGP))
> > > +               dev->agp = drm_agp_init(dev);
> > > +       if (dev->agp) {
> > > +               dev->agp->agp_mtrr = arch_phys_wc_add(
> > > +                       dev->agp->agp_info.aper_base,
> > > +                       dev->agp->agp_info.aper_size *
> > > +                       1024 * 1024);
> > > +       }
> > > +
> > IMHO a better solution is kill off the drm_agpsupport.c dependency all together.
> > As-is it's still used, making the legacy vs not line really moot.
> >
> > Especially, since the AGP ioctl (in the legacy code) can manipulate
> > the underlying state.
> >
> > Off the top of my head, in radeon_agp_init():
> >  - at the top agp_backend_acquire() + agp_copy_info()
> >  - followed up by existing mode magic
> >  - opencode the enable - agp_enable() + acquired = true;
> >  - mtrr = arch_phys_wc_add() and the rest
> >
> > In radeon_agp_fini()
> >  - if !acquired { agp_backend_release(); acquired = false }
> >
> >
> > Something like ^^ should result in a net diffstat of around zero.
> > All thanks to the interesting layer that drm_agp is ;-)
> >
> > With this in place we can make move drm_device::agp and
> > DRM_IOCTL_AGP_INFO behind CONFIG_DRM_LEGACY.
>
> Yeah could be done, but I was more out to get the drm_pci.c stuff, not
> the agp stuff. But I did realize that nouveau also just directly
> accesses the agp bridge stuff, so we could indeed nuke the midlayer
> here. The legacy agp stuff ioctl stuff should be behind DRIVER_LEGACY
> checks already, so no way to cause harm that way (I hope at least, if
> there's a gap we'd need to close it).

Haha, I should read code before hitting send.

It's totally wide open root hole. I guess no one ever bothered to run
a fuzzer on a radeon.ko or amdgpu.ko (before the patch to stop using
drm_get_pci_dev at least) hw. And I never cared since we've killed the
fake agp stuff that i915.ko used a long time ago.

So yeah I think at least sprinkling DRIVER_LEGACY checks over all
this, and reviewing old radeon userspace, would be really good. Once
we have that we can then do the second step and retire the agp
midlayer. But first we need to add the uapi checks/changes.
-Daniel
Alex Deucher Feb. 25, 2020, 3:26 p.m. UTC | #4
On Sat, Feb 22, 2020 at 12:54 PM Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> It's the last user, and more importantly, it's the last non-legacy
> user of anything in drm_pci.c.
>
> The only tricky bit is the agp initialization. But a close look shows
> that radeon does not use the drm_agp midlayer (the main use of that is
> drm_bufs for legacy drivers), and instead could use the agp subsystem
> directly (like nouveau does already). Hence we can just pull this in
> too.
>
> A further step would be to entirely drop the use of drm_device->agp,
> but feels like too much churn just for this patch.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "Christian König" <christian.koenig@amd.com>
> Cc: "David (ChunMing) Zhou" <David1.Zhou@amd.com>
> Cc: amd-gfx@lists.freedesktop.org

Applied.  Thanks!

Alex

> ---
>  drivers/gpu/drm/radeon/radeon_drv.c | 43 +++++++++++++++++++++++++++--
>  drivers/gpu/drm/radeon/radeon_kms.c |  6 ++++
>  2 files changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
> index 49ce2e7d5f9e..59f8186a2415 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -37,6 +37,7 @@
>  #include <linux/vga_switcheroo.h>
>  #include <linux/mmu_notifier.h>
>
> +#include <drm/drm_agpsupport.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_fb_helper.h>
> @@ -322,6 +323,7 @@ static int radeon_pci_probe(struct pci_dev *pdev,
>                             const struct pci_device_id *ent)
>  {
>         unsigned long flags = 0;
> +       struct drm_device *dev;
>         int ret;
>
>         if (!ent)
> @@ -362,7 +364,44 @@ static int radeon_pci_probe(struct pci_dev *pdev,
>         if (ret)
>                 return ret;
>
> -       return drm_get_pci_dev(pdev, ent, &kms_driver);
> +       dev = drm_dev_alloc(&kms_driver, &pdev->dev);
> +       if (IS_ERR(dev))
> +               return PTR_ERR(dev);
> +
> +       ret = pci_enable_device(pdev);
> +       if (ret)
> +               goto err_free;
> +
> +       dev->pdev = pdev;
> +#ifdef __alpha__
> +       dev->hose = pdev->sysdata;
> +#endif
> +
> +       pci_set_drvdata(pdev, dev);
> +
> +       if (pci_find_capability(dev->pdev, PCI_CAP_ID_AGP))
> +               dev->agp = drm_agp_init(dev);
> +       if (dev->agp) {
> +               dev->agp->agp_mtrr = arch_phys_wc_add(
> +                       dev->agp->agp_info.aper_base,
> +                       dev->agp->agp_info.aper_size *
> +                       1024 * 1024);
> +       }
> +
> +       ret = drm_dev_register(dev, ent->driver_data);
> +       if (ret)
> +               goto err_agp;
> +
> +       return 0;
> +
> +err_agp:
> +       if (dev->agp)
> +               arch_phys_wc_del(dev->agp->agp_mtrr);
> +       kfree(dev->agp);
> +       pci_disable_device(pdev);
> +err_free:
> +       drm_dev_put(dev);
> +       return ret;
>  }
>
>  static void
> @@ -562,7 +601,7 @@ static const struct file_operations radeon_driver_kms_fops = {
>
>  static struct drm_driver kms_driver = {
>         .driver_features =
> -           DRIVER_USE_AGP | DRIVER_GEM | DRIVER_RENDER,
> +           DRIVER_GEM | DRIVER_RENDER,
>         .load = radeon_driver_load_kms,
>         .open = radeon_driver_open_kms,
>         .postclose = radeon_driver_postclose_kms,
> diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
> index cab891f86dc0..58176db85952 100644
> --- a/drivers/gpu/drm/radeon/radeon_kms.c
> +++ b/drivers/gpu/drm/radeon/radeon_kms.c
> @@ -32,6 +32,7 @@
>  #include <linux/uaccess.h>
>  #include <linux/vga_switcheroo.h>
>
> +#include <drm/drm_agpsupport.h>
>  #include <drm/drm_fb_helper.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_ioctl.h>
> @@ -77,6 +78,11 @@ void radeon_driver_unload_kms(struct drm_device *dev)
>         radeon_modeset_fini(rdev);
>         radeon_device_fini(rdev);
>
> +       if (dev->agp)
> +               arch_phys_wc_del(dev->agp->agp_mtrr);
> +       kfree(dev->agp);
> +       dev->agp = NULL;
> +
>  done_free:
>         kfree(rdev);
>         dev->dev_private = NULL;
> --
> 2.24.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Patch
diff mbox series

diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index 49ce2e7d5f9e..59f8186a2415 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -37,6 +37,7 @@ 
 #include <linux/vga_switcheroo.h>
 #include <linux/mmu_notifier.h>
 
+#include <drm/drm_agpsupport.h>
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_fb_helper.h>
@@ -322,6 +323,7 @@  static int radeon_pci_probe(struct pci_dev *pdev,
 			    const struct pci_device_id *ent)
 {
 	unsigned long flags = 0;
+	struct drm_device *dev;
 	int ret;
 
 	if (!ent)
@@ -362,7 +364,44 @@  static int radeon_pci_probe(struct pci_dev *pdev,
 	if (ret)
 		return ret;
 
-	return drm_get_pci_dev(pdev, ent, &kms_driver);
+	dev = drm_dev_alloc(&kms_driver, &pdev->dev);
+	if (IS_ERR(dev))
+		return PTR_ERR(dev);
+
+	ret = pci_enable_device(pdev);
+	if (ret)
+		goto err_free;
+
+	dev->pdev = pdev;
+#ifdef __alpha__
+	dev->hose = pdev->sysdata;
+#endif
+
+	pci_set_drvdata(pdev, dev);
+
+	if (pci_find_capability(dev->pdev, PCI_CAP_ID_AGP))
+		dev->agp = drm_agp_init(dev);
+	if (dev->agp) {
+		dev->agp->agp_mtrr = arch_phys_wc_add(
+			dev->agp->agp_info.aper_base,
+			dev->agp->agp_info.aper_size *
+			1024 * 1024);
+	}
+
+	ret = drm_dev_register(dev, ent->driver_data);
+	if (ret)
+		goto err_agp;
+
+	return 0;
+
+err_agp:
+	if (dev->agp)
+		arch_phys_wc_del(dev->agp->agp_mtrr);
+	kfree(dev->agp);
+	pci_disable_device(pdev);
+err_free:
+	drm_dev_put(dev);
+	return ret;
 }
 
 static void
@@ -562,7 +601,7 @@  static const struct file_operations radeon_driver_kms_fops = {
 
 static struct drm_driver kms_driver = {
 	.driver_features =
-	    DRIVER_USE_AGP | DRIVER_GEM | DRIVER_RENDER,
+	    DRIVER_GEM | DRIVER_RENDER,
 	.load = radeon_driver_load_kms,
 	.open = radeon_driver_open_kms,
 	.postclose = radeon_driver_postclose_kms,
diff --git a/drivers/gpu/drm/radeon/radeon_kms.c b/drivers/gpu/drm/radeon/radeon_kms.c
index cab891f86dc0..58176db85952 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -32,6 +32,7 @@ 
 #include <linux/uaccess.h>
 #include <linux/vga_switcheroo.h>
 
+#include <drm/drm_agpsupport.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_file.h>
 #include <drm/drm_ioctl.h>
@@ -77,6 +78,11 @@  void radeon_driver_unload_kms(struct drm_device *dev)
 	radeon_modeset_fini(rdev);
 	radeon_device_fini(rdev);
 
+	if (dev->agp)
+		arch_phys_wc_del(dev->agp->agp_mtrr);
+	kfree(dev->agp);
+	dev->agp = NULL;
+
 done_free:
 	kfree(rdev);
 	dev->dev_private = NULL;