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

[Yang Kuankuan <ykk@rock-chips.com>]

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 <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.

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.