Hi ykk,
On Sat, Jan 31, 2015 at 10:34 PM, Yang Kuankuan <[email protected]> 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
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 <[email protected]> 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.
> 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.)
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?"
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
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 <[email protected]> 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.
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 <[email protected]> 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.
> >>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.
> 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.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
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 <[email protected]> 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?"
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 <[email protected]> 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.
>