Re: [PATCH 1/3] drm/i915/hda: Add audio component stub

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Apr 07, 2016 at 12:57:22PM +0200, Takashi Iwai wrote:
> From: Imre Deak <imre.deak@intel.com>
> 
> User may pass nomodeset or i915.modeset=0 option to disable i915 KMS
> explicitly.  Although this itself works fine, it breaks the weak
> dependency the HD-audio driver requires, and it's the reason the
> delayed component binding isn't implemented in HD-audio.  Since i915
> doesn't notify its disablement, HD-audio would be blocked
> unnecessarily endlessly, waiting for the bind with i915.
> 
> This patch introduces a stub audio component binding when i915 driver
> is loaded with KMS off like the boot options above.  Then i915 driver
> still registers the slave component but with the new "disabled" ops
> flag, so that the master component (HD-audio) can know clearly the
> slave state.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  drivers/gpu/drm/i915/i915_drv.c    | 34 +++++++++++++--------
>  drivers/gpu/drm/i915/intel_audio.c | 61 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
>  include/drm/i915_component.h       |  5 ++++
>  4 files changed, 90 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 20e82008b8b6..21c6bac5468e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -950,6 +950,19 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	struct intel_device_info *intel_info =
>  		(struct intel_device_info *) ent->driver_data;
>  
> +	if (!(driver.driver_features & DRIVER_MODESET)) {
> +		/*
> +		 * Notify the HDA driver so that it doesn't block waiting for
> +		 * i915 to become ready.
> +		 */
> +		i915_audio_component_stub_init(&pdev->dev);
> +
> +		/* Silently fail loading to not upset userspace. */
> +		DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
> +
> +		return 0;
> +	}
> +

Moving this here means we're actually going to bind to the device even
w/o DRIVER_MODESET. Hopefully the fact that we don't register with any
subsystems would make that OK. But since we need the actual device for
the component stuff, I guess this is the only way.

>  	if (IS_PRELIMINARY_HW(intel_info) && !i915.preliminary_hw_support) {
>  		DRM_INFO("This hardware requires preliminary hardware support.\n"
>  			 "See CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT, and/or modparam preliminary_hw_support\n");
> @@ -979,8 +992,14 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  static void
>  i915_pci_remove(struct pci_dev *pdev)
>  {
> -	struct drm_device *dev = pci_get_drvdata(pdev);
> +	struct drm_device *dev;
>  
> +	if (!(driver.driver_features & DRIVER_MODESET)) {
> +		i915_audio_component_stub_cleanup(&pdev->dev);
> +		return;
> +	}
> +
> +	dev = pci_get_drvdata(pdev);
>  	drm_put_dev(dev);
>  }
>  
> @@ -1747,24 +1766,15 @@ static int __init i915_init(void)
>  		driver.driver_features &= ~DRIVER_MODESET;
>  #endif
>  
> -	if (!(driver.driver_features & DRIVER_MODESET)) {
> -		/* Silently fail loading to not upset userspace. */
> -		DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
> -		return 0;
> -	}
> -
>  	if (i915.nuclear_pageflip)
>  		driver.driver_features |= DRIVER_ATOMIC;
>  
> -	return drm_pci_init(&driver, &i915_pci_driver);
> +	return pci_register_driver(&i915_pci_driver);
>  }
>  
>  static void __exit i915_exit(void)
>  {
> -	if (!(driver.driver_features & DRIVER_MODESET))
> -		return; /* Never loaded a driver. */
> -
> -	drm_pci_exit(&driver, &i915_pci_driver);
> +	pci_unregister_driver(&i915_pci_driver);
>  }
>  
>  module_init(i915_init);
> diff --git a/drivers/gpu/drm/i915/intel_audio.c b/drivers/gpu/drm/i915/intel_audio.c
> index 30f921421b0c..bd9e9760903a 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -835,3 +835,64 @@ void i915_audio_component_cleanup(struct drm_i915_private *dev_priv)
>  	component_del(dev_priv->dev->dev, &i915_audio_component_bind_ops);
>  	dev_priv->audio_component_registered = false;
>  }
> +
> +static const struct i915_audio_component_ops i915_audio_component_stub_ops = {
> +	.owner		= THIS_MODULE,
> +	.disabled	= true,
> +};
> +
> +static int i915_audio_component_stub_bind(struct device *i915_stub_dev,
> +					  struct device *hda_dev, void *data)
> +{
> +	struct i915_audio_component *acomp = data;
> +
> +	if (WARN_ON(acomp->ops || acomp->dev))
> +		return -EEXIST;
> +
> +	acomp->ops = &i915_audio_component_stub_ops;
> +	acomp->dev = i915_stub_dev;
> +
> +	return 0;
> +}
> +
> +static void i915_audio_component_stub_unbind(struct device *i915_stub_dev,
> +					     struct device *hda_dev, void *data)
> +{
> +	struct i915_audio_component *acomp = data;
> +
> +	acomp->ops = NULL;
> +	acomp->dev = NULL;
> +}
> +
> +static const struct component_ops i915_audio_component_stub_bind_ops = {
> +	.bind	= i915_audio_component_stub_bind,
> +	.unbind	= i915_audio_component_stub_unbind,
> +};
> +
> +static const struct file_operations component_stub_dev_ops = {
> +	.owner = THIS_MODULE,
> +};
> +
> +static bool i915_audio_component_stub_registered;
> +
> +void i915_audio_component_stub_init(struct device *dev)
> +{
> +	int ret;
> +
> +	ret = component_add(dev, &i915_audio_component_stub_bind_ops);
> +	if (ret < 0) {
> +		DRM_ERROR("failed to add audio stub component (%d)\n", ret);
> +		return;
> +	}
> +
> +	i915_audio_component_stub_registered = true;

Maybe just fail the probe on error and avoid the flag?

> +}
> +
> +void i915_audio_component_stub_cleanup(struct device *dev)
> +{
> +	if (!i915_audio_component_stub_registered)
> +		return;
> +
> +	component_del(dev, &i915_audio_component_stub_bind_ops);
> +	i915_audio_component_stub_registered = false;
> +}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4c027d69fac9..e1d9c33b113e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1056,6 +1056,8 @@ void intel_audio_codec_enable(struct intel_encoder *encoder);
>  void intel_audio_codec_disable(struct intel_encoder *encoder);
>  void i915_audio_component_init(struct drm_i915_private *dev_priv);
>  void i915_audio_component_cleanup(struct drm_i915_private *dev_priv);
> +void i915_audio_component_stub_init(struct device *dev);
> +void i915_audio_component_stub_cleanup(struct device *dev);
>  
>  /* intel_display.c */
>  extern const struct drm_plane_funcs intel_plane_funcs;
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> index b46fa0ef3005..33da85aa70c3 100644
> --- a/include/drm/i915_component.h
> +++ b/include/drm/i915_component.h
> @@ -39,6 +39,11 @@ struct i915_audio_component_ops {
>  	 */
>  	struct module *owner;
>  	/**
> +	 * @disabled: i915 driver loaded with modeset=0, the services provided
> +	 * via the audio component interface are not available.
> +	 */
> +	bool disabled;
> +	/**
>  	 * @get_power: get the POWER_DOMAIN_AUDIO power well
>  	 *
>  	 * Request the power well to be turned on.
> -- 
> 2.8.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx