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

[Sharma, Shashank]

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.
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;