Received: by 10.213.65.68 with SMTP id h4csp1110723imn; Wed, 4 Apr 2018 12:49:47 -0700 (PDT) X-Google-Smtp-Source: AIpwx49nmKs0hZNmw5IdsRf7INHXfbvKq5jgZgf9a2YMoettYWticL0fXXvgtmNrAfwZMJMyJl/e X-Received: by 10.99.119.195 with SMTP id s186mr13072236pgc.453.1522871387762; Wed, 04 Apr 2018 12:49:47 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1522871387; cv=none; d=google.com; s=arc-20160816; b=0jJD5WP046enKRvew52ALGdeLHEB8hIe5OGLo7adMAWn62z+uEcthdXdYllOLLUbbL sm4lKHggTqNuDheyl3Rg7tGWPdk090b6MVhk7DW+W9HzQSG0PQx6guo4weucr/7l+J2i g65+KFdZ8LIA1QtQkHi0lfKncP5Ek7tyJ/lhqvRJZkRpxk43v42cOVnLGtNd2X0G8Zxb AKZhXs3VxpWTFNEZUlOTZlJ0Fx4PQ2FHeVZPCzabvRiuib83HpAUrlL8DKm/sto2AKu8 5UGBoQHYd/zohFHIByQwa4B/kMvDYn/t6jDWZtCtfbg8gfsmaxFzyhMHrpC2324BhtpD 7QBA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :organization:references:in-reply-to:date:cc:to:from:subject :message-id:arc-authentication-results; bh=jXPkpnUrvUtn4He/WfyVisvAZrfGZ4a2F1887SGnfD8=; b=Zga+Wo5RjJVDWoZv56nNgxolzgYdrEazaGxP3xgQs4VnFd3COu6zJgMaWYR2/hul5i JBn2n2ynVhHFrQzew8AGG9qrxsBemU3XXeACa8maolOGDa1hhWqNwpkGyCmEC6kilUH3 kmOvAofogxgkYtBEadFdOoUZZxHKXkTZyYgxH+UNqVN/ANiiFJw5papTwHs2er5cXbIG e9Yaw80skcC1681y+g5NuPXR41YHxV3C/RqKLz8Ylm1ePiwBVL9qymYyUHaLcTNhXvSr gxM4bRvW82SrQ2Q1z2DxIoi8i/jLxStNwAAC7NAR7kuCQOFNoV5WTeolfx+y6JbaLF8e ZNhw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id p13-v6si3971884pll.416.2018.04.04.12.49.33; Wed, 04 Apr 2018 12:49:47 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751871AbeDDTqr (ORCPT + 99 others); Wed, 4 Apr 2018 15:46:47 -0400 Received: from mail-qk0-f193.google.com ([209.85.220.193]:43881 "EHLO mail-qk0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751535AbeDDTqp (ORCPT ); Wed, 4 Apr 2018 15:46:45 -0400 Received: by mail-qk0-f193.google.com with SMTP id v2so23805202qkh.10 for ; Wed, 04 Apr 2018 12:46:45 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:organization:mime-version:content-transfer-encoding; bh=jXPkpnUrvUtn4He/WfyVisvAZrfGZ4a2F1887SGnfD8=; b=KQyRgmuxA7F50L/qGQ+zVlsja1TFqs5CCUaG233egRPn2prRRROpN6w7dTUe2zZEEk s3HH5MMCyEqXIRro4J10ymMN4/HYvjTbb7gXrMBeISOdMURoWK2is5XCFK1dHSN+zHdS jZ7y32rt7P1loOqA4Lr7ZwXmws8ecOej83XDFbscDDmdeDr92fL5kERAqMLOa3RKwy7k 6730uuTT7mhLguuCU5x3SJ8RitYcLQ0rL7+Czn0ytVIQT8DSctspF/TMHVntmsyRbIwP Hh3+BPBsCHAm3EqHEkFufCXaZyJ5dSkRazNqtCcIn2rvTeTySPTuDqyNHsZN/azOUYJ7 CT0w== X-Gm-Message-State: ALQs6tAoFH9nYm0y3+oR3mcRcgZj6lmA4y1CljYgyk6YGasaaSHDYF6X YYt7fIYomupK3HUCmRLH3CHLHw== X-Received: by 10.55.31.17 with SMTP id f17mr24936034qkf.266.1522871204839; Wed, 04 Apr 2018 12:46:44 -0700 (PDT) Received: from dhcp-10-20-1-55.bss.redhat.com ([144.121.20.162]) by smtp.gmail.com with ESMTPSA id b201sm4448227qkg.48.2018.04.04.12.46.44 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 04 Apr 2018 12:46:44 -0700 (PDT) Message-ID: <1522871203.3935.10.camel@redhat.com> Subject: Re: [PATCH v2] drm/i915: Keep AUX block running when disabling DPMS for MST From: Lyude Paul To: Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= Cc: intel-gfx@lists.freedesktop.org, Laura Abbott , Dhinakaran Pandiyan , stable@vger.kernel.org, Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , David Airlie , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Date: Wed, 04 Apr 2018 15:46:43 -0400 In-Reply-To: <20180404193126.GH5453@intel.com> References: <20180402212142.19841-1-lyude@redhat.com> <20180402212617.21247-1-lyude@redhat.com> <20180404153429.GE5453@intel.com> <1522867061.12403.6.camel@redhat.com> <20180404185313.GG5453@intel.com> <1522868349.12403.12.camel@redhat.com> <20180404193126.GH5453@intel.com> Organization: Red Hat Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.26.6 (3.26.6-1.fc27) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2018-04-04 at 22:31 +0300, Ville Syrjälä wrote: > On Wed, Apr 04, 2018 at 02:59:09PM -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 > > > > > > Cc: Laura Abbott > > > > > > Cc: Dhinakaran Pandiyan > > > > > > Cc: Ville Syrjälä > > > > > > 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? > > > > That's my hypothesis at least. I'm betting that on the fact that when I > > was > > implementing MST retraining before we put the intel_dp_check_mst_status() > > (or > > whatever it's called) into the dig workqueue, getting the sink to go down > > and > > come back up was a lot more unreliable whenever I introduced anything that > > would block the esi handler for longer then a very brief period of time > > (say, > > 50-100ms?). I've seen some notes elsewhere too that seemed to imply for > > SST, > > things were pretty sensitive to irq latency (line 1050, intel_dp.c) so it > > wouldn't be terribly surprising if it's the same for MST. At the very > > least, > > now that we've got the ESI handler running in the dig worker things seem > > to > > have gotten a /lot/ more reliable now that we can basically go the whole > > modeset without blocking the ESI handler for very long. > > Hmm. OK, so the spec seems to be saying that we have 100ms to read > the UP_REQ/DOWN_REPLY msg after the IRQ_HPD. That's still a lot more > than the 1ms max allowed wakeup time. Looks like there's a extended > wakeup time request/grant mechanism now, but without the explicit > grant (which we don't do) the 1ms still holds. mm, that is true. There were definitely interrupts getting dropped though with this patch, although it was rather rare. The other thing too (and this was a lot easier to reproduce) was that when we do a modeset that lowers the link rate: * check modeset * commit disables so we can reprogram the vcpi (puts sink into D3) * commit enables * actually send request to put sink into D0 * Go to start link training... * *TIMEOUT on response from hub* * continue modeset... * commit finishes * short period of time after the commit finishes, we receive the response from the hub where we tried to start link training This being said: I think it may actually be a good idea for us to consider waiting for the hub anyway when we're turning it on like we do with SST, since we could be waking it up from a D3 state we didn't set ourselves anyway, but either way I think since we can at least benefit from not having to deal with the wakeup time at all in modesets like this (and it's not really problematic to other hubs), we probably should do that as well with or without a busy wait. > > > > > > > > * 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 > > -- Cheers, Lyude Paul