[PATCH 0/3] More robust i915 / audio binding

[PATCH 0/3] More robust i915 / audio binding

[Takashi Iwai]

Hi,

here is a patchset to make the i915 / audio binding more robustly.
It's based on Imre's work to implement the stub component ops for
notifying the i915 disablement.


Takashi

===

Imre Deak (1):
  drm/i915/hda: Add audio component stub

Takashi Iwai (2):
  ALSA: hda - Immediately fail if the i915 audio component is disabled
  ALSA: hda - Support deferred i915 audio component binding

 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 ++++
 sound/hda/hdac_i915.c              | 28 ++++++++++++-----
 5 files changed, 110 insertions(+), 20 deletions(-)

-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

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;
+	}
+
 	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;
+}
+
+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

[PATCH 2/3] ALSA: hda - Immediately fail if the i915 audio component is disabled

[Takashi Iwai]

Since i915 notifies its disabled state via the stub component binding,
we can bail out immediately once when the disabled flag is detected.

Based on the original patch by Imre Deak <imre.deak@intel.com>

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/hda/hdac_i915.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 6800e0c5a38f..812777f9af1f 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -223,8 +223,18 @@ static int hdac_component_master_bind(struct device *dev)
 	if (ret < 0)
 		return ret;
 
-	if (WARN_ON(!(acomp->dev && acomp->ops && acomp->ops->get_power &&
-		      acomp->ops->put_power && acomp->ops->get_cdclk_freq))) {
+	if (WARN_ON(!(acomp->dev && acomp->ops))) {
+		ret = -EINVAL;
+		goto out_unbind;
+	}
+
+	if (acomp->ops->disabled) {
+		ret = -ENODEV;
+		goto out_unbind;
+	}
+
+	if (WARN_ON(!(acomp->ops->get_power && acomp->ops->put_power &&
+		      acomp->ops->get_cdclk_freq))) {
 		ret = -EINVAL;
 		goto out_unbind;
 	}
@@ -342,7 +352,7 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
 	 */
 	request_module("i915");
 
-	if (!acomp->ops) {
+	if (!acomp->ops || acomp->ops->disabled) {
 		ret = -ENODEV;
 		goto out_master_del;
 	}
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] ALSA: hda - Support deferred i915 audio component binding

[Takashi Iwai]

Since we have the reliable way to detect the availability of i915
audio component, the HD-audio driver may wait for the binding safely.

This patch puts a simple completion call in the binding callback and
let snd_hdac_i915_init() waiting until the i915 component gets bound
or it notifies.

The wait for binding has a timeout of 60 seconds, in case where the
i915 module isn't present on the media by some reason.  Such a
situation is rather the system error, but it's still safe to get a
timeout and indicate user about that.

The explicit call of request_module() is still kept in this patch in
order to avoid regression even for a system without the implicit
driver loading.  But now it's called in the condition only when no ops
is found, so it's slightly optimized.

The pin-down of i915 module is still kept, too, as the dynamic
unbinding is still problematic.  It may be racy and lead to the
refcount unbalance.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/hda/hdac_i915.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index 812777f9af1f..6455d239a928 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -22,6 +22,7 @@
 #include <sound/hda_i915.h>
 
 static struct i915_audio_component *hdac_acomp;
+static DECLARE_COMPLETION(bind_comp);
 
 /**
  * snd_hdac_set_codec_wakeup - Enable / disable HDMI/DP codec wakeup
@@ -248,10 +249,12 @@ static int hdac_component_master_bind(struct device *dev)
 		goto out_unbind;
 	}
 
+	complete_all(&bind_comp);
 	return 0;
 
 out_unbind:
 	component_unbind_all(dev, acomp);
+	complete_all(&bind_comp);
 
 	return ret;
 }
@@ -339,6 +342,7 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
 		return -ENOMEM;
 	bus->audio_component = acomp;
 	hdac_acomp = acomp;
+	init_completion(&bind_comp);
 
 	component_match_add(dev, &match, hdac_component_master_match, bus);
 	ret = component_master_add_with_match(dev, &hdac_component_master_ops,
@@ -346,12 +350,10 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
 	if (ret < 0)
 		goto out_err;
 
-	/*
-	 * Atm, we don't support deferring the component binding, so make sure
-	 * i915 is loaded and that the binding successfully completes.
-	 */
-	request_module("i915");
+	if (!acomp->ops)
+		request_module("i915");
 
+	wait_for_completion_timeout(&bind_comp, msecs_to_jiffies(60 * 1000));
 	if (!acomp->ops || acomp->ops->disabled) {
 		ret = -ENODEV;
 		goto out_master_del;
-- 
2.8.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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

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

[Imre Deak]

On to, 2016-04-07 at 14:55 +0300, Ville Syrjälä wrote:
> 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.

Yes, the need for the device was the reason I moved this here. An
alternative would be to add a miscdevice. I wanted to keep things
simple and didn't see any problem of keeping the device bound, so chose
this way.

One other thing I was thinking about is to also bind the stub component
if the real probing failed to make things more consistent and avoid the
longish timeout on the HDA side. This would also get messier with the
miscdevice approach, since the probe error is not propagated to the
caller of pci_register_driver(). If people don't see a problem with
this I could add this in the next version.

> >  	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?

Makes sense, will change that.

> > +}
> > +
> > +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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for More robust i915 / audio binding

[Patchwork]

== Series Details ==

Series: More robust i915 / audio binding
URL   : https://patchwork.freedesktop.org/series/5412/
State : failure

== Summary ==

Series 5412v1 More robust i915 / audio binding
http://patchwork.freedesktop.org/api/1.0/series/5412/revisions/1/mbox/

Test core_auth:
        Subgroup basic-auth:
                pass       -> INCOMPLETE (ilk-hp8440p)
Test drv_module_reload_basic:
                pass       -> DMESG-WARN (bsw-nuc-2)
                pass       -> INCOMPLETE (hsw-gt2)
Test gem_ctx_create:
        Subgroup basic:
                skip       -> INCOMPLETE (ilk-hp8440p)
Test gem_ctx_param_basic:
        Subgroup basic:
                skip       -> INCOMPLETE (ilk-hp8440p)
        Subgroup invalid-param-get:
                pass       -> INCOMPLETE (bsw-nuc-2)
        Subgroup invalid-size-set:
                skip       -> INCOMPLETE (ilk-hp8440p)
        Subgroup root-set:
                skip       -> INCOMPLETE (ilk-hp8440p)
        Subgroup root-set-no-zeromap-enabled:
                skip       -> INCOMPLETE (ilk-hp8440p)
Test gem_exec_basic:
        Subgroup basic-blt:
                skip       -> INCOMPLETE (ilk-hp8440p)
        Subgroup basic-vebox:
                pass       -> SKIP       (bsw-nuc-2)
        Subgroup gtt-blt:
                pass       -> SKIP       (bsw-nuc-2)
        Subgroup gtt-render:
                pass       -> INCOMPLETE (ilk-hp8440p)
        Subgroup gtt-vebox:
                skip       -> INCOMPLETE (ivb-t430s)
        Subgroup readonly-bsd2:
                skip       -> INCOMPLETE (bsw-nuc-2)
Test gem_exec_parse:
        Subgroup basic-rejected:
                pass       -> INCOMPLETE (ivb-t430s)
Test gem_exec_store:
        Subgroup basic-all:
                pass       -> DMESG-WARN (bsw-nuc-2)
                pass       -> INCOMPLETE (ilk-hp8440p)
        Subgroup basic-blt:
                pass       -> INCOMPLETE (bsw-nuc-2)
        Subgroup basic-bsd:
                pass       -> INCOMPLETE (bsw-nuc-2)
                pass       -> INCOMPLETE (ivb-t430s)
        Subgroup basic-bsd1:
                skip       -> INCOMPLETE (ilk-hp8440p)
Test gem_exec_whisper:
        Subgroup basic:
                pass       -> INCOMPLETE (snb-x220t)
                pass       -> DMESG-FAIL (bsw-nuc-2)
Test gem_flink_basic:
        Subgroup bad-open:
                pass       -> INCOMPLETE (ivb-t430s)
        Subgroup double-flink:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test gem_mmap:
        Subgroup basic:
                pass       -> INCOMPLETE (ilk-hp8440p)
                pass       -> INCOMPLETE (ivb-t430s)
        Subgroup basic-small-bo:
                pass       -> INCOMPLETE (ilk-hp8440p)
Test gem_mmap_gtt:
        Subgroup basic-read:
                pass       -> INCOMPLETE (bsw-nuc-2)
        Subgroup basic-read-write-distinct:
                pass       -> INCOMPLETE (bsw-nuc-2)
        Subgroup basic-small-bo-tiledx:
                pass       -> INCOMPLETE (bsw-nuc-2)
        Subgroup basic-small-bo-tiledy:
                pass       -> INCOMPLETE (bsw-nuc-2)
        Subgroup basic-small-copy:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic-small-copy-xy:
                pass       -> INCOMPLETE (bsw-nuc-2)
        Subgroup basic-write-gtt-no-prefault:
                pass       -> INCOMPLETE (ivb-t430s)
        Subgroup basic-write-read:
                pass       -> INCOMPLETE (ivb-t430s)
        Subgroup basic-write-read-distinct:
                pass       -> INCOMPLETE (bsw-nuc-2)
Test gem_render_tiled_blits:
        Subgroup basic:
                pass       -> INCOMPLETE (bsw-nuc-2)
Test gem_ringfill:
        Subgroup basic-default-hang:
                pass       -> INCOMPLETE (ilk-hp8440p)
Test gem_storedw_loop:
        Subgroup basic-bsd:
                skip       -> INCOMPLETE (ilk-hp8440p)
                pass       -> INCOMPLETE (ivb-t430s)
Test gem_sync:
        Subgroup basic-bsd:
                pass       -> INCOMPLETE (bsw-nuc-2)
        Subgroup basic-each:
                dmesg-fail -> PASS       (bsw-nuc-2)
                pass       -> INCOMPLETE (ivb-t430s)
        Subgroup basic-render:
                pass       -> INCOMPLETE (ivb-t430s)
Test gem_tiled_fence_blits:
        Subgroup basic:
                pass       -> INCOMPLETE (ilk-hp8440p)
Test gem_tiled_pread_basic:
                pass       -> INCOMPLETE (ivb-t430s)
Test kms_addfb_basic:
        Subgroup addfb25-bad-modifier:
                pass       -> INCOMPLETE (ivb-t430s)
        Subgroup addfb25-framebuffer-vs-set-tiling:
                pass       -> INCOMPLETE (ivb-t430s)
        Subgroup addfb25-modifier-no-flag:
                pass       -> INCOMPLETE (ilk-hp8440p)
        Subgroup addfb25-x-tiled-mismatch:
                pass       -> INCOMPLETE (ilk-hp8440p)
        Subgroup bad-pitch-63:
                pass       -> INCOMPLETE (bsw-nuc-2)
        Subgroup bad-pitch-999:
                pass       -> INCOMPLETE (bsw-nuc-2)
        Subgroup bo-too-small:
                pass       -> INCOMPLETE (ivb-t430s)
        Subgroup clobberred-modifier:
                pass       -> INCOMPLETE (bsw-nuc-2)
        Subgroup framebuffer-vs-set-tiling:
                pass       -> INCOMPLETE (bsw-nuc-2)
        Subgroup no-handle:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup small-bo:
                pass       -> INCOMPLETE (ilk-hp8440p)
        Subgroup tile-pitch-mismatch:
                pass       -> INCOMPLETE (ilk-hp8440p)
        Subgroup unused-handle:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup unused-offsets:
                pass       -> INCOMPLETE (ilk-hp8440p)
        Subgroup unused-pitches:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-FAIL (bsw-nuc-2)
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-modeset:
                pass       -> INCOMPLETE (bsw-nuc-2)
                pass       -> INCOMPLETE (ilk-hp8440p) UNSTABLE
Test kms_force_connector_basic:
        Subgroup force-connector-state:
                skip       -> INCOMPLETE (bsw-nuc-2)
        Subgroup force-edid:
                skip       -> PASS       (ilk-hp8440p)
        Subgroup prune-stale-modes:
                skip       -> PASS       (ilk-hp8440p)
Test kms_frontbuffer_tracking:
        Subgroup basic:
                dmesg-fail -> PASS       (bsw-nuc-2)
                skip       -> INCOMPLETE (ilk-hp8440p)
                pass       -> INCOMPLETE (ivb-t430s)
Test kms_pipe_crc_basic:
        Subgroup bad-nb-words-1:
                pass       -> INCOMPLETE (ilk-hp8440p)
        Subgroup bad-pipe:
                pass       -> INCOMPLETE (ilk-hp8440p)
        Subgroup hang-read-crc-pipe-a:
                pass       -> INCOMPLETE (ilk-hp8440p)
        Subgroup hang-read-crc-pipe-b:
                skip       -> INCOMPLETE (bsw-nuc-2)
        Subgroup hang-read-crc-pipe-c:
                pass       -> INCOMPLETE (bsw-nuc-2)
        Subgroup nonblocking-crc-pipe-a:
                pass       -> INCOMPLETE (ilk-hp8440p)
        Subgroup nonblocking-crc-pipe-b:
                pass       -> SKIP       (hsw-brixbox)
        Subgroup nonblocking-crc-pipe-c:
                pass       -> INCOMPLETE (ivb-t430s)
        Subgroup nonblocking-crc-pipe-c-frame-sequence:
                pass       -> TIMEOUT    (hsw-gt2)
        Subgroup read-crc-pipe-b:
                pass       -> INCOMPLETE (ilk-hp8440p)
        Subgroup read-crc-pipe-b-frame-sequence:
                skip       -> INCOMPLETE (bsw-nuc-2)
        Subgroup read-crc-pipe-c:
                skip       -> INCOMPLETE (ilk-hp8440p)
        Subgroup read-crc-pipe-c-frame-sequence:
                pass       -> INCOMPLETE (bsw-nuc-2)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                skip       -> INCOMPLETE (ivb-t430s)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic-rte:
                pass       -> INCOMPLETE (bsw-nuc-2)
Test pm_rps:
        Subgroup basic-api:
                skip       -> INCOMPLETE (ilk-hp8440p)
Test prime_self_import:
        Subgroup basic-with_two_bos:
                pass       -> INCOMPLETE (ilk-hp8440p)

bdw-nuci7        total:196  pass:184  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:196  pass:175  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:196  pass:128  dwarn:8   dfail:2   fail:0   skip:35 
byt-nuc          total:196  pass:161  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:196  pass:173  dwarn:0   dfail:0   fail:0   skip:23 
hsw-gt2          total:93   pass:84   dwarn:0   dfail:0   fail:0   skip:7  
ilk-hp8440p      total:167  pass:98   dwarn:1   dfail:0   fail:0   skip:38 
ivb-t430s        total:196  pass:156  dwarn:0   dfail:0   fail:0   skip:23 
skl-i7k-2        total:196  pass:173  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:196  pass:185  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:196  pass:162  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:171  pass:143  dwarn:0   dfail:0   fail:1   skip:26 

Results at /archive/results/CI_IGT_test/Patchwork_1828/

72a18a7b81a4d12966eaf1c362c89a88ce6e5f54 drm-intel-nightly: 2016y-04m-07d-11h-51m-13s UTC integration manifest
b9ac0987ba6040659d3a0540f8ece38bf22e308b ALSA: hda - Support deferred i915 audio component binding
5347c8787cbd72b2093732418bbf6afc4d84bef6 ALSA: hda - Immediately fail if the i915 audio component is disabled
5e27579dfab2cdde6755f78726707780a19a7a6f drm/i915/hda: Add audio component stub

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Thu, 07 Apr 2016 14:30:45 +0200,
Imre Deak wrote:
> 
> On to, 2016-04-07 at 14:55 +0300, Ville Syrjälä wrote:
> > 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.
> 
> Yes, the need for the device was the reason I moved this here. An
> alternative would be to add a miscdevice. I wanted to keep things
> simple and didn't see any problem of keeping the device bound, so chose
> this way.
> 
> One other thing I was thinking about is to also bind the stub component
> if the real probing failed to make things more consistent and avoid the
> longish timeout on the HDA side. This would also get messier with the
> miscdevice approach, since the probe error is not propagated to the
> caller of pci_register_driver(). If people don't see a problem with
> this I could add this in the next version.

Is a newer version already available?


thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

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.

v2:
- Fail the probe in case component registration fails, instead of
  suppressing the error. (Ville)
- Register the component only for the real PCI device function.

CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/i915/i915_drv.c    | 51 +++++++++++++++++++++++-------------
 drivers/gpu/drm/i915/intel_audio.c | 53 ++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h   |  2 ++
 include/drm/i915_component.h       |  5 ++++
 4 files changed, 93 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5ae7960..b127810 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1016,12 +1016,6 @@ 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 (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");
-		return -ENODEV;
-	}
-
 	/* Only bind to function 0 of the device. Early generations
 	 * used function 1 as a placeholder for multi-head. This causes
 	 * us confusion instead, especially on the systems where both
@@ -1030,6 +1024,29 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (PCI_FUNC(pdev->devfn))
 		return -ENODEV;
 
+	if (!(driver.driver_features & DRIVER_MODESET)) {
+		int ret;
+
+		/*
+		 * Notify the HDA driver so that it doesn't block waiting for
+		 * i915 to become ready.
+		 */
+		ret = i915_audio_component_stub_init(&pdev->dev);
+		if (ret)
+			return ret;
+
+		/* Silently fail loading to not upset userspace. */
+		DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
+
+		return 0;
+	}
+
+	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");
+		return -ENODEV;
+	}
+
 	/*
 	 * apple-gmux is needed on dual GPU MacBook Pro
 	 * to probe the panel if we're the inactive GPU.
@@ -1045,8 +1062,15 @@ 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);
 }
 
@@ -1767,24 +1791,15 @@ static int __init i915_init(void)
 	if (vgacon_text_force() && i915.modeset == -1)
 		driver.driver_features &= ~DRIVER_MODESET;
 
-	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 b9329c2..603b3fe 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -824,3 +824,56 @@ 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,
+};
+
+int 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 ret;
+}
+
+void i915_audio_component_stub_cleanup(struct device *dev)
+{
+	component_del(dev, &i915_audio_component_stub_bind_ops);
+}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0a9c10d..a828e00 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1111,6 +1111,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);
+int i915_audio_component_stub_init(struct device *dev);
+void i915_audio_component_stub_cleanup(struct device *dev);
 
 /* intel_display.c */
 void intel_update_rawclk(struct drm_i915_private *dev_priv);
diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
index b46fa0e..33da85a 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.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for More robust i915 / audio binding (rev2)

[Patchwork]

== Series Details ==

Series: More robust i915 / audio binding (rev2)
URL   : https://patchwork.freedesktop.org/series/5412/
State : failure

== Summary ==

Series 5412v2 More robust i915 / audio binding
http://patchwork.freedesktop.org/api/1.0/series/5412/revisions/2/mbox

Test drv_hangman:
        Subgroup error-state-basic:
                fail       -> PASS       (ro-ilk1-i5-650)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-cmd:
                pass       -> FAIL       (ro-byt-n2820)
Test gem_exec_whisper:
        Subgroup basic:
                pass       -> INCOMPLETE (ro-snb-i7-2620M)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> INCOMPLETE (ro-ivb-i7-3770)
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (ro-hsw-i7-4770r)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-c:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
        Subgroup nonblocking-crc-pipe-c:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
        Subgroup nonblocking-crc-pipe-c-frame-sequence:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
        Subgroup read-crc-pipe-c:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
        Subgroup read-crc-pipe-c-frame-sequence:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                skip       -> INCOMPLETE (ro-hsw-i3-4010u)
Test kms_setmode:
        Subgroup basic-clone-single-crtc:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
Test pm_backlight:
        Subgroup basic-brightness:
                skip       -> INCOMPLETE (ro-hsw-i3-4010u)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
        Subgroup basic-rte:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
Test pm_rps:
        Subgroup basic-api:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
Test prime_self_import:
        Subgroup basic-llseek-bad:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
        Subgroup basic-llseek-size:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
        Subgroup basic-with_fd_dup:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
        Subgroup basic-with_one_bo:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
        Subgroup basic-with_one_bo_two_files:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)
        Subgroup basic-with_two_bos:
                pass       -> INCOMPLETE (ro-hsw-i3-4010u)

ro-bdw-i5-5250u  total:219  pass:181  dwarn:0   dfail:0   fail:0   skip:38 
ro-bdw-i7-5557U  total:219  pass:206  dwarn:0   dfail:0   fail:0   skip:13 
ro-bdw-i7-5600u  total:219  pass:187  dwarn:0   dfail:0   fail:0   skip:32 
ro-bsw-n3050     total:183  pass:152  dwarn:0   dfail:0   fail:2   skip:28 
ro-byt-n2820     total:218  pass:174  dwarn:0   dfail:0   fail:3   skip:41 
ro-hsw-i3-4010u  total:218  pass:177  dwarn:0   dfail:0   fail:0   skip:23 
ro-hsw-i7-4770r  total:219  pass:194  dwarn:0   dfail:0   fail:0   skip:25 
ro-ilk-i7-620lm  total:219  pass:132  dwarn:0   dfail:0   fail:1   skip:57 
ro-ilk1-i5-650   total:214  pass:152  dwarn:0   dfail:0   fail:1   skip:61 
ro-ivb-i7-3770   total:182  pass:149  dwarn:0   dfail:0   fail:0   skip:32 
ro-ivb2-i7-3770  total:219  pass:186  dwarn:0   dfail:0   fail:1   skip:32 
ro-skl-i7-6700hq total:214  pass:190  dwarn:0   dfail:0   fail:0   skip:24 
ro-snb-i7-2620M  total:113  pass:79   dwarn:0   dfail:0   fail:0   skip:33 

Results at /archive/results/CI_IGT_test/RO_Patchwork_871/

a6ac4ab drm-intel-nightly: 2016y-05m-12d-15h-37m-38s UTC integration manifest
89501f6 ALSA: hda - Support deferred i915 audio component binding
85696b1 ALSA: hda - Immediately fail if the i915 audio component is disabled
2a93ae0 drm/i915/hda: Add audio component stub

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Thu, 12 May 2016 20:06:53 +0200,
Imre Deak wrote:
> 
> 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.
> 
> v2:
> - Fail the probe in case component registration fails, instead of
>   suppressing the error. (Ville)
> - Register the component only for the real PCI device function.
> 
> CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

I haven't received v2 for the rest two patches.  I suppose they are
unchanged from v1?

It'd be good if we can get this into 4.7.  Will this go through drm
tree, or my tree?  I don't mind either way.  If you merge this do
drm(-intel) tree, just let me know the stable git id so that I can
merge it to sound.git tree with my rest two patches.


thanks,

Takashi

> ---
>  drivers/gpu/drm/i915/i915_drv.c    | 51 +++++++++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_audio.c | 53 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
>  include/drm/i915_component.h       |  5 ++++
>  4 files changed, 93 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 5ae7960..b127810 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1016,12 +1016,6 @@ 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 (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");
> -		return -ENODEV;
> -	}
> -
>  	/* Only bind to function 0 of the device. Early generations
>  	 * used function 1 as a placeholder for multi-head. This causes
>  	 * us confusion instead, especially on the systems where both
> @@ -1030,6 +1024,29 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (PCI_FUNC(pdev->devfn))
>  		return -ENODEV;
>  
> +	if (!(driver.driver_features & DRIVER_MODESET)) {
> +		int ret;
> +
> +		/*
> +		 * Notify the HDA driver so that it doesn't block waiting for
> +		 * i915 to become ready.
> +		 */
> +		ret = i915_audio_component_stub_init(&pdev->dev);
> +		if (ret)
> +			return ret;
> +
> +		/* Silently fail loading to not upset userspace. */
> +		DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
> +
> +		return 0;
> +	}
> +
> +	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");
> +		return -ENODEV;
> +	}
> +
>  	/*
>  	 * apple-gmux is needed on dual GPU MacBook Pro
>  	 * to probe the panel if we're the inactive GPU.
> @@ -1045,8 +1062,15 @@ 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);
>  }
>  
> @@ -1767,24 +1791,15 @@ static int __init i915_init(void)
>  	if (vgacon_text_force() && i915.modeset == -1)
>  		driver.driver_features &= ~DRIVER_MODESET;
>  
> -	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 b9329c2..603b3fe 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -824,3 +824,56 @@ 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,
> +};
> +
> +int 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 ret;
> +}
> +
> +void i915_audio_component_stub_cleanup(struct device *dev)
> +{
> +	component_del(dev, &i915_audio_component_stub_bind_ops);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0a9c10d..a828e00 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1111,6 +1111,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);
> +int i915_audio_component_stub_init(struct device *dev);
> +void i915_audio_component_stub_cleanup(struct device *dev);
>  
>  /* intel_display.c */
>  void intel_update_rawclk(struct drm_i915_private *dev_priv);
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> index b46fa0e..33da85a 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.5.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

On Fri, 2016-05-13 at 09:48 +0200, Takashi Iwai wrote:
> On Thu, 12 May 2016 20:06:53 +0200,
> Imre Deak wrote:
> > 
> > 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.
> > 
> > v2:
> > - Fail the probe in case component registration fails, instead of
> >   suppressing the error. (Ville)
> > - Register the component only for the real PCI device function.
> > 
> > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> I haven't received v2 for the rest two patches.  I suppose they are
> unchanged from v1?

I haven't changed them. They look ok to me, but someone else should
review patch 3.

> It'd be good if we can get this into 4.7.  Will this go through drm
> tree, or my tree?  I don't mind either way.  If you merge this do
> drm(-intel) tree, just let me know the stable git id so that I can
> merge it to sound.git tree with my rest two patches.

Adding Daniel. I think for bisectability patch 1 and 2 should be
combined, then that combined patch could go through drm-intel, while
patch 3 could go through the sound tree.

--Imre

> thanks,
> 
> Takashi
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c    | 51 +++++++++++++++++++++++-------------
> >  drivers/gpu/drm/i915/intel_audio.c | 53 ++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
> >  include/drm/i915_component.h       |  5 ++++
> >  4 files changed, 93 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 5ae7960..b127810 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1016,12 +1016,6 @@ 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 (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");
> > -		return -ENODEV;
> > -	}
> > -
> >  	/* Only bind to function 0 of the device. Early generations
> >  	 * used function 1 as a placeholder for multi-head. This causes
> >  	 * us confusion instead, especially on the systems where both
> > @@ -1030,6 +1024,29 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  	if (PCI_FUNC(pdev->devfn))
> >  		return -ENODEV;
> >  
> > +	if (!(driver.driver_features & DRIVER_MODESET)) {
> > +		int ret;
> > +
> > +		/*
> > +		 * Notify the HDA driver so that it doesn't block waiting for
> > +		 * i915 to become ready.
> > +		 */
> > +		ret = i915_audio_component_stub_init(&pdev->dev);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Silently fail loading to not upset userspace. */
> > +		DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
> > +
> > +		return 0;
> > +	}
> > +
> > +	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");
> > +		return -ENODEV;
> > +	}
> > +
> >  	/*
> >  	 * apple-gmux is needed on dual GPU MacBook Pro
> >  	 * to probe the panel if we're the inactive GPU.
> > @@ -1045,8 +1062,15 @@ 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);
> >  }
> >  
> > @@ -1767,24 +1791,15 @@ static int __init i915_init(void)
> >  	if (vgacon_text_force() && i915.modeset == -1)
> >  		driver.driver_features &= ~DRIVER_MODESET;
> >  
> > -	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 b9329c2..603b3fe 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -824,3 +824,56 @@ 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,
> > +};
> > +
> > +int 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 ret;
> > +}
> > +
> > +void i915_audio_component_stub_cleanup(struct device *dev)
> > +{
> > +	component_del(dev, &i915_audio_component_stub_bind_ops);
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 0a9c10d..a828e00 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1111,6 +1111,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);
> > +int i915_audio_component_stub_init(struct device *dev);
> > +void i915_audio_component_stub_cleanup(struct device *dev);
> >  
> >  /* intel_display.c */
> >  void intel_update_rawclk(struct drm_i915_private *dev_priv);
> > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> > index b46fa0e..33da85a 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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Fri, 13 May 2016 11:50:09 +0200,
Imre Deak wrote:
> 
> On Fri, 2016-05-13 at 09:48 +0200, Takashi Iwai wrote:
> > On Thu, 12 May 2016 20:06:53 +0200,
> > Imre Deak wrote:
> > > 
> > > 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.
> > > 
> > > v2:
> > > - Fail the probe in case component registration fails, instead of
> > >   suppressing the error. (Ville)
> > > - Register the component only for the real PCI device function.
> > > 
> > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > 
> > I haven't received v2 for the rest two patches.  I suppose they are
> > unchanged from v1?
> 
> I haven't changed them. They look ok to me, but someone else should
> review patch 3.
> 
> > It'd be good if we can get this into 4.7.  Will this go through drm
> > tree, or my tree?  I don't mind either way.  If you merge this do
> > drm(-intel) tree, just let me know the stable git id so that I can
> > merge it to sound.git tree with my rest two patches.
> 
> Adding Daniel. I think for bisectability patch 1 and 2 should be
> combined, then that combined patch could go through drm-intel, while
> patch 3 could go through the sound tree.

Well, the patch 2 and patch 3 have to be applied after the patch 1 in
anyway.  So, I can apply 2 and 3 once after merging the change for
patch 1 back into sound tree.  That is, three options:

1. Apply all 1, 2 and 3 in drm tree:
   however, patch 3 won't be applied cleanly to 4.6, so drm tree would
   need to merge sound for-next branch beforehand.

2. Apply all 1, 2 and 3 in sound tree:
   patch 1 won't be applied to 4.6 cleanly, so sound tree needs to
   merge drm tree at first.

3. Apply patch 1 to drm tree, then apply 2 and 3 in sound tree:
   sound tree will merge drm tree applied with patch 1, then apply 2
   and 3 on top of it.


Takashi

> 
> --Imre
> 
> > thanks,
> > 
> > Takashi
> > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c    | 51 +++++++++++++++++++++++-------------
> > >  drivers/gpu/drm/i915/intel_audio.c | 53 ++++++++++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
> > >  include/drm/i915_component.h       |  5 ++++
> > >  4 files changed, 93 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 5ae7960..b127810 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -1016,12 +1016,6 @@ 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 (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");
> > > -		return -ENODEV;
> > > -	}
> > > -
> > >  	/* Only bind to function 0 of the device. Early generations
> > >  	 * used function 1 as a placeholder for multi-head. This causes
> > >  	 * us confusion instead, especially on the systems where both
> > > @@ -1030,6 +1024,29 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > >  	if (PCI_FUNC(pdev->devfn))
> > >  		return -ENODEV;
> > >  
> > > +	if (!(driver.driver_features & DRIVER_MODESET)) {
> > > +		int ret;
> > > +
> > > +		/*
> > > +		 * Notify the HDA driver so that it doesn't block waiting for
> > > +		 * i915 to become ready.
> > > +		 */
> > > +		ret = i915_audio_component_stub_init(&pdev->dev);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		/* Silently fail loading to not upset userspace. */
> > > +		DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
> > > +
> > > +		return 0;
> > > +	}
> > > +
> > > +	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");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > >  	/*
> > >  	 * apple-gmux is needed on dual GPU MacBook Pro
> > >  	 * to probe the panel if we're the inactive GPU.
> > > @@ -1045,8 +1062,15 @@ 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);
> > >  }
> > >  
> > > @@ -1767,24 +1791,15 @@ static int __init i915_init(void)
> > >  	if (vgacon_text_force() && i915.modeset == -1)
> > >  		driver.driver_features &= ~DRIVER_MODESET;
> > >  
> > > -	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 b9329c2..603b3fe 100644
> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > @@ -824,3 +824,56 @@ 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,
> > > +};
> > > +
> > > +int 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 ret;
> > > +}
> > > +
> > > +void i915_audio_component_stub_cleanup(struct device *dev)
> > > +{
> > > +	component_del(dev, &i915_audio_component_stub_bind_ops);
> > > +}
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 0a9c10d..a828e00 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1111,6 +1111,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);
> > > +int i915_audio_component_stub_init(struct device *dev);
> > > +void i915_audio_component_stub_cleanup(struct device *dev);
> > >  
> > >  /* intel_display.c */
> > >  void intel_update_rawclk(struct drm_i915_private *dev_priv);
> > > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> > > index b46fa0e..33da85a 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.
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

On Fri, 2016-05-13 at 12:04 +0200, Takashi Iwai wrote:
> On Fri, 13 May 2016 11:50:09 +0200,
> Imre Deak wrote:
> > 
> > On Fri, 2016-05-13 at 09:48 +0200, Takashi Iwai wrote:
> > > On Thu, 12 May 2016 20:06:53 +0200,
> > > Imre Deak wrote:
> > > > 
> > > > 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.
> > > > 
> > > > v2:
> > > > - Fail the probe in case component registration fails, instead
> > > > of
> > > >   suppressing the error. (Ville)
> > > > - Register the component only for the real PCI device function.
> > > > 
> > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > 
> > > I haven't received v2 for the rest two patches.  I suppose they
> > > are
> > > unchanged from v1?
> > 
> > I haven't changed them. They look ok to me, but someone else should
> > review patch 3.
> > 
> > > It'd be good if we can get this into 4.7.  Will this go through
> > > drm
> > > tree, or my tree?  I don't mind either way.  If you merge this do
> > > drm(-intel) tree, just let me know the stable git id so that I
> > > can
> > > merge it to sound.git tree with my rest two patches.
> > 
> > Adding Daniel. I think for bisectability patch 1 and 2 should be
> > combined, then that combined patch could go through drm-intel,
> > while
> > patch 3 could go through the sound tree.
> 
> Well, the patch 2 and patch 3 have to be applied after the patch 1 in
> anyway.  So, I can apply 2 and 3 once after merging the change for
> patch 1 back into sound tree.  That is, three options:

Actually things will work ok even right after patch 1. In case of
nomodeset hdac_component_master_bind() will WARN, but the outcome will
be failing the sound driver loading which is expected at that point. So
I'm fine with either of the 3 options you suggest.

--Imre

> 1. Apply all 1, 2 and 3 in drm tree:
>    however, patch 3 won't be applied cleanly to 4.6, so drm tree would
>    need to merge sound for-next branch beforehand.
> 
> 2. Apply all 1, 2 and 3 in sound tree:
>    patch 1 won't be applied to 4.6 cleanly, so sound tree needs to
>    merge drm tree at first.
> 
> 3. Apply patch 1 to drm tree, then apply 2 and 3 in sound tree:
>    sound tree will merge drm tree applied with patch 1, then apply 2
>    and 3 on top of it.
> 
> 
> Takashi
> 
> > 
> > --Imre
> > 
> > > thanks,
> > > 
> > > Takashi
> > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c    | 51
> > > > +++++++++++++++++++++++-------------
> > > >  drivers/gpu/drm/i915/intel_audio.c | 53
> > > > ++++++++++++++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
> > > >  include/drm/i915_component.h       |  5 ++++
> > > >  4 files changed, 93 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > index 5ae7960..b127810 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -1016,12 +1016,6 @@ 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 (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");
> > > > -		return -ENODEV;
> > > > -	}
> > > > -
> > > >  	/* Only bind to function 0 of the device. Early
> > > > generations
> > > >  	 * used function 1 as a placeholder for multi-head.
> > > > This causes
> > > >  	 * us confusion instead, especially on the systems
> > > > where both
> > > > @@ -1030,6 +1024,29 @@ static int i915_pci_probe(struct pci_dev
> > > > *pdev, const struct pci_device_id *ent)
> > > >  	if (PCI_FUNC(pdev->devfn))
> > > >  		return -ENODEV;
> > > >  
> > > > +	if (!(driver.driver_features & DRIVER_MODESET)) {
> > > > +		int ret;
> > > > +
> > > > +		/*
> > > > +		 * Notify the HDA driver so that it doesn't
> > > > block waiting for
> > > > +		 * i915 to become ready.
> > > > +		 */
> > > > +		ret = i915_audio_component_stub_init(&pdev-
> > > > >dev);
> > > > +		if (ret)
> > > > +			return ret;
> > > > +
> > > > +		/* Silently fail loading to not upset
> > > > userspace. */
> > > > +		DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
> > > > +
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	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");
> > > > +		return -ENODEV;
> > > > +	}
> > > > +
> > > >  	/*
> > > >  	 * apple-gmux is needed on dual GPU MacBook Pro
> > > >  	 * to probe the panel if we're the inactive GPU.
> > > > @@ -1045,8 +1062,15 @@ 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);
> > > >  }
> > > >  
> > > > @@ -1767,24 +1791,15 @@ static int __init i915_init(void)
> > > >  	if (vgacon_text_force() && i915.modeset == -1)
> > > >  		driver.driver_features &= ~DRIVER_MODESET;
> > > >  
> > > > -	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 b9329c2..603b3fe 100644
> > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > @@ -824,3 +824,56 @@ 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,
> > > > +};
> > > > +
> > > > +int 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 ret;
> > > > +}
> > > > +
> > > > +void i915_audio_component_stub_cleanup(struct device *dev)
> > > > +{
> > > > +	component_del(dev,
> > > > &i915_audio_component_stub_bind_ops);
> > > > +}
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 0a9c10d..a828e00 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1111,6 +1111,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);
> > > > +int i915_audio_component_stub_init(struct device *dev);
> > > > +void i915_audio_component_stub_cleanup(struct device *dev);
> > > >  
> > > >  /* intel_display.c */
> > > >  void intel_update_rawclk(struct drm_i915_private *dev_priv);
> > > > diff --git a/include/drm/i915_component.h
> > > > b/include/drm/i915_component.h
> > > > index b46fa0e..33da85a 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.
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> 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.
> 
> v2:
> - Fail the probe in case component registration fails, instead of
>   suppressing the error. (Ville)
> - Register the component only for the real PCI device function.
> 
> CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>

We don't support not running with modesetting. Why do we suddenly care?
Same for users creating a .config that fails to boot or work ...

If HDA needs to coporate with gfx to get things done, then imo we should
just require that i915.ko is there.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c    | 51 +++++++++++++++++++++++-------------
>  drivers/gpu/drm/i915/intel_audio.c | 53 ++++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
>  include/drm/i915_component.h       |  5 ++++
>  4 files changed, 93 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 5ae7960..b127810 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1016,12 +1016,6 @@ 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 (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");
> -		return -ENODEV;
> -	}
> -
>  	/* Only bind to function 0 of the device. Early generations
>  	 * used function 1 as a placeholder for multi-head. This causes
>  	 * us confusion instead, especially on the systems where both
> @@ -1030,6 +1024,29 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (PCI_FUNC(pdev->devfn))
>  		return -ENODEV;
>  
> +	if (!(driver.driver_features & DRIVER_MODESET)) {
> +		int ret;
> +
> +		/*
> +		 * Notify the HDA driver so that it doesn't block waiting for
> +		 * i915 to become ready.
> +		 */
> +		ret = i915_audio_component_stub_init(&pdev->dev);
> +		if (ret)
> +			return ret;
> +
> +		/* Silently fail loading to not upset userspace. */
> +		DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
> +
> +		return 0;
> +	}
> +
> +	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");
> +		return -ENODEV;
> +	}
> +
>  	/*
>  	 * apple-gmux is needed on dual GPU MacBook Pro
>  	 * to probe the panel if we're the inactive GPU.
> @@ -1045,8 +1062,15 @@ 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);
>  }
>  
> @@ -1767,24 +1791,15 @@ static int __init i915_init(void)
>  	if (vgacon_text_force() && i915.modeset == -1)
>  		driver.driver_features &= ~DRIVER_MODESET;
>  
> -	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 b9329c2..603b3fe 100644
> --- a/drivers/gpu/drm/i915/intel_audio.c
> +++ b/drivers/gpu/drm/i915/intel_audio.c
> @@ -824,3 +824,56 @@ 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,
> +};
> +
> +int 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 ret;
> +}
> +
> +void i915_audio_component_stub_cleanup(struct device *dev)
> +{
> +	component_del(dev, &i915_audio_component_stub_bind_ops);
> +}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0a9c10d..a828e00 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1111,6 +1111,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);
> +int i915_audio_component_stub_init(struct device *dev);
> +void i915_audio_component_stub_cleanup(struct device *dev);
>  
>  /* intel_display.c */
>  void intel_update_rawclk(struct drm_i915_private *dev_priv);
> diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> index b46fa0e..33da85a 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.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Takashi Iwai]

On Tue, 17 May 2016 09:20:48 +0200,
Daniel Vetter wrote:
> 
> On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > 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.
> > 
> > v2:
> > - Fail the probe in case component registration fails, instead of
> >   suppressing the error. (Ville)
> > - Register the component only for the real PCI device function.
> > 
> > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> We don't support not running with modesetting. Why do we suddenly care?

This is needed for the patch 2 and 3.  Right now we have no blocking
or deferred component binding, so far, in HD-audio side.  This caused
problems when async module probe was done.

So, the patch 3 implements the blocking behavior of HD-audio side.  It
would lead to another regression when i915 doesn't notify its disabled
state by this patch.  Otherwise the HD-audio driver will be blocked
endlessly of unnecessarily long.

> Same for users creating a .config that fails to boot or work ...

The config isn't cared much, but the problem is about the runtime boot
option.

> If HDA needs to coporate with gfx to get things done, then imo we should
> just require that i915.ko is there.

Other way round: we do already require i915 in HD-audio side.  But in
this case, we do *not* want to require i915 when it's disabled in
runtime. 


thanks,

Takashi

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c    | 51 +++++++++++++++++++++++-------------
> >  drivers/gpu/drm/i915/intel_audio.c | 53 ++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
> >  include/drm/i915_component.h       |  5 ++++
> >  4 files changed, 93 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 5ae7960..b127810 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1016,12 +1016,6 @@ 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 (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");
> > -		return -ENODEV;
> > -	}
> > -
> >  	/* Only bind to function 0 of the device. Early generations
> >  	 * used function 1 as a placeholder for multi-head. This causes
> >  	 * us confusion instead, especially on the systems where both
> > @@ -1030,6 +1024,29 @@ static int i915_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> >  	if (PCI_FUNC(pdev->devfn))
> >  		return -ENODEV;
> >  
> > +	if (!(driver.driver_features & DRIVER_MODESET)) {
> > +		int ret;
> > +
> > +		/*
> > +		 * Notify the HDA driver so that it doesn't block waiting for
> > +		 * i915 to become ready.
> > +		 */
> > +		ret = i915_audio_component_stub_init(&pdev->dev);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Silently fail loading to not upset userspace. */
> > +		DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
> > +
> > +		return 0;
> > +	}
> > +
> > +	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");
> > +		return -ENODEV;
> > +	}
> > +
> >  	/*
> >  	 * apple-gmux is needed on dual GPU MacBook Pro
> >  	 * to probe the panel if we're the inactive GPU.
> > @@ -1045,8 +1062,15 @@ 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);
> >  }
> >  
> > @@ -1767,24 +1791,15 @@ static int __init i915_init(void)
> >  	if (vgacon_text_force() && i915.modeset == -1)
> >  		driver.driver_features &= ~DRIVER_MODESET;
> >  
> > -	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 b9329c2..603b3fe 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -824,3 +824,56 @@ 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,
> > +};
> > +
> > +int 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 ret;
> > +}
> > +
> > +void i915_audio_component_stub_cleanup(struct device *dev)
> > +{
> > +	component_del(dev, &i915_audio_component_stub_bind_ops);
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 0a9c10d..a828e00 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1111,6 +1111,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);
> > +int i915_audio_component_stub_init(struct device *dev);
> > +void i915_audio_component_stub_cleanup(struct device *dev);
> >  
> >  /* intel_display.c */
> >  void intel_update_rawclk(struct drm_i915_private *dev_priv);
> > diff --git a/include/drm/i915_component.h b/include/drm/i915_component.h
> > index b46fa0e..33da85a 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.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

On ti, 2016-05-17 at 09:20 +0200, Daniel Vetter wrote:
> On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > 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.
> > 
> > v2:
> > - Fail the probe in case component registration fails, instead of
> >   suppressing the error. (Ville)
> > - Register the component only for the real PCI device function.
> > 
> > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> 
> We don't support not running with modesetting. Why do we suddenly
> care?
> Same for users creating a .config that fails to boot or work ...

Yes, as Takashi said, this would be only for the case when someone
decides to boot with the nomodeset parameter and still expects audio
functionality (without HDMI codec functionality). This approach would
ensure the proper i915->audio init order via the component bind hook as
opposed to the current way of depending on request_module("i915") on
the audio side. This latter one is kinda unreliable due to depending on
user space (think of the last related kmod bug) and also doesn't always
work (audio built-in, i915 module).

> If HDA needs to coporate with gfx to get things done, then imo we
> should
> just require that i915.ko is there.

Agreed there should be a proper kconfig dependency on i915 if HDMI
functionality is enabled in the audio driver. Takashi said that this is
already the case.

--Imre


> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c    | 51 +++++++++++++++++++++++---
> > ----------
> >  drivers/gpu/drm/i915/intel_audio.c | 53
> > ++++++++++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h   |  2 ++
> >  include/drm/i915_component.h       |  5 ++++
> >  4 files changed, 93 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > b/drivers/gpu/drm/i915/i915_drv.c
> > index 5ae7960..b127810 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -1016,12 +1016,6 @@ 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 (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");
> > -		return -ENODEV;
> > -	}
> > -
> >  	/* Only bind to function 0 of the device. Early
> > generations
> >  	 * used function 1 as a placeholder for multi-head. This
> > causes
> >  	 * us confusion instead, especially on the systems where
> > both
> > @@ -1030,6 +1024,29 @@ static int i915_pci_probe(struct pci_dev
> > *pdev, const struct pci_device_id *ent)
> >  	if (PCI_FUNC(pdev->devfn))
> >  		return -ENODEV;
> >  
> > +	if (!(driver.driver_features & DRIVER_MODESET)) {
> > +		int ret;
> > +
> > +		/*
> > +		 * Notify the HDA driver so that it doesn't block
> > waiting for
> > +		 * i915 to become ready.
> > +		 */
> > +		ret = i915_audio_component_stub_init(&pdev->dev);
> > +		if (ret)
> > +			return ret;
> > +
> > +		/* Silently fail loading to not upset userspace.
> > */
> > +		DRM_DEBUG_DRIVER("KMS and UMS disabled.\n");
> > +
> > +		return 0;
> > +	}
> > +
> > +	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");
> > +		return -ENODEV;
> > +	}
> > +
> >  	/*
> >  	 * apple-gmux is needed on dual GPU MacBook Pro
> >  	 * to probe the panel if we're the inactive GPU.
> > @@ -1045,8 +1062,15 @@ 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);
> >  }
> >  
> > @@ -1767,24 +1791,15 @@ static int __init i915_init(void)
> >  	if (vgacon_text_force() && i915.modeset == -1)
> >  		driver.driver_features &= ~DRIVER_MODESET;
> >  
> > -	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 b9329c2..603b3fe 100644
> > --- a/drivers/gpu/drm/i915/intel_audio.c
> > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > @@ -824,3 +824,56 @@ 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,
> > +};
> > +
> > +int 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 ret;
> > +}
> > +
> > +void i915_audio_component_stub_cleanup(struct device *dev)
> > +{
> > +	component_del(dev, &i915_audio_component_stub_bind_ops);
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 0a9c10d..a828e00 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1111,6 +1111,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);
> > +int i915_audio_component_stub_init(struct device *dev);
> > +void i915_audio_component_stub_cleanup(struct device *dev);
> >  
> >  /* intel_display.c */
> >  void intel_update_rawclk(struct drm_i915_private *dev_priv);
> > diff --git a/include/drm/i915_component.h
> > b/include/drm/i915_component.h
> > index b46fa0e..33da85a 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.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, May 17, 2016 at 09:37:12AM +0200, Takashi Iwai wrote:
> On Tue, 17 May 2016 09:20:48 +0200,
> Daniel Vetter wrote:
> > 
> > On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > > 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.
> > > 
> > > v2:
> > > - Fail the probe in case component registration fails, instead of
> > >   suppressing the error. (Ville)
> > > - Register the component only for the real PCI device function.
> > > 
> > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > 
> > We don't support not running with modesetting. Why do we suddenly care?
> 
> This is needed for the patch 2 and 3.  Right now we have no blocking
> or deferred component binding, so far, in HD-audio side.  This caused
> problems when async module probe was done.
> 
> So, the patch 3 implements the blocking behavior of HD-audio side.  It
> would lead to another regression when i915 doesn't notify its disabled
> state by this patch.  Otherwise the HD-audio driver will be blocked
> endlessly of unnecessarily long.
> 
> > Same for users creating a .config that fails to boot or work ...
> 
> The config isn't cared much, but the problem is about the runtime boot
> option.
> 
> > If HDA needs to coporate with gfx to get things done, then imo we should
> > just require that i915.ko is there.
> 
> Other way round: we do already require i915 in HD-audio side.  But in
> this case, we do *not* want to require i915 when it's disabled in
> runtime. 

That's what I mean: If you boot with i915.nomodeset you're explicitly fine
with a somewhat non-useable system - that option is for debugging only
really. If that means audio also doesn't work, then I think that's ok.
Adding complexity for this case (which means more error paths we don't
ever test and hence _will_ break) seems over the top.

I'm quite opposed to adding error handling for every condition in general
because the combinatorial testing madness just can't be handled. The one
exception in the i915.ko driver is that when the render side died
(terminal gpu hang) we'll try our best to keep the display alive. But
that's it, and the justification for that is "we want users to be able to
get the bug report out". I don't see a justification of that magnitude for
this feature here at all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Tue, 17 May 2016 11:42:17 +0200,
Daniel Vetter wrote:
> 
> On Tue, May 17, 2016 at 09:37:12AM +0200, Takashi Iwai wrote:
> > On Tue, 17 May 2016 09:20:48 +0200,
> > Daniel Vetter wrote:
> > > 
> > > On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > > > 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.
> > > > 
> > > > v2:
> > > > - Fail the probe in case component registration fails, instead of
> > > >   suppressing the error. (Ville)
> > > > - Register the component only for the real PCI device function.
> > > > 
> > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > 
> > > We don't support not running with modesetting. Why do we suddenly care?
> > 
> > This is needed for the patch 2 and 3.  Right now we have no blocking
> > or deferred component binding, so far, in HD-audio side.  This caused
> > problems when async module probe was done.
> > 
> > So, the patch 3 implements the blocking behavior of HD-audio side.  It
> > would lead to another regression when i915 doesn't notify its disabled
> > state by this patch.  Otherwise the HD-audio driver will be blocked
> > endlessly of unnecessarily long.
> > 
> > > Same for users creating a .config that fails to boot or work ...
> > 
> > The config isn't cared much, but the problem is about the runtime boot
> > option.
> > 
> > > If HDA needs to coporate with gfx to get things done, then imo we should
> > > just require that i915.ko is there.
> > 
> > Other way round: we do already require i915 in HD-audio side.  But in
> > this case, we do *not* want to require i915 when it's disabled in
> > runtime. 
> 
> That's what I mean: If you boot with i915.nomodeset you're explicitly fine
> with a somewhat non-useable system - that option is for debugging only
> really. If that means audio also doesn't work, then I think that's ok.

It's not only "it doesn't work".  The module load gets stuck.  So we
need some notification for the blocked component binding.

> Adding complexity for this case (which means more error paths we don't
> ever test and hence _will_ break) seems over the top.
> 
> I'm quite opposed to adding error handling for every condition in general
> because the combinatorial testing madness just can't be handled. The one
> exception in the i915.ko driver is that when the render side died
> (terminal gpu hang) we'll try our best to keep the display alive. But
> that's it, and the justification for that is "we want users to be able to
> get the bug report out". I don't see a justification of that magnitude for
> this feature here at all.

Well, actually the patchset was proposed just because Intel CI tests
failed due to async module probes.  If Intel is happy with continued
CI test failures, I'm also happy with the current situation ;)


Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

On ti, 2016-05-17 at 11:59 +0200, Takashi Iwai wrote:
> On Tue, 17 May 2016 11:42:17 +0200,
> Daniel Vetter wrote:
> > 
> > On Tue, May 17, 2016 at 09:37:12AM +0200, Takashi Iwai wrote:
> > > On Tue, 17 May 2016 09:20:48 +0200,
> > > Daniel Vetter wrote:
> > > > 
> > > > On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > > > > 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.
> > > > > 
> > > > > v2:
> > > > > - Fail the probe in case component registration fails, instead of
> > > > >   suppressing the error. (Ville)
> > > > > - Register the component only for the real PCI device function.
> > > > > 
> > > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > 
> > > > We don't support not running with modesetting. Why do we suddenly care?
> > > 
> > > This is needed for the patch 2 and 3.  Right now we have no blocking
> > > or deferred component binding, so far, in HD-audio side.  This caused
> > > problems when async module probe was done.
> > > 
> > > So, the patch 3 implements the blocking behavior of HD-audio side.  It
> > > would lead to another regression when i915 doesn't notify its disabled
> > > state by this patch.  Otherwise the HD-audio driver will be blocked
> > > endlessly of unnecessarily long.
> > > 
> > > > Same for users creating a .config that fails to boot or work ...
> > > 
> > > The config isn't cared much, but the problem is about the runtime boot
> > > option.
> > > 
> > > > If HDA needs to coporate with gfx to get things done, then imo we should
> > > > just require that i915.ko is there.
> > > 
> > > Other way round: we do already require i915 in HD-audio side.  But in
> > > this case, we do *not* want to require i915 when it's disabled in
> > > runtime. 
> > 
> > That's what I mean: If you boot with i915.nomodeset you're explicitly fine
> > with a somewhat non-useable system - that option is for debugging only
> > really. If that means audio also doesn't work, then I think that's ok.
> 
> It's not only "it doesn't work".  The module load gets stuck.  So we
> need some notification for the blocked component binding.
> 
> > Adding complexity for this case (which means more error paths we don't
> > ever test and hence _will_ break) seems over the top.
> > 
> > I'm quite opposed to adding error handling for every condition in general
> > because the combinatorial testing madness just can't be handled. The one
> > exception in the i915.ko driver is that when the render side died
> > (terminal gpu hang) we'll try our best to keep the display alive. But
> > that's it, and the justification for that is "we want users to be able to
> > get the bug report out". I don't see a justification of that magnitude for
> > this feature here at all.
> 
> Well, actually the patchset was proposed just because Intel CI tests
> failed due to async module probes.  If Intel is happy with continued
> CI test failures, I'm also happy with the current situation ;)

