Hdmi hotplug detection by libinyang · Pull Request #935 · thesofproject/linux

libinyang · 2019-05-13T05:34:52Z

This PR try to implement the feature of #912
The root cause is to export an kcontrol to userspace.
I will make patch for other platforms later.

The original code will not export the HDMI jack kcontrols to userspace, which will cause UCM can't support the jack detection.

With these 2 patch, HDMI jack kcontrols will be exported to userspace, so that UCM can use the kcontrols for Jack detection.

libinyang · 2019-05-13T06:02:05Z

Sorry, I submitted from a wrong branch, this makes too many unrelated reviewers. Please ignore my review requirement if you are not the owner.

kv2019i · 2019-05-13T10:25:08Z

 EXPORT_SYMBOL_GPL(snd_soc_card_jack_new);

+/**
+ * snd_soc_card_jack_new_ctl - Create a new jack with a kcontrol


Hmm, this took a while to understand the different variants. Maybe name this snd_soc_card_jack_new_kctl() (like in jack.h snd_jack_add_new_kctl()) ?

Right. I will change to xxx_kctl.

Yes please, without knowing what problem it fixes or what feature it improves it is hard to understand why we need this change

kv2019i

The commit description should explain why you are doing this change. It's not immediately obvious from this.

libinyang · 2019-05-13T14:34:02Z

The commit description should explain why you are doing this change. It's not immediately obvious from this.

OK. I will update the commit description.

plbossart

It may be really clear in your mind but we are missing
a) a problem statement. What is the issue
b) what the fix should be?
c) what are the effects before and after this PR?
d) why is the solution the best, considering possible alternatives.

It's not productive for reviewers to speculate on your intent, so please clarify what this PR is about.

libinyang · 2019-05-14T03:00:21Z

patches have been updated.

use xxx_kctl instead of xxx_ctl
update the second patch commit description
update the PR description to describe the issue which the PR is fixing.

ranj063 · 2019-05-14T05:02:17Z

@libinyang whats the benefit of adding another function when you could call snd_jack_add_new_kctl() after the new jack is created with snd_soc_card_jack_new_kctl() in skl_dsp_hda_common?

libinyang · 2019-05-14T05:42:30Z

@libinyang whats the benefit of adding another function when you could call snd_jack_add_new_kctl() after the new jack is created with snd_soc_card_jack_new_kctl() in skl_dsp_hda_common?

@ranj063 Do you mean to call "snd_soc_card_jack_new()" and "snd_jack_add_new_kctl()"instead of "snd_soc_card_jack_new_kctl()"? I think they are similar. I'm OK with both. There are some machine drivers that will create jack kctls. Maybe it is simple to call snd_soc_card_jack_new_kctl(). I'm open to both methods.

ranj063 · 2019-05-14T05:48:00Z

@ranj063 Do you mean to call "snd_soc_card_jack_new()" and "snd_jack_add_new_kctl()"instead of "snd_soc_card_jack_new_kctl()"?

Yes, exactly.

I think they are similar. I'm OK with both. There are some machine drivers that will create jack kctls. Maybe it is simple to call snd_soc_card_jack_new_kctl(). I'm open to both methods.

And thats why we have snd_jack_add_new_kctl(). so I dont see the benefit of adding another function. I think it makes sense to just call snd_jack_add_new_kctl() after the jack is created in the hda machine driver. Other drivers if/when they need it can do the same.

libinyang · 2019-05-14T05:50:59Z

@ranj063 OK to me. I will update the patch. However, I may update the patch later as my platform is occupied by QA today.

plbossart · 2019-05-14T13:04:53Z

 		snprintf(jack_name, sizeof(jack_name),
 			 "HDMI/DP, pcm=%d Jack", pcm->device);
-		err = snd_soc_card_jack_new(card, jack_name,
+		err = snd_soc_card_jack_new_kctl(card, jack_name,


fix alignment

plbossart · 2019-05-14T13:10:26Z

@libinyang I still don't get what your patch does. UCM has been able to detect jack plug/unplug for many years without your patch, see e.g. all existing Chromebook machine drivers. I will accept that there are differences e.g. the use of gpios/pins, but I don't see any kcontrol anywhere. There is something unclear about the root cause really that you need to explain in more details if you want to convince reviewers about adding an new API which only sets a flag to one.

libinyang · 2019-05-15T06:21:49Z

patch updated

libinyang · 2019-05-15T06:24:01Z

@plbossart This is because they are using the legacy method. The old method of pulse-audio doesn't need jack kctl.

plbossart

can you add in your commit message more details on why PulseAudio needs this? when you state that 'userspace can use kcontrols' it's not the same thing as "PulseAudio needs kcontrols", the former wording means its optional and possibly useless while the latter means it's mandatory.

See also comments below.

plbossart · 2019-05-15T11:43:22Z

+					    jack_name, SND_JACK_AVOUT);
+		if (err)
+			dev_warn(component->dev, "failed creating Jack kctl\n");
+


can you clarify for me how this is released? there is no matching 'release' for the 'new'.

Also should we add those controls once the hdac_hdmi_jack_init() succceds below, otherwise you may expose controls that are not functional.

can you clarify for me how this is released? there is no matching 'release' for the 'new'.

Also should we add those controls once the hdac_hdmi_jack_init() succceds below, otherwise you may expose controls that are not functional.

@plbossart ALSA jack has a mechanism that it will release them automatically. Jack is seemed as a snd_device. So when driver is removed, the snd_deivce's dev_free() will be called by alsa core. In Jack's dev_free(), it will free all the resource. So you will see there is no free kctl code in our audio driver. You can refer jack.c: snd_jack_dev_free()

Also should we add those controls once the hdac_hdmi_jack_init() succceds below, otherwise you may expose controls that are not functional.

the hdac_hdmi_jack_init() function has no relationship with the HDMI Jack kctl. They are totally doing the different things. snd_jack_add_new_kctl() only depends on the "snd_soc_card_jack_new()".

@libinyang what do you mean that they are doing different things, they use the same argument and both have a jack in the function name. Can you please elaborate. This may be very clear to you but if you are not sharing the information we don't progress as a team. Thanks

@plbossart hdac_hdmi_jack_init() main task is creating hdac_hdmi_pcm instance and calling snd_hdac_add_chmap_ctls(). It is creating a normal channel map ctls.

A jack kctl is a instance of struct snd_jack_kctl.

so you are saying the function name hdac_hdmi_jack_init is misleading, should we rename it then?

Yes, I think the name is not good. However, it does do a little thing related jack init: pcm->jack = jack to assign the jack to hdac_hdmi_pcm.

@libinyang your circular explanations leave me scratching my head. The function has nothing to do with jack, except that it "assigns the jack to hdac_hdmi_pcm".
I am going to approve and move on, can't keep doing this.

This patch call snd_jack_add_new_kctl() to create the HDMI jack kctls. Userspace needs these kctls to detect the hdmi monitor hotplug. In /usr/share/alsa/ucm, the config file needs to assign a jack kctl to "JackControl" to let PA get the jack hotplug status. Signed-off-by: Libin Yang <libin.yang@intel.com>

libinyang · 2019-05-16T02:31:53Z

patch updated.

plbossart

need another reviewer

The fstest btrfs/154 reports [ 8675.381709] BTRFS: Transaction aborted (error -28) [ 8675.383302] WARNING: CPU: 1 PID: 31900 at fs/btrfs/block-group.c:2038 btrfs_create_pending_block_groups+0x1e0/0x1f0 [btrfs] [ 8675.390925] CPU: 1 PID: 31900 Comm: btrfs Not tainted 5.5.0-rc6-default+ #935 [ 8675.392780] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.0-59-gc9ba527-rebuilt.opensuse.org 04/01/2014 [ 8675.395452] RIP: 0010:btrfs_create_pending_block_groups+0x1e0/0x1f0 [btrfs] [ 8675.402672] RSP: 0018:ffffb2090888fb00 EFLAGS: 00010286 [ 8675.404413] RAX: 0000000000000000 RBX: ffff92026dfa91c8 RCX: 0000000000000001 [ 8675.406609] RDX: 0000000000000000 RSI: ffffffff8e100899 RDI: ffffffff8e100971 [ 8675.408775] RBP: ffff920247c61660 R08: 0000000000000000 R09: 0000000000000000 [ 8675.410978] R10: 0000000000000000 R11: 0000000000000000 R12: 00000000ffffffe4 [ 8675.412647] R13: ffff92026db74000 R14: ffff920247c616b8 R15: ffff92026dfbc000 [ 8675.413994] FS: 00007fd5e57248c0(0000) GS:ffff92027d800000(0000) knlGS:0000000000000000 [ 8675.416146] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 8675.417833] CR2: 0000564aa51682d8 CR3: 000000006dcbc004 CR4: 0000000000160ee0 [ 8675.419801] Call Trace: [ 8675.420742] btrfs_start_dirty_block_groups+0x355/0x480 [btrfs] [ 8675.422600] btrfs_commit_transaction+0xc8/0xaf0 [btrfs] [ 8675.424335] reset_balance_state+0x14a/0x190 [btrfs] [ 8675.425824] btrfs_balance.cold+0xe7/0x154 [btrfs] [ 8675.427313] ? kmem_cache_alloc_trace+0x235/0x2c0 [ 8675.428663] btrfs_ioctl_balance+0x298/0x350 [btrfs] [ 8675.430285] btrfs_ioctl+0x466/0x2550 [btrfs] [ 8675.431788] ? mem_cgroup_charge_statistics+0x51/0xf0 [ 8675.433487] ? mem_cgroup_commit_charge+0x56/0x400 [ 8675.435122] ? do_raw_spin_unlock+0x4b/0xc0 [ 8675.436618] ? _raw_spin_unlock+0x1f/0x30 [ 8675.438093] ? __handle_mm_fault+0x499/0x740 [ 8675.439619] ? do_vfs_ioctl+0x56e/0x770 [ 8675.441034] do_vfs_ioctl+0x56e/0x770 [ 8675.442411] ksys_ioctl+0x3a/0x70 [ 8675.443718] ? trace_hardirqs_off_thunk+0x1a/0x1c [ 8675.445333] __x64_sys_ioctl+0x16/0x20 [ 8675.446705] do_syscall_64+0x50/0x210 [ 8675.448059] entry_SYSCALL_64_after_hwframe+0x49/0xbe [ 8675.479187] BTRFS: error (device vdb) in btrfs_create_pending_block_groups:2038: errno=-28 No space left We now use btrfs_can_overcommit() to see if we can flip a block group read only. Before this would fail because we weren't taking into account the usable un-allocated space for allocating chunks. With my patches we were allowed to do the balance, which is technically correct. The test is trying to start balance on degraded mount. So now we're trying to allocate a chunk and cannot because we want to allocate a RAID1 chunk, but there's only 1 device that's available for usage. This results in an ENOSPC. But we shouldn't even be making it this far, we don't have enough devices to restripe. The problem is we're using btrfs_num_devices(), that also includes missing devices. That's not actually what we want, we need to use rw_devices. The chunk_mutex is not needed here, rw_devices changes only in device add, remove or replace, all are excluded by EXCL_OP mechanism. Fixes: e4d8ec0 ("Btrfs: implement online profile changing") CC: stable@vger.kernel.org # 4.4+ Signed-off-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: David Sterba <dsterba@suse.com> [ add stacktrace, update changelog, drop chunk_mutex ] Signed-off-by: David Sterba <dsterba@suse.com>

libinyang requested review from RanderWang, bardliao, dbaluta, juimonen, keyonjie, kv2019i, lgirdwood, lyakh, plbossart, ranj063, singalsu and xiulipan as code owners May 13, 2019 05:34

libinyang mentioned this pull request May 13, 2019

[FEATURE] hdmi needs to support Jack detection #912

Closed

libinyang force-pushed the hdmi_hotplug_detection branch from 0e60166 to 212f563 Compare May 13, 2019 06:00

kv2019i reviewed May 13, 2019

View reviewed changes

kv2019i requested changes May 13, 2019

View reviewed changes

plbossart reviewed May 13, 2019

View reviewed changes

libinyang force-pushed the hdmi_hotplug_detection branch from 212f563 to af24623 Compare May 14, 2019 02:54

plbossart reviewed May 14, 2019

View reviewed changes

libinyang force-pushed the hdmi_hotplug_detection branch from af24623 to 5b25b40 Compare May 15, 2019 06:20

plbossart requested changes May 15, 2019

View reviewed changes

libinyang force-pushed the hdmi_hotplug_detection branch from 5b25b40 to 6dca7d9 Compare May 16, 2019 02:31

mengdonglin mentioned this pull request May 17, 2019

[feature][sof->pulseaudio] Thinking about dropping the UCM, try to use legacy hdaudio mixer name #920

Closed

plbossart approved these changes May 17, 2019

View reviewed changes

ranj063 approved these changes May 17, 2019

View reviewed changes

plbossart merged commit 6b58a10 into thesofproject:topic/sof-dev May 17, 2019

Conversation

libinyang commented May 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

libinyang commented May 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

libinyang commented May 13, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

libinyang commented May 14, 2019

Uh oh!

ranj063 commented May 14, 2019

Uh oh!

libinyang commented May 14, 2019

Uh oh!

ranj063 commented May 14, 2019

Uh oh!

libinyang commented May 14, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plbossart commented May 14, 2019

Uh oh!

libinyang commented May 15, 2019

Uh oh!

libinyang commented May 15, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

libinyang May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

libinyang May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

libinyang commented May 16, 2019

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

libinyang commented May 13, 2019 •

edited

Loading

libinyang May 16, 2019 •

edited

Loading

libinyang May 17, 2019 •

edited

Loading