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

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

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 <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.  : )
> 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:
>
> 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.
Thank you very much.  : )
I will take this order in next v3.
>>>> 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.



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