It's not only due to those particular failures. That was caused by a
kmod bug and as such would be good to not depend on that mechanism. But
things will fail atm even in the normal case when audio is built-in and
i915 is a module. This patchset would solve that too.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, May 17, 2016 at 01:22:52PM +0300, Imre Deak wrote:
> On ti, 2016-05-17 at 11:59 +0200, Takashi Iwai wrote:
> > On Tue, 17 May 2016 11:42:17 +0200,
> > Daniel Vetter wrote:
> > > 
> > > On Tue, May 17, 2016 at 09:37:12AM +0200, Takashi Iwai wrote:
> > > > On Tue, 17 May 2016 09:20:48 +0200,
> > > > Daniel Vetter wrote:
> > > > > 
> > > > > On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > > > > > 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.
> > > > > > 
> > > > > > v2:
> > > > > > - Fail the probe in case component registration fails, instead of
> > > > > >   suppressing the error. (Ville)
> > > > > > - Register the component only for the real PCI device function.
> > > > > > 
> > > > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > 
> > > > > We don't support not running with modesetting. Why do we suddenly care?
> > > > 
> > > > This is needed for the patch 2 and 3.  Right now we have no blocking
> > > > or deferred component binding, so far, in HD-audio side.  This caused
> > > > problems when async module probe was done.
> > > > 
> > > > So, the patch 3 implements the blocking behavior of HD-audio side.  It
> > > > would lead to another regression when i915 doesn't notify its disabled
> > > > state by this patch.  Otherwise the HD-audio driver will be blocked
> > > > endlessly of unnecessarily long.
> > > > 
> > > > > Same for users creating a .config that fails to boot or work ...
> > > > 
> > > > The config isn't cared much, but the problem is about the runtime boot
> > > > option.
> > > > 
> > > > > If HDA needs to coporate with gfx to get things done, then imo we should
> > > > > just require that i915.ko is there.
> > > > 
> > > > Other way round: we do already require i915 in HD-audio side.  But in
> > > > this case, we do *not* want to require i915 when it's disabled in
> > > > runtime. 
> > > 
> > > That's what I mean: If you boot with i915.nomodeset you're explicitly fine
> > > with a somewhat non-useable system - that option is for debugging only
> > > really. If that means audio also doesn't work, then I think that's ok.
> > 
> > It's not only "it doesn't work".  The module load gets stuck.  So we
> > need some notification for the blocked component binding.
> > 
> > > Adding complexity for this case (which means more error paths we don't
> > > ever test and hence _will_ break) seems over the top.
> > > 
> > > I'm quite opposed to adding error handling for every condition in general
> > > because the combinatorial testing madness just can't be handled. The one
> > > exception in the i915.ko driver is that when the render side died
> > > (terminal gpu hang) we'll try our best to keep the display alive. But
> > > that's it, and the justification for that is "we want users to be able to
> > > get the bug report out". I don't see a justification of that magnitude for
> > > this feature here at all.
> > 
> > Well, actually the patchset was proposed just because Intel CI tests
> > failed due to async module probes.  If Intel is happy with continued
> > CI test failures, I'm also happy with the current situation ;)
> 
> It's not only due to those particular failures. That was caused by a
> kmod bug and as such would be good to not depend on that mechanism. But
> things will fail atm even in the normal case when audio is built-in and
> i915 is a module. This patchset would solve that too.

I'm not against patch 3 from this series, which seems to be the bugfix we
want. I'm against the kludge in patch 1 here only (and maybe patch 2, not
sure about that). Patch 1 here looks like a workaround purely for the
i915.modeset=0 case. And I don't want special code for debug knobs if we
can avoid it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

On ti, 2016-05-17 at 13:10 +0200, Daniel Vetter wrote:
> On Tue, May 17, 2016 at 01:22:52PM +0300, Imre Deak wrote:
> > On ti, 2016-05-17 at 11:59 +0200, Takashi Iwai wrote:
> > > On Tue, 17 May 2016 11:42:17 +0200,
> > > Daniel Vetter wrote:
> > > > 
> > > > On Tue, May 17, 2016 at 09:37:12AM +0200, Takashi Iwai wrote:
> > > > > On Tue, 17 May 2016 09:20:48 +0200,
> > > > > Daniel Vetter wrote:
> > > > > > 
> > > > > > On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > > > > > > 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.
> > > > > > > 
> > > > > > > v2:
> > > > > > > - Fail the probe in case component registration fails,
> > > > > > > instead of
> > > > > > >   suppressing the error. (Ville)
> > > > > > > - Register the component only for the real PCI device
> > > > > > > function.
> > > > > > > 
> > > > > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > > 
> > > > > > We don't support not running with modesetting. Why do we
> > > > > > suddenly care?
> > > > > 
> > > > > This is needed for the patch 2 and 3.  Right now we have no
> > > > > blocking
> > > > > or deferred component binding, so far, in HD-audio
> > > > > side.  This caused
> > > > > problems when async module probe was done.
> > > > > 
> > > > > So, the patch 3 implements the blocking behavior of HD-audio
> > > > > side.  It
> > > > > would lead to another regression when i915 doesn't notify its
> > > > > disabled
> > > > > state by this patch.  Otherwise the HD-audio driver will be
> > > > > blocked
> > > > > endlessly of unnecessarily long.
> > > > > 
> > > > > > Same for users creating a .config that fails to boot or
> > > > > > work ...
> > > > > 
> > > > > The config isn't cared much, but the problem is about the
> > > > > runtime boot
> > > > > option.
> > > > > 
> > > > > > If HDA needs to coporate with gfx to get things done, then
> > > > > > imo we should
> > > > > > just require that i915.ko is there.
> > > > > 
> > > > > Other way round: we do already require i915 in HD-audio
> > > > > side.  But in
> > > > > this case, we do *not* want to require i915 when it's
> > > > > disabled in
> > > > > runtime. 
> > > > 
> > > > That's what I mean: If you boot with i915.nomodeset you're
> > > > explicitly fine
> > > > with a somewhat non-useable system - that option is for
> > > > debugging only
> > > > really. If that means audio also doesn't work, then I think
> > > > that's ok.
> > > 
> > > It's not only "it doesn't work".  The module load gets stuck.  So
> > > we
> > > need some notification for the blocked component binding.
> > > 
> > > > Adding complexity for this case (which means more error paths
> > > > we don't
> > > > ever test and hence _will_ break) seems over the top.
> > > > 
> > > > I'm quite opposed to adding error handling for every condition
> > > > in general
> > > > because the combinatorial testing madness just can't be
> > > > handled. The one
> > > > exception in the i915.ko driver is that when the render side
> > > > died
> > > > (terminal gpu hang) we'll try our best to keep the display
> > > > alive. But
> > > > that's it, and the justification for that is "we want users to
> > > > be able to
> > > > get the bug report out". I don't see a justification of that
> > > > magnitude for
> > > > this feature here at all.
> > > 
> > > Well, actually the patchset was proposed just because Intel CI
> > > tests
> > > failed due to async module probes.  If Intel is happy with
> > > continued
> > > CI test failures, I'm also happy with the current situation ;)
> > 
> > It's not only due to those particular failures. That was caused by
> > a
> > kmod bug and as such would be good to not depend on that mechanism.
> > But
> > things will fail atm even in the normal case when audio is built-in 
> > and
> > i915 is a module. This patchset would solve that too.
> 
> I'm not against patch 3 from this series, which seems to be the
> bugfix we
> want. I'm against the kludge in patch 1 here only (and maybe patch 2,
> not
> sure about that). Patch 1 here looks like a workaround purely for the
> i915.modeset=0 case. And I don't want special code for debug knobs if
> we
> can avoid it.

We have discussed this in more detail, see:
https://bugs.freedesktop.org/show_bug.cgi?id=94566#c26

The point from Takashi in that discussion was that in order to do the
probing via the component bind hook as we want and as patch 3 does we
need to make sure basic audio functionality works even with the
nomodeset option. For that you need the kludge in patch 1. I think of
this as a similar kludge we have for not failing modprobing i915 in
case of nomodeset. Legacy userspace depends on this for basic graphics
functionality and in the same way some other userspace bits may depend
on basic audio functionality.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, May 17, 2016 at 03:11:02PM +0300, Imre Deak wrote:
> On ti, 2016-05-17 at 13:10 +0200, Daniel Vetter wrote:
> > On Tue, May 17, 2016 at 01:22:52PM +0300, Imre Deak wrote:
> > > On ti, 2016-05-17 at 11:59 +0200, Takashi Iwai wrote:
> > > > On Tue, 17 May 2016 11:42:17 +0200,
> > > > Daniel Vetter wrote:
> > > > > 
> > > > > On Tue, May 17, 2016 at 09:37:12AM +0200, Takashi Iwai wrote:
> > > > > > On Tue, 17 May 2016 09:20:48 +0200,
> > > > > > Daniel Vetter wrote:
> > > > > > > 
> > > > > > > On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > > > > > > > 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.
> > > > > > > > 
> > > > > > > > v2:
> > > > > > > > - Fail the probe in case component registration fails,
> > > > > > > > instead of
> > > > > > > >   suppressing the error. (Ville)
> > > > > > > > - Register the component only for the real PCI device
> > > > > > > > function.
> > > > > > > > 
> > > > > > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > > > 
> > > > > > > We don't support not running with modesetting. Why do we
> > > > > > > suddenly care?
> > > > > > 
> > > > > > This is needed for the patch 2 and 3.  Right now we have no
> > > > > > blocking
> > > > > > or deferred component binding, so far, in HD-audio
> > > > > > side.  This caused
> > > > > > problems when async module probe was done.
> > > > > > 
> > > > > > So, the patch 3 implements the blocking behavior of HD-audio
> > > > > > side.  It
> > > > > > would lead to another regression when i915 doesn't notify its
> > > > > > disabled
> > > > > > state by this patch.  Otherwise the HD-audio driver will be
> > > > > > blocked
> > > > > > endlessly of unnecessarily long.
> > > > > > 
> > > > > > > Same for users creating a .config that fails to boot or
> > > > > > > work ...
> > > > > > 
> > > > > > The config isn't cared much, but the problem is about the
> > > > > > runtime boot
> > > > > > option.
> > > > > > 
> > > > > > > If HDA needs to coporate with gfx to get things done, then
> > > > > > > imo we should
> > > > > > > just require that i915.ko is there.
> > > > > > 
> > > > > > Other way round: we do already require i915 in HD-audio
> > > > > > side.  But in
> > > > > > this case, we do *not* want to require i915 when it's
> > > > > > disabled in
> > > > > > runtime. 
> > > > > 
> > > > > That's what I mean: If you boot with i915.nomodeset you're
> > > > > explicitly fine
> > > > > with a somewhat non-useable system - that option is for
> > > > > debugging only
> > > > > really. If that means audio also doesn't work, then I think
> > > > > that's ok.
> > > > 
> > > > It's not only "it doesn't work".  The module load gets stuck.  So
> > > > we
> > > > need some notification for the blocked component binding.
> > > > 
> > > > > Adding complexity for this case (which means more error paths
> > > > > we don't
> > > > > ever test and hence _will_ break) seems over the top.
> > > > > 
> > > > > I'm quite opposed to adding error handling for every condition
> > > > > in general
> > > > > because the combinatorial testing madness just can't be
> > > > > handled. The one
> > > > > exception in the i915.ko driver is that when the render side
> > > > > died
> > > > > (terminal gpu hang) we'll try our best to keep the display
> > > > > alive. But
> > > > > that's it, and the justification for that is "we want users to
> > > > > be able to
> > > > > get the bug report out". I don't see a justification of that
> > > > > magnitude for
> > > > > this feature here at all.
> > > > 
> > > > Well, actually the patchset was proposed just because Intel CI
> > > > tests
> > > > failed due to async module probes.  If Intel is happy with
> > > > continued
> > > > CI test failures, I'm also happy with the current situation ;)
> > > 
> > > It's not only due to those particular failures. That was caused by
> > > a
> > > kmod bug and as such would be good to not depend on that mechanism.
> > > But
> > > things will fail atm even in the normal case when audio is built-in 
> > > and
> > > i915 is a module. This patchset would solve that too.
> > 
> > I'm not against patch 3 from this series, which seems to be the
> > bugfix we
> > want. I'm against the kludge in patch 1 here only (and maybe patch 2,
> > not
> > sure about that). Patch 1 here looks like a workaround purely for the
> > i915.modeset=0 case. And I don't want special code for debug knobs if
> > we
> > can avoid it.
> 
> We have discussed this in more detail, see:
> https://bugs.freedesktop.org/show_bug.cgi?id=94566#c26

I disagree with Takashi here. modeset=0 is a debug option, and we should
probably mark it as _unsafe.
> 
> The point from Takashi in that discussion was that in order to do the
> probing via the component bind hook as we want and as patch 3 does we
> need to make sure basic audio functionality works even with the
> nomodeset option. For that you need the kludge in patch 1. I think of
> this as a similar kludge we have for not failing modprobing i915 in
> case of nomodeset. Legacy userspace depends on this for basic graphics
> functionality and in the same way some other userspace bits may depend
> on basic audio functionality.

nomodeset isn't supported really anymore in upstream, we've thrown out the
Kconfig option needed by distros for that:

commit fd930478fb797e4cbaa799d9ddd970e9a1fa1b4a
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri Jun 19 20:27:27 2015 +0100

    drm/i915: Remove KMS Kconfig option

That was almost a year ago, and no one complained. I don't think modeset=0
is a use-case any more, and hence I don't think we need special code to
support it. If some users scream that we broke their setup we can fix this
later on, but right now I'm firmly in the "nope" territory.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, May 17, 2016 at 02:34:21PM +0200, Daniel Vetter wrote:
> On Tue, May 17, 2016 at 03:11:02PM +0300, Imre Deak wrote:
> > On ti, 2016-05-17 at 13:10 +0200, Daniel Vetter wrote:
> > > On Tue, May 17, 2016 at 01:22:52PM +0300, Imre Deak wrote:
> > > > On ti, 2016-05-17 at 11:59 +0200, Takashi Iwai wrote:
> > > > > On Tue, 17 May 2016 11:42:17 +0200,
> > > > > Daniel Vetter wrote:
> > > > > > 
> > > > > > On Tue, May 17, 2016 at 09:37:12AM +0200, Takashi Iwai wrote:
> > > > > > > On Tue, 17 May 2016 09:20:48 +0200,
> > > > > > > Daniel Vetter wrote:
> > > > > > > > 
> > > > > > > > On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > > > > > > > > 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.
> > > > > > > > > 
> > > > > > > > > v2:
> > > > > > > > > - Fail the probe in case component registration fails,
> > > > > > > > > instead of
> > > > > > > > >   suppressing the error. (Ville)
> > > > > > > > > - Register the component only for the real PCI device
> > > > > > > > > function.
> > > > > > > > > 
> > > > > > > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > > > > 
> > > > > > > > We don't support not running with modesetting. Why do we
> > > > > > > > suddenly care?
> > > > > > > 
> > > > > > > This is needed for the patch 2 and 3.  Right now we have no
> > > > > > > blocking
> > > > > > > or deferred component binding, so far, in HD-audio
> > > > > > > side.  This caused
> > > > > > > problems when async module probe was done.
> > > > > > > 
> > > > > > > So, the patch 3 implements the blocking behavior of HD-audio
> > > > > > > side.  It
> > > > > > > would lead to another regression when i915 doesn't notify its
> > > > > > > disabled
> > > > > > > state by this patch.  Otherwise the HD-audio driver will be
> > > > > > > blocked
> > > > > > > endlessly of unnecessarily long.
> > > > > > > 
> > > > > > > > Same for users creating a .config that fails to boot or
> > > > > > > > work ...
> > > > > > > 
> > > > > > > The config isn't cared much, but the problem is about the
> > > > > > > runtime boot
> > > > > > > option.
> > > > > > > 
> > > > > > > > If HDA needs to coporate with gfx to get things done, then
> > > > > > > > imo we should
> > > > > > > > just require that i915.ko is there.
> > > > > > > 
> > > > > > > Other way round: we do already require i915 in HD-audio
> > > > > > > side.  But in
> > > > > > > this case, we do *not* want to require i915 when it's
> > > > > > > disabled in
> > > > > > > runtime. 
> > > > > > 
> > > > > > That's what I mean: If you boot with i915.nomodeset you're
> > > > > > explicitly fine
> > > > > > with a somewhat non-useable system - that option is for
> > > > > > debugging only
> > > > > > really. If that means audio also doesn't work, then I think
> > > > > > that's ok.
> > > > > 
> > > > > It's not only "it doesn't work".  The module load gets stuck.  So
> > > > > we
> > > > > need some notification for the blocked component binding.
> > > > > 
> > > > > > Adding complexity for this case (which means more error paths
> > > > > > we don't
> > > > > > ever test and hence _will_ break) seems over the top.
> > > > > > 
> > > > > > I'm quite opposed to adding error handling for every condition
> > > > > > in general
> > > > > > because the combinatorial testing madness just can't be
> > > > > > handled. The one
> > > > > > exception in the i915.ko driver is that when the render side
> > > > > > died
> > > > > > (terminal gpu hang) we'll try our best to keep the display
> > > > > > alive. But
> > > > > > that's it, and the justification for that is "we want users to
> > > > > > be able to
> > > > > > get the bug report out". I don't see a justification of that
> > > > > > magnitude for
> > > > > > this feature here at all.
> > > > > 
> > > > > Well, actually the patchset was proposed just because Intel CI
> > > > > tests
> > > > > failed due to async module probes.  If Intel is happy with
> > > > > continued
> > > > > CI test failures, I'm also happy with the current situation ;)
> > > > 
> > > > It's not only due to those particular failures. That was caused by
> > > > a
> > > > kmod bug and as such would be good to not depend on that mechanism.
> > > > But
> > > > things will fail atm even in the normal case when audio is built-in 
> > > > and
> > > > i915 is a module. This patchset would solve that too.
> > > 
> > > I'm not against patch 3 from this series, which seems to be the
> > > bugfix we
> > > want. I'm against the kludge in patch 1 here only (and maybe patch 2,
> > > not
> > > sure about that). Patch 1 here looks like a workaround purely for the
> > > i915.modeset=0 case. And I don't want special code for debug knobs if
> > > we
> > > can avoid it.
> > 
> > We have discussed this in more detail, see:
> > https://bugs.freedesktop.org/show_bug.cgi?id=94566#c26
> 
> I disagree with Takashi here. modeset=0 is a debug option, and we should
> probably mark it as _unsafe.
> > 
> > The point from Takashi in that discussion was that in order to do the
> > probing via the component bind hook as we want and as patch 3 does we
> > need to make sure basic audio functionality works even with the
> > nomodeset option. For that you need the kludge in patch 1. I think of
> > this as a similar kludge we have for not failing modprobing i915 in
> > case of nomodeset. Legacy userspace depends on this for basic graphics
> > functionality and in the same way some other userspace bits may depend
> > on basic audio functionality.
> 
> nomodeset isn't supported really anymore in upstream, we've thrown out the
> Kconfig option needed by distros for that:
> 
> commit fd930478fb797e4cbaa799d9ddd970e9a1fa1b4a
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Fri Jun 19 20:27:27 2015 +0100
> 
>     drm/i915: Remove KMS Kconfig option
> 
> That was almost a year ago, and no one complained. I don't think modeset=0
> is a use-case any more, and hence I don't think we need special code to
> support it. If some users scream that we broke their setup we can fix this
> later on, but right now I'm firmly in the "nope" territory.

If audio without i915.ko is really a use-case we care about then I guess
we could make it all work if you disable CONFIG_DRM_I915. So #ifdef around
the corresponding code in patch 3. That way the complexity is in the
snd-hda driver, and sound folks can deal with it if they want this. But
adding fallback code for every totally insane combination of config
options a users might come up with is not something I like at all. At
least not in i915.ko. We have the same discussion with lack of firmware
blobs, fbc, psr, everything else. It's impossible to test all those paths
and keep them working.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, May 17, 2016 at 02:37:49PM +0200, Daniel Vetter wrote:
> On Tue, May 17, 2016 at 02:34:21PM +0200, Daniel Vetter wrote:
> > On Tue, May 17, 2016 at 03:11:02PM +0300, Imre Deak wrote:
> > > On ti, 2016-05-17 at 13:10 +0200, Daniel Vetter wrote:
> > > > On Tue, May 17, 2016 at 01:22:52PM +0300, Imre Deak wrote:
> > > > > On ti, 2016-05-17 at 11:59 +0200, Takashi Iwai wrote:
> > > > > > On Tue, 17 May 2016 11:42:17 +0200,
> > > > > > Daniel Vetter wrote:
> > > > > > > 
> > > > > > > On Tue, May 17, 2016 at 09:37:12AM +0200, Takashi Iwai wrote:
> > > > > > > > On Tue, 17 May 2016 09:20:48 +0200,
> > > > > > > > Daniel Vetter wrote:
> > > > > > > > > 
> > > > > > > > > On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > > > > > > > > > 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.
> > > > > > > > > > 
> > > > > > > > > > v2:
> > > > > > > > > > - Fail the probe in case component registration fails,
> > > > > > > > > > instead of
> > > > > > > > > >   suppressing the error. (Ville)
> > > > > > > > > > - Register the component only for the real PCI device
> > > > > > > > > > function.
> > > > > > > > > > 
> > > > > > > > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > > > > > 
> > > > > > > > > We don't support not running with modesetting. Why do we
> > > > > > > > > suddenly care?
> > > > > > > > 
> > > > > > > > This is needed for the patch 2 and 3.  Right now we have no
> > > > > > > > blocking
> > > > > > > > or deferred component binding, so far, in HD-audio
> > > > > > > > side.  This caused
> > > > > > > > problems when async module probe was done.
> > > > > > > > 
> > > > > > > > So, the patch 3 implements the blocking behavior of HD-audio
> > > > > > > > side.  It
> > > > > > > > would lead to another regression when i915 doesn't notify its
> > > > > > > > disabled
> > > > > > > > state by this patch.  Otherwise the HD-audio driver will be
> > > > > > > > blocked
> > > > > > > > endlessly of unnecessarily long.
> > > > > > > > 
> > > > > > > > > Same for users creating a .config that fails to boot or
> > > > > > > > > work ...
> > > > > > > > 
> > > > > > > > The config isn't cared much, but the problem is about the
> > > > > > > > runtime boot
> > > > > > > > option.
> > > > > > > > 
> > > > > > > > > If HDA needs to coporate with gfx to get things done, then
> > > > > > > > > imo we should
> > > > > > > > > just require that i915.ko is there.
> > > > > > > > 
> > > > > > > > Other way round: we do already require i915 in HD-audio
> > > > > > > > side.  But in
> > > > > > > > this case, we do *not* want to require i915 when it's
> > > > > > > > disabled in
> > > > > > > > runtime. 
> > > > > > > 
> > > > > > > That's what I mean: If you boot with i915.nomodeset you're
> > > > > > > explicitly fine
> > > > > > > with a somewhat non-useable system - that option is for
> > > > > > > debugging only
> > > > > > > really. If that means audio also doesn't work, then I think
> > > > > > > that's ok.
> > > > > > 
> > > > > > It's not only "it doesn't work".  The module load gets stuck.  So
> > > > > > we
> > > > > > need some notification for the blocked component binding.
> > > > > > 
> > > > > > > Adding complexity for this case (which means more error paths
> > > > > > > we don't
> > > > > > > ever test and hence _will_ break) seems over the top.
> > > > > > > 
> > > > > > > I'm quite opposed to adding error handling for every condition
> > > > > > > in general
> > > > > > > because the combinatorial testing madness just can't be
> > > > > > > handled. The one
> > > > > > > exception in the i915.ko driver is that when the render side
> > > > > > > died
> > > > > > > (terminal gpu hang) we'll try our best to keep the display
> > > > > > > alive. But
> > > > > > > that's it, and the justification for that is "we want users to
> > > > > > > be able to
> > > > > > > get the bug report out". I don't see a justification of that
> > > > > > > magnitude for
> > > > > > > this feature here at all.
> > > > > > 
> > > > > > Well, actually the patchset was proposed just because Intel CI
> > > > > > tests
> > > > > > failed due to async module probes.  If Intel is happy with
> > > > > > continued
> > > > > > CI test failures, I'm also happy with the current situation ;)
> > > > > 
> > > > > It's not only due to those particular failures. That was caused by
> > > > > a
> > > > > kmod bug and as such would be good to not depend on that mechanism.
> > > > > But
> > > > > things will fail atm even in the normal case when audio is built-in 
> > > > > and
> > > > > i915 is a module. This patchset would solve that too.
> > > > 
> > > > I'm not against patch 3 from this series, which seems to be the
> > > > bugfix we
> > > > want. I'm against the kludge in patch 1 here only (and maybe patch 2,
> > > > not
> > > > sure about that). Patch 1 here looks like a workaround purely for the
> > > > i915.modeset=0 case. And I don't want special code for debug knobs if
> > > > we
> > > > can avoid it.
> > > 
> > > We have discussed this in more detail, see:
> > > https://bugs.freedesktop.org/show_bug.cgi?id=94566#c26
> > 
> > I disagree with Takashi here. modeset=0 is a debug option, and we should
> > probably mark it as _unsafe.
> > > 
> > > The point from Takashi in that discussion was that in order to do the
> > > probing via the component bind hook as we want and as patch 3 does we
> > > need to make sure basic audio functionality works even with the
> > > nomodeset option. For that you need the kludge in patch 1. I think of
> > > this as a similar kludge we have for not failing modprobing i915 in
> > > case of nomodeset. Legacy userspace depends on this for basic graphics
> > > functionality and in the same way some other userspace bits may depend
> > > on basic audio functionality.
> > 
> > nomodeset isn't supported really anymore in upstream, we've thrown out the
> > Kconfig option needed by distros for that:
> > 
> > commit fd930478fb797e4cbaa799d9ddd970e9a1fa1b4a
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Fri Jun 19 20:27:27 2015 +0100
> > 
> >     drm/i915: Remove KMS Kconfig option
> > 
> > That was almost a year ago, and no one complained. I don't think modeset=0
> > is a use-case any more, and hence I don't think we need special code to
> > support it. If some users scream that we broke their setup we can fix this
> > later on, but right now I'm firmly in the "nope" territory.
> 
> If audio without i915.ko is really a use-case we care about then I guess
> we could make it all work if you disable CONFIG_DRM_I915. So #ifdef around
> the corresponding code in patch 3. That way the complexity is in the
> snd-hda driver, and sound folks can deal with it if they want this. But
> adding fallback code for every totally insane combination of config
> options a users might come up with is not something I like at all. At
> least not in i915.ko. We have the same discussion with lack of firmware
> blobs, fbc, psr, everything else. It's impossible to test all those paths
> and keep them working.

And if we _really_ want this to work at runtime too, then we need a
system-wide nomodeset option that snd-hda can look up and act accordingly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Tue, 17 May 2016 14:34:21 +0200,
Daniel Vetter wrote:
> 
> On Tue, May 17, 2016 at 03:11:02PM +0300, Imre Deak wrote:
> > On ti, 2016-05-17 at 13:10 +0200, Daniel Vetter wrote:
> > > On Tue, May 17, 2016 at 01:22:52PM +0300, Imre Deak wrote:
> > > > On ti, 2016-05-17 at 11:59 +0200, Takashi Iwai wrote:
> > > > > On Tue, 17 May 2016 11:42:17 +0200,
> > > > > Daniel Vetter wrote:
> > > > > > 
> > > > > > On Tue, May 17, 2016 at 09:37:12AM +0200, Takashi Iwai wrote:
> > > > > > > On Tue, 17 May 2016 09:20:48 +0200,
> > > > > > > Daniel Vetter wrote:
> > > > > > > > 
> > > > > > > > On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > > > > > > > > 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.
> > > > > > > > > 
> > > > > > > > > v2:
> > > > > > > > > - Fail the probe in case component registration fails,
> > > > > > > > > instead of
> > > > > > > > >   suppressing the error. (Ville)
> > > > > > > > > - Register the component only for the real PCI device
> > > > > > > > > function.
> > > > > > > > > 
> > > > > > > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > > > > 
> > > > > > > > We don't support not running with modesetting. Why do we
> > > > > > > > suddenly care?
> > > > > > > 
> > > > > > > This is needed for the patch 2 and 3.  Right now we have no
> > > > > > > blocking
> > > > > > > or deferred component binding, so far, in HD-audio
> > > > > > > side.  This caused
> > > > > > > problems when async module probe was done.
> > > > > > > 
> > > > > > > So, the patch 3 implements the blocking behavior of HD-audio
> > > > > > > side.  It
> > > > > > > would lead to another regression when i915 doesn't notify its
> > > > > > > disabled
> > > > > > > state by this patch.  Otherwise the HD-audio driver will be
> > > > > > > blocked
> > > > > > > endlessly of unnecessarily long.
> > > > > > > 
> > > > > > > > Same for users creating a .config that fails to boot or
> > > > > > > > work ...
> > > > > > > 
> > > > > > > The config isn't cared much, but the problem is about the
> > > > > > > runtime boot
> > > > > > > option.
> > > > > > > 
> > > > > > > > If HDA needs to coporate with gfx to get things done, then
> > > > > > > > imo we should
> > > > > > > > just require that i915.ko is there.
> > > > > > > 
> > > > > > > Other way round: we do already require i915 in HD-audio
> > > > > > > side.  But in
> > > > > > > this case, we do *not* want to require i915 when it's
> > > > > > > disabled in
> > > > > > > runtime. 
> > > > > > 
> > > > > > That's what I mean: If you boot with i915.nomodeset you're
> > > > > > explicitly fine
> > > > > > with a somewhat non-useable system - that option is for
> > > > > > debugging only
> > > > > > really. If that means audio also doesn't work, then I think
> > > > > > that's ok.
> > > > > 
> > > > > It's not only "it doesn't work".  The module load gets stuck.  So
> > > > > we
> > > > > need some notification for the blocked component binding.
> > > > > 
> > > > > > Adding complexity for this case (which means more error paths
> > > > > > we don't
> > > > > > ever test and hence _will_ break) seems over the top.
> > > > > > 
> > > > > > I'm quite opposed to adding error handling for every condition
> > > > > > in general
> > > > > > because the combinatorial testing madness just can't be
> > > > > > handled. The one
> > > > > > exception in the i915.ko driver is that when the render side
> > > > > > died
> > > > > > (terminal gpu hang) we'll try our best to keep the display
> > > > > > alive. But
> > > > > > that's it, and the justification for that is "we want users to
> > > > > > be able to
> > > > > > get the bug report out". I don't see a justification of that
> > > > > > magnitude for
> > > > > > this feature here at all.
> > > > > 
> > > > > Well, actually the patchset was proposed just because Intel CI
> > > > > tests
> > > > > failed due to async module probes.  If Intel is happy with
> > > > > continued
> > > > > CI test failures, I'm also happy with the current situation ;)
> > > > 
> > > > It's not only due to those particular failures. That was caused by
> > > > a
> > > > kmod bug and as such would be good to not depend on that mechanism.
> > > > But
> > > > things will fail atm even in the normal case when audio is built-in 
> > > > and
> > > > i915 is a module. This patchset would solve that too.
> > > 
> > > I'm not against patch 3 from this series, which seems to be the
> > > bugfix we
> > > want. I'm against the kludge in patch 1 here only (and maybe patch 2,
> > > not
> > > sure about that). Patch 1 here looks like a workaround purely for the
> > > i915.modeset=0 case. And I don't want special code for debug knobs if
> > > we
> > > can avoid it.
> > 
> > We have discussed this in more detail, see:
> > https://bugs.freedesktop.org/show_bug.cgi?id=94566#c26
> 
> I disagree with Takashi here. modeset=0 is a debug option, and we should
> probably mark it as _unsafe.

Well, even if it's a debug option, making the system unusable isn't a
nice move.  With the blocking component binding, the audio side stalls
unexpectedly (and silently).  How user would know it?

I'm no big fan of the current implementation, but I really would like
to avoid users hitting the unexpected results.

> > The point from Takashi in that discussion was that in order to do the
> > probing via the component bind hook as we want and as patch 3 does we
> > need to make sure basic audio functionality works even with the
> > nomodeset option. For that you need the kludge in patch 1. I think of
> > this as a similar kludge we have for not failing modprobing i915 in
> > case of nomodeset. Legacy userspace depends on this for basic graphics
> > functionality and in the same way some other userspace bits may depend
> > on basic audio functionality.
> 
> nomodeset isn't supported really anymore in upstream, we've thrown out the
> Kconfig option needed by distros for that:
> 
> commit fd930478fb797e4cbaa799d9ddd970e9a1fa1b4a
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Fri Jun 19 20:27:27 2015 +0100
> 
>     drm/i915: Remove KMS Kconfig option
> 
> That was almost a year ago, and no one complained. I don't think modeset=0
> is a use-case any more, and hence I don't think we need special code to
> support it. If some users scream that we broke their setup we can fix this
> later on, but right now I'm firmly in the "nope" territory.

Hmm, I thought nomodeset option still keeps working?
i915 checks vgacon_text_force(), and nomodeset is set there.


thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Tue, 17 May 2016 14:37:49 +0200,
Daniel Vetter wrote:
> 
> On Tue, May 17, 2016 at 02:34:21PM +0200, Daniel Vetter wrote:
> > On Tue, May 17, 2016 at 03:11:02PM +0300, Imre Deak wrote:
> > > On ti, 2016-05-17 at 13:10 +0200, Daniel Vetter wrote:
> > > > On Tue, May 17, 2016 at 01:22:52PM +0300, Imre Deak wrote:
> > > > > On ti, 2016-05-17 at 11:59 +0200, Takashi Iwai wrote:
> > > > > > On Tue, 17 May 2016 11:42:17 +0200,
> > > > > > Daniel Vetter wrote:
> > > > > > > 
> > > > > > > On Tue, May 17, 2016 at 09:37:12AM +0200, Takashi Iwai wrote:
> > > > > > > > On Tue, 17 May 2016 09:20:48 +0200,
> > > > > > > > Daniel Vetter wrote:
> > > > > > > > > 
> > > > > > > > > On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > > > > > > > > > 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.
> > > > > > > > > > 
> > > > > > > > > > v2:
> > > > > > > > > > - Fail the probe in case component registration fails,
> > > > > > > > > > instead of
> > > > > > > > > >   suppressing the error. (Ville)
> > > > > > > > > > - Register the component only for the real PCI device
> > > > > > > > > > function.
> > > > > > > > > > 
> > > > > > > > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > > > > > 
> > > > > > > > > We don't support not running with modesetting. Why do we
> > > > > > > > > suddenly care?
> > > > > > > > 
> > > > > > > > This is needed for the patch 2 and 3.  Right now we have no
> > > > > > > > blocking
> > > > > > > > or deferred component binding, so far, in HD-audio
> > > > > > > > side.  This caused
> > > > > > > > problems when async module probe was done.
> > > > > > > > 
> > > > > > > > So, the patch 3 implements the blocking behavior of HD-audio
> > > > > > > > side.  It
> > > > > > > > would lead to another regression when i915 doesn't notify its
> > > > > > > > disabled
> > > > > > > > state by this patch.  Otherwise the HD-audio driver will be
> > > > > > > > blocked
> > > > > > > > endlessly of unnecessarily long.
> > > > > > > > 
> > > > > > > > > Same for users creating a .config that fails to boot or
> > > > > > > > > work ...
> > > > > > > > 
> > > > > > > > The config isn't cared much, but the problem is about the
> > > > > > > > runtime boot
> > > > > > > > option.
> > > > > > > > 
> > > > > > > > > If HDA needs to coporate with gfx to get things done, then
> > > > > > > > > imo we should
> > > > > > > > > just require that i915.ko is there.
> > > > > > > > 
> > > > > > > > Other way round: we do already require i915 in HD-audio
> > > > > > > > side.  But in
> > > > > > > > this case, we do *not* want to require i915 when it's
> > > > > > > > disabled in
> > > > > > > > runtime. 
> > > > > > > 
> > > > > > > That's what I mean: If you boot with i915.nomodeset you're
> > > > > > > explicitly fine
> > > > > > > with a somewhat non-useable system - that option is for
> > > > > > > debugging only
> > > > > > > really. If that means audio also doesn't work, then I think
> > > > > > > that's ok.
> > > > > > 
> > > > > > It's not only "it doesn't work".  The module load gets stuck.  So
> > > > > > we
> > > > > > need some notification for the blocked component binding.
> > > > > > 
> > > > > > > Adding complexity for this case (which means more error paths
> > > > > > > we don't
> > > > > > > ever test and hence _will_ break) seems over the top.
> > > > > > > 
> > > > > > > I'm quite opposed to adding error handling for every condition
> > > > > > > in general
> > > > > > > because the combinatorial testing madness just can't be
> > > > > > > handled. The one
> > > > > > > exception in the i915.ko driver is that when the render side
> > > > > > > died
> > > > > > > (terminal gpu hang) we'll try our best to keep the display
> > > > > > > alive. But
> > > > > > > that's it, and the justification for that is "we want users to
> > > > > > > be able to
> > > > > > > get the bug report out". I don't see a justification of that
> > > > > > > magnitude for
> > > > > > > this feature here at all.
> > > > > > 
> > > > > > Well, actually the patchset was proposed just because Intel CI
> > > > > > tests
> > > > > > failed due to async module probes.  If Intel is happy with
> > > > > > continued
> > > > > > CI test failures, I'm also happy with the current situation ;)
> > > > > 
> > > > > It's not only due to those particular failures. That was caused by
> > > > > a
> > > > > kmod bug and as such would be good to not depend on that mechanism.
> > > > > But
> > > > > things will fail atm even in the normal case when audio is built-in 
> > > > > and
> > > > > i915 is a module. This patchset would solve that too.
> > > > 
> > > > I'm not against patch 3 from this series, which seems to be the
> > > > bugfix we
> > > > want. I'm against the kludge in patch 1 here only (and maybe patch 2,
> > > > not
> > > > sure about that). Patch 1 here looks like a workaround purely for the
> > > > i915.modeset=0 case. And I don't want special code for debug knobs if
> > > > we
> > > > can avoid it.
> > > 
> > > We have discussed this in more detail, see:
> > > https://bugs.freedesktop.org/show_bug.cgi?id=94566#c26
> > 
> > I disagree with Takashi here. modeset=0 is a debug option, and we should
> > probably mark it as _unsafe.
> > > 
> > > The point from Takashi in that discussion was that in order to do the
> > > probing via the component bind hook as we want and as patch 3 does we
> > > need to make sure basic audio functionality works even with the
> > > nomodeset option. For that you need the kludge in patch 1. I think of
> > > this as a similar kludge we have for not failing modprobing i915 in
> > > case of nomodeset. Legacy userspace depends on this for basic graphics
> > > functionality and in the same way some other userspace bits may depend
> > > on basic audio functionality.
> > 
> > nomodeset isn't supported really anymore in upstream, we've thrown out the
> > Kconfig option needed by distros for that:
> > 
> > commit fd930478fb797e4cbaa799d9ddd970e9a1fa1b4a
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Fri Jun 19 20:27:27 2015 +0100
> > 
> >     drm/i915: Remove KMS Kconfig option
> > 
> > That was almost a year ago, and no one complained. I don't think modeset=0
> > is a use-case any more, and hence I don't think we need special code to
> > support it. If some users scream that we broke their setup we can fix this
> > later on, but right now I'm firmly in the "nope" territory.
> 
> If audio without i915.ko is really a use-case we care about then I guess
> we could make it all work if you disable CONFIG_DRM_I915. So #ifdef around
> the corresponding code in patch 3. That way the complexity is in the
> snd-hda driver, and sound folks can deal with it if they want this. But
> adding fallback code for every totally insane combination of config
> options a users might come up with is not something I like at all. At
> least not in i915.ko. We have the same discussion with lack of firmware
> blobs, fbc, psr, everything else. It's impossible to test all those paths
> and keep them working.

Daniel, it's not about Kconfig dependency.  The code in question has
already the dependency and the whole file is built only when
CONFIG_DRM_I915 is enabled.

On SKL, we need to probe the whole codecs on the bus.  And if it were
a HDMI/DP codec, the power well has to be handled beforehand.  That's
why i915 component gets involved even at the simple driver probe
level of HD-audio.


Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Tue, 17 May 2016 14:39:26 +0200,
Daniel Vetter wrote:
> 
> On Tue, May 17, 2016 at 02:37:49PM +0200, Daniel Vetter wrote:
> > On Tue, May 17, 2016 at 02:34:21PM +0200, Daniel Vetter wrote:
> > > On Tue, May 17, 2016 at 03:11:02PM +0300, Imre Deak wrote:
> > > > On ti, 2016-05-17 at 13:10 +0200, Daniel Vetter wrote:
> > > > > On Tue, May 17, 2016 at 01:22:52PM +0300, Imre Deak wrote:
> > > > > > On ti, 2016-05-17 at 11:59 +0200, Takashi Iwai wrote:
> > > > > > > On Tue, 17 May 2016 11:42:17 +0200,
> > > > > > > Daniel Vetter wrote:
> > > > > > > > 
> > > > > > > > On Tue, May 17, 2016 at 09:37:12AM +0200, Takashi Iwai wrote:
> > > > > > > > > On Tue, 17 May 2016 09:20:48 +0200,
> > > > > > > > > Daniel Vetter wrote:
> > > > > > > > > > 
> > > > > > > > > > On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > > > > > > > > > > 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.
> > > > > > > > > > > 
> > > > > > > > > > > v2:
> > > > > > > > > > > - Fail the probe in case component registration fails,
> > > > > > > > > > > instead of
> > > > > > > > > > >   suppressing the error. (Ville)
> > > > > > > > > > > - Register the component only for the real PCI device
> > > > > > > > > > > function.
> > > > > > > > > > > 
> > > > > > > > > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > > > > > > 
> > > > > > > > > > We don't support not running with modesetting. Why do we
> > > > > > > > > > suddenly care?
> > > > > > > > > 
> > > > > > > > > This is needed for the patch 2 and 3.  Right now we have no
> > > > > > > > > blocking
> > > > > > > > > or deferred component binding, so far, in HD-audio
> > > > > > > > > side.  This caused
> > > > > > > > > problems when async module probe was done.
> > > > > > > > > 
> > > > > > > > > So, the patch 3 implements the blocking behavior of HD-audio
> > > > > > > > > side.  It
> > > > > > > > > would lead to another regression when i915 doesn't notify its
> > > > > > > > > disabled
> > > > > > > > > state by this patch.  Otherwise the HD-audio driver will be
> > > > > > > > > blocked
> > > > > > > > > endlessly of unnecessarily long.
> > > > > > > > > 
> > > > > > > > > > Same for users creating a .config that fails to boot or
> > > > > > > > > > work ...
> > > > > > > > > 
> > > > > > > > > The config isn't cared much, but the problem is about the
> > > > > > > > > runtime boot
> > > > > > > > > option.
> > > > > > > > > 
> > > > > > > > > > If HDA needs to coporate with gfx to get things done, then
> > > > > > > > > > imo we should
> > > > > > > > > > just require that i915.ko is there.
> > > > > > > > > 
> > > > > > > > > Other way round: we do already require i915 in HD-audio
> > > > > > > > > side.  But in
> > > > > > > > > this case, we do *not* want to require i915 when it's
> > > > > > > > > disabled in
> > > > > > > > > runtime. 
> > > > > > > > 
> > > > > > > > That's what I mean: If you boot with i915.nomodeset you're
> > > > > > > > explicitly fine
> > > > > > > > with a somewhat non-useable system - that option is for
> > > > > > > > debugging only
> > > > > > > > really. If that means audio also doesn't work, then I think
> > > > > > > > that's ok.
> > > > > > > 
> > > > > > > It's not only "it doesn't work".  The module load gets stuck.  So
> > > > > > > we
> > > > > > > need some notification for the blocked component binding.
> > > > > > > 
> > > > > > > > Adding complexity for this case (which means more error paths
> > > > > > > > we don't
> > > > > > > > ever test and hence _will_ break) seems over the top.
> > > > > > > > 
> > > > > > > > I'm quite opposed to adding error handling for every condition
> > > > > > > > in general
> > > > > > > > because the combinatorial testing madness just can't be
> > > > > > > > handled. The one
> > > > > > > > exception in the i915.ko driver is that when the render side
> > > > > > > > died
> > > > > > > > (terminal gpu hang) we'll try our best to keep the display
> > > > > > > > alive. But
> > > > > > > > that's it, and the justification for that is "we want users to
> > > > > > > > be able to
> > > > > > > > get the bug report out". I don't see a justification of that
> > > > > > > > magnitude for
> > > > > > > > this feature here at all.
> > > > > > > 
> > > > > > > Well, actually the patchset was proposed just because Intel CI
> > > > > > > tests
> > > > > > > failed due to async module probes.  If Intel is happy with
> > > > > > > continued
> > > > > > > CI test failures, I'm also happy with the current situation ;)
> > > > > > 
> > > > > > It's not only due to those particular failures. That was caused by
> > > > > > a
> > > > > > kmod bug and as such would be good to not depend on that mechanism.
> > > > > > But
> > > > > > things will fail atm even in the normal case when audio is built-in 
> > > > > > and
> > > > > > i915 is a module. This patchset would solve that too.
> > > > > 
> > > > > I'm not against patch 3 from this series, which seems to be the
> > > > > bugfix we
> > > > > want. I'm against the kludge in patch 1 here only (and maybe patch 2,
> > > > > not
> > > > > sure about that). Patch 1 here looks like a workaround purely for the
> > > > > i915.modeset=0 case. And I don't want special code for debug knobs if
> > > > > we
> > > > > can avoid it.
> > > > 
> > > > We have discussed this in more detail, see:
> > > > https://bugs.freedesktop.org/show_bug.cgi?id=94566#c26
> > > 
> > > I disagree with Takashi here. modeset=0 is a debug option, and we should
> > > probably mark it as _unsafe.
> > > > 
> > > > The point from Takashi in that discussion was that in order to do the
> > > > probing via the component bind hook as we want and as patch 3 does we
> > > > need to make sure basic audio functionality works even with the
> > > > nomodeset option. For that you need the kludge in patch 1. I think of
> > > > this as a similar kludge we have for not failing modprobing i915 in
> > > > case of nomodeset. Legacy userspace depends on this for basic graphics
> > > > functionality and in the same way some other userspace bits may depend
> > > > on basic audio functionality.
> > > 
> > > nomodeset isn't supported really anymore in upstream, we've thrown out the
> > > Kconfig option needed by distros for that:
> > > 
> > > commit fd930478fb797e4cbaa799d9ddd970e9a1fa1b4a
> > > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > > Date:   Fri Jun 19 20:27:27 2015 +0100
> > > 
> > >     drm/i915: Remove KMS Kconfig option
> > > 
> > > That was almost a year ago, and no one complained. I don't think modeset=0
> > > is a use-case any more, and hence I don't think we need special code to
> > > support it. If some users scream that we broke their setup we can fix this
> > > later on, but right now I'm firmly in the "nope" territory.
> > 
> > If audio without i915.ko is really a use-case we care about then I guess
> > we could make it all work if you disable CONFIG_DRM_I915. So #ifdef around
> > the corresponding code in patch 3. That way the complexity is in the
> > snd-hda driver, and sound folks can deal with it if they want this. But
> > adding fallback code for every totally insane combination of config
> > options a users might come up with is not something I like at all. At
> > least not in i915.ko. We have the same discussion with lack of firmware
> > blobs, fbc, psr, everything else. It's impossible to test all those paths
> > and keep them working.
> 
> And if we _really_ want this to work at runtime too, then we need a
> system-wide nomodeset option that snd-hda can look up and act accordingly.

Yes, this would be one option.


Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, May 17, 2016 at 02:47:44PM +0200, Takashi Iwai wrote:
> On Tue, 17 May 2016 14:34:21 +0200,
> Daniel Vetter wrote:
> > 
> > On Tue, May 17, 2016 at 03:11:02PM +0300, Imre Deak wrote:
> > > On ti, 2016-05-17 at 13:10 +0200, Daniel Vetter wrote:
> > > > On Tue, May 17, 2016 at 01:22:52PM +0300, Imre Deak wrote:
> > > > > On ti, 2016-05-17 at 11:59 +0200, Takashi Iwai wrote:
> > > > > > On Tue, 17 May 2016 11:42:17 +0200,
> > > > > > Daniel Vetter wrote:
> > > > > > > 
> > > > > > > On Tue, May 17, 2016 at 09:37:12AM +0200, Takashi Iwai wrote:
> > > > > > > > On Tue, 17 May 2016 09:20:48 +0200,
> > > > > > > > Daniel Vetter wrote:
> > > > > > > > > 
> > > > > > > > > On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > > > > > > > > > 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.
> > > > > > > > > > 
> > > > > > > > > > v2:
> > > > > > > > > > - Fail the probe in case component registration fails,
> > > > > > > > > > instead of
> > > > > > > > > >   suppressing the error. (Ville)
> > > > > > > > > > - Register the component only for the real PCI device
> > > > > > > > > > function.
> > > > > > > > > > 
> > > > > > > > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > > > > > 
> > > > > > > > > We don't support not running with modesetting. Why do we
> > > > > > > > > suddenly care?
> > > > > > > > 
> > > > > > > > This is needed for the patch 2 and 3.  Right now we have no
> > > > > > > > blocking
> > > > > > > > or deferred component binding, so far, in HD-audio
> > > > > > > > side.  This caused
> > > > > > > > problems when async module probe was done.
> > > > > > > > 
> > > > > > > > So, the patch 3 implements the blocking behavior of HD-audio
> > > > > > > > side.  It
> > > > > > > > would lead to another regression when i915 doesn't notify its
> > > > > > > > disabled
> > > > > > > > state by this patch.  Otherwise the HD-audio driver will be
> > > > > > > > blocked
> > > > > > > > endlessly of unnecessarily long.
> > > > > > > > 
> > > > > > > > > Same for users creating a .config that fails to boot or
> > > > > > > > > work ...
> > > > > > > > 
> > > > > > > > The config isn't cared much, but the problem is about the
> > > > > > > > runtime boot
> > > > > > > > option.
> > > > > > > > 
> > > > > > > > > If HDA needs to coporate with gfx to get things done, then
> > > > > > > > > imo we should
> > > > > > > > > just require that i915.ko is there.
> > > > > > > > 
> > > > > > > > Other way round: we do already require i915 in HD-audio
> > > > > > > > side.  But in
> > > > > > > > this case, we do *not* want to require i915 when it's
> > > > > > > > disabled in
> > > > > > > > runtime. 
> > > > > > > 
> > > > > > > That's what I mean: If you boot with i915.nomodeset you're
> > > > > > > explicitly fine
> > > > > > > with a somewhat non-useable system - that option is for
> > > > > > > debugging only
> > > > > > > really. If that means audio also doesn't work, then I think
> > > > > > > that's ok.
> > > > > > 
> > > > > > It's not only "it doesn't work".  The module load gets stuck.  So
> > > > > > we
> > > > > > need some notification for the blocked component binding.
> > > > > > 
> > > > > > > Adding complexity for this case (which means more error paths
> > > > > > > we don't
> > > > > > > ever test and hence _will_ break) seems over the top.
> > > > > > > 
> > > > > > > I'm quite opposed to adding error handling for every condition
> > > > > > > in general
> > > > > > > because the combinatorial testing madness just can't be
> > > > > > > handled. The one
> > > > > > > exception in the i915.ko driver is that when the render side
> > > > > > > died
> > > > > > > (terminal gpu hang) we'll try our best to keep the display
> > > > > > > alive. But
> > > > > > > that's it, and the justification for that is "we want users to
> > > > > > > be able to
> > > > > > > get the bug report out". I don't see a justification of that
> > > > > > > magnitude for
> > > > > > > this feature here at all.
> > > > > > 
> > > > > > Well, actually the patchset was proposed just because Intel CI
> > > > > > tests
> > > > > > failed due to async module probes.  If Intel is happy with
> > > > > > continued
> > > > > > CI test failures, I'm also happy with the current situation ;)
> > > > > 
> > > > > It's not only due to those particular failures. That was caused by
> > > > > a
> > > > > kmod bug and as such would be good to not depend on that mechanism.
> > > > > But
> > > > > things will fail atm even in the normal case when audio is built-in 
> > > > > and
> > > > > i915 is a module. This patchset would solve that too.
> > > > 
> > > > I'm not against patch 3 from this series, which seems to be the
> > > > bugfix we
> > > > want. I'm against the kludge in patch 1 here only (and maybe patch 2,
> > > > not
> > > > sure about that). Patch 1 here looks like a workaround purely for the
> > > > i915.modeset=0 case. And I don't want special code for debug knobs if
> > > > we
> > > > can avoid it.
> > > 
> > > We have discussed this in more detail, see:
> > > https://bugs.freedesktop.org/show_bug.cgi?id=94566#c26
> > 
> > I disagree with Takashi here. modeset=0 is a debug option, and we should
> > probably mark it as _unsafe.
> 
> Well, even if it's a debug option, making the system unusable isn't a
> nice move.  With the blocking component binding, the audio side stalls
> unexpectedly (and silently).  How user would know it?
> 
> I'm no big fan of the current implementation, but I really would like
> to avoid users hitting the unexpected results.

Ok, I looked at patch 3, and that indeed would lead to trouble without
patch 1. But the real trouble is the unconditional wait_completion in
there - blocking for another driver to complete loading from a driver load
function is a no-go. The correct way to do this is to bail out with
EPROBE_DEFER if not all the parts are available there. Also, throw out
that request_module.

By bailing out with EPROBE_DEFER you avoid deadlocks, and the driver core
also knows what's going on. Which is incidentally what's used to
implicitly order suspend/resume. Driver core will restart your probe as
soon as some new devices/drivers have registers (assuming that hopefully
then you're unblocking), but if you're unlucky your driver can go through
that loop a few times.

But that was just a very quick look, we definitely shouldn't need any
wait_completion in driver load to handle cross-module depencies.
 
> > > The point from Takashi in that discussion was that in order to do the
> > > probing via the component bind hook as we want and as patch 3 does we
> > > need to make sure basic audio functionality works even with the
> > > nomodeset option. For that you need the kludge in patch 1. I think of
> > > this as a similar kludge we have for not failing modprobing i915 in
> > > case of nomodeset. Legacy userspace depends on this for basic graphics
> > > functionality and in the same way some other userspace bits may depend
> > > on basic audio functionality.
> > 
> > nomodeset isn't supported really anymore in upstream, we've thrown out the
> > Kconfig option needed by distros for that:
> > 
> > commit fd930478fb797e4cbaa799d9ddd970e9a1fa1b4a
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Fri Jun 19 20:27:27 2015 +0100
> > 
> >     drm/i915: Remove KMS Kconfig option
> > 
> > That was almost a year ago, and no one complained. I don't think modeset=0
> > is a use-case any more, and hence I don't think we need special code to
> > support it. If some users scream that we broke their setup we can fix this
> > later on, but right now I'm firmly in the "nope" territory.
> 
> Hmm, I thought nomodeset option still keeps working?
> i915 checks vgacon_text_force(), and nomodeset is set there.

Yes, but a debug option only. We have lots of those, and many of them
break your system if you touch them and don't know what to you're doing
;-)
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Tue, 17 May 2016 14:56:16 +0200,
Daniel Vetter wrote:
> 
> On Tue, May 17, 2016 at 02:47:44PM +0200, Takashi Iwai wrote:
> > On Tue, 17 May 2016 14:34:21 +0200,
> > Daniel Vetter wrote:
> > > 
> > > On Tue, May 17, 2016 at 03:11:02PM +0300, Imre Deak wrote:
> > > > On ti, 2016-05-17 at 13:10 +0200, Daniel Vetter wrote:
> > > > > On Tue, May 17, 2016 at 01:22:52PM +0300, Imre Deak wrote:
> > > > > > On ti, 2016-05-17 at 11:59 +0200, Takashi Iwai wrote:
> > > > > > > On Tue, 17 May 2016 11:42:17 +0200,
> > > > > > > Daniel Vetter wrote:
> > > > > > > > 
> > > > > > > > On Tue, May 17, 2016 at 09:37:12AM +0200, Takashi Iwai wrote:
> > > > > > > > > On Tue, 17 May 2016 09:20:48 +0200,
> > > > > > > > > Daniel Vetter wrote:
> > > > > > > > > > 
> > > > > > > > > > On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote:
> > > > > > > > > > > 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.
> > > > > > > > > > > 
> > > > > > > > > > > v2:
> > > > > > > > > > > - Fail the probe in case component registration fails,
> > > > > > > > > > > instead of
> > > > > > > > > > >   suppressing the error. (Ville)
> > > > > > > > > > > - Register the component only for the real PCI device
> > > > > > > > > > > function.
> > > > > > > > > > > 
> > > > > > > > > > > CC: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > > > > > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > > > > > > > > > 
> > > > > > > > > > We don't support not running with modesetting. Why do we
> > > > > > > > > > suddenly care?
> > > > > > > > > 
> > > > > > > > > This is needed for the patch 2 and 3.  Right now we have no
> > > > > > > > > blocking
> > > > > > > > > or deferred component binding, so far, in HD-audio
> > > > > > > > > side.  This caused
> > > > > > > > > problems when async module probe was done.
> > > > > > > > > 
> > > > > > > > > So, the patch 3 implements the blocking behavior of HD-audio
> > > > > > > > > side.  It
> > > > > > > > > would lead to another regression when i915 doesn't notify its
> > > > > > > > > disabled
> > > > > > > > > state by this patch.  Otherwise the HD-audio driver will be
> > > > > > > > > blocked
> > > > > > > > > endlessly of unnecessarily long.
> > > > > > > > > 
> > > > > > > > > > Same for users creating a .config that fails to boot or
> > > > > > > > > > work ...
> > > > > > > > > 
> > > > > > > > > The config isn't cared much, but the problem is about the
> > > > > > > > > runtime boot
> > > > > > > > > option.
> > > > > > > > > 
> > > > > > > > > > If HDA needs to coporate with gfx to get things done, then
> > > > > > > > > > imo we should
> > > > > > > > > > just require that i915.ko is there.
> > > > > > > > > 
> > > > > > > > > Other way round: we do already require i915 in HD-audio
> > > > > > > > > side.  But in
> > > > > > > > > this case, we do *not* want to require i915 when it's
> > > > > > > > > disabled in
> > > > > > > > > runtime. 
> > > > > > > > 
> > > > > > > > That's what I mean: If you boot with i915.nomodeset you're
> > > > > > > > explicitly fine
> > > > > > > > with a somewhat non-useable system - that option is for
> > > > > > > > debugging only
> > > > > > > > really. If that means audio also doesn't work, then I think
> > > > > > > > that's ok.
> > > > > > > 
> > > > > > > It's not only "it doesn't work".  The module load gets stuck.  So
> > > > > > > we
> > > > > > > need some notification for the blocked component binding.
> > > > > > > 
> > > > > > > > Adding complexity for this case (which means more error paths
> > > > > > > > we don't
> > > > > > > > ever test and hence _will_ break) seems over the top.
> > > > > > > > 
> > > > > > > > I'm quite opposed to adding error handling for every condition
> > > > > > > > in general
> > > > > > > > because the combinatorial testing madness just can't be
> > > > > > > > handled. The one
> > > > > > > > exception in the i915.ko driver is that when the render side
> > > > > > > > died
> > > > > > > > (terminal gpu hang) we'll try our best to keep the display
> > > > > > > > alive. But
> > > > > > > > that's it, and the justification for that is "we want users to
> > > > > > > > be able to
> > > > > > > > get the bug report out". I don't see a justification of that
> > > > > > > > magnitude for
> > > > > > > > this feature here at all.
> > > > > > > 
> > > > > > > Well, actually the patchset was proposed just because Intel CI
> > > > > > > tests
> > > > > > > failed due to async module probes.  If Intel is happy with
> > > > > > > continued
> > > > > > > CI test failures, I'm also happy with the current situation ;)
> > > > > > 
> > > > > > It's not only due to those particular failures. That was caused by
> > > > > > a
> > > > > > kmod bug and as such would be good to not depend on that mechanism.
> > > > > > But
> > > > > > things will fail atm even in the normal case when audio is built-in 
> > > > > > and
> > > > > > i915 is a module. This patchset would solve that too.
> > > > > 
> > > > > I'm not against patch 3 from this series, which seems to be the
> > > > > bugfix we
> > > > > want. I'm against the kludge in patch 1 here only (and maybe patch 2,
> > > > > not
> > > > > sure about that). Patch 1 here looks like a workaround purely for the
> > > > > i915.modeset=0 case. And I don't want special code for debug knobs if
> > > > > we
> > > > > can avoid it.
> > > > 
> > > > We have discussed this in more detail, see:
> > > > https://bugs.freedesktop.org/show_bug.cgi?id=94566#c26
> > > 
> > > I disagree with Takashi here. modeset=0 is a debug option, and we should
> > > probably mark it as _unsafe.
> > 
> > Well, even if it's a debug option, making the system unusable isn't a
> > nice move.  With the blocking component binding, the audio side stalls
> > unexpectedly (and silently).  How user would know it?
> > 
> > I'm no big fan of the current implementation, but I really would like
> > to avoid users hitting the unexpected results.
> 
> Ok, I looked at patch 3, and that indeed would lead to trouble without
> patch 1. But the real trouble is the unconditional wait_completion in
> there - blocking for another driver to complete loading from a driver load
> function is a no-go. The correct way to do this is to bail out with
> EPROBE_DEFER if not all the parts are available there. Also, throw out
> that request_module.
> 
> By bailing out with EPROBE_DEFER you avoid deadlocks, and the driver core
> also knows what's going on. Which is incidentally what's used to
> implicitly order suspend/resume. Driver core will restart your probe as
> soon as some new devices/drivers have registers (assuming that hopefully
> then you're unblocking), but if you're unlucky your driver can go through
> that loop a few times.
> 
> But that was just a very quick look, we definitely shouldn't need any
> wait_completion in driver load to handle cross-module depencies.

Yeah, I admit that wait_completion() is hackish.  OTOH, EPROBE_DEFER
doesn't work in the case of HD-audio because we want to give up
binding and continue without i915 but only with onboard audio, instead
of endlessly reprobing for the never-appearing component.  The i915
binding is no hard dependency; i.e. it isn't (always) mandatory, and
EPROBE_DEFER can't handle such a fallback, AFAIK.

If there is a good way to deal with it, please let me know.  I'd love
to rewrite to a cleaner way.


thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Tue, 17 May 2016 14:53:40 +0200,
Takashi Iwai wrote:
> 
> On Tue, 17 May 2016 14:39:26 +0200,
> Daniel Vetter wrote:
> > 
> > And if we _really_ want this to work at runtime too, then we need a
> > system-wide nomodeset option that snd-hda can look up and act accordingly.
> 
> Yes, this would be one option.

And I noticed that this can be easily added to patch 3, just like i915
driver does:

diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
index c9af022676c2..c48a014be7b4 100644
--- a/sound/hda/hdac_i915.c
+++ b/sound/hda/hdac_i915.c
@@ -361,6 +361,11 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
 	if (WARN_ON(hdac_acomp))
 		return -EBUSY;
 
+#ifdef CONFIG_VGA_CONSOLE
+	if (vgacon_text_force())
+		return -ENODEV;
+#endif
+
 	if (!i915_gfx_present())
 		return -ENODEV;
 

It's hackish, but should work practically enough...


Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, May 17, 2016 at 03:57:30PM +0200, Takashi Iwai wrote:
> On Tue, 17 May 2016 14:53:40 +0200,
> Takashi Iwai wrote:
> > 
> > On Tue, 17 May 2016 14:39:26 +0200,
> > Daniel Vetter wrote:
> > > 
> > > And if we _really_ want this to work at runtime too, then we need a
> > > system-wide nomodeset option that snd-hda can look up and act accordingly.
> > 
> > Yes, this would be one option.
> 
> And I noticed that this can be easily added to patch 3, just like i915
> driver does:
> 
> diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> index c9af022676c2..c48a014be7b4 100644
> --- a/sound/hda/hdac_i915.c
> +++ b/sound/hda/hdac_i915.c
> @@ -361,6 +361,11 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
>  	if (WARN_ON(hdac_acomp))
>  		return -EBUSY;
>  
> +#ifdef CONFIG_VGA_CONSOLE
> +	if (vgacon_text_force())
> +		return -ENODEV;
> +#endif

There's a dummy vgacon_text_force now, so you don't even need the #ifdef.
-Daniel

> +
>  	if (!i915_gfx_present())
>  		return -ENODEV;
>  
> 
> It's hackish, but should work practically enough...
> 
> 
> Takashi

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

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

[Takashi Iwai]

On Tue, 17 May 2016 15:59:17 +0200,
Daniel Vetter wrote:
> 
> On Tue, May 17, 2016 at 03:57:30PM +0200, Takashi Iwai wrote:
> > On Tue, 17 May 2016 14:53:40 +0200,
> > Takashi Iwai wrote:
> > > 
> > > On Tue, 17 May 2016 14:39:26 +0200,
> > > Daniel Vetter wrote:
> > > > 
> > > > And if we _really_ want this to work at runtime too, then we need a
> > > > system-wide nomodeset option that snd-hda can look up and act accordingly.
> > > 
> > > Yes, this would be one option.
> > 
> > And I noticed that this can be easily added to patch 3, just like i915
> > driver does:
> > 
> > diff --git a/sound/hda/hdac_i915.c b/sound/hda/hdac_i915.c
> > index c9af022676c2..c48a014be7b4 100644
> > --- a/sound/hda/hdac_i915.c
> > +++ b/sound/hda/hdac_i915.c
> > @@ -361,6 +361,11 @@ int snd_hdac_i915_init(struct hdac_bus *bus)
> >  	if (WARN_ON(hdac_acomp))
> >  		return -EBUSY;
> >  
> > +#ifdef CONFIG_VGA_CONSOLE
> > +	if (vgacon_text_force())
> > +		return -ENODEV;
> > +#endif
> 
> There's a dummy vgacon_text_force now, so you don't even need the #ifdef.

Good to know!


Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, May 17, 2016 at 3:20 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> Ok, I looked at patch 3, and that indeed would lead to trouble without
>> patch 1. But the real trouble is the unconditional wait_completion in
>> there - blocking for another driver to complete loading from a driver load
>> function is a no-go. The correct way to do this is to bail out with
>> EPROBE_DEFER if not all the parts are available there. Also, throw out
>> that request_module.
>>
>> By bailing out with EPROBE_DEFER you avoid deadlocks, and the driver core
>> also knows what's going on. Which is incidentally what's used to
>> implicitly order suspend/resume. Driver core will restart your probe as
>> soon as some new devices/drivers have registers (assuming that hopefully
>> then you're unblocking), but if you're unlucky your driver can go through
>> that loop a few times.
>>
>> But that was just a very quick look, we definitely shouldn't need any
>> wait_completion in driver load to handle cross-module depencies.
>
> Yeah, I admit that wait_completion() is hackish.  OTOH, EPROBE_DEFER
> doesn't work in the case of HD-audio because we want to give up
> binding and continue without i915 but only with onboard audio, instead
> of endlessly reprobing for the never-appearing component.  The i915
> binding is no hard dependency; i.e. it isn't (always) mandatory, and
> EPROBE_DEFER can't handle such a fallback, AFAIK.
>
> If there is a good way to deal with it, please let me know.  I'd love
> to rewrite to a cleaner way.

The only way to deal with that is to split the driver into two, and
hotplug them individually. Fundamentally any approach where you need
to know whether i915 shows up or not and act accordingly is just plain
flawed, there's no way around it. That's also why EPROBE_DEFER doesn't
bother dealing with it.

Imo if you have the sound side of hdmi/dp audio, then just
EPROBE_DEFER until i915 is loaded (assuming it's not disabled through
nomodeset or Kconfig). If it's not there then continue without it (and
without hdmi/dp audio ofc). Trying to be clever just means we need to
hand roll things all over the place all the time. We have some code on
earlier platforms for runtime clock adjustements (on ironlake) in
i915.ko, and I really don't want that kind of hacks any more.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, May 17, 2016 at 6:18 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, May 17, 2016 at 3:20 PM, Takashi Iwai <tiwai@suse.de> wrote:
>>> Ok, I looked at patch 3, and that indeed would lead to trouble without
>>> patch 1. But the real trouble is the unconditional wait_completion in
>>> there - blocking for another driver to complete loading from a driver load
>>> function is a no-go. The correct way to do this is to bail out with
>>> EPROBE_DEFER if not all the parts are available there. Also, throw out
>>> that request_module.
>>>
>>> By bailing out with EPROBE_DEFER you avoid deadlocks, and the driver core
>>> also knows what's going on. Which is incidentally what's used to
>>> implicitly order suspend/resume. Driver core will restart your probe as
>>> soon as some new devices/drivers have registers (assuming that hopefully
>>> then you're unblocking), but if you're unlucky your driver can go through
>>> that loop a few times.
>>>
>>> But that was just a very quick look, we definitely shouldn't need any
>>> wait_completion in driver load to handle cross-module depencies.
>>
>> Yeah, I admit that wait_completion() is hackish.  OTOH, EPROBE_DEFER
>> doesn't work in the case of HD-audio because we want to give up
>> binding and continue without i915 but only with onboard audio, instead
>> of endlessly reprobing for the never-appearing component.  The i915
>> binding is no hard dependency; i.e. it isn't (always) mandatory, and
>> EPROBE_DEFER can't handle such a fallback, AFAIK.
>>
>> If there is a good way to deal with it, please let me know.  I'd love
>> to rewrite to a cleaner way.
>
> The only way to deal with that is to split the driver into two, and
> hotplug them individually. Fundamentally any approach where you need
> to know whether i915 shows up or not and act accordingly is just plain
> flawed, there's no way around it. That's also why EPROBE_DEFER doesn't
> bother dealing with it.
>
> Imo if you have the sound side of hdmi/dp audio, then just
> EPROBE_DEFER until i915 is loaded (assuming it's not disabled through
> nomodeset or Kconfig). If it's not there then continue without it (and
> without hdmi/dp audio ofc). Trying to be clever just means we need to
> hand roll things all over the place all the time. We have some code on
> earlier platforms for runtime clock adjustements (on ironlake) in
> i915.ko, and I really don't want that kind of hacks any more.

In case you don't believe me that your hack is broken: I often boot
with i915 blacklisted, so that I can set up netconsole and other
instrumenting and then load it again with modeset. Until that's done
snd-hda will be stuck in that wait_completion. You really can't ever
know when userspace or the user decides to finally load the driver,
and the only reasonable thing to do is to defer until everything you
need is there. Except of course when the user told you it's not going
to show up through nomodeset or Kconfig knobs, but that's kinda the
exception.

Imo the best course forward would be:
- Implement EPROBE_DEFER correctly in snd-hda (i.e. no
wait_completion, no deferred work or anything like that, just return
-EPROBE_DEFER when i915 isn't there yet).
- Add a bail-out option if CONFIG_DRM_I915 isn't enabled to snd-hda.
- Add a runtime bail-out for nomodeset/vgacon_text_force() to snd-hda.

That should cover everyone's debug needs, while giving us a clean
architecture moving forward. Thoughts?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Tue, 17 May 2016 18:18:27 +0200,
Daniel Vetter wrote:
> 
> On Tue, May 17, 2016 at 3:20 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> Ok, I looked at patch 3, and that indeed would lead to trouble without
> >> patch 1. But the real trouble is the unconditional wait_completion in
> >> there - blocking for another driver to complete loading from a driver load
> >> function is a no-go. The correct way to do this is to bail out with
> >> EPROBE_DEFER if not all the parts are available there. Also, throw out
> >> that request_module.
> >>
> >> By bailing out with EPROBE_DEFER you avoid deadlocks, and the driver core
> >> also knows what's going on. Which is incidentally what's used to
> >> implicitly order suspend/resume. Driver core will restart your probe as
> >> soon as some new devices/drivers have registers (assuming that hopefully
> >> then you're unblocking), but if you're unlucky your driver can go through
> >> that loop a few times.
> >>
> >> But that was just a very quick look, we definitely shouldn't need any
> >> wait_completion in driver load to handle cross-module depencies.
> >
> > Yeah, I admit that wait_completion() is hackish.  OTOH, EPROBE_DEFER
> > doesn't work in the case of HD-audio because we want to give up
> > binding and continue without i915 but only with onboard audio, instead
> > of endlessly reprobing for the never-appearing component.  The i915
> > binding is no hard dependency; i.e. it isn't (always) mandatory, and
> > EPROBE_DEFER can't handle such a fallback, AFAIK.
> >
> > If there is a good way to deal with it, please let me know.  I'd love
> > to rewrite to a cleaner way.
> 
> The only way to deal with that is to split the driver into two, and
> hotplug them individually. Fundamentally any approach where you need
> to know whether i915 shows up or not and act accordingly is just plain
> flawed, there's no way around it. That's also why EPROBE_DEFER doesn't
> bother dealing with it.
> 
> Imo if you have the sound side of hdmi/dp audio, then just
> EPROBE_DEFER until i915 is loaded (assuming it's not disabled through
> nomodeset or Kconfig).

One of the problems that make things complicated is: we don't know
whether there is HDMI/DP codec present before probing on the bus.
SKL has a common HD-audio bus, and it might be that there is no i915
HDMI codec on it but only the onboard analog codec.  However, for
probing the HDMI codec itself, i915 has to be initialized beforehand.

So, your assumption "if you have the sound side of hdmi/dp audio":
this can't be known before i915 is loaded and the HDMI/DP audio is
probed.  It's a kind of chicken and egg problem.

Yes, we can know at least whether i915 graphics is present on PCI bus,
and we already check it for Nvidia/AMD hybrid systems.  But it doesn't
give a chance to know whether i915 gfx is available at all (nomodeset
or whatever disablement).  This is why the whole this patchset was
invented.

The rest -- how to bind things -- is a matter of implementation
details.  Once when we have a material, we can write more elegantly.

But, if there is no disablement notification, EPROBE_DEFER can't work
well with the fallback mechanism.

> If it's not there then continue without it (and
> without hdmi/dp audio ofc). Trying to be clever just means we need to
> hand roll things all over the place all the time. We have some code on
> earlier platforms for runtime clock adjustements (on ironlake) in
> i915.ko, and I really don't want that kind of hacks any more.

Yes, I know of the pains.  A better implementation is always welcome,
and I'm not willing to stick with my current patches.


Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Tue, 17 May 2016 18:23:40 +0200,
Daniel Vetter wrote:
> 
> On Tue, May 17, 2016 at 6:18 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, May 17, 2016 at 3:20 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >>> Ok, I looked at patch 3, and that indeed would lead to trouble without
> >>> patch 1. But the real trouble is the unconditional wait_completion in
> >>> there - blocking for another driver to complete loading from a driver load
> >>> function is a no-go. The correct way to do this is to bail out with
> >>> EPROBE_DEFER if not all the parts are available there. Also, throw out
> >>> that request_module.
> >>>
> >>> By bailing out with EPROBE_DEFER you avoid deadlocks, and the driver core
> >>> also knows what's going on. Which is incidentally what's used to
> >>> implicitly order suspend/resume. Driver core will restart your probe as
> >>> soon as some new devices/drivers have registers (assuming that hopefully
> >>> then you're unblocking), but if you're unlucky your driver can go through
> >>> that loop a few times.
> >>>
> >>> But that was just a very quick look, we definitely shouldn't need any
> >>> wait_completion in driver load to handle cross-module depencies.
> >>
> >> Yeah, I admit that wait_completion() is hackish.  OTOH, EPROBE_DEFER
> >> doesn't work in the case of HD-audio because we want to give up
> >> binding and continue without i915 but only with onboard audio, instead
> >> of endlessly reprobing for the never-appearing component.  The i915
> >> binding is no hard dependency; i.e. it isn't (always) mandatory, and
> >> EPROBE_DEFER can't handle such a fallback, AFAIK.
> >>
> >> If there is a good way to deal with it, please let me know.  I'd love
> >> to rewrite to a cleaner way.
> >
> > The only way to deal with that is to split the driver into two, and
> > hotplug them individually. Fundamentally any approach where you need
> > to know whether i915 shows up or not and act accordingly is just plain
> > flawed, there's no way around it. That's also why EPROBE_DEFER doesn't
> > bother dealing with it.
> >
> > Imo if you have the sound side of hdmi/dp audio, then just
> > EPROBE_DEFER until i915 is loaded (assuming it's not disabled through
> > nomodeset or Kconfig). If it's not there then continue without it (and
> > without hdmi/dp audio ofc). Trying to be clever just means we need to
> > hand roll things all over the place all the time. We have some code on
> > earlier platforms for runtime clock adjustements (on ironlake) in
> > i915.ko, and I really don't want that kind of hacks any more.
> 
> In case you don't believe me that your hack is broken: I often boot
> with i915 blacklisted, so that I can set up netconsole and other
> instrumenting and then load it again with modeset. Until that's done
> snd-hda will be stuck in that wait_completion.

Yes, but it has a fallback after some long timeout.  Then at least
user would see if binding failure happens and going without HDMI/DP.

> You really can't ever
> know when userspace or the user decides to finally load the driver,
> and the only reasonable thing to do is to defer until everything you
> need is there. Except of course when the user told you it's not going
> to show up through nomodeset or Kconfig knobs, but that's kinda the
> exception.
> 
> Imo the best course forward would be:
> - Implement EPROBE_DEFER correctly in snd-hda (i.e. no
> wait_completion, no deferred work or anything like that, just return
> -EPROBE_DEFER when i915 isn't there yet).

We actually don't need EPROBE_DEFER, either.  The component master
bind would take care of that already.  I used wait_for_completion()
just because I didn't want the hda-i915 binder API function.  If it
takes the function to be continued, master binder can call it after
binding with i915 properly.

> - Add a bail-out option if CONFIG_DRM_I915 isn't enabled to snd-hda.

You can forget about Kconfig.  It's already handled.  No relevant code
will be executed when CONFIG_DRM_I915 isn't set.

> - Add a runtime bail-out for nomodeset/vgacon_text_force() to snd-hda.

Yes, that's an easy part.

> That should cover everyone's debug needs, while giving us a clean
> architecture moving forward. Thoughts?

But this doesn't cover all cases.  As you mentioned,  what if user
would blacklist i915, or set i915.modeset=0?  In my patch, HD-audio
tries to continue at least some timeout.


thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, May 17, 2016 at 08:19:32PM +0200, Takashi Iwai wrote:
> On Tue, 17 May 2016 18:23:40 +0200,
> Daniel Vetter wrote:
> > 
> > On Tue, May 17, 2016 at 6:18 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Tue, May 17, 2016 at 3:20 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > >>> Ok, I looked at patch 3, and that indeed would lead to trouble without
> > >>> patch 1. But the real trouble is the unconditional wait_completion in
> > >>> there - blocking for another driver to complete loading from a driver load
> > >>> function is a no-go. The correct way to do this is to bail out with
> > >>> EPROBE_DEFER if not all the parts are available there. Also, throw out
> > >>> that request_module.
> > >>>
> > >>> By bailing out with EPROBE_DEFER you avoid deadlocks, and the driver core
> > >>> also knows what's going on. Which is incidentally what's used to
> > >>> implicitly order suspend/resume. Driver core will restart your probe as
> > >>> soon as some new devices/drivers have registers (assuming that hopefully
> > >>> then you're unblocking), but if you're unlucky your driver can go through
> > >>> that loop a few times.
> > >>>
> > >>> But that was just a very quick look, we definitely shouldn't need any
> > >>> wait_completion in driver load to handle cross-module depencies.
> > >>
> > >> Yeah, I admit that wait_completion() is hackish.  OTOH, EPROBE_DEFER
> > >> doesn't work in the case of HD-audio because we want to give up
> > >> binding and continue without i915 but only with onboard audio, instead
> > >> of endlessly reprobing for the never-appearing component.  The i915
> > >> binding is no hard dependency; i.e. it isn't (always) mandatory, and
> > >> EPROBE_DEFER can't handle such a fallback, AFAIK.
> > >>
> > >> If there is a good way to deal with it, please let me know.  I'd love
> > >> to rewrite to a cleaner way.
> > >
> > > The only way to deal with that is to split the driver into two, and
> > > hotplug them individually. Fundamentally any approach where you need
> > > to know whether i915 shows up or not and act accordingly is just plain
> > > flawed, there's no way around it. That's also why EPROBE_DEFER doesn't
> > > bother dealing with it.
> > >
> > > Imo if you have the sound side of hdmi/dp audio, then just
> > > EPROBE_DEFER until i915 is loaded (assuming it's not disabled through
> > > nomodeset or Kconfig). If it's not there then continue without it (and
> > > without hdmi/dp audio ofc). Trying to be clever just means we need to
> > > hand roll things all over the place all the time. We have some code on
> > > earlier platforms for runtime clock adjustements (on ironlake) in
> > > i915.ko, and I really don't want that kind of hacks any more.
> > 
> > In case you don't believe me that your hack is broken: I often boot
> > with i915 blacklisted, so that I can set up netconsole and other
> > instrumenting and then load it again with modeset. Until that's done
> > snd-hda will be stuck in that wait_completion.
> 
> Yes, but it has a fallback after some long timeout.  Then at least
> user would see if binding failure happens and going without HDMI/DP.
> 
> > You really can't ever
> > know when userspace or the user decides to finally load the driver,
> > and the only reasonable thing to do is to defer until everything you
> > need is there. Except of course when the user told you it's not going
> > to show up through nomodeset or Kconfig knobs, but that's kinda the
> > exception.
> > 
> > Imo the best course forward would be:
> > - Implement EPROBE_DEFER correctly in snd-hda (i.e. no
> > wait_completion, no deferred work or anything like that, just return
> > -EPROBE_DEFER when i915 isn't there yet).
> 
> We actually don't need EPROBE_DEFER, either.  The component master
> bind would take care of that already.  I used wait_for_completion()
> just because I didn't want the hda-i915 binder API function.  If it
> takes the function to be continued, master binder can call it after
> binding with i915 properly.

Well component does a deferred probe internally afaik.

> > - Add a bail-out option if CONFIG_DRM_I915 isn't enabled to snd-hda.
> 
> You can forget about Kconfig.  It's already handled.  No relevant code
> will be executed when CONFIG_DRM_I915 isn't set.
> 
> > - Add a runtime bail-out for nomodeset/vgacon_text_force() to snd-hda.
> 
> Yes, that's an easy part.
> 
> > That should cover everyone's debug needs, while giving us a clean
> > architecture moving forward. Thoughts?
> 
> But this doesn't cover all cases.  As you mentioned,  what if user
> would blacklist i915, or set i915.modeset=0?  In my patch, HD-audio
> tries to continue at least some timeout.

My opinion is that you can't decide correctly. Either the timeout is too
long, and you have users complaining that audio doesn't work because the
driver doesn't complete loading. Or it's too short, and you break
EDEFER_PROBING. I guess if you want you can code up a timeout in snd-hda,
but since patch 1 doesn't fix anything when a user outright blacklist i915
I guess we can drop that one at least?

I just don't want these kinds of hacks to leak into i915, if you're fine
with that I don't mind.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

On Tue, 17 May 2016 23:11:10 +0200,
Daniel Vetter wrote:
> 
> On Tue, May 17, 2016 at 08:19:32PM +0200, Takashi Iwai wrote:
> > On Tue, 17 May 2016 18:23:40 +0200,
> > Daniel Vetter wrote:
> > > 
> > > On Tue, May 17, 2016 at 6:18 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > On Tue, May 17, 2016 at 3:20 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > > >>> Ok, I looked at patch 3, and that indeed would lead to trouble without
> > > >>> patch 1. But the real trouble is the unconditional wait_completion in
> > > >>> there - blocking for another driver to complete loading from a driver load
> > > >>> function is a no-go. The correct way to do this is to bail out with
> > > >>> EPROBE_DEFER if not all the parts are available there. Also, throw out
> > > >>> that request_module.
> > > >>>
> > > >>> By bailing out with EPROBE_DEFER you avoid deadlocks, and the driver core
> > > >>> also knows what's going on. Which is incidentally what's used to
> > > >>> implicitly order suspend/resume. Driver core will restart your probe as
> > > >>> soon as some new devices/drivers have registers (assuming that hopefully
> > > >>> then you're unblocking), but if you're unlucky your driver can go through
> > > >>> that loop a few times.
> > > >>>
> > > >>> But that was just a very quick look, we definitely shouldn't need any
> > > >>> wait_completion in driver load to handle cross-module depencies.
> > > >>
> > > >> Yeah, I admit that wait_completion() is hackish.  OTOH, EPROBE_DEFER
> > > >> doesn't work in the case of HD-audio because we want to give up
> > > >> binding and continue without i915 but only with onboard audio, instead
> > > >> of endlessly reprobing for the never-appearing component.  The i915
> > > >> binding is no hard dependency; i.e. it isn't (always) mandatory, and
> > > >> EPROBE_DEFER can't handle such a fallback, AFAIK.
> > > >>
> > > >> If there is a good way to deal with it, please let me know.  I'd love
> > > >> to rewrite to a cleaner way.
> > > >
> > > > The only way to deal with that is to split the driver into two, and
> > > > hotplug them individually. Fundamentally any approach where you need
> > > > to know whether i915 shows up or not and act accordingly is just plain
> > > > flawed, there's no way around it. That's also why EPROBE_DEFER doesn't
> > > > bother dealing with it.
> > > >
> > > > Imo if you have the sound side of hdmi/dp audio, then just
> > > > EPROBE_DEFER until i915 is loaded (assuming it's not disabled through
> > > > nomodeset or Kconfig). If it's not there then continue without it (and
> > > > without hdmi/dp audio ofc). Trying to be clever just means we need to
> > > > hand roll things all over the place all the time. We have some code on
> > > > earlier platforms for runtime clock adjustements (on ironlake) in
> > > > i915.ko, and I really don't want that kind of hacks any more.
> > > 
> > > In case you don't believe me that your hack is broken: I often boot
> > > with i915 blacklisted, so that I can set up netconsole and other
> > > instrumenting and then load it again with modeset. Until that's done
> > > snd-hda will be stuck in that wait_completion.
> > 
> > Yes, but it has a fallback after some long timeout.  Then at least
> > user would see if binding failure happens and going without HDMI/DP.
> > 
> > > You really can't ever
> > > know when userspace or the user decides to finally load the driver,
> > > and the only reasonable thing to do is to defer until everything you
> > > need is there. Except of course when the user told you it's not going
> > > to show up through nomodeset or Kconfig knobs, but that's kinda the
> > > exception.
> > > 
> > > Imo the best course forward would be:
> > > - Implement EPROBE_DEFER correctly in snd-hda (i.e. no
> > > wait_completion, no deferred work or anything like that, just return
> > > -EPROBE_DEFER when i915 isn't there yet).
> > 
> > We actually don't need EPROBE_DEFER, either.  The component master
> > bind would take care of that already.  I used wait_for_completion()
> > just because I didn't want the hda-i915 binder API function.  If it
> > takes the function to be continued, master binder can call it after
> > binding with i915 properly.
> 
> Well component does a deferred probe internally afaik.

Really?  I see that component_*() functions just call the ops
directly...

But in our case, it doesn't matter, since the component binding itself
is already done in the deferred work.

In anyway, my point is that EPROBE_DEFER can't handle the timeout, and
EPROBE_DEFER is superfluous when the component binding is used.

> > > - Add a bail-out option if CONFIG_DRM_I915 isn't enabled to snd-hda.
> > 
> > You can forget about Kconfig.  It's already handled.  No relevant code
> > will be executed when CONFIG_DRM_I915 isn't set.
> > 
> > > - Add a runtime bail-out for nomodeset/vgacon_text_force() to snd-hda.
> > 
> > Yes, that's an easy part.
> > 
> > > That should cover everyone's debug needs, while giving us a clean
> > > architecture moving forward. Thoughts?
> > 
> > But this doesn't cover all cases.  As you mentioned,  what if user
> > would blacklist i915, or set i915.modeset=0?  In my patch, HD-audio
> > tries to continue at least some timeout.
> 
> My opinion is that you can't decide correctly. Either the timeout is too
> long, and you have users complaining that audio doesn't work because the
> driver doesn't complete loading. Or it's too short, and you break
> EDEFER_PROBING. I guess if you want you can code up a timeout in snd-hda,
> but since patch 1 doesn't fix anything when a user outright blacklist i915
> I guess we can drop that one at least?

Surely there can't be any 100% workable solution.  But if i915 driver
knows that it's disabled, it'd be still helpful to notify it if any
others expect i915 getting up.

Imagine you know that you can't attend a party you are invited in this
evening.  What's the best act?  A cancel notification would make
things working better than just let people waiting.

In this analogy, people also shouldn't wait endlessly for you,
either.  Setting a timeout is very reasonable.

> I just don't want these kinds of hacks to leak into i915, if you're fine
> with that I don't mind.

Well, this can be seen as a lack in component binding mechanism, too.
There is no cancellation notification there.  So, Imre came up with a
stub ops implementation.  It's hacky, yes.

Maybe vgacon_text_force() check and some timeout without i915 side
changes would be enough as a start, until some good way for cancel
notification is implemented.  User can still reload the sound module
after i915 if needed, too.  The timeout can be an adjustable module
option, if it really matters.


thanks,

Takashi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx