[PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Bard liao]

From: Bard liao <bard.liao@intel.com>

Add Icelake device id. Also, Icelake's pin2port mapping table is
complicated. So we use a mapping table to do the pin2port mapping.

Signed-off-by: Bard liao <bard.liao@intel.com>
---
 sound/soc/codecs/hdac_hdmi.c | 63 ++++++++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 9 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 4e9854889a95..fac397326515 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -121,8 +121,16 @@ struct hdac_hdmi_dai_port_map {
 	struct hdac_hdmi_cvt *cvt;
 };
 
+/*
+ * pin to port mapping table where the value indicate the pin number and
+ * the index indicate the port number with 1 base.
+ */
+static const int icl_pin2port_map[] = {0x4, 0x6, 0x8, 0xa, 0xb};
+
 struct hdac_hdmi_drv_data {
 	unsigned int vendor_nid;
+	const int *port_map; /* pin to port mapping table */
+	int port_num;
 };
 
 struct hdac_hdmi_priv {
@@ -1329,11 +1337,12 @@ static int hdac_hdmi_add_pin(struct hdac_device *hdev, hda_nid_t nid)
 	return 0;
 }
 
-#define INTEL_VENDOR_NID 0x08
-#define INTEL_GLK_VENDOR_NID 0x0b
+#define INTEL_VENDOR_NID_0x2 0x02
+#define INTEL_VENDOR_NID_0x8 0x08
+#define INTEL_VENDOR_NID_0xb 0x0b
 #define INTEL_GET_VENDOR_VERB 0xf81
 #define INTEL_SET_VENDOR_VERB 0x781
-#define INTEL_EN_DP12			0x02 /* enable DP 1.2 features */
+#define INTEL_EN_DP12		0x02 /* enable DP 1.2 features */
 #define INTEL_EN_ALL_PIN_CVTS	0x01 /* enable 2nd & 3rd pins and convertors */
 
 static void hdac_hdmi_skl_enable_all_pins(struct hdac_device *hdev)
@@ -1538,7 +1547,26 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_device *hdev,
 
 static int hdac_hdmi_pin2port(void *aptr, int pin)
 {
-	return pin - 4; /* map NID 0x05 -> port #1 */
+	struct hdac_device *hdev = aptr;
+	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
+	const int *map = hdmi->drv_data->port_map;
+	int i;
+
+	if (!hdmi->drv_data->port_num)
+		return pin - 4; /* map NID 0x05 -> port #1 */
+
+	/*
+	 * looking for the pin number in the mapping table and return
+	 * the index which indicate the port number
+	 */
+	for (i = 0; i < hdmi->drv_data->port_num; i++) {
+		if (pin == map[i])
+			return i + 1;
+	}
+
+	/* return -1 if pin number exceeds our expectation */
+	dev_err(&hdev->dev, "Can't find the port for pin %d\n", pin);
+	return -1;
 }
 
 static void hdac_hdmi_eld_notify_cb(void *aptr, int port, int pipe)
@@ -1549,9 +1577,18 @@ static void hdac_hdmi_eld_notify_cb(void *aptr, int port, int pipe)
 	struct hdac_hdmi_port *hport = NULL;
 	struct snd_soc_component *component = hdmi->component;
 	int i;
-
-	/* Don't know how this mapping is derived */
-	hda_nid_t pin_nid = port + 0x04;
+	hda_nid_t pin_nid;
+
+	if (!hdmi->drv_data->port_num) {
+		/* for legacy platforms */
+		pin_nid = port + 0x04;
+	} else if (port < hdmi->drv_data->port_num) {
+		/* get pin number from the pin2port mapping table */
+		pin_nid = hdmi->drv_data->port_map[port - 1];
+	} else {
+		dev_err(&hdev->dev, "Can't find the pin for port %d\n", port);
+		return;
+	}
 
 	dev_dbg(&hdev->dev, "%s: for pin:%d port=%d\n", __func__,
 							pin_nid, pipe);
@@ -1973,12 +2010,18 @@ static int hdac_hdmi_get_spk_alloc(struct hdac_device *hdev, int pcm_idx)
 	return port->eld.info.spk_alloc;
 }
 
+static struct hdac_hdmi_drv_data intel_icl_drv_data  = {
+	.vendor_nid = INTEL_VENDOR_NID_0x2,
+	.port_map = icl_pin2port_map,
+	.port_num = ARRAY_SIZE(icl_pin2port_map),
+};
+
 static struct hdac_hdmi_drv_data intel_glk_drv_data  = {
-	.vendor_nid = INTEL_GLK_VENDOR_NID,
+	.vendor_nid = INTEL_VENDOR_NID_0xb,
 };
 
 static struct hdac_hdmi_drv_data intel_drv_data  = {
-	.vendor_nid = INTEL_VENDOR_NID,
+	.vendor_nid = INTEL_VENDOR_NID_0x8,
 };
 
 static int hdac_hdmi_dev_probe(struct hdac_device *hdev)
@@ -2259,6 +2302,8 @@ static const struct hda_device_id hdmi_list[] = {
 						   &intel_glk_drv_data),
 	HDA_CODEC_EXT_ENTRY(0x8086280d, 0x100000, "Geminilake HDMI",
 						   &intel_glk_drv_data),
+	HDA_CODEC_EXT_ENTRY(0x8086280f, 0x100000, "Icelake HDMI",
+						   &intel_icl_drv_data),
 	{}
 };
 
-- 
2.17.1

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Takashi Iwai]

On Sat, 10 Nov 2018 22:18:46 +0100,
Bard liao wrote:
> 
> From: Bard liao <bard.liao@intel.com>
> 
> Add Icelake device id. Also, Icelake's pin2port mapping table is
> complicated. So we use a mapping table to do the pin2port mapping.
> 
> Signed-off-by: Bard liao <bard.liao@intel.com>

The recent code has already pin2port callback in drm_audio_component
ops.  This is called in sound/hda/hdac_component.c before eld notify
callback gets called.  So, use this callback instead of introducing
yet another one.

Also, it'd be helpful if you fix the same for the legacy HD-audio HDMI
codec driver.


thanks,

Takashi


>  sound/soc/codecs/hdac_hdmi.c | 63 ++++++++++++++++++++++++++++++------
>  1 file changed, 54 insertions(+), 9 deletions(-)
> 
> diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> index 4e9854889a95..fac397326515 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -121,8 +121,16 @@ struct hdac_hdmi_dai_port_map {
>  	struct hdac_hdmi_cvt *cvt;
>  };
>  
> +/*
> + * pin to port mapping table where the value indicate the pin number and
> + * the index indicate the port number with 1 base.
> + */
> +static const int icl_pin2port_map[] = {0x4, 0x6, 0x8, 0xa, 0xb};
> +
>  struct hdac_hdmi_drv_data {
>  	unsigned int vendor_nid;
> +	const int *port_map; /* pin to port mapping table */
> +	int port_num;
>  };
>  
>  struct hdac_hdmi_priv {
> @@ -1329,11 +1337,12 @@ static int hdac_hdmi_add_pin(struct hdac_device *hdev, hda_nid_t nid)
>  	return 0;
>  }
>  
> -#define INTEL_VENDOR_NID 0x08
> -#define INTEL_GLK_VENDOR_NID 0x0b
> +#define INTEL_VENDOR_NID_0x2 0x02
> +#define INTEL_VENDOR_NID_0x8 0x08
> +#define INTEL_VENDOR_NID_0xb 0x0b
>  #define INTEL_GET_VENDOR_VERB 0xf81
>  #define INTEL_SET_VENDOR_VERB 0x781
> -#define INTEL_EN_DP12			0x02 /* enable DP 1.2 features */
> +#define INTEL_EN_DP12		0x02 /* enable DP 1.2 features */
>  #define INTEL_EN_ALL_PIN_CVTS	0x01 /* enable 2nd & 3rd pins and convertors */
>  
>  static void hdac_hdmi_skl_enable_all_pins(struct hdac_device *hdev)
> @@ -1538,7 +1547,26 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_device *hdev,
>  
>  static int hdac_hdmi_pin2port(void *aptr, int pin)
>  {
> -	return pin - 4; /* map NID 0x05 -> port #1 */
> +	struct hdac_device *hdev = aptr;
> +	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
> +	const int *map = hdmi->drv_data->port_map;
> +	int i;
> +
> +	if (!hdmi->drv_data->port_num)
> +		return pin - 4; /* map NID 0x05 -> port #1 */
> +
> +	/*
> +	 * looking for the pin number in the mapping table and return
> +	 * the index which indicate the port number
> +	 */
> +	for (i = 0; i < hdmi->drv_data->port_num; i++) {
> +		if (pin == map[i])
> +			return i + 1;
> +	}
> +
> +	/* return -1 if pin number exceeds our expectation */
> +	dev_err(&hdev->dev, "Can't find the port for pin %d\n", pin);
> +	return -1;
>  }
>  
>  static void hdac_hdmi_eld_notify_cb(void *aptr, int port, int pipe)
> @@ -1549,9 +1577,18 @@ static void hdac_hdmi_eld_notify_cb(void *aptr, int port, int pipe)
>  	struct hdac_hdmi_port *hport = NULL;
>  	struct snd_soc_component *component = hdmi->component;
>  	int i;
> -
> -	/* Don't know how this mapping is derived */
> -	hda_nid_t pin_nid = port + 0x04;
> +	hda_nid_t pin_nid;
> +
> +	if (!hdmi->drv_data->port_num) {
> +		/* for legacy platforms */
> +		pin_nid = port + 0x04;
> +	} else if (port < hdmi->drv_data->port_num) {
> +		/* get pin number from the pin2port mapping table */
> +		pin_nid = hdmi->drv_data->port_map[port - 1];
> +	} else {
> +		dev_err(&hdev->dev, "Can't find the pin for port %d\n", port);
> +		return;
> +	}
>  
>  	dev_dbg(&hdev->dev, "%s: for pin:%d port=%d\n", __func__,
>  							pin_nid, pipe);
> @@ -1973,12 +2010,18 @@ static int hdac_hdmi_get_spk_alloc(struct hdac_device *hdev, int pcm_idx)
>  	return port->eld.info.spk_alloc;
>  }
>  
> +static struct hdac_hdmi_drv_data intel_icl_drv_data  = {
> +	.vendor_nid = INTEL_VENDOR_NID_0x2,
> +	.port_map = icl_pin2port_map,
> +	.port_num = ARRAY_SIZE(icl_pin2port_map),
> +};
> +
>  static struct hdac_hdmi_drv_data intel_glk_drv_data  = {
> -	.vendor_nid = INTEL_GLK_VENDOR_NID,
> +	.vendor_nid = INTEL_VENDOR_NID_0xb,
>  };
>  
>  static struct hdac_hdmi_drv_data intel_drv_data  = {
> -	.vendor_nid = INTEL_VENDOR_NID,
> +	.vendor_nid = INTEL_VENDOR_NID_0x8,
>  };
>  
>  static int hdac_hdmi_dev_probe(struct hdac_device *hdev)
> @@ -2259,6 +2302,8 @@ static const struct hda_device_id hdmi_list[] = {
>  						   &intel_glk_drv_data),
>  	HDA_CODEC_EXT_ENTRY(0x8086280d, 0x100000, "Geminilake HDMI",
>  						   &intel_glk_drv_data),
> +	HDA_CODEC_EXT_ENTRY(0x8086280f, 0x100000, "Icelake HDMI",
> +						   &intel_icl_drv_data),
>  	{}
>  };
>  
> -- 
> 2.17.1
> 

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Pierre-Louis Bossart]

