| Message ID | 20180314054825.1718-1-dhinakaran.pandiyan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 13, 2018 at 10:48:25PM -0700, Dhinakaran Pandiyan wrote: > If bios sets up an MST output and hardware state readout code sees this is > an SST configuration, when disabling the encoder we end up calling > ->post_disable_dp() hook instead of the MST version. Consequently, we write > to the DP_SET_POWER dpcd to set it D3 state. Further along when we try > enable the encoder in MST mode, POWER_UP_PHY transaction fails to power up > the MST hub. This results in continuous link training failures which keep > the system busy delaying boot. We could identify bios MST boot discrepancy > and handle it accordingly but a simple way to solve this is to write to the > DP_SET_POWER dpcd for MST too. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105470 > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index dbcf1a0586f9..8c2d778560f0 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2205,8 +2205,7 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > intel_prepare_dp_ddi_buffers(encoder, crtc_state); > > intel_ddi_init_dp_buf_reg(encoder); > - if (!is_mst) > - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); The spec is perhaps a bit unclear on this. "If the message is sent as a path request, all DP nodes from the source immediate downstream device and the targeted DP node will be placed in the D0 power state." Doesn't quite tell me whether the immediate downstream device is included in that list of nodes. However the spec goes on to say "Each nodes immediate upstream device will use Native AUX writes to the SET_POWER & SET_DP_PWR_VOLTAGE register (DPCD Address 00600h) to set the power state of the downstream node." and since we are the immediate upstream for that device I guess it makes sense that we should still do the DP_SET_POWER manually. But I have to wonder what the original issue was before we started to use POWER_UP/DOWN_PHY. I suppose somehow some further downstream device wasn't in D0 when we needed it. But that is a bit weird as I believe all devices should really start up in D0. Anyways based on my reading of the spec I can justify this so Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> I presume we want cc:stable + fixes: on this? > intel_dp_start_link_train(intel_dp); > if (port != PORT_A || INTEL_GEN(dev_priv) >= 9) > intel_dp_stop_link_train(intel_dp); > @@ -2304,14 +2303,12 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > struct intel_dp *intel_dp = &dig_port->dp; > - bool is_mst = intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST); > > /* > * Power down sink before disabling the port, otherwise we end > * up getting interrupts from the sink on detecting link loss. > */ > - if (!is_mst) > - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > > intel_disable_ddi_buf(encoder); > > -- > 2.14.1
On Wed, 2018-03-14 at 15:35 +0200, Ville Syrjälä wrote: > On Tue, Mar 13, 2018 at 10:48:25PM -0700, Dhinakaran Pandiyan wrote: > > If bios sets up an MST output and hardware state readout code sees this is > > an SST configuration, when disabling the encoder we end up calling > > ->post_disable_dp() hook instead of the MST version. Consequently, we write > > to the DP_SET_POWER dpcd to set it D3 state. Further along when we try > > enable the encoder in MST mode, POWER_UP_PHY transaction fails to power up > > the MST hub. This results in continuous link training failures which keep > > the system busy delaying boot. We could identify bios MST boot discrepancy > > and handle it accordingly but a simple way to solve this is to write to the > > DP_SET_POWER dpcd for MST too. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105470 > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> Reported-by: Laura Abbott <labbott@redhat.com> Cc: stable@vger.kernel.org Fixes: 5ea2355a100a ("drm/i915/mst: Use MST sideband message transactions for dpms control") > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 7 ++----- > > 1 file changed, 2 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index dbcf1a0586f9..8c2d778560f0 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -2205,8 +2205,7 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > intel_prepare_dp_ddi_buffers(encoder, crtc_state); > > > > intel_ddi_init_dp_buf_reg(encoder); > > - if (!is_mst) > > - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > > + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); > > The spec is perhaps a bit unclear on this. > > "If the message is sent as a path request, all DP nodes from the source > immediate downstream device and the targeted DP node will be placed in > the D0 power state." > > Doesn't quite tell me whether the immediate downstream device is > included in that list of nodes. > > However the spec goes on to say > "Each nodes immediate upstream device will use Native AUX writes to the > SET_POWER & SET_DP_PWR_VOLTAGE register (DPCD Address 00600h) to set > the power state of the downstream node." > > and since we are the immediate upstream for that device I guess it makes > sense that we should still do the DP_SET_POWER manually. > > But I have to wonder what the original issue was before we started to use > POWER_UP/DOWN_PHY. I suppose somehow some further downstream device > wasn't in D0 when we needed it. Correct, sinks farther downstream weren't lighting up. > But that is a bit weird as I believe all > devices should really start up in D0. > > Anyways based on my reading of the spec I can justify this so > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Thanks for the review. > I presume we want cc:stable + fixes: on this? Yeah, I suppose we should wait for the reporter to confirm that this indeed fixes the bug. It does fix the problem on the Thinkpad laptop + dock I tested it on. > > > intel_dp_start_link_train(intel_dp); > > if (port != PORT_A || INTEL_GEN(dev_priv) >= 9) > > intel_dp_stop_link_train(intel_dp); > > @@ -2304,14 +2303,12 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > > struct intel_dp *intel_dp = &dig_port->dp; > > - bool is_mst = intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST); > > > > /* > > * Power down sink before disabling the port, otherwise we end > > * up getting interrupts from the sink on detecting link loss. > > */ > > - if (!is_mst) > > - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > > + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); > > > > intel_disable_ddi_buf(encoder); > > > > -- > > 2.14.1 >
On 03/14/2018 11:26 AM, Pandiyan, Dhinakaran wrote: > > > > On Wed, 2018-03-14 at 15:35 +0200, Ville Syrjälä wrote: >> On Tue, Mar 13, 2018 at 10:48:25PM -0700, Dhinakaran Pandiyan wrote: >>> If bios sets up an MST output and hardware state readout code sees this is >>> an SST configuration, when disabling the encoder we end up calling >>> ->post_disable_dp() hook instead of the MST version. Consequently, we write >>> to the DP_SET_POWER dpcd to set it D3 state. Further along when we try >>> enable the encoder in MST mode, POWER_UP_PHY transaction fails to power up >>> the MST hub. This results in continuous link training failures which keep >>> the system busy delaying boot. We could identify bios MST boot discrepancy >>> and handle it accordingly but a simple way to solve this is to write to the >>> DP_SET_POWER dpcd for MST too. >>> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105470 >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >>> Cc: Jani Nikula <jani.nikula@intel.com> > > Reported-by: Laura Abbott <labbott@redhat.com> > Cc: stable@vger.kernel.org > Fixes: 5ea2355a100a ("drm/i915/mst: Use MST sideband message > transactions for dpms control") > Tested-by: Laura Abbott <labbott@redhat.com> >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_ddi.c | 7 ++----- >>> 1 file changed, 2 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >>> index dbcf1a0586f9..8c2d778560f0 100644 >>> --- a/drivers/gpu/drm/i915/intel_ddi.c >>> +++ b/drivers/gpu/drm/i915/intel_ddi.c >>> @@ -2205,8 +2205,7 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, >>> intel_prepare_dp_ddi_buffers(encoder, crtc_state); >>> >>> intel_ddi_init_dp_buf_reg(encoder); >>> - if (!is_mst) >>> - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); >>> + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); >> >> The spec is perhaps a bit unclear on this. >> >> "If the message is sent as a path request, all DP nodes from the source >> immediate downstream device and the targeted DP node will be placed in >> the D0 power state." >> >> Doesn't quite tell me whether the immediate downstream device is >> included in that list of nodes. >> >> However the spec goes on to say >> "Each nodes immediate upstream device will use Native AUX writes to the >> SET_POWER & SET_DP_PWR_VOLTAGE register (DPCD Address 00600h) to set >> the power state of the downstream node." >> >> and since we are the immediate upstream for that device I guess it makes >> sense that we should still do the DP_SET_POWER manually. >> >> But I have to wonder what the original issue was before we started to use >> POWER_UP/DOWN_PHY. I suppose somehow some further downstream device >> wasn't in D0 when we needed it. > > Correct, sinks farther downstream weren't lighting up. > >> But that is a bit weird as I believe all >> devices should really start up in D0. >> >> Anyways based on my reading of the spec I can justify this so >> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > > Thanks for the review. > >> I presume we want cc:stable + fixes: on this? > > Yeah, I suppose we should wait for the reporter to confirm that this > indeed fixes the bug. It does fix the problem on the Thinkpad laptop + > dock I tested it on. > > >> >>> intel_dp_start_link_train(intel_dp); >>> if (port != PORT_A || INTEL_GEN(dev_priv) >= 9) >>> intel_dp_stop_link_train(intel_dp); >>> @@ -2304,14 +2303,12 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, >>> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >>> struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); >>> struct intel_dp *intel_dp = &dig_port->dp; >>> - bool is_mst = intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST); >>> >>> /* >>> * Power down sink before disabling the port, otherwise we end >>> * up getting interrupts from the sink on detecting link loss. >>> */ >>> - if (!is_mst) >>> - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); >>> + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); >>> >>> intel_disable_ddi_buf(encoder); >>> >>> -- >>> 2.14.1 >>
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index dbcf1a0586f9..8c2d778560f0 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2205,8 +2205,7 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, intel_prepare_dp_ddi_buffers(encoder, crtc_state); intel_ddi_init_dp_buf_reg(encoder); - if (!is_mst) - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); intel_dp_start_link_train(intel_dp); if (port != PORT_A || INTEL_GEN(dev_priv) >= 9) intel_dp_stop_link_train(intel_dp); @@ -2304,14 +2303,12 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); struct intel_dp *intel_dp = &dig_port->dp; - bool is_mst = intel_crtc_has_type(old_crtc_state, INTEL_OUTPUT_DP_MST); /* * Power down sink before disabling the port, otherwise we end * up getting interrupts from the sink on detecting link loss. */ - if (!is_mst) - intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); + intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF); intel_disable_ddi_buf(encoder);
If bios sets up an MST output and hardware state readout code sees this is an SST configuration, when disabling the encoder we end up calling ->post_disable_dp() hook instead of the MST version. Consequently, we write to the DP_SET_POWER dpcd to set it D3 state. Further along when we try enable the encoder in MST mode, POWER_UP_PHY transaction fails to power up the MST hub. This results in continuous link training failures which keep the system busy delaying boot. We could identify bios MST boot discrepancy and handle it accordingly but a simple way to solve this is to write to the DP_SET_POWER dpcd for MST too. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105470 Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)