Re: [Intel-gfx] [PATCH] drm/i915: Synchronize connectors states when switching from poll to irq

[Paul Kocialkowski]

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@xxxxxxxxxxxxxxx>
> > ---
> >  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 <paul.kocialkowski@xxxxxxxxxxxxxxx>
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland