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 Content-Length: 3905 Lines: 101 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