[Intel-gfx] [PATCH 57/59] drm/ast: Use managed pci functions

[Daniel Vetter]

On Wed, Apr 15, 2020 at 9:52 AM Thomas Zimmermann <tzimmermann at suse.de> wrote:
>
> Hi Daniel
>
> Am 15.04.20 um 09:40 schrieb Daniel Vetter:
> > Allows us to remove a bit of cleanup code.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > Cc: Dave Airlie <airlied at redhat.com>
> > Cc: Thomas Zimmermann <tzimmermann at suse.de>
> > Cc: Gerd Hoffmann <kraxel at redhat.com>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Cc: Emil Velikov <emil.velikov at collabora.com>
> > Cc: "Noralf Trønnes" <noralf at tronnes.org>
> > Cc: Sam Ravnborg <sam at ravnborg.org>
> > Cc: "Christian König" <christian.koenig at amd.com>
> > Cc: "Y.C. Chen" <yc_chen at aspeedtech.com>
> > ---
> >  drivers/gpu/drm/ast/ast_drv.c  | 10 +++-------
> >  drivers/gpu/drm/ast/ast_main.c |  3 ---
> >  2 files changed, 3 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> > index b7ba22dddcad..48a9cc4e080a 100644
> > --- a/drivers/gpu/drm/ast/ast_drv.c
> > +++ b/drivers/gpu/drm/ast/ast_drv.c
> > @@ -91,15 +91,13 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >
> >       ast_kick_out_firmware_fb(pdev);
> >
> > -     ret = pci_enable_device(pdev);
> > +     ret = pcim_enable_device(pdev);
> >       if (ret)
> >               return ret;
> >
> >       dev = drm_dev_alloc(&driver, &pdev->dev);
> > -     if (IS_ERR(dev)) {
> > -             ret = PTR_ERR(dev);
> > -             goto err_pci_disable_device;
> > -     }
> > +     if (IS_ERR(dev))
> > +             return  PTR_ERR(dev);
> >
> >       dev->pdev = pdev;
> >       pci_set_drvdata(pdev, dev);
> > @@ -120,8 +118,6 @@ static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >       ast_driver_unload(dev);
> >  err_drm_dev_put:
> >       drm_dev_put(dev);
> > -err_pci_disable_device:
> > -     pci_disable_device(pdev);
> >       return ret;
> >
> >  }
> > diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> > index e5398e3dabe7..1b35728ad871 100644
> > --- a/drivers/gpu/drm/ast/ast_main.c
> > +++ b/drivers/gpu/drm/ast/ast_main.c
> > @@ -531,8 +531,5 @@ void ast_driver_unload(struct drm_device *dev)
> >       drm_mode_config_cleanup(dev);
> >
> >       ast_mm_fini(ast);
> > -     if (ast->ioregs != ast->regs + AST_IO_MM_OFFSET)
> > -             pci_iounmap(dev->pdev, ast->ioregs);
> > -     pci_iounmap(dev->pdev, ast->regs);
>
> This gets unmapped as part of the automatic pci_disable_device(), I guess?

Yup, once you go with pcim_enable_device all pci_ functions on that
device become manged and auto-cleanup.

> Do we need drm_dev_enter()/_exit() to make I/O work reliably?

That does nothing without drm_dev_unplug(), which has the annoying
side effect that it also shuts up stuff like
drm_atomic_helper_shutdown for module unload. And developers really
want their devices to be shut off on driver unload. So yeah
unfortunately we currently can decide between "correct for hotunplug"
and "convenient for driver unload for driver authors". I'm not sure
what to best do here, since all options are kinda not great for one
use-case or the other.
-Daniel

> Best regards
> Thomas
>
> >       kfree(ast);
> >  }
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch