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

[Russell King - ARM Linux]

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:

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