Re: [PATCH 6/8] drm/i915: Demidlayer driver loading

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux