On Wed, Apr 04, 2018 at 03:00:12PM -0400, Lyude Paul wrote:
On Wed, 2018-04-04 at 21:53 +0300, Ville Syrjälä wrote:
On Wed, Apr 04, 2018 at 02:37:41PM -0400, Lyude Paul wrote:
On Wed, 2018-04-04 at 18:34 +0300, Ville Syrjälä wrote:
On Mon, Apr 02, 2018 at 05:26:16PM -0400, Lyude Paul wrote:
While enabling/disabling DPMS before link training with MST hubs is perfectly valid; unfortunately disabling DPMS results in some devices disabling their AUX CH block as well. For SST this isn't as much of a problem, but for MST we need to be able to continue handling aux transactions even when none of the sinks are turned on since it's possible for us to have a single atomic commit which results in disabling each downstream sink, followed by subsequently re-enabling each sink.
If we don't do this, we'll end up stalling any pending ESI interrupts from the sink for up to 1ms. Unfortunately, dropping ESIs during this timespan makes it so that link fallback retraining for MST (which I will be submitting to the ML shortly) fails due to the channel EQ failure interrupts potentially getting dropped. Additionally, when performing a modeset that brings the hub status's link status from bad -> good having ESIs disabled for that long causes us to miss the hub's response to us trying to start link training as well.
Since any sink with MST is going to support DisplayPort 1.2 anyway, save us the hassle of trying to wait until the sink comes back up and just never shut the aux block down.
Changes since v2:
- Fix patch name, no functional changes
Signed-off-by: Lyude Paul lyude@redhat.com Cc: Laura Abbott labbott@redhat.com Cc: Dhinakaran Pandiyan dhinakaran.pandiyan@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Cc: stable@vger.kernel.org Fixes: ad260ab32a4d9 ("drm/i915/dp: Write to SET_POWER dpcd to enable MST hub.")
drivers/gpu/drm/i915/intel_dp.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 62f82c4298ac..0479c377981b 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2589,11 +2589,13 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode) return; if (mode != DRM_MODE_DPMS_ON) {
unsigned char data = intel_dp->is_mst ?DP_SET_POWER_D3_AUX_ON : DP_SET_POWER_D3;This smells like a workaround for an actual bug somewhere. Why exactly is the slower wakeup or the AUX block a problem for MST but not for SST when the link training is exactly the same for SST and MST?
I actually thought about this but I still think this is the appropriate fix. So; the real reason for the wakeup not being a problem with SST is that for DPMS on with SST, we actually do a wait to make sure that the hub is ready before continuing. And yes: I'm fairly sure SST does actually have around the same wakeup time that MST does, but with the wait we do it doesn't reallhy make a difference. With MST, we could do this but there's a few reasons I don't think we should:
- We don't need to. D3_AUX_ON is a part of the 1.2 spec, so any hub that
has MST is going to be guaranteed to have this.
- Turning off the aux block means that there's a high chance we're going
to miss ESIs from sinks
And how exactly do we lose irqs? The hub/whatever throws the up req msgs away if we don't read them within some really short time?
Oh-additionally I did forget to mention that i have actually witnessed the channel eq failures in the ESI getting dropped without this patch.
Not sure what that means. I don't think there is any sideband messaging involved in link training so not sure what is dropped in this case. The link status/etc. are just polled directly by the upstream device.
Meaning if we miss them, there's a chance the hub may just not choose to send them again for whatever reason.
- It's faster to keep the aux block on anyway
- if (downstream_hpd_needs_d0(intel_dp)) return;
ret = drm_dp_dpcd_writeb(&intel_dp->aux,DP_SET_POWER,
DP_SET_POWER_D3);
ret = drm_dp_dpcd_writeb(&intel_dp->aux,DP_SET_POWER, data); } else { struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp); -- 2.14.3
-- Cheers, Lyude Paul
-- Cheers, Lyude Paul