| Message ID | 20170626123229.27939-1-paul.kocialkowski@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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. > > Without this, no irq will be triggered and the hpd change will be > lost. Does anyone have feedback to provide on this? It looks like it should be a no-brainer. Cheers, Paul > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> > --- > 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("Reenabling > 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); > + > intel_runtime_pm_put(dev_priv); > } >
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? > 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? > Without this, no irq will be triggered and the hpd change will be lost. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> > --- > 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("Reenabling 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? > + > intel_runtime_pm_put(dev_priv); > } >
On Tue, Jul 18, 2017 at 03:11:42PM +0300, Paul Kocialkowski 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. 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. > > > > Without this, no irq will be triggered and the hpd change will be > > lost. > > Does anyone have feedback to provide on this? > It looks like it should be a no-brainer. > > Cheers, > > Paul > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> > > --- > > 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) { So if this hpd pin in intel_connector->encoder is set then that means it got the hpd but because connector->polled is != intel_connector->polled polling didnt detect that connector. Is that what you are trying to do here? Manasi > > - if (connector->polled != > > intel_connector->polled) > > + if (connector->polled != > > intel_connector->polled) { > > DRM_DEBUG_DRIVER("Reenabling > > 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); > > + > > intel_runtime_pm_put(dev_priv); > > } > > > -- > Paul Kocialkowski <paul.kocialkowski@linux.intel.com> > Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 <paul.kocialkowski@linux.intel.com> > > --- > > 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); > > } > >
On Wed, 2017-07-19 at 23:11 -0700, Manasi Navare wrote: > On Tue, Jul 18, 2017 at 03:11:42PM +0300, Paul Kocialkowski 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. > > > 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. > > > > > > Without this, no irq will be triggered and the hpd change will be > > > lost. > > > > Does anyone have feedback to provide on this? > > It looks like it should be a no-brainer. > > > > Cheers, > > > > Paul > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.co > > > m> > > > --- > > > 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) { > > So if this hpd pin in intel_connector->encoder is set then that > means it got the hpd but because connector->polled is != > intel_connector->polled > polling didnt detect that connector. > > Is that what you are trying to do here? The first test is simply a way to match the intel_connector to the drm connector. The second one indicates whether we are using polling or irq mode. If they don't match (given the test earlier in the code that only selects hpd pins that use irq mode), it means that we are switching from polling mode to irq mode. When that happens, it is necessary to assume that the connector state may have changed (in the time window between the last poll and enabling irq), so the hotplug work is scheduled. It will check whether a hpd state change did happen or not and issue a uevent if that is the case. > > > - if (connector->polled != > > > intel_connector->polled) > > > + if (connector->polled != > > > intel_connector->polled) { > > > DRM_DEBUG_DRIVER("Reenabl > > > ing > > > HPD on connector %s\n", > > > connecto > > > r- > > > > 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); > > > + > > > intel_runtime_pm_put(dev_priv); > > > } > > > > > > > -- > > Paul Kocialkowski <paul.kocialkowski@linux.intel.com> > > Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, > > Finland > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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("Reenabling 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); + intel_runtime_pm_put(dev_priv); }
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. 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. Without this, no irq will be triggered and the hpd change will be lost. Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> --- drivers/gpu/drm/i915/intel_hotplug.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)