* [PATCH 0/3] More robust i915 / audio binding
@ 2016-04-07 10:57 Takashi Iwai
2016-04-07 10:57 ` [PATCH 1/3] drm/i915/hda: Add audio component stub Takashi Iwai
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Takashi Iwai @ 2016-04-07 10:57 UTC (permalink / raw)
To: alsa-devel; +Cc: Daniel Vetter, intel-gfx
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
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH 1/3] drm/i915/hda: Add audio component stub 2016-04-07 10:57 [PATCH 0/3] More robust i915 / audio binding Takashi Iwai @ 2016-04-07 10:57 ` Takashi Iwai 2016-04-07 11:55 ` Ville Syrjälä 2016-04-07 10:57 ` [PATCH 2/3] ALSA: hda - Immediately fail if the i915 audio component is disabled Takashi Iwai 2016-04-07 10:57 ` [PATCH 3/3] ALSA: hda - Support deferred i915 audio component binding Takashi Iwai 2 siblings, 1 reply; 7+ messages in thread From: Takashi Iwai @ 2016-04-07 10:57 UTC (permalink / raw) To: alsa-devel; +Cc: Daniel Vetter, intel-gfx 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] drm/i915/hda: Add audio component stub 2016-04-07 10:57 ` [PATCH 1/3] drm/i915/hda: Add audio component stub Takashi Iwai @ 2016-04-07 11:55 ` Ville Syrjälä 2016-04-07 12:30 ` Imre Deak 0 siblings, 1 reply; 7+ messages in thread From: Ville Syrjälä @ 2016-04-07 11:55 UTC (permalink / raw) To: Takashi Iwai; +Cc: Daniel Vetter, alsa-devel, intel-gfx 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] drm/i915/hda: Add audio component stub 2016-04-07 11:55 ` Ville Syrjälä @ 2016-04-07 12:30 ` Imre Deak 2016-04-25 12:46 ` Takashi Iwai 0 siblings, 1 reply; 7+ messages in thread From: Imre Deak @ 2016-04-07 12:30 UTC (permalink / raw) To: Ville Syrjälä, Takashi Iwai Cc: Daniel Vetter, alsa-devel, intel-gfx 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] drm/i915/hda: Add audio component stub 2016-04-07 12:30 ` Imre Deak @ 2016-04-25 12:46 ` Takashi Iwai 0 siblings, 0 replies; 7+ messages in thread From: Takashi Iwai @ 2016-04-25 12:46 UTC (permalink / raw) To: imre.deak; +Cc: Daniel Vetter, alsa-devel, intel-gfx 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/3] ALSA: hda - Immediately fail if the i915 audio component is disabled 2016-04-07 10:57 [PATCH 0/3] More robust i915 / audio binding Takashi Iwai 2016-04-07 10:57 ` [PATCH 1/3] drm/i915/hda: Add audio component stub Takashi Iwai @ 2016-04-07 10:57 ` Takashi Iwai 2016-04-07 10:57 ` [PATCH 3/3] ALSA: hda - Support deferred i915 audio component binding Takashi Iwai 2 siblings, 0 replies; 7+ messages in thread From: Takashi Iwai @ 2016-04-07 10:57 UTC (permalink / raw) To: alsa-devel; +Cc: Daniel Vetter, intel-gfx 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] ALSA: hda - Support deferred i915 audio component binding 2016-04-07 10:57 [PATCH 0/3] More robust i915 / audio binding Takashi Iwai 2016-04-07 10:57 ` [PATCH 1/3] drm/i915/hda: Add audio component stub Takashi Iwai 2016-04-07 10:57 ` [PATCH 2/3] ALSA: hda - Immediately fail if the i915 audio component is disabled Takashi Iwai @ 2016-04-07 10:57 ` Takashi Iwai 2 siblings, 0 replies; 7+ messages in thread From: Takashi Iwai @ 2016-04-07 10:57 UTC (permalink / raw) To: alsa-devel; +Cc: Daniel Vetter, intel-gfx 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-04-25 12:46 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-04-07 10:57 [PATCH 0/3] More robust i915 / audio binding Takashi Iwai 2016-04-07 10:57 ` [PATCH 1/3] drm/i915/hda: Add audio component stub Takashi Iwai 2016-04-07 11:55 ` Ville Syrjälä 2016-04-07 12:30 ` Imre Deak 2016-04-25 12:46 ` Takashi Iwai 2016-04-07 10:57 ` [PATCH 2/3] ALSA: hda - Immediately fail if the i915 audio component is disabled Takashi Iwai 2016-04-07 10:57 ` [PATCH 3/3] ALSA: hda - Support deferred i915 audio component binding Takashi Iwai
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).