| Message ID | 20181110211846.23667-1-yung-chuan.liao@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 019033c854a20e10f691f6cc0e897df8817d9521 |
| Headers | show |
| Series | ASoC: Intel: hdac_hdmi: add Icelake support | expand |
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
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
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 >
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
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), > {} > }; >
>>> 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
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
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, } };
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
>> 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).
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
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
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
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
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
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
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), {} };