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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
