On Tue, Jun 21, 2016 at 09:53:46AM +0100, Chris Wilson wrote:
> Take control over allocating, loading and registering the driver from the
> DRM midlayer by performing it manually from i915_pci_probe. This allows
> us to carefully control the order of when we setup the hardware vs when
> it becomes visible to third parties (including userspace). The current
> ordering makes the driver visible to userspace first (in order to
> coordinate with removed DRI1 userspace), but that ordering incurs risk.
> The risk increases as we strive for more asynchronous loading.
>
> One side effect of controlling the allocation is that we can allocate
> both the drm_device + drm_i915_private in one block, the next step
> towards subclassing.
>
> Unload is still left as before, a mix of midlayer and driver.
>
> v2: After drm_dev_init(), we should call drm_dev_unref() so that we call
> drm_dev_release() and free everything from drm_dev_init().
>
> Suggested-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
A bit much s/dev/dev_priv/ conversion here for my taste, but meh. One real
issue spotted below.
> ---
> drivers/gpu/drm/i915/i915_dma.c | 76 ++++++++++++++++++++++++++---------------
> drivers/gpu/drm/i915/i915_drv.c | 3 +-
> drivers/gpu/drm/i915/i915_drv.h | 6 +++-
> 3 files changed, 54 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 91623874f9a3..0e43ad511833 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1075,9 +1075,10 @@ static void i915_workqueues_cleanup(struct drm_i915_private *dev_priv)
> * function hooks not requiring accessing the device.
> */
> static int i915_driver_init_early(struct drm_i915_private *dev_priv,
> - struct drm_device *dev,
> - struct intel_device_info *info)
> + const struct pci_device_id *ent)
> {
> + const struct intel_device_info *match_info =
> + (struct intel_device_info *)ent->driver_data;
> struct intel_device_info *device_info;
> int ret = 0;
>
> @@ -1086,8 +1087,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>
> /* Setup the write-once "constant" device info */
> device_info = (struct intel_device_info *)&dev_priv->info;
> - memcpy(device_info, info, sizeof(dev_priv->info));
> - device_info->device_id = dev->pdev->device;
> + memcpy(device_info, match_info, sizeof(*device_info));
> + device_info->device_id = dev_priv->drm.pdev->device;
>
> BUG_ON(device_info->gen > sizeof(device_info->gen_mask) * BITS_PER_BYTE);
> device_info->gen_mask = BIT(device_info->gen - 1);
> @@ -1113,18 +1114,18 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
> goto err_workqueues;
>
> /* This must be called before any calls to HAS_PCH_* */
> - intel_detect_pch(dev);
> + intel_detect_pch(&dev_priv->drm);
>
> - intel_pm_setup(dev);
> + intel_pm_setup(&dev_priv->drm);
> intel_init_dpio(dev_priv);
> intel_power_domains_init(dev_priv);
> intel_irq_init(dev_priv);
> intel_init_display_hooks(dev_priv);
> intel_init_clock_gating_hooks(dev_priv);
> intel_init_audio_hooks(dev_priv);
> - i915_gem_load_init(dev);
> + i915_gem_load_init(&dev_priv->drm);
>
> - intel_display_crc_init(dev);
> + intel_display_crc_init(&dev_priv->drm);
>
> i915_dump_device_info(dev_priv);
>
> @@ -1132,7 +1133,7 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
> * very first ones. Almost everything should work, except for maybe
> * suspend/resume. And we don't implement workarounds that affect only
> * pre-production machines. */
> - if (IS_HSW_EARLY_SDV(dev))
> + if (IS_HSW_EARLY_SDV(dev_priv))
> DRM_INFO("This is an early pre-production Haswell machine. "
> "It may not be fully functional.\n");
>
> @@ -1390,6 +1391,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
> struct drm_device *dev = dev_priv->dev;
>
> i915_gem_shrinker_init(dev_priv);
> +
> /*
> * Notify a valid surface after modesetting,
> * when running inside a VM.
> @@ -1397,9 +1399,13 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
> if (intel_vgpu_active(dev_priv))
> I915_WRITE(vgtif_reg(display_ready), VGT_DRV_DISPLAY_READY);
>
> - i915_debugfs_register(dev_priv);
> - i915_setup_sysfs(dev);
> - intel_modeset_register(dev_priv);
> + /* Reveal our presence to userspace */
> + if (drm_dev_register(dev, 0) == 0) {
> + i915_debugfs_register(dev_priv);
> + i915_setup_sysfs(dev);
> + intel_modeset_register(dev_priv);
> + } else
> + DRM_ERROR("Failed to register driver for userspace access!\n");
>
> if (INTEL_INFO(dev_priv)->num_pipes) {
> /* Must be done after probing outputs */
> @@ -1449,24 +1455,38 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
> * - allocate initial config memory
> * - setup the DRM framebuffer with the allocated memory
> */
> -int i915_driver_load(struct drm_device *dev, unsigned long flags)
> +int i915_driver_load(struct pci_dev *pdev,
> + const struct pci_device_id *ent,
> + struct drm_driver *driver)
> {
> struct drm_i915_private *dev_priv;
> - int ret = 0;
> + int ret;
>
> + ret = 0;
> dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL);
> - if (dev_priv == NULL)
> + if (dev_priv)
> + ret = drm_dev_init(&dev_priv->drm, driver, &pdev->dev);
> + if (ret) {
This ended up a bit too clever, will fail to spot the failure when
dev_priv == NULL. I guess you wanted a ret = -ENOMEM up there.
-Daniel
> + dev_printk(KERN_ERR, &pdev->dev,
> + "[" DRM_NAME ":%s] allocation failed\n", __func__);
> + kfree(dev_priv);
> return -ENOMEM;
> + }
>
> - dev->dev_private = dev_priv;
> /* Must be set before calling __i915_printk */
> - dev_priv->dev = dev;
> + dev_priv->drm.pdev = pdev;
> + dev_priv->drm.dev_private = dev_priv;
> + dev_priv->dev = &dev_priv->drm;
>
> - ret = i915_driver_init_early(dev_priv, dev,
> - (struct intel_device_info *)flags);
> + ret = pci_enable_device(pdev);
> + if (ret)
> + goto out_free_priv;
>
> + pci_set_drvdata(pdev, &dev_priv->drm);
> +
> + ret = i915_driver_init_early(dev_priv, ent);
> if (ret < 0)
> - goto out_free_priv;
> + goto out_pci_disable;
>
> intel_runtime_pm_get(dev_priv);
>
> @@ -1483,13 +1503,14 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> * of the i915_driver_init_/i915_driver_register functions according
> * to the role/effect of the given init step.
> */
> - if (INTEL_INFO(dev)->num_pipes) {
> - ret = drm_vblank_init(dev, INTEL_INFO(dev)->num_pipes);
> + if (INTEL_INFO(dev_priv)->num_pipes) {
> + ret = drm_vblank_init(dev_priv->dev,
> + INTEL_INFO(dev_priv)->num_pipes);
> if (ret)
> goto out_cleanup_hw;
> }
>
> - ret = i915_load_modeset_init(dev);
> + ret = i915_load_modeset_init(dev_priv->dev);
> if (ret < 0)
> goto out_cleanup_vblank;
>
> @@ -1502,7 +1523,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> return 0;
>
> out_cleanup_vblank:
> - drm_vblank_cleanup(dev);
> + drm_vblank_cleanup(dev_priv->dev);
> out_cleanup_hw:
> i915_driver_cleanup_hw(dev_priv);
> out_cleanup_mmio:
> @@ -1510,11 +1531,11 @@ out_cleanup_mmio:
> out_runtime_pm_put:
> intel_runtime_pm_put(dev_priv);
> i915_driver_cleanup_early(dev_priv);
> +out_pci_disable:
> + pci_disable_device(pdev);
> out_free_priv:
> i915_load_error(dev_priv, "Device initialization failed (%d)\n", ret);
> -
> - kfree(dev_priv);
> -
> + drm_dev_unref(&dev_priv->drm);
> return ret;
> }
>
> @@ -1579,7 +1600,6 @@ int i915_driver_unload(struct drm_device *dev)
> intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
>
> i915_driver_cleanup_early(dev_priv);
> - kfree(dev_priv);
>
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3ea09bd83a5a..1a335e1a8b62 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1034,7 +1034,7 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> if (vga_switcheroo_client_probe_defer(pdev))
> return -EPROBE_DEFER;
>
> - return drm_get_pci_dev(pdev, ent, &driver);
> + return i915_driver_load(pdev, ent, &driver);
> }
>
> static void
> @@ -1748,7 +1748,6 @@ static struct drm_driver driver = {
> .driver_features =
> DRIVER_HAVE_IRQ | DRIVER_IRQ_SHARED | DRIVER_GEM | DRIVER_PRIME |
> DRIVER_RENDER | DRIVER_MODESET,
> - .load = i915_driver_load,
> .unload = i915_driver_unload,
> .open = i915_driver_open,
> .lastclose = i915_driver_lastclose,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 44e6715d8b1d..20a82b6a050d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1732,6 +1732,8 @@ struct intel_wm_config {
> };
>
> struct drm_i915_private {
> + struct drm_device drm;
> +
> struct drm_device *dev;
> struct kmem_cache *objects;
> struct kmem_cache *vmas;
> @@ -2902,7 +2904,9 @@ __i915_printk(struct drm_i915_private *dev_priv, const char *level,
> #define i915_report_error(dev_priv, fmt, ...) \
> __i915_printk(dev_priv, KERN_ERR, fmt, ##__VA_ARGS__)
>
> -extern int i915_driver_load(struct drm_device *, unsigned long flags);
> +extern int i915_driver_load(struct pci_dev *pdev,
> + const struct pci_device_id *ent,
> + struct drm_driver *driver);
> extern int i915_driver_unload(struct drm_device *);
> extern int i915_driver_open(struct drm_device *dev, struct drm_file *file);
> extern void i915_driver_lastclose(struct drm_device * dev);
> --
> 2.8.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
