| Message ID | 1422617543-25684-1-git-send-email-ykk@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/31/2015 06:48 AM, Russell King - ARM Linux wrote: > 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. okay, i will fixed what you have suggest in cover-letter first. Thanks. : ) >> +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. > >> +} >> + >> +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 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. > 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?"
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 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: 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.
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 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: > > 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. >
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c index 8cc7b3d..25c1678 100644 --- a/drivers/gpu/drm/bridge/dw_hdmi.c +++ b/drivers/gpu/drm/bridge/dw_hdmi.c @@ -16,6 +16,7 @@ #include <linux/err.h> #include <linux/clk.h> #include <linux/hdmi.h> +#include <linux/mutex.h> #include <linux/of_device.h> #include <drm/drm_of.h> @@ -126,7 +127,10 @@ struct dw_hdmi { struct i2c_adapter *ddc; void __iomem *regs; - unsigned int sample_rate; + struct hdmi_audio_fmt aud_fmt; + struct mutex audio_mutex; + bool audio_enable; + int ratio; void (*write)(struct dw_hdmi *hdmi, u8 val, int offset); @@ -208,7 +212,7 @@ static void hdmi_set_schnl(struct dw_hdmi *hdmi) { u8 aud_schnl_samplerate; - switch (hdmi->sample_rate) { + switch (hdmi->aud_fmt.sample_rate) { case 32000: aud_schnl_samplerate = HDMI_FC_AUDSCHNLS7_SMPRATE_32K; break; @@ -444,9 +448,9 @@ static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi, { unsigned int clk_n, clk_cts; - clk_n = hdmi_compute_n(hdmi->sample_rate, pixel_clk, + clk_n = hdmi_compute_n(hdmi->aud_fmt.sample_rate, pixel_clk, hdmi->ratio); - clk_cts = hdmi_compute_cts(hdmi->sample_rate, pixel_clk, + clk_cts = hdmi_compute_cts(hdmi->aud_fmt.sample_rate, pixel_clk, hdmi->ratio); if (!clk_cts) { @@ -456,7 +460,7 @@ static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi, } dev_dbg(hdmi->dev, "%s: samplerate=%d ratio=%d pixelclk=%lu N=%d cts=%d\n", - __func__, hdmi->sample_rate, hdmi->ratio, + __func__, hdmi->aud_fmt.sample_rate, hdmi->ratio, pixel_clk, clk_n, clk_cts); hdmi_regenerate_n_cts(hdmi, clk_n, clk_cts); @@ -1023,6 +1027,25 @@ static void hdmi_tx_hdcp_config(struct dw_hdmi *hdmi) HDMI_A_HDCPCFG1_ENCRYPTIONDISABLE_MASK, HDMI_A_HDCPCFG1); } +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); +} + static void hdmi_config_AVI(struct dw_hdmi *hdmi) { u8 val, pix_fmt, under_scan; @@ -1212,6 +1235,34 @@ static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi) hdmi->phy_enabled = false; } +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); +} + +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); +} + +bool hdmi_get_connect_status(struct dw_hdmi *hdmi) +{ + return (hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD) ? true : false; +} + /* HDMI Initialization Step B.4 */ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi) { @@ -1240,11 +1291,8 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi) clkdis &= ~HDMI_MC_CLKDIS_CSCCLK_DISABLE; hdmi_writeb(hdmi, clkdis, HDMI_MC_CLKDIS); } -} -static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi) -{ - hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS); + hdmi_audio_clk_disable(hdmi); } /* Workaround to clear the overflow condition */ @@ -1342,7 +1390,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode) /* HDMI Initialization Step E - Configure audio */ hdmi_clk_regenerator_update_pixel_clock(hdmi); - hdmi_enable_audio_clk(hdmi); + hdmi_audio_clk_enable(hdmi); /* HDMI Initialization Step F - Configure AVI InfoFrame */ hdmi_config_AVI(hdmi); @@ -1669,7 +1717,7 @@ int dw_hdmi_bind(struct device *dev, struct device *master, hdmi->plat_data = plat_data; hdmi->dev = dev; hdmi->dev_type = plat_data->dev_type; - hdmi->sample_rate = 48000; + hdmi->aud_fmt.sample_rate = 48000; hdmi->ratio = 100; hdmi->encoder = encoder; diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h b/drivers/gpu/drm/bridge/dw_hdmi.h index 603e645..3ab14b6 100644 --- a/drivers/gpu/drm/bridge/dw_hdmi.h +++ b/drivers/gpu/drm/bridge/dw_hdmi.h @@ -904,6 +904,12 @@ enum { HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_POL = 0x08, HDMI_PHY_I2CM_CTLINT_ADDR_ARBITRATION_MASK = 0x04, +/* AUD_CONF0 field values */ + AUDIO_CONF0_INTERFACE_MSK = 0x20, + AUDIO_CONF0_INTERFACE_II2S = 0x20, + AUDIO_CONF0_INTERFACE_SPDIF = 0x00, + AUDIO_CONF0_INTERFACE_GPA = 0x00, + /* AUD_N3 field values */ HDMI_AUD_N3_NCTS_ATOMIC_WRITE_MASK = 0x80, HDMI_AUD_N3_NCTS_ATOMIC_WRITE_SET = 0x80, diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h index 8476cfc..aafa447 100644 --- a/include/drm/bridge/dw_hdmi.h +++ b/include/drm/bridge/dw_hdmi.h @@ -44,6 +44,36 @@ struct dw_hdmi_sym_term { u16 term; /*transmission termination value*/ }; +enum hdmi_audio_wordlength { + AUDIO_WORDLENGTH_16BIT = 16, + AUDIO_WORDLENGTH_24BIT = 24, + AUDIO_CONF1_DATWIDTH_MSK = 0x1F, +}; + +enum hdmi_audio_daifmt { + AUDIO_DAIFMT_IIS = 0x00, + AUDIO_DAIFMT_RIGHT_J = 0x20, + AUDIO_DAIFMT_LEFT_J = 0x40, + AUDIO_DAIFMT_BURST_1 = 0x60, + AUDIO_DAIFMT_BURST_2 = 0x80, + AUDIO_CONF1_DATAMODE_MSK = 0xE0, +}; + +enum hdmi_audio_channelnum { + AUDIO_CHANNELNUM_2 = 0x01, + AUDIO_CHANNELNUM_4 = 0x03, + AUDIO_CHANNELNUM_6 = 0x07, + AUDIO_CHANNELNUM_8 = 0x0F, + AUDIO_CONF0_I2SINEN_MSK = 0x0F, +}; + +struct hdmi_audio_fmt { + unsigned int sample_rate; + enum hdmi_audio_daifmt dai_fmt; + enum hdmi_audio_channelnum chan_num; + enum hdmi_audio_wordlength word_length; +}; + struct dw_hdmi_plat_data { enum dw_hdmi_devtype dev_type; const struct dw_hdmi_mpll_config *mpll_cfg;
Designware HDMI supports four interfaces to config hdmi audio (I2S, S/PDIF, Generic Parallel Audio, AHB Audio DMA), but rk3288 only support two ways to config hdmi audio(I2S, S/PDIF), So we take I2S as hdmi audio operation interfaces. Signed-off-by: Yakir Yang <ykk@rock-chips.com> --- Changes in v2: - Add audio config interfaces to dw_hdmi driver drivers/gpu/drm/bridge/dw_hdmi.c | 70 +++++++++++++++++++++++++++++++++------- drivers/gpu/drm/bridge/dw_hdmi.h | 6 ++++ include/drm/bridge/dw_hdmi.h | 30 +++++++++++++++++ 3 files changed, 95 insertions(+), 11 deletions(-)