From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.8 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 877ECC3524A for ; Tue, 4 Feb 2020 07:47:21 +0000 (UTC) Received: from alsa0.perex.cz (alsa0.perex.cz [77.48.224.243]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id DFF992087E for ; Tue, 4 Feb 2020 07:47:20 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=alsa-project.org header.i=@alsa-project.org header.b="aN2xHtgx" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DFF992087E Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=alsa-devel-bounces@alsa-project.org Received: from alsa1.perex.cz (alsa1.perex.cz [207.180.221.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa0.perex.cz (Postfix) with ESMTPS id 29E091654; Tue, 4 Feb 2020 08:46:29 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa0.perex.cz 29E091654 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=alsa-project.org; s=default; t=1580802439; bh=sn3Wa6U64H6nJvv7n+d6HktOZ0UE+vfOqL4c/osImgw=; h=Date:From:To:In-Reply-To:References:Cc:Subject:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=aN2xHtgxRrFrUvL4VQsAU5lesYSW34CVOqqcBigKpCoYzqZbmtXnA92RAx7KrzzFZ Hucwp2OYSdgSLYhhKSIY6rSJjyAVARNbVpSC+LzBlXCbh/NmMsbSvq9Gwkx+DwvTkf aXZMjDVqV2lWbs8fX7l65P/yqt6acsUqNRfLEZBE= Received: from alsa1.perex.cz (localhost.localdomain [127.0.0.1]) by alsa1.perex.cz (Postfix) with ESMTP id 9600CF8015B; Tue, 4 Feb 2020 08:46:28 +0100 (CET) Received: by alsa1.perex.cz (Postfix, from userid 50401) id 92040F80162; Tue, 4 Feb 2020 08:46:26 +0100 (CET) Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id 3F308F80051 for ; Tue, 4 Feb 2020 08:46:22 +0100 (CET) DKIM-Filter: OpenDKIM Filter v2.11.0 alsa1.perex.cz 3F308F80051 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 41A1BAC35; Tue, 4 Feb 2020 07:46:19 +0000 (UTC) Date: Tue, 04 Feb 2020 08:46:17 +0100 Message-ID: From: Takashi Iwai To: Nikhil Mahale In-Reply-To: References: <20200203100617.3856-1-nmahale@nvidia.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Cc: alsa-devel@alsa-project.org, martin@larkos.de, kai.vehmanen@linux.intel.com, aplattner@nvidia.com Subject: Re: [alsa-devel] [PATCH] ALSA: hda - Fix DP-MST support for NVIDIA codecs X-BeenThere: alsa-devel@alsa-project.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: "Alsa-devel mailing list for ALSA developers - http://www.alsa-project.org" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" On Tue, 04 Feb 2020 06:08:19 +0100, Nikhil Mahale wrote: > > On 2/3/20 4:10 PM, Takashi Iwai wrote: > > External email: Use caution opening links or attachments > > > > > > On Mon, 03 Feb 2020 11:06:17 +0100, > > Nikhil Mahale wrote: > >> > >> If dyn_pcm_assign is set, different jack objects are being created > >> for pcm and pins. > >> > >> If dyn_pcm_assign is set, generic_hdmi_build_jack() calls into > >> add_hdmi_jack_kctl() to create and track separate jack object for > >> pcm. Like sync_eld_via_acomp(), hdmi_present_sense_via_verbs() also > >> need to report status change of the pcm jack. > >> > >> Rename pin_idx_to_jack() to pin_idx_to_pcm_jack(). The code to > >> report status change of pcm jack, move it to update_eld() which is > >> common for acomp and !acomp code paths. > > > > Thanks, that's the cause of the regression. > > However, this needs yet more careful handling, I'm afraid. > > > > - hdmi_present_sense_via_verbs() may return true, and its callers > > (both check_presence_and_report() and hdmi_repoll_eld()) do call > > snd_hda_jack_report_sync() again. > > > > - For non-dyn_pcm_assign case, we shouldn't call jack report there, > > but rather simply return true for calling report sync. > > > > - There is another workaround to block the jack report in > > hdmi_present_sense_via_verbs() which is applied after update_eld(), > > and this will be ignored. > > > > So, I guess that we need the conditional application of the individual > > snd_jack_report() only for spec->dyn_pcm_assign==true, and assure that > > hdmi_present() returns false. > Yeah, you are right. But I don't think we should return false from > hdmi_present(). > > Before dyn_pcm_assign support for non-acomp drivers: > 1) pcm and pin plug detection were controlled by same jack object, and > 2) change in plug status was reported from snd_hda_jack_report_sync(). > > If dyn_pcm_assign support is enabled for non-acomp drivers, then pcm > and pin detection are controlled by different jack object. Now, report > for plug status change of both jack object, requires to be in sync. That's my concern. Basically we're seeing two different jacks for a single hotplug. OTOH, from the user-space POV, it must be one jack that should report back. IOW, with dyn_pcm_assign, you should ignore the jack triggered from the unsol handler but report back only for the jack that is assigned to the PCM. > snd_hda_jack_report_sync() reports change in plug status of pin jack > object. > > I think after snd_hda_jack_report_sync() we should loop over > all pins, detect change in plug status, and report change in plug > status of pcm jack. This is what snd_hda_jack_report_sync() does; it loops over all registered jacks and report the changes. So, I think that in dyn_pcm_assign=true without acomp, we need to clear jack->dirty for the originally reported jack in addition to the translation to the PCM jack and reporting. FWIW, the dirty flag is set in hdmi_intrinsic_event() and hdmi_repoll_eld(). And if you call snd_jack_report() for the necessary jack, there is no need to call snd_jack_report_sync() at all. This is utterly superfluous. Hence, if the repoll isn't needed and dyn_pcm_assign=1, the function should return false. > > The last item (the jack report block) is still unclear to me; it's a > > workaround that was needed for Nvidia drivers in the past due to > > instability. If this is still needed for DP-MST case, we have to > > reconsider how to deal with it. Otherwise, this can be applied only > > for non-dyn_pcm_assign case. > The jack report block, was added by commit 464837a7bc0a (ALSA: hda - > block HDMI jack reports while repolling), to avoid race condition > with repolling. That is not NVIDIA specific. Ah yes, that was for some unstable state handling. I thought the same workaround applied to some old Nvidia driver, too, though. > > BTW, the condition for jack->block_report and return value in > > hdmi_present_sense_via_verbs() looks currently complicated, but it > > could have been simplified like: > > > > -- 8< -- > > --- a/sound/pci/hda/patch_hdmi.c > > +++ b/sound/pci/hda/patch_hdmi.c > > @@ -1569,7 +1569,6 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin, > > * the unsolicited response to avoid custom WARs. > > */ > > int present; > > - bool ret; > > bool do_repoll = false; > > > > present = snd_hda_jack_pin_sense(codec, pin_nid, dev_id); > > @@ -1603,16 +1602,14 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin, > > else > > update_eld(codec, per_pin, eld); > > > > - ret = !repoll || !eld->monitor_present || eld->eld_valid; > > - > > jack = snd_hda_jack_tbl_get_mst(codec, pin_nid, per_pin->dev_id); > > if (jack) { > > - jack->block_report = !ret; > > + jack->block_report = do_repoll; > > jack->pin_sense = (eld->monitor_present && eld->eld_valid) ? > > AC_PINSENSE_PRESENCE : 0; > > } > > mutex_unlock(&per_pin->lock); > > - return ret; > > + return !do_repoll; > > } > > > > static struct snd_jack *pin_idx_to_jack(struct hda_codec *codec, > > -- 8< -- > Yeah, this is simple to understand. > > I am sending fresh patches, see if they make sense. The cleanup like the above shouldn't be applied before the critical fix. That is, we have to fix the regression at the very first patch with Cc to stable. This must be applicable cleanly and working on 5.5 kernel as is. After the fix, we may apply the remaining cleanups like the above. So, let's concentrate on one (or split if needed) fix patch for now. thanks, Takashi _______________________________________________ Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel