[PATCH v2] drm/tegra: Do not use ->load() and ->unload() callbacks

[PATCH v2] drm/tegra: Do not use ->load() and ->unload() callbacks

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

The ->load() and ->unload() drivers are midlayers and should be avoided
in modern drivers. Fix this by moving the code into the driver ->probe()
and ->remove() implementations, respectively.

v2: kick out conflicting framebuffers before initializing fbdev

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Michał, Dmitry,

do you guys have a way of testing that the removal of the conflicting
framebuffer actually works?

Thierry

 drivers/gpu/drm/tegra/drm.c | 389 +++++++++++++++++-------------------
 1 file changed, 188 insertions(+), 201 deletions(-)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 3012f13bab97..ed2ae898726c 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -82,202 +82,6 @@ tegra_drm_mode_config_helpers = {
 	.atomic_commit_tail = tegra_atomic_commit_tail,
 };
 
-static int tegra_drm_load(struct drm_device *drm, unsigned long flags)
-{
-	struct host1x_device *device = to_host1x_device(drm->dev);
-	struct iommu_domain *domain;
-	struct tegra_drm *tegra;
-	int err;
-
-	tegra = kzalloc(sizeof(*tegra), GFP_KERNEL);
-	if (!tegra)
-		return -ENOMEM;
-
-	/*
-	 * If the Tegra DRM clients are backed by an IOMMU, push buffers are
-	 * likely to be allocated beyond the 32-bit boundary if sufficient
-	 * system memory is available. This is problematic on earlier Tegra
-	 * generations where host1x supports a maximum of 32 address bits in
-	 * the GATHER opcode. In this case, unless host1x is behind an IOMMU
-	 * as well it won't be able to process buffers allocated beyond the
-	 * 32-bit boundary.
-	 *
-	 * The DMA API will use bounce buffers in this case, so that could
-	 * perhaps still be made to work, even if less efficient, but there
-	 * is another catch: in order to perform cache maintenance on pages
-	 * allocated for discontiguous buffers we need to map and unmap the
-	 * SG table representing these buffers. This is fine for something
-	 * small like a push buffer, but it exhausts the bounce buffer pool
-	 * (typically on the order of a few MiB) for framebuffers (many MiB
-	 * for any modern resolution).
-	 *
-	 * Work around this by making sure that Tegra DRM clients only use
-	 * an IOMMU if the parent host1x also uses an IOMMU.
-	 *
-	 * Note that there's still a small gap here that we don't cover: if
-	 * the DMA API is backed by an IOMMU there's no way to control which
-	 * device is attached to an IOMMU and which isn't, except via wiring
-	 * up the device tree appropriately. This is considered an problem
-	 * of integration, so care must be taken for the DT to be consistent.
-	 */
-	domain = iommu_get_domain_for_dev(drm->dev->parent);
-
-	if (domain && iommu_present(&platform_bus_type)) {
-		tegra->domain = iommu_domain_alloc(&platform_bus_type);
-		if (!tegra->domain) {
-			err = -ENOMEM;
-			goto free;
-		}
-
-		err = iova_cache_get();
-		if (err < 0)
-			goto domain;
-	}
-
-	mutex_init(&tegra->clients_lock);
-	INIT_LIST_HEAD(&tegra->clients);
-
-	drm->dev_private = tegra;
-	tegra->drm = drm;
-
-	drm_mode_config_init(drm);
-
-	drm->mode_config.min_width = 0;
-	drm->mode_config.min_height = 0;
-
-	drm->mode_config.max_width = 4096;
-	drm->mode_config.max_height = 4096;
-
-	drm->mode_config.allow_fb_modifiers = true;
-
-	drm->mode_config.normalize_zpos = true;
-
-	drm->mode_config.funcs = &tegra_drm_mode_config_funcs;
-	drm->mode_config.helper_private = &tegra_drm_mode_config_helpers;
-
-	err = tegra_drm_fb_prepare(drm);
-	if (err < 0)
-		goto config;
-
-	drm_kms_helper_poll_init(drm);
-
-	err = host1x_device_init(device);
-	if (err < 0)
-		goto fbdev;
-
-	if (tegra->group) {
-		u64 carveout_start, carveout_end, gem_start, gem_end;
-		u64 dma_mask = dma_get_mask(&device->dev);
-		dma_addr_t start, end;
-		unsigned long order;
-
-		start = tegra->domain->geometry.aperture_start & dma_mask;
-		end = tegra->domain->geometry.aperture_end & dma_mask;
-
-		gem_start = start;
-		gem_end = end - CARVEOUT_SZ;
-		carveout_start = gem_end + 1;
-		carveout_end = end;
-
-		order = __ffs(tegra->domain->pgsize_bitmap);
-		init_iova_domain(&tegra->carveout.domain, 1UL << order,
-				 carveout_start >> order);
-
-		tegra->carveout.shift = iova_shift(&tegra->carveout.domain);
-		tegra->carveout.limit = carveout_end >> tegra->carveout.shift;
-
-		drm_mm_init(&tegra->mm, gem_start, gem_end - gem_start + 1);
-		mutex_init(&tegra->mm_lock);
-
-		DRM_DEBUG_DRIVER("IOMMU apertures:\n");
-		DRM_DEBUG_DRIVER("  GEM: %#llx-%#llx\n", gem_start, gem_end);
-		DRM_DEBUG_DRIVER("  Carveout: %#llx-%#llx\n", carveout_start,
-				 carveout_end);
-	} else if (tegra->domain) {
-		iommu_domain_free(tegra->domain);
-		tegra->domain = NULL;
-		iova_cache_put();
-	}
-
-	if (tegra->hub) {
-		err = tegra_display_hub_prepare(tegra->hub);
-		if (err < 0)
-			goto device;
-	}
-
-	/*
-	 * We don't use the drm_irq_install() helpers provided by the DRM
-	 * core, so we need to set this manually in order to allow the
-	 * DRM_IOCTL_WAIT_VBLANK to operate correctly.
-	 */
-	drm->irq_enabled = true;
-
-	/* syncpoints are used for full 32-bit hardware VBLANK counters */
-	drm->max_vblank_count = 0xffffffff;
-
-	err = drm_vblank_init(drm, drm->mode_config.num_crtc);
-	if (err < 0)
-		goto hub;
-
-	drm_mode_config_reset(drm);
-
-	err = tegra_drm_fb_init(drm);
-	if (err < 0)
-		goto hub;
-
-	return 0;
-
-hub:
-	if (tegra->hub)
-		tegra_display_hub_cleanup(tegra->hub);
-device:
-	if (tegra->domain) {
-		mutex_destroy(&tegra->mm_lock);
-		drm_mm_takedown(&tegra->mm);
-		put_iova_domain(&tegra->carveout.domain);
-		iova_cache_put();
-	}
-
-	host1x_device_exit(device);
-fbdev:
-	drm_kms_helper_poll_fini(drm);
-	tegra_drm_fb_free(drm);
-config:
-	drm_mode_config_cleanup(drm);
-domain:
-	if (tegra->domain)
-		iommu_domain_free(tegra->domain);
-free:
-	kfree(tegra);
-	return err;
-}
-
-static void tegra_drm_unload(struct drm_device *drm)
-{
-	struct host1x_device *device = to_host1x_device(drm->dev);
-	struct tegra_drm *tegra = drm->dev_private;
-	int err;
-
-	drm_kms_helper_poll_fini(drm);
-	tegra_drm_fb_exit(drm);
-	drm_atomic_helper_shutdown(drm);
-	drm_mode_config_cleanup(drm);
-
-	err = host1x_device_exit(device);
-	if (err < 0)
-		return;
-
-	if (tegra->domain) {
-		mutex_destroy(&tegra->mm_lock);
-		drm_mm_takedown(&tegra->mm);
-		put_iova_domain(&tegra->carveout.domain);
-		iova_cache_put();
-		iommu_domain_free(tegra->domain);
-	}
-
-	kfree(tegra);
-}
-
 static int tegra_drm_open(struct drm_device *drm, struct drm_file *filp)
 {
 	struct tegra_drm_file *fpriv;
@@ -1046,8 +850,6 @@ static int tegra_debugfs_init(struct drm_minor *minor)
 static struct drm_driver tegra_drm_driver = {
 	.driver_features = DRIVER_MODESET | DRIVER_GEM |
 			   DRIVER_ATOMIC | DRIVER_RENDER,
-	.load = tegra_drm_load,
-	.unload = tegra_drm_unload,
 	.open = tegra_drm_open,
 	.postclose = tegra_drm_postclose,
 	.lastclose = drm_fb_helper_lastclose,
@@ -1231,6 +1033,8 @@ void tegra_drm_free(struct tegra_drm *tegra, size_t size, void *virt,
 static int host1x_drm_probe(struct host1x_device *dev)
 {
 	struct drm_driver *driver = &tegra_drm_driver;
+	struct iommu_domain *domain;
+	struct tegra_drm *tegra;
 	struct drm_device *drm;
 	int err;
 
@@ -1238,18 +1042,180 @@ static int host1x_drm_probe(struct host1x_device *dev)
 	if (IS_ERR(drm))
 		return PTR_ERR(drm);
 
+	tegra = kzalloc(sizeof(*tegra), GFP_KERNEL);
+	if (!tegra) {
+		err = -ENOMEM;
+		goto put;
+	}
+
+	/*
+	 * If the Tegra DRM clients are backed by an IOMMU, push buffers are
+	 * likely to be allocated beyond the 32-bit boundary if sufficient
+	 * system memory is available. This is problematic on earlier Tegra
+	 * generations where host1x supports a maximum of 32 address bits in
+	 * the GATHER opcode. In this case, unless host1x is behind an IOMMU
+	 * as well it won't be able to process buffers allocated beyond the
+	 * 32-bit boundary.
+	 *
+	 * The DMA API will use bounce buffers in this case, so that could
+	 * perhaps still be made to work, even if less efficient, but there
+	 * is another catch: in order to perform cache maintenance on pages
+	 * allocated for discontiguous buffers we need to map and unmap the
+	 * SG table representing these buffers. This is fine for something
+	 * small like a push buffer, but it exhausts the bounce buffer pool
+	 * (typically on the order of a few MiB) for framebuffers (many MiB
+	 * for any modern resolution).
+	 *
+	 * Work around this by making sure that Tegra DRM clients only use
+	 * an IOMMU if the parent host1x also uses an IOMMU.
+	 *
+	 * Note that there's still a small gap here that we don't cover: if
+	 * the DMA API is backed by an IOMMU there's no way to control which
+	 * device is attached to an IOMMU and which isn't, except via wiring
+	 * up the device tree appropriately. This is considered an problem
+	 * of integration, so care must be taken for the DT to be consistent.
+	 */
+	domain = iommu_get_domain_for_dev(drm->dev->parent);
+
+	if (domain && iommu_present(&platform_bus_type)) {
+		tegra->domain = iommu_domain_alloc(&platform_bus_type);
+		if (!tegra->domain) {
+			err = -ENOMEM;
+			goto free;
+		}
+
+		err = iova_cache_get();
+		if (err < 0)
+			goto domain;
+	}
+
+	mutex_init(&tegra->clients_lock);
+	INIT_LIST_HEAD(&tegra->clients);
+
 	dev_set_drvdata(&dev->dev, drm);
+	drm->dev_private = tegra;
+	tegra->drm = drm;
+
+	drm_mode_config_init(drm);
+
+	drm->mode_config.min_width = 0;
+	drm->mode_config.min_height = 0;
+
+	drm->mode_config.max_width = 4096;
+	drm->mode_config.max_height = 4096;
+
+	drm->mode_config.allow_fb_modifiers = true;
+
+	drm->mode_config.normalize_zpos = true;
+
+	drm->mode_config.funcs = &tegra_drm_mode_config_funcs;
+	drm->mode_config.helper_private = &tegra_drm_mode_config_helpers;
 
-	err = drm_fb_helper_remove_conflicting_framebuffers(NULL, "tegradrmfb", false);
+	err = tegra_drm_fb_prepare(drm);
 	if (err < 0)
-		goto put;
+		goto config;
+
+	drm_kms_helper_poll_init(drm);
+
+	err = host1x_device_init(dev);
+	if (err < 0)
+		goto fbdev;
+
+	if (tegra->group) {
+		u64 carveout_start, carveout_end, gem_start, gem_end;
+		u64 dma_mask = dma_get_mask(&dev->dev);
+		dma_addr_t start, end;
+		unsigned long order;
+
+		start = tegra->domain->geometry.aperture_start & dma_mask;
+		end = tegra->domain->geometry.aperture_end & dma_mask;
+
+		gem_start = start;
+		gem_end = end - CARVEOUT_SZ;
+		carveout_start = gem_end + 1;
+		carveout_end = end;
+
+		order = __ffs(tegra->domain->pgsize_bitmap);
+		init_iova_domain(&tegra->carveout.domain, 1UL << order,
+				 carveout_start >> order);
+
+		tegra->carveout.shift = iova_shift(&tegra->carveout.domain);
+		tegra->carveout.limit = carveout_end >> tegra->carveout.shift;
+
+		drm_mm_init(&tegra->mm, gem_start, gem_end - gem_start + 1);
+		mutex_init(&tegra->mm_lock);
+
+		DRM_DEBUG_DRIVER("IOMMU apertures:\n");
+		DRM_DEBUG_DRIVER("  GEM: %#llx-%#llx\n", gem_start, gem_end);
+		DRM_DEBUG_DRIVER("  Carveout: %#llx-%#llx\n", carveout_start,
+				 carveout_end);
+	} else if (tegra->domain) {
+		iommu_domain_free(tegra->domain);
+		tegra->domain = NULL;
+		iova_cache_put();
+	}
+
+	if (tegra->hub) {
+		err = tegra_display_hub_prepare(tegra->hub);
+		if (err < 0)
+			goto device;
+	}
+
+	/*
+	 * We don't use the drm_irq_install() helpers provided by the DRM
+	 * core, so we need to set this manually in order to allow the
+	 * DRM_IOCTL_WAIT_VBLANK to operate correctly.
+	 */
+	drm->irq_enabled = true;
+
+	/* syncpoints are used for full 32-bit hardware VBLANK counters */
+	drm->max_vblank_count = 0xffffffff;
+
+	err = drm_vblank_init(drm, drm->mode_config.num_crtc);
+	if (err < 0)
+		goto hub;
+
+	drm_mode_config_reset(drm);
+
+	err = drm_fb_helper_remove_conflicting_framebuffers(NULL, "tegradrmfb",
+							    false);
+	if (err < 0)
+		goto hub;
+
+	err = tegra_drm_fb_init(drm);
+	if (err < 0)
+		goto hub;
 
 	err = drm_dev_register(drm, 0);
 	if (err < 0)
-		goto put;
+		goto fb;
 
 	return 0;
 
+fb:
+	tegra_drm_fb_exit(drm);
+hub:
+	if (tegra->hub)
+		tegra_display_hub_cleanup(tegra->hub);
+device:
+	if (tegra->domain) {
+		mutex_destroy(&tegra->mm_lock);
+		drm_mm_takedown(&tegra->mm);
+		put_iova_domain(&tegra->carveout.domain);
+		iova_cache_put();
+	}
+
+	host1x_device_exit(dev);
+fbdev:
+	drm_kms_helper_poll_fini(drm);
+	tegra_drm_fb_free(drm);
+config:
+	drm_mode_config_cleanup(drm);
+domain:
+	if (tegra->domain)
+		iommu_domain_free(tegra->domain);
+free:
+	kfree(tegra);
 put:
 	drm_dev_put(drm);
 	return err;
@@ -1258,8 +1224,29 @@ static int host1x_drm_probe(struct host1x_device *dev)
 static int host1x_drm_remove(struct host1x_device *dev)
 {
 	struct drm_device *drm = dev_get_drvdata(&dev->dev);
+	struct tegra_drm *tegra = drm->dev_private;
+	int err;
 
 	drm_dev_unregister(drm);
+
+	drm_kms_helper_poll_fini(drm);
+	tegra_drm_fb_exit(drm);
+	drm_atomic_helper_shutdown(drm);
+	drm_mode_config_cleanup(drm);
+
+	err = host1x_device_exit(dev);
+	if (err < 0)
+		dev_err(&dev->dev, "host1x device cleanup failed: %d\n", err);
+
+	if (tegra->domain) {
+		mutex_destroy(&tegra->mm_lock);
+		drm_mm_takedown(&tegra->mm);
+		put_iova_domain(&tegra->carveout.domain);
+		iova_cache_put();
+		iommu_domain_free(tegra->domain);
+	}
+
+	kfree(tegra);
 	drm_dev_put(drm);
 
 	return 0;
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/tegra: Do not use ->load() and ->unload() callbacks

[Michał Mirosław]

On Thu, Oct 24, 2019 at 07:31:37PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The ->load() and ->unload() drivers are midlayers and should be avoided
> in modern drivers. Fix this by moving the code into the driver ->probe()
> and ->remove() implementations, respectively.
> 
> v2: kick out conflicting framebuffers before initializing fbdev
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Michał, Dmitry,
> 
> do you guys have a way of testing that the removal of the conflicting
> framebuffer actually works?
[...]

I might be able to check during the weekend. Is this patch alone enough
for v5.3?

Best Regards,
Michał Mirosław
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/tegra: Do not use ->load() and ->unload() callbacks

[Dmitry Osipenko]

24.10.2019 21:15, Michał Mirosław пишет:
> On Thu, Oct 24, 2019 at 07:31:37PM +0200, Thierry Reding wrote:
>> From: Thierry Reding <treding@nvidia.com>
>>
>> The ->load() and ->unload() drivers are midlayers and should be avoided
>> in modern drivers. Fix this by moving the code into the driver ->probe()
>> and ->remove() implementations, respectively.
>>
>> v2: kick out conflicting framebuffers before initializing fbdev
>>
>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>> ---
>> Michał, Dmitry,
>>
>> do you guys have a way of testing that the removal of the conflicting
>> framebuffer actually works?
> [...]
> 
> I might be able to check during the weekend. Is this patch alone enough
> for v5.3?

I don't think it will apply cleanly on top of 5.3, but should work with
linux-next or by cherry-picking 9d5a54987265.

Regarding the testing, I never used simple-fb on Tegra before and it's
not working for me now. Hopefully it will work for you, Michał.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/tegra: Do not use ->load() and ->unload() callbacks

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2174 bytes --]

On Fri, Oct 25, 2019 at 12:10:49AM +0300, Dmitry Osipenko wrote:
> 24.10.2019 21:15, Michał Mirosław пишет:
> > On Thu, Oct 24, 2019 at 07:31:37PM +0200, Thierry Reding wrote:
> >> From: Thierry Reding <treding@nvidia.com>
> >>
> >> The ->load() and ->unload() drivers are midlayers and should be avoided
> >> in modern drivers. Fix this by moving the code into the driver ->probe()
> >> and ->remove() implementations, respectively.
> >>
> >> v2: kick out conflicting framebuffers before initializing fbdev
> >>
> >> Signed-off-by: Thierry Reding <treding@nvidia.com>
> >> ---
> >> Michał, Dmitry,
> >>
> >> do you guys have a way of testing that the removal of the conflicting
> >> framebuffer actually works?
> > [...]
> > 
> > I might be able to check during the weekend. Is this patch alone enough
> > for v5.3?
> 
> I don't think it will apply cleanly on top of 5.3, but should work with
> linux-next or by cherry-picking 9d5a54987265.

I just noticed that I based this version on top of a local branch that
will cause conflicts if you apply this to either 5.3 or linux-next. I'll
resend this later rebased onto drm/tegra/for-next, so it should apply
cleanly on at least linux-next. drm/tegra/for-next also contains this:

commit 051172e8c1ceef8749f19faacc1d3bef65d20d8d
Author: Thierry Reding <treding@nvidia.com>
Date:   Wed Sep 25 13:26:59 2019 +0200

    drm/tegra: Fix ordering of cleanup code

    Commit Fixes: b9f8b09ce256 ("drm/tegra: Setup shared IOMMU domain after
    initialization") changed the initialization order of the IOMMU related
    bits but didn't update the cleanup path accordingly. This asymmetry can
    cause failures during error recovery.

    Fixes: b9f8b09ce256 ("drm/tegra: Setup shared IOMMU domain after initialization")
    Signed-off-by: Thierry Reding <treding@nvidia.com>
    Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
    Tested-by: Dmitry Osipenko <digetx@gmail.com>

which will conflict with the ->load() and ->unload() removal patch.

If for some reason you can't use linux-next, it shouldn't be terribly
complicated to backport this to v5.3, though.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel