From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755868AbbBCDGT (ORCPT ); Mon, 2 Feb 2015 22:06:19 -0500 Received: from regular1.263xmail.com ([211.150.99.137]:47516 "EHLO regular1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751540AbbBCDGS (ORCPT ); Mon, 2 Feb 2015 22:06:18 -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: <62df87a4a88df21302b753f9813750a8> X-ATTACHMENT-NUM: 0 X-DNS-TYPE: 0 Message-ID: <54D03B03.5040808@rock-chips.com> Date: Mon, 02 Feb 2015 22:05:39 -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 CC: Daniel Kurtz , 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> <54CF6E45.8080104@rock-chips.com> <20150202130920.GC8656@n2100.arm.linux.org.uk> In-Reply-To: <20150202130920.GC8656@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 On 02/02/2015 08:09 AM, Russell King - ARM Linux wrote: > On Mon, Feb 02, 2015 at 07:32:05AM -0500, Yang Kuankuan wrote: >> 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. : ) > I've been moving my code closer to what you have posted. I've split up > your patches into smaller steps so each change can be evaluated on iMX6. > So far: Thank you very much. : ) > > drm: bridge/dw_hdmi: combine hdmi_set_clock_regenerator_n() and hdmi_regenerate_cts() > > This patch _just_ combines the two functions without any other changes. > > drm: bridge/dw_hdmi: protect n/cts setting with a mutex > > This patch _just_ adds a mutex to protect the calls to > hdmi_set_clk_regenerator(), since we will need to call that from both > the DRM and audio drivers. > > drm: bridge/dw_hdmi: adjust n/cts setting order > > This patch changes the order to: > - write CTS3 CTS_manual = 0 > - write CTS3 N_shift = 0 > - write CTS3 CTS value > - write CTS2 CTS value > - write CTS1 CTS value > - write N3 N value > - write N2 N value > - write N1 N value > which is broadly what you're doing, but without the initial N3 write > and without CTS_manual=1. I've asked Freescale whether they can > clarify what effect adding those would have on their SoCs. > > Note: the mutex in my second patch needs to be a spinlock - as it seems > my new workaround for iMX6 ERR005174 needs to call the CTS/N setting > functions in an irqs-off region (from the ALSA ->trigger callback.) > That will need further rework of the CTS/N code to reduce the size of > the spinlock protected region. > > Incidentally, your Synopsis IP indicates that it is the same revision > as the iMX6's IP revision which suffers from this ERR005174 errata. > I think you need to check whether this errata applies on your SoC too. > Seach for "iMX6 ERR005174" in google. >>>> 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 ? > The audio driver shouldn't care about the display resolution. That > should be the responsibility of the dw_hdmi core - as it is at the > moment. Do you mean that we should disable audio clock and deinit the n/cts values, until we meet the audio enable single like this. if (hdmi->vmode.mdiv) { /* HDMI Initialization Step E - Configure audio */ hdmi_clk_regenerator_update_pixel_clock(hdmi); hdmi_enable_audio_clk(hdmi); } >> 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. > What I don't know right now is what triggers this overflow indication in > HDMI_IH_FC_STAT2, and whether the code I quoted (which fakes the audio > setup) is actually necessary. > > In other words: > - is it necessary to have the audio clock enabled when video is enabled > to prevent the overflow indication? We don't know. Therefore, we > can't say whether it is permitted to disable the audio clock when > audio is inactive. > - is it necessary to program a dummy CTS/N value for 74.25MHz/48kHz, > or can we program a non-zero CTS value and a zero N at initialisation > until the audio driver comes up? Again, we don't know. > > What we do know is that as the driver stands, it works for video output. > With my changes for AHB audio support on the iMX6 (which includes some > errata workarounds found in the iMX6 errata documentation to avoid FIFO > issues), we have working audio there. > I don't know the effect of overflow indication in HDMI_IH_FC_STAT2, seems the irq function have not handle the FC_STAT2 interrupt in dw_hdmi driver, also not found in your dw-hdmi-audio driver. But I will talk with our IC colleges, - is it necessary to have the audio clock enabled when video is enabled to prevent the overflow indication ? If it is not necessary, maybe we can keep the audio status in mode_set. - Also is it necessary to program a dummy CTS/N value for 74.25MHz/48kHz, or can we program a non-zero CTS value and a zero N at initialisation until the audio driver comes up >>>>>> 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?"