On 11/11/18 2:55 AM, Takashi Iwai wrote:
> On Sat, 10 Nov 2018 22:18:46 +0100,
> Bard liao wrote:
>> From: Bard liao <bard.liao@intel.com>
>>
>> Add Icelake device id. Also, Icelake's pin2port mapping table is
>> complicated. So we use a mapping table to do the pin2port mapping.
>>
>> Signed-off-by: Bard liao <bard.liao@intel.com>
> The recent code has already pin2port callback in drm_audio_component
> ops.  This is called in sound/hda/hdac_component.c before eld notify
> callback gets called.  So, use this callback instead of introducing
> yet another one.
Sorry Takashi, can you elaborate? What this patch does is modify the 
existing implementation of the .pin2port callback, not add a new one. 
The existing logic (return pin - 4) is no longer sufficient on IceLake.
>
> Also, it'd be helpful if you fix the same for the legacy HD-audio HDMI
> codec driver.
Our intention was to revisit differences between legacy and non-legacy 
in a separate patch if that's all right with you. We've identified 
missing IDs and other things that should be fixed separately.
>
>
> thanks,
>
> Takashi
>
>
>>   sound/soc/codecs/hdac_hdmi.c | 63 ++++++++++++++++++++++++++++++------
>>   1 file changed, 54 insertions(+), 9 deletions(-)
>>
>> diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
>> index 4e9854889a95..fac397326515 100644
>> --- a/sound/soc/codecs/hdac_hdmi.c
>> +++ b/sound/soc/codecs/hdac_hdmi.c
>> @@ -121,8 +121,16 @@ struct hdac_hdmi_dai_port_map {
>>   	struct hdac_hdmi_cvt *cvt;
>>   };
>>   
>> +/*
>> + * pin to port mapping table where the value indicate the pin number and
>> + * the index indicate the port number with 1 base.
>> + */
>> +static const int icl_pin2port_map[] = {0x4, 0x6, 0x8, 0xa, 0xb};
>> +
>>   struct hdac_hdmi_drv_data {
>>   	unsigned int vendor_nid;
>> +	const int *port_map; /* pin to port mapping table */
>> +	int port_num;
>>   };
>>   
>>   struct hdac_hdmi_priv {
>> @@ -1329,11 +1337,12 @@ static int hdac_hdmi_add_pin(struct hdac_device *hdev, hda_nid_t nid)
>>   	return 0;
>>   }
>>   
>> -#define INTEL_VENDOR_NID 0x08
>> -#define INTEL_GLK_VENDOR_NID 0x0b
>> +#define INTEL_VENDOR_NID_0x2 0x02
>> +#define INTEL_VENDOR_NID_0x8 0x08
>> +#define INTEL_VENDOR_NID_0xb 0x0b
>>   #define INTEL_GET_VENDOR_VERB 0xf81
>>   #define INTEL_SET_VENDOR_VERB 0x781
>> -#define INTEL_EN_DP12			0x02 /* enable DP 1.2 features */
>> +#define INTEL_EN_DP12		0x02 /* enable DP 1.2 features */
>>   #define INTEL_EN_ALL_PIN_CVTS	0x01 /* enable 2nd & 3rd pins and convertors */
>>   
>>   static void hdac_hdmi_skl_enable_all_pins(struct hdac_device *hdev)
>> @@ -1538,7 +1547,26 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_device *hdev,
>>   
>>   static int hdac_hdmi_pin2port(void *aptr, int pin)
>>   {
>> -	return pin - 4; /* map NID 0x05 -> port #1 */
>> +	struct hdac_device *hdev = aptr;
>> +	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
>> +	const int *map = hdmi->drv_data->port_map;
>> +	int i;
>> +
>> +	if (!hdmi->drv_data->port_num)
>> +		return pin - 4; /* map NID 0x05 -> port #1 */
>> +
>> +	/*
>> +	 * looking for the pin number in the mapping table and return
>> +	 * the index which indicate the port number
>> +	 */
>> +	for (i = 0; i < hdmi->drv_data->port_num; i++) {
>> +		if (pin == map[i])
>> +			return i + 1;
>> +	}
>> +
>> +	/* return -1 if pin number exceeds our expectation */
>> +	dev_err(&hdev->dev, "Can't find the port for pin %d\n", pin);
>> +	return -1;
>>   }
>>   
>>   static void hdac_hdmi_eld_notify_cb(void *aptr, int port, int pipe)
>> @@ -1549,9 +1577,18 @@ static void hdac_hdmi_eld_notify_cb(void *aptr, int port, int pipe)
>>   	struct hdac_hdmi_port *hport = NULL;
>>   	struct snd_soc_component *component = hdmi->component;
>>   	int i;
>> -
>> -	/* Don't know how this mapping is derived */
>> -	hda_nid_t pin_nid = port + 0x04;
>> +	hda_nid_t pin_nid;
>> +
>> +	if (!hdmi->drv_data->port_num) {
>> +		/* for legacy platforms */
>> +		pin_nid = port + 0x04;
>> +	} else if (port < hdmi->drv_data->port_num) {
>> +		/* get pin number from the pin2port mapping table */
>> +		pin_nid = hdmi->drv_data->port_map[port - 1];
>> +	} else {
>> +		dev_err(&hdev->dev, "Can't find the pin for port %d\n", port);
>> +		return;
>> +	}
>>   
>>   	dev_dbg(&hdev->dev, "%s: for pin:%d port=%d\n", __func__,
>>   							pin_nid, pipe);
>> @@ -1973,12 +2010,18 @@ static int hdac_hdmi_get_spk_alloc(struct hdac_device *hdev, int pcm_idx)
>>   	return port->eld.info.spk_alloc;
>>   }
>>   
>> +static struct hdac_hdmi_drv_data intel_icl_drv_data  = {
>> +	.vendor_nid = INTEL_VENDOR_NID_0x2,
>> +	.port_map = icl_pin2port_map,
>> +	.port_num = ARRAY_SIZE(icl_pin2port_map),
>> +};
>> +
>>   static struct hdac_hdmi_drv_data intel_glk_drv_data  = {
>> -	.vendor_nid = INTEL_GLK_VENDOR_NID,
>> +	.vendor_nid = INTEL_VENDOR_NID_0xb,
>>   };
>>   
>>   static struct hdac_hdmi_drv_data intel_drv_data  = {
>> -	.vendor_nid = INTEL_VENDOR_NID,
>> +	.vendor_nid = INTEL_VENDOR_NID_0x8,
>>   };
>>   
>>   static int hdac_hdmi_dev_probe(struct hdac_device *hdev)
>> @@ -2259,6 +2302,8 @@ static const struct hda_device_id hdmi_list[] = {
>>   						   &intel_glk_drv_data),
>>   	HDA_CODEC_EXT_ENTRY(0x8086280d, 0x100000, "Geminilake HDMI",
>>   						   &intel_glk_drv_data),
>> +	HDA_CODEC_EXT_ENTRY(0x8086280f, 0x100000, "Icelake HDMI",
>> +						   &intel_icl_drv_data),
>>   	{}
>>   };
>>   
>> -- 
>> 2.17.1
>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Takashi Iwai]

On Sun, 11 Nov 2018 16:10:10 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> On 11/11/18 2:55 AM, Takashi Iwai wrote:
> > On Sat, 10 Nov 2018 22:18:46 +0100,
> > Bard liao wrote:
> >> From: Bard liao <bard.liao@intel.com>
> >>
> >> Add Icelake device id. Also, Icelake's pin2port mapping table is
> >> complicated. So we use a mapping table to do the pin2port mapping.
> >>
> >> Signed-off-by: Bard liao <bard.liao@intel.com>
> > The recent code has already pin2port callback in drm_audio_component
> > ops.  This is called in sound/hda/hdac_component.c before eld notify
> > callback gets called.  So, use this callback instead of introducing
> > yet another one.
> Sorry Takashi, can you elaborate? What this patch does is modify the
> existing implementation of the .pin2port callback, not add a new
> one. The existing logic (return pin - 4) is no longer sufficient on
> IceLake.

The recent change added the pin2port call in
drm_audio_component_audio_ops.  You can put the ICL-specific call into
this if any specific conversion is needed.

> > Also, it'd be helpful if you fix the same for the legacy HD-audio HDMI
> > codec driver.
> Our intention was to revisit differences between legacy and non-legacy
> in a separate patch if that's all right with you. We've identified
> missing IDs and other things that should be fixed separately.

Sure, I don't mean to fix both in a single patch, but just to remind
you guys not to forget about that code path.


thanks,

Takashi

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Bard liao]

On 11/12/2018 1:35 AM, Takashi Iwai wrote:
> On Sun, 11 Nov 2018 16:10:10 +0100,
> Pierre-Louis Bossart wrote:
>>
>> On 11/11/18 2:55 AM, Takashi Iwai wrote:
>>> On Sat, 10 Nov 2018 22:18:46 +0100,
>>> Bard liao wrote:
>>>> From: Bard liao <bard.liao@intel.com>
>>>>
>>>> Add Icelake device id. Also, Icelake's pin2port mapping table is
>>>> complicated. So we use a mapping table to do the pin2port mapping.
>>>>
>>>> Signed-off-by: Bard liao <bard.liao@intel.com>
>>> The recent code has already pin2port callback in drm_audio_component
>>> ops.  This is called in sound/hda/hdac_component.c before eld notify
>>> callback gets called.  So, use this callback instead of introducing
>>> yet another one.
>> Sorry Takashi, can you elaborate? What this patch does is modify the
>> existing implementation of the .pin2port callback, not add a new
>> one. The existing logic (return pin - 4) is no longer sufficient on
>> IceLake.
> The recent change added the pin2port call in
> drm_audio_component_audio_ops.  You can put the ICL-specific call into
> this if any specific conversion is needed.

hdac_hdmi_pin2port is indeed the pin2port call back function in
drm_audio_component_audio_ops :)
What the patch did is to modify the current pin2port call back
function in drm_audio_component_audio_ops.

>>> Also, it'd be helpful if you fix the same for the legacy HD-audio HDMI
>>> codec driver.
>> Our intention was to revisit differences between legacy and non-legacy
>> in a separate patch if that's all right with you. We've identified
>> missing IDs and other things that should be fixed separately.
> Sure, I don't mean to fix both in a single patch, but just to remind
> you guys not to forget about that code path.
>
>
> thanks,
>
> Takashi
>

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Takashi Iwai]

On Mon, 12 Nov 2018 14:04:11 +0100,
Bard liao wrote:
> 
> On 11/12/2018 1:35 AM, Takashi Iwai wrote:
> > On Sun, 11 Nov 2018 16:10:10 +0100,
> > Pierre-Louis Bossart wrote:
> >>
> >> On 11/11/18 2:55 AM, Takashi Iwai wrote:
> >>> On Sat, 10 Nov 2018 22:18:46 +0100,
> >>> Bard liao wrote:
> >>>> From: Bard liao <bard.liao@intel.com>
> >>>>
> >>>> Add Icelake device id. Also, Icelake's pin2port mapping table is
> >>>> complicated. So we use a mapping table to do the pin2port mapping.
> >>>>
> >>>> Signed-off-by: Bard liao <bard.liao@intel.com>
> >>> The recent code has already pin2port callback in drm_audio_component
> >>> ops.  This is called in sound/hda/hdac_component.c before eld notify
> >>> callback gets called.  So, use this callback instead of introducing
> >>> yet another one.
> >> Sorry Takashi, can you elaborate? What this patch does is modify the
> >> existing implementation of the .pin2port callback, not add a new
> >> one. The existing logic (return pin - 4) is no longer sufficient on
> >> IceLake.
> > The recent change added the pin2port call in
> > drm_audio_component_audio_ops.  You can put the ICL-specific call into
> > this if any specific conversion is needed.
> 
> hdac_hdmi_pin2port is indeed the pin2port call back function in
> drm_audio_component_audio_ops :)
> What the patch did is to modify the current pin2port call back
> function in drm_audio_component_audio_ops.

Ah OK, then it's fine.


thanks,

Takashi

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Pierre-Louis Bossart]

On 11/10/18 3:18 PM, Bard liao wrote:
> From: Bard liao <bard.liao@intel.com>
>
> Add Icelake device id. Also, Icelake's pin2port mapping table is
> complicated. So we use a mapping table to do the pin2port mapping.

I double-checked the mappings in the hardware specs so:

Acked-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

>
> Signed-off-by: Bard liao <bard.liao@intel.com>
> ---
>   sound/soc/codecs/hdac_hdmi.c | 63 ++++++++++++++++++++++++++++++------
>   1 file changed, 54 insertions(+), 9 deletions(-)
>
> diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
> index 4e9854889a95..fac397326515 100644
> --- a/sound/soc/codecs/hdac_hdmi.c
> +++ b/sound/soc/codecs/hdac_hdmi.c
> @@ -121,8 +121,16 @@ struct hdac_hdmi_dai_port_map {
>   	struct hdac_hdmi_cvt *cvt;
>   };
>   
> +/*
> + * pin to port mapping table where the value indicate the pin number and
> + * the index indicate the port number with 1 base.
> + */
> +static const int icl_pin2port_map[] = {0x4, 0x6, 0x8, 0xa, 0xb};
> +
>   struct hdac_hdmi_drv_data {
>   	unsigned int vendor_nid;
> +	const int *port_map; /* pin to port mapping table */
> +	int port_num;
>   };
>   
>   struct hdac_hdmi_priv {
> @@ -1329,11 +1337,12 @@ static int hdac_hdmi_add_pin(struct hdac_device *hdev, hda_nid_t nid)
>   	return 0;
>   }
>   
> -#define INTEL_VENDOR_NID 0x08
> -#define INTEL_GLK_VENDOR_NID 0x0b
> +#define INTEL_VENDOR_NID_0x2 0x02
> +#define INTEL_VENDOR_NID_0x8 0x08
> +#define INTEL_VENDOR_NID_0xb 0x0b
>   #define INTEL_GET_VENDOR_VERB 0xf81
>   #define INTEL_SET_VENDOR_VERB 0x781
> -#define INTEL_EN_DP12			0x02 /* enable DP 1.2 features */
> +#define INTEL_EN_DP12		0x02 /* enable DP 1.2 features */
>   #define INTEL_EN_ALL_PIN_CVTS	0x01 /* enable 2nd & 3rd pins and convertors */
>   
>   static void hdac_hdmi_skl_enable_all_pins(struct hdac_device *hdev)
> @@ -1538,7 +1547,26 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_device *hdev,
>   
>   static int hdac_hdmi_pin2port(void *aptr, int pin)
>   {
> -	return pin - 4; /* map NID 0x05 -> port #1 */
> +	struct hdac_device *hdev = aptr;
> +	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
> +	const int *map = hdmi->drv_data->port_map;
> +	int i;
> +
> +	if (!hdmi->drv_data->port_num)
> +		return pin - 4; /* map NID 0x05 -> port #1 */
> +
> +	/*
> +	 * looking for the pin number in the mapping table and return
> +	 * the index which indicate the port number
> +	 */
> +	for (i = 0; i < hdmi->drv_data->port_num; i++) {
> +		if (pin == map[i])
> +			return i + 1;
> +	}
> +
> +	/* return -1 if pin number exceeds our expectation */
> +	dev_err(&hdev->dev, "Can't find the port for pin %d\n", pin);
> +	return -1;
>   }
>   
>   static void hdac_hdmi_eld_notify_cb(void *aptr, int port, int pipe)
> @@ -1549,9 +1577,18 @@ static void hdac_hdmi_eld_notify_cb(void *aptr, int port, int pipe)
>   	struct hdac_hdmi_port *hport = NULL;
>   	struct snd_soc_component *component = hdmi->component;
>   	int i;
> -
> -	/* Don't know how this mapping is derived */
> -	hda_nid_t pin_nid = port + 0x04;
> +	hda_nid_t pin_nid;
> +
> +	if (!hdmi->drv_data->port_num) {
> +		/* for legacy platforms */
> +		pin_nid = port + 0x04;
> +	} else if (port < hdmi->drv_data->port_num) {
> +		/* get pin number from the pin2port mapping table */
> +		pin_nid = hdmi->drv_data->port_map[port - 1];
> +	} else {
> +		dev_err(&hdev->dev, "Can't find the pin for port %d\n", port);
> +		return;
> +	}
>   
>   	dev_dbg(&hdev->dev, "%s: for pin:%d port=%d\n", __func__,
>   							pin_nid, pipe);
> @@ -1973,12 +2010,18 @@ static int hdac_hdmi_get_spk_alloc(struct hdac_device *hdev, int pcm_idx)
>   	return port->eld.info.spk_alloc;
>   }
>   
> +static struct hdac_hdmi_drv_data intel_icl_drv_data  = {
> +	.vendor_nid = INTEL_VENDOR_NID_0x2,
> +	.port_map = icl_pin2port_map,
> +	.port_num = ARRAY_SIZE(icl_pin2port_map),
> +};
> +
>   static struct hdac_hdmi_drv_data intel_glk_drv_data  = {
> -	.vendor_nid = INTEL_GLK_VENDOR_NID,
> +	.vendor_nid = INTEL_VENDOR_NID_0xb,
>   };
>   
>   static struct hdac_hdmi_drv_data intel_drv_data  = {
> -	.vendor_nid = INTEL_VENDOR_NID,
> +	.vendor_nid = INTEL_VENDOR_NID_0x8,
>   };
>   
>   static int hdac_hdmi_dev_probe(struct hdac_device *hdev)
> @@ -2259,6 +2302,8 @@ static const struct hda_device_id hdmi_list[] = {
>   						   &intel_glk_drv_data),
>   	HDA_CODEC_EXT_ENTRY(0x8086280d, 0x100000, "Geminilake HDMI",
>   						   &intel_glk_drv_data),
> +	HDA_CODEC_EXT_ENTRY(0x8086280f, 0x100000, "Icelake HDMI",
> +						   &intel_icl_drv_data),
>   	{}
>   };
>   

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Pierre-Louis Bossart]

>>> Also, it'd be helpful if you fix the same for the legacy HD-audio HDMI
>>> codec driver.
>> Our intention was to revisit differences between legacy and non-legacy
>> in a separate patch if that's all right with you. We've identified
>> missing IDs and other things that should be fixed separately.
> Sure, I don't mean to fix both in a single patch, but just to remind
> you guys not to forget about that code path.

Yes, this is very much on our radar. I don't like the current approach 
where patch_hdmi.c and hdac_hdmi.c have duplicated/different definitions 
for the same things or capabilities that can't be traced back to 
hardware documentation or known issues. the main issue I am facing with 
the legacy path is ironically validation. Our team necessarily have 
access to all commercially-available platforms listed in those files or 
when we do it's not necessarily easy to work with the latest kernel or 
handle BIOS issues - we'll do what we can though.

And btw the big topic is still how we provide distributions the means to 
handle a 'graceful' fallback from DSP-enabled solutions (SST or SOF) to 
legacy HDAudio, it's already popped up for cases where we have HDaudio 
solutions with DMICs.

-Pierre

Applied "ASoC: Intel: hdac_hdmi: add Icelake support" to the asoc tree

[Mark Brown]

The patch

   ASoC: Intel: hdac_hdmi: add Icelake support

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 019033c854a20e10f691f6cc0e897df8817d9521 Mon Sep 17 00:00:00 2001
From: Bard liao <bard.liao@intel.com>
Date: Sun, 11 Nov 2018 05:18:46 +0800
Subject: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

Add Icelake device id. Also, Icelake's pin2port mapping table is
complicated. So we use a mapping table to do the pin2port mapping.

Signed-off-by: Bard liao <bard.liao@intel.com>
Acked-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/hdac_hdmi.c | 63 ++++++++++++++++++++++++++++++------
 1 file changed, 54 insertions(+), 9 deletions(-)

diff --git a/sound/soc/codecs/hdac_hdmi.c b/sound/soc/codecs/hdac_hdmi.c
index 4e9854889a95..fac397326515 100644
--- a/sound/soc/codecs/hdac_hdmi.c
+++ b/sound/soc/codecs/hdac_hdmi.c
@@ -121,8 +121,16 @@ struct hdac_hdmi_dai_port_map {
 	struct hdac_hdmi_cvt *cvt;
 };
 
+/*
+ * pin to port mapping table where the value indicate the pin number and
+ * the index indicate the port number with 1 base.
+ */
+static const int icl_pin2port_map[] = {0x4, 0x6, 0x8, 0xa, 0xb};
+
 struct hdac_hdmi_drv_data {
 	unsigned int vendor_nid;
+	const int *port_map; /* pin to port mapping table */
+	int port_num;
 };
 
 struct hdac_hdmi_priv {
@@ -1329,11 +1337,12 @@ static int hdac_hdmi_add_pin(struct hdac_device *hdev, hda_nid_t nid)
 	return 0;
 }
 
-#define INTEL_VENDOR_NID 0x08
-#define INTEL_GLK_VENDOR_NID 0x0b
+#define INTEL_VENDOR_NID_0x2 0x02
+#define INTEL_VENDOR_NID_0x8 0x08
+#define INTEL_VENDOR_NID_0xb 0x0b
 #define INTEL_GET_VENDOR_VERB 0xf81
 #define INTEL_SET_VENDOR_VERB 0x781
-#define INTEL_EN_DP12			0x02 /* enable DP 1.2 features */
+#define INTEL_EN_DP12		0x02 /* enable DP 1.2 features */
 #define INTEL_EN_ALL_PIN_CVTS	0x01 /* enable 2nd & 3rd pins and convertors */
 
 static void hdac_hdmi_skl_enable_all_pins(struct hdac_device *hdev)
@@ -1538,7 +1547,26 @@ static int hdac_hdmi_parse_and_map_nid(struct hdac_device *hdev,
 
 static int hdac_hdmi_pin2port(void *aptr, int pin)
 {
-	return pin - 4; /* map NID 0x05 -> port #1 */
+	struct hdac_device *hdev = aptr;
+	struct hdac_hdmi_priv *hdmi = hdev_to_hdmi_priv(hdev);
+	const int *map = hdmi->drv_data->port_map;
+	int i;
+
+	if (!hdmi->drv_data->port_num)
+		return pin - 4; /* map NID 0x05 -> port #1 */
+
+	/*
+	 * looking for the pin number in the mapping table and return
+	 * the index which indicate the port number
+	 */
+	for (i = 0; i < hdmi->drv_data->port_num; i++) {
+		if (pin == map[i])
+			return i + 1;
+	}
+
+	/* return -1 if pin number exceeds our expectation */
+	dev_err(&hdev->dev, "Can't find the port for pin %d\n", pin);
+	return -1;
 }
 
 static void hdac_hdmi_eld_notify_cb(void *aptr, int port, int pipe)
@@ -1549,9 +1577,18 @@ static void hdac_hdmi_eld_notify_cb(void *aptr, int port, int pipe)
 	struct hdac_hdmi_port *hport = NULL;
 	struct snd_soc_component *component = hdmi->component;
 	int i;
-
-	/* Don't know how this mapping is derived */
-	hda_nid_t pin_nid = port + 0x04;
+	hda_nid_t pin_nid;
+
+	if (!hdmi->drv_data->port_num) {
+		/* for legacy platforms */
+		pin_nid = port + 0x04;
+	} else if (port < hdmi->drv_data->port_num) {
+		/* get pin number from the pin2port mapping table */
+		pin_nid = hdmi->drv_data->port_map[port - 1];
+	} else {
+		dev_err(&hdev->dev, "Can't find the pin for port %d\n", port);
+		return;
+	}
 
 	dev_dbg(&hdev->dev, "%s: for pin:%d port=%d\n", __func__,
 							pin_nid, pipe);
@@ -1973,12 +2010,18 @@ static int hdac_hdmi_get_spk_alloc(struct hdac_device *hdev, int pcm_idx)
 	return port->eld.info.spk_alloc;
 }
 
+static struct hdac_hdmi_drv_data intel_icl_drv_data  = {
+	.vendor_nid = INTEL_VENDOR_NID_0x2,
+	.port_map = icl_pin2port_map,
+	.port_num = ARRAY_SIZE(icl_pin2port_map),
+};
+
 static struct hdac_hdmi_drv_data intel_glk_drv_data  = {
-	.vendor_nid = INTEL_GLK_VENDOR_NID,
+	.vendor_nid = INTEL_VENDOR_NID_0xb,
 };
 
 static struct hdac_hdmi_drv_data intel_drv_data  = {
-	.vendor_nid = INTEL_VENDOR_NID,
+	.vendor_nid = INTEL_VENDOR_NID_0x8,
 };
 
 static int hdac_hdmi_dev_probe(struct hdac_device *hdev)
@@ -2259,6 +2302,8 @@ static const struct hda_device_id hdmi_list[] = {
 						   &intel_glk_drv_data),
 	HDA_CODEC_EXT_ENTRY(0x8086280d, 0x100000, "Geminilake HDMI",
 						   &intel_glk_drv_data),
+	HDA_CODEC_EXT_ENTRY(0x8086280f, 0x100000, "Icelake HDMI",
+						   &intel_icl_drv_data),
 	{}
 };
 
-- 
2.19.1

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Takashi Iwai]

On Mon, 12 Nov 2018 15:00:36 +0100,
Pierre-Louis Bossart wrote:
> 
> And btw the big topic is still how we provide distributions the means
> to handle a 'graceful' fallback from DSP-enabled solutions (SST or
> SOF) to legacy HDAudio, it's already popped up for cases where we have
> HDaudio solutions with DMICs.

Yeah, that's a long-standing problem.  I've experimented some
scenarios in the past, and the conclusion is that there is no really
working fallback mechanism in general in Linux driver binding.
That is, the only reasonable way seems to make a dedicated driver for
the specific PCI ID (SKL+) doing the probe-and-fallback by itself,
while excluding these IDs from other existing driver entries.

I'd love to proceed this but unfortunately I have no machine that can
run SKL+ SST driver right now.  I have a new CFL devel box, but it has
no support (PCI ID 8086:a348) as well as no firmware...


thanks,

Takashi

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Pierre-Louis Bossart]

On 11/14/18 8:53 AM, Takashi Iwai wrote:
> On Mon, 12 Nov 2018 15:00:36 +0100,
> Pierre-Louis Bossart wrote:
>> And btw the big topic is still how we provide distributions the means
>> to handle a 'graceful' fallback from DSP-enabled solutions (SST or
>> SOF) to legacy HDAudio, it's already popped up for cases where we have
>> HDaudio solutions with DMICs.
> Yeah, that's a long-standing problem.  I've experimented some
> scenarios in the past, and the conclusion is that there is no really
> working fallback mechanism in general in Linux driver binding.
> That is, the only reasonable way seems to make a dedicated driver for
> the specific PCI ID (SKL+) doing the probe-and-fallback by itself,
> while excluding these IDs from other existing driver entries.
Yep, agree, the PCI folks I talked to came to the same conclusion. 
Except that we made things more complicated with SOF, so it's really a 
choice between 3 drivers...
>
> I'd love to proceed this but unfortunately I have no machine that can
> run SKL+ SST driver right now.  I have a new CFL devel box, but it has
> no support (PCI ID 8086:a348) as well as no firmware...

Looks like the CFL is already supported in Linux

sound/pci/hda/hda_intel.c:#define IS_CFL(pci) ((pci)->vendor == 0x8086 
&& (pci)->device == 0xa348)

So in theory all we need to add is a new table entry in skl.c, e.g. with 
the following untested code. I'll have to check if this is correct 
offline but it'd allow you to test the probe part.

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index ac0b4ff21acc..375f4b60e515 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -1098,6 +1098,9 @@ static const struct pci_device_id skl_ids[] = {
         /* CNL */
         { PCI_DEVICE(0x8086, 0x9dc8),
                 .driver_data = (unsigned 
long)&snd_soc_acpi_intel_cnl_machines},
+       /* CFL */
+       { PCI_DEVICE(0x8086, 0xa348),
+               .driver_data = (unsigned 
long)&snd_soc_acpi_intel_cnl_machines}
         { 0, }
  };



_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Takashi Iwai]

On Wed, 14 Nov 2018 17:23:08 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> On 11/14/18 8:53 AM, Takashi Iwai wrote:
> > On Mon, 12 Nov 2018 15:00:36 +0100,
> > Pierre-Louis Bossart wrote:
> >> And btw the big topic is still how we provide distributions the means
> >> to handle a 'graceful' fallback from DSP-enabled solutions (SST or
> >> SOF) to legacy HDAudio, it's already popped up for cases where we have
> >> HDaudio solutions with DMICs.
> > Yeah, that's a long-standing problem.  I've experimented some
> > scenarios in the past, and the conclusion is that there is no really
> > working fallback mechanism in general in Linux driver binding.
> > That is, the only reasonable way seems to make a dedicated driver for
> > the specific PCI ID (SKL+) doing the probe-and-fallback by itself,
> > while excluding these IDs from other existing driver entries.
> Yep, agree, the PCI folks I talked to came to the same
> conclusion. Except that we made things more complicated with SOF, so
> it's really a choice between 3 drivers...

Heh, English plural has no distinguish between 2 and 3 :)

> > I'd love to proceed this but unfortunately I have no machine that can
> > run SKL+ SST driver right now.  I have a new CFL devel box, but it has
> > no support (PCI ID 8086:a348) as well as no firmware...
> 
> Looks like the CFL is already supported in Linux
> 
> sound/pci/hda/hda_intel.c:#define IS_CFL(pci) ((pci)->vendor == 0x8086
> && (pci)->device == 0xa348)

Yes, the legacy driver works fine.

> So in theory all we need to add is a new table entry in skl.c,
> e.g. with the following untested code. I'll have to check if this is
> correct offline but it'd allow you to test the probe part.
> 
> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> index ac0b4ff21acc..375f4b60e515 100644
> --- a/sound/soc/intel/skylake/skl.c
> +++ b/sound/soc/intel/skylake/skl.c
> @@ -1098,6 +1098,9 @@ static const struct pci_device_id skl_ids[] = {
>         /* CNL */
>         { PCI_DEVICE(0x8086, 0x9dc8),
>                 .driver_data = (unsigned
> long)&snd_soc_acpi_intel_cnl_machines},
> +       /* CFL */
> +       { PCI_DEVICE(0x8086, 0xa348),
> +               .driver_data = (unsigned
> long)&snd_soc_acpi_intel_cnl_machines}
>         { 0, }
>  };

I've already tried it but this doesn't seem enough.  The similar
addition for 0xa348 is needed in skl-messages.c for clk setup.
(BTW, reloading the module after this error triggered the leftover
 sysfs entries; you can try to inject the error and reproduce it.)

After these changes, the driver was loaded, but it still complains
about the lack of firmware (both SOF and fallback one).
The binding with HDMI codec seems working, but the analog one is still
missing.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Pierre-Louis Bossart]

>> So in theory all we need to add is a new table entry in skl.c,
>> e.g. with the following untested code. I'll have to check if this is
>> correct offline but it'd allow you to test the probe part.
>>
>> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
>> index ac0b4ff21acc..375f4b60e515 100644
>> --- a/sound/soc/intel/skylake/skl.c
>> +++ b/sound/soc/intel/skylake/skl.c
>> @@ -1098,6 +1098,9 @@ static const struct pci_device_id skl_ids[] = {
>>          /* CNL */
>>          { PCI_DEVICE(0x8086, 0x9dc8),
>>                  .driver_data = (unsigned
>> long)&snd_soc_acpi_intel_cnl_machines},
>> +       /* CFL */
>> +       { PCI_DEVICE(0x8086, 0xa348),
>> +               .driver_data = (unsigned
>> long)&snd_soc_acpi_intel_cnl_machines}
>>          { 0, }
>>   };
> I've already tried it but this doesn't seem enough.  The similar
> addition for 0xa348 is needed in skl-messages.c for clk setup.
> (BTW, reloading the module after this error triggered the leftover
>   sysfs entries; you can try to inject the error and reproduce it.)
>
> After these changes, the driver was loaded, but it still complains
> about the lack of firmware (both SOF and fallback one).
> The binding with HDMI codec seems working, but the analog one is still
> missing.

Is this lack of firmware or firmware that won't boot due to 
configuration issues?

If you can share the dmesg log somewhere it'd be interesting, I am 
trying to help fix support for WHL and CFL *should* be similar (I 
haven't checked the actual specs though).

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Takashi Iwai]

On Wed, 14 Nov 2018 17:41:27 +0100,
Pierre-Louis Bossart wrote:
> 
> 
> >> So in theory all we need to add is a new table entry in skl.c,
> >> e.g. with the following untested code. I'll have to check if this is
> >> correct offline but it'd allow you to test the probe part.
> >>
> >> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> >> index ac0b4ff21acc..375f4b60e515 100644
> >> --- a/sound/soc/intel/skylake/skl.c
> >> +++ b/sound/soc/intel/skylake/skl.c
> >> @@ -1098,6 +1098,9 @@ static const struct pci_device_id skl_ids[] = {
> >>          /* CNL */
> >>          { PCI_DEVICE(0x8086, 0x9dc8),
> >>                  .driver_data = (unsigned
> >> long)&snd_soc_acpi_intel_cnl_machines},
> >> +       /* CFL */
> >> +       { PCI_DEVICE(0x8086, 0xa348),
> >> +               .driver_data = (unsigned
> >> long)&snd_soc_acpi_intel_cnl_machines}
> >>          { 0, }
> >>   };
> > I've already tried it but this doesn't seem enough.  The similar
> > addition for 0xa348 is needed in skl-messages.c for clk setup.
> > (BTW, reloading the module after this error triggered the leftover
> >   sysfs entries; you can try to inject the error and reproduce it.)
> >
> > After these changes, the driver was loaded, but it still complains
> > about the lack of firmware (both SOF and fallback one).
> > The binding with HDMI codec seems working, but the analog one is still
> > missing.
> 
> Is this lack of firmware or firmware that won't boot due to
> configuration issues?

The former.  I'll check again after updating the linux-firmware
package.

> If you can share the dmesg log somewhere it'd be interesting, I am
> trying to help fix support for WHL and CFL *should* be similar (I
> haven't checked the actual specs though).

OK, will send you later.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Pierre-Louis Bossart]

On 11/14/18 10:44 AM, Takashi Iwai wrote:
> On Wed, 14 Nov 2018 17:41:27 +0100,
> Pierre-Louis Bossart wrote:
>>
>>>> So in theory all we need to add is a new table entry in skl.c,
>>>> e.g. with the following untested code. I'll have to check if this is
>>>> correct offline but it'd allow you to test the probe part.
>>>>
>>>> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
>>>> index ac0b4ff21acc..375f4b60e515 100644
>>>> --- a/sound/soc/intel/skylake/skl.c
>>>> +++ b/sound/soc/intel/skylake/skl.c
>>>> @@ -1098,6 +1098,9 @@ static const struct pci_device_id skl_ids[] = {
>>>>           /* CNL */
>>>>           { PCI_DEVICE(0x8086, 0x9dc8),
>>>>                   .driver_data = (unsigned
>>>> long)&snd_soc_acpi_intel_cnl_machines},
>>>> +       /* CFL */
>>>> +       { PCI_DEVICE(0x8086, 0xa348),
>>>> +               .driver_data = (unsigned
>>>> long)&snd_soc_acpi_intel_cnl_machines}
>>>>           { 0, }
>>>>    };
>>> I've already tried it but this doesn't seem enough.  The similar
>>> addition for 0xa348 is needed in skl-messages.c for clk setup.
>>> (BTW, reloading the module after this error triggered the leftover
>>>    sysfs entries; you can try to inject the error and reproduce it.)
>>>
>>> After these changes, the driver was loaded, but it still complains
>>> about the lack of firmware (both SOF and fallback one).
>>> The binding with HDMI codec seems working, but the analog one is still
>>> missing.
>> Is this lack of firmware or firmware that won't boot due to
>> configuration issues?
> The former.  I'll check again after updating the linux-firmware
> package.

Strange, the CNL firmware has been around for some time.

git log intel/dsp_fw_cnl.bin
commit 89c62115f14dd8f414dfd2702913a6f1fcac3287
Author: Vinod Koul <vinod.koul@intel.com>
Date:   Mon Dec 4 09:04:57 2017 +0530

     linux-firmware: intel: Add Cannonlake audio firmware


>
>> If you can share the dmesg log somewhere it'd be interesting, I am
>> trying to help fix support for WHL and CFL *should* be similar (I
>> haven't checked the actual specs though).
> OK, will send you later.
>
>
> thanks,
>
> Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Takashi Iwai]

[-- Attachment #1: Type: text/plain, Size: 1630 bytes --]

On Wed, 14 Nov 2018 15:53:23 +0100,
Takashi Iwai wrote:
> 
> On Mon, 12 Nov 2018 15:00:36 +0100,
> Pierre-Louis Bossart wrote:
> > 
> > And btw the big topic is still how we provide distributions the means
> > to handle a 'graceful' fallback from DSP-enabled solutions (SST or
> > SOF) to legacy HDAudio, it's already popped up for cases where we have
> > HDaudio solutions with DMICs.
> 
> Yeah, that's a long-standing problem.  I've experimented some
> scenarios in the past, and the conclusion is that there is no really
> working fallback mechanism in general in Linux driver binding.
> That is, the only reasonable way seems to make a dedicated driver for
> the specific PCI ID (SKL+) doing the probe-and-fallback by itself,
> while excluding these IDs from other existing driver entries.
> 
> I'd love to proceed this but unfortunately I have no machine that can
> run SKL+ SST driver right now.  I have a new CFL devel box, but it has
> no support (PCI ID 8086:a348) as well as no firmware...

So, with a working machine, I could finally hack this a bit.
Below is a freshly cooked patch(set).  The first one is just to add
the support for my CFL machine, and another one is to add the fallback
binding with the legacy HD-audio on snd-soc-skl.

The changes aren't that big.  And you can still control the binding
via a module option, e.g. snd_soc_skl.legacy=1 will let it bound only
with the legacy driver, snd_soc_skl.legacy=2 for ASoC only.  The
default is 0, the fallback to legacy if ASoC binding fails.

It's still a PoC, no proper patch description is put yet.

Comments / suggestions appreciated.


thanks,

Takashi


[-- Attachment #2: 0001-ASoC-intel-skl-Add-CFL-S-support.patch --]
[-- Type: application/octet-stream, Size: 1585 bytes --]

>From 5dc96e698b71a55a71c667a352c0664d8e9c9d8e Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Fri, 16 Nov 2018 11:46:29 +0100
Subject: [PATCH 1/2] ASoC: intel: skl: Add CFL-S support

It's with CNP, supposed to be equivalent with CNL entry.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/intel/skylake/skl-messages.c | 8 ++++++++
 sound/soc/intel/skylake/skl.c          | 3 +++
 2 files changed, 11 insertions(+)

diff --git a/sound/soc/intel/skylake/skl-messages.c b/sound/soc/intel/skylake/skl-messages.c
index 8bfb8b0fa3d5..b0e6fb93eaf8 100644
--- a/sound/soc/intel/skylake/skl-messages.c
+++ b/sound/soc/intel/skylake/skl-messages.c
@@ -247,6 +247,14 @@ static const struct skl_dsp_ops dsp_ops[] = {
 		.init_fw = cnl_sst_init_fw,
 		.cleanup = cnl_sst_dsp_cleanup
 	},
+	{
+		.id = 0xa348,
+		.num_cores = 4,
+		.loader_ops = bxt_get_loader_ops,
+		.init = cnl_sst_dsp_init,
+		.init_fw = cnl_sst_init_fw,
+		.cleanup = cnl_sst_dsp_cleanup
+	},
 };
 
 const struct skl_dsp_ops *skl_get_dsp_ops(int pci_id)
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 29225623b4b4..f6d8cc23c471 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -1102,6 +1102,9 @@ static const struct pci_device_id skl_ids[] = {
 	/* CNL */
 	{ PCI_DEVICE(0x8086, 0x9dc8),
 		.driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
+	/* CFL */
+	{ PCI_DEVICE(0x8086, 0xa348),
+		.driver_data = (unsigned long)&snd_soc_acpi_intel_cnl_machines},
 	{ 0, }
 };
 MODULE_DEVICE_TABLE(pci, skl_ids);
-- 
2.19.1


[-- Attachment #3: 0002-ALSA-hda-Allow-fallback-binding-with-legacy-HD-audio.patch --]
[-- Type: application/octet-stream, Size: 10950 bytes --]

>From fc6edd975ea6f21f23fec93ace130a482c1ad9bd Mon Sep 17 00:00:00 2001
From: Takashi Iwai <tiwai@suse.de>
Date: Fri, 16 Nov 2018 12:57:46 +0100
Subject: [PATCH 2/2] ALSA: hda: Allow fallback binding with legacy HD-audio
 for Intel SKL+

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 include/sound/hdaudio.h        |  6 +++
 sound/pci/hda/hda_controller.h |  2 +-
 sound/pci/hda/hda_intel.c      | 50 ++++++++++++++++----
 sound/soc/intel/Kconfig        |  8 ++++
 sound/soc/intel/skylake/skl.c  | 83 ++++++++++++++++++++++++++++++++--
 5 files changed, 137 insertions(+), 12 deletions(-)

diff --git a/include/sound/hdaudio.h b/include/sound/hdaudio.h
index cd1773d0e08f..2ed67f315962 100644
--- a/include/sound/hdaudio.h
+++ b/include/sound/hdaudio.h
@@ -8,6 +8,7 @@
 
 #include <linux/device.h>
 #include <linux/interrupt.h>
+#include <linux/pci.h>
 #include <linux/pm_runtime.h>
 #include <linux/timecounter.h>
 #include <sound/core.h>
@@ -634,4 +635,9 @@ static inline unsigned int snd_array_index(struct snd_array *array, void *ptr)
 	for ((idx) = 0, (ptr) = (array)->list; (idx) < (array)->used; \
 	     (ptr) = snd_array_elem(array, ++(idx)))
 
+/* shared resource with ASoC and legacy HD-audio drivers */
+#ifdef CONFIG_SND_SOC_INTEL_SKL_LEGACY_SUPPORT
+const struct pci_driver *snd_hda_intel_probe(struct pci_dev *pci);
+#endif
+
 #endif /* __SOUND_HDAUDIO_H */
diff --git a/sound/pci/hda/hda_controller.h b/sound/pci/hda/hda_controller.h
index c95097bb5a0c..2351115f922e 100644
--- a/sound/pci/hda/hda_controller.h
+++ b/sound/pci/hda/hda_controller.h
@@ -37,7 +37,7 @@
 #else
 #define AZX_DCAPS_I915_COMPONENT 0		/* NOP */
 #endif
-/* 14 unused */
+#define AZX_DCAPS_INTEL_SHARED	(1 << 14)	/* shared with ASoC */
 #define AZX_DCAPS_CTX_WORKAROUND (1 << 15)	/* X-Fi workaround */
 #define AZX_DCAPS_POSFIX_LPIB	(1 << 16)	/* Use LPIB as default */
 /* 17 unused */
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index d8eb2b5f51ae..92d1b96f6ef1 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -2081,8 +2081,8 @@ static const struct hda_controller_ops pci_hda_ops = {
 	.link_power = azx_intel_link_power,
 };
 
-static int azx_probe(struct pci_dev *pci,
-		     const struct pci_device_id *pci_id)
+static int __azx_probe(struct pci_dev *pci, const struct pci_device_id *pci_id,
+		       bool skip_shared)
 {
 	static int dev;
 	struct snd_card *card;
@@ -2091,6 +2091,10 @@ static int azx_probe(struct pci_dev *pci,
 	bool schedule_probe;
 	int err;
 
+	/* skip the entry if it's shared with ASoC */
+	if (skip_shared && (pci_id->driver_data & AZX_DCAPS_INTEL_SHARED))
+		return -ENODEV;
+
 	if (dev >= SNDRV_CARDS)
 		return -ENODEV;
 	if (!enable[dev]) {
@@ -2158,6 +2162,12 @@ static int azx_probe(struct pci_dev *pci,
 	return err;
 }
 
+static int azx_probe(struct pci_dev *pci, const struct pci_device_id *pci_id)
+{
+	return __azx_probe(pci, pci_id,
+			   IS_ENABLED(CONFIG_SND_SOC_INTEL_SKL_LEGACY_SUPPORT));
+}
+
 #ifdef CONFIG_PM
 /* On some boards setting power_save to a non 0 value leads to clicking /
  * popping sounds when ever we enter/leave powersaving mode. Ideally we would
@@ -2406,34 +2416,40 @@ static const struct pci_device_id azx_ids[] = {
 	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
 	/* Sunrise Point-LP */
 	{ PCI_DEVICE(0x8086, 0x9d70),
-	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
+	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
+	  AZX_DCAPS_INTEL_SHARED },
 	/* Kabylake */
 	{ PCI_DEVICE(0x8086, 0xa171),
 	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
 	/* Kabylake-LP */
 	{ PCI_DEVICE(0x8086, 0x9d71),
-	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
+	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
+	  AZX_DCAPS_INTEL_SHARED },
 	/* Kabylake-H */
 	{ PCI_DEVICE(0x8086, 0xa2f0),
 	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE },
 	/* Coffelake */
 	{ PCI_DEVICE(0x8086, 0xa348),
-	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE},
+	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
+	  AZX_DCAPS_INTEL_SHARED },
 	/* Cannonlake */
 	{ PCI_DEVICE(0x8086, 0x9dc8),
-	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE},
+	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
+	  AZX_DCAPS_INTEL_SHARED },
 	/* Icelake */
 	{ PCI_DEVICE(0x8086, 0x34c8),
 	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE},
 	/* Broxton-P(Apollolake) */
 	{ PCI_DEVICE(0x8086, 0x5a98),
-	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON },
+	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON |
+	  AZX_DCAPS_INTEL_SHARED },
 	/* Broxton-T */
 	{ PCI_DEVICE(0x8086, 0x1a98),
 	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON },
 	/* Gemini-Lake */
 	{ PCI_DEVICE(0x8086, 0x3198),
-	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON },
+	  .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_BROXTON |
+	  AZX_DCAPS_INTEL_SHARED },
 	/* Haswell */
 	{ PCI_DEVICE(0x8086, 0x0a0c),
 	  .driver_data = AZX_DRIVER_HDMI | AZX_DCAPS_INTEL_HASWELL },
@@ -2650,4 +2666,22 @@ static struct pci_driver azx_driver = {
 	},
 };
 
+#ifdef CONFIG_SND_SOC_INTEL_SKL_LEGACY_SUPPORT
+const struct pci_driver *
+snd_hda_intel_probe(struct pci_dev *pci)
+{
+	const struct pci_device_id *pci_id;
+	int ret;
+
+	pci_id = pci_match_id(azx_ids, pci);
+	if (!pci_id)
+		return ERR_PTR(-ENODEV);
+	ret = __azx_probe(pci, pci_id, false);
+	if (ret < 0)
+		return ERR_PTR(ret);
+	return &azx_driver;
+}
+EXPORT_SYMBOL_GPL(snd_hda_intel_probe);
+#endif
+
 module_pci_driver(azx_driver);
diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index 0caa1f4eb94d..f19ac24af272 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -117,6 +117,14 @@ config SND_SOC_INTEL_SKYLAKE
 	  GeminiLake or CannonLake platform with the DSP enabled in the BIOS
 	  then enable this option by saying Y or m.
 
+config SND_SOC_INTEL_SKL_LEGACY_SUPPORT
+	bool "Fallback legacy HD-audio binding"
+	depends on SND_SOC_INTEL_SKYLAKE
+	depends on SND_HDA_INTEL=y || SND_SOC_INTEL_SKYLAKE=SND_HDA_INTEL
+	help
+	  Fallback binding with the legacy HD-audio driver when no DSP is
+	  found
+
 config SND_SOC_ACPI_INTEL_MATCH
 	tristate
 	select SND_SOC_ACPI if ACPI
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index f6d8cc23c471..427bbda38805 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -39,6 +39,31 @@
 #include "skl-sst-ipc.h"
 #include "../../../soc/codecs/hdac_hda.h"
 
+enum {
+	SKL_BIND_AUTO,		/* fallback to legacy driver */
+	SKL_BIND_LEGACY,	/* bind only with legacy driver */
+	SKL_BIND_ASOC		/* bind only with ASoC driver */
+};
+
+#ifdef CONFIG_SND_SOC_INTEL_SKL_LEGACY_SUPPORT
+static const struct pci_driver *skl_fallback_driver;
+#define FALLBACK_PM_CALL(dev, method)					\
+	do {								\
+		if (skl_fallback_driver &&				\
+		    skl_fallback_driver->driver.pm->method)		\
+			return skl_fallback_driver->driver.pm->method(dev); \
+	} while (0)
+
+static int skl_legacy_binding;
+module_param_named(legacy, skl_legacy_binding, int, 0444);
+MODULE_PARM_DESC(legacy, "Binding with legacy HD-audio (0=fallback, 1=only legacy, 2=only asoc");
+#else
+#define skl_fallback_driver	NULL
+#define FALLBACK_PM_CALL(dev, method) do {} while (0)
+#define skl_legacy_binding	SKL_BIND_ASOC
+#endif
+
+
 /*
  * initialize the PCI registers
  */
@@ -262,6 +287,9 @@ static int skl_suspend_late(struct device *dev)
 	struct hdac_bus *bus = pci_get_drvdata(pci);
 	struct skl *skl = bus_to_skl(bus);
 
+	if (skl_fallback_driver)
+		return 0;
+
 	return skl_suspend_late_dsp(skl);
 }
 
@@ -311,6 +339,8 @@ static int skl_suspend(struct device *dev)
 	struct skl *skl  = bus_to_skl(bus);
 	int ret = 0;
 
+	FALLBACK_PM_CALL(dev, suspend);
+
 	/*
 	 * Do not suspend if streams which are marked ignore suspend are
 	 * running, we need to save the state for these and continue
@@ -349,6 +379,8 @@ static int skl_resume(struct device *dev)
 	struct hdac_ext_link *hlink = NULL;
 	int ret;
 
+	FALLBACK_PM_CALL(dev, resume);
+
 	/* Turned OFF in HDMI codec driver after codec reconfiguration */
 	if (IS_ENABLED(CONFIG_SND_SOC_HDAC_HDMI)) {
 		ret = snd_hdac_display_power(bus, true);
@@ -403,6 +435,8 @@ static int skl_runtime_suspend(struct device *dev)
 	struct pci_dev *pci = to_pci_dev(dev);
 	struct hdac_bus *bus = pci_get_drvdata(pci);
 
+	FALLBACK_PM_CALL(dev, runtime_suspend);
+
 	dev_dbg(bus->dev, "in %s\n", __func__);
 
 	return _skl_suspend(bus);
@@ -413,15 +447,24 @@ static int skl_runtime_resume(struct device *dev)
 	struct pci_dev *pci = to_pci_dev(dev);
 	struct hdac_bus *bus = pci_get_drvdata(pci);
 
+	FALLBACK_PM_CALL(dev, runtime_resume);
+
 	dev_dbg(bus->dev, "in %s\n", __func__);
 
 	return _skl_resume(bus);
 }
+
+static int skl_runtime_idle(struct device *dev)
+{
+	FALLBACK_PM_CALL(dev, runtime_idle);
+	return 0;
+}
 #endif /* CONFIG_PM */
 
 static const struct dev_pm_ops skl_pm = {
 	SET_SYSTEM_SLEEP_PM_OPS(skl_suspend, skl_resume)
-	SET_RUNTIME_PM_OPS(skl_runtime_suspend, skl_runtime_resume, NULL)
+	SET_RUNTIME_PM_OPS(skl_runtime_suspend, skl_runtime_resume,
+			   skl_runtime_idle)
 	.suspend_late = skl_suspend_late,
 };
 
@@ -955,8 +998,8 @@ static int skl_first_init(struct hdac_bus *bus)
 	return skl_init_chip(bus, true);
 }
 
-static int skl_probe(struct pci_dev *pci,
-		     const struct pci_device_id *pci_id)
+static int __skl_probe(struct pci_dev *pci,
+		       const struct pci_device_id *pci_id)
 {
 	struct skl *skl;
 	struct hdac_bus *bus = NULL;
@@ -1037,6 +1080,25 @@ static int skl_probe(struct pci_dev *pci,
 	return err;
 }
 
+static int skl_probe(struct pci_dev *pci,
+		     const struct pci_device_id *pci_id)
+{
+	int ret = -ENODEV;
+
+	if (skl_legacy_binding != SKL_BIND_LEGACY)
+		ret = __skl_probe(pci, pci_id);
+
+#ifdef CONFIG_SND_SOC_INTEL_SKL_LEGACY_SUPPORT
+	if (ret < 0 && skl_legacy_binding != SKL_BIND_ASOC) {
+		skl_fallback_driver = snd_hda_intel_probe(pci);
+		ret = PTR_ERR_OR_ZERO(skl_fallback_driver);
+		if (ret)
+			skl_fallback_driver = NULL;
+	}
+#endif
+	return ret;
+}
+
 static void skl_shutdown(struct pci_dev *pci)
 {
 	struct hdac_bus *bus = pci_get_drvdata(pci);
@@ -1044,6 +1106,13 @@ static void skl_shutdown(struct pci_dev *pci)
 	struct hdac_ext_stream *stream;
 	struct skl *skl;
 
+#ifdef CONFIG_SND_SOC_INTEL_SKL_LEGACY_SUPPORT
+	if (skl_fallback_driver) {
+		skl_fallback_driver->shutdown(pci);
+		return;
+	}
+#endif
+
 	if (!bus)
 		return;
 
@@ -1066,6 +1135,14 @@ static void skl_remove(struct pci_dev *pci)
 	struct hdac_bus *bus = pci_get_drvdata(pci);
 	struct skl *skl = bus_to_skl(bus);
 
+#ifdef CONFIG_SND_SOC_INTEL_SKL_LEGACY_SUPPORT
+	if (skl_fallback_driver) {
+		skl_fallback_driver->remove(pci);
+		skl_fallback_driver = NULL;
+		return;
+	}
+#endif
+
 	release_firmware(skl->tplg);
 
 	pm_runtime_get_noresume(&pci->dev);
-- 
2.19.1


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

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Pierre-Louis Bossart]

On 11/16/18 7:49 AM, Takashi Iwai wrote:
> On Wed, 14 Nov 2018 15:53:23 +0100,
> Takashi Iwai wrote:
>> On Mon, 12 Nov 2018 15:00:36 +0100,
>> Pierre-Louis Bossart wrote:
>>> And btw the big topic is still how we provide distributions the means
>>> to handle a 'graceful' fallback from DSP-enabled solutions (SST or
>>> SOF) to legacy HDAudio, it's already popped up for cases where we have
>>> HDaudio solutions with DMICs.
>> Yeah, that's a long-standing problem.  I've experimented some
>> scenarios in the past, and the conclusion is that there is no really
>> working fallback mechanism in general in Linux driver binding.
>> That is, the only reasonable way seems to make a dedicated driver for
>> the specific PCI ID (SKL+) doing the probe-and-fallback by itself,
>> while excluding these IDs from other existing driver entries.
>>
>> I'd love to proceed this but unfortunately I have no machine that can
>> run SKL+ SST driver right now.  I have a new CFL devel box, but it has
>> no support (PCI ID 8086:a348) as well as no firmware...
> So, with a working machine, I could finally hack this a bit.
> Below is a freshly cooked patch(set).  The first one is just to add
> the support for my CFL machine, and another one is to add the fallback
> binding with the legacy HD-audio on snd-soc-skl.
>
> The changes aren't that big.  And you can still control the binding
> via a module option, e.g. snd_soc_skl.legacy=1 will let it bound only
> with the legacy driver, snd_soc_skl.legacy=2 for ASoC only.  The
> default is 0, the fallback to legacy if ASoC binding fails.
>
> It's still a PoC, no proper patch description is put yet.

Thanks Takashi, much appreciate. will give it a try later today.

One comment is that we will have to deal with SOF as well. While the 
end-goal is to converge on a single SOF-based driver, it'll take time to 
support all shipping platforms and deal with firmware authentication 
issues, so the fallback could be SOF->SST->legacy (hopefully on a small 
set of platforms). SST->SOF will likely never happen.

I also wanted to try what happens with your solution if the 
authentication fails with SST (as in your case with the current CNL 
firmware). It's my understanding that the firmware download is deferred 
with a work queue and takes time anyways, so maybe the decision not to 
go back to legacy would be made too early? We'd also need to check with 
a platform where the DSP is not enabled to see if the fallback happens 
immediately (no need to try and download firmware to a non-enabled DSP).

And while I am at it, we may want to think about a single table for PCI 
IDs so that by construction we remove the risk of a platform supported 
with legacy but not the others, and vice versa.

Just my 2 cents before coffee.

>
> Comments / suggestions appreciated.
>
>
> thanks,
>
> Takashi
>
>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Takashi Iwai]

On Fri, 16 Nov 2018 15:11:27 +0100,
Pierre-Louis Bossart wrote:
> 
> On 11/16/18 7:49 AM, Takashi Iwai wrote:
> 
>     On Wed, 14 Nov 2018 15:53:23 +0100,
>     Takashi Iwai wrote:
>     
>         On Mon, 12 Nov 2018 15:00:36 +0100,
>         Pierre-Louis Bossart wrote:
>         
>             And btw the big topic is still how we provide distributions the means
>             to handle a 'graceful' fallback from DSP-enabled solutions (SST or
>             SOF) to legacy HDAudio, it's already popped up for cases where we have
>             HDaudio solutions with DMICs.
>             
>         Yeah, that's a long-standing problem.  I've experimented some
>         scenarios in the past, and the conclusion is that there is no really
>         working fallback mechanism in general in Linux driver binding.
>         That is, the only reasonable way seems to make a dedicated driver for
>         the specific PCI ID (SKL+) doing the probe-and-fallback by itself,
>         while excluding these IDs from other existing driver entries.
>         
>         I'd love to proceed this but unfortunately I have no machine that can
>         run SKL+ SST driver right now.  I have a new CFL devel box, but it has
>         no support (PCI ID 8086:a348) as well as no firmware...
>         
>     So, with a working machine, I could finally hack this a bit.
>     Below is a freshly cooked patch(set).  The first one is just to add
>     the support for my CFL machine, and another one is to add the fallback
>     binding with the legacy HD-audio on snd-soc-skl.
>     
>     The changes aren't that big.  And you can still control the binding
>     via a module option, e.g. snd_soc_skl.legacy=1 will let it bound only
>     with the legacy driver, snd_soc_skl.legacy=2 for ASoC only.  The
>     default is 0, the fallback to legacy if ASoC binding fails.
>     
>     It's still a PoC, no proper patch description is put yet.
>     
> Thanks Takashi, much appreciate. will give it a try later today.
> 
> One comment is that we will have to deal with SOF as well. While the end-goal
> is to converge on a single SOF-based driver, it'll take time to support all
> shipping platforms and deal with firmware authentication issues, so the
> fallback could be SOF->SST->legacy (hopefully on a small set of platforms).
> SST->SOF will likely never happen.

OK, good to know.

> I also wanted to try what happens with your solution if the authentication
> fails with SST (as in your case with the current CNL firmware). It's my
> understanding that the firmware download is deferred with a work queue and
> takes time anyways, so maybe the decision not to go back to legacy would be
> made too early? We'd also need to check with a platform where the DSP is not
> enabled to see if the fallback happens immediately (no need to try and
> download firmware to a non-enabled DSP).

It's a good question, and I guess it's hard to give a fallback in that
level.  IOW, the fallback to the legacy is only for the case where no
DSP is available at all in the hardware level.  If the legacy driver
is required by some reason (faulty hardware detection or the
non-public firmware), we can still enforce to the legacy mode via a
module option.

But for the fallback of SOF to SST is a different question.  I guess
we should check the availability of the firmware at the beginning of
probe?  But request_firmware() inside the probe is basically no-go
(although it might work), so the fallback point would be pushed later
in that case.

Even my simple PoC has a potential problem: as it doesn't reach to the
probe error in the driver core, the previously called devres and other
stuff aren't cleared at calling the fallback driver.  This problem
will remain even if we change the fallback place, so this needs to be
solved (or just ignore if it's only a few memory allocations).

> And while I am at it, we may want to think about a single table for PCI IDs so
> that by construction we remove the risk of a platform supported with legacy
> but not the others, and vice versa.

It's also what I thought, too, but it turned out to be difficult
because both drivers require the own driver_data field.


thanks,

Takashi

Re: [PATCH] ASoC: Intel: hdac_hdmi: add Icelake support

[Pierre-Louis Bossart]

Hi Takashi,

>>      So, with a working machine, I could finally hack this a bit.
>>      Below is a freshly cooked patch(set).  The first one is just to add
>>      the support for my CFL machine, and another one is to add the fallback
>>      binding with the legacy HD-audio on snd-soc-skl.
>>      
>>      The changes aren't that big.  And you can still control the binding
>>      via a module option, e.g. snd_soc_skl.legacy=1 will let it bound only
>>      with the legacy driver, snd_soc_skl.legacy=2 for ASoC only.  The
>>      default is 0, the fallback to legacy if ASoC binding fails.
>>      
>>      It's still a PoC, no proper patch description is put yet.
>>      
>> Thanks Takashi, much appreciate. will give it a try later today.
>>
>> One comment is that we will have to deal with SOF as well. While the end-goal
>> is to converge on a single SOF-based driver, it'll take time to support all
>> shipping platforms and deal with firmware authentication issues, so the
>> fallback could be SOF->SST->legacy (hopefully on a small set of platforms).
>> SST->SOF will likely never happen.
> OK, good to know.
>
>> I also wanted to try what happens with your solution if the authentication
>> fails with SST (as in your case with the current CNL firmware). It's my
>> understanding that the firmware download is deferred with a work queue and
>> takes time anyways, so maybe the decision not to go back to legacy would be
>> made too early? We'd also need to check with a platform where the DSP is not
>> enabled to see if the fallback happens immediately (no need to try and
>> download firmware to a non-enabled DSP).
> It's a good question, and I guess it's hard to give a fallback in that
> level.  IOW, the fallback to the legacy is only for the case where no
> DSP is available at all in the hardware level.  If the legacy driver
> is required by some reason (faulty hardware detection or the
> non-public firmware), we can still enforce to the legacy mode via a
> module option.
>
> But for the fallback of SOF to SST is a different question.  I guess
> we should check the availability of the firmware at the beginning of
> probe?  But request_firmware() inside the probe is basically no-go
> (although it might work), so the fallback point would be pushed later
> in that case.
>
> Even my simple PoC has a potential problem: as it doesn't reach to the
> probe error in the driver core, the previously called devres and other
> stuff aren't cleared at calling the fallback driver.  This problem
> will remain even if we change the fallback place, so this needs to be
> solved (or just ignore if it's only a few memory allocations).

I took a longer look at the suggested code, and it's good enough for me 
- actually smarter than what I had in mind :-).

What we really want is detect if the DSP is present or not, and fall 
back to legacy if not.

The problems with firmware are one step too far, there are cases where 
we have firmware but it's the wrong one due to authentication issues 
that cannot be handled automagically.

Likewise the fallback between SOF and SST is also one bridge too far. If 
the problem is with firmware authentication, then likewise it's 
something that needs to be sorted out by Intel releasing the correct 
firmware and/or users selecting a different firmware which is specific 
to their platforms.

Also the SST driver depends on topology files that have not been shared 
in most cases, so a generic distribution that doesn't have access to all 
the parts would just not work. It's just simpler to select what is known 
to work (e.g. legacy until KBL, SST after), and gradually deprecate the 
SST driver as both features and topologies are provided with SOF.

You have a good point on the allocations, but if we modify the skylake 
driver to only allocate all the resources if the DSP is present, then 
it's only a couple of structures that matter. Today the test

/* check if dsp is there */
     if (bus->ppcap) {

in skl.c is done too late, there is no point in e.g allocating DMA pages 
and stuff if the DSP is not there is the first place...

>
>> And while I am at it, we may want to think about a single table for PCI IDs so
>> that by construction we remove the risk of a platform supported with legacy
>> but not the others, and vice versa.
> It's also what I thought, too, but it turned out to be difficult
> because both drivers require the own driver_data field.

Yes agree. I think we can have a more granular approach though, it's 
simple enough to change the Skylake driver and select SKL/APL/GLK/KBL 
independently (as we do for SOF), so we could do this fallback on an SOC 
basis, e.g something like:

/* Sunrise Point-LP */
     { PCI_DEVICE(0x8086, 0x9d70),
       .driver_data = AZX_DRIVER_SKL | AZX_DCAPS_INTEL_SKYLAKE |
       IS_ENABLED(CONFIG_SND_HDA_INTEL_SKL_SHARED) & 
AZX_DCAPS_INTEL_SHARED },

with SND_HDA_INTEL_SKL_SHARED selected by the SKL driver.

this would scale well with SOF as well.

Thanks for starting this POC, much appreciated.

-Pierre

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel