lkml.org 
[lkml]   [2015]   [Feb]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces

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:

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?"





\
 
 \ /
  Last update: 2015-02-03 04:21    [W:0.159 / U:0.316 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site