Re: [PATCH v2 02/37] drm/nouveau: handle pci/tegra drm_dev_{alloc, register} from common code

[Danilo Krummrich <dakr@redhat.com>]

On Thu, Jul 18, 2024 at 05:14:15PM +1000, Ben Skeggs wrote:
> On 10/7/24 01:16, Danilo Krummrich wrote:
> 
> > On Fri, Jul 05, 2024 at 04:36:46AM +1000, Ben Skeggs wrote:
> > > The next commit will change the pointer we store via dev_set_drvdata()
> > > to allow simplifying the code using it.
> > Please don't use future tense, e.g "In subsequent commits, the pointer we store
> > [...]". Also, when you mention that something is changes (such as the pointer
> > type here), it probably makes sense to mention what it is changed to instead.
> > 
> > > Here we want to unify some more of the PCI/Tegra DRM driver init, both
> > > as a general cleanup, and to enable the dev_set_drvdata() change to be
> > > made in a single place.
> > In this case I think it makes sense to switch things up. First mention what the
> > commit does and why, i.e. "Unify some more of the PCI/Tegra DRM driver init
> > [...]" and then mention that the actual change to the pointer stored in a
> > device' drvdata in done in a subsequent commit.
> 
> I've reworded the commit message.
> 
> 
> > 
> > > Signed-off-by: Ben Skeggs <bskeggs@nvidia.com>
> > > ---
> > >   drivers/gpu/drm/nouveau/nouveau_drm.c      | 93 ++++++++++++++--------
> > >   drivers/gpu/drm/nouveau/nouveau_platform.c |  6 --
> > >   2 files changed, 60 insertions(+), 39 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > index eae48c87e3d5..9beff8737617 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > @@ -628,20 +628,14 @@ nouveau_drm_device_fini(struct drm_device *dev)
> > >   	destroy_workqueue(drm->sched_wq);
> > >   	nvif_parent_dtor(&drm->parent);
> > >   	mutex_destroy(&drm->clients_lock);
> > > -	kfree(drm);
> > >   }
> > >   static int
> > > -nouveau_drm_device_init(struct drm_device *dev)
> > > +nouveau_drm_device_init(struct nouveau_drm *drm)
> > >   {
> > > -	struct nouveau_drm *drm;
> > > +	struct drm_device *dev = drm->dev;
> > >   	int ret;
> > > -	if (!(drm = kzalloc(sizeof(*drm), GFP_KERNEL)))
> > > -		return -ENOMEM;
> > > -	dev->dev_private = drm;
> > > -	drm->dev = dev;
> > > -
> > >   	nvif_parent_ctor(&nouveau_parent, &drm->parent);
> > >   	drm->master.base.object.parent = &drm->parent;
> > > @@ -711,6 +705,12 @@ nouveau_drm_device_init(struct drm_device *dev)
> > >   		pm_runtime_put(dev->dev);
> > >   	}
> > > +	ret = drm_dev_register(drm->dev, 0);
> > > +	if (ret) {
> > > +		nouveau_drm_device_fini(drm->dev);
> > > +		return ret;
> > > +	}
> > > +
> > >   	return 0;
> > >   fail_dispinit:
> > >   	nouveau_display_destroy(dev);
> > > @@ -728,10 +728,47 @@ nouveau_drm_device_init(struct drm_device *dev)
> > >   	destroy_workqueue(drm->sched_wq);
> > >   fail_alloc:
> > >   	nvif_parent_dtor(&drm->parent);
> > > -	kfree(drm);
> > >   	return ret;
> > >   }
> > > +static void
> > > +nouveau_drm_device_del(struct nouveau_drm *drm)
> > > +{
> > > +	if (drm->dev)
> > > +		drm_dev_put(drm->dev);
> > > +
> > > +	kfree(drm);
> > > +}
> > > +
> > > +static struct nouveau_drm *
> > > +nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *parent,
> > > +		       struct nvkm_device *device)
> > > +{
> > > +	struct nouveau_drm *drm;
> > > +	int ret;
> > > +
> > > +	drm = kzalloc(sizeof(*drm), GFP_KERNEL);
> > > +	if (!drm)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	drm->dev = drm_dev_alloc(drm_driver, parent);
> > Since you're reworking this anyways, can we switch to devm_drm_dev_alloc()?
> > 
> > This also gets us rid of nouveau_drm_device_del().
> 
> No, we can't.  I originally had this change as a cleanup patch in the series
> I posted implementing aux bus support.  However it turns out that in order
> to avoid breaking udev etc, we can't use the aux device as parent of the drm

Can you please expand a bit on what was breaking?

> device and instead have to continue passing the pci/platform device as we do
> now.
> 
> Using devm_drm_dev_alloc() with the pci device as parent would tie the
> lifetime of the drm device to the pci device, which is owned by nvkm (after

How does this tie the lifetime of the drm device to the pci device? It's the
other way around, the drm device takes a reference of its parent (i.e. the pci
device).

I don't think there's anything wrong with that.

> the auxdev series).  We could look at changing devm_drm_dev_alloc() of
> course, but I think that's best left until later.

I don't think that this is necessary.

> 
> > 
> > > +	if (IS_ERR(drm->dev)) {
> > > +		ret = PTR_ERR(drm->dev);
> > > +		goto done;
> > > +	}
> > > +
> > > +	drm->dev->dev_private = drm;
> > > +	dev_set_drvdata(parent, drm->dev);
> > > +
> > > +done:
> > > +	if (ret) {
> > > +		nouveau_drm_device_del(drm);
> > > +		drm = NULL;
> > > +	}
> > > +
> > > +	return ret ? ERR_PTR(ret) : drm;
> > > +}
> > > +
> > >   /*
> > >    * On some Intel PCIe bridge controllers doing a
> > >    * D0 -> D3hot -> D3cold -> D0 sequence causes Nvidia GPUs to not reappear.
> > > @@ -794,7 +831,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > >   			     const struct pci_device_id *pent)
> > >   {
> > >   	struct nvkm_device *device;
> > > -	struct drm_device *drm_dev;
> > > +	struct nouveau_drm *drm;
> > >   	int ret;
> > >   	if (vga_switcheroo_client_probe_defer(pdev))
> > > @@ -825,9 +862,9 @@ 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);
> > > +	drm = nouveau_drm_device_new(&driver_pci, &pdev->dev, device);
> > > +	if (IS_ERR(drm)) {
> > > +		ret = PTR_ERR(drm);
> > >   		goto fail_nvkm;
> > >   	}
> > > @@ -835,30 +872,22 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
> > >   	if (ret)
> > >   		goto fail_drm;
> > > -	pci_set_drvdata(pdev, drm_dev);
> > > -
> > > -	ret = nouveau_drm_device_init(drm_dev);
> > > +	ret = nouveau_drm_device_init(drm);
> > >   	if (ret)
> > >   		goto fail_pci;
> > > -	ret = drm_dev_register(drm_dev, pent->driver_data);
> > > -	if (ret)
> > > -		goto fail_drm_dev_init;
> > > -
> > > -	if (nouveau_drm(drm_dev)->client.device.info.ram_size <= 32 * 1024 * 1024)
> > > -		drm_fbdev_ttm_setup(drm_dev, 8);
> > > +	if (drm->client.device.info.ram_size <= 32 * 1024 * 1024)
> > > +		drm_fbdev_ttm_setup(drm->dev, 8);
> > >   	else
> > > -		drm_fbdev_ttm_setup(drm_dev, 32);
> > > +		drm_fbdev_ttm_setup(drm->dev, 32);
> > >   	quirk_broken_nv_runpm(pdev);
> > >   	return 0;
> > > -fail_drm_dev_init:
> > > -	nouveau_drm_device_fini(drm_dev);
> > >   fail_pci:
> > >   	pci_disable_device(pdev);
> > >   fail_drm:
> > > -	drm_dev_put(drm_dev);
> > > +	nouveau_drm_device_del(drm);
> > >   fail_nvkm:
> > >   	nvkm_device_del(&device);
> > >   	return ret;
> > > @@ -877,7 +906,7 @@ nouveau_drm_device_remove(struct drm_device *dev)
> > >   	device = nvkm_device_find(client->device);
> > >   	nouveau_drm_device_fini(dev);
> > > -	drm_dev_put(dev);
> > > +	nouveau_drm_device_del(drm);
> > >   	nvkm_device_del(&device);
> > >   }
> > > @@ -1369,7 +1398,7 @@ 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 *drm;
> > >   	int err;
> > >   	err = nvkm_device_tegra_new(func, pdev, nouveau_config, nouveau_debug,
> > > @@ -1377,7 +1406,7 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
> > >   	if (err)
> > >   		goto err_free;
> > > -	drm = drm_dev_alloc(&driver_platform, &pdev->dev);
> > > +	drm = nouveau_drm_device_new(&driver_platform, &pdev->dev, *pdevice);
> > >   	if (IS_ERR(drm)) {
> > >   		err = PTR_ERR(drm);
> > >   		goto err_free;
> > > @@ -1387,12 +1416,10 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
> > >   	if (err)
> > >   		goto err_put;
> > > -	platform_set_drvdata(pdev, drm);
> > > -
> > > -	return drm;
> > > +	return drm->dev;
> > >   err_put:
> > > -	drm_dev_put(drm);
> > > +	nouveau_drm_device_del(drm);
> > >   err_free:
> > >   	nvkm_device_del(pdevice);
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_platform.c b/drivers/gpu/drm/nouveau/nouveau_platform.c
> > > index bf2dc7567ea4..d0a63f0f54a2 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_platform.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_platform.c
> > > @@ -34,12 +34,6 @@ static int nouveau_platform_probe(struct platform_device *pdev)
> > >   	if (IS_ERR(drm))
> > >   		return PTR_ERR(drm);
> > > -	ret = drm_dev_register(drm, 0);
> > > -	if (ret < 0) {
> > > -		drm_dev_put(drm);
> > > -		return ret;
> > > -	}
> > > -
> > >   	return 0;
> > >   }
> > > -- 
> > > 2.45.1
> > > 
>