On Fri, Jan 30, 2015 at 06:27:10AM -0500, Yakir Yang wrote:

When transmitting IEC60985 linear PCM audio, we configure the Aduio Sample Channel Status information of all the channel status bits in the IEC60958 frame.

Signed-off-by: Yakir Yang ykk@rock-chips.com

Changes in v2:

• Add audio sample channel status setting

See my comments on

"[PATCH 01/11] drm: bridge/dw_hdmi: add audio sample channel status setting"

sent immediately prior to this series. This patch appears to be a copy of that patch.

On Sat, Jan 31, 2015 at 06:22:19AM -0500, Yang Kuankuan wrote:

On 01/31/2015 06:08 AM, Russell King - ARM Linux wrote:

...

On Fri, Jan 30, 2015 at 06:27:10AM -0500, Yakir Yang wrote:

...

When transmitting IEC60985 linear PCM audio, we configure the Aduio Sample Channel Status information of all the channel status bits in the IEC60958 frame.

Signed-off-by: Yakir Yang ykk@rock-chips.com

Changes in v2:

• Add audio sample channel status setting

See my comments on

"[PATCH 01/11] drm: bridge/dw_hdmi: add audio sample channel status setting"

sent immediately prior to this series. This patch appears to be a copy of that patch.

"[PATCH 01/11] drm: bridge/dw_hdmi: add audio sample channel status setting"

should not be send, it's my mistaken, i will remove it.

Please see my comments on the above patch for comments on _this_ patch. All my comments on the mistakenly sent 01/11 also apply to 02/12.

Thanks.

On 01/31/2015 06:20 AM, Russell King - ARM Linux wrote:

On Fri, Jan 30, 2015 at 06:30:39AM -0500, Yakir Yang wrote:

...

Add more n/cts values, in that case we can support audio for more display resolutions (128 * SampleRate = PixelClock * N / CTS).

Where do these come from? The iMX6 manuals give the set which are already in the driver, and says that others are not supported - to quote...

"Table below shows the CTS and N values for the supported standard. All other TMDS clocks are not supported, the TMDS clocks divided or multiplied by 1,001 coefficients are not supported."

and the table lists values for TMDS clocks of 25.2, 27, 54, 74.25, 148.5 and 297MHz.

This will need to be tested on iMX6 to validate whether it works. If it doesn't work, we need to conditionalise the new TMDS clocks with the IP version.

I just want to add audio support for some No-CEA display resolutions, and actually it can works on rk3288 platform. If it can not work on iMX6 platform, i will add IP verison select in next version v3.

Thanks. : )

On Fri, Jan 30, 2015 at 06:31:33AM -0500, Yakir Yang wrote:

If the monitor support audio, so we should support audio for it, even if the display resolution is No-CEA mode.

I can't find where it was documented at the moment, but I seem to remember reading somewhere that the iMX6 SoC version of this IP doesn't support audio with non-CEA modes - though, maybe that's a result of the limited TMDS clocks which are supported.

This is a change which will need testing on iMX6.

On Fri, Jan 30, 2015 at 06:32:23AM -0500, Yakir Yang wrote:

+static void hdmi_config_audio(struct dw_hdmi *hdmi,

• 	      struct hdmi_audio_fmt *aud_fmt)
  

+{

• if (aud_fmt)

• hdmi->aud_fmt = *aud_fmt;
  

• hdmi_modb(hdmi, AUDIO_CONF0_INTERFACE_II2S,

•   AUDIO_CONF0_INTERFACE_MSK, HDMI_AUD_CONF0);
  

• hdmi_modb(hdmi, hdmi->aud_fmt.chan_num, AUDIO_CONF0_I2SINEN_MSK,

•   HDMI_AUD_CONF0);
  

• hdmi_modb(hdmi, hdmi->aud_fmt.word_length, AUDIO_CONF1_DATWIDTH_MSK,

•   HDMI_AUD_CONF1);
  

• hdmi_modb(hdmi, hdmi->aud_fmt.dai_fmt, AUDIO_CONF1_DATAMODE_MSK,

•   HDMI_AUD_CONF1);
  

These registers are not defined on iMX6 SoCs, so this probably needs to be conditionalised with the dw-hdmi IP version.

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

+}

+void hdmi_audio_clk_disable(struct dw_hdmi *hdmi) +{

• if (!hdmi->audio_enable)

• return;
  

• mutex_lock(&hdmi->audio_mutex);
• hdmi_modb(hdmi, HDMI_MC_CLKDIS_AUDCLK_DISABLE,

•   HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
  

• hdmi->audio_enable = false;
• mutex_unlock(&hdmi->audio_mutex);

Ditto.

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); }

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

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

On 01/31/2015 07:00 AM, Russell King - ARM Linux wrote:

On Fri, Jan 30, 2015 at 06:23:51AM -0500, Yakir Yang wrote:

...

We found Designware hdmi driver only support audio clock config, we can not play sound through it. To add Designware HDMI Audio support, we make those patch set: 1): modify n/cts config order, according to dw_hdmi document. 2): add Audio Sample Channel Status config interfaces to dw_hdmi driver. 3): add audio support for more display resolutions(eg. 800x600). 4): add audio support for No-CEA display resolutions. 5): fixed dw_hdmi irq bug, add irq control to suspend/resume interfaces. 6): add suspend/resume callback for dw_hdmi rockchip driver. 7): filter interlace mode in rockchip vop driver. 8): add hdmi audio config interfaces to dw_hdmi driver. 9): creat "dw_hdmi-audio" platform device in dw_hdmi driver. 10): add codec driver for hdmi audio, callback dw_hdmi audio config functions. 11): add sound driver for hdmi audio, creat hdmi audio sound card. 12): add dt-bings file and add hdmi_audio node to corresponding dt file.

I think the overall issue with this patch is working out how to support both the iMX6 version of this IP, and the Rockchip version of the IP.

These two hardware IPs seem to be configured at synthesis time with entirely different audio architectures, which change which registers are available, and sometimes which bits in the registers are present, which makes it more difficult to come up with a unified audio driver.

Also, I think that it would be a good idea to start documenting which registers are available in which versions of the IP in dw_hdmi.h, otherwise I can see that it's going to be very easy for someone to assume that some register or bit which is available in one IP is present on all.

The CONFIGx_ID register values for the iMX6 SoC are:

CONFIG0_ID 0x8f CONFIG1_ID 0x01 CONFIG2_ID 0xf2 CONFIG3_ID 0x02

CONFIG0_ID appears to contain bits which indicate whether the IP supports I2S and SPDIF mode. Presumably your IP has bit 4 set for I2S, and maybe bit 5 for SPDIF?

CONFIG1_ID bit 0 indicates whether the AHB interface is present, which is presumably zero for your IP?

CONFIG3_ID bit 0 indicates whether "generic parallel audio, GPAUD" is present.

Could you provide (in addition to the values printed in the message I requested in another reply) the values of the CONFIGx_ID registers please, and whether any of the bits in there are documented as being applicable to audio.

Thanks.

The IP version on rk3288 : dwhdmi-rockchip ff980000.hdmi: Detected HDMI controller 0x20:0xa:0xa0:0xc1

The CONFIGx_ID register values for the rk3288 soc are: CONFIG0_ID 0xbf CONFIG1_ID 0x22 CONFIG2_ID 0xc2 CONFIG3_ID 0x0

After looking at iMX6 DQRM, i found only CONFIG1_ID & CONFIG3_ID are different.

CONFIG1_ID bit 5 indicates whether the HDMI2.0 is present. Bit 1 indicates whether configuration interface is APB interface.

CONFIG3_ID bit 3 indicates whether the AHBAUDDMA is present. Bit 1 indicates whether Generic Parallel Audio is present.