From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH v5] drm/i915: Fix DDC probe for passive adapters Date: Tue, 02 Jun 2015 19:43:08 +0300 Message-ID: <87oaky6oyr.fsf@intel.com> References: <20150602130909.GZ5176@intel.com> <1433262075-23775-1-git-send-email-jani.nikula@intel.com> <20150602164035.GC5176@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 ESMTP id 99B166E03A for ; Tue, 2 Jun 2015 09:44:54 -0700 (PDT) In-Reply-To: <20150602164035.GC5176@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Ville =?utf-8?B?U3lyasOkbMOk?= Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org T24gVHVlLCAwMiBKdW4gMjAxNSwgVmlsbGUgU3lyasOkbMOkIDx2aWxsZS5zeXJqYWxhQGxpbnV4 LmludGVsLmNvbT4gd3JvdGU6Cj4gT24gVHVlLCBKdW4gMDIsIDIwMTUgYXQgMDc6MjE6MTVQTSAr MDMwMCwgSmFuaSBOaWt1bGEgd3JvdGU6Cj4+IFBhc3NpdmUgRFAtPkRWSS9IRE1JIGRvbmdsZXMg b24gRFArKyBwb3J0cyBzaG93IHVwIHRvIHRoZSBzeXN0ZW0gYXMgSERNSQo+PiBkZXZpY2VzLCBh cyB0aGV5IGRvIG5vdCBoYXZlIGEgc2luayBkZXZpY2UgaW4gdGhlbSB0byByZXNwb25kIHRvIGFu eSBBVVgKPj4gdHJhZmZpYy4gV2hlbiBwcm9iaW5nIHRoZXNlIGRvbmdsZXMgb3ZlciB0aGUgRERD LCBzb21ldGltZXMgdGhleSB3aWxsCj4+IE5BSyB0aGUgZmlyc3QgYXR0ZW1wdCBldmVuIHRob3Vn aCB0aGUgdHJhbnNhY3Rpb24gaXMgdmFsaWQgYW5kIHRoZXkKPj4gc3VwcG9ydCB0aGUgRERDIHBy b3RvY29sLiBUaGUgcmV0cnkgbG9vcCBpbnNpZGUgb2YKPj4gZHJtX2RvX3Byb2JlX2RkY19lZGlk KCkgd291bGQgbm9ybWFsbHkgY2F0Y2ggdGhpcyBjYXNlIGFuZCB0cnkgdGhlCj4+IHRyYW5zYWN0 aW9uIGFnYWluLCByZXN1bHRpbmcgaW4gc3VjY2Vzcy4KPj4gCj4+IFRoYXQsIGhvd2V2ZXIsIHdh cyB0aHdhcnRlZCBieSB0aGUgZml4IGZvciBbMV06Cj4+IAo+PiBjb21taXQgOTI5MmYzN2UxZjVj Nzk0MDAyNTRkY2E0NmY4MzMxMzQ4ODA5MzgyNQo+PiBBdXRob3I6IEV1Z2VuaSBEb2Rvbm92IDxl dWdlbmkuZG9kb25vdkBpbnRlbC5jb20+Cj4+IERhdGU6ICAgVGh1IEphbiA1IDA5OjM0OjI4IDIw MTIgLTAyMDAKPj4gCj4+ICAgICBkcm06IGdpdmUgdXAgb24gZWRpZCByZXRyaWVzIHdoZW4gaTJj IGJ1cyBpcyBub3QgcmVzcG9uZGluZwo+PiAKPj4gVGhpcyBhZGRlZCBjb2RlIHRvIGV4aXQgaW1t ZWRpYXRlbHkgaWYgdGhlIHJldHVybiBjb2RlIGZyb20gdGhlCj4+IGkyY190cmFuc2ZlciBmdW5j dGlvbiB3YXMgLUVOWElPIGluIG9yZGVyIHRvIHJlZHVjZSB0aGUgYW1vdW50IG9mIHRpbWUKPj4g c3BlbnQgaW4gd2FpdGluZyBmb3IgdW5yZXNwb25zaXZlIG9yIGRpc2Nvbm5lY3RlZCBkZXZpY2Vz LiBUaGF0IHdhcwo+PiBwb3NzaWJsZSBiZWNhdXNlIHRoZSB1bmRlcmx5aW5nIGkyYyBiaXQgYmFu Z2luZyBhbGdvcml0aG0gaGFkIHJldHJpZXMgb2YKPj4gaXRzIG93biAod2hpY2gsIG9mIGNvdXJz ZSwgd2VyZSBwYXJ0IG9mIHRoZSByZWFzb24gZm9yIHRoZSBidWcgdGhlCj4+IGNvbW1pdCBmaXhl cykuCj4+IAo+PiBTaW5jZSBpdHMgaW50cm9kdWN0aW9uIGluCj4+IAo+PiBjb21taXQgZjg5OWZj NjRjZGE4NTY5ZDA1Mjk0NTJhYWZjMGRhMzFjMDQyZGYyZQo+PiBBdXRob3I6IENocmlzIFdpbHNv biA8Y2hyaXNAY2hyaXMtd2lsc29uLmNvLnVrPgo+PiBEYXRlOiAgIFR1ZSBKdWwgMjAgMTU6NDQ6 NDUgMjAxMCAtMDcwMAo+PiAKPj4gICAgIGRybS9pOTE1OiB1c2UgR01CVVMgdG8gbWFuYWdlIGky YyBsaW5rcwo+PiAKPj4gd2UndmUgYmVlbiBmbGlwcGluZyBiYWNrIGFuZCBmb3J0aCBlbmFibGlu ZyB0aGUgR01CVVMgdHJhbnNmZXJzLCBidXQKPj4gd2UndmUgc2V0dGxlZCBzaW5jZSB0aGVuLiBU aGUgR01CVVMgaW1wbGVtZW50YXRpb24gZG9lcyBub3QgZG8gYW55Cj4+IHJldHJpZXMsIGhvd2V2 ZXIsIGJhaWxpbmcgb3V0IG9mIHRoZSBkcm1fZG9fcHJvYmVfZGRjX2VkaWQoKSByZXRyeSBsb29w Cj4+IG9uIGZpcnN0IGVuY291bnRlciBvZiAtRU5YSU8uIFRoaXMsIGNvbWJpbmVkIHdpdGggRXVn ZW5pJ3MgY29tbWl0LCBicm9rZQo+PiB0aGUgcmV0cnkgb24gLUVOWElPLgo+PiAKPj4gUmV0cnkg R01CVVMgb25jZSBvbiAtRU5YSU8gb24gZmlyc3QgbWVzc2FnZSB0byBtaXRpZ2F0ZSB0aGUgaXNz dWVzIHdpdGgKPj4gcGFzc2l2ZSBhZGFwdGVycy4KPj4gCj4+IFRoaXMgcGF0Y2ggaXMgYmFzZWQg b24gdGhlIHdvcmssIGFuZCBjb21taXQgbWVzc2FnZSwgYnkgVG9kZCBQcmV2aXRlCj4+IDx0cHJl dml0ZUBnbWFpbC5jb20+Lgo+PiAKPj4gWzFdIGh0dHBzOi8vYnVncy5mcmVlZGVza3RvcC5vcmcv c2hvd19idWcuY2dpP2lkPTQxMDU5Cj4+IAo+PiB2MjogRG9uJ3QgcmV0cnkgaWYgdXNpbmcgYml0 IGJhbmdpbmcuCj4+IAo+PiB2MzogTW92ZSByZXRyeSB3aXRoaW4gZ21idXhfeGZlciwgcmV0cnkg b25seSBvbiBmaXJzdCBtZXNzYWdlLgo+PiAKPj4gdjQ6IEluaXRpYWxpemUgR01CVVMwIG9uIHJl dHJ5IChWaWxsZSkuCj4+IAo+PiB2NTogVGFrZSBpbmRleCByZWFkcyBpbnRvIGFjY291bnQgKFZp bGxlKS4KPj4gCj4+IEJ1Z3ppbGxhOiBodHRwczovL2J1Z3MuZnJlZWRlc2t0b3Aub3JnL3Nob3df YnVnLmNnaT9pZD04NTkyNAo+PiBDYzogVG9kZCBQcmV2aXRlIDx0cHJldml0ZUBnbWFpbC5jb20+ Cj4+IENjOiBzdGFibGVAdmdlci5rZXJuZWwub3JnCj4+IFNpZ25lZC1vZmYtYnk6IEphbmkgTmlr dWxhIDxqYW5pLm5pa3VsYUBpbnRlbC5jb20+Cj4KPiBPSywgSSB0aGluayBJJ20gZG9uZSBzaG9v dGluZyBhbnkgbW9yZSBob2xlcyBpbnRvIHRoaXMgb25lOgo+IFJldmlld2VkLWJ5OiBWaWxsZSBT eXJqw6Rsw6QgPHZpbGxlLnN5cmphbGFAbGludXguaW50ZWwuY29tPgoKXG8vCgpUaGFua3MgZm9y IGFsbCB0aGUgcmV2aWV3LiBJIHN1Y2suCgo+IEhvcGVmdWxseSBpdCB3b3JrcyB0b28uIEkgZG9u J3QgaGF2ZSB2ZXJ5IGdvb2QgaW50dWl0aW9uIGludG8gdGhlIGdtYnVzCj4gaGFyZHdhcmUuIEkg dGhpbmsgb25lIGRheSBJIG5lZWQgdG8gc2V0IHVwIGEgYml0LWJhbmdlZCBpMmMgc2xhdmUgYW5k Cj4gcGxheSBhcm91bmQgd2l0aCBpdCBqdXN0IGZvciBmdW4uCgpBRkFJQ1QgaXQgc2hvdWxkIGJl IGZ1bmN0aW9uYWxseSBlcXVpdmFsZW50IHRvIHRoZSB2MSBwYXRjaCB0aGF0IGdvdAp0ZXN0ZWQg aW4gdGhlIHJlZmVyZW5jZWQgYnVnLiBJJ20gaG9waW5nIHRvIGdldCBhIHRlc3QgcmVzdWx0IGZv ciB0aGlzCm9uZSB0b28uCgoKPgo+PiAtLS0KPj4gIGRyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVs X2kyYy5jIHwgMjAgKysrKysrKysrKysrKysrKystLS0KPj4gIDEgZmlsZSBjaGFuZ2VkLCAxNyBp bnNlcnRpb25zKCspLCAzIGRlbGV0aW9ucygtKQo+PiAKPj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMv Z3B1L2RybS9pOTE1L2ludGVsX2kyYy5jIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfaTJj LmMKPj4gaW5kZXggOTIwNzJmNTZlNDE4Li5hNjRmMjZjNjcwYWYgMTAwNjQ0Cj4+IC0tLSBhL2Ry aXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2kyYy5jCj4+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9p OTE1L2ludGVsX2kyYy5jCj4+IEBAIC00ODYsNyArNDg2LDcgQEAgZ21idXNfeGZlcihzdHJ1Y3Qg aTJjX2FkYXB0ZXIgKmFkYXB0ZXIsCj4+ICAJCQkJCSAgICAgICBzdHJ1Y3QgaW50ZWxfZ21idXMs Cj4+ICAJCQkJCSAgICAgICBhZGFwdGVyKTsKPj4gIAlzdHJ1Y3QgZHJtX2k5MTVfcHJpdmF0ZSAq ZGV2X3ByaXYgPSBidXMtPmRldl9wcml2Owo+PiAtCWludCBpLCByZWdfb2Zmc2V0Owo+PiArCWlu dCBpID0gMCwgaW5jLCB0cnkgPSAwLCByZWdfb2Zmc2V0Owo+PiAgCWludCByZXQgPSAwOwo+PiAg Cj4+ICAJaW50ZWxfYXV4X2Rpc3BsYXlfcnVudGltZV9nZXQoZGV2X3ByaXYpOwo+PiBAQCAtNDk5 LDEyICs0OTksMTQgQEAgZ21idXNfeGZlcihzdHJ1Y3QgaTJjX2FkYXB0ZXIgKmFkYXB0ZXIsCj4+ ICAKPj4gIAlyZWdfb2Zmc2V0ID0gZGV2X3ByaXYtPmdwaW9fbW1pb19iYXNlOwo+PiAgCj4+ICty ZXRyeToKPj4gIAlJOTE1X1dSSVRFKEdNQlVTMCArIHJlZ19vZmZzZXQsIGJ1cy0+cmVnMCk7Cj4+ ICAKPj4gLQlmb3IgKGkgPSAwOyBpIDwgbnVtOyBpKyspIHsKPj4gKwlmb3IgKDsgaSA8IG51bTsg aSArPSBpbmMpIHsKPj4gKwkJaW5jID0gMTsKPj4gIAkJaWYgKGdtYnVzX2lzX2luZGV4X3JlYWQo bXNncywgaSwgbnVtKSkgewo+PiAgCQkJcmV0ID0gZ21idXNfeGZlcl9pbmRleF9yZWFkKGRldl9w cml2LCAmbXNnc1tpXSk7Cj4+IC0JCQlpICs9IDE7ICAvKiBzZXQgaSB0byB0aGUgaW5kZXggb2Yg dGhlIHJlYWQgeGZlciAqLwo+PiArCQkJaW5jID0gMjsgLyogYW4gaW5kZXggcmVhZCBpcyB0d28g bXNncyAqLwo+PiAgCQl9IGVsc2UgaWYgKG1zZ3NbaV0uZmxhZ3MgJiBJMkNfTV9SRCkgewo+PiAg CQkJcmV0ID0gZ21idXNfeGZlcl9yZWFkKGRldl9wcml2LCAmbXNnc1tpXSwgMCk7Cj4+ICAJCX0g ZWxzZSB7Cj4+IEBAIC01NzYsNiArNTc4LDE4IEBAIGNsZWFyX2VycjoKPj4gIAkJCSBhZGFwdGVy LT5uYW1lLCBtc2dzW2ldLmFkZHIsCj4+ICAJCQkgKG1zZ3NbaV0uZmxhZ3MgJiBJMkNfTV9SRCkg PyAncicgOiAndycsIG1zZ3NbaV0ubGVuKTsKPj4gIAo+PiArCS8qCj4+ICsJICogUGFzc2l2ZSBh ZGFwdGVycyBzb21ldGltZXMgTkFLIHRoZSBmaXJzdCBwcm9iZS4gUmV0cnkgdGhlIGZpcnN0Cj4+ ICsJICogbWVzc2FnZSBvbmNlIG9uIC1FTlhJTyBmb3IgR01CVVMgdHJhbnNmZXJzOyB0aGUgYml0 IGJhbmdpbmcgYWxnb3JpdGhtCj4+ICsJICogaGFzIHJldHJpZXMgaW50ZXJuYWxseS4gU2VlIGFs c28gdGhlIHJldHJ5IGxvb3AgaW4KPj4gKwkgKiBkcm1fZG9fcHJvYmVfZGRjX2VkaWQsIHdoaWNo IGJhaWxzIG91dCBvbiB0aGUgZmlyc3QgLUVOWElPLgo+PiArCSAqLwo+PiArCWlmIChyZXQgPT0g LUVOWElPICYmIGkgPT0gMCAmJiB0cnkrKyA9PSAwKSB7Cj4+ICsJCURSTV9ERUJVR19LTVMoIkdN QlVTIFslc10gTkFLIG9uIGZpcnN0IG1lc3NhZ2UsIHJldHJ5XG4iLAo+PiArCQkJICAgICAgYWRh cHRlci0+bmFtZSk7Cj4+ICsJCWdvdG8gcmV0cnk7Cj4+ICsJfQo+PiArCj4+ICAJZ290byBvdXQ7 Cj4+ICAKPj4gIHRpbWVvdXQ6Cj4+IC0tIAo+PiAyLjEuNAo+Cj4gLS0gCj4gVmlsbGUgU3lyasOk bMOkCj4gSW50ZWwgT1RDCgotLSAKSmFuaSBOaWt1bGEsIEludGVsIE9wZW4gU291cmNlIFRlY2hu b2xvZ3kgQ2VudGVyCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fCkludGVsLWdmeCBtYWlsaW5nIGxpc3QKSW50ZWwtZ2Z4QGxpc3RzLmZyZWVkZXNrdG9wLm9y ZwpodHRwOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4 Cg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com ([192.55.52.88]:49129 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758949AbbFBQoz convert rfc822-to-8bit (ORCPT ); Tue, 2 Jun 2015 12:44:55 -0400 From: Jani Nikula To: Ville =?utf-8?B?U3lyasOkbMOk?= Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org Subject: Re: [PATCH v5] drm/i915: Fix DDC probe for passive adapters In-Reply-To: <20150602164035.GC5176@intel.com> References: <20150602130909.GZ5176@intel.com> <1433262075-23775-1-git-send-email-jani.nikula@intel.com> <20150602164035.GC5176@intel.com> Date: Tue, 02 Jun 2015 19:43:08 +0300 Message-ID: <87oaky6oyr.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: stable-owner@vger.kernel.org List-ID: On Tue, 02 Jun 2015, Ville Syrjälä wrote: > On Tue, Jun 02, 2015 at 07:21:15PM +0300, Jani Nikula wrote: >> Passive DP->DVI/HDMI dongles on DP++ ports show up to the system as HDMI >> devices, as they do not have a sink device in them to respond to any AUX >> traffic. When probing these dongles over the DDC, sometimes they will >> NAK the first attempt even though the transaction is valid and they >> support the DDC protocol. The retry loop inside of >> drm_do_probe_ddc_edid() would normally catch this case and try the >> transaction again, resulting in success. >> >> That, however, was thwarted by the fix for [1]: >> >> commit 9292f37e1f5c79400254dca46f83313488093825 >> Author: Eugeni Dodonov >> Date: Thu Jan 5 09:34:28 2012 -0200 >> >> drm: give up on edid retries when i2c bus is not responding >> >> This added code to exit immediately if the return code from the >> i2c_transfer function was -ENXIO in order to reduce the amount of time >> spent in waiting for unresponsive or disconnected devices. That was >> possible because the underlying i2c bit banging algorithm had retries of >> its own (which, of course, were part of the reason for the bug the >> commit fixes). >> >> Since its introduction in >> >> commit f899fc64cda8569d0529452aafc0da31c042df2e >> Author: Chris Wilson >> Date: Tue Jul 20 15:44:45 2010 -0700 >> >> drm/i915: use GMBUS to manage i2c links >> >> we've been flipping back and forth enabling the GMBUS transfers, but >> we've settled since then. The GMBUS implementation does not do any >> retries, however, bailing out of the drm_do_probe_ddc_edid() retry loop >> on first encounter of -ENXIO. This, combined with Eugeni's commit, broke >> the retry on -ENXIO. >> >> Retry GMBUS once on -ENXIO on first message to mitigate the issues with >> passive adapters. >> >> This patch is based on the work, and commit message, by Todd Previte >> . >> >> [1] https://bugs.freedesktop.org/show_bug.cgi?id=41059 >> >> v2: Don't retry if using bit banging. >> >> v3: Move retry within gmbux_xfer, retry only on first message. >> >> v4: Initialize GMBUS0 on retry (Ville). >> >> v5: Take index reads into account (Ville). >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85924 >> Cc: Todd Previte >> Cc: stable@vger.kernel.org >> Signed-off-by: Jani Nikula > > OK, I think I'm done shooting any more holes into this one: > Reviewed-by: Ville Syrjälä \o/ Thanks for all the review. I suck. > Hopefully it works too. I don't have very good intuition into the gmbus > hardware. I think one day I need to set up a bit-banged i2c slave and > play around with it just for fun. AFAICT it should be functionally equivalent to the v1 patch that got tested in the referenced bug. I'm hoping to get a test result for this one too. > >> --- >> drivers/gpu/drm/i915/intel_i2c.c | 20 +++++++++++++++++--- >> 1 file changed, 17 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c >> index 92072f56e418..a64f26c670af 100644 >> --- a/drivers/gpu/drm/i915/intel_i2c.c >> +++ b/drivers/gpu/drm/i915/intel_i2c.c >> @@ -486,7 +486,7 @@ gmbus_xfer(struct i2c_adapter *adapter, >> struct intel_gmbus, >> adapter); >> struct drm_i915_private *dev_priv = bus->dev_priv; >> - int i, reg_offset; >> + int i = 0, inc, try = 0, reg_offset; >> int ret = 0; >> >> intel_aux_display_runtime_get(dev_priv); >> @@ -499,12 +499,14 @@ gmbus_xfer(struct i2c_adapter *adapter, >> >> reg_offset = dev_priv->gpio_mmio_base; >> >> +retry: >> I915_WRITE(GMBUS0 + reg_offset, bus->reg0); >> >> - for (i = 0; i < num; i++) { >> + for (; i < num; i += inc) { >> + inc = 1; >> if (gmbus_is_index_read(msgs, i, num)) { >> ret = gmbus_xfer_index_read(dev_priv, &msgs[i]); >> - i += 1; /* set i to the index of the read xfer */ >> + inc = 2; /* an index read is two msgs */ >> } else if (msgs[i].flags & I2C_M_RD) { >> ret = gmbus_xfer_read(dev_priv, &msgs[i], 0); >> } else { >> @@ -576,6 +578,18 @@ clear_err: >> adapter->name, msgs[i].addr, >> (msgs[i].flags & I2C_M_RD) ? 'r' : 'w', msgs[i].len); >> >> + /* >> + * Passive adapters sometimes NAK the first probe. Retry the first >> + * message once on -ENXIO for GMBUS transfers; the bit banging algorithm >> + * has retries internally. See also the retry loop in >> + * drm_do_probe_ddc_edid, which bails out on the first -ENXIO. >> + */ >> + if (ret == -ENXIO && i == 0 && try++ == 0) { >> + DRM_DEBUG_KMS("GMBUS [%s] NAK on first message, retry\n", >> + adapter->name); >> + goto retry; >> + } >> + >> goto out; >> >> timeout: >> -- >> 2.1.4 > > -- > Ville Syrjälä > Intel OTC -- Jani Nikula, Intel Open Source Technology Center