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

[Imre Deak <imre.deak@intel.com>]

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