Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754657AbbBBMc7 (ORCPT ); Mon, 2 Feb 2015 07:32:59 -0500 Received: from regular1.263xmail.com ([211.150.99.139]:35408 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753790AbbBBMc6 (ORCPT ); Mon, 2 Feb 2015 07:32:58 -0500 X-263anti-spam: KSV:0; X-MAIL-GRAY: 0 X-MAIL-DELIVERY: 1 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: ykk@rock-chips.com X-FST-TO: rockchip-discuss@chromium.org X-SENDER-IP: 192.253.240.203 X-LOGIN-NAME: ykk@rock-chips.com X-UNIQUE-TAG: <390cd02696587414e49b6db7fcad38a3> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <54CF6E45.8080104@rock-chips.com> Date: Mon, 02 Feb 2015 07:32:05 -0500 From: Yang Kuankuan User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Russell King - ARM Linux , Daniel Kurtz CC: David Airlie , Philipp Zabel , Fabio Estevam , Shawn Guo , Rob Clark , Mark Yao , Daniel Vetter , dri-devel , "linux-kernel@vger.kernel.org" , dbehr@chromoum.org, =?windows-1252?Q?Heiko_St=FCbner?= , Douglas Anderson , =?windows-1252?Q?St=E9phane_Marchesin?= , rockchip-discuss Subject: Re: [PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces References: <1422617031-25098-1-git-send-email-ykk@rock-chips.com> <1422617543-25684-1-git-send-email-ykk@rock-chips.com> <20150131114806.GD26493@n2100.arm.linux.org.uk> <54CCE7EF.5040706@rock-chips.com> <20150202115315.GB8656@n2100.arm.linux.org.uk> In-Reply-To: <20150202115315.GB8656@n2100.arm.linux.org.uk> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5390 Lines: 137 On 02/02/2015 06:53 AM, Russell King - ARM Linux wrote: > On Mon, Feb 02, 2015 at 12:02:50PM +0800, Daniel Kurtz wrote: >> Hi ykk, >> >> On Sat, Jan 31, 2015 at 10:34 PM, Yang Kuankuan wrote: >>> On 01/31/2015 06:48 AM, Russell King - ARM Linux wrote: >>>>> +void hdmi_audio_clk_enable(struct dw_hdmi *hdmi) >>>>> +{ >>>>> + if (hdmi->audio_enable) >>>>> + return; >>>>> + >>>>> + mutex_lock(&hdmi->audio_mutex); >>>>> + hdmi->audio_enable = true; >>>>> + hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, >>>>> HDMI_MC_CLKDIS); >>>>> + mutex_unlock(&hdmi->audio_mutex); >>>> This is racy. The test needs to be within the mutex-protected region. >>> This function will be called by other driver (dw-hdmi-audio), both modify >>> the variable "hdmi->audio_enable", so i add the mutex-protected. >> >From your comment it isn't clear whether you understand what Russell meant. >> He is say you should do the following: >> >> { >> mutex_lock(&hdmi->audio_mutex); >> >> if (hdmi->audio_enable) { >> mutex_unlock(&hdmi->audio_mutex); >> return; >> } >> hdmi->audio_enable = true; >> hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS); >> >> mutex_unlock(&hdmi->audio_mutex); >> } > Yes, however, I prefer this kind of layout: > > mutex_lock(&hdmi->audio_mutex); > > if (!hdmi->audio_enable) { > hdmi->audio_enable = true; > hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, > HDMI_MC_CLKDIS); > } > > mutex_unlock(&hdmi->audio_mutex); > > but that's a matter of personal opinion. The important thing is that the > testing and setting of the flag are both within the protected region. > > However, there are other bugs here: what if the audio driver is calling > the sample rate setting function at the same time that a mode switch is > occuring. We actually need a mutex to protect more than just the > audio_enable flag. Okay, got it. Thanks. : ) >> By the way, it doesn't matter that the function is called from another driver. >> What matters is that this function can be called concurrently on >> multiple different threads of execution to change the hdmi audio >> enable state. >> >From DRM land, it is called with DRM lock held when enabling/disabling >> hdmi audio (mode_set / DPMS). >> It is also called from audio land, when enabling/disabling audio in >> response to some audio events (userspace ioctls?). I'm not sure >> exactly how the audio side works, or what locks are involved, but this >> mutex synchronizes calls from these two worlds to ensure that >> "hdmi->audio_enable" field always matches the current (intended) >> status of the hdmi audio clock. This would be useful, for example, if >> you needed to temporarily disable all HDMI clocks during a mode set, >> and then restore the audio clock to its pre-mode_set state: > There's some rather quirky comments in the driver right now which make > me uneasy about changing things too much. > > My initial idea would be that audio should remain disabled until the > audio driver wants it enabled, and the CTS/N values should either be > left alone, or set to a value which disables them (there is an iMX6 > errata which says to set N=0 initially, but as seems common with iMX6 > errata, I see no code implementing the method specified in the > documentation - I have found code implementing something similar > though.) I am confused with the way that audio driver realize the display resolution-changing. If audio driver cannot knowing that, then audio clock may be closed permanently ? > > However, there is this in the binding function: > > /* > * To prevent overflows in HDMI_IH_FC_STAT2, set the clk regenerator > * N and cts values before enabling phy > */ > hdmi_init_clk_regenerator(hdmi); > > which sets the N/CTS values assuming a 74.25MHz video clock and a 48kHz > sample rate. I've always wondered why this is necessary (I haven't > experimented with that yet.) > > Then there's this in the mode set function: > > /* HDMI Initialization Step E - Configure audio */ > hdmi_clk_regenerator_update_pixel_clock(hdmi); > hdmi_enable_audio_clk(hdmi); > > Where these "steps" come from, I've no idea (I can't find any documentation > which specifies them - maybe its from the Synopsis documentation?) but > this has always raised the question "what if audio is not enabled?" > static void hdmi_keep_audio_clk_status(struct dw_hdmi *hdmi) { mutex_lock(&hdmi->audio_mutex); if (hdmi->audio_enable) hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS); else hdmi_modb(hdmi, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS); mutex_unlock(&hdmi->audio_mutex); } /* HDMI Initialization Step E - Configure audio */ hdmi_clk_regenerator_update_pixel_clock(hdmi); hdmi_keep_audio_clk_status(hdmi); keep audio status maybe suitable here. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/