From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paul Kocialkowski Subject: Re: [PATCH] drm/i915: Synchronize connectors states when switching from poll to irq Date: Thu, 20 Jul 2017 17:35:13 +0300 Message-ID: <1500561313.1362.8.camel@linux.intel.com> References: <20170626123229.27939-1-paul.kocialkowski@linux.intel.com> <1500513858.21694.13.camel@dk-H97M-D3H> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1500513858.21694.13.camel@dk-H97M-D3H> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: "Pandiyan, Dhinakaran" Cc: "airlied@linux.ie" , "Vetter, Daniel" , "intel-gfx@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "dri-devel@lists.freedesktop.org" List-Id: dri-devel@lists.freedesktop.org T24gVGh1LCAyMDE3LTA3LTIwIGF0IDAxOjA0ICswMDAwLCBQYW5kaXlhbiwgRGhpbmFrYXJhbiB3 cm90ZToKPiBPbiBNb24sIDIwMTctMDYtMjYgYXQgMTU6MzIgKzAzMDAsIFBhdWwgS29jaWFsa293 c2tpIHdyb3RlOgo+ID4gQWZ0ZXIgZGV0ZWN0aW5nIGFuIElSUSBzdG9ybSwgaG90cGx1ZyBkZXRl Y3Rpb24gd2lsbCBzd2l0Y2ggZnJvbQo+ID4gaXJxLWJhc2VkIGRldGVjdGlvbiB0byBwb2xsLWJh c2VkIGRldGVjdGlvbi4gQWZ0ZXIgYSBzaG9ydCBkZWxheSBvcgo+ID4gd2hlbiByZXNldHRpbmcg c3Rvcm0gZGV0ZWN0aW9uIGZyb20gZGVidWdmcywgZGV0ZWN0aW9uIHdpbGwgc3dpdGNoCj4gPiBi YWNrIHRvIGJlaW5nIGlycS1iYXNlZC4KPiA+IAo+ID4gSG93ZXZlciwgaXQgbWF5IG9jY3VyIHRo YXQgcG9sbGluZyBkb2VzIG5vdCBoYXZlIGVub3VnaCB0aW1lIHRvCj4gPiBkZXRlY3QKPiA+IHRo ZSBjdXJyZW50IGNvbm5lY3RvciBzdGF0ZSB3aGVuIHRoYXQgc2Vjb25kIHN3aXRjaCB0YWtlcyBw bGFjZS4gCj4gCj4gU29tZSBxdWVzdGlvbnMgc28gdGhhdCBJIHVuZGVyc3RhbmQgdGhpcyBiZXR0 ZXIuIEhvdyBzaG9ydCB3b3VsZCB0aGlzCj4gaGF2ZSB0byBiZSBmb3IgZGV0ZWN0IHRvIG5vdCBj b21wbGV0ZT8gSXMgdGhlcmUgYSB3YXkgSSBjYW4gZWFzaWx5Cj4gdGVzdCB0aGlzIHNjZW5hcmlv PwoKV2VsbCwgaXQgZG9lc24ndCByZWFsbHkgbWF0dGVyLCB0aGUgcG9pbnQgaXMgdGhhdCBpdCBt YXkgaGFwcGVuIHRoYXQgdGhlCmNvbm5lY3RvcidzIGhwZCBsaW5lIGNoYW5nZXMgaW4gdGhlIHRp bWUgd2luZG93IGJldHdlZW4gdGhlIGxhc3QgcG9sbAoodGhhdCBoYXBwZW5zIGV2ZXJ5IDEwMG1z KSBhbmQgdGhlIG1vbWVudCB0aGUgaXJxIGlzIGVuYWJsZWQgYmFjay4gVGhpcwp0aW1lIHdpbmRv dywgaG93ZXZlciBsb25nIGl0IG1heSBiZSwgYWx3YXlzIGV4aXN0cyBhbmQgaXQgbWF5IGhhcHBl bgp0aGF0IHRoaXMgaXMgZXhhY3RseSB3aGVuIHRoZSBocGQgZXZlbnQgb2NjdXJzLgoKSXQncyBw b3NzaWJsZSB0byB0ZXN0IHRoaXMgYnkgdHJpZ2dlcmluZyBhbiBocGQgc3Rvcm0sIHRoZW4gdHJp Z2dlcmluZyBhCnJlZ3VsYXIgaHBkIHRvZ2dsZSBhbmQgZGlyZWN0bHkgZGlzYWJsaW5nIHBvbGxp bmcgbW9kZSB2aWEgZGVidWdmcy4KSSB3YXMgYWJsZSB0byBkbyB0aGlzIHdpdGggYSBjaGFtZWxp dW0gYW5kIGRpZCBzZWUgdGhlIHByb2JsZW0gdGFrZQpwbGFjZS4KCj4gPiBUaHVzLAo+ID4gdGhp cyBzZXRzIHRoZSBhcHByb3ByaWF0ZSBob3RwbHVnIGV2ZW50IGJpdHMgZm9yIHRoZSBjb25jZXJu ZWQKPiA+IGNvbm5lY3RvcnMgYW5kIHNjaGVkdWxlcyB0aGUgaG90cGx1ZyB3b3JrLCB0aGF0IHdp bGwgZW5zdXJlIHRoZQo+ID4gY29ubmVjdG9ycyBzdGF0ZXMgYXJlIGluIHN5bmMgd2hlbiBzd2l0 Y2hpbmcgYmFjayB0byBpcnEuCj4gCj4gTm90IHN1cmUgSSB1bmRlcnN0YW5kIHRoaXMgY29ycmVj dGx5LCBsb29rcyBsaWtlIHlvdSBhcmUgc2V0dGluZyB0aGUKPiBldmVudF9iaXRzIGV2ZW4gaWYg dGhlcmUgd2FzIG5vIGlycSBkdXJpbmcgdGhlIHBvbGxpbmcgaW50ZXJ2YWwuIElzCj4gdGhhdCBy aWdodD8KClllcywgeW91IGFyZSBwZXJmZWN0bHkgcmlnaHQuIGl0IGlzIG5lY2Vzc2FyeSB0byBk byB0aGlzIGJlY2F1c2UgdGhlCmV2ZW50IHdpbGwgbm90IGJlIGRldGVjdGVkIGVpdGhlciBieSBw b2xsaW5nIChoYXBwZW5pbmcgdG9vIGxhdGUpIG5vciBieQp0aGUgaXJxIChoYXBwZW5pbmcgdG9v IGVhcmx5KS4gU28gd2UgbXVzdCB0cmlnZ2VyIHRoZSBob3RwbHVnIHdvcmsgdG8KbWFrZSBpdCBj aGVjayB3aGV0aGVyIHRoZSBjb25uZWN0b3JzIHN0YXRlcyBjaGFuZ2VkLgoKPiA+IFdpdGhvdXQg dGhpcywgbm8gaXJxIHdpbGwgYmUgdHJpZ2dlcmVkIGFuZCB0aGUgaHBkIGNoYW5nZSB3aWxsIGJl Cj4gPiBsb3N0Lgo+ID4gCj4gPiBTaWduZWQtb2ZmLWJ5OiBQYXVsIEtvY2lhbGtvd3NraSA8cGF1 bC5rb2NpYWxrb3dza2lAbGludXguaW50ZWwuY29tPgo+ID4gLS0tCj4gPiAgZHJpdmVycy9ncHUv ZHJtL2k5MTUvaW50ZWxfaG90cGx1Zy5jIHwgOCArKysrKysrLQo+ID4gIDEgZmlsZSBjaGFuZ2Vk LCA3IGluc2VydGlvbnMoKyksIDEgZGVsZXRpb24oLSkKPiA+IAo+ID4gZGlmZiAtLWdpdCBhL2Ry aXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2hvdHBsdWcuYwo+ID4gYi9kcml2ZXJzL2dwdS9kcm0v aTkxNS9pbnRlbF9ob3RwbHVnLmMKPiA+IGluZGV4IGYxMjAwMjcyYTY5OS4uMjlmNTU0ODBiMGJi IDEwMDY0NAo+ID4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfaG90cGx1Zy5jCj4g PiArKysgYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9ob3RwbHVnLmMKPiA+IEBAIC0yMTgs OSArMjE4LDEzIEBAIHN0YXRpYyB2b2lkCj4gPiBpbnRlbF9ocGRfaXJxX3N0b3JtX3JlZW5hYmxl X3dvcmsoc3RydWN0IHdvcmtfc3RydWN0ICp3b3JrKQo+ID4gIAkJCXN0cnVjdCBpbnRlbF9jb25u ZWN0b3IgKmludGVsX2Nvbm5lY3RvciA9Cj4gPiB0b19pbnRlbF9jb25uZWN0b3IoY29ubmVjdG9y KTsKPiA+ICAKPiA+ICAJCQlpZiAoaW50ZWxfY29ubmVjdG9yLT5lbmNvZGVyLT5ocGRfcGluID09 IGkpCj4gPiB7Cj4gPiAtCQkJCWlmIChjb25uZWN0b3ItPnBvbGxlZCAhPQo+ID4gaW50ZWxfY29u bmVjdG9yLT5wb2xsZWQpCj4gPiArCQkJCWlmIChjb25uZWN0b3ItPnBvbGxlZCAhPQo+ID4gaW50 ZWxfY29ubmVjdG9yLT5wb2xsZWQpIHsKPiA+ICAJCQkJCURSTV9ERUJVR19EUklWRVIoIlJlZW5h Ymxpbgo+ID4gZyBIUEQgb24gY29ubmVjdG9yICVzXG4iLAo+ID4gIAkJCQkJCQkgY29ubmVjdG9y LQo+ID4gPm5hbWUpOwo+ID4gKwo+ID4gKwkJCQkJZGV2X3ByaXYtCj4gPiA+aG90cGx1Zy5ldmVu dF9iaXRzIHw9ICgxIDw8IGkpOwo+ID4gKwkJCQl9Cj4gPiArCj4gPiAgCQkJCWNvbm5lY3Rvci0+ cG9sbGVkID0KPiA+IGludGVsX2Nvbm5lY3Rvci0+cG9sbGVkOwo+ID4gIAkJCQlpZiAoIWNvbm5l Y3Rvci0+cG9sbGVkKQo+ID4gIAkJCQkJY29ubmVjdG9yLT5wb2xsZWQgPQo+ID4gRFJNX0NPTk5F Q1RPUl9QT0xMX0hQRDsKPiA+IEBAIC0yMzIsNiArMjM2LDggQEAgc3RhdGljIHZvaWQKPiA+IGlu dGVsX2hwZF9pcnFfc3Rvcm1fcmVlbmFibGVfd29yayhzdHJ1Y3Qgd29ya19zdHJ1Y3QgKndvcmsp Cj4gPiAgCQlkZXZfcHJpdi0+ZGlzcGxheS5ocGRfaXJxX3NldHVwKGRldl9wcml2KTsKPiA+ICAJ c3Bpbl91bmxvY2tfaXJxKCZkZXZfcHJpdi0+aXJxX2xvY2spOwo+ID4gIAo+ID4gKwlzY2hlZHVs ZV93b3JrKCZkZXZfcHJpdi0+aG90cGx1Zy5ob3RwbHVnX3dvcmspOwo+IAo+IEhvdyBkb2VzIHRo aXMgd29yayB3aXRoIERQIGNvbm5lY3RvcnM/IEZyb20gd2hhdCBJIHVuZGVyc3RhbmQsIHRoZQo+ IGV2ZW50X2JpdHMgYXJlIHNldCBmb3IgRFAgYmFzZWQgb24gdGhlIHJldHVybiB2YWx1ZSBmcm9t Cj4gaW50ZWxfZHBfaHBkX3B1bHNlKCkuIEJ1dCwgZG9lc24ndCB0aGlzIHBhdGNoIHNldCB0aGUg Yml0cwo+IHVuY29uZGl0aW9uYWxseT8KCkl0IHdvcmtzIHRoZSBzYW1lIGFzIG90aGVyIGNvbm5l Y3RvcnMsIG5vdGhpbmcgc3BlY2lmaWMgdGhlcmUuIFRoZQpob3RwbHVnIHdvcmsgd2lsbCByZWFk IHRoZSBjb25uZWN0b3IncyBzdGF0ZSBhbmQgaXNzdWUgYSB1ZXZlbnQgaWYgaXRzCnZhbHVlIGhh cyBjaGFuZ2VkLgoKPiA+ICsKPiA+ICAJaW50ZWxfcnVudGltZV9wbV9wdXQoZGV2X3ByaXYpOwo+ ID4gIH0KPiA+ICAKLS0gClBhdWwgS29jaWFsa293c2tpIDxwYXVsLmtvY2lhbGtvd3NraUBsaW51 eC5pbnRlbC5jb20+CkludGVsIEZpbmxhbmQgT3kgLSBCSUMgMDM1NzYwNi00IC0gV2VzdGVuZGlu a2F0dSA3LCAwMjE2MCBFc3BvbywgRmlubGFuZApfX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fXwpJbnRlbC1nZnggbWFpbGluZyBsaXN0CkludGVsLWdmeEBsaXN0 cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9s aXN0aW5mby9pbnRlbC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754784AbdGTOff (ORCPT ); Thu, 20 Jul 2017 10:35:35 -0400 Received: from mga01.intel.com ([192.55.52.88]:40889 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752822AbdGTOfd (ORCPT ); Thu, 20 Jul 2017 10:35:33 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,384,1496127600"; d="scan'208";a="1174759821" Message-ID: <1500561313.1362.8.camel@linux.intel.com> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Synchronize connectors states when switching from poll to irq From: Paul Kocialkowski To: "Pandiyan, Dhinakaran" Cc: "dri-devel@lists.freedesktop.org" , "intel-gfx@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , "Vetter, Daniel" , "airlied@linux.ie" Date: Thu, 20 Jul 2017 17:35:13 +0300 In-Reply-To: <1500513858.21694.13.camel@dk-H97M-D3H> References: <20170626123229.27939-1-paul.kocialkowski@linux.intel.com> <1500513858.21694.13.camel@dk-H97M-D3H> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.24.3 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2017-07-20 at 01:04 +0000, Pandiyan, Dhinakaran wrote: > On Mon, 2017-06-26 at 15:32 +0300, Paul Kocialkowski wrote: > > After detecting an IRQ storm, hotplug detection will switch from > > irq-based detection to poll-based detection. After a short delay or > > when resetting storm detection from debugfs, detection will switch > > back to being irq-based. > > > > However, it may occur that polling does not have enough time to > > detect > > the current connector state when that second switch takes place. > > Some questions so that I understand this better. How short would this > have to be for detect to not complete? Is there a way I can easily > test this scenario? Well, it doesn't really matter, the point is that it may happen that the connector's hpd line changes in the time window between the last poll (that happens every 100ms) and the moment the irq is enabled back. This time window, however long it may be, always exists and it may happen that this is exactly when the hpd event occurs. It's possible to test this by triggering an hpd storm, then triggering a regular hpd toggle and directly disabling polling mode via debugfs. I was able to do this with a chamelium and did see the problem take place. > > Thus, > > this sets the appropriate hotplug event bits for the concerned > > connectors and schedules the hotplug work, that will ensure the > > connectors states are in sync when switching back to irq. > > Not sure I understand this correctly, looks like you are setting the > event_bits even if there was no irq during the polling interval. Is > that right? Yes, you are perfectly right. it is necessary to do this because the event will not be detected either by polling (happening too late) nor by the irq (happening too early). So we must trigger the hotplug work to make it check whether the connectors states changed. > > Without this, no irq will be triggered and the hpd change will be > > lost. > > > > Signed-off-by: Paul Kocialkowski > > --- > > drivers/gpu/drm/i915/intel_hotplug.c | 8 +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_hotplug.c > > b/drivers/gpu/drm/i915/intel_hotplug.c > > index f1200272a699..29f55480b0bb 100644 > > --- a/drivers/gpu/drm/i915/intel_hotplug.c > > +++ b/drivers/gpu/drm/i915/intel_hotplug.c > > @@ -218,9 +218,13 @@ static void > > intel_hpd_irq_storm_reenable_work(struct work_struct *work) > > struct intel_connector *intel_connector = > > to_intel_connector(connector); > > > > if (intel_connector->encoder->hpd_pin == i) > > { > > - if (connector->polled != > > intel_connector->polled) > > + if (connector->polled != > > intel_connector->polled) { > > DRM_DEBUG_DRIVER("Reenablin > > g HPD on connector %s\n", > > connector- > > >name); > > + > > + dev_priv- > > >hotplug.event_bits |= (1 << i); > > + } > > + > > connector->polled = > > intel_connector->polled; > > if (!connector->polled) > > connector->polled = > > DRM_CONNECTOR_POLL_HPD; > > @@ -232,6 +236,8 @@ static void > > intel_hpd_irq_storm_reenable_work(struct work_struct *work) > > dev_priv->display.hpd_irq_setup(dev_priv); > > spin_unlock_irq(&dev_priv->irq_lock); > > > > + schedule_work(&dev_priv->hotplug.hotplug_work); > > How does this work with DP connectors? From what I understand, the > event_bits are set for DP based on the return value from > intel_dp_hpd_pulse(). But, doesn't this patch set the bits > unconditionally? It works the same as other connectors, nothing specific there. The hotplug work will read the connector's state and issue a uevent if its value has changed. > > + > > intel_runtime_pm_put(dev_priv); > > } > > -- Paul Kocialkowski Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland