Re: [PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

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 <ykk@rock-chips.com> 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.