Re: [PATCH v2 08/12] drm: bridge/dw_hdmi: add audio config interfaces
From: Yang Kuankuan
Date: Mon Feb 02 2015 - 22:06:23 EST
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@xxxxxxxxxxxxxx> 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?"
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/