ACK/Cmnt: [SRU][Bionic][OEM-B][PATCH v2 0/6] HDMI/DP audio can't work on the laptop of Dell Latitude 5495

[Hui Wang]

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 at 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
>