On Tue, Jun 30, 2015 at 03:47:41PM +0300, Ville Syrjälä wrote: > On Tue, Jun 30, 2015 at 03:30:16PM +0300, Jani Nikula wrote: > > On Tue, 30 Jun 2015, Daniel Vetter <daniel@xxxxxxxx> wrote: > > > On Tue, Jun 30, 2015 at 01:19:57PM +0300, Jani Nikula wrote: > > >> On Tue, 30 Jun 2015, Daniel Vetter <daniel@xxxxxxxx> wrote: > > >> > On Tue, Jun 30, 2015 at 08:45:48AM +0530, Sivakumar Thulasimani wrote: > > >> >> > > >> >> > > >> >> On 6/29/2015 10:07 PM, Daniel Vetter wrote: > > >> >> >On Mon, Jun 29, 2015 at 04:30:40PM +0530, Sivakumar Thulasimani wrote: > > >> >> >>From: "Thulasimani, Sivakumar" <sivakumar.thulasimani@xxxxxxxxx> > > >> >> >> > > >> >> >>HPD storm is detected in intel_hpd_irq_handler and disabled for respective > > >> >> >>port immediately but polling is enabled only in i915_hotplug_work_func and > > >> >> >>not in i915_digport_work_func. This will result in disabled hpd never enabled > > >> >> >>back again. This is fixed by calling the appropriate storm disable function > > >> >> >>that will handle the rest of the sequence (both polling enable and reenabling > > >> >> >>of HPD later). > > >> >> >>--- > > >> >> >> drivers/gpu/drm/i915/intel_hotplug.c | 4 ++++ > > >> >> >> 1 file changed, 4 insertions(+) > > >> >> >> > > >> >> >>diff --git a/drivers/gpu/drm/i915/intel_hotplug.c b/drivers/gpu/drm/i915/intel_hotplug.c > > >> >> >>index 3c53aac..8e18587 100644 > > >> >> >>--- a/drivers/gpu/drm/i915/intel_hotplug.c > > >> >> >>+++ b/drivers/gpu/drm/i915/intel_hotplug.c > > >> >> >>@@ -205,6 +205,10 @@ static void i915_digport_work_func(struct work_struct *work) > > >> >> >> dev_priv->hotplug.long_port_mask = 0; > > >> >> >> short_port_mask = dev_priv->hotplug.short_port_mask; > > >> >> >> dev_priv->hotplug.short_port_mask = 0; > > >> >> >>+ > > >> >> >>+ /* Disable hotplug on connectors that hit an irq storm. */ > > >> >> >>+ intel_hpd_irq_storm_disable(dev_priv); > > >> >> >digport_work_func schedules the hotplug handler for everything not > > >> >> >handled, which should result in this getting called. It really shouldn't > > >> >> >matter when exactly it gets called. > > >> >> > > > >> >> >Can you please provide more data and details for your analysis? Like bug > > >> >> >reports, backtraces and dmesg traces showing that the handler is stuck and > > >> >> >similar things. > > >> >> > > > >> >> >Also your patch is missing the s-o-b line. > > >> >> >-Daniel > > >> >> > > > >> >> there is no bug filed for this, it was observed as part of code analysis > > >> >> (that is provided below) > > >> >> I'll try to get more info as soon as i get access to a system. > > >> >> > > >> >> short answer: > > >> >> the issue will be seen during hpd storm, where the last HPD is handled > > >> >> inside intel_dp_hpd_pulse. > > >> >> so i915_hotplug_work_func will not be queued thus missing the storm_disable > > >> >> call. > > >> >> > > >> >> long answer : > > >> >> To give a bit more background, lets assume that we get a call to > > >> >> intel_hpd_irq_handler, with params long pulse for DP panel in PORT_B > > >> >> on a HSW/BDW system during HPD storm scenario. > > >> >> The following sequence will take place > > >> >> *) is_dig_port will be set and will result in queue_dig being set as well > > >> >> *) intel_hpd_irq_storm_detect will detect that this is 6th hpd call within > > >> >> the > > >> >> HPD_STORM_DETECT_PERIOD and so will mark the HPD status of PORT_B to > > >> >> HPD_MARK_DISABLED > > >> >> *) This will result in HPD for PORT_B being disabled immediately(masked in > > >> >> case of LPT) > > >> >> *) i915_digport_work_func will be queued at the end of this function, since > > >> >> queue_dig is set > > >> >> *) once in the i915_digport_work_func, hpd_pulse func pointer will be > > >> >> executed since it is defined for DP > > >> >> *) intel_dp_hpd_pulse, will have long_hpd set and since the panel is plugged > > >> >> in still, > > >> >> ISR will be high and so will return true. > > >> >> *) intel_dp_get_dpcd, will succeed since DP is connected > > >> >> *) finally IRQ_HANDLED will be returned > > >> >> *) once call exits intel_hpd_irq_handler, HPD on port B will never be > > >> >> enabled again > > >> >> (unmasked in case of LPT) and no more hot plug notifications. > > >> > > > >> > The assumption of the storm code is that when there is a DP sink, a storm > > >> > will never happen. We need that since otherwise the mst code (which > > >> > creates ridiculous amounts of hpds on the DP port) will run into the storm > > >> > detection code all the time. > > >> > > > >> > Might be better to document this design assumption somewhere, but it is > > >> > baked in. Hence my question whether you've seen this happen in the real > > >> > world - DP storms haven't been observed yet afaik, and it would be a much > > >> > more serious problem. > > >> > > >> The dp short hotplug irqs (used by mst) are not caught by the irq storm > > >> code, but the long hotplug irqs are. > > > > > > We assume there's no DP hotplug storms ever, whether long or short pulses. > > > Trying to fix that will require serious rework since we need to wait until > > > dig_port_work has run to know whether the hpd was a real one or just > > > fluctuation, and only update storm statistic then. And once we do DP is > > > essentially broken, which means we also need to enable polling for dp aux > > > short pulses (which will probably piss off some sink device). > > > > > > In short: If you have a hpd storm, and there's something DP-like > > > connected, you're screwed. Until we have real-world evidence of this > > > happening updating comments is really the only thing we need. > > > > In that case we should update the code to never do hotplug irq storm > > detection on dp long hpd, which we currently do. > > The HPD pin is shared for DP and HDMI so we can't disable HPD just for > HDMI when a storm is detected. Yup this is the crux. The real fix really is wiring up IRQ_NONE handling all the way to be able to differentiate storms from normal/expected irq load. But that really has to wait until this happens in reality somewhere with a DP sink. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx