From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754531AbbBBLxn (ORCPT ); Mon, 2 Feb 2015 06:53:43 -0500 Received: from pandora.arm.linux.org.uk ([78.32.30.218]:42576 "EHLO pandora.arm.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751540AbbBBLxl (ORCPT ); Mon, 2 Feb 2015 06:53:41 -0500 Date: Mon, 2 Feb 2015 11:53:15 +0000 From: Russell King - ARM Linux To: Daniel Kurtz Cc: Yang Kuankuan , David Airlie , Philipp Zabel , Fabio Estevam , Shawn Guo , Rob Clark , Mark Yao , Daniel Vetter , dri-devel , "linux-kernel@vger.kernel.org" , dbehr@chromoum.org, Heiko =?iso-8859-1?Q?St=FCbner?= , Douglas Anderson , =?iso-8859-1?Q?St=E9phane?= Marchesin , rockchip-discuss Subject: Re: [PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces Message-ID: <20150202115315.GB8656@n2100.arm.linux.org.uk> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > 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.) 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?" -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.