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

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Oct 28, 2019 at 01:16:10PM +0100, 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
> v3: rebase onto drm/tegra/for-next
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>

My for v2 seems to have been lost, so:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/tegra/drm.c | 321 +++++++++++++++++-------------------
>  1 file changed, 154 insertions(+), 167 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index b74362cb63eb..7480f575188d 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -86,168 +86,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 tegra_drm *tegra;
> -	int err;
> -
> -	tegra = kzalloc(sizeof(*tegra), GFP_KERNEL);
> -	if (!tegra)
> -		return -ENOMEM;
> -
> -	if (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->domain) {
> -		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);
> -	}
> -
> -	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;
> @@ -1014,8 +852,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,
> @@ -1202,6 +1038,7 @@ 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 tegra_drm *tegra;
>  	struct drm_device *drm;
>  	int err;
>  
> @@ -1209,18 +1046,147 @@ 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 (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;
>  
> -	err = drm_fb_helper_remove_conflicting_framebuffers(NULL, "tegradrmfb", false);
> +	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 put;
> +		goto config;
> +
> +	drm_kms_helper_poll_init(drm);
> +
> +	err = host1x_device_init(dev);
> +	if (err < 0)
> +		goto fbdev;
> +
> +	if (tegra->domain) {
> +		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);
> +	}
> +
> +	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;
> @@ -1229,8 +1195,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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel