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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56e51c49-176e-3316-bd8c-f947061732bf@collabora.com>
Date:   Tue, 14 Mar 2017 08:53:16 +0100
From:   Romain Perier <romain.perier@...labora.com>
To:     Jose Abreu <Jose.Abreu@...opsys.com>,
        Russell King - ARM Linux <linux@...linux.org.uk>
Cc:     Neil Armstrong <narmstrong@...libre.com>,
        Archit Taneja <architt@...eaurora.org>,
        David Airlie <airlied@...ux.ie>,
        Heiko Stuebner <heiko@...ech.de>, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org,
        linux-rockchip@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the
 enablement functions

Hi,


Le 13/03/2017 à 19:49, Jose Abreu a écrit :
> Hi Russell,
>
>
> On 13-03-2017 13:10, Russell King - ARM Linux wrote:
>> On Mon, Mar 13, 2017 at 12:55:58PM +0000, Jose Abreu wrote:
>>> Hi,
>>>
>>>
>>> On 13-03-2017 09:36, Russell King - ARM Linux wrote:
>>>> On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
>>>>> On 03/10/2017 10:35 AM, 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().
>>>>>>
>>>>>> Signed-off-by: Romain Perier <romain.perier@...labora.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
>>>>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>>>>
>>>>> Hi Romain, Russell, Jose,
>>>>>
>>>>> This is a little out of scope, but I was wondering why the CTS calculation
>>>>> was not left in AUTO mode in the dw-hdmi driver ?
>>>> There is no indication in the iMX6 manuals that the iMX6 supports
>>>> automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
>>>> marked as "reserved".
>>>>
>>>> We're reliant on the information in the iMX6 manuals as we don't have
>>>> access to Synopsis' databooks for these parts (I understand you have
>>>> to be a customer to have access to that.)
>>>>
>>> (Synopsis -> Synopsys :))
>>>
>>> Trying to catch up with the conversation:
>>>
>>> 1) In AHB audio mode the bits are always reserved.
>>> 2) I think we should enable/disable clock instead of just forcing
>>> N/CTS, though, I don't know what could be the implications for
>>> iMX platform. I remember at the time I tested this using I2S
>>> (I've never used AHB), and HDMI protocol analyzers were
>>> complaining about the N/CTS being forced to zero.
>> What you're recommending is to basically ignore the published workaround,
>> which, presumably, was arrived at by proper analysis of the issue both
>> by Freescale engineers and Synopsys engineers, and instead try some other
>> solution.
>>
>> I'm not sure that's a good idea.  Only those with knowledge of the IP can
>> say what effect disabling the audio clock will have: will it still allow
>> the FIFO to be loaded, as required by the errata?  If not, it's not a
>> solution that AHB can use.  I would imagine that _if_ it was as simple as
>> disabling the audio clock, that simple approach would have been documented
>> in Freescale's errata documents, rather than the rather more complex
>> method of zeroing the CTS/N values.
> I'm just following what the user guide says: the final step of
> configuration is enabling the audio clock. There is no reference
> neither in datasheet neither in user guide about this behavior
> but, as I said, I've never used AHB so I can't prove what is the
> best solution. Could it be platform specific?

And that's precisely why I am enabling it

>
>> I think there are two choices here:
>>
>> 1) get some definitive information about the IP and the errata for AHB,
>>    and determine whether the solution you propose would work.  (That's
>>    out of scope for me, I've no contacts with FSL/NXP and I'm not a
>>    Synopsys customer.)
> This is the right thing to do but if you can't test in the
> Freescale platform then we have not much else to do.
>
> Best regards,
> Jose Miguel Abreu
>
>> 2) the I2S audio support stops re-using the AHB audio support functions,
>>    switching to their own implementation that behaves correctly for them.
>>    (Remember, it was I2S's choice to re-use the AHB audio support functions
>>    I added which we're now discussing - they didn't have to do that, and
>>    regressing AHB audio just for I2S is not something we should ever
>>    consider doing.)
>>

The workaround looks AHB specific in any cases and does not seem to work
with I2S. The I2S variant should not used the same functions, at least
for enabling/disabling audio stream.

Regards,
Romain