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

[Daniel Kurtz <djkurtz@chromium.org>]

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);
}

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:

  // temporarily disable hdmi audio clk
  dw_hdmi_audio_clk_disable(hdmi);
  // do mode_set ...
  dw_hdmi_audio_clk_reenable(hdmi);

 static void dw_hdmi_audio_clk_reenable()
 {
    mutex_lock()
    if (hdmi->audio_enable)
      dw_hdmi_audio_clk_enable(hdmi)
    mutex_unlock()
  }

However, AFAICT, the "hdmi->audio_enable" field is never actually used
like this here or in later patches, so it and the mutex do not seem to
actually be doing anything useful.  In this patch it is probably
better to just drop the mutex and audio_enable, and add them as a
preparatory patch in the patch set where they will actually be used.

-Dan