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@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?
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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
