On Thu, Oct 24, 2019 at 06:07:54PM +0200, Daniel Vetter wrote:
> On Thu, Oct 24, 2019 at 05:10:30PM +0200, Thierry Reding wrote:
> > From: Thierry Reding <treding@xxxxxxxxxx>
> >
> > 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.
> >
> > Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> > ---
> > drivers/gpu/drm/tegra/drm.c | 386 +++++++++++++++++-------------------
> > 1 file changed, 186 insertions(+), 200 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > index 3012f13bab97..bd7a00272965 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,179 @@ 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 = tegra_drm_fb_prepare(drm);
> > + if (err < 0)
> > + 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 = tegra_drm_fb_init(drm);
> > + if (err < 0)
> > + goto hub;
> >
> > err = drm_fb_helper_remove_conflicting_framebuffers(NULL, "tegradrmfb", false);
>
> I think you want to do this before you set up the drmfb. Well probably you
> want to do this as the one of the very first things in your probe code,
> before you start touching any of the hw. At least that's what the old
> order did.
Hm... you're right. I had actually wondered about this and then
concluded that it might be best to only remove the conflicting
framebuffers when it was relatively certain that the driver could
correctly create a new DRM framebuffer.
But yeah, it definitely needs to kick out the conflicting framebuffer
before the call to tegra_drm_fb_init().
I'll Cc Michał on the next version, maybe he's got a way to actually
test this. I don't have access to any hardware where the firmware
installs a framebuffer.
Thierry
>
> > if (err < 0)
> > - goto put;
> > + goto fb;
> >
> > 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 +1223,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;
>
> Aside from the one question:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>
> > --
> > 2.23.0
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Attachment:
signature.asc
Description: PGP signature
