Hi folks,
Currently, when the device is removed (or the driver is unbound) the
nouveau_drm structure de-allocated. However, it's still accessible from
and used by some DRM layer callbacks. For example, file handles can be
closed after the device has been removed (physically or otherwise). This
series converts the Nouveau device structure to be allocated and
de-allocated with the devm_drm_dev_alloc() API.
In the future, additional resources that should be bound to the lifetime
of the drm_device can be added, and the drmm_add_action() APIs offer a
nice hook for arbitrary cleanup actions before the drm_device is
destroyed, so I suspect much of the current cleanup code in Nouveau
would benefit from some refactoring to use this.
Finally, although not *strictly* necessary for this series, I included
some documentation for structures I investigated for this work.
Jeremy Cline (3):
drm/nouveau: Use helper to convert nouveau_drm to drm_device
drm/nouveau: manage nouveau_drm lifetime with devres
drm/nouveau: begin documenting core nouveau structures
drivers/gpu/drm/nouveau/dispnv04/crtc.c | 10 +-
drivers/gpu/drm/nouveau/dispnv50/base.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/base507c.c | 7 +-
drivers/gpu/drm/nouveau/dispnv50/core.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/core507d.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/curs.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/curs507a.c | 5 +-
drivers/gpu/drm/nouveau/dispnv50/disp.c | 17 +--
drivers/gpu/drm/nouveau/dispnv50/oimm.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/oimm507b.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/ovly.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/ovly507e.c | 5 +-
drivers/gpu/drm/nouveau/dispnv50/wimm.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/wimmc37b.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/wndw.c | 2 +-
drivers/gpu/drm/nouveau/dispnv50/wndwc37e.c | 5 +-
drivers/gpu/drm/nouveau/nouveau_bo.c | 16 ++-
drivers/gpu/drm/nouveau/nouveau_debugfs.c | 9 +-
drivers/gpu/drm/nouveau/nouveau_display.c | 16 +--
drivers/gpu/drm/nouveau/nouveau_dmem.c | 17 +--
drivers/gpu/drm/nouveau/nouveau_drm.c | 41 ++++----
drivers/gpu/drm/nouveau/nouveau_drv.h | 111 +++++++++++++++++++-
drivers/gpu/drm/nouveau/nouveau_fbcon.c | 19 ++--
drivers/gpu/drm/nouveau/nouveau_gem.c | 8 +-
drivers/gpu/drm/nouveau/nouveau_svm.c | 4 +-
drivers/gpu/drm/nouveau/nouveau_ttm.c | 4 +-
drivers/gpu/drm/nouveau/nouveau_vga.c | 8 +-
27 files changed, 216 insertions(+), 106 deletions(-)
--
2.28.0
Make use of the devm_drm_dev_alloc() API to bind the lifetime of
nouveau_drm structure to the drm_device. This is important because a
reference to nouveau_drm is accessible from drm_device, which is
provided to a number of DRM layer callbacks that can run after the
deallocation of nouveau_drm currently occurs.
Signed-off-by: Jeremy Cline <[email protected]>
---
drivers/gpu/drm/nouveau/nouveau_drm.c | 44 ++++++++++++---------------
drivers/gpu/drm/nouveau/nouveau_drv.h | 10 ++++--
2 files changed, 26 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index bc6f51bf23b7..f750c25e92f9 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -30,9 +30,11 @@
#include <linux/vga_switcheroo.h>
#include <linux/mmu_notifier.h>
+#include <drm/drm_drv.h>
#include <drm/drm_crtc_helper.h>
#include <drm/drm_ioctl.h>
#include <drm/drm_vblank.h>
+#include <drm/drm_managed.h>
#include <core/gpuobj.h>
#include <core/option.h>
@@ -532,13 +534,8 @@ nouveau_parent = {
static int
nouveau_drm_device_init(struct drm_device *dev)
{
- struct nouveau_drm *drm;
int ret;
-
- if (!(drm = kzalloc(sizeof(*drm), GFP_KERNEL)))
- return -ENOMEM;
- dev->dev_private = drm;
- drm->dev = dev;
+ struct nouveau_drm *drm = nouveau_drm(dev);
nvif_parent_ctor(&nouveau_parent, &drm->parent);
drm->master.base.object.parent = &drm->parent;
@@ -620,7 +617,6 @@ nouveau_drm_device_init(struct drm_device *dev)
nouveau_cli_fini(&drm->master);
fail_alloc:
nvif_parent_dtor(&drm->parent);
- kfree(drm);
return ret;
}
@@ -654,7 +650,6 @@ nouveau_drm_device_fini(struct drm_device *dev)
nouveau_cli_fini(&drm->client);
nouveau_cli_fini(&drm->master);
nvif_parent_dtor(&drm->parent);
- kfree(drm);
}
/*
@@ -720,6 +715,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
{
struct nvkm_device *device;
struct drm_device *drm_dev;
+ struct nouveau_drm *nv_dev;
int ret;
if (vga_switcheroo_client_probe_defer(pdev))
@@ -750,15 +746,16 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
if (nouveau_atomic)
driver_pci.driver_features |= DRIVER_ATOMIC;
- drm_dev = drm_dev_alloc(&driver_pci, &pdev->dev);
- if (IS_ERR(drm_dev)) {
- ret = PTR_ERR(drm_dev);
+ nv_dev = devm_drm_dev_alloc(&pdev->dev, &driver_stub, typeof(*nv_dev), drm_dev);
+ if (IS_ERR(nv_dev)) {
+ ret = PTR_ERR(nv_dev);
goto fail_nvkm;
}
+ drm_dev = nouveau_to_drm_dev(nv_dev);
ret = pci_enable_device(pdev);
if (ret)
- goto fail_drm;
+ goto fail_nvkm;
drm_dev->pdev = pdev;
pci_set_drvdata(pdev, drm_dev);
@@ -778,8 +775,6 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
nouveau_drm_device_fini(drm_dev);
fail_pci:
pci_disable_device(pdev);
-fail_drm:
- drm_dev_put(drm_dev);
fail_nvkm:
nvkm_device_del(&device);
return ret;
@@ -799,7 +794,6 @@ nouveau_drm_device_remove(struct drm_device *dev)
device = nvkm_device_find(client->device);
nouveau_drm_device_fini(dev);
- drm_dev_put(dev);
nvkm_device_del(&device);
}
@@ -1285,7 +1279,8 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
struct platform_device *pdev,
struct nvkm_device **pdevice)
{
- struct drm_device *drm;
+ struct nouveau_drm *nv_dev;
+ struct drm_device *drm_dev;
int err;
err = nvkm_device_tegra_new(func, pdev, nouveau_config, nouveau_debug,
@@ -1293,22 +1288,21 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
if (err)
goto err_free;
- drm = drm_dev_alloc(&driver_platform, &pdev->dev);
- if (IS_ERR(drm)) {
- err = PTR_ERR(drm);
+ nv_dev = devm_drm_dev_alloc(&pdev->dev, &driver_platform, typeof(*nv_dev), drm_dev);
+ if (IS_ERR(nv_dev)) {
+ err = PTR_ERR(nv_dev);
goto err_free;
}
+ drm_dev = nouveau_to_drm_dev(nv_dev);
- err = nouveau_drm_device_init(drm);
+ err = nouveau_drm_device_init(drm_dev);
if (err)
- goto err_put;
+ goto err_free;
- platform_set_drvdata(pdev, drm);
+ platform_set_drvdata(pdev, drm_dev);
- return drm;
+ return drm_dev;
-err_put:
- drm_dev_put(drm);
err_free:
nvkm_device_del(pdevice);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
index 3e2920a10099..cf6c33e52a5c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -137,7 +137,11 @@ struct nouveau_drm {
struct nvif_parent parent;
struct nouveau_cli master;
struct nouveau_cli client;
- struct drm_device *dev;
+
+ /**
+ * @drm_dev: The parent DRM device object.
+ */
+ struct drm_device drm_dev;
struct list_head clients;
@@ -237,7 +241,7 @@ struct nouveau_drm {
static inline struct nouveau_drm *
nouveau_drm(struct drm_device *dev)
{
- return dev->dev_private;
+ return container_of(dev, struct nouveau_drm, drm_dev);
}
/**
@@ -251,7 +255,7 @@ nouveau_drm(struct drm_device *dev)
*/
static inline struct drm_device *
nouveau_to_drm_dev(struct nouveau_drm *nv_dev) {
- return nv_dev->dev;
+ return &nv_dev->drm_dev;
}
/**
--
2.28.0
Start on documentation for the Nouveau device structure and the NVIF
client structure it uses. This documentation is not complete as the
structures are non-trivial and I am not familiar with large portions of
them.
Signed-off-by: Jeremy Cline <[email protected]>
---
drivers/gpu/drm/nouveau/nouveau_drv.h | 67 +++++++++++++++++++++++++++
1 file changed, 67 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
index cf6c33e52a5c..cf83d4bf3c6c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -90,8 +90,20 @@ enum nouveau_drm_handle {
NVDRM_NVSW = 0x55550000,
};
+/**
+ * struct nouveau_cli - A DRM-specific NVIF client.
+ *
+ * This encapsulates a NVIF client and is intended to be the sole interface
+ * between the DRM APIs and NVKM. An instance of this structure is allocated
+ * for each userspace client when they open the device file. Additionally,
+ * there are several allocated strictly for the kernel's use.
+ */
struct nouveau_cli {
struct nvif_client base;
+
+ /**
+ * @drm: A reference to the device that the client is associated with.
+ */
struct nouveau_drm *drm;
struct mutex mutex;
@@ -101,6 +113,9 @@ struct nouveau_cli {
struct nouveau_vmm svm;
const struct nvif_mclass *mem;
+ /**
+ * @head: The list entry for this client in the @drm device's list of clients.
+ */
struct list_head head;
void *abi16;
struct list_head objects;
@@ -108,13 +123,36 @@ struct nouveau_cli {
char name[32];
struct work_struct work;
+
+ /**
+ * @worker: List of pending &struct nouveau_cli_work associated with this client.
+ */
struct list_head worker;
+
+ /**
+ * @lock: Protects the @worker list. Additionally, this lock on the
+ * @drm.master instance is used to serialize destruction of the @base
+ * member in this structure, as well as the destruction of the &struct
+ * nvif_mem embedded in &struct nouveau_mem instances.
+ */
struct mutex lock;
};
+/**
+ * struct nouveau_cli_work - A pending work item for an NVIF client.
+ */
struct nouveau_cli_work {
void (*func)(struct nouveau_cli_work *);
+
+ /**
+ * @cli: Reference to the NVIF client this work belongs to.
+ */
struct nouveau_cli *cli;
+
+ /**
+ * @head: The list entry for this work item in the &struct nouveau_cli
+ * worker list.
+ */
struct list_head head;
struct dma_fence *fence;
@@ -133,9 +171,32 @@ nouveau_cli(struct drm_file *fpriv)
#include <nvif/object.h>
#include <nvif/parent.h>
+/**
+ * struct nouveau_drm - The nouveau-specific device structure.
+ *
+ * This structure is allocated for a device when it is probed and keeps track
+ * of all the nouveau-specific device details. The lifetime of this structure
+ * is the same as the lifetime of a &struct drm_device and is managed by the
+ * DRM layer.
+ */
struct nouveau_drm {
+ /**
+ * @parent: Implementation of the interface required to use the NVIF_DEBUG
+ * and NVIF_ERROR macros
+ */
struct nvif_parent parent;
+
+ /**
+ * @master: This NVIF client is used to initialize the NVIF driver and used
+ * for TTM memory allocations. It is the root of the NVIF object tree.
+ */
struct nouveau_cli master;
+
+ /**
+ * @client: This NVIF client is used by the DRM layer to interact with
+ * the NVKM layer for everything except TTM memory allocations. It, and
+ * all other clients, are children of the primary (@master) client.
+ */
struct nouveau_cli client;
/**
@@ -143,6 +204,12 @@ struct nouveau_drm {
*/
struct drm_device drm_dev;
+ /**
+ * @clients: List of all &struct nouveau_cli allocated for userspace
+ * associated with this DRM device. Clients are allocated when the DRM
+ * file is opened and deallocated when the file is closed. This list is
+ * protected by the mutex in @client.
+ */
struct list_head clients;
u8 old_pm_cap;
--
2.28.0
On Fri, Nov 6, 2020 at 3:17 AM Jeremy Cline <[email protected]> wrote:
>
> Make use of the devm_drm_dev_alloc() API to bind the lifetime of
> nouveau_drm structure to the drm_device. This is important because a
> reference to nouveau_drm is accessible from drm_device, which is
> provided to a number of DRM layer callbacks that can run after the
> deallocation of nouveau_drm currently occurs.
>
> Signed-off-by: Jeremy Cline <[email protected]>
> ---
> drivers/gpu/drm/nouveau/nouveau_drm.c | 44 ++++++++++++---------------
> drivers/gpu/drm/nouveau/nouveau_drv.h | 10 ++++--
> 2 files changed, 26 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index bc6f51bf23b7..f750c25e92f9 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -30,9 +30,11 @@
> #include <linux/vga_switcheroo.h>
> #include <linux/mmu_notifier.h>
>
> +#include <drm/drm_drv.h>
> #include <drm/drm_crtc_helper.h>
> #include <drm/drm_ioctl.h>
> #include <drm/drm_vblank.h>
> +#include <drm/drm_managed.h>
>
> #include <core/gpuobj.h>
> #include <core/option.h>
> @@ -532,13 +534,8 @@ nouveau_parent = {
> static int
> nouveau_drm_device_init(struct drm_device *dev)
> {
> - struct nouveau_drm *drm;
> int ret;
> -
> - if (!(drm = kzalloc(sizeof(*drm), GFP_KERNEL)))
> - return -ENOMEM;
> - dev->dev_private = drm;
> - drm->dev = dev;
> + struct nouveau_drm *drm = nouveau_drm(dev);
>
> nvif_parent_ctor(&nouveau_parent, &drm->parent);
> drm->master.base.object.parent = &drm->parent;
> @@ -620,7 +617,6 @@ nouveau_drm_device_init(struct drm_device *dev)
> nouveau_cli_fini(&drm->master);
> fail_alloc:
> nvif_parent_dtor(&drm->parent);
> - kfree(drm);
> return ret;
> }
>
> @@ -654,7 +650,6 @@ nouveau_drm_device_fini(struct drm_device *dev)
> nouveau_cli_fini(&drm->client);
> nouveau_cli_fini(&drm->master);
> nvif_parent_dtor(&drm->parent);
> - kfree(drm);
> }
>
> /*
> @@ -720,6 +715,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> {
> struct nvkm_device *device;
> struct drm_device *drm_dev;
> + struct nouveau_drm *nv_dev;
> int ret;
>
> if (vga_switcheroo_client_probe_defer(pdev))
> @@ -750,15 +746,16 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> if (nouveau_atomic)
> driver_pci.driver_features |= DRIVER_ATOMIC;
>
> - drm_dev = drm_dev_alloc(&driver_pci, &pdev->dev);
> - if (IS_ERR(drm_dev)) {
> - ret = PTR_ERR(drm_dev);
> + nv_dev = devm_drm_dev_alloc(&pdev->dev, &driver_stub, typeof(*nv_dev), drm_dev);
> + if (IS_ERR(nv_dev)) {
> + ret = PTR_ERR(nv_dev);
> goto fail_nvkm;
> }
> + drm_dev = nouveau_to_drm_dev(nv_dev);
>
> ret = pci_enable_device(pdev);
> if (ret)
> - goto fail_drm;
> + goto fail_nvkm;
>
> drm_dev->pdev = pdev;
> pci_set_drvdata(pdev, drm_dev);
> @@ -778,8 +775,6 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> nouveau_drm_device_fini(drm_dev);
> fail_pci:
> pci_disable_device(pdev);
> -fail_drm:
> - drm_dev_put(drm_dev);
it sounded like that when using devm_drm_dev_alloc we still have an
initial refcount of 1, so at least in this regard nothing changed so I
am wondering why this change is necessary and if the reason is
unrelated it might make sense to move it into its own patch.
> fail_nvkm:
> nvkm_device_del(&device);
> return ret;
> @@ -799,7 +794,6 @@ nouveau_drm_device_remove(struct drm_device *dev)
> device = nvkm_device_find(client->device);
>
> nouveau_drm_device_fini(dev);
> - drm_dev_put(dev);
> nvkm_device_del(&device);
> }
>
> @@ -1285,7 +1279,8 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
> struct platform_device *pdev,
> struct nvkm_device **pdevice)
> {
> - struct drm_device *drm;
> + struct nouveau_drm *nv_dev;
> + struct drm_device *drm_dev;
> int err;
>
> err = nvkm_device_tegra_new(func, pdev, nouveau_config, nouveau_debug,
> @@ -1293,22 +1288,21 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
> if (err)
> goto err_free;
>
> - drm = drm_dev_alloc(&driver_platform, &pdev->dev);
> - if (IS_ERR(drm)) {
> - err = PTR_ERR(drm);
> + nv_dev = devm_drm_dev_alloc(&pdev->dev, &driver_platform, typeof(*nv_dev), drm_dev);
> + if (IS_ERR(nv_dev)) {
> + err = PTR_ERR(nv_dev);
> goto err_free;
> }
> + drm_dev = nouveau_to_drm_dev(nv_dev);
>
> - err = nouveau_drm_device_init(drm);
> + err = nouveau_drm_device_init(drm_dev);
> if (err)
> - goto err_put;
> + goto err_free;
>
> - platform_set_drvdata(pdev, drm);
> + platform_set_drvdata(pdev, drm_dev);
>
> - return drm;
> + return drm_dev;
>
> -err_put:
> - drm_dev_put(drm);
> err_free:
> nvkm_device_del(pdevice);
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
> index 3e2920a10099..cf6c33e52a5c 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> @@ -137,7 +137,11 @@ struct nouveau_drm {
> struct nvif_parent parent;
> struct nouveau_cli master;
> struct nouveau_cli client;
> - struct drm_device *dev;
> +
> + /**
> + * @drm_dev: The parent DRM device object.
> + */
> + struct drm_device drm_dev;
>
> struct list_head clients;
>
> @@ -237,7 +241,7 @@ struct nouveau_drm {
> static inline struct nouveau_drm *
> nouveau_drm(struct drm_device *dev)
> {
> - return dev->dev_private;
> + return container_of(dev, struct nouveau_drm, drm_dev);
> }
>
> /**
> @@ -251,7 +255,7 @@ nouveau_drm(struct drm_device *dev)
> */
> static inline struct drm_device *
> nouveau_to_drm_dev(struct nouveau_drm *nv_dev) {
> - return nv_dev->dev;
> + return &nv_dev->drm_dev;
> }
>
> /**
> --
> 2.28.0
>
> _______________________________________________
> Nouveau mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/nouveau
>
On Fri, Nov 06, 2020 at 02:31:44PM +0100, Karol Herbst wrote:
> On Fri, Nov 6, 2020 at 3:17 AM Jeremy Cline <[email protected]> wrote:
> >
> > Make use of the devm_drm_dev_alloc() API to bind the lifetime of
> > nouveau_drm structure to the drm_device. This is important because a
> > reference to nouveau_drm is accessible from drm_device, which is
> > provided to a number of DRM layer callbacks that can run after the
> > deallocation of nouveau_drm currently occurs.
> >
> > Signed-off-by: Jeremy Cline <[email protected]>
> > ---
> > drivers/gpu/drm/nouveau/nouveau_drm.c | 44 ++++++++++++---------------
> > drivers/gpu/drm/nouveau/nouveau_drv.h | 10 ++++--
> > 2 files changed, 26 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index bc6f51bf23b7..f750c25e92f9 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -30,9 +30,11 @@
> > #include <linux/vga_switcheroo.h>
> > #include <linux/mmu_notifier.h>
> >
> > +#include <drm/drm_drv.h>
> > #include <drm/drm_crtc_helper.h>
> > #include <drm/drm_ioctl.h>
> > #include <drm/drm_vblank.h>
> > +#include <drm/drm_managed.h>
> >
> > #include <core/gpuobj.h>
> > #include <core/option.h>
> > @@ -532,13 +534,8 @@ nouveau_parent = {
> > static int
> > nouveau_drm_device_init(struct drm_device *dev)
> > {
> > - struct nouveau_drm *drm;
> > int ret;
> > -
> > - if (!(drm = kzalloc(sizeof(*drm), GFP_KERNEL)))
> > - return -ENOMEM;
> > - dev->dev_private = drm;
> > - drm->dev = dev;
> > + struct nouveau_drm *drm = nouveau_drm(dev);
> >
> > nvif_parent_ctor(&nouveau_parent, &drm->parent);
> > drm->master.base.object.parent = &drm->parent;
> > @@ -620,7 +617,6 @@ nouveau_drm_device_init(struct drm_device *dev)
> > nouveau_cli_fini(&drm->master);
> > fail_alloc:
> > nvif_parent_dtor(&drm->parent);
> > - kfree(drm);
> > return ret;
> > }
> >
> > @@ -654,7 +650,6 @@ nouveau_drm_device_fini(struct drm_device *dev)
> > nouveau_cli_fini(&drm->client);
> > nouveau_cli_fini(&drm->master);
> > nvif_parent_dtor(&drm->parent);
> > - kfree(drm);
> > }
> >
> > /*
> > @@ -720,6 +715,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > {
> > struct nvkm_device *device;
> > struct drm_device *drm_dev;
> > + struct nouveau_drm *nv_dev;
> > int ret;
> >
> > if (vga_switcheroo_client_probe_defer(pdev))
> > @@ -750,15 +746,16 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > if (nouveau_atomic)
> > driver_pci.driver_features |= DRIVER_ATOMIC;
> >
> > - drm_dev = drm_dev_alloc(&driver_pci, &pdev->dev);
> > - if (IS_ERR(drm_dev)) {
> > - ret = PTR_ERR(drm_dev);
> > + nv_dev = devm_drm_dev_alloc(&pdev->dev, &driver_stub, typeof(*nv_dev), drm_dev);
> > + if (IS_ERR(nv_dev)) {
> > + ret = PTR_ERR(nv_dev);
> > goto fail_nvkm;
> > }
> > + drm_dev = nouveau_to_drm_dev(nv_dev);
> >
> > ret = pci_enable_device(pdev);
> > if (ret)
> > - goto fail_drm;
> > + goto fail_nvkm;
> >
> > drm_dev->pdev = pdev;
> > pci_set_drvdata(pdev, drm_dev);
> > @@ -778,8 +775,6 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > nouveau_drm_device_fini(drm_dev);
> > fail_pci:
> > pci_disable_device(pdev);
> > -fail_drm:
> > - drm_dev_put(drm_dev);
>
> it sounded like that when using devm_drm_dev_alloc we still have an
> initial refcount of 1, so at least in this regard nothing changed so I
> am wondering why this change is necessary and if the reason is
> unrelated it might make sense to move it into its own patch.
>
The way I read the supporting code is that when the allocation occurs,
an action is registered on the parent device to call drm_dev_put(), so
as long as the PCI device is dropped, and as far as I could tell it is
when an error is returned, it should be handled automatically. The same
I *think* goes for the platform device variety with Tegra.
However, this is by far the most likely thing for me to have
misunderstood so I'll look through it a second time and would love to
have a second opinion on it.
> > fail_nvkm:
> > nvkm_device_del(&device);
> > return ret;
> > @@ -799,7 +794,6 @@ nouveau_drm_device_remove(struct drm_device *dev)
> > device = nvkm_device_find(client->device);
> >
> > nouveau_drm_device_fini(dev);
> > - drm_dev_put(dev);
> > nvkm_device_del(&device);
> > }
> >
> > @@ -1285,7 +1279,8 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
> > struct platform_device *pdev,
> > struct nvkm_device **pdevice)
> > {
> > - struct drm_device *drm;
> > + struct nouveau_drm *nv_dev;
> > + struct drm_device *drm_dev;
> > int err;
> >
> > err = nvkm_device_tegra_new(func, pdev, nouveau_config, nouveau_debug,
> > @@ -1293,22 +1288,21 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
> > if (err)
> > goto err_free;
> >
> > - drm = drm_dev_alloc(&driver_platform, &pdev->dev);
> > - if (IS_ERR(drm)) {
> > - err = PTR_ERR(drm);
> > + nv_dev = devm_drm_dev_alloc(&pdev->dev, &driver_platform, typeof(*nv_dev), drm_dev);
> > + if (IS_ERR(nv_dev)) {
> > + err = PTR_ERR(nv_dev);
> > goto err_free;
> > }
> > + drm_dev = nouveau_to_drm_dev(nv_dev);
> >
> > - err = nouveau_drm_device_init(drm);
> > + err = nouveau_drm_device_init(drm_dev);
> > if (err)
> > - goto err_put;
> > + goto err_free;
> >
> > - platform_set_drvdata(pdev, drm);
> > + platform_set_drvdata(pdev, drm_dev);
> >
> > - return drm;
> > + return drm_dev;
> >
> > -err_put:
> > - drm_dev_put(drm);
> > err_free:
> > nvkm_device_del(pdevice);
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > index 3e2920a10099..cf6c33e52a5c 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > @@ -137,7 +137,11 @@ struct nouveau_drm {
> > struct nvif_parent parent;
> > struct nouveau_cli master;
> > struct nouveau_cli client;
> > - struct drm_device *dev;
> > +
> > + /**
> > + * @drm_dev: The parent DRM device object.
> > + */
> > + struct drm_device drm_dev;
> >
> > struct list_head clients;
> >
> > @@ -237,7 +241,7 @@ struct nouveau_drm {
> > static inline struct nouveau_drm *
> > nouveau_drm(struct drm_device *dev)
> > {
> > - return dev->dev_private;
> > + return container_of(dev, struct nouveau_drm, drm_dev);
> > }
> >
> > /**
> > @@ -251,7 +255,7 @@ nouveau_drm(struct drm_device *dev)
> > */
> > static inline struct drm_device *
> > nouveau_to_drm_dev(struct nouveau_drm *nv_dev) {
> > - return nv_dev->dev;
> > + return &nv_dev->drm_dev;
> > }
> >
> > /**
> > --
> > 2.28.0
> >
> > _______________________________________________
> > Nouveau mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/nouveau
> >
>
On Fri, Nov 06, 2020 at 02:31:44PM +0100, Karol Herbst wrote:
> On Fri, Nov 6, 2020 at 3:17 AM Jeremy Cline <[email protected]> wrote:
> >
> > Make use of the devm_drm_dev_alloc() API to bind the lifetime of
> > nouveau_drm structure to the drm_device. This is important because a
> > reference to nouveau_drm is accessible from drm_device, which is
> > provided to a number of DRM layer callbacks that can run after the
> > deallocation of nouveau_drm currently occurs.
> >
> > Signed-off-by: Jeremy Cline <[email protected]>
> > ---
> > drivers/gpu/drm/nouveau/nouveau_drm.c | 44 ++++++++++++---------------
> > drivers/gpu/drm/nouveau/nouveau_drv.h | 10 ++++--
> > 2 files changed, 26 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > index bc6f51bf23b7..f750c25e92f9 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > @@ -30,9 +30,11 @@
> > #include <linux/vga_switcheroo.h>
> > #include <linux/mmu_notifier.h>
> >
> > +#include <drm/drm_drv.h>
> > #include <drm/drm_crtc_helper.h>
> > #include <drm/drm_ioctl.h>
> > #include <drm/drm_vblank.h>
> > +#include <drm/drm_managed.h>
> >
> > #include <core/gpuobj.h>
> > #include <core/option.h>
> > @@ -532,13 +534,8 @@ nouveau_parent = {
> > static int
> > nouveau_drm_device_init(struct drm_device *dev)
> > {
> > - struct nouveau_drm *drm;
> > int ret;
> > -
> > - if (!(drm = kzalloc(sizeof(*drm), GFP_KERNEL)))
> > - return -ENOMEM;
> > - dev->dev_private = drm;
> > - drm->dev = dev;
> > + struct nouveau_drm *drm = nouveau_drm(dev);
> >
> > nvif_parent_ctor(&nouveau_parent, &drm->parent);
> > drm->master.base.object.parent = &drm->parent;
> > @@ -620,7 +617,6 @@ nouveau_drm_device_init(struct drm_device *dev)
> > nouveau_cli_fini(&drm->master);
> > fail_alloc:
> > nvif_parent_dtor(&drm->parent);
> > - kfree(drm);
> > return ret;
> > }
> >
> > @@ -654,7 +650,6 @@ nouveau_drm_device_fini(struct drm_device *dev)
> > nouveau_cli_fini(&drm->client);
> > nouveau_cli_fini(&drm->master);
> > nvif_parent_dtor(&drm->parent);
> > - kfree(drm);
> > }
> >
> > /*
> > @@ -720,6 +715,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > {
> > struct nvkm_device *device;
> > struct drm_device *drm_dev;
> > + struct nouveau_drm *nv_dev;
> > int ret;
> >
> > if (vga_switcheroo_client_probe_defer(pdev))
> > @@ -750,15 +746,16 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > if (nouveau_atomic)
> > driver_pci.driver_features |= DRIVER_ATOMIC;
> >
> > - drm_dev = drm_dev_alloc(&driver_pci, &pdev->dev);
> > - if (IS_ERR(drm_dev)) {
> > - ret = PTR_ERR(drm_dev);
> > + nv_dev = devm_drm_dev_alloc(&pdev->dev, &driver_stub, typeof(*nv_dev), drm_dev);
> > + if (IS_ERR(nv_dev)) {
> > + ret = PTR_ERR(nv_dev);
> > goto fail_nvkm;
> > }
> > + drm_dev = nouveau_to_drm_dev(nv_dev);
> >
> > ret = pci_enable_device(pdev);
> > if (ret)
> > - goto fail_drm;
> > + goto fail_nvkm;
> >
> > drm_dev->pdev = pdev;
> > pci_set_drvdata(pdev, drm_dev);
> > @@ -778,8 +775,6 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > nouveau_drm_device_fini(drm_dev);
> > fail_pci:
> > pci_disable_device(pdev);
> > -fail_drm:
> > - drm_dev_put(drm_dev);
>
> it sounded like that when using devm_drm_dev_alloc we still have an
> initial refcount of 1, so at least in this regard nothing changed so I
> am wondering why this change is necessary and if the reason is
> unrelated it might make sense to move it into its own patch.
>
Okay, after a more thorough investigation and testing with
drm.debug=0x201 to trace through the allocations and de-allocations,
everything is indeed cleaned up, both here when a fake failure is
injected, and through the normal removal path.
I believe it would be problematic if nouveau was presented the device,
called devm_drm_dev_alloc(), failed later on in the probe, and then a
different driver claimed the device. It looks like that's not a problem
in practice, but I'm not familiar enough with all the layers to be 100%
confident I'm reading everything right. As far as I can tell, amdgpu
isn't explicitly dropping the reference either.
> > fail_nvkm:
> > nvkm_device_del(&device);
> > return ret;
> > @@ -799,7 +794,6 @@ nouveau_drm_device_remove(struct drm_device *dev)
> > device = nvkm_device_find(client->device);
> >
> > nouveau_drm_device_fini(dev);
> > - drm_dev_put(dev);
> > nvkm_device_del(&device);
> > }
> >
> > @@ -1285,7 +1279,8 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
> > struct platform_device *pdev,
> > struct nvkm_device **pdevice)
> > {
> > - struct drm_device *drm;
> > + struct nouveau_drm *nv_dev;
> > + struct drm_device *drm_dev;
> > int err;
> >
> > err = nvkm_device_tegra_new(func, pdev, nouveau_config, nouveau_debug,
> > @@ -1293,22 +1288,21 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
> > if (err)
> > goto err_free;
> >
> > - drm = drm_dev_alloc(&driver_platform, &pdev->dev);
> > - if (IS_ERR(drm)) {
> > - err = PTR_ERR(drm);
> > + nv_dev = devm_drm_dev_alloc(&pdev->dev, &driver_platform, typeof(*nv_dev), drm_dev);
> > + if (IS_ERR(nv_dev)) {
> > + err = PTR_ERR(nv_dev);
> > goto err_free;
> > }
> > + drm_dev = nouveau_to_drm_dev(nv_dev);
> >
> > - err = nouveau_drm_device_init(drm);
> > + err = nouveau_drm_device_init(drm_dev);
> > if (err)
> > - goto err_put;
> > + goto err_free;
> >
> > - platform_set_drvdata(pdev, drm);
> > + platform_set_drvdata(pdev, drm_dev);
> >
> > - return drm;
> > + return drm_dev;
> >
> > -err_put:
> > - drm_dev_put(drm);
> > err_free:
> > nvkm_device_del(pdevice);
> >
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > index 3e2920a10099..cf6c33e52a5c 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > @@ -137,7 +137,11 @@ struct nouveau_drm {
> > struct nvif_parent parent;
> > struct nouveau_cli master;
> > struct nouveau_cli client;
> > - struct drm_device *dev;
> > +
> > + /**
> > + * @drm_dev: The parent DRM device object.
> > + */
> > + struct drm_device drm_dev;
> >
> > struct list_head clients;
> >
> > @@ -237,7 +241,7 @@ struct nouveau_drm {
> > static inline struct nouveau_drm *
> > nouveau_drm(struct drm_device *dev)
> > {
> > - return dev->dev_private;
> > + return container_of(dev, struct nouveau_drm, drm_dev);
> > }
> >
> > /**
> > @@ -251,7 +255,7 @@ nouveau_drm(struct drm_device *dev)
> > */
> > static inline struct drm_device *
> > nouveau_to_drm_dev(struct nouveau_drm *nv_dev) {
> > - return nv_dev->dev;
> > + return &nv_dev->drm_dev;
> > }
> >
> > /**
> > --
> > 2.28.0
> >
> > _______________________________________________
> > Nouveau mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/nouveau
> >
>