Re: [PATCH] ALSA: hda - program ICT bits to support HBR audio

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 09/19/2017 04:31 PM, Pierre-Louis Bossart wrote:
> On 9/19/17 3:10 PM, Takashi Iwai wrote:
>> On Tue, 19 Sep 2017 17:00:01 +0200,
>> Subhransu S. Prusty wrote:
>>>
>>> From: Sriram Periyasamy <sriramx.periyasamy@intel.com>
>>>
>>> On recent Intel platforms (Haswell, Broadwell, Skylake, ApolloLake,
>>> KabyLake, ...), the IEC Coding Type (ICT) bitfield in the Digital
>>> Converter Control #3 needs to be set explicitly for HDMI/DisplayPort
>>> High Bit Rate (HBR) audio playback to work. This was not required in
>>> earlier platforms when HBR was first introduced. The ICT bits are
>>> defined in Section 7.3.3.9 of the HDaudio 1.0a specification.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98797
>>>
>>> Signed-off-by: Sriram Periyasamy <sriramx.periyasamy@intel.com>
>>> Signed-off-by: Pierre-Louis Bossart 
>>> <pierre-louis.bossart@linux.intel.com>
>>> Signed-off-by: Subhransu S. Prusty <subhransu.s.prusty@intel.com>
>>> ---
>>>   include/sound/hda_verbs.h  |  1 +
>>>   sound/pci/hda/patch_hdmi.c | 18 ++++++++++++++++++
>>>   2 files changed, 19 insertions(+)
>>>
>>> diff --git a/include/sound/hda_verbs.h b/include/sound/hda_verbs.h
>>> index d0509db6d0ec..d2004d7feb2b 100644
>>> --- a/include/sound/hda_verbs.h
>>> +++ b/include/sound/hda_verbs.h
>>> @@ -54,6 +54,7 @@ enum {
>>>   #define AC_VERB_GET_EAPD_BTLENABLE        0x0f0c
>>>   #define AC_VERB_GET_DIGI_CONVERT_1        0x0f0d
>>>   #define AC_VERB_GET_DIGI_CONVERT_2        0x0f0e /* unused */
>>> +#define AC_VERB_SET_DIGI_CONVERT_3        0x73e
>>
>> The SET verb should be placed in the dedicated section instead.
>>
>>
>>>   #define AC_VERB_GET_VOLUME_KNOB_CONTROL        0x0f0f
>>>   /* f10-f1a: GPIO */
>>>   #define AC_VERB_GET_GPIO_DATA            0x0f15
>>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
>>> index 53f9311370de..2dcd99d2a757 100644
>>> --- a/sound/pci/hda/patch_hdmi.c
>>> +++ b/sound/pci/hda/patch_hdmi.c
>>> @@ -906,6 +906,7 @@ static int hdmi_setup_stream(struct hda_codec 
>>> *codec, hda_nid_t cvt_nid,
>>>                     hda_nid_t pin_nid, u32 stream_tag, int format)
>>>   {
>>>       struct hdmi_spec *spec = codec->spec;
>>> +    unsigned int param;
>>>       int err;
>>>         err = spec->ops.pin_hbr_setup(codec, pin_nid, 
>>> is_hbr_format(format));
>>> @@ -915,6 +916,23 @@ static int hdmi_setup_stream(struct hda_codec 
>>> *codec, hda_nid_t cvt_nid,
>>>           return err;
>>>       }
>>>   +    /*
>>> +     * on recent platforms IEC Coding Type is required for HBR 
>>> support,
>>> +     * read current Digital Converter settings and set ICT bitfield if
>>> +     * needed.
>>> +     */
>>> +    param = snd_hda_codec_read(codec, cvt_nid, 0,
>>> +                AC_VERB_GET_DIGI_CONVERT_1, 0);
>>> +
>>> +    param = (param >> 16) & ~(AC_DIG3_ICT);
>>> +
>>> +    /* on recent platforms ICT mode is required for HBR support */
>>> +    if (is_hbr_format(format))
>>> +        param |= 0x1;
>>> +
>>> +    snd_hda_codec_write(codec, cvt_nid, 0,
>>> +                AC_VERB_SET_DIGI_CONVERT_3, param);
>>
>> Do all codecs support this verb?  I'm a bit worried, since this
>> function gets called for all codecs, and some of them must be fairly
>> old.
>
> My favorite hobby: forensic software engineering...
>
> This converter contains both the ICT (IEC coding type) and KAE (Keep 
> Alive Enable).
> The HDaudio 1.0 spec dated (April 15, 2004) makes no reference to any 
> of those bits - even if it lists HDMI as a supported output (HDMI 1.0 
> was released in 2002).
>
> HDMI 1.3 added support for HDMI HBR and was released on June 22, 2006,
>
> The first DCN I see with those bits is from June 6, 2009
> https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/high-definition-audio-low-power.pdf 
>
> KAE became mandatory after July 1st, 2011.
>
> So yes in theory there is a risk that on a platform produced between 
> 2004 and 2009 (maybe 2011) accessing this verb might generate an error 
> or an warning. I don't know the standard and implementation enough to 
> know if this can be queried and what happens when accessing undefined 
> verbs. The oldest working machine I have is from 2010, older stuff was 
> scrapped/recycled.
>
> I guess we could set a capability and set it for newer platforms 
> making use of intel_hsw_common_init() in patch_hdmi.c (we already use 
> the codec ID to match the relevant quirks). We could also check on 
> Baytrail if it's needed with a Minnowboard.
I just tested on Baytrail and this fix is not needed. It's also not 
needed on Cantiga/IronLake, so adding a is_haswell_plus test should be 
enough to both enable HBR and avoid accessing a register for no good 
reason. will send a v2 shortly.