Re: [PATCH] drm/i915/chv: Enable HDMI Clock recovery for Pipe C

[Clint Taylor <clinton.a.taylor@intel.com>]

On 12/05/2014 05:23 AM, Imre Deak wrote:
> On Fri, 2014-12-05 at 13:57 +0200, Jani Nikula wrote:
>> On Fri, 05 Dec 2014, Imre Deak <imre.deak@intel.com> wrote:
>>> On Fri, 2014-12-05 at 10:37 +0200, Jani Nikula wrote:
>>>> On Fri, 05 Dec 2014, Clint Taylor <clinton.a.taylor@intel.com> wrote:
>>>>> On 12/04/2014 11:08 AM, Clint Taylor wrote:
>>>>>> On 12/04/2014 12:41 AM, Jani Nikula wrote:
>>>>>>> On Wed, 03 Dec 2014, Clint Taylor <clinton.a.taylor@intel.com> wrote:
>>>>>>>> On 12/03/2014 01:01 PM, Ville Syrjälä wrote:
>>>>>>>>> On Wed, Dec 03, 2014 at 10:10:30AM -0800, clinton.a.taylor@intel.com
>>>>>>>>> wrote:
>>>>>>>>>> From: Clint Taylor <clinton.a.taylor@intel.com>
>>>>>>>>>>
>>>>>>>>>> Added PIPE C register support for CHV audio programming.
>>>>>>>>>
>>>>>>>>> nak. The offset between the pipes looks constant so it should work
>>>>>>>>> just fine with _PIPE().
>>>>>>>>
>>>>>>>> Correct. Now I just need to figure out why AUD_CONFIG_C is cleared after
>>>>>>>> ChromeOS boot.
>>>>>>>
>>>>>>> "after boot" is a long time! ;)
>>>>>>>
>>>>>>> Do your logs show that ilk_audio_codec_disable (see intel_audio.c) gets
>>>>>>> called? With drm.debug=14 you should see "Disable audio codec on port
>>>>>>> %c, pipe %c\n".
>>>>>>>
>>>>>>> BR,
>>>>>>> Jani.
>>>>>>>
>>>>>>
>>>>>> I've actually instrumented the driver with more logging and at 22.43 in
>>>>>> the dmesg log the last call to audio_codec_enable() is called and is
>>>>>> setting the Pixel_Clock bits of AUD_CONFIG_C to 0x9 which is correct for
>>>>>> 1080p60. Sometime after 22.436 and before the ChromeOS login screen
>>>>>> appears the register is being cleared to 0x0 without knowledge/action of
>>>>>> the i915 driver. Mode change and power state change seem to restore the
>>>>>> programming correctly.
>>>>>>
>>>>>>    Most likely candidate right now is an ordering issue with snd_hda
>>>>>> during boot.
>>>>>>
>>>>>
>>>>> Ok, removing snd_hda drivers from the boot process did not change the
>>>>> result. Any ideas about what could reset the HD_AUDIO registers to their
>>>>> default?
>>>>
>>>> Maybe it needs some power domain/well we fail to get? Similar to what
>>>> DDI based platforms do before and after calls to
>>>> intel_audio_codec_enable and intel_audio_codec_disable in intel_ddi.c.
>>>>
>>>> Not really my area, maybe Imre and Ville know better?
>>>
>>> The audio power domain->power well mapping is mapped correctly in the
>>> i915 driver (HDA belongs to the display aka pipe-A power well there).
>>> But AFAICS the intel_hda driver doesn't ask for this power domain on
>>> VLV/CHV, AZX_DCAPS_I915_POWERWELL is not included in the relevant hda
>>> device capability flags. CC'ing Takashi for this.
>>
>> Imre, i915_request_power_well() only works for hsw/bdw.
>
> Hm, right I should have checked that first. And it could be also the
> reason why AZX_DCAPS_I915_POWERWELL was left out for everything but
> HSW/BDW. I will send a patch to fix that in i915.
>
> Thanks,
> Imre
>
I have applied Imre's request_power_well patch, and tried setting 
intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO) directly when the 
audio codec is enabled and the register is still not accepting pixel 
clock programming updates during boot.

I have found that the register will not accept the value until after 
snd_hda_intel initializes the PCI device. Unfortunately time wise this 
occurs after the last attempt made by the i915 driver.

I will continue to investigate next week.

[   25.993784]  *** CAT display power get
[   25.998016]  *** CAT enable aud_config_c read = 0x00000000

[   31.825282] snd_hda_intel 0000:00:1b.0: irq 403 for MSI/MSI-X

-Clint


>>
>> BR,
>> Jani.
>>
>>>
>>> Still, I'm not sure why the register gets zeroed, if there is no power
>>> well off->on transition. You could first check if this is really true,
>>> via the debug print intel_display_power_get() and
>>> intel_display_power_put() emits.
>>>
>>> Another idea would be to trace writes to this register through
>>> I915_WRITE.
>>>
>>> --Imre
>>>
>>>>
>>>>
>>>> BR,
>>>> Jani.
>>>>
>>>>
>>>>
>>>>>
>>>>> -clint
>>>>>
>>>>>
>>>>>> [    8.607022] [drm:intel_audio_codec_enable] ELD on
>>>>>> [CONNECTOR:37:HDMI-A-3], [ENCODER:36:TMDS-36]
>>>>>> [    8.607027] [drm:ilk_audio_codec_enable] Enable audio codec on port
>>>>>> D, pipe C, 8 bytes ELD
>>>>>> [    8.607044]  *** CAT enable aud_config reg 0x001E2200 = 0x00090000
>>>>>> [   14.741047] [drm:ilk_audio_codec_disable] Disable audio codec on port
>>>>>> D, pipe C
>>>>>> [   14.741055]  *** CAT disable aud_config reg 0x001E2200 = 0x10000000
>>>>>> [   15.773618] [drm:intel_audio_codec_enable] ELD on
>>>>>> [CONNECTOR:37:HDMI-A-3], [ENCODER:36:TMDS-36]
>>>>>> [   15.773624] [drm:ilk_audio_codec_enable] Enable audio codec on port
>>>>>> D, pipe C, 8 bytes ELD
>>>>>> [   15.779315]  *** CAT enable aud_config reg 0x001E2200 = 0x00090000
>>>>>> [   19.936767] [drm:ilk_audio_codec_disable] Disable audio codec on port
>>>>>> D, pipe C
>>>>>> [   19.936778]  *** CAT disable aud_config reg 0x001E2200 = 0x10000000
>>>>>> [   20.292920] [drm:intel_audio_codec_enable] ELD on
>>>>>> [CONNECTOR:37:HDMI-A-3], [ENCODER:36:TMDS-36]
>>>>>> [   20.292928] [drm:ilk_audio_codec_enable] Enable audio codec on port
>>>>>> D, pipe C, 8 bytes ELD
>>>>>> [   20.298615]  *** CAT enable aud_config reg 0x001E2200 = 0x00090000
>>>>>> [   20.389110] [drm:ilk_audio_codec_disable] Disable audio codec on port
>>>>>> D, pipe C
>>>>>> [   20.389121]  *** CAT disable aud_config reg 0x001E2200 = 0x10000000
>>>>>> [   22.430931] [drm:intel_audio_codec_enable] ELD on
>>>>>> [CONNECTOR:37:HDMI-A-3], [ENCODER:36:TMDS-36]
>>>>>> [   22.430940] [drm:ilk_audio_codec_enable] Enable audio codec on port
>>>>>> D, pipe C, 8 bytes ELD
>>>>>> [   22.436627]  *** CAT enable aud_config reg 0x001E2200 = 0x00090000
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> -Clint
>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
>>>>>>>>>> ---
>>>>>>>>>>     drivers/gpu/drm/i915/i915_reg.h |   18 ++++++++++++------
>>>>>>>>>>     1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>>>>>>>>>> b/drivers/gpu/drm/i915/i915_reg.h
>>>>>>>>>> index dc03fac..3d5813a 100644
>>>>>>>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>>>>>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>>>>>>>> @@ -6189,14 +6189,18 @@ enum punit_power_well {
>>>>>>>>>>
>>>>>>>>>>     #define _VLV_HDMIW_HDMIEDID_A        (VLV_DISPLAY_BASE + 0x62050)
>>>>>>>>>>     #define _VLV_HDMIW_HDMIEDID_B        (VLV_DISPLAY_BASE + 0x62150)
>>>>>>>>>> -#define VLV_HDMIW_HDMIEDID(pipe) _PIPE(pipe, \
>>>>>>>>>> +#define _CHV_HDMIW_HDMIEDID_C        (VLV_DISPLAY_BASE + 0x62250)
>>>>>>>>>> +#define VLV_HDMIW_HDMIEDID(pipe) _PIPE3(pipe, \
>>>>>>>>>>                         _VLV_HDMIW_HDMIEDID_A, \
>>>>>>>>>> -                    _VLV_HDMIW_HDMIEDID_B)
>>>>>>>>>> +                    _VLV_HDMIW_HDMIEDID_B, \
>>>>>>>>>> +                    _CHV_HDMIW_HDMIEDID_C)
>>>>>>>>>>     #define _VLV_AUD_CNTL_ST_A        (VLV_DISPLAY_BASE + 0x620B4)
>>>>>>>>>>     #define _VLV_AUD_CNTL_ST_B        (VLV_DISPLAY_BASE + 0x621B4)
>>>>>>>>>> -#define VLV_AUD_CNTL_ST(pipe) _PIPE(pipe, \
>>>>>>>>>> +#define _CHV_AUD_CNTL_ST_C        (VLV_DISPLAY_BASE + 0x622B4)
>>>>>>>>>> +#define VLV_AUD_CNTL_ST(pipe) _PIPE3(pipe, \
>>>>>>>>>>                         _VLV_AUD_CNTL_ST_A, \
>>>>>>>>>> -                    _VLV_AUD_CNTL_ST_B)
>>>>>>>>>> +                    _VLV_AUD_CNTL_ST_B, \
>>>>>>>>>> +                    _CHV_AUD_CNTL_ST_C)
>>>>>>>>>>     #define VLV_AUD_CNTL_ST2        (VLV_DISPLAY_BASE + 0x620C0)
>>>>>>>>>>
>>>>>>>>>>     /* These are the 4 32-bit write offset registers for each stream
>>>>>>>>>> @@ -6217,9 +6221,11 @@ enum punit_power_well {
>>>>>>>>>>                         _CPT_AUD_CONFIG_B)
>>>>>>>>>>     #define _VLV_AUD_CONFIG_A        (VLV_DISPLAY_BASE + 0x62000)
>>>>>>>>>>     #define _VLV_AUD_CONFIG_B        (VLV_DISPLAY_BASE + 0x62100)
>>>>>>>>>> -#define VLV_AUD_CFG(pipe) _PIPE(pipe, \
>>>>>>>>>> +#define _CHV_AUD_CONFIG_C        (VLV_DISPLAY_BASE + 0x62200)
>>>>>>>>>> +#define VLV_AUD_CFG(pipe) _PIPE3(pipe, \
>>>>>>>>>>                         _VLV_AUD_CONFIG_A, \
>>>>>>>>>> -                    _VLV_AUD_CONFIG_B)
>>>>>>>>>> +                    _VLV_AUD_CONFIG_B, \
>>>>>>>>>> +                    _CHV_AUD_CONFIG_C)
>>>>>>>>>>
>>>>>>>>>>     #define   AUD_CONFIG_N_VALUE_INDEX        (1 << 29)
>>>>>>>>>>     #define   AUD_CONFIG_N_PROG_ENABLE        (1 << 28)
>>>>>>>>>> --
>>>>>>>>>> 1.7.9.5
>>>>>>>>>>
>>>>>>>>>> _______________________________________________
>>>>>>>>>> Intel-gfx mailing list
>>>>>>>>>> Intel-gfx@lists.freedesktop.org
>>>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>>>>>>>
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> Intel-gfx mailing list
>>>>>>>> Intel-gfx@lists.freedesktop.org
>>>>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> Intel-gfx mailing list
>>>>>> Intel-gfx@lists.freedesktop.org
>>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>>>
>>>>
>>>
>>>
>>
>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx