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

[Imre Deak <imre.deak@intel.com>]

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@intel.com>
> >>>Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >>>Cc: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >>>Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>>Cc: <stable@vger.kernel.org> # v4.9+
> >>>Signed-off-by: Imre Deak <imre.deak@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;
>