From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v2] drm/i915: Perform link quality check unconditionally during long pulse Date: Tue, 14 Mar 2017 12:15:10 +0200 Message-ID: <20170314101510.GN31595@intel.com> References: <20170216153007.14868-1-ville.syrjala@linux.intel.com> <20170313205323.12339-1-ville.syrjala@linux.intel.com> <20170313230953.GA4943@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id 97D1A6E1A5 for ; Tue, 14 Mar 2017 10:15:14 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20170313230953.GA4943@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Manasi Navare Cc: intel-gfx@lists.freedesktop.org, Palmer Dabbelt , stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org T24gTW9uLCBNYXIgMTMsIDIwMTcgYXQgMDQ6MDk6NTNQTSAtMDcwMCwgTWFuYXNpIE5hdmFyZSB3 cm90ZToKPiBPbiBNb24sIE1hciAxMywgMjAxNyBhdCAxMDo1MzoyM1BNICswMjAwLCB2aWxsZS5z eXJqYWxhQGxpbnV4LmludGVsLmNvbSB3cm90ZToKPiA+IEZyb206IFZpbGxlIFN5cmrDpGzDpCA8 dmlsbGUuc3lyamFsYUBsaW51eC5pbnRlbC5jb20+Cj4gPiAKPiA+IEFwcGFyZW50bHkgc29tZSBE UCBzaW5rcyBhcmUgYSBsaXR0bGUgbnV0cyBhbmQgY2F1c2UgSFBEIHRvIGRyb3AKPiA+IGludGVy bWl0dGVudGx5IGR1cmluZyBtb2Rlc2V0cy4gVGhpcyBoYXBwZW5zIGVnLiBvbiBhbiBBU1VTIFBC Mjg3US4KPiA+IEluIG9kZXIgdG8gcmVjb3ZlciBmcm9tIHRoaXMgd2UgY2FuJ3QgcmVhbGx5IHVz ZSB0aGUgcHJldmlvdXMKPiA+IGNvbm5lY3RvciBzdGF0dXMgdG8gZGV0ZXJtaW5lIGlmIHRoZSBs aW5rIG5lZWRzIHJldHJhaW5pbmcsIHNvIGxldCdzCj4gPiBqdXN0IGlnbm9yZSB0aGF0IHBpZWNl IG9mIGluZm9ybWF0aW9uIGFuZCBkbyB0aGUgcmV0cmFpbgo+ID4gdW5jb25kaXRpb25hbGx5LiBX ZSBkbyBvZiBjb3Vyc2Ugc3RpbGwgY2hlY2sgd2hldGhlciB0aGUgbGluayBpcwo+ID4gc3VwcG9z ZWQgdG8gYmUgcnVubmluZyBvciBub3QuCj4gPiAKPiA+IFRvIGFjdHVhbGx5IGdldCByZWFkIG91 dCB0aGUgRURJRCBhbmQgdXBkYXRlIHRoaW5ncyBwcm9wZXJseSB3ZQo+ID4gYWxzbyBuZWVkIHRv IG51a2UgdGhlIGdvdG8gb3V0IGFkZGVkIGJ5IGNvbW1pdCA3ZDIzZTNjMzdiYjMKPiA+ICgiZHJt L2k5MTU6IENsZWFuaW5nIHVwIGludGVsX2RwX2hwZF9wdWxzZSIpLiBJJ20gYWN0dWFsbHkgbm90 IHN1cmUKPiA+IHdoeSB0aGF0IHdhcyB0aGVyZS4gUGVyaGFwcyB0byBhdm9pZCBhbiBFRElEIHJl YWQgaWYgdGhlIGNvbm5lY3Rvcgo+ID4gc3RhdHVzIGRpZG4ndCBhcHBlYXIgdG8gY2hhbmdlLCBi dXQgdGhhdCBzb3J0IG9mIHRoaW5nIGlzIHF1aXRlIHJhY3kKPiA+IGFuZCB3b3VsZCBoYXZlIGZh aWxlZCBhbnl3YXkgaWYgd2UgZmFpbGVkIHRvIGtlZXAgdXAgd2l0aCB0aGUKPiA+IGhvdHBsdWdz IChpZiB3ZSBtaXNzZWQgdGhlIEhQRCBkb3duIGluIGJldHdlZW4gdHdvIEhQRCB1cHMpLiBBbmQK PiA+IG5vdyB0aGF0IHdlIHRha2UgdGhpcyBjb2RlcGF0aCB1bmNvbmRpdGlvbmFsbHkgd2UgZGVm aW5pdGVseSBuZWVkCj4gPiB0byBkcm9wIHRoZSBnb3RvIGFzIG90aGVyd2lzZSB3ZSB3b3VsZCBu ZXZlciBkbyB0aGUgRURJRCByZWFkLgo+ID4gCj4gPiB2MjogRHJvcCB0aGUgZ290byB0aGF0IG1h ZGUgdXMgc2tpcCBFRElEIHJlYWRzIGVudGlyZWx5LiBEb2ghCj4gPiAKPiA+IENjOiBzdGFibGVA dmdlci5rZXJuZWwub3JnCj4gPiBDYzogTWFuYXNpIE5hdmFyZSA8bWFuYXNpLmQubmF2YXJlQGlu dGVsLmNvbT4KPiA+IENjOiBQYWxtZXIgRGFiYmVsdCA8cGFsbWVyQGRhYmJlbHQuY29tPgo+ID4g UmVwb3J0ZWQtYnk6IFBhbG1lciBEYWJiZWx0IDxwYWxtZXJAZGFiYmVsdC5jb20+Cj4gPiBCdWd6 aWxsYTogaHR0cHM6Ly9idWdzLmZyZWVkZXNrdG9wLm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTk3NjYK PiA+IFJlZmVyZW5jZXM6IGh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL2FyY2hpdmVzL2lu dGVsLWdmeC8yMDE3LUZlYnJ1YXJ5LzExOTc3OS5odG1sCj4gPiBTaWduZWQtb2ZmLWJ5OiBWaWxs ZSBTeXJqw6Rsw6QgPHZpbGxlLnN5cmphbGFAbGludXguaW50ZWwuY29tPgo+ID4gLS0tCj4gPiAg ZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfZHAuYyB8IDE2ICsrKysrKysrKysrLS0tLS0KPiA+ ICAxIGZpbGUgY2hhbmdlZCwgMTEgaW5zZXJ0aW9ucygrKSwgNSBkZWxldGlvbnMoLSkKPiA+IAo+ ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2RwLmMgYi9kcml2ZXJz L2dwdS9kcm0vaTkxNS9pbnRlbF9kcC5jCj4gPiBpbmRleCBmZDk2YTZjZjczMjYuLjVjMmYxYjM3 YjU4ZiAxMDA2NDQKPiA+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2RwLmMKPiA+ ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2RwLmMKPiA+IEBAIC00NjM0LDE2ICs0 NjM0LDIyIEBAIGludGVsX2RwX2xvbmdfcHVsc2Uoc3RydWN0IGludGVsX2Nvbm5lY3RvciAqaW50 ZWxfY29ubmVjdG9yKQo+ID4gIAkJICovCj4gPiAgCQlzdGF0dXMgPSBjb25uZWN0b3Jfc3RhdHVz X2Rpc2Nvbm5lY3RlZDsKPiA+ICAJCWdvdG8gb3V0Owo+ID4gLQl9IGVsc2UgaWYgKGNvbm5lY3Rv ci0+c3RhdHVzID09IGNvbm5lY3Rvcl9zdGF0dXNfY29ubmVjdGVkKSB7Cj4gPiArCX0gZWxzZSB7 Cj4gPiAgCQkvKgo+ID4gLQkJICogSWYgZGlzcGxheSB3YXMgY29ubmVjdGVkIGFscmVhZHkgYW5k IGlzIHN0aWxsIGNvbm5lY3RlZAo+ID4gLQkJICogY2hlY2sgbGlua3Mgc3RhdHVzLCB0aGVyZSBo YXMgYmVlbiBrbm93biBpc3N1ZXMgb2YKPiA+IC0JCSAqIGxpbmsgbG9zcyB0cmlnZ2VycmluZyBs b25nIHB1bHNlISEhIQo+ID4gKwkJICogSWYgZGlzcGxheSBpcyBub3cgY29ubmVjdGVkIGNoZWNr IGxpbmtzIHN0YXR1cywKPiA+ICsJCSAqIHRoZXJlIGhhcyBiZWVuIGtub3duIGlzc3VlcyBvZiBs aW5rIGxvc3MgdHJpZ2dlcnJpbmcKPiA+ICsJCSAqIGxvbmcgcHVsc2UuCj4gPiArCQkgKgo+ID4g KwkJICogU29tZSBzaW5rcyAoZWcuIEFTVVMgUEIyODdRKSBzZWVtIHRvIHBlcmZvcm0gc29tZQo+ ID4gKwkJICogd2VpcmQgSFBEIHBpbmcgcG9uZyBkdXJpbmcgbW9kZXNldHMuIFNvIHdlIGNhbiBh cHBhcmVseQo+ID4gKwkJICogZW5kIHVwIHdpdGggSFBEIGdvaW5nIGxvdyBkdXJpbmcgYSBtb2Rl c2V0LCBhbmQgdGhlbgo+ID4gKwkJICogZ29pbmcgYmFjayB1cCBzb29uIGFmdGVyLiBBbmQgb25j ZSB0aGF0IGhhcHBlbnMgd2UgbXVzdAo+ID4gKwkJICogcmV0cmFpbiB0aGUgbGluayB0byBnZXQg YSBwaWN0dXJlLiBUaGF0J3MgaW4gY2FzZSBubwo+ID4gKwkJICogdXNlcnNwYWNlIGNvbXBvbmVu dCByZWFjdGVkIHRvIGludGVybWl0dGVudCBIUEQgZGlwLgo+ID4gIAkJICovCj4gPiAgCQlkcm1f bW9kZXNldF9sb2NrKCZkZXYtPm1vZGVfY29uZmlnLmNvbm5lY3Rpb25fbXV0ZXgsIE5VTEwpOwo+ ID4gIAkJaW50ZWxfZHBfY2hlY2tfbGlua19zdGF0dXMoaW50ZWxfZHApOwo+ID4gIAkJZHJtX21v ZGVzZXRfdW5sb2NrKCZkZXYtPm1vZGVfY29uZmlnLmNvbm5lY3Rpb25fbXV0ZXgpOwo+ID4gLQkJ Z290byBvdXQ7Cj4gPiAgCX0KPiA+Cj4gCj4gWWVzIG1ha2VzIHNlbnNlIHRvIG1lIHRvbyBzaW5j ZSB3ZSBoYXZlIGEgY2hlY2sgZm9yIGNydGMtPmFjdGl2ZSBzbwo+IGl0IHdpbGwgcmV0cmFpbiBv bmx5IGlmIHRoZSBsaW5rIGlzIGFscmVhZHkgdXAgYW5kIHJ1bm5pbmcgd2l0aCBhbiBhY3RpdmUg Y3J0Ywo+IGFuZCBpbnRlbF9kcC0+bGFuZV9jb3VudCBpcyBub3QgMC4KPiBJbiBjYXNlIG9mIGxp bmsgZmFpbHVyZXMgdGhvdWdoIHdoZW4gd2Ugc2VuZCB0aGUgaG90cGx1ZyB1ZXZlbnQgaXQgd2ls bCBnZXQgdGhyb3VnaAo+IHRoaXMgcGF0aCBhbmQgaXQgd2lsbCB0cnkgdG8gcmV0cmFpbiB3aGlj aCB3ZSBkb250IHdhbnQgc2luY2UgdGhlIHJldHJhaW5pbmcKPiBzaG91bGQgaGFwcGVuIG9ubHkg aW4gdGhlIGF0b21pY19jb21taXQgYXQgdGhlIGZhbGxiYWNrIHJhdGUsIG5vdCBoZXJlLgo+IFNv IEkgZ3Vlc3Mgb24gbGluayBmYWlsdXJlLCBJIGNhbiBzZXQgaW50ZWxfZHAtPmxhbmVfY291bnQg dG8gMCB0byBpbnZhbGlkYXRlIHRoYXQKPiBhbmQgYXZvaWQgcmV0cmFpbmluZyBoZXJlLgoKSSdt IG5vdCBzdXJlIHdlIHdhbnQgdG8gZXhwYW5kIHRoZSBsaW5rX2NvdW50IGFidXNlIGhlcmUuIEl0 IHdhcyBvbmx5CmFkZGVkIGFzIGEgaGFjayBhbnl3YXkuIEJ1dCB0aGUgcmV0cmFpbmluZyBjb2Rl IHNob3VsZCBwZXJoYXBzIGNoZWNrCndoZXRoZXIgdGhlIGN1cnJlbnQgbGluayBwYXJhbWV0ZXJz IGFyZSBzdGlsbCBpbiB0aGUgc2V0IG9mCnZhbGlkL3VudGVzdGVkIHBhcmFtZXRlcnMgYW5kIHNr aXAgdGhlIHJldHJhaW5pbmcgaWYgbm90LgoKLS0gClZpbGxlIFN5cmrDpGzDpApJbnRlbCBPVEMK X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com ([192.55.52.88]:49628 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750749AbdCNKPP (ORCPT ); Tue, 14 Mar 2017 06:15:15 -0400 Date: Tue, 14 Mar 2017 12:15:10 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Manasi Navare Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org, Palmer Dabbelt Subject: Re: [PATCH v2] drm/i915: Perform link quality check unconditionally during long pulse Message-ID: <20170314101510.GN31595@intel.com> References: <20170216153007.14868-1-ville.syrjala@linux.intel.com> <20170313205323.12339-1-ville.syrjala@linux.intel.com> <20170313230953.GA4943@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170313230953.GA4943@intel.com> Sender: stable-owner@vger.kernel.org List-ID: On Mon, Mar 13, 2017 at 04:09:53PM -0700, Manasi Navare wrote: > On Mon, Mar 13, 2017 at 10:53:23PM +0200, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrj�l� > > > > Apparently some DP sinks are a little nuts and cause HPD to drop > > intermittently during modesets. This happens eg. on an ASUS PB287Q. > > In oder to recover from this we can't really use the previous > > connector status to determine if the link needs retraining, so let's > > just ignore that piece of information and do the retrain > > unconditionally. We do of course still check whether the link is > > supposed to be running or not. > > > > To actually get read out the EDID and update things properly we > > also need to nuke the goto out added by commit 7d23e3c37bb3 > > ("drm/i915: Cleaning up intel_dp_hpd_pulse"). I'm actually not sure > > why that was there. Perhaps to avoid an EDID read if the connector > > status didn't appear to change, but that sort of thing is quite racy > > and would have failed anyway if we failed to keep up with the > > hotplugs (if we missed the HPD down in between two HPD ups). And > > now that we take this codepath unconditionally we definitely need > > to drop the goto as otherwise we would never do the EDID read. > > > > v2: Drop the goto that made us skip EDID reads entirely. Doh! > > > > Cc: stable@vger.kernel.org > > Cc: Manasi Navare > > Cc: Palmer Dabbelt > > Reported-by: Palmer Dabbelt > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99766 > > References: https://lists.freedesktop.org/archives/intel-gfx/2017-February/119779.html > > Signed-off-by: Ville Syrj�l� > > --- > > drivers/gpu/drm/i915/intel_dp.c | 16 +++++++++++----- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index fd96a6cf7326..5c2f1b37b58f 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4634,16 +4634,22 @@ intel_dp_long_pulse(struct intel_connector *intel_connector) > > */ > > status = connector_status_disconnected; > > goto out; > > - } else if (connector->status == connector_status_connected) { > > + } else { > > /* > > - * If display was connected already and is still connected > > - * check links status, there has been known issues of > > - * link loss triggerring long pulse!!!! > > + * If display is now connected check links status, > > + * there has been known issues of link loss triggerring > > + * long pulse. > > + * > > + * Some sinks (eg. ASUS PB287Q) seem to perform some > > + * weird HPD ping pong during modesets. So we can apparely > > + * end up with HPD going low during a modeset, and then > > + * going back up soon after. And once that happens we must > > + * retrain the link to get a picture. That's in case no > > + * userspace component reacted to intermittent HPD dip. > > */ > > drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); > > intel_dp_check_link_status(intel_dp); > > drm_modeset_unlock(&dev->mode_config.connection_mutex); > > - goto out; > > } > > > > Yes makes sense to me too since we have a check for crtc->active so > it will retrain only if the link is already up and running with an active crtc > and intel_dp->lane_count is not 0. > In case of link failures though when we send the hotplug uevent it will get through > this path and it will try to retrain which we dont want since the retraining > should happen only in the atomic_commit at the fallback rate, not here. > So I guess on link failure, I can set intel_dp->lane_count to 0 to invalidate that > and avoid retraining here. I'm not sure we want to expand the link_count abuse here. It was only added as a hack anyway. But the retraining code should perhaps check whether the current link parameters are still in the set of valid/untested parameters and skip the retraining if not. -- Ville Syrj�l� Intel OTC