[Intel-gfx] [PATCH] drm/i915/hotplug: Fixing storm handling for digital ports

[Daniel Vetter]

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 at ffwll.ch> wrote:
> > > On Tue, Jun 30, 2015 at 01:19:57PM +0300, Jani Nikula wrote:
> > >> On Tue, 30 Jun 2015, Daniel Vetter <daniel at ffwll.ch> 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 at intel.com>
> > >> >> >>
> > >> >> >>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