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@xxxxxxxxx> > >>>Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> > >>>Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > >>>Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> > >>>Cc: <stable@xxxxxxxxxxxxxxx> # v4.9+ > >>>Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > >>>--- > >>> 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; > -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html