[PATCH 2/3] drm/radeon: Inline drm_get_pci_dev

[Daniel Vetter]

On Mon, Feb 24, 2020 at 5:31 PM Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
> On Sat, 22 Feb 2020 at 17:54, Daniel Vetter <daniel.vetter at 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 at intel.com>
> > Cc: Alex Deucher <alexander.deucher at amd.com>
> > Cc: "Christian König" <christian.koenig at amd.com>
> > Cc: "David (ChunMing) Zhou" <David1.Zhou at amd.com>
> > Cc: amd-gfx at 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
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch