From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Fix hpd handling for pins with two encoders Date: Fri, 9 Nov 2018 17:57:43 +0200 Message-ID: <20181109155743.GM9144@intel.com> References: <20181108200424.28371-1-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1D59B6E0C7 for ; Fri, 9 Nov 2018 15:57:47 +0000 (UTC) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Lyude Paul Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org, Rodrigo Vivi List-Id: intel-gfx@lists.freedesktop.org T24gVGh1LCBOb3YgMDgsIDIwMTggYXQgMDY6MzU6MTBQTSAtMDUwMCwgTHl1ZGUgUGF1bCB3cm90 ZToKPiBsZ3RtCj4gCj4gUmV2aWV3ZWQtYnk6IEx5dWRlIFBhdWwgPGx5dWRlQHJlZGhhdC5jb20+ CgpUaGFua3MuIFB1c2hlZCB0byBkaW5xLgoKPiAKPiBPbiBUaHUsIDIwMTgtMTEtMDggYXQgMjI6 MDQgKzAyMDAsIFZpbGxlIFN5cmphbGEgd3JvdGU6Cj4gPiBGcm9tOiBWaWxsZSBTeXJqw6Rsw6Qg PHZpbGxlLnN5cmphbGFAbGludXguaW50ZWwuY29tPgo+ID4gCj4gPiBJbiBteSBoYXN0ZSB0byBy ZW1vdmUgaXJxX3BvcnRbXSBJIGFjY2lkZW50YWxseSBjaGFuZ2VkIHRoZQo+ID4gd2F5IHdlIGRl YWwgd2l0aCBocGQgcGlucyB0aGF0IGFyZSBzaGFyZWQgYnkgbXVsdGlwbGUgZW5jb2RlcnMKPiA+ IChEUCBhbmQgSERNSSBmb3IgcHJlLURESSBwbGF0Zm9ybXMpLiBQcmV2aW91c2x5IHdlIHdvdWxk IG9ubHkKPiA+IGhhbmRsZSBzdWNoIHBpbnMgdmlhIC0+aHBkX3B1bHNlKCksIGJ1dCBub3cgd2Ug cXVldWUgdXAgdGhlCj4gPiBob3RwbHVnIHdvcmsgZm9yIHRoZSBIRE1JIGVuY29kZXIgZGlyZWN0 bHkuIFdvcnNlIHlldCwgd2Ugbm93Cj4gPiBjb3VudCBlYWNoIGhwZCB0d2ljZSBhbmQgdGhpcyBp bmNyZW1lbnQgdGhlIGhwZCBzdG9ybSBjb3VudAo+ID4gdHdpY2UgYXMgZmFzdC4gVGhpcyBjYW4g bGVhZCB0byBzcHVyaW91cyBzdG9ybXMgYmVpbmcgZGV0ZWN0ZWQuCj4gPiAKPiA+IEdvIGJhY2sg dG8gdGhlIG9sZCB3YXkgb2YgZG9pbmcgdGhpbmdzLCBpZS4gZGVsZWdhdGUgdG8KPiA+IC0+aHBk X3B1bHNlKCkgZm9yIGFueSBwaW4gd2hpY2ggaGFzIGFuIGVuY29kZXIgd2l0aCB0aGF0IGhvb2sK PiA+IGltcGxlbWVudGVkLiBJIGRvbid0IHJlYWxseSBsaWtlIHRoZSBpZGVhIG9mIGFkZGluZyBp cnFfcG9ydFtdCj4gPiBiYWNrIHNvIGxldCdzIGxvb3AgdGhyb3VnaCB0aGUgZW5jb2RlcnMgZmly c3QgdG8gY2hlY2sgaWYgd2UKPiA+IGhhdmUgYW4gZW5jb2RlciB3aXRoIC0+aHBkX3B1bHNlKCkg Zm9yIHRoZSBwaW4sIGFuZCB0aGVuIGdvCj4gPiB0aHJvdWdoIGFsbCB0aGUgcGlucyBhbmQgZGVj aWRlZCBvbiB0aGUgY29ycmVjdCBjb3Vyc2Ugb2YgYWN0aW9uCj4gPiBiYXNlZCBvbiB0aGUgZWFy bGllciBmaW5kaW5ncy4KPiA+IAo+ID4gSSBoYXZlIG9jY2FzaW9uYWxseSB0b3llZCB3aXRoIHRo ZSBpZGVhIG9mIHVuaWZ5aW5nIHRoZSBwcmUtRERJCj4gPiBIRE1JIGFuZCBEUCBlbmNvZGVycyBp bnRvIGEgc2luZ2xlIGVuY29kZXIgYXMgd2VsbC4gQmVzaWRlcyB0aGUKPiA+IGhvdHBsdWcgcHJv Y2Vzc2luZyBpdCB3b3VsZCBoYXZlIHRoZSBvdGhlciBiZW5lZml0IG9mIHByZXZlbnRpbmcKPiA+ IHVzZXJzcGFjZSBmcm9tIHRyeWluZyB0byBlbmFibGUgYm90aCBlbmNvZGVycyBhdCB0aGUgc2Ft ZSB0aW1lLgo+ID4gVGhhdCBpcyBzaW1wbHkgaWxsZWdhbCBhcyB0aGV5IHNoYXJlIHRoZSBzYW1l IGNsb2NrL2RhdGEgcGlucy4KPiA+IFdlIGhhdmUgc29tZSB0ZXN0Y2FzZXMgdGhhdCB3aWxsIGF0 dGVtcHQgdGhhdCBhbmQgdGh1cyBmYWlsIG9uCj4gPiBtYW55IG9sZGVyIG1hY2hpbmVzLiBCdXQg Zm9yIG5vdyBsZXQncyBzdGljayB0byBmaXhpbmcganVzdCB0aGUKPiA+IGhvdHBsdWcgY29kZS4K PiA+IAo+ID4gQ2M6IHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmcgIyA0LjE5Kwo+ID4gQ2M6IEx5dWRl IFBhdWwgPGx5dWRlQHJlZGhhdC5jb20+Cj4gPiBDYzogUm9kcmlnbyBWaXZpIDxyb2RyaWdvLnZp dmlAaW50ZWwuY29tPgo+ID4gRml4ZXM6IGI2Y2EzZWVlMThiYSAoImRybS9pOTE1OiBOdWtlIGRl dl9wcml2LT5pcnFfcG9ydFtdIikKPiA+IFNpZ25lZC1vZmYtYnk6IFZpbGxlIFN5cmrDpGzDpCA8 dmlsbGUuc3lyamFsYUBsaW51eC5pbnRlbC5jb20+Cj4gPiAtLS0KPiA+ICBkcml2ZXJzL2dwdS9k cm0vaTkxNS9pbnRlbF9ob3RwbHVnLmMgfCA1NSArKysrKysrKysrKysrKysrKysrKystLS0tLS0t Cj4gPiAgMSBmaWxlIGNoYW5nZWQsIDQyIGluc2VydGlvbnMoKyksIDEzIGRlbGV0aW9ucygtKQo+ ID4gCj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfaG90cGx1Zy5j Cj4gPiBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2hvdHBsdWcuYwo+ID4gaW5kZXggNDJl NjFlMTBmNTE3Li5lMjQxNzRkMDhmZWQgMTAwNjQ0Cj4gPiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0v aTkxNS9pbnRlbF9ob3RwbHVnLmMKPiA+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVs X2hvdHBsdWcuYwo+ID4gQEAgLTQxNCwzMyArNDE0LDU0IEBAIHZvaWQgaW50ZWxfaHBkX2lycV9o YW5kbGVyKHN0cnVjdCBkcm1faTkxNV9wcml2YXRlCj4gPiAqZGV2X3ByaXYsCj4gPiAgCXN0cnVj dCBpbnRlbF9lbmNvZGVyICplbmNvZGVyOwo+ID4gIAlib29sIHN0b3JtX2RldGVjdGVkID0gZmFs c2U7Cj4gPiAgCWJvb2wgcXVldWVfZGlnID0gZmFsc2UsIHF1ZXVlX2hwID0gZmFsc2U7Cj4gPiAr CXUzMiBsb25nX2hwZF9wdWxzZV9tYXNrID0gMDsKPiA+ICsJdTMyIHNob3J0X2hwZF9wdWxzZV9t YXNrID0gMDsKPiA+ICsJZW51bSBocGRfcGluIHBpbjsKPiA+ICAKPiA+ICAJaWYgKCFwaW5fbWFz aykKPiA+ICAJCXJldHVybjsKPiA+ICAKPiA+ICAJc3Bpbl9sb2NrKCZkZXZfcHJpdi0+aXJxX2xv Y2spOwo+ID4gKwo+ID4gKwkvKgo+ID4gKwkgKiBEZXRlcm1pbmUgd2hldGhlciAtPmhwZF9wdWxz ZSgpIGV4aXN0cyBmb3IgZWFjaCBwaW4sIGFuZAo+ID4gKwkgKiB3aGV0aGVyIHdlIGhhdmUgYSBz aG9ydCBvciBhIGxvbmcgcHVsc2UuIFRoaXMgaXMgbmVlZGVkCj4gPiArCSAqIGFzIGVhY2ggcGlu IG1heSBoYXZlIHVwIHRvIHR3byBlbmNvZGVycyAoSERNSSBhbmQgRFApIGFuZAo+ID4gKwkgKiBv bmx5IHRoZSBvbmUgb2YgdGhlbSAoRFApIHdpbGwgaGF2ZSAtPmhwZF9wdWxzZSgpLgo+ID4gKwkg Ki8KPiA+ICAJZm9yX2VhY2hfaW50ZWxfZW5jb2RlcigmZGV2X3ByaXYtPmRybSwgZW5jb2Rlcikg ewo+ID4gLQkJZW51bSBocGRfcGluIHBpbiA9IGVuY29kZXItPmhwZF9waW47Cj4gPiAgCQlib29s IGhhc19ocGRfcHVsc2UgPSBpbnRlbF9lbmNvZGVyX2hhc19ocGRfcHVsc2UoZW5jb2Rlcik7Cj4g PiAtCQlib29sIGxvbmdfaHBkID0gdHJ1ZTsKPiA+ICsJCWVudW0gcG9ydCBwb3J0ID0gZW5jb2Rl ci0+cG9ydDsKPiA+ICsJCWJvb2wgbG9uZ19ocGQ7Cj4gPiAgCj4gPiArCQlwaW4gPSBlbmNvZGVy LT5ocGRfcGluOwo+ID4gIAkJaWYgKCEoQklUKHBpbikgJiBwaW5fbWFzaykpCj4gPiAgCQkJY29u dGludWU7Cj4gPiAgCj4gPiAtCQlpZiAoaGFzX2hwZF9wdWxzZSkgewo+ID4gLQkJCWVudW0gcG9y dCBwb3J0ID0gZW5jb2Rlci0+cG9ydDsKPiA+ICsJCWlmICghaGFzX2hwZF9wdWxzZSkKPiA+ICsJ CQljb250aW51ZTsKPiA+ICAKPiA+IC0JCQlsb25nX2hwZCA9IGxvbmdfbWFzayAmIEJJVChwaW4p Owo+ID4gKwkJbG9uZ19ocGQgPSBsb25nX21hc2sgJiBCSVQocGluKTsKPiA+ICAKPiA+IC0JCQlE Uk1fREVCVUdfRFJJVkVSKCJkaWdpdGFsIGhwZCBwb3J0ICVjIC0gJXNcbiIsCj4gPiBwb3J0X25h bWUocG9ydCksCj4gPiAtCQkJCQkgbG9uZ19ocGQgPyAibG9uZyIgOiAic2hvcnQiKTsKPiA+IC0J CQlxdWV1ZV9kaWcgPSB0cnVlOwo+ID4gLQkJCWlmIChsb25nX2hwZCkKPiA+IC0JCQkJZGV2X3By aXYtPmhvdHBsdWcubG9uZ19wb3J0X21hc2sgfD0gKDEgPDwKPiA+IHBvcnQpOwo+ID4gLQkJCWVs c2UKPiA+IC0JCQkJZGV2X3ByaXYtPmhvdHBsdWcuc2hvcnRfcG9ydF9tYXNrIHw9ICgxIDw8Cj4g PiBwb3J0KTsKPiA+ICsJCURSTV9ERUJVR19EUklWRVIoImRpZ2l0YWwgaHBkIHBvcnQgJWMgLSAl c1xuIiwKPiA+IHBvcnRfbmFtZShwb3J0KSwKPiA+ICsJCQkJIGxvbmdfaHBkID8gImxvbmciIDog InNob3J0Iik7Cj4gPiArCQlxdWV1ZV9kaWcgPSB0cnVlOwo+ID4gIAo+ID4gKwkJaWYgKGxvbmdf aHBkKSB7Cj4gPiArCQkJbG9uZ19ocGRfcHVsc2VfbWFzayB8PSBCSVQocGluKTsKPiA+ICsJCQlk ZXZfcHJpdi0+aG90cGx1Zy5sb25nX3BvcnRfbWFzayB8PSBCSVQocG9ydCk7Cj4gPiArCQl9IGVs c2Ugewo+ID4gKwkJCXNob3J0X2hwZF9wdWxzZV9tYXNrIHw9IEJJVChwaW4pOwo+ID4gKwkJCWRl dl9wcml2LT5ob3RwbHVnLnNob3J0X3BvcnRfbWFzayB8PSBCSVQocG9ydCk7Cj4gPiAgCQl9Cj4g PiArCX0KPiA+ICsKPiA+ICsJLyogTm93IHByb2Nlc3MgZWFjaCBwaW4ganVzdCBvbmNlICovCj4g PiArCWZvcl9lYWNoX2hwZF9waW4ocGluKSB7Cj4gPiArCQlib29sIGxvbmdfaHBkOwo+ID4gKwo+ ID4gKwkJaWYgKCEoQklUKHBpbikgJiBwaW5fbWFzaykpCj4gPiArCQkJY29udGludWU7Cj4gPiAg Cj4gPiAgCQlpZiAoZGV2X3ByaXYtPmhvdHBsdWcuc3RhdHNbcGluXS5zdGF0ZSA9PSBIUERfRElT QUJMRUQpIHsKPiA+ICAJCQkvKgo+ID4gQEAgLTQ1Nyw4ICs0NzgsMTYgQEAgdm9pZCBpbnRlbF9o cGRfaXJxX2hhbmRsZXIoc3RydWN0IGRybV9pOTE1X3ByaXZhdGUKPiA+ICpkZXZfcHJpdiwKPiA+ ICAJCWlmIChkZXZfcHJpdi0+aG90cGx1Zy5zdGF0c1twaW5dLnN0YXRlICE9IEhQRF9FTkFCTEVE KQo+ID4gIAkJCWNvbnRpbnVlOwo+ID4gIAo+ID4gLQkJaWYgKCFoYXNfaHBkX3B1bHNlKSB7Cj4g PiArCQkvKgo+ID4gKwkJICogRGVsZWdhdGUgdG8gLT5ocGRfcHVsc2UoKSBpZiBvbmUgb2YgdGhl IGVuY29kZXJzIGZvciB0aGlzCj4gPiArCQkgKiBwaW4gaGFzIGl0LCBvdGhlcndpc2UgbGV0IHRo ZSBob3RwbHVnX3dvcmsgZGVhbCB3aXRoIHRoaXMKPiA+ICsJCSAqIHBpbiBkaXJlY3RseS4KPiA+ ICsJCSAqLwo+ID4gKwkJaWYgKCgoc2hvcnRfaHBkX3B1bHNlX21hc2sgfCBsb25nX2hwZF9wdWxz ZV9tYXNrKSAmIEJJVChwaW4pKSkKPiA+IHsKPiA+ICsJCQlsb25nX2hwZCA9IGxvbmdfaHBkX3B1 bHNlX21hc2sgJiBCSVQocGluKTsKPiA+ICsJCX0gZWxzZSB7Cj4gPiAgCQkJZGV2X3ByaXYtPmhv dHBsdWcuZXZlbnRfYml0cyB8PSBCSVQocGluKTsKPiA+ICsJCQlsb25nX2hwZCA9IHRydWU7Cj4g PiAgCQkJcXVldWVfaHAgPSB0cnVlOwo+ID4gIAkJfQo+ID4gIAo+IC0tIAo+IENoZWVycywKPiAJ THl1ZGUgUGF1bAoKLS0gClZpbGxlIFN5cmrDpGzDpApJbnRlbApfX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fXwpJbnRlbC1nZnggbWFpbGluZyBsaXN0CkludGVs LWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcv bWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com ([192.55.52.88]:25533 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727784AbeKJBi4 (ORCPT ); Fri, 9 Nov 2018 20:38:56 -0500 Date: Fri, 9 Nov 2018 17:57:43 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Lyude Paul Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org, Rodrigo Vivi Subject: Re: [PATCH] drm/i915: Fix hpd handling for pins with two encoders Message-ID: <20181109155743.GM9144@intel.com> References: <20181108200424.28371-1-ville.syrjala@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Sender: stable-owner@vger.kernel.org List-ID: On Thu, Nov 08, 2018 at 06:35:10PM -0500, Lyude Paul wrote: > lgtm > > Reviewed-by: Lyude Paul Thanks. Pushed to dinq. > > On Thu, 2018-11-08 at 22:04 +0200, Ville Syrjala wrote: > > From: Ville Syrj�l� > > > > In my haste to remove irq_port[] I accidentally changed the > > way we deal with hpd pins that are shared by multiple encoders > > (DP and HDMI for pre-DDI platforms). Previously we would only > > handle such pins via ->hpd_pulse(), but now we queue up the > > hotplug work for the HDMI encoder directly. Worse yet, we now > > count each hpd twice and this increment the hpd storm count > > twice as fast. This can lead to spurious storms being detected. > > > > Go back to the old way of doing things, ie. delegate to > > ->hpd_pulse() for any pin which has an encoder with that hook > > implemented. I don't really like the idea of adding irq_port[] > > back so let's loop through the encoders first to check if we > > have an encoder with ->hpd_pulse() for the pin, and then go > > through all the pins and decided on the correct course of action > > based on the earlier findings. > > > > I have occasionally toyed with the idea of unifying the pre-DDI > > HDMI and DP encoders into a single encoder as well. Besides the > > hotplug processing it would have the other benefit of preventing > > userspace from trying to enable both encoders at the same time. > > That is simply illegal as they share the same clock/data pins. > > We have some testcases that will attempt that and thus fail on > > many older machines. But for now let's stick to fixing just the > > hotplug code. > > > > Cc: stable@vger.kernel.org # 4.19+ > > Cc: Lyude Paul > > Cc: Rodrigo Vivi > > Fixes: b6ca3eee18ba ("drm/i915: Nuke dev_priv->irq_port[]") > > Signed-off-by: Ville Syrj�l� > > --- > > drivers/gpu/drm/i915/intel_hotplug.c | 55 +++++++++++++++++++++------- > > 1 file changed, 42 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c > > b/drivers/gpu/drm/i915/intel_hotplug.c > > index 42e61e10f517..e24174d08fed 100644 > > --- a/drivers/gpu/drm/i915/intel_hotplug.c > > +++ b/drivers/gpu/drm/i915/intel_hotplug.c > > @@ -414,33 +414,54 @@ void intel_hpd_irq_handler(struct drm_i915_private > > *dev_priv, > > struct intel_encoder *encoder; > > bool storm_detected = false; > > bool queue_dig = false, queue_hp = false; > > + u32 long_hpd_pulse_mask = 0; > > + u32 short_hpd_pulse_mask = 0; > > + enum hpd_pin pin; > > > > if (!pin_mask) > > return; > > > > spin_lock(&dev_priv->irq_lock); > > + > > + /* > > + * Determine whether ->hpd_pulse() exists for each pin, and > > + * whether we have a short or a long pulse. This is needed > > + * as each pin may have up to two encoders (HDMI and DP) and > > + * only the one of them (DP) will have ->hpd_pulse(). > > + */ > > for_each_intel_encoder(&dev_priv->drm, encoder) { > > - enum hpd_pin pin = encoder->hpd_pin; > > bool has_hpd_pulse = intel_encoder_has_hpd_pulse(encoder); > > - bool long_hpd = true; > > + enum port port = encoder->port; > > + bool long_hpd; > > > > + pin = encoder->hpd_pin; > > if (!(BIT(pin) & pin_mask)) > > continue; > > > > - if (has_hpd_pulse) { > > - enum port port = encoder->port; > > + if (!has_hpd_pulse) > > + continue; > > > > - long_hpd = long_mask & BIT(pin); > > + long_hpd = long_mask & BIT(pin); > > > > - DRM_DEBUG_DRIVER("digital hpd port %c - %s\n", > > port_name(port), > > - long_hpd ? "long" : "short"); > > - queue_dig = true; > > - if (long_hpd) > > - dev_priv->hotplug.long_port_mask |= (1 << > > port); > > - else > > - dev_priv->hotplug.short_port_mask |= (1 << > > port); > > + DRM_DEBUG_DRIVER("digital hpd port %c - %s\n", > > port_name(port), > > + long_hpd ? "long" : "short"); > > + queue_dig = true; > > > > + if (long_hpd) { > > + long_hpd_pulse_mask |= BIT(pin); > > + dev_priv->hotplug.long_port_mask |= BIT(port); > > + } else { > > + short_hpd_pulse_mask |= BIT(pin); > > + dev_priv->hotplug.short_port_mask |= BIT(port); > > } > > + } > > + > > + /* Now process each pin just once */ > > + for_each_hpd_pin(pin) { > > + bool long_hpd; > > + > > + if (!(BIT(pin) & pin_mask)) > > + continue; > > > > if (dev_priv->hotplug.stats[pin].state == HPD_DISABLED) { > > /* > > @@ -457,8 +478,16 @@ void intel_hpd_irq_handler(struct drm_i915_private > > *dev_priv, > > if (dev_priv->hotplug.stats[pin].state != HPD_ENABLED) > > continue; > > > > - if (!has_hpd_pulse) { > > + /* > > + * Delegate to ->hpd_pulse() if one of the encoders for this > > + * pin has it, otherwise let the hotplug_work deal with this > > + * pin directly. > > + */ > > + if (((short_hpd_pulse_mask | long_hpd_pulse_mask) & BIT(pin))) > > { > > + long_hpd = long_hpd_pulse_mask & BIT(pin); > > + } else { > > dev_priv->hotplug.event_bits |= BIT(pin); > > + long_hpd = true; > > queue_hp = true; > > } > > > -- > Cheers, > Lyude Paul -- Ville Syrj�l� Intel