From: Jani Nikula <jani.nikula@linux.intel.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
DRI Development <dri-devel@lists.freedesktop.org>
Cc: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>,
Mika Kuoppala <mika.kuoppala@linux.intel.com>,
Andi Shyti <andi.shyti@intel.com>,
Tvrtko Ursulin <tvrtko.ursulin@intel.com>,
Daniel Vetter <daniel.vetter@ffwll.ch>,
Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>,
Matthew Auld <matthew.auld@intel.com>,
Rodrigo Vivi <rodrigo.vivi@intel.com>,
Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 08/51] drm/i915: Use drmm_add_final_kfree
Date: Thu, 26 Mar 2020 15:10:50 +0200 [thread overview]
Message-ID: <87zhc3gt2d.fsf@intel.com> (raw)
In-Reply-To: <20200323144950.3018436-9-daniel.vetter@ffwll.ch>
On Mon, 23 Mar 2020, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> With this we can drop the final kfree from the release function.
>
> The mock device in the selftests needed it's pci_device split
> up from the drm_device. In the future we could simplify this again
> by allocating the pci_device as a managed allocation too.
>
> v2: I overlooked that i915_driver_destroy is also called in the
> unwind code of the error path. There we need a drm_dev_put.
> Similar for the mock object.
>
> Now the problem with that is that the drm_driver->release callbacks
> for both the real driver and the mock one assume everything has been
> set up. Hence going through that path for a partially set up driver
> will result in issues. Quickest fix is to disable the ->release() hook
> until the driver is fully initialized, and keep the onion unwinding.
> Long term would be cleanest to move everything over to drmm_ release
> actions, but that's a lot of work for a big driver like i915. Plus
> more core work needed first anyway.
>
> v3: Fix i915_drm pointer wrangling in mock_gem_device. Also switch
> over to start using drm_dev_put() to clean up even on the error path.
> Aside I think the current error path is leaking the allocation.
>
> v4: more fixes for intel-gfx-ci, some if it damage from v3 :-/
>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Matthew Auld <matthew.auld@intel.com>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Abdiel Janulgue <abdiel.janulgue@linux.intel.com>
> Cc: intel-gfx@lists.freedesktop.org
Okay, I didn't find any holes in this, and while I wish I had more
confidence I checked all the corner cases, this is
Reviewed-by: Jani Nikula <jani.nikula@intel.com>
A couple of notes below, don't have to do anything about them. (Maybe
better *not* to do anything about them, to move this forward. ;)
> ---
> drivers/gpu/drm/i915/i915_drv.c | 10 ++++++-
> drivers/gpu/drm/i915/i915_drv.h | 3 ++
> .../gpu/drm/i915/selftests/mock_gem_device.c | 30 ++++++++++++++-----
> 3 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 4792051e9e2e..481313536b5a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -43,6 +43,7 @@
> #include <drm/drm_atomic_helper.h>
> #include <drm/drm_ioctl.h>
> #include <drm/drm_irq.h>
> +#include <drm/drm_managed.h>
> #include <drm/drm_probe_helper.h>
>
> #include "display/intel_acpi.h"
> @@ -890,6 +891,8 @@ i915_driver_create(struct pci_dev *pdev, const struct pci_device_id *ent)
> return ERR_PTR(err);
> }
>
> + drmm_add_final_kfree(&i915->drm, i915);
> +
> i915->drm.pdev = pdev;
> pci_set_drvdata(pdev, i915);
>
> @@ -908,7 +911,6 @@ static void i915_driver_destroy(struct drm_i915_private *i915)
> struct pci_dev *pdev = i915->drm.pdev;
>
> drm_dev_fini(&i915->drm);
> - kfree(i915);
> }
>
> /**
> @@ -992,6 +994,8 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>
> i915_welcome_messages(i915);
>
> + i915->do_release = true;
> +
This whole ->do_release thing is obviously a bummer. I did wonder if we
could set driver->release to NULL initially, and set it to the proper
thing here. It would make drm_dev_put() handle drm_dev_fini() internally
too.
Less obvious? I don't know.
> return 0;
>
> out_cleanup_irq:
> @@ -1012,6 +1016,7 @@ int i915_driver_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> out_fini:
> i915_probe_error(i915, "Device initialization failed (%d)\n", ret);
> i915_driver_destroy(i915);
> + drm_dev_put(&i915->drm);
Also wondered about throwing i915_driver_destroy away, and inlining the
drm_dev_fini()...
> return ret;
> }
>
> @@ -1051,6 +1056,9 @@ static void i915_driver_release(struct drm_device *dev)
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct intel_runtime_pm *rpm = &dev_priv->runtime_pm;
>
> + if (!dev_priv->do_release)
...or, calling drm_dev_fini() in this branch, avoiding the need to call
it elsewhere.
*shrug*
All of it can be done afterwards, if deemed useful.
> + return;
> +
> disable_rpm_wakeref_asserts(rpm);
>
> i915_gem_driver_release(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a7ea1d855359..7ae652723ed7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -821,6 +821,9 @@ struct i915_selftest_stash {
> struct drm_i915_private {
> struct drm_device drm;
>
> + /* FIXME: Device release actions should all be moved to drmm_ */
> + bool do_release;
> +
> const struct intel_device_info __info; /* Use INTEL_INFO() to access. */
> struct intel_runtime_info __runtime; /* Use RUNTIME_INFO() to access. */
> struct intel_driver_caps caps;
> diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> index 754d0eb6beaa..acf889e4b993 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
> @@ -25,6 +25,8 @@
> #include <linux/pm_domain.h>
> #include <linux/pm_runtime.h>
>
> +#include <drm/drm_managed.h>
> +
> #include "gt/intel_gt.h"
> #include "gt/intel_gt_requests.h"
> #include "gt/mock_engine.h"
> @@ -55,6 +57,9 @@ static void mock_device_release(struct drm_device *dev)
> {
> struct drm_i915_private *i915 = to_i915(dev);
>
> + if (!i915->do_release)
> + goto out;
> +
> mock_device_flush(i915);
> intel_gt_driver_remove(&i915->gt);
>
> @@ -72,7 +77,9 @@ static void mock_device_release(struct drm_device *dev)
> drm_mode_config_cleanup(&i915->drm);
>
> drm_dev_fini(&i915->drm);
> +out:
> put_device(&i915->drm.pdev->dev);
> + i915->drm.pdev = NULL;
> }
>
> static struct drm_driver mock_driver = {
> @@ -114,9 +121,14 @@ struct drm_i915_private *mock_gem_device(void)
> struct pci_dev *pdev;
> int err;
>
> - pdev = kzalloc(sizeof(*pdev) + sizeof(*i915), GFP_KERNEL);
> + pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> if (!pdev)
> - goto err;
> + 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;
> @@ -129,7 +141,6 @@ struct drm_i915_private *mock_gem_device(void)
> pdev->dev.archdata.iommu = (void *)-1;
> #endif
>
> - i915 = (struct drm_i915_private *)(pdev + 1);
> pci_set_drvdata(pdev, i915);
>
> dev_pm_domain_set(&pdev->dev, &pm_domain);
> @@ -141,9 +152,13 @@ struct drm_i915_private *mock_gem_device(void)
> err = drm_dev_init(&i915->drm, &mock_driver, &pdev->dev);
> if (err) {
> pr_err("Failed to initialise mock GEM device: err=%d\n", err);
> - goto put_device;
> + put_device(&pdev->dev);
> + kfree(i915);
> +
> + return NULL;
> }
> i915->drm.pdev = pdev;
> + drmm_add_final_kfree(&i915->drm, i915);
>
> intel_runtime_pm_init_early(&i915->runtime_pm);
>
> @@ -188,6 +203,8 @@ struct drm_i915_private *mock_gem_device(void)
> __clear_bit(I915_WEDGED, &i915->gt.reset.flags);
> intel_engines_driver_register(i915);
>
> + i915->do_release = true;
> +
> return i915;
>
> err_context:
> @@ -199,8 +216,7 @@ struct drm_i915_private *mock_gem_device(void)
> intel_memory_regions_driver_release(i915);
> drm_mode_config_cleanup(&i915->drm);
> drm_dev_fini(&i915->drm);
> -put_device:
> - put_device(&pdev->dev);
> -err:
> + drm_dev_put(&i915->drm);
> +
> return NULL;
> }
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2020-03-26 13:11 UTC|newest]
Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-03-23 14:48 [PATCH 00/51] drm_device managed resources, v5 Daniel Vetter
2020-03-23 14:49 ` [PATCH 01/51] mm/sl[uo]b: export __kmalloc_track(_node)_caller Daniel Vetter
2020-03-26 13:46 ` Daniel Vetter
2020-03-23 14:49 ` [PATCH 02/51] drm/i915: Don't clear drvdata in ->release Daniel Vetter
2020-03-25 18:20 ` [Intel-gfx] " Jani Nikula
2020-03-26 13:15 ` Jani Nikula
2020-03-23 14:49 ` [PATCH 03/51] drm: add managed resources tied to drm_device Daniel Vetter
2020-03-23 18:36 ` Sam Ravnborg
2020-03-24 12:45 ` [PATCH] " Daniel Vetter
2020-03-23 14:49 ` [PATCH 04/51] drm: Set final_kfree in drm_dev_alloc Daniel Vetter
2020-03-23 14:49 ` [PATCH 05/51] drm/mipi_dbi: Use drmm_add_final_kfree in all drivers Daniel Vetter
2020-03-23 14:49 ` [PATCH 06/51] drm/udl: Use drmm_add_final_kfree Daniel Vetter
2020-03-23 14:49 ` [PATCH 07/51] drm/qxl: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 08/51] drm/i915: " Daniel Vetter
2020-03-26 13:10 ` Jani Nikula [this message]
2020-03-26 13:33 ` Daniel Vetter
2020-03-23 14:49 ` [PATCH 09/51] drm/cirrus: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 10/51] drm/v3d: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 11/51] drm/tidss: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 12/51] drm/mcde: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 13/51] drm/vgem: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 14/51] drm/vkms: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 15/51] drm/repaper: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 16/51] drm/ingenic: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 17/51] drm/gm12u320: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 18/51] drm/<drivers>: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 19/51] drm: Cleanups after drmm_add_final_kfree rollout Daniel Vetter
2020-04-02 0:50 ` Laurent Pinchart
2020-04-02 5:17 ` Daniel Vetter
2020-04-02 9:39 ` Laurent Pinchart
2020-04-02 9:50 ` Daniel Vetter
2020-03-23 14:49 ` [PATCH 20/51] drm: Handle dev->unique with drmm_ Daniel Vetter
2020-03-23 14:49 ` [PATCH 21/51] drm: Use drmm_ for drm_dev_init cleanup Daniel Vetter
2020-03-24 21:20 ` Sam Ravnborg
2020-03-23 14:49 ` [PATCH 22/51] drm: manage drm_minor cleanup with drmm_ Daniel Vetter
2020-03-24 8:54 ` Thomas Zimmermann
2020-03-24 20:39 ` [PATCH] " Daniel Vetter
2020-03-24 21:42 ` Sam Ravnborg
2020-03-25 9:09 ` Daniel Vetter
2020-03-24 21:36 ` [PATCH 22/51] " Sam Ravnborg
2020-03-25 9:07 ` Daniel Vetter
2020-03-23 14:49 ` [PATCH 23/51] drm: Manage drm_gem_init " Daniel Vetter
2020-03-23 14:49 ` [PATCH 24/51] drm: Manage drm_vblank_cleanup " Daniel Vetter
2020-03-23 14:49 ` [PATCH 25/51] drm: Garbage collect drm_dev_fini Daniel Vetter
2020-03-23 14:49 ` [PATCH 26/51] drm: Manage drm_mode_config_init with drmm_ Daniel Vetter
2020-03-23 14:49 ` [PATCH 27/51] drm/bochs: Remove leftover drm_atomic_helper_shutdown Daniel Vetter
2020-03-23 14:49 ` [PATCH 28/51] drm/bochs: Drop explicit drm_mode_config_cleanup Daniel Vetter
2020-03-23 14:49 ` [PATCH 29/51] drm/cirrus: Drop explicit drm_mode_config_cleanup call Daniel Vetter
2020-03-23 14:49 ` [PATCH 30/51] drm/cirrus: Fully embrace devm_ Daniel Vetter
2020-03-23 14:49 ` [PATCH 31/51] drm/ingenic: Drop explicit drm_mode_config_cleanup call Daniel Vetter
2020-03-23 14:49 ` [PATCH 32/51] drm/mcde: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 33/51] drm/mcde: More devm_drm_dev_init Daniel Vetter
2020-03-23 14:49 ` [PATCH 34/51] drm/meson: Drop explicit drm_mode_config_cleanup call Daniel Vetter
2020-03-23 14:49 ` [PATCH 35/51] drm/pl111: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 36/51] drm/rcar-du: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 37/51] drm/rockchip: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 38/51] drm/stm: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 39/51] drm/shmob: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 40/51] drm/mtk: " Daniel Vetter
2020-03-23 15:27 ` Chun-Kuang Hu
2020-03-23 14:49 ` [PATCH 41/51] drm/tidss: " Daniel Vetter
2020-03-23 14:49 ` [PATCH 42/51] drm/gm12u320: More drmm_ Daniel Vetter
2020-03-23 14:49 ` [PATCH 43/51] drm/gm12u320: Use devm_drm_dev_init Daniel Vetter
2020-03-23 14:49 ` [PATCH 44/51] drm/gm12u320: Use helpers for shutdown/suspend/resume Daniel Vetter
2020-03-23 14:49 ` [PATCH 45/51] drm/gm12u320: Simplify upload work Daniel Vetter
2020-03-23 14:49 ` [PATCH 46/51] drm/repaper: Drop explicit drm_mode_config_cleanup call Daniel Vetter
2020-03-23 14:49 ` [PATCH 47/51] drm/mipi-dbi: Move drm_mode_config_init into mipi library Daniel Vetter
2020-03-23 14:49 ` [PATCH 48/51] drm/mipi-dbi: Drop explicit drm_mode_config_cleanup call Daniel Vetter
2020-03-23 14:49 ` [PATCH 49/51] drm/udl: " Daniel Vetter
2020-03-24 8:56 ` Thomas Zimmermann
2020-03-23 14:49 ` [PATCH 50/51] drm/udl: drop drm_driver.release hook Daniel Vetter
2020-03-23 14:49 ` [PATCH 51/51] drm: Add docs for managed resources Daniel Vetter
2020-03-26 15:10 ` [PATCH 00/51] drm_device managed resources, v5 Daniel Vetter
-- strict thread matches above, loose matches on Subject: below --
2020-03-02 22:25 [PATCH 00/51] drm_device managed resources, v4 Daniel Vetter
2020-03-02 22:25 ` [PATCH 08/51] drm/i915: Use drmm_add_final_kfree Daniel Vetter
2020-02-27 18:14 [PATCH 00/51] drm managed resources, v3 Daniel Vetter
2020-02-27 18:14 ` [PATCH 08/51] drm/i915: Use drmm_add_final_kfree Daniel Vetter
2020-02-21 21:02 [PATCH 00/51] drm managed resources, v2 Daniel Vetter
2020-02-21 21:02 ` [PATCH 08/51] drm/i915: Use drmm_add_final_kfree Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87zhc3gt2d.fsf@intel.com \
--to=jani.nikula@linux.intel.com \
--cc=abdiel.janulgue@linux.intel.com \
--cc=andi.shyti@intel.com \
--cc=daniel.vetter@ffwll.ch \
--cc=daniel.vetter@intel.com \
--cc=daniele.ceraolospurio@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=matthew.auld@intel.com \
--cc=mika.kuoppala@linux.intel.com \
--cc=rodrigo.vivi@intel.com \
--cc=tvrtko.ursulin@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).