| Message ID | 20210223122205.233701-1-hui.wang@canonical.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ALSA: hda/hdmi: let new platforms assign the pcm slot dynamically | expand |
On Tue, 23 Feb 2021 13:22:05 +0100, Hui Wang wrote: > > If the platform set the dyn_pcm_assign to true, it will call > hdmi_find_pcm_slot() to find a pcm slot when hdmi/dp monitor is > connected and need to create a pcm. > > So far only intel_hsw_common_init() and patch_nvhdmi() set the > dyn_pcm_assign to true, here we let tgl platforms assign the pcm slot > dynamically first, if the driver runs for a period of time and there > is no regression reported, we could set no_fixed_assgin to true in > the intel_hsw_common_init(), and then set it to true in the > patch_nvhdmi(). > > This change comes from the discussion between Takashi and > Kai Vehmanen. Please refer to: > https://github.com/alsa-project/alsa-lib/pull/118 > > Signed-off-by: Hui Wang <hui.wang@canonical.com> As this would "fix" actual use cases, I'd love to merge this for 5.12, but of course it needs to be verified beforehand. So this was actually tested in your side, right? thanks, Takashi > --- > sound/pci/hda/patch_hdmi.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > index e405be7929e3..379a4d786b3b 100644 > --- a/sound/pci/hda/patch_hdmi.c > +++ b/sound/pci/hda/patch_hdmi.c > @@ -157,6 +157,7 @@ struct hdmi_spec { > > bool dyn_pin_out; > bool dyn_pcm_assign; > + bool no_fixed_assign; > bool intel_hsw_fixup; /* apply Intel platform-specific fixups */ > /* > * Non-generic VIA/NVIDIA specific > @@ -1345,6 +1346,12 @@ static int hdmi_find_pcm_slot(struct hdmi_spec *spec, > { > int i; > > + /* on the new machines, try to assign the pcm slot dynamically, > + * not use the preferred fixed map anymore. > + */ > + if (spec->no_fixed_assign) > + goto last_try; > + > /* > * generic_hdmi_build_pcms() may allocate extra PCMs on some > * platforms (with maximum of 'num_nids + dev_num - 1') > @@ -1374,6 +1381,7 @@ static int hdmi_find_pcm_slot(struct hdmi_spec *spec, > return i; > } > > + last_try: > /* the last try; check the empty slots in pins */ > for (i = 0; i < spec->num_nids; i++) { > if (!test_bit(i, &spec->pcm_bitmap)) > @@ -2937,6 +2945,9 @@ static int intel_hsw_common_init(struct hda_codec *codec, hda_nid_t vendor_nid, > spec->port_num = port_num; > spec->intel_hsw_fixup = true; > > + if (port_num > 6) > + spec->no_fixed_assign = true; > + > intel_haswell_enable_all_pins(codec, true); > intel_haswell_fixup_enable_dp12(codec); > > -- > 2.25.1 >
Hi, thanks for the patch! On Tue, 23 Feb 2021, Hui Wang wrote: > If the platform set the dyn_pcm_assign to true, it will call > hdmi_find_pcm_slot() to find a pcm slot when hdmi/dp monitor is > connected and need to create a pcm. [...] > This change comes from the discussion between Takashi and > Kai Vehmanen. Please refer to: > https://github.com/alsa-project/alsa-lib/pull/118 I did propose to merge the alsa-lib change to give us a bit more time to think about how this should be handled in kernel. While this patch certainly solves the problem of kernel picking ALSA PCMs, which current alsa-lib cannot handle, it leaves us a bit halfway. We'd create many PCMs that will never be used. And this change is a bit more involved. > So far only intel_hsw_common_init() and patch_nvhdmi() set the > dyn_pcm_assign to true, here we let tgl platforms assign the pcm slot > dynamically first, if the driver runs for a period of time and there > is no regression reported, we could set no_fixed_assgin to true in > the intel_hsw_common_init(), and then set it to true in the > patch_nvhdmi(). Staged plan sounds good here, although I'd be fairly cautious with this. People using Pulseaudio/Pipewire+UCM won't notice a thing, but I'm sure there are people out there assuming a fixed "physical connector -> ALSA PCM" mapping and not using UCM. Probably at least some way to opt-out would be needed for older platforms. > + if (port_num > 6) > + spec->no_fixed_assign = true; I think this is magic enough of a number to be defined separately along with some documentation. So basicly user-space has a max limit of 8 now and two PCMs are reserved for DP-MST, so that brings us to six, right? This is somewhat arbitrary still. If we simply want to enable the mode for TGL only, easier and cleaned would be to set this flag in patch_i915_tgl_hdmi() directly. Br, Kai
Dne 23. 02. 21 v 15:14 Kai Vehmanen napsal(a): > This is somewhat arbitrary still. If we simply want to enable the mode for > TGL only, easier and cleaned would be to set this flag in > patch_i915_tgl_hdmi() directly. A dumb question: Does TGL really support up to 11 separate displays or it's just to handle 11 connections and the number of maximal simultanenous connected displays is lower? In the later case, the dynamic allocation makes sense a lot. Jaroslav
On Tue, 23 Feb 2021 17:25:23 +0100, Jaroslav Kysela wrote: > > Dne 23. 02. 21 v 15:14 Kai Vehmanen napsal(a): > > > This is somewhat arbitrary still. If we simply want to enable the mode for > > TGL only, easier and cleaned would be to set this flag in > > patch_i915_tgl_hdmi() directly. > > A dumb question: Does TGL really support up to 11 separate displays or it's > just to handle 11 connections and the number of maximal simultanenous > connected displays is lower? In the later case, the dynamic allocation makes > sense a lot. That's the latter. And, the fixed assignment was merely for compatibility with legacy usage, and supposed to be 3 fixed ones or so. Extending to that high number wasn't intended when the mechanism was introduced. We should have noticed it at ICL support (which has up to 6 devices). About the number of devices: from the implementation POV, number 4 makes sense. In the current implementation, with more than 4 HDMI devices, the PCM device number itself is assigned dynamically, hence you can't enumerate fully statically, in anyway. Takashi
Hi, On Tue, 23 Feb 2021, Takashi Iwai wrote: > On Tue, 23 Feb 2021 17:25:23 +0100, Jaroslav Kysela wrote: > > A dumb question: Does TGL really support up to 11 separate displays or it's > > just to handle 11 connections and the number of maximal simultanenous > > connected displays is lower? In the later case, the dynamic allocation makes > > That's the latter. And, the fixed assignment was merely for > compatibility with legacy usage, and supposed to be 3 fixed ones or > so. Extending to that high number wasn't intended when the mechanism > was introduced. We should have noticed it at ICL support (which has > up to 6 devices). yes, exactly. The pins relate to physical ports. There are more pins now to cater for various DP-over-USB-C topologies (versus just native HDMI and DP ports). Most product have less physical ports connected, but on the HDA interface all pins are exposed. Each pin does provide functionality to query whether a display is connected to it, and whether the connected display has audio capability. The maximum number of concurrent displays is described as converters. On TGL this is 4. With SOF, we didn't have legacy userspace, so the HDMI/DP PCMs are already exposed differently and only a small number (3 or 4) of PCMs are created depending on hardware generation. Now the question is how we move snd-hda-intel to similar model with minimal distraction to existing user-space. Br, Kai
On 2/24/21 1:51 AM, Kai Vehmanen wrote: > Hi, > > On Tue, 23 Feb 2021, Takashi Iwai wrote: > >> On Tue, 23 Feb 2021 17:25:23 +0100, Jaroslav Kysela wrote: >>> A dumb question: Does TGL really support up to 11 separate displays or it's >>> just to handle 11 connections and the number of maximal simultanenous >>> connected displays is lower? In the later case, the dynamic allocation makes >> That's the latter. And, the fixed assignment was merely for >> compatibility with legacy usage, and supposed to be 3 fixed ones or >> so. Extending to that high number wasn't intended when the mechanism >> was introduced. We should have noticed it at ICL support (which has >> up to 6 devices). > yes, exactly. The pins relate to physical ports. There are more pins now > to cater for various DP-over-USB-C topologies (versus just native HDMI and > DP ports). Most product have less physical ports connected, but on the HDA > interface all pins are exposed. Each pin does provide functionality to > query whether a display is connected to it, and whether the connected > display has audio capability. > > The maximum number of concurrent displays is described as converters. > On TGL this is 4. If a physical port supports DP-MST, does the 3 connections on this physical port share a single converter? And each connection has an independent pcm, maybe the driver should create pcm pool according to num_converter * 3. > > With SOF, we didn't have legacy userspace, so the HDMI/DP PCMs are already > exposed differently and only a small number (3 or 4) of PCMs are created > depending on hardware generation. Now the question is how we move > snd-hda-intel to similar model with minimal distraction to existing > user-space. Let's do some change on TGL first, let us see if it will break some user-space apps, then we could evaluate how to handle it. Thanks, Hui. > Br, Kai
On 2/23/21 9:09 PM, Takashi Iwai wrote: > On Tue, 23 Feb 2021 13:22:05 +0100, > Hui Wang wrote: >> If the platform set the dyn_pcm_assign to true, it will call >> hdmi_find_pcm_slot() to find a pcm slot when hdmi/dp monitor is >> connected and need to create a pcm. >> >> So far only intel_hsw_common_init() and patch_nvhdmi() set the >> dyn_pcm_assign to true, here we let tgl platforms assign the pcm slot >> dynamically first, if the driver runs for a period of time and there >> is no regression reported, we could set no_fixed_assgin to true in >> the intel_hsw_common_init(), and then set it to true in the >> patch_nvhdmi(). >> >> This change comes from the discussion between Takashi and >> Kai Vehmanen. Please refer to: >> https://github.com/alsa-project/alsa-lib/pull/118 >> >> Signed-off-by: Hui Wang <hui.wang@canonical.com> > As this would "fix" actual use cases, I'd love to merge this for 5.12, > but of course it needs to be verified beforehand. > > So this was actually tested in your side, right? > I tested it on TGL machines with Ubuntu, both sof and legacy hda, so far there is no regression. Regards, Hui. > thanks, > > Takashi >
On 2/23/21 10:14 PM, Kai Vehmanen wrote: > Hi, > > thanks for the patch! > > On Tue, 23 Feb 2021, Hui Wang wrote: > >> If the platform set the dyn_pcm_assign to true, it will call >> hdmi_find_pcm_slot() to find a pcm slot when hdmi/dp monitor is >> connected and need to create a pcm. > [...] >> This change comes from the discussion between Takashi and >> Kai Vehmanen. Please refer to: >> https://github.com/alsa-project/alsa-lib/pull/118 > I did propose to merge the alsa-lib change to give us a bit more time to > think about how this should be handled in kernel. That is not merged yet, and my PM was pushing me to find a solution, then I wrote this patch. > > While this patch certainly solves the problem of kernel picking ALSA PCMs, > which current alsa-lib cannot handle, it leaves us a bit halfway. We'd > create many PCMs that will never be used. And this change is a bit more > involved. > >> So far only intel_hsw_common_init() and patch_nvhdmi() set the >> dyn_pcm_assign to true, here we let tgl platforms assign the pcm slot >> dynamically first, if the driver runs for a period of time and there >> is no regression reported, we could set no_fixed_assgin to true in >> the intel_hsw_common_init(), and then set it to true in the >> patch_nvhdmi(). > Staged plan sounds good here, although I'd be fairly cautious with this. > People using Pulseaudio/Pipewire+UCM won't notice a thing, but I'm sure > there are people out there assuming a fixed "physical connector -> ALSA > PCM" mapping and not using UCM. Probably at least some way to opt-out > would be needed for older platforms. > >> + if (port_num > 6) >> + spec->no_fixed_assign = true; > I think this is magic enough of a number to be defined separately along > with some documentation. So basicly user-space has a max limit of 8 now > and two PCMs are reserved for DP-MST, so that brings us to six, right? Yes, and also before the TGL, the max number of physical pins is 6 (icl). > > This is somewhat arbitrary still. If we simply want to enable the mode for > TGL only, easier and cleaned would be to set this flag in > patch_i915_tgl_hdmi() directly. OK, will consider it. Thanks, Hui. > Br, Kai
Hey, On Wed, 24 Feb 2021, Hui Wang wrote: > On 2/24/21 1:51 AM, Kai Vehmanen wrote: > > interface all pins are exposed. Each pin does provide functionality to > > query whether a display is connected to it, and whether the connected > > display has audio capability. > > > > The maximum number of concurrent displays is described as converters. > > On TGL this is 4. > If a physical port supports DP-MST, does the 3 connections on this physical > port share a single converter? And each connection has an independent pcm, > maybe the driver should create pcm pool according to num_converter * 3. DP-MST is is reported per-pin, so basicly the interface can report display connection status for "numpins*3" endpoints so that would be 9*3 on Intel TGL systems. However, this doesn't affect the converters. There is still four converters, so 4 PCMs are enough to cover all possible combination of plain DP/HDMI and DP-MST. User-space can query the ELD information to learn the mapping from a PCM to a specific display (like e.g. Pulseaudio does). I sent a patch "ALSA: hda/proc - print DP-MST connections" to visualize these a bit better in procfs output. I put example output in the commit: https://lore.kernel.org/r/20201208185736.2877541-1-kai.vehmanen@linux.intel.com The existing driver provides a PCM for each pin, plus reserve two extra PCMs for DP-MST. There's merit to this design as well, but arguably the SOF approach is easier to understand on systems like TGL and ICL. Br, Kai
On 2/24/21 6:17 PM, Kai Vehmanen wrote: > Hey, > > On Wed, 24 Feb 2021, Hui Wang wrote: > >> On 2/24/21 1:51 AM, Kai Vehmanen wrote: >>> interface all pins are exposed. Each pin does provide functionality to >>> query whether a display is connected to it, and whether the connected >>> display has audio capability. >>> >>> The maximum number of concurrent displays is described as converters. >>> On TGL this is 4. >> If a physical port supports DP-MST, does the 3 connections on this physical >> port share a single converter? And each connection has an independent pcm, >> maybe the driver should create pcm pool according to num_converter * 3. > DP-MST is is reported per-pin, so basicly the interface can report display > connection status for "numpins*3" endpoints so that would be 9*3 on > Intel TGL systems. > > However, this doesn't affect the converters. There is still four > converters, so 4 PCMs are enough to cover all possible combination of > plain DP/HDMI and DP-MST. User-space can query the ELD information to > learn the mapping from a PCM to a specific display (like e.g. Pulseaudio > does). > > I sent a patch "ALSA: hda/proc - print DP-MST connections" to visualize > these a bit better in procfs output. I put example output in the commit: > https://lore.kernel.org/r/20201208185736.2877541-1-kai.vehmanen@linux.intel.com Indeed, today I found a thunderbolt dock station which plays DP-MST hub and I connected 2 monitors on it, they are 2 sub-devices belong to a physical pin, and they are assigned different converters. And the procfs output is very clear to show it. Thanks, Hui. > > The existing driver provides a PCM for each pin, plus reserve two extra > PCMs for DP-MST. There's merit to this design as well, but arguably > the SOF approach is easier to understand on systems like TGL and ICL. > > Br, Kai
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index e405be7929e3..379a4d786b3b 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -157,6 +157,7 @@ struct hdmi_spec { bool dyn_pin_out; bool dyn_pcm_assign; + bool no_fixed_assign; bool intel_hsw_fixup; /* apply Intel platform-specific fixups */ /* * Non-generic VIA/NVIDIA specific @@ -1345,6 +1346,12 @@ static int hdmi_find_pcm_slot(struct hdmi_spec *spec, { int i; + /* on the new machines, try to assign the pcm slot dynamically, + * not use the preferred fixed map anymore. + */ + if (spec->no_fixed_assign) + goto last_try; + /* * generic_hdmi_build_pcms() may allocate extra PCMs on some * platforms (with maximum of 'num_nids + dev_num - 1') @@ -1374,6 +1381,7 @@ static int hdmi_find_pcm_slot(struct hdmi_spec *spec, return i; } + last_try: /* the last try; check the empty slots in pins */ for (i = 0; i < spec->num_nids; i++) { if (!test_bit(i, &spec->pcm_bitmap)) @@ -2937,6 +2945,9 @@ static int intel_hsw_common_init(struct hda_codec *codec, hda_nid_t vendor_nid, spec->port_num = port_num; spec->intel_hsw_fixup = true; + if (port_num > 6) + spec->no_fixed_assign = true; + intel_haswell_enable_all_pins(codec, true); intel_haswell_fixup_enable_dp12(codec);
If the platform set the dyn_pcm_assign to true, it will call hdmi_find_pcm_slot() to find a pcm slot when hdmi/dp monitor is connected and need to create a pcm. So far only intel_hsw_common_init() and patch_nvhdmi() set the dyn_pcm_assign to true, here we let tgl platforms assign the pcm slot dynamically first, if the driver runs for a period of time and there is no regression reported, we could set no_fixed_assgin to true in the intel_hsw_common_init(), and then set it to true in the patch_nvhdmi(). This change comes from the discussion between Takashi and Kai Vehmanen. Please refer to: https://github.com/alsa-project/alsa-lib/pull/118 Signed-off-by: Hui Wang <hui.wang@canonical.com> --- sound/pci/hda/patch_hdmi.c | 11 +++++++++++ 1 file changed, 11 insertions(+)