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 <[email protected]>
---
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);
}
--
2.13.1
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 <[email protected]>
> ---
> 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);
> }
>
--
Paul Kocialkowski <[email protected]>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland
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 <[email protected]>
> ---
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland
> _______________________________________________
> dri-devel mailing list
> [email protected]
> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland
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 <[email protected]
> > > 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 <[email protected]>
> > Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo,
> > Finland
> > _______________________________________________
> > dri-devel mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Paul Kocialkowski <[email protected]>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland