[Intel-gfx] [PATCH 2/4] drm/i915/lspcon: Fix resume time initialization due to unasserted HPD

[Sharma, Shashank]

Reviewed-by: Shashank Sharma

Regards
Shashank
On 1/29/2017 9:43 PM, Imre Deak wrote:
> On Sun, Jan 29, 2017 at 10:33:07AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 1/28/2017 1:47 PM, Imre Deak wrote:
>>> On Sat, Jan 28, 2017 at 10:32:03AM +0530, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>>
>>>> On 1/27/2017 3:09 PM, Imre Deak wrote:
>>>>> During system resume time initialization the HPD level on LSPCON ports
>>>>> can stay low for an extended amount of time, leading to failed AUX
>>>>> transfers and LSPCON initialization. Fix this by waiting for HPD to get
>>>>> asserted.
>>>>>
>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99178
>>>>> Cc: Shashank Sharma <shashank.sharma at intel.com>
>>>>> Cc: Jani Nikula <jani.nikula at linux.intel.com>
>>>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>>>>> Cc: <stable at vger.kernel.org> # v4.9+
>>>>> Signed-off-by: Imre Deak <imre.deak at intel.com>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/intel_dp.c     | 4 ++--
>>>>>   drivers/gpu/drm/i915/intel_drv.h    | 2 ++
>>>>>   drivers/gpu/drm/i915/intel_lspcon.c | 5 ++++-
>>>>>   3 files changed, 8 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>> index e0f9b9e..a7785ce 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>> @@ -4400,8 +4400,8 @@ static bool bxt_digital_port_connected(struct drm_i915_private *dev_priv,
>>>>>    *
>>>>>    * Return %true if @port is connected, %false otherwise.
>>>>>    */
>>>>> -static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
>>>>> -					 struct intel_digital_port *port)
>>>>> +bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
>>>>> +				  struct intel_digital_port *port)
>>>>>   {
>>>>>   	if (HAS_PCH_IBX(dev_priv))
>>>>>   		return ibx_digital_port_connected(dev_priv, port);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>>> index b71fece..b9cde11 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>>> @@ -1489,6 +1489,8 @@ bool __intel_dp_read_desc(struct intel_dp *intel_dp,
>>>>>   bool intel_dp_read_desc(struct intel_dp *intel_dp);
>>>>>   int intel_dp_link_required(int pixel_clock, int bpp);
>>>>>   int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
>>>>> +bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
>>>>> +				  struct intel_digital_port *port);
>>>>>   /* intel_dp_aux_backlight.c */
>>>>>   int intel_dp_aux_init_backlight_funcs(struct intel_connector *intel_connector);
>>>>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
>>>>> index f6d4e69..c300647 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>>>>> @@ -158,6 +158,8 @@ static bool lspcon_probe(struct intel_lspcon *lspcon)
>>>>>   static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
>>>>>   {
>>>>>   	struct intel_dp *intel_dp = lspcon_to_intel_dp(lspcon);
>>>>> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>>>>> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>>>>>   	unsigned long start = jiffies;
>>>>>   	if (!lspcon->desc_valid)
>>>>> @@ -173,7 +175,8 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
>>>>>   		if (!__intel_dp_read_desc(intel_dp, &desc))
>>>>>   			return;
>>>>> -		if (!memcmp(&intel_dp->desc, &desc, sizeof(desc))) {
>>>>> +		if (intel_digital_port_connected(dev_priv, dig_port) &&
>>>> when in PCON mode, live_status is always up for LSPCON port, so this check
>>>> will be always true, isn't it ?
>>> Not at least in case of the MegaChips LSPCON I've seen, there it takes a
>>> while for HPD to get asserted. And while it's not asserted AUX
>>> transactions will either:
>>> - return garbage in case of native AUX transactions
>>> - hang the chip/FW until next cold reboot in case of I2C-over-AUX
>>>    transactions
>>> - simply not get a reply if the chip/FW initialization has reached a
>>>    certain phase
>>>
>>> Based on the DP specification the sink/branch device is not required to
>>> respond in case the HPD is not asserted, so the 3rd scenario is
>>> compliant, but the first two are not.
>>>
>>> In PCON mode after initialization and HPD getting asserted, HPD will
>>> stay asserted except for the short pulses signaling an output
>>> connect/disconnect.
>> The reason might be, that LSPCON resumes in LS type2 adapter state, where
>> the live_status behavior is normal
>> and reflects the real HPD line, but the moment we move to PCON mode, HPD
>> gets asserted low.
> No, this chip/firmware resumes in PCON mode not in LS mode. Otherwise
> native AUX transfers wouldn't return any reply (per LSPCON
> specification) and HPD wouldn't get asserted on its own without a
> display connected. Notice that we only check the LS/PCON state and
> change it to PCON mode if necessary only after HPD gets asserted.
>
> --Imre
>
>> I know you would have tested this well, but I also want to test this code
>> and logic first, before we go ahead with the patch.
>>
>> Shashank
>>> --Imre
>>>
>>>> - Shashank
>>>>> +		    !memcmp(&intel_dp->desc, &desc, sizeof(desc))) {
>>>>>   			DRM_DEBUG_KMS("LSPCON recovering in PCON mode after %u ms\n",
>>>>>   				      jiffies_to_msecs(jiffies - start));
>>>>>   			return;