[PATCH 1/3] ALSA: hda: add hdac_adsp_enable module flag

[PATCH 1/3] ALSA: hda: add hdac_adsp_enable module flag

[Vinod Koul]

Some Intel HDA controllers sport a DSP. These systems can also be enabled
with ASoC HDA driver as well. So add a flag in hda-core to enable/disable
aDSP This flag for now is false, and should be true once the ASoC based
systems mature.  The integrators/OS vendors can configure this flag based on
system preference.

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 include/sound/hdaudio.h  | 11 +++++++++++
 sound/hda/hda_bus_type.c |  4 ++++
 2 files changed, 15 insertions(+)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index b97c59eab7ab..015bec1079f9 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -12,6 +12,17 @@
 #include <sound/memalloc.h>
 #include <sound/hda_verbs.h>
 
+/*
+ * hdac_adsp_enable: exported HD-A aDSP enable configuration.
+ *
+ * Some Intel HDA controllers sport a DSP, for these platform we can bypass
+ * aDSP and use as regular HDA controller or enable aDSP and use aDSP
+ * along with I2S codecs etc.
+ * hdac_adsp_enable would enable the aDSP based HDA controller if the
+ * platform supports it
+ */
+extern bool hdac_adsp_enable;
+
 /* codec node id */
 typedef u16 hda_nid_t;
 
diff --git a/sound/hda/hda_bus_type.c b/sound/hda/hda_bus_type.c
index 519914a12e8a..80e0570ffbf4 100644
--- a/sound/hda/hda_bus_type.c
+++ b/sound/hda/hda_bus_type.c
@@ -10,6 +10,10 @@
 MODULE_DESCRIPTION("HD-audio bus");
 MODULE_LICENSE("GPL");
 
+bool hdac_adsp_enable = false;
+module_param(hdac_adsp_enable, bool, 0444);
+MODULE_PARM_DESC(hdac_adsp_enable, "Enable aDSP on Intel HDA based systems");
+
 static int hda_bus_match(struct device *dev, struct device_driver *drv)
 {
 	struct hdac_device *hdev = dev_to_hdac_dev(dev);
-- 
1.9.1

[PATCH 2/3] ALSA: hda: remove module_pci_driver

[Vinod Koul]

Since we want to selectively register for Intel aDSP systems based on the
module flag we need to bring back explicit code for driver registration and
remove module_pci_driver

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/pci/hda/hda_intel.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index f7cdf4d2e24e..488dab208ddc 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2217,4 +2217,19 @@ static struct pci_driver azx_driver = {
 	},
 };
 
-module_pci_driver(azx_driver);
+static int __init azx_module_init(void)
+{
+	int ret;
+
+	ret = pci_register_driver(&azx_driver);
+
+	return ret;
+}
+module_init(azx_module_init);
+
+static void __exit azx_module_exit(void)
+{
+	pci_unregister_driver(&azx_driver);
+
+}
+module_exit(azx_module_exit);
-- 
1.9.1

[PATCH 3/3] ALSA: hda: register selectively for SPT-LP

[Vinod Koul]

Sunrisepoint-LP HDA controller supports aDSP, so register selectively for
this controller

Signed-off-by: Vinod Koul <vinod.koul@intel.com>
---
 sound/pci/hda/hda_intel.c | 31 ++++++++++++++++++++++++++++---
 1 file changed, 28 insertions(+), 3 deletions(-)

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 488dab208ddc..314a7beb2b29 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2023,9 +2023,6 @@ static const struct pci_device_id azx_ids[] = {
 	/* Sunrise Point */
 	{ PCI_DEVICE(0x8086, 0xa170),
 	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_SKYLAKE },
-	/* Sunrise Point-LP */
-	{ PCI_DEVICE(0x8086, 0x9d70),
-	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_SKYLAKE },
 	/* Haswell */
 	{ PCI_DEVICE(0x8086, 0x0a0c),
 	  .driver_data = AZX_DRIVER_HDMI | AZX_DCAPS_INTEL_HASWELL },
@@ -2205,6 +2202,14 @@ static const struct pci_device_id azx_ids[] = {
 };
 MODULE_DEVICE_TABLE(pci, azx_ids);
 
+static const struct pci_device_id azx_intel_adsp_ids[] = {
+	/* Sunrise Point-LP */
+	{ PCI_DEVICE(0x8086, 0x9d70),
+	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_SKYLAKE },
+	{ 0, }
+};
+MODULE_DEVICE_TABLE(pci, azx_intel_adsp_ids);
+
 /* pci_driver definition */
 static struct pci_driver azx_driver = {
 	.name = KBUILD_MODNAME,
@@ -2217,12 +2222,25 @@ static struct pci_driver azx_driver = {
 	},
 };
 
+static struct pci_driver azx_intel_adsp_driver = {
+	.name = KBUILD_MODNAME,
+	.id_table = azx_intel_adsp_ids,
+	.probe = azx_probe,
+	.remove = azx_remove,
+	.shutdown = azx_shutdown,
+	.driver = {
+		.pm = AZX_PM_OPS,
+	},
+};
+
 static int __init azx_module_init(void)
 {
 	int ret;
 
 	ret = pci_register_driver(&azx_driver);
 
+	if (!hdac_adsp_enable)
+		ret = pci_register_driver(&azx_intel_adsp_driver);
 	return ret;
 }
 module_init(azx_module_init);
@@ -2231,5 +2249,12 @@ static void __exit azx_module_exit(void)
 {
 	pci_unregister_driver(&azx_driver);
 
+	/* Some Intel HDA controllers support aDSP this is enabled thru the
+	 * ASoC HDA aDSP driver So we register for these devices only when
+	 * aDSP is disabled by the hdac_adsp_enable flag
+	 */
+	if (!hdac_adsp_enable)
+		pci_unregister_driver(&azx_intel_adsp_driver);
+
 }
 module_exit(azx_module_exit);
-- 
1.9.1

Re: [PATCH 1/3] ALSA: hda: add hdac_adsp_enable module flag

[Pierre-Louis Bossart]

On 4/30/15 9:52 AM, Vinod Koul wrote:
> Some Intel HDA controllers sport a DSP. These systems can also be enabled
> with ASoC HDA driver as well. So add a flag in hda-core to enable/disable
> aDSP This flag for now is false, and should be true once the ASoC based
> systems mature.  The integrators/OS vendors can configure this flag based on
> system preference.

This choice is contingent on the BIOS options, you can't enable the DSP 
if the BIOS said no DSP...

>
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>   include/sound/hdaudio.h  | 11 +++++++++++
>   sound/hda/hda_bus_type.c |  4 ++++
>   2 files changed, 15 insertions(+)
>
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index b97c59eab7ab..015bec1079f9 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -12,6 +12,17 @@
>   #include <sound/memalloc.h>
>   #include <sound/hda_verbs.h>
>
> +/*
> + * hdac_adsp_enable: exported HD-A aDSP enable configuration.
> + *
> + * Some Intel HDA controllers sport a DSP, for these platform we can bypass
> + * aDSP and use as regular HDA controller or enable aDSP and use aDSP
> + * along with I2S codecs etc.
> + * hdac_adsp_enable would enable the aDSP based HDA controller if the
> + * platform supports it
> + */
> +extern bool hdac_adsp_enable;
> +
>   /* codec node id */
>   typedef u16 hda_nid_t;
>
> diff --git a/sound/hda/hda_bus_type.c b/sound/hda/hda_bus_type.c
> index 519914a12e8a..80e0570ffbf4 100644
> --- a/sound/hda/hda_bus_type.c
> +++ b/sound/hda/hda_bus_type.c
> @@ -10,6 +10,10 @@
>   MODULE_DESCRIPTION("HD-audio bus");
>   MODULE_LICENSE("GPL");
>
> +bool hdac_adsp_enable = false;
> +module_param(hdac_adsp_enable, bool, 0444);
> +MODULE_PARM_DESC(hdac_adsp_enable, "Enable aDSP on Intel HDA based systems");
> +
>   static int hda_bus_match(struct device *dev, struct device_driver *drv)
>   {
>   	struct hdac_device *hdev = dev_to_hdac_dev(dev);
>

Re: [PATCH 1/3] ALSA: hda: add hdac_adsp_enable module flag

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 674 bytes --]

On Thu, Apr 30, 2015 at 11:02:19AM -0500, Pierre-Louis Bossart wrote:
> On 4/30/15 9:52 AM, Vinod Koul wrote:

> >Some Intel HDA controllers sport a DSP. These systems can also be enabled
> >with ASoC HDA driver as well. So add a flag in hda-core to enable/disable
> >aDSP This flag for now is false, and should be true once the ASoC based
> >systems mature.  The integrators/OS vendors can configure this flag based on
> >system preference.

> This choice is contingent on the BIOS options, you can't enable the DSP if
> the BIOS said no DSP...

Presumably this flag should be coming from the BIOS by default and this
providing a disable only setting for test/development?

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/3] ALSA: hda: register selectively for SPT-LP

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1196 bytes --]

On Thu, Apr 30, 2015 at 08:22:36PM +0530, Vinod Koul wrote:

> -	/* Sunrise Point-LP */
> -	{ PCI_DEVICE(0x8086, 0x9d70),
> -	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_SKYLAKE },

> +static const struct pci_device_id azx_intel_adsp_ids[] = {
> +	/* Sunrise Point-LP */
> +	{ PCI_DEVICE(0x8086, 0x9d70),
> +	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_SKYLAKE },
> +	{ 0, }
> +};
> +MODULE_DEVICE_TABLE(pci, azx_intel_adsp_ids);
> +

>  static int __init azx_module_init(void)
>  {
>  	int ret;
>  
>  	ret = pci_register_driver(&azx_driver);
>  
> +	if (!hdac_adsp_enable)
> +		ret = pci_register_driver(&azx_intel_adsp_driver);

This feels like the wrong idiom here.  I'd expect us to do this by
binding to the device but ignoring the DSP functionality, or if the
device has only DSP functionality (it's not clear with the context I
have but I think that's the case) just printing a message and not
telling the sound core about it.  Having the driver just fail to load
seems like it might be a bit obscure.

Though to be honest if it *is* going to register as a separate HDA
device then it seems like userspace quirking for this is just as viable.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/3] ALSA: hda: add hdac_adsp_enable module flag

[Takashi Iwai]

At Thu, 30 Apr 2015 20:27:26 +0100,
Mark Brown wrote:
> 
> On Thu, Apr 30, 2015 at 11:02:19AM -0500, Pierre-Louis Bossart wrote:
> > On 4/30/15 9:52 AM, Vinod Koul wrote:
> 
> > >Some Intel HDA controllers sport a DSP. These systems can also be enabled
> > >with ASoC HDA driver as well. So add a flag in hda-core to enable/disable
> > >aDSP This flag for now is false, and should be true once the ASoC based
> > >systems mature.  The integrators/OS vendors can configure this flag based on
> > >system preference.
> 
> > This choice is contingent on the BIOS options, you can't enable the DSP if
> > the BIOS said no DSP...
> 
> Presumably this flag should be coming from the BIOS by default and this
> providing a disable only setting for test/development?

It's hard to know exactly how many models will be with DSP and how
many without...  The problem we're dealing with is that we'll have two
individual drivers supporting the very same PCI ID.  And, there is no
good mechanism to prioritize the driver load on Linux, so far.

So, this is the consequence after lengthy discussions: it's fairly
simple but effective enough.  Distros may build both drivers, but
which driver to use can be decided by this option.  The default value
of this switch should be set via a Kconfig, but it can be overridden
dynamically via either a static module option or boot option.


Takashi

Re: [PATCH 1/3] ALSA: hda: add hdac_adsp_enable module flag

[Takashi Iwai]

At Thu, 30 Apr 2015 20:22:34 +0530,
Vinod Koul wrote:
> 
> Some Intel HDA controllers sport a DSP. These systems can also be enabled
> with ASoC HDA driver as well. So add a flag in hda-core to enable/disable
> aDSP This flag for now is false, and should be true once the ASoC based
> systems mature.  The integrators/OS vendors can configure this flag based on
> system preference.
> 
> Signed-off-by: Vinod Koul <vinod.koul@intel.com>
> ---
>  include/sound/hdaudio.h  | 11 +++++++++++
>  sound/hda/hda_bus_type.c |  4 ++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
> index b97c59eab7ab..015bec1079f9 100644
> --- a/include/sound/hdaudio.h
> +++ b/include/sound/hdaudio.h
> @@ -12,6 +12,17 @@
>  #include <sound/memalloc.h>
>  #include <sound/hda_verbs.h>
>  
> +/*
> + * hdac_adsp_enable: exported HD-A aDSP enable configuration.
> + *
> + * Some Intel HDA controllers sport a DSP, for these platform we can bypass
> + * aDSP and use as regular HDA controller or enable aDSP and use aDSP
> + * along with I2S codecs etc.
> + * hdac_adsp_enable would enable the aDSP based HDA controller if the
> + * platform supports it
> + */
> +extern bool hdac_adsp_enable;

I prefer snd_ prefix for exported symbols, partly for consistency and
partly for safety reason.

> +
>  /* codec node id */
>  typedef u16 hda_nid_t;
>  
> diff --git a/sound/hda/hda_bus_type.c b/sound/hda/hda_bus_type.c
> index 519914a12e8a..80e0570ffbf4 100644
> --- a/sound/hda/hda_bus_type.c
> +++ b/sound/hda/hda_bus_type.c
> @@ -10,6 +10,10 @@
>  MODULE_DESCRIPTION("HD-audio bus");
>  MODULE_LICENSE("GPL");
>  
> +bool hdac_adsp_enable = false;
> +module_param(hdac_adsp_enable, bool, 0444);

Try to reduce to a shorter option name, e.g. enable_adsp.  With
module_param_named(), you can have different option- and variable
names.


thanks,

Takashi

Re: [PATCH 3/3] ALSA: hda: register selectively for SPT-LP

[Takashi Iwai]

At Thu, 30 Apr 2015 21:33:55 +0100,
Mark Brown wrote:
> 
> On Thu, Apr 30, 2015 at 08:22:36PM +0530, Vinod Koul wrote:
> 
> > -	/* Sunrise Point-LP */
> > -	{ PCI_DEVICE(0x8086, 0x9d70),
> > -	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_SKYLAKE },
> 
> > +static const struct pci_device_id azx_intel_adsp_ids[] = {
> > +	/* Sunrise Point-LP */
> > +	{ PCI_DEVICE(0x8086, 0x9d70),
> > +	  .driver_data = AZX_DRIVER_PCH | AZX_DCAPS_INTEL_SKYLAKE },
> > +	{ 0, }
> > +};
> > +MODULE_DEVICE_TABLE(pci, azx_intel_adsp_ids);
> > +
> 
> >  static int __init azx_module_init(void)
> >  {
> >  	int ret;
> >  
> >  	ret = pci_register_driver(&azx_driver);
> >  
> > +	if (!hdac_adsp_enable)
> > +		ret = pci_register_driver(&azx_intel_adsp_driver);
> 
> This feels like the wrong idiom here.  I'd expect us to do this by
> binding to the device but ignoring the DSP functionality, or if the
> device has only DSP functionality (it's not clear with the context I
> have but I think that's the case) just printing a message and not
> telling the sound core about it.  Having the driver just fail to load
> seems like it might be a bit obscure.
> 
> Though to be honest if it *is* going to register as a separate HDA
> device then it seems like userspace quirking for this is just as viable.

Ideally, it'd be best if the system can do everything automatically,
e.g. loads the DSP driver at first, then falls back to the legacy
driver if no DSP is found.  But there seems no clean way to do this,
so far.  Thus the flag was introduced.

BTW, the patch won't work as expected.  The device list is created at
the module build time (modpost), thus it must expose all devices,
including SKL.  The patch essentially excludes SKL PCI ID from the
list.

Another way to skip the unwanted driver is just to return -ENODEV from
its probe callback when the given PCI is SKL and the flag is set.
Then the driver core ignores this and continues to the next possible
driver (i.e. the dsp one).  The same should be applied to hda-soc-skl
driver in vice versa.


Takashi

Re: [PATCH 3/3] ALSA: hda: register selectively for SPT-LP

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 764 bytes --]

On Thu, Apr 30, 2015 at 10:59:22PM +0200, Takashi Iwai wrote:

> Ideally, it'd be best if the system can do everything automatically,
> e.g. loads the DSP driver at first, then falls back to the legacy
> driver if no DSP is found.  But there seems no clean way to do this,
> so far.  Thus the flag was introduced.

Eeew, that's horrible.  I guess another DMI quirk table is going to be
needed...  

> Another way to skip the unwanted driver is just to return -ENODEV from
> its probe callback when the given PCI is SKL and the flag is set.
> Then the driver core ignores this and continues to the next possible
> driver (i.e. the dsp one).  The same should be applied to hda-soc-skl
> driver in vice versa.

Right, that was one of my suggestions.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/3] ALSA: hda: register selectively for SPT-LP

[Vinod Koul]

On Thu, Apr 30, 2015 at 10:23:30PM +0100, Mark Brown wrote:
> On Thu, Apr 30, 2015 at 10:59:22PM +0200, Takashi Iwai wrote:
> 
> > Ideally, it'd be best if the system can do everything automatically,
> > e.g. loads the DSP driver at first, then falls back to the legacy
> > driver if no DSP is found.  But there seems no clean way to do this,
> > so far.  Thus the flag was introduced.
> 
> Eeew, that's horrible.  I guess another DMI quirk table is going to be
> needed...  
> 
> > Another way to skip the unwanted driver is just to return -ENODEV from
> > its probe callback when the given PCI is SKL and the flag is set.
> > Then the driver core ignores this and continues to the next possible
> > driver (i.e. the dsp one).  The same should be applied to hda-soc-skl
> > driver in vice versa.
> 
> Right, that was one of my suggestions.
okay that sounds okay to me. So the device probe checks the value of this
flag and then either goes ahead with probe or returns -ENODEV

Thanks
-- 
~Vinod

Re: [PATCH 1/3] ALSA: hda: add hdac_adsp_enable module flag

[Vinod Koul]

On Thu, Apr 30, 2015 at 10:44:21PM +0200, Takashi Iwai wrote:
> At Thu, 30 Apr 2015 20:27:26 +0100,
> Mark Brown wrote:
> > 
> > On Thu, Apr 30, 2015 at 11:02:19AM -0500, Pierre-Louis Bossart wrote:
> > > On 4/30/15 9:52 AM, Vinod Koul wrote:
> > 
> > > >Some Intel HDA controllers sport a DSP. These systems can also be enabled
> > > >with ASoC HDA driver as well. So add a flag in hda-core to enable/disable
> > > >aDSP This flag for now is false, and should be true once the ASoC based
> > > >systems mature.  The integrators/OS vendors can configure this flag based on
> > > >system preference.
> > 
> > > This choice is contingent on the BIOS options, you can't enable the DSP if
> > > the BIOS said no DSP...
That is right, but by default DSP in On. Btw this is not like older HSW
platform where HDA and DSP and muxed and BIOS plays key role.
> > 
> > Presumably this flag should be coming from the BIOS by default and this
> > providing a disable only setting for test/development?
> 
> It's hard to know exactly how many models will be with DSP and how
> many without...  The problem we're dealing with is that we'll have two
> individual drivers supporting the very same PCI ID.  And, there is no
> good mechanism to prioritize the driver load on Linux, so far.
> 
> So, this is the consequence after lengthy discussions: it's fairly
> simple but effective enough.  Distros may build both drivers, but
> which driver to use can be decided by this option.  The default value
> of this switch should be set via a Kconfig, but it can be overridden
> dynamically via either a static module option or boot option.
Okay i will respin the series based on latest discussion

Thanks
-- 
~Vinod

Re: [PATCH 1/3] ALSA: hda: add hdac_adsp_enable module flag

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1522 bytes --]

On Thu, Apr 30, 2015 at 10:44:21PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:
> > On Thu, Apr 30, 2015 at 11:02:19AM -0500, Pierre-Louis Bossart wrote:
j
> > > This choice is contingent on the BIOS options, you can't enable the DSP if
> > > the BIOS said no DSP...

> > Presumably this flag should be coming from the BIOS by default and this
> > providing a disable only setting for test/development?

> It's hard to know exactly how many models will be with DSP and how
> many without...  The problem we're dealing with is that we'll have two
> individual drivers supporting the very same PCI ID.  And, there is no
> good mechanism to prioritize the driver load on Linux, so far.

Well, I'm a bit confused why we're loading separate drivers at all here.
Shouldn't we be loading one driver and then figuring out what to do with
the hardware based on something like DMI matching (you'd hope that there
would be some config readback from the firmware but recent performance
suggests not)?

> So, this is the consequence after lengthy discussions: it's fairly
> simple but effective enough.  Distros may build both drivers, but
> which driver to use can be decided by this option.  The default value
> of this switch should be set via a Kconfig, but it can be overridden
> dynamically via either a static module option or boot option.

I don't understand how it's going to work well for users, especially
distros where you're building something to be installed on a wide range
of hardware, and it doesn't seem idiomatic.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 1/3] ALSA: hda: add hdac_adsp_enable module flag

[Pierre-Louis Bossart]

On 4/30/15 11:39 PM, Vinod Koul wrote:
> On Thu, Apr 30, 2015 at 10:44:21PM +0200, Takashi Iwai wrote:
>> At Thu, 30 Apr 2015 20:27:26 +0100,
>> Mark Brown wrote:
>>>
>>> On Thu, Apr 30, 2015 at 11:02:19AM -0500, Pierre-Louis Bossart wrote:
>>>> On 4/30/15 9:52 AM, Vinod Koul wrote:
>>>
>>>>> Some Intel HDA controllers sport a DSP. These systems can also be enabled
>>>>> with ASoC HDA driver as well. So add a flag in hda-core to enable/disable
>>>>> aDSP This flag for now is false, and should be true once the ASoC based
>>>>> systems mature.  The integrators/OS vendors can configure this flag based on
>>>>> system preference.
>>>
>>>> This choice is contingent on the BIOS options, you can't enable the DSP if
>>>> the BIOS said no DSP...
> That is right, but by default DSP in On. Btw this is not like older HSW
> platform where HDA and DSP and muxed and BIOS plays key role.
>>>
>>> Presumably this flag should be coming from the BIOS by default and this
>>> providing a disable only setting for test/development?
>>
>> It's hard to know exactly how many models will be with DSP and how
>> many without...  The problem we're dealing with is that we'll have two
>> individual drivers supporting the very same PCI ID.  And, there is no
>> good mechanism to prioritize the driver load on Linux, so far.
>>
>> So, this is the consequence after lengthy discussions: it's fairly
>> simple but effective enough.  Distros may build both drivers, but
>> which driver to use can be decided by this option.  The default value
>> of this switch should be set via a Kconfig, but it can be overridden
>> dynamically via either a static module option or boot option.
> Okay i will respin the series based on latest discussion

Maybe I confused everyone, it's not complicated: there is a register 
that indicates if the DSP is enabled and that can be queried before 
launching the DSP driver. There is no guessing or need for DMI-based 
quirks, the capabilities are exposed and that should be used. You can 
then add a driver parameter to fall back to legacy mode, e.g. for 
testing, but that would be a second level disable. For once the hardware 
does tell us what to do, we need to use the information...

Re: [PATCH 1/3] ALSA: hda: add hdac_adsp_enable module flag

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 840 bytes --]

On Fri, May 01, 2015 at 09:15:10AM -0500, Pierre-Louis Bossart wrote:

> Maybe I confused everyone, it's not complicated: there is a register that
> indicates if the DSP is enabled and that can be queried before launching the
> DSP driver. There is no guessing or need for DMI-based quirks, the
> capabilities are exposed and that should be used. You can then add a driver
> parameter to fall back to legacy mode, e.g. for testing, but that would be a
> second level disable. For once the hardware does tell us what to do, we need
> to use the information...

Oh, OK.  That sounds sensible!  If that's the case then surely we should
just have one driver for the PCI ID which just skips enabling the DSP
bits based on a combination of the register and the driver parameter?
We shouldn't need this multiple drivers for the same PCI ID stuff.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]