| Message ID | 1532914344-7055-1-git-send-email-hui.wang@canonical.com |
|---|---|
| Headers | show |
| Series | HDMI/DP audio can't work on the laptop of Dell Latitude 5495 | expand |
On 30.07.2018 03:32, Hui Wang wrote: > BugLink: https://bugs.launchpad.net/bugs/1782689 > > In the v2: > the patchset is also sent to OEM-B since the OEM project is waiting for the patchset. > > The v1 introduced a building error, my investigation is shown as below: > The declaration of "enum vga_switcheroo_client_id (*get_client_id)(struct pci_dev *pdev);" > and the implementation of "static int nouveau_dsm_get_client_id(struct pci_dev *pdev)" > exist in the kernel (bionic, artful) for a long time, but "-Werror=incompatible-pointer-types" > didn't report it as an error, after applied these 2 patches, it is reported as an error, > that is because the patch touches the definition of "enum vga_switcheroo_client_id", > before applying these 2 patches, the vga_switcheroo_client_id includes a negative numer: > VGA_SWITCHEROO_UNKNOWN_ID = -1, but after applying the patches, there is no negative number > anymore in the "enum vga_swicheroo_client_id". > I guess if the definition of enum includes negative number, the c compiler treat it as int type, > that is why artful and bionic did not expose building error before. > > To fix the building error, I cherry-picked 4 more patches [3/6-6/6], and the purpose of those > 4 patches is straightforward. > > > > In the v1: > This bug comes from the OEM project, and the engineer Jim Qu from AMD took > almost 2 weeks to investigate this problem, then he worte these two patches to > fix this problem and now the patches are merged to sound repository. > > According to Jim Qu's investigation, the root cause of this problem is: > 1. there is two GPU on the system. iGPU has a audio codec for HDMI output, > and dGPU is without audio codec it is only for offload rendering. > 2. under runtime pm, when dGPU suspend, amdgpu will also call vgaswitchroo > driver to power off audio which vgaswitchroo client is VGA_SWITCHEROO_DIS. > 3. In current HDA audio driver, the iGPU's audio will register to vgaswitchroo > as VGA_SWITCHEROO_DIS, therefore, the audio will be selected and powered off > by runtime pm. > > [Impact] > On the Dell Latitude 5495, If we plug a monitor with audio capability to the > HDMI/DP ports, we can't find HDMI/DP audio sinks as expected, then we can't > play any sound through HDMI/DP audio. > > [Fix] > With these two patches, the driver will not always set vgaswicheroo clients of > HDA audio as VGA_SWITCHEROO_DIS, it will set it to _DIS or _IGD with the help > of callback function of DRM drivers. So on this machine, the vgaswicheroo client > will be set _IGD, then it will not be powered off when discrete gpu is powered off. > > [Test Case] > We tested plug/unplug detection and playback through HDMI/DP audio, everything works > well. > > [Regression Potential] > Very low, without these two patches, the vgaswitchroo client of audio will be > set to _DIS unconditionally, it did not expose any problem because in the past, > all the HDMI/DP audio codecs are in the discreate GPU. But on Latitude 5495, the > HDMI/DP audio codec is in the integrated GPU, so we need to change the driver to > let DRM driver decide if it is _DIS or _IGD, it will not introduce regression for > old mahcines on old machines, the client will be set to _DIS as before. > > And we have tested these two patches on some old machines with two gpus like A+A > , I+A and I+N, all of them worked well as before. > > Jim Qu (2): > ALSA: hda: use PCI_BASE_CLASS_DISPLAY to replace PCI_CLASS_DISPLAY_VGA > vga_switcheroo: set audio client id according to bound GPU id > > Luc Van Oostenryck (4): > drm/nouveau: fix nouveau_dsm_get_client_id()'s return type > drm/radeon: fix radeon_atpx_get_client_id()'s return type > drm/amdgpu: fix amdgpu_atpx_get_client_id()'s return type > platform/x86: apple-gmux: fix gmux_get_client_id()'s return type > > drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c | 2 +- > drivers/gpu/drm/nouveau/nouveau_acpi.c | 2 +- > drivers/gpu/drm/radeon/radeon_atpx_handler.c | 2 +- > drivers/gpu/vga/vga_switcheroo.c | 62 +++++++++++++++++++----- > drivers/platform/x86/apple-gmux.c | 2 +- > include/linux/vga_switcheroo.h | 8 +-- > sound/pci/hda/hda_intel.c | 13 ++--- > 7 files changed, 66 insertions(+), 25 deletions(-) > Acked-by: Stefan Bader <stefan.bader@canonical.com> I think the additional patches need to be applied before at least current patch #2 (maybe just before both) to avoid compile being broken in between. Also, you should update the SRU justification in the bug report to match the new number of patches. -Stefan
On 2018年07月30日 18:39, Stefan Bader wrote: > On 30.07.2018 03:32, Hui Wang wrote: >> BugLink: https://bugs.launchpad.net/bugs/1782689 >> >> In the v2: >> the patchset is also sent to OEM-B since the OEM project is waiting for the patchset. >> >> The v1 introduced a building error, my investigation is shown as below: >> The declaration of "enum vga_switcheroo_client_id (*get_client_id)(struct pci_dev *pdev);" >> and the implementation of "static int nouveau_dsm_get_client_id(struct pci_dev *pdev)" >> exist in the kernel (bionic, artful) for a long time, but "-Werror=incompatible-pointer-types" >> didn't report it as an error, after applied these 2 patches, it is reported as an error, >> that is because the patch touches the definition of "enum vga_switcheroo_client_id", >> before applying these 2 patches, the vga_switcheroo_client_id includes a negative numer: >> VGA_SWITCHEROO_UNKNOWN_ID = -1, but after applying the patches, there is no negative number >> anymore in the "enum vga_swicheroo_client_id". >> I guess if the definition of enum includes negative number, the c compiler treat it as int type, >> that is why artful and bionic did not expose building error before. >> >> To fix the building error, I cherry-picked 4 more patches [3/6-6/6], and the purpose of those >> 4 patches is straightforward. >> >> >> >> In the v1: >> This bug comes from the OEM project, and the engineer Jim Qu from AMD took >> almost 2 weeks to investigate this problem, then he worte these two patches to >> fix this problem and now the patches are merged to sound repository. >> >> According to Jim Qu's investigation, the root cause of this problem is: >> 1. there is two GPU on the system. iGPU has a audio codec for HDMI output, >> and dGPU is without audio codec it is only for offload rendering. >> 2. under runtime pm, when dGPU suspend, amdgpu will also call vgaswitchroo >> driver to power off audio which vgaswitchroo client is VGA_SWITCHEROO_DIS. >> 3. In current HDA audio driver, the iGPU's audio will register to vgaswitchroo >> as VGA_SWITCHEROO_DIS, therefore, the audio will be selected and powered off >> by runtime pm. >> >> [Impact] >> On the Dell Latitude 5495, If we plug a monitor with audio capability to the >> HDMI/DP ports, we can't find HDMI/DP audio sinks as expected, then we can't >> play any sound through HDMI/DP audio. >> >> [Fix] >> With these two patches, the driver will not always set vgaswicheroo clients of >> HDA audio as VGA_SWITCHEROO_DIS, it will set it to _DIS or _IGD with the help >> of callback function of DRM drivers. So on this machine, the vgaswicheroo client >> will be set _IGD, then it will not be powered off when discrete gpu is powered off. >> >> [Test Case] >> We tested plug/unplug detection and playback through HDMI/DP audio, everything works >> well. >> >> [Regression Potential] >> Very low, without these two patches, the vgaswitchroo client of audio will be >> set to _DIS unconditionally, it did not expose any problem because in the past, >> all the HDMI/DP audio codecs are in the discreate GPU. But on Latitude 5495, the >> HDMI/DP audio codec is in the integrated GPU, so we need to change the driver to >> let DRM driver decide if it is _DIS or _IGD, it will not introduce regression for >> old mahcines on old machines, the client will be set to _DIS as before. >> >> And we have tested these two patches on some old machines with two gpus like A+A >> , I+A and I+N, all of them worked well as before. >> >> Jim Qu (2): >> ALSA: hda: use PCI_BASE_CLASS_DISPLAY to replace PCI_CLASS_DISPLAY_VGA >> vga_switcheroo: set audio client id according to bound GPU id >> >> Luc Van Oostenryck (4): >> drm/nouveau: fix nouveau_dsm_get_client_id()'s return type >> drm/radeon: fix radeon_atpx_get_client_id()'s return type >> drm/amdgpu: fix amdgpu_atpx_get_client_id()'s return type >> platform/x86: apple-gmux: fix gmux_get_client_id()'s return type >> >> drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c | 2 +- >> drivers/gpu/drm/nouveau/nouveau_acpi.c | 2 +- >> drivers/gpu/drm/radeon/radeon_atpx_handler.c | 2 +- >> drivers/gpu/vga/vga_switcheroo.c | 62 +++++++++++++++++++----- >> drivers/platform/x86/apple-gmux.c | 2 +- >> include/linux/vga_switcheroo.h | 8 +-- >> sound/pci/hda/hda_intel.c | 13 ++--- >> 7 files changed, 66 insertions(+), 25 deletions(-) >> > Acked-by: Stefan Bader <stefan.bader@canonical.com> > > I think the additional patches need to be applied before at least current patch > #2 (maybe just before both) to avoid compile being broken in between. Yes, please, move the last 4 patches ahead of first 2 ones. > Also, you should update the SRU justification in the bug report to match the new > number of patches. Got it, and fixed it in the bug description of #1782689 Thanks. > -Stefan >
On 07/30/18 03:32, Hui Wang wrote: > BugLink: https://bugs.launchpad.net/bugs/1782689 > > In the v2: > the patchset is also sent to OEM-B since the OEM project is waiting for the patchset. > > The v1 introduced a building error, my investigation is shown as below: > The declaration of "enum vga_switcheroo_client_id (*get_client_id)(struct pci_dev *pdev);" > and the implementation of "static int nouveau_dsm_get_client_id(struct pci_dev *pdev)" > exist in the kernel (bionic, artful) for a long time, but "-Werror=incompatible-pointer-types" > didn't report it as an error, after applied these 2 patches, it is reported as an error, > that is because the patch touches the definition of "enum vga_switcheroo_client_id", > before applying these 2 patches, the vga_switcheroo_client_id includes a negative numer: > VGA_SWITCHEROO_UNKNOWN_ID = -1, but after applying the patches, there is no negative number > anymore in the "enum vga_swicheroo_client_id". > I guess if the definition of enum includes negative number, the c compiler treat it as int type, > that is why artful and bionic did not expose building error before. > > To fix the building error, I cherry-picked 4 more patches [3/6-6/6], and the purpose of those > 4 patches is straightforward. > > > > In the v1: > This bug comes from the OEM project, and the engineer Jim Qu from AMD took > almost 2 weeks to investigate this problem, then he worte these two patches to > fix this problem and now the patches are merged to sound repository. > > According to Jim Qu's investigation, the root cause of this problem is: > 1. there is two GPU on the system. iGPU has a audio codec for HDMI output, > and dGPU is without audio codec it is only for offload rendering. > 2. under runtime pm, when dGPU suspend, amdgpu will also call vgaswitchroo > driver to power off audio which vgaswitchroo client is VGA_SWITCHEROO_DIS. > 3. In current HDA audio driver, the iGPU's audio will register to vgaswitchroo > as VGA_SWITCHEROO_DIS, therefore, the audio will be selected and powered off > by runtime pm. > > [Impact] > On the Dell Latitude 5495, If we plug a monitor with audio capability to the > HDMI/DP ports, we can't find HDMI/DP audio sinks as expected, then we can't > play any sound through HDMI/DP audio. > > [Fix] > With these two patches, the driver will not always set vgaswicheroo clients of > HDA audio as VGA_SWITCHEROO_DIS, it will set it to _DIS or _IGD with the help > of callback function of DRM drivers. So on this machine, the vgaswicheroo client > will be set _IGD, then it will not be powered off when discrete gpu is powered off. > > [Test Case] > We tested plug/unplug detection and playback through HDMI/DP audio, everything works > well. > > [Regression Potential] > Very low, without these two patches, the vgaswitchroo client of audio will be > set to _DIS unconditionally, it did not expose any problem because in the past, > all the HDMI/DP audio codecs are in the discreate GPU. But on Latitude 5495, the > HDMI/DP audio codec is in the integrated GPU, so we need to change the driver to > let DRM driver decide if it is _DIS or _IGD, it will not introduce regression for > old mahcines on old machines, the client will be set to _DIS as before. > > And we have tested these two patches on some old machines with two gpus like A+A > , I+A and I+N, all of them worked well as before. > > Jim Qu (2): > ALSA: hda: use PCI_BASE_CLASS_DISPLAY to replace PCI_CLASS_DISPLAY_VGA > vga_switcheroo: set audio client id according to bound GPU id > > Luc Van Oostenryck (4): > drm/nouveau: fix nouveau_dsm_get_client_id()'s return type > drm/radeon: fix radeon_atpx_get_client_id()'s return type > drm/amdgpu: fix amdgpu_atpx_get_client_id()'s return type > platform/x86: apple-gmux: fix gmux_get_client_id()'s return type > > drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c | 2 +- > drivers/gpu/drm/nouveau/nouveau_acpi.c | 2 +- > drivers/gpu/drm/radeon/radeon_atpx_handler.c | 2 +- > drivers/gpu/vga/vga_switcheroo.c | 62 +++++++++++++++++++----- > drivers/platform/x86/apple-gmux.c | 2 +- > include/linux/vga_switcheroo.h | 8 +-- > sound/pci/hda/hda_intel.c | 13 ++--- > 7 files changed, 66 insertions(+), 25 deletions(-) > Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>
On 07/30/18 03:32, Hui Wang wrote: > BugLink: https://bugs.launchpad.net/bugs/1782689 > > In the v2: > the patchset is also sent to OEM-B since the OEM project is waiting for the patchset. > > The v1 introduced a building error, my investigation is shown as below: > The declaration of "enum vga_switcheroo_client_id (*get_client_id)(struct pci_dev *pdev);" > and the implementation of "static int nouveau_dsm_get_client_id(struct pci_dev *pdev)" > exist in the kernel (bionic, artful) for a long time, but "-Werror=incompatible-pointer-types" > didn't report it as an error, after applied these 2 patches, it is reported as an error, > that is because the patch touches the definition of "enum vga_switcheroo_client_id", > before applying these 2 patches, the vga_switcheroo_client_id includes a negative numer: > VGA_SWITCHEROO_UNKNOWN_ID = -1, but after applying the patches, there is no negative number > anymore in the "enum vga_swicheroo_client_id". > I guess if the definition of enum includes negative number, the c compiler treat it as int type, > that is why artful and bionic did not expose building error before. > > To fix the building error, I cherry-picked 4 more patches [3/6-6/6], and the purpose of those > 4 patches is straightforward. > > > > In the v1: > This bug comes from the OEM project, and the engineer Jim Qu from AMD took > almost 2 weeks to investigate this problem, then he worte these two patches to > fix this problem and now the patches are merged to sound repository. > > According to Jim Qu's investigation, the root cause of this problem is: > 1. there is two GPU on the system. iGPU has a audio codec for HDMI output, > and dGPU is without audio codec it is only for offload rendering. > 2. under runtime pm, when dGPU suspend, amdgpu will also call vgaswitchroo > driver to power off audio which vgaswitchroo client is VGA_SWITCHEROO_DIS. > 3. In current HDA audio driver, the iGPU's audio will register to vgaswitchroo > as VGA_SWITCHEROO_DIS, therefore, the audio will be selected and powered off > by runtime pm. > > [Impact] > On the Dell Latitude 5495, If we plug a monitor with audio capability to the > HDMI/DP ports, we can't find HDMI/DP audio sinks as expected, then we can't > play any sound through HDMI/DP audio. > > [Fix] > With these two patches, the driver will not always set vgaswicheroo clients of > HDA audio as VGA_SWITCHEROO_DIS, it will set it to _DIS or _IGD with the help > of callback function of DRM drivers. So on this machine, the vgaswicheroo client > will be set _IGD, then it will not be powered off when discrete gpu is powered off. > > [Test Case] > We tested plug/unplug detection and playback through HDMI/DP audio, everything works > well. > > [Regression Potential] > Very low, without these two patches, the vgaswitchroo client of audio will be > set to _DIS unconditionally, it did not expose any problem because in the past, > all the HDMI/DP audio codecs are in the discreate GPU. But on Latitude 5495, the > HDMI/DP audio codec is in the integrated GPU, so we need to change the driver to > let DRM driver decide if it is _DIS or _IGD, it will not introduce regression for > old mahcines on old machines, the client will be set to _DIS as before. > > And we have tested these two patches on some old machines with two gpus like A+A > , I+A and I+N, all of them worked well as before. > > Jim Qu (2): > ALSA: hda: use PCI_BASE_CLASS_DISPLAY to replace PCI_CLASS_DISPLAY_VGA > vga_switcheroo: set audio client id according to bound GPU id > > Luc Van Oostenryck (4): > drm/nouveau: fix nouveau_dsm_get_client_id()'s return type > drm/radeon: fix radeon_atpx_get_client_id()'s return type > drm/amdgpu: fix amdgpu_atpx_get_client_id()'s return type > platform/x86: apple-gmux: fix gmux_get_client_id()'s return type > > drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c | 2 +- > drivers/gpu/drm/nouveau/nouveau_acpi.c | 2 +- > drivers/gpu/drm/radeon/radeon_atpx_handler.c | 2 +- > drivers/gpu/vga/vga_switcheroo.c | 62 +++++++++++++++++++----- > drivers/platform/x86/apple-gmux.c | 2 +- > include/linux/vga_switcheroo.h | 8 +-- > sound/pci/hda/hda_intel.c | 13 ++--- > 7 files changed, 66 insertions(+), 25 deletions(-) > Applied to bionic/master-next branch, in the following order: 3/6 - drm/nouveau: fix nouveau_dsm_get_client_id()'s return type 4/6 - drm/radeon: fix radeon_atpx_get_client_id()'s return type 5/6 - drm/amdgpu: fix amdgpu_atpx_get_client_id()'s return type 6/6 - platform/x86: apple-gmux: fix gmux_get_client_id()'s return type 1/6 - ALSA: hda: use PCI_BASE_CLASS_DISPLAY to replace PCI_CLASS_DISPLAY_VGA 2/6 - (HEAD -> master-next) vga_switcheroo: set audio client id according to bound GPU id Thanks, Kleber
On Mon, Jul 30, 2018 at 09:32:18AM +0800, Hui Wang wrote: > BugLink: https://bugs.launchpad.net/bugs/1782689 > > In the v2: > the patchset is also sent to OEM-B since the OEM project is waiting for the patchset. > > The v1 introduced a building error, my investigation is shown as below: > The declaration of "enum vga_switcheroo_client_id (*get_client_id)(struct pci_dev *pdev);" > and the implementation of "static int nouveau_dsm_get_client_id(struct pci_dev *pdev)" > exist in the kernel (bionic, artful) for a long time, but "-Werror=incompatible-pointer-types" > didn't report it as an error, after applied these 2 patches, it is reported as an error, > that is because the patch touches the definition of "enum vga_switcheroo_client_id", > before applying these 2 patches, the vga_switcheroo_client_id includes a negative numer: > VGA_SWITCHEROO_UNKNOWN_ID = -1, but after applying the patches, there is no negative number > anymore in the "enum vga_swicheroo_client_id". > I guess if the definition of enum includes negative number, the c compiler treat it as int type, > that is why artful and bionic did not expose building error before. > > To fix the building error, I cherry-picked 4 more patches [3/6-6/6], and the purpose of those > 4 patches is straightforward. > > > > In the v1: > This bug comes from the OEM project, and the engineer Jim Qu from AMD took > almost 2 weeks to investigate this problem, then he worte these two patches to > fix this problem and now the patches are merged to sound repository. > > According to Jim Qu's investigation, the root cause of this problem is: > 1. there is two GPU on the system. iGPU has a audio codec for HDMI output, > and dGPU is without audio codec it is only for offload rendering. > 2. under runtime pm, when dGPU suspend, amdgpu will also call vgaswitchroo > driver to power off audio which vgaswitchroo client is VGA_SWITCHEROO_DIS. > 3. In current HDA audio driver, the iGPU's audio will register to vgaswitchroo > as VGA_SWITCHEROO_DIS, therefore, the audio will be selected and powered off > by runtime pm. > > [Impact] > On the Dell Latitude 5495, If we plug a monitor with audio capability to the > HDMI/DP ports, we can't find HDMI/DP audio sinks as expected, then we can't > play any sound through HDMI/DP audio. > > [Fix] > With these two patches, the driver will not always set vgaswicheroo clients of > HDA audio as VGA_SWITCHEROO_DIS, it will set it to _DIS or _IGD with the help > of callback function of DRM drivers. So on this machine, the vgaswicheroo client > will be set _IGD, then it will not be powered off when discrete gpu is powered off. > > [Test Case] > We tested plug/unplug detection and playback through HDMI/DP audio, everything works > well. > > [Regression Potential] > Very low, without these two patches, the vgaswitchroo client of audio will be > set to _DIS unconditionally, it did not expose any problem because in the past, > all the HDMI/DP audio codecs are in the discreate GPU. But on Latitude 5495, the > HDMI/DP audio codec is in the integrated GPU, so we need to change the driver to > let DRM driver decide if it is _DIS or _IGD, it will not introduce regression for > old mahcines on old machines, the client will be set to _DIS as before. > > And we have tested these two patches on some old machines with two gpus like A+A > , I+A and I+N, all of them worked well as before. > > Jim Qu (2): > ALSA: hda: use PCI_BASE_CLASS_DISPLAY to replace PCI_CLASS_DISPLAY_VGA > vga_switcheroo: set audio client id according to bound GPU id > > Luc Van Oostenryck (4): > drm/nouveau: fix nouveau_dsm_get_client_id()'s return type > drm/radeon: fix radeon_atpx_get_client_id()'s return type > drm/amdgpu: fix amdgpu_atpx_get_client_id()'s return type > platform/x86: apple-gmux: fix gmux_get_client_id()'s return type > > drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c | 2 +- > drivers/gpu/drm/nouveau/nouveau_acpi.c | 2 +- > drivers/gpu/drm/radeon/radeon_atpx_handler.c | 2 +- > drivers/gpu/vga/vga_switcheroo.c | 62 +++++++++++++++++++----- > drivers/platform/x86/apple-gmux.c | 2 +- > include/linux/vga_switcheroo.h | 8 +-- > sound/pci/hda/hda_intel.c | 13 ++--- > 7 files changed, 66 insertions(+), 25 deletions(-) Applied to cosmic/master-next, with the backport for "vga_switcheroo: set audio client id according to bound GPU id" replaced with a cherry pick from the maintainer tree. Also applied that patch and "ALSA: hda: use PCI_BASE_CLASS_DISPLAY to replace PCI_CLASS_DISPLAY_VGA" to unstable/master, as the other patches are already in 4.18. Thanks!