Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1596917 > unrolled thread

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the  enablement functions Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-10 10:50 +0100
    Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the  enablement functions Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-10 11:30 +0100
      Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the  enablement functions Romain Perier <romain.perier@collabora.com> - 2017-03-10 12:00 +0100
        Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the  enablement functions Russell King - ARM Linux <linux@armlinux.org.uk> - 2017-03-10 12:20 +0100
          Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the  enablement functions Romain Perier <romain.perier@collabora.com> - 2017-03-10 14:00 +0100
    Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the  enablement functions Romain Perier <romain.perier@collabora.com> - 2017-03-10 11:30 +0100

#1596917 — Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

On Fri, Mar 10, 2017 at 10:35:09AM +0100, Romain Perier wrote:
> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
> step E. and is kept enabled for later use. This clock should be enabled
> and disabled along with the actual audio stream and not always on (that
> is bad for PM). Futhermore, this might cause sound glitches with some
> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
> while the audio clock is still running.
> 
> This commit adds a parameter to hdmi_audio_enable_clk() that controls
> when the audio sample clock must be enabled or disabled. Then, it moves
> the call to this function into dw_hdmi_audio_enable() and
> dw_hdmi_audio_disable().

How does this interact with the workaround given in my commit introducing
these functions?  (Commit b90120a96608).

Setting N=0 is a work-around for iMX6, and we need the audio FIFO to be
loaded with data prior to setting N non-zero.  If disabling the audio
clock prevents the audio FIFO being loaded, your patch will break iMX6.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [next] | [standalone]

#1597005

On Fri, Mar 10, 2017 at 11:21:53AM +0100, Romain Perier wrote:
> Hello,
> 
> Le 10/03/2017 à 10:46, Russell King - ARM Linux a écrit :
> > On Fri, Mar 10, 2017 at 10:35:09AM +0100, Romain Perier wrote:
> >> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
> >> step E. and is kept enabled for later use. This clock should be enabled
> >> and disabled along with the actual audio stream and not always on (that
> >> is bad for PM). Futhermore, this might cause sound glitches with some
> >> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
> >> while the audio clock is still running.
> >>
> >> This commit adds a parameter to hdmi_audio_enable_clk() that controls
> >> when the audio sample clock must be enabled or disabled. Then, it moves
> >> the call to this function into dw_hdmi_audio_enable() and
> >> dw_hdmi_audio_disable().
> > How does this interact with the workaround given in my commit introducing
> > these functions?  (Commit b90120a96608).
> >
> > Setting N=0 is a work-around for iMX6, and we need the audio FIFO to be
> > loaded with data prior to setting N non-zero.  If disabling the audio
> > clock prevents the audio FIFO being loaded, your patch will break iMX6.
> >
> Mhhh, the fact is I have no IMX6 devices here (only Rockchip). So
> I only tested on Rockchip devices. An approach might be to introduce an
> option for handling this errata, because that's platform specific and
> other platforms (like Rockchip) are in conflict with this.

I'd say that they _aren't_ in conflict with it - or are you saying
that the I2S driver was never functionally tested as working?

I also would not think that it's platform specific - remember that
this is Designware IP, and it's likely that other platforms with
exactly the same IP would suffer from the same problem.  It's
probably revision specific, but we need Synopsis' input on that.

However, I do believe that your patch probably doesn't have much
merit in any case: on a mode set, you enable the audio clock, and
it will remain enabled until the audio device is first opened and
then closed.  If another mode set comes along, then exactly the
same situation happens again.  So, really it isn't achieving all
that much.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1597097

Hello,


Le 10/03/2017 à 11:27, Russell King - ARM Linux a écrit :
> On Fri, Mar 10, 2017 at 11:21:53AM +0100, Romain Perier wrote:
>> Hello,
>>
>> Le 10/03/2017 à 10:46, Russell King - ARM Linux a écrit :
>>> On Fri, Mar 10, 2017 at 10:35:09AM +0100, Romain Perier wrote:
>>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>>>> step E. and is kept enabled for later use. This clock should be enabled
>>>> and disabled along with the actual audio stream and not always on (that
>>>> is bad for PM). Futhermore, this might cause sound glitches with some
>>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>>>> while the audio clock is still running.
>>>>
>>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>>>> when the audio sample clock must be enabled or disabled. Then, it moves
>>>> the call to this function into dw_hdmi_audio_enable() and
>>>> dw_hdmi_audio_disable().
>>> How does this interact with the workaround given in my commit introducing
>>> these functions?  (Commit b90120a96608).
>>>
>>> Setting N=0 is a work-around for iMX6, and we need the audio FIFO to be
>>> loaded with data prior to setting N non-zero.  If disabling the audio
>>> clock prevents the audio FIFO being loaded, your patch will break iMX6.
>>>
>> Mhhh, the fact is I have no IMX6 devices here (only Rockchip). So
>> I only tested on Rockchip devices. An approach might be to introduce an
>> option for handling this errata, because that's platform specific and
>> other platforms (like Rockchip) are in conflict with this.

> or are you saying
> that the I2S driver was never functionally tested as working?

No ^^

>
> I also would not think that it's platform specific - remember that
> this is Designware IP, and it's likely that other platforms with
> exactly the same IP would suffer from the same problem.  It's
> probably revision specific, but we need Synopsis' input on that.
>
> However, I do believe that your patch probably doesn't have much
> merit in any case: on a mode set, you enable the audio clock, and
> it will remain enabled until the audio device is first opened and
> then closed.  If another mode set comes along, then exactly the
> same situation happens again.  So, really it isn't achieving all
> that much.
>
The fact is we still have sound glitches caused by this workaround on
Rockchip, we probably have a revision
of the IP without this issue. If CTS+N is not forced to zero we no
longer have the glitches :/ (with or without touching the clock)

Romain

[toc] | [prev] | [next] | [standalone]

#1597121

On Fri, Mar 10, 2017 at 11:58:19AM +0100, Romain Perier wrote:
> Hello,
> 
> Le 10/03/2017 à 11:27, Russell King - ARM Linux a écrit :
> > I also would not think that it's platform specific - remember that
> > this is Designware IP, and it's likely that other platforms with
> > exactly the same IP would suffer from the same problem.  It's
> > probably revision specific, but we need Synopsis' input on that.
> >
> > However, I do believe that your patch probably doesn't have much
> > merit in any case: on a mode set, you enable the audio clock, and
> > it will remain enabled until the audio device is first opened and
> > then closed.  If another mode set comes along, then exactly the
> > same situation happens again.  So, really it isn't achieving all
> > that much.
>
> The fact is we still have sound glitches caused by this workaround on
> Rockchip, we probably have a revision
> of the IP without this issue. If CTS+N is not forced to zero we no
> longer have the glitches :/ (with or without touching the clock)

Are you sure that removing the workaround just doesn't result in the
same bug as on iMX6 appearing?  The bug concerned is the ordering of
the channels, so unless you're (eg0 monitoring left/right separately
or directing audio to just one channel, you may not (with modern TVs)
realise that the channels are swapped.  I'll include the errata
description and impact below.

There are occasional issues on iMX6 as well despite this work-around,
but I don't clearly remember what devmem2 tweaks I've done in the past
to get it to resolve itself, nor could I describe them from memory
any better than "burbling audio".


  When the AHB Audio DMA is started, by setting to 1'b1 for the first
  time the register field AHB_DMA_START.data_buffer_ready, the AHB
  Audio DMA will request data from the AHB bus to fill its internal
  AHB DMA FIFO. It is possible that a AHB DMA FIFO read action occurs
  during the time window between the first sample stored on the AHB
  DMA FIFO and when the AHB DMA FIFO has stored, at least, the number
  of configured audio channels in samples. If this happens, the AHB
  DMA FIFO will reply with samples that are currently on the AHB
  Audio FIFO and will repeat the last sample after the empty condition
  is reached.

  Projected Impact:
  This will miss-align the audio stream, causing a shift in the
  distribution of audio on the channels on the HDMI sink side, with no
  knowledge of the DWC_hdmi_tx enabled system.


If you know that this definitely does not apply to your version, then
we need to split the audio enable/disable functions between the AHB
and I2S variants.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[toc] | [prev] | [next] | [standalone]

#1597480

[Multipart message — attachments visible in raw view] — view raw

Hello,


Le 10/03/2017 à 12:15, Russell King - ARM Linux a écrit :
> On Fri, Mar 10, 2017 at 11:58:19AM +0100, Romain Perier wrote:
>> Hello,
>>
>> Le 10/03/2017 à 11:27, Russell King - ARM Linux a écrit :
>>> I also would not think that it's platform specific - remember that
>>> this is Designware IP, and it's likely that other platforms with
>>> exactly the same IP would suffer from the same problem.  It's
>>> probably revision specific, but we need Synopsis' input on that.
>>>
>>> However, I do believe that your patch probably doesn't have much
>>> merit in any case: on a mode set, you enable the audio clock, and
>>> it will remain enabled until the audio device is first opened and
>>> then closed.  If another mode set comes along, then exactly the
>>> same situation happens again.  So, really it isn't achieving all
>>> that much.
>> The fact is we still have sound glitches caused by this workaround on
>> Rockchip, we probably have a revision
>> of the IP without this issue. If CTS+N is not forced to zero we no
>> longer have the glitches :/ (with or without touching the clock)
> Are you sure that removing the workaround just doesn't result in the
> same bug as on iMX6 appearing?  The bug concerned is the ordering of
> the channels, so unless you're (eg0 monitoring left/right separately
> or directing audio to just one channel, you may not (with modern TVs)
> realise that the channels are swapped.  I'll include the errata
> description and impact below.
>
> There are occasional issues on iMX6 as well despite this work-around,
> but I don't clearly remember what devmem2 tweaks I've done in the past
> to get it to resolve itself, nor could I describe them from memory
> any better than "burbling audio".

Well, I have reverted locally your patch (I did it quickly by hand
because it did not work via git revert... anyway...),
And I no longer have the issue at all. It's working fine all the time
and it's truly deterministic (see attachment)

Also, I have found that the version of the IP is not exactly the same.
On Rockchip it's 0x20:0xa:0xa0:0xc1, on IMX.6 it's  0x13:0xa:0xa0:0xc1.
>
>
>   When the AHB Audio DMA is started, by setting to 1'b1 for the first
>   time the register field AHB_DMA_START.data_buffer_ready, the AHB
>   Audio DMA will request data from the AHB bus to fill its internal
>   AHB DMA FIFO. It is possible that a AHB DMA FIFO read action occurs
>   during the time window between the first sample stored on the AHB
>   DMA FIFO and when the AHB DMA FIFO has stored, at least, the number
>   of configured audio channels in samples. If this happens, the AHB
>   DMA FIFO will reply with samples that are currently on the AHB
>   Audio FIFO and will repeat the last sample after the empty condition
>   is reached.
>
>   Projected Impact:
>   This will miss-align the audio stream, causing a shift in the
>   distribution of audio on the channels on the HDMI sink side, with no
>   knowledge of the DWC_hdmi_tx enabled system.

Thanks for this, it helps!
It looks specific to AHB audio

>
>
> If you know that this definitely does not apply to your version, then
> we need to split the audio enable/disable functions between the AHB
> and I2S variants.
Mhhh, yeah both seem to work differently.

Romain

[toc] | [prev] | [next] | [standalone]

#1597014

Hello,

Le 10/03/2017 à 10:46, Russell King - ARM Linux a écrit :
> On Fri, Mar 10, 2017 at 10:35:09AM +0100, Romain Perier wrote:
>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>> step E. and is kept enabled for later use. This clock should be enabled
>> and disabled along with the actual audio stream and not always on (that
>> is bad for PM). Futhermore, this might cause sound glitches with some
>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>> while the audio clock is still running.
>>
>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>> when the audio sample clock must be enabled or disabled. Then, it moves
>> the call to this function into dw_hdmi_audio_enable() and
>> dw_hdmi_audio_disable().
> How does this interact with the workaround given in my commit introducing
> these functions?  (Commit b90120a96608).
>
> Setting N=0 is a work-around for iMX6, and we need the audio FIFO to be
> loaded with data prior to setting N non-zero.  If disabling the audio
> clock prevents the audio FIFO being loaded, your patch will break iMX6.
>
Mhhh, the fact is I have no IMX6 devices here (only Rockchip). So
I only tested on Rockchip devices. An approach might be to introduce an
option for handling this errata, because that's platform specific and
other platforms (like Rockchip) are in conflict with this.

Romain

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web