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




\
 
 \ /
  Last update: 2015-02-02 14:01    [W:0.132 / U:0.756 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site