diff mbox series

[2/4] drm/i915/selftests: align more to real device lifetimes

Message ID 20200918132505.2316382-3-daniel.vetter@ffwll.ch (mailing list archive)
State New, archived
Headers show
Series managed drm_device, absolute final leftover bits | expand

Commit Message

Daniel Vetter Sept. 18, 2020, 1:25 p.m. UTC 
The big change is device_add so that device_del can auto-cleanup
devres resources. This allows us to use devm_drm_dev_alloc, which
removes the last user of drm_dev_init.

v2: Rebased

v3: use devres_open/release_group so we can use devm without real
hacks in the driver core or having to create an entire fake bus for
testing drivers. Might want to extract this into helpers eventually,
maybe as a mock_drm_dev_alloc or test_drm_dev_alloc.

v4:
- Fix IS_ERR handling (Matt)
- Delete surplus put_device() in mock_device_release (intel-gfx-ci)

Cc: Matthew Auld <matthew.william.auld@gmail.com>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (v3)
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 .../gpu/drm/i915/selftests/mock_gem_device.c  | 44 +++++++++++--------
 1 file changed, 25 insertions(+), 19 deletions(-)

Comments

Matthew Auld Sept. 18, 2020, 5:50 p.m. UTC | #1 
On Fri, 18 Sep 2020 at 14:25, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> The big change is device_add so that device_del can auto-cleanup
> devres resources. This allows us to use devm_drm_dev_alloc, which
> removes the last user of drm_dev_init.
>
> v2: Rebased
>
> v3: use devres_open/release_group so we can use devm without real
> hacks in the driver core or having to create an entire fake bus for
> testing drivers. Might want to extract this into helpers eventually,
> maybe as a mock_drm_dev_alloc or test_drm_dev_alloc.
>
> v4:
> - Fix IS_ERR handling (Matt)
> - Delete surplus put_device() in mock_device_release (intel-gfx-ci)
>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (v3)
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  .../gpu/drm/i915/selftests/mock_gem_device.c  | 44 +++++++++++--------
>  1 file changed, 25 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> index ac600d395c8f..816f9af15fb3 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> @@ -79,8 +79,6 @@ static void mock_device_release(struct drm_device *dev)
>
>  out:
>         i915_params_free(&i915->params);
> -       put_device(&i915->drm.pdev->dev);
> -       i915->drm.pdev = NULL;
>  }
>
>  static struct drm_driver mock_driver = {
> @@ -128,12 +126,6 @@ struct drm_i915_private *mock_gem_device(void)
>         pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
>         if (!pdev)
>                 return NULL;
> -       i915 = kzalloc(sizeof(*i915), GFP_KERNEL);
> -       if (!i915) {
> -               kfree(pdev);
> -               return NULL;
> -       }
> -
>         device_initialize(&pdev->dev);
>         pdev->class = PCI_BASE_CLASS_DISPLAY << 16;
>         pdev->dev.release = release_dev;
> @@ -144,8 +136,29 @@ struct drm_i915_private *mock_gem_device(void)
>         /* HACK to disable iommu for the fake device; force identity mapping */
>         pdev->dev.iommu = &fake_iommu;
>  #endif
> +       err = device_add(&pdev->dev);
> +       if (err) {
> +               kfree(pdev);
> +               return NULL;
> +       }
> +
> +       if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
> +               device_del(&pdev->dev);
> +               return NULL;
> +       }
> +
> +       i915 = devm_drm_dev_alloc(&pdev->dev, &mock_driver,
> +                                 struct drm_i915_private, drm);
> +       if (IS_ERR(i915)) {
> +               pr_err("Failed to allocate mock GEM device: err=%d\n", err);

err = PTR_ERR(i915)

Reviewed-by: Matthew Auld <matthew.auld@intel.com>
Daniel Vetter Sept. 18, 2020, 6:22 p.m. UTC | #2 
On Fri, Sep 18, 2020 at 7:50 PM Matthew Auld
<matthew.william.auld@gmail.com> wrote:
>
> On Fri, 18 Sep 2020 at 14:25, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > The big change is device_add so that device_del can auto-cleanup
> > devres resources. This allows us to use devm_drm_dev_alloc, which
> > removes the last user of drm_dev_init.
> >
> > v2: Rebased
> >
> > v3: use devres_open/release_group so we can use devm without real
> > hacks in the driver core or having to create an entire fake bus for
> > testing drivers. Might want to extract this into helpers eventually,
> > maybe as a mock_drm_dev_alloc or test_drm_dev_alloc.
> >
> > v4:
> > - Fix IS_ERR handling (Matt)
> > - Delete surplus put_device() in mock_device_release (intel-gfx-ci)
> >
> > Cc: Matthew Auld <matthew.william.auld@gmail.com>
> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (v3)
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  .../gpu/drm/i915/selftests/mock_gem_device.c  | 44 +++++++++++--------
> >  1 file changed, 25 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> > index ac600d395c8f..816f9af15fb3 100644
> > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> > @@ -79,8 +79,6 @@ static void mock_device_release(struct drm_device *dev)
> >
> >  out:
> >         i915_params_free(&i915->params);
> > -       put_device(&i915->drm.pdev->dev);
> > -       i915->drm.pdev = NULL;
> >  }
> >
> >  static struct drm_driver mock_driver = {
> > @@ -128,12 +126,6 @@ struct drm_i915_private *mock_gem_device(void)
> >         pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> >         if (!pdev)
> >                 return NULL;
> > -       i915 = kzalloc(sizeof(*i915), GFP_KERNEL);
> > -       if (!i915) {
> > -               kfree(pdev);
> > -               return NULL;
> > -       }
> > -
> >         device_initialize(&pdev->dev);
> >         pdev->class = PCI_BASE_CLASS_DISPLAY << 16;
> >         pdev->dev.release = release_dev;
> > @@ -144,8 +136,29 @@ struct drm_i915_private *mock_gem_device(void)
> >         /* HACK to disable iommu for the fake device; force identity mapping */
> >         pdev->dev.iommu = &fake_iommu;
> >  #endif
> > +       err = device_add(&pdev->dev);
> > +       if (err) {
> > +               kfree(pdev);
> > +               return NULL;
> > +       }
> > +
> > +       if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
> > +               device_del(&pdev->dev);
> > +               return NULL;
> > +       }
> > +
> > +       i915 = devm_drm_dev_alloc(&pdev->dev, &mock_driver,
> > +                                 struct drm_i915_private, drm);
> > +       if (IS_ERR(i915)) {
> > +               pr_err("Failed to allocate mock GEM device: err=%d\n", err);
>
> err = PTR_ERR(i915)

Are you sure? We return a pointer here, and callers just expect NULL
when stuff fails (so neither errno nor ptr-encoded errno).
-Daniel

> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
Matthew Auld Sept. 18, 2020, 6:31 p.m. UTC | #3 
On Fri, 18 Sep 2020 at 19:22, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
> On Fri, Sep 18, 2020 at 7:50 PM Matthew Auld
> <matthew.william.auld@gmail.com> wrote:
> >
> > On Fri, 18 Sep 2020 at 14:25, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > >
> > > The big change is device_add so that device_del can auto-cleanup
> > > devres resources. This allows us to use devm_drm_dev_alloc, which
> > > removes the last user of drm_dev_init.
> > >
> > > v2: Rebased
> > >
> > > v3: use devres_open/release_group so we can use devm without real
> > > hacks in the driver core or having to create an entire fake bus for
> > > testing drivers. Might want to extract this into helpers eventually,
> > > maybe as a mock_drm_dev_alloc or test_drm_dev_alloc.
> > >
> > > v4:
> > > - Fix IS_ERR handling (Matt)
> > > - Delete surplus put_device() in mock_device_release (intel-gfx-ci)
> > >
> > > Cc: Matthew Auld <matthew.william.auld@gmail.com>
> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (v3)
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > ---
> > >  .../gpu/drm/i915/selftests/mock_gem_device.c  | 44 +++++++++++--------
> > >  1 file changed, 25 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> > > index ac600d395c8f..816f9af15fb3 100644
> > > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> > > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> > > @@ -79,8 +79,6 @@ static void mock_device_release(struct drm_device *dev)
> > >
> > >  out:
> > >         i915_params_free(&i915->params);
> > > -       put_device(&i915->drm.pdev->dev);
> > > -       i915->drm.pdev = NULL;
> > >  }
> > >
> > >  static struct drm_driver mock_driver = {
> > > @@ -128,12 +126,6 @@ struct drm_i915_private *mock_gem_device(void)
> > >         pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> > >         if (!pdev)
> > >                 return NULL;
> > > -       i915 = kzalloc(sizeof(*i915), GFP_KERNEL);
> > > -       if (!i915) {
> > > -               kfree(pdev);
> > > -               return NULL;
> > > -       }
> > > -
> > >         device_initialize(&pdev->dev);
> > >         pdev->class = PCI_BASE_CLASS_DISPLAY << 16;
> > >         pdev->dev.release = release_dev;
> > > @@ -144,8 +136,29 @@ struct drm_i915_private *mock_gem_device(void)
> > >         /* HACK to disable iommu for the fake device; force identity mapping */
> > >         pdev->dev.iommu = &fake_iommu;
> > >  #endif
> > > +       err = device_add(&pdev->dev);
> > > +       if (err) {
> > > +               kfree(pdev);
> > > +               return NULL;
> > > +       }
> > > +
> > > +       if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
> > > +               device_del(&pdev->dev);
> > > +               return NULL;
> > > +       }
> > > +
> > > +       i915 = devm_drm_dev_alloc(&pdev->dev, &mock_driver,
> > > +                                 struct drm_i915_private, drm);
> > > +       if (IS_ERR(i915)) {
> > > +               pr_err("Failed to allocate mock GEM device: err=%d\n", err);
> >
> > err = PTR_ERR(i915)
>
> Are you sure? We return a pointer here, and callers just expect NULL
> when stuff fails (so neither errno nor ptr-encoded errno).

I just meant for the pr_err() which is printing the err(from the
copy-paste), but it will always be zero without the above.

> -Daniel
>
> > Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Daniel Vetter Sept. 18, 2020, 6:46 p.m. UTC | #4 
On Fri, Sep 18, 2020 at 8:31 PM Matthew Auld
<matthew.william.auld@gmail.com> wrote:
>
> On Fri, 18 Sep 2020 at 19:22, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> > On Fri, Sep 18, 2020 at 7:50 PM Matthew Auld
> > <matthew.william.auld@gmail.com> wrote:
> > >
> > > On Fri, 18 Sep 2020 at 14:25, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > >
> > > > The big change is device_add so that device_del can auto-cleanup
> > > > devres resources. This allows us to use devm_drm_dev_alloc, which
> > > > removes the last user of drm_dev_init.
> > > >
> > > > v2: Rebased
> > > >
> > > > v3: use devres_open/release_group so we can use devm without real
> > > > hacks in the driver core or having to create an entire fake bus for
> > > > testing drivers. Might want to extract this into helpers eventually,
> > > > maybe as a mock_drm_dev_alloc or test_drm_dev_alloc.
> > > >
> > > > v4:
> > > > - Fix IS_ERR handling (Matt)
> > > > - Delete surplus put_device() in mock_device_release (intel-gfx-ci)
> > > >
> > > > Cc: Matthew Auld <matthew.william.auld@gmail.com>
> > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> (v3)
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > > > ---
> > > >  .../gpu/drm/i915/selftests/mock_gem_device.c  | 44 +++++++++++--------
> > > >  1 file changed, 25 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> > > > index ac600d395c8f..816f9af15fb3 100644
> > > > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> > > > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> > > > @@ -79,8 +79,6 @@ static void mock_device_release(struct drm_device *dev)
> > > >
> > > >  out:
> > > >         i915_params_free(&i915->params);
> > > > -       put_device(&i915->drm.pdev->dev);
> > > > -       i915->drm.pdev = NULL;
> > > >  }
> > > >
> > > >  static struct drm_driver mock_driver = {
> > > > @@ -128,12 +126,6 @@ struct drm_i915_private *mock_gem_device(void)
> > > >         pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> > > >         if (!pdev)
> > > >                 return NULL;
> > > > -       i915 = kzalloc(sizeof(*i915), GFP_KERNEL);
> > > > -       if (!i915) {
> > > > -               kfree(pdev);
> > > > -               return NULL;
> > > > -       }
> > > > -
> > > >         device_initialize(&pdev->dev);
> > > >         pdev->class = PCI_BASE_CLASS_DISPLAY << 16;
> > > >         pdev->dev.release = release_dev;
> > > > @@ -144,8 +136,29 @@ struct drm_i915_private *mock_gem_device(void)
> > > >         /* HACK to disable iommu for the fake device; force identity mapping */
> > > >         pdev->dev.iommu = &fake_iommu;
> > > >  #endif
> > > > +       err = device_add(&pdev->dev);
> > > > +       if (err) {
> > > > +               kfree(pdev);
> > > > +               return NULL;
> > > > +       }
> > > > +
> > > > +       if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
> > > > +               device_del(&pdev->dev);
> > > > +               return NULL;
> > > > +       }
> > > > +
> > > > +       i915 = devm_drm_dev_alloc(&pdev->dev, &mock_driver,
> > > > +                                 struct drm_i915_private, drm);
> > > > +       if (IS_ERR(i915)) {
> > > > +               pr_err("Failed to allocate mock GEM device: err=%d\n", err);
> > >
> > > err = PTR_ERR(i915)
> >
> > Are you sure? We return a pointer here, and callers just expect NULL
> > when stuff fails (so neither errno nor ptr-encoded errno).
>
> I just meant for the pr_err() which is printing the err(from the
> copy-paste), but it will always be zero without the above.

Ah right, I missed that when applying your previous comment, thanks
for pointing it out.
-Daniel

>
> > -Daniel
> >
> > > Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
index ac600d395c8f..816f9af15fb3 100644
--- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
+++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
@@ -79,8 +79,6 @@  static void mock_device_release(struct drm_device *dev)
 
 out:
 	i915_params_free(&i915->params);
-	put_device(&i915->drm.pdev->dev);
-	i915->drm.pdev = NULL;
 }
 
 static struct drm_driver mock_driver = {
@@ -128,12 +126,6 @@  struct drm_i915_private *mock_gem_device(void)
 	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
 	if (!pdev)
 		return NULL;
-	i915 = kzalloc(sizeof(*i915), GFP_KERNEL);
-	if (!i915) {
-		kfree(pdev);
-		return NULL;
-	}
-
 	device_initialize(&pdev->dev);
 	pdev->class = PCI_BASE_CLASS_DISPLAY << 16;
 	pdev->dev.release = release_dev;
@@ -144,8 +136,29 @@  struct drm_i915_private *mock_gem_device(void)
 	/* HACK to disable iommu for the fake device; force identity mapping */
 	pdev->dev.iommu = &fake_iommu;
 #endif
+	err = device_add(&pdev->dev);
+	if (err) {
+		kfree(pdev);
+		return NULL;
+	}
+
+	if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
+		device_del(&pdev->dev);
+		return NULL;
+	}
+
+	i915 = devm_drm_dev_alloc(&pdev->dev, &mock_driver,
+				  struct drm_i915_private, drm);
+	if (IS_ERR(i915)) {
+		pr_err("Failed to allocate mock GEM device: err=%d\n", err);
+		devres_release_group(&pdev->dev, NULL);
+		device_del(&pdev->dev);
+
+		return NULL;
+	}
 
 	pci_set_drvdata(pdev, i915);
+	i915->drm.pdev = pdev;
 
 	dev_pm_domain_set(&pdev->dev, &pm_domain);
 	pm_runtime_enable(&pdev->dev);
@@ -153,16 +166,6 @@  struct drm_i915_private *mock_gem_device(void)
 	if (pm_runtime_enabled(&pdev->dev))
 		WARN_ON(pm_runtime_get_sync(&pdev->dev));
 
-	err = drm_dev_init(&i915->drm, &mock_driver, &pdev->dev);
-	if (err) {
-		pr_err("Failed to initialise mock GEM device: err=%d\n", err);
-		put_device(&pdev->dev);
-		kfree(i915);
-
-		return NULL;
-	}
-	i915->drm.pdev = pdev;
-	drmm_add_final_kfree(&i915->drm, i915);
 
 	i915_params_copy(&i915->params, &i915_modparams);
 
@@ -229,5 +232,8 @@  struct drm_i915_private *mock_gem_device(void)
 
 void mock_destroy_device(struct drm_i915_private *i915)
 {
-	drm_dev_put(&i915->drm);
+	struct device *dev = i915->drm.dev;
+
+	devres_release_group(dev, NULL);
+	device_del(dev);
 }