[Intel-gfx] [alsa-devel] [PATCH v2 00/11] drm/i915: LPE audio runtime PM and multipipe (v2)

Pierre-Louis Bossart pierre-louis.bossart at linux.intel.com
Tue May 2 20:15:20 UTC 2017


On 5/2/17 1:27 PM, Ville Syrjälä wrote:
> On Mon, May 01, 2017 at 08:29:10PM -0500, Pierre-Louis Bossart wrote:
>>
>>
>> On 04/28/2017 02:37 PM, Ville Syrjälä wrote:
>>> On Fri, Apr 28, 2017 at 12:10:31PM -0500, Pierre-Louis Bossart wrote:
>>>>
>>>> On 04/28/2017 03:41 AM, Takashi Iwai wrote:
>>>>> On Thu, 27 Apr 2017 18:02:19 +0200,
>>>>> ville.syrjala at linux.intel.com wrote:
>>>>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>>>
>>>>>> Okay, here's the second attempt at getting multiple pipes playing back
>>>>>> audio on the VLV/CHV HDMI LPE audio device. The main change from v1 is
>>>>>> that now the PCM devices are associated with ports instead of pipes,
>>>>>> so the audio from one device always gets output on the same display.
>>>>>>
>>>>>> I've also tacked on the alsa-lib conf update. No clue whether it's
>>>>>> really correct or not (the config language isn't a close friend
>>>>>> of mine).
>>>>>>
>>>>>> BTW I did notice that with LPE audio all the controls say iface=PCM,
>>>>>> whereas on HDA a bunch of them say iface=MIXER. No idea if that's
>>>>>> OK or not, just something I spotted when I was comparing the results
>>>>>> with HDA.
>>>>> We generally accept both iface types for IEC958 stuff, since
>>>>> historically many drivers have already mixed them up.  So it's no
>>>>> problem :)
>>>>>
>>>>>
>>>>>> Entire series available here:
>>>>>> git://github.com/vsyrjala/linux.git lpe_audio_multipipe_2
>>>>>>
>>>>>> Cc: Takashi Iwai <tiwai at suse.de>
>>>>>> Cc: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>
>>>>> All look good, and feel free to take my reviewed-by tag
>>>>>     Reviewed-by: Takashi Iwai <tiwai at suse.de>
>>>>>
>>>>> As said previously, my only slight concern is the compatibility.
>>>>> But, in the current situation with PulseAudio, only few people would
>>>>> use this driver, so it shouldn't be so big impact, I suppose.
>>>>>
>>>>> BTW, which port is used in general on BYT/CHT?
>>>>>
>>>>> Oh, also, I suppose you want to carry these over i915 tree?
>>>>> I don't mind either way, I can take them through sound tree if
>>>>> preferred, too.
>>>> I see frequent oops on startup with this lpe_audio_multipipe_2 branch
>>>> with my CHT device not booting or no HDMI audio device created.
>>>> Not sure if these issues are due to the new patches or to the rest of
>>>> the drm code?
>>>>
>>>> [    5.529023] BUG: unable to handle kernel NULL pointer dereference
>>>> at           (null)
>>>> [    5.529143] IP: hdmi_lpe_audio_probe+0x40f/0x650 [snd_hdmi_lpe_audio]
>>>> [    5.529202] PGD 0
>>>>
>>>> [    5.529242] Oops: 0000 [#1] SMP
>>>> [    5.529274] Modules linked in: snd_soc_sst_atom_hifi2_platform
>>>> snd_soc_sst_match snd_soc_core snd_compress lpc_ich snd_seq
>>>> snd_seq_device shpchp snd_hdmi_lpe_audio(+) snd_pcm snd_timer dw_dmac
>>>> snd soundcore i2c_designware_platform(+) i2c_designware_core
>>>> spi_pxa2xx_platform acpi_pad mac_hid nfsd auth_rpcgss nfs_acl lockd
>>>> grace sunrpc ip_tables x_tables hid_generic mmc_block i2c_hid usbhid hid
>>>> autofs4
>>>> [    5.529605] CPU: 2 PID: 512 Comm: systemd-udevd Not tainted
>>>> 4.11.0-rc8-test+ #11
>>>> [    5.529671] Hardware name: ZOTAC XXXXXX/Cherry Trail FFD, BIOS 5.11
>>>> 09/28/2016
>>>> [    5.529736] task: ffff88007485b780 task.stack: ffffc90000bfc000
>>>> [    5.529793] RIP: 0010:hdmi_lpe_audio_probe+0x40f/0x650
>>>> [snd_hdmi_lpe_audio]
>>>> [    5.529855] RSP: 0018:ffffc90000bffaf0 EFLAGS: 00010246
>>>> [    5.529904] RAX: 0000000000000000 RBX: ffff880079209898 RCX:
>>>> ffff88007920f078
>>>> [    5.529967] RDX: 0000000000000014 RSI: ffffc90000bffb28 RDI:
>>>> 0000000000000002
>>>> [    5.530031] RBP: ffffc90000bffb70 R08: 0000000000000001 R09:
>>>> 0000000000000000
>>>> [    5.530094] R10: ffff88007441bf00 R11: ffffc90000bffb36 R12:
>>>> ffff88007920ef20
>>>> [    5.530159] R13: ffff88007920ef48 R14: 0000000000005688 R15:
>>>> 0000000000000047
>>>> [    5.530225] FS:  00007f627c988640(0000) GS:ffff88007b300000(0000)
>>>> knlGS:0000000000000000
>>>> [    5.530299] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>> [    5.530352] CR2: 0000000000000000 CR3: 0000000078cb8000 CR4:
>>>> 00000000001006e0
>>>> [    5.530416] Call Trace:
>>>> [    5.530452]  platform_drv_probe+0x3b/0xa0
>>>> [    5.530494]  driver_probe_device+0x2bb/0x460
>>>> [    5.530538]  __driver_attach+0xdf/0xf0
>>>> [    5.530576]  ? driver_probe_device+0x460/0x460
>>>> [    5.530620]  bus_for_each_dev+0x60/0xa0
>>>> [    5.530658]  driver_attach+0x1e/0x20
>>>> [    5.530693]  bus_add_driver+0x170/0x270
>>>> [    5.530731]  driver_register+0x60/0xe0
>>>> [    5.530769]  ? 0xffffffffa01cb000
>>>> [    5.530803]  __platform_driver_register+0x36/0x40
>>>> [    5.530851]  hdmi_lpe_audio_driver_init+0x17/0x1000 [snd_hdmi_lpe_audio]
>>>> [    5.530915]  do_one_initcall+0x43/0x180
>>>> [    5.530956]  ? __vunmap+0x81/0xd0
>>>> [    5.530991]  ? kfree+0x14c/0x160
>>>> [    5.531024]  ? kmem_cache_alloc_trace+0x38/0x150
>>>> [    5.531070]  do_init_module+0x5f/0x1f8
>>>> [    5.531108]  load_module+0x271e/0x2bd0
>>>> [    5.531147]  ? kernel_read_file+0x1a3/0x1c0
>>>> [    5.531188]  SYSC_finit_module+0xbc/0xf0
>>>> [    5.531226]  ? SYSC_finit_module+0xbc/0xf0
>>>> [    5.531267]  SyS_finit_module+0xe/0x10
>>>> [    5.531305]  do_syscall_64+0x6e/0x180
>>>> [    5.531345]  entry_SYSCALL64_slow_path+0x25/0x25
>>>> [    5.531389] RIP: 0033:0x7f627b5fbbf9
>>>> [    5.531424] RSP: 002b:00007ffe053eee68 EFLAGS: 00000246 ORIG_RAX:
>>>> 0000000000000139
>>>> [    5.531493] RAX: ffffffffffffffda RBX: 000055d6c745b690 RCX:
>>>> 00007f627b5fbbf9
>>>> [    5.531558] RDX: 0000000000000000 RSI: 00007f627c134995 RDI:
>>>> 0000000000000007
>>>> [    5.531622] RBP: 00007f627c134995 R08: 0000000000000000 R09:
>>>> 00007ffe053eef80
>>>> [    5.531687] R10: 0000000000000007 R11: 0000000000000246 R12:
>>>> 0000000000000000
>>>> [    5.531751] R13: 000055d6c7459ae0 R14: 0000000000020000 R15:
>>>> 000055d6c745b690
>>>> [    5.531816] Code: 48 8b 45 b0 8b 48 18 e8 e0 cb 22 e1 49 8b 44 24 28
>>>> 4a 8d 8c 33 58 01 00 00 48 8d 75 b8 45 31 c9 41 b8 01 00 00 00 ba 14 00
>>>> 00 00 <48> 8b 38 e8 a9 2a ff ff 85 c0 0f 88 09 01 00 00 49 8b 84 24 58
>>> Disassembling that I get:
>>>
>>>    4005a0:       48 8b 45 b0             mov    -0x50(%rbp),%rax
>>>    4005a4:       8b 48 18                mov    0x18(%rax),%ecx
>>>    4005a7:       e8 e0 cb 22 e1          callq  ffffffffe162d18c <_end+0xffffffffe102c14c>
>>>    4005ac:       49 8b 44 24 28          mov    0x28(%r12),%rax
>>>    4005b1:       4a 8d 8c 33 58 01 00    lea    0x158(%rbx,%r14,1),%rcx
>>>    4005b8:       00
>>>    4005b9:       48 8d 75 b8             lea    -0x48(%rbp),%rsi
>>>    4005bd:       45 31 c9                xor    %r9d,%r9d
>>>    4005c0:       41 b8 01 00 00 00       mov    $0x1,%r8d
>>>    4005c6:       ba 14 00 00 00          mov    $0x14,%edx
>>>    4005cb:       48 8b 38                mov    (%rax),%rdi
>>>    4005ce:       e8 a9 2a ff ff          callq  3f307c <_init-0xd32c>
>>>    4005d3:       85 c0                   test   %eax,%eax
>>>    4005d5:       0f 88 09 01 00 00       js     4006e4 <__GNU_EH_FRAME_HDR+0x100>
>>>    4005db:       49                      rex.WB
>>>    4005dc:       8b                      .byte 0x8b
>>>    4005dd:       84 24 58                test   %ah,(%rax,%rbx,2)
>>>
>>> Comparing that with the disassembly from my build, that
>>> first call looks like the snprintf() and the second one would
>>> then be snd_jack_new(). So it seems to blow in the
>>> ctx->card_ctx->card dereference. But I don't really see how
>>> a NULL pointer could sneak in there.
>>
>> There was an standard PORT_B off-by-one error which resulted in memory
>> corruption.
>
> Doh.
>
>> the patch below seems to fix the problem, I can get the same
>> functionality as before with concurrent playback on HDMI and DP, and the
>> jack values are correctly set.
>
> Thanks. I do wonder a bit which side should apply this offset.
> But I guess it doesn't really matter in the end as long as both
> sides know what the deal is. So unless there are good reasons
> for anything else I'll just squash this into my patch(es).

Fine with me.
One open I still have is that it's not self-explanatory for the user to 
figure out which output the devices correspond to, e.g. on my CHT device 
HDMI is device 2 and DP is device 0.
I may be mistaken but in the previous pipe-based solution, audio 
playback would progress at a normal rate even if nothing was connected. 
With the port-based, it looks like playback doesn't progress if there is 
nothing connected - speaker-test is stuck. Not sure if the open or 
hw_params should fail in that case? One could argue that the user should 
know better and check the jack status, but in reality I don't think any 
userspace layer will ever do this.

>
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c
>> b/drivers/gpu/drm/i915/intel_lpe_audio.c
>> index fa728ed..3bf6528 100644
>> --- a/drivers/gpu/drm/i915/intel_lpe_audio.c
>> +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c
>> @@ -332,7 +332,7 @@ void intel_lpe_audio_notify(struct drm_i915_private
>> *dev_priv,
>>                  return;
>>
>>          pdata = dev_get_platdata(&dev_priv->lpe_audio.platdev->dev);
>> -       ppdata = &pdata->port[port];
>> +       ppdata = &pdata->port[port - PORT_B];
>>
>>          spin_lock_irqsave(&pdata->lpe_audio_slock, irqflags);
>>
>> @@ -359,7 +359,7 @@ void intel_lpe_audio_notify(struct drm_i915_private
>> *dev_priv,
>>          }
>>
>>          if (pdata->notify_audio_lpe)
>> - pdata->notify_audio_lpe(dev_priv->lpe_audio.platdev, port);
>> + pdata->notify_audio_lpe(dev_priv->lpe_audio.platdev, port - PORT_B);
>>
>>          spin_unlock_irqrestore(&pdata->lpe_audio_slock, irqflags);
>>   }
>> diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c
>> index 909391d..0153439 100644
>> --- a/sound/x86/intel_hdmi_audio.c
>> +++ b/sound/x86/intel_hdmi_audio.c
>> @@ -1579,7 +1579,7 @@ static irqreturn_t
>> display_pipe_interrupt_handler(int irq, void *dev_id)
>>   static void notify_audio_lpe(struct platform_device *pdev, int port)
>>   {
>>          struct snd_intelhad_card *card_ctx = platform_get_drvdata(pdev);
>> -       struct snd_intelhad *ctx = &card_ctx->pcm_ctx[port];
>> +       struct snd_intelhad *ctx = &card_ctx->pcm_ctx[port]; /* warning:
>> offset with -PORT_B */
>>
>>          schedule_work(&ctx->hdmi_audio_wq);
>>   }
>>
>>
>



More information about the Intel-gfx mailing list