[Intel-gfx] [PATCH 2/5] drm/i915/irq: abstract irq storm hotplug disabling
On 22 June 2015 at 23:02, Daniel Vetter wrote: > On Thu, Jun 18, 2015 at 01:06:14PM +0300, Jani Nikula wrote: >> Continue abstracting hotplug storm related functions to clarify the >> code. This time, abstract hotplug irq storm related hotplug >> disabling. While at it, clean up the loop iterating over connectors for >> readability. >> >> Signed-off-by: Jani Nikula >> --- >> drivers/gpu/drm/i915/i915_irq.c | 77 >> ++--- >> 1 file changed, 50 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c >> b/drivers/gpu/drm/i915/i915_irq.c >> index d64d6895a2e5..bf4c15d0ea2b 100644 >> --- a/drivers/gpu/drm/i915/i915_irq.c >> +++ b/drivers/gpu/drm/i915/i915_irq.c >> @@ -879,7 +879,7 @@ static void i915_digport_work_func(struct work_struct >> *work) >> /* >> * Handle hotplug events outside the interrupt handler proper. >> */ >> -#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000) >> +static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv); >> >> static void i915_hotplug_work_func(struct work_struct *work) >> { >> @@ -890,7 +890,6 @@ static void i915_hotplug_work_func(struct work_struct >> *work) >> struct intel_connector *intel_connector; >> struct intel_encoder *intel_encoder; >> struct drm_connector *connector; >> - bool hpd_disabled = false; >> bool changed = false; >> u32 hpd_event_bits; >> >> @@ -901,31 +900,9 @@ static void i915_hotplug_work_func(struct work_struct >> *work) >> >> hpd_event_bits = dev_priv->hotplug.event_bits; >> dev_priv->hotplug.event_bits = 0; >> - list_for_each_entry(connector, _config->connector_list, head) { > > Random comment: We have piles of connector_list walking in probe codde, > and DP MST adds/removes them without much thought really users of these. > Dave? Do we need a connector_list spinlock? > > Just grabbing one of the modeset locks won't cut it I think since it'll > serialize way too much. In my tree currently this code grabs mode_config->mutex, so it should be fine wrt the connector list disappearing, as removing connectors is protected my mode_config->mutex as well. if we do add a spinlock, we need to be careful about dropping it and restarting the loops etc, as I would guess there are a fair few paths we don't want to descend holding the spin lock, due to possible sleeping/scheduling etc. I think the intel_hpd_irq_reenable_work is the one that stands out to me, the short hotplug paths don't traverse connector lists deliberately to avoid the problem. Dave.
[Intel-gfx] [PATCH 2/5] drm/i915/irq: abstract irq storm hotplug disabling
On Tue, Jun 23, 2015 at 10:29:27AM +1000, Dave Airlie wrote: > On 22 June 2015 at 23:02, Daniel Vetter wrote: > > On Thu, Jun 18, 2015 at 01:06:14PM +0300, Jani Nikula wrote: > >> Continue abstracting hotplug storm related functions to clarify the > >> code. This time, abstract hotplug irq storm related hotplug > >> disabling. While at it, clean up the loop iterating over connectors for > >> readability. > >> > >> Signed-off-by: Jani Nikula > >> --- > >> drivers/gpu/drm/i915/i915_irq.c | 77 > >> ++--- > >> 1 file changed, 50 insertions(+), 27 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_irq.c > >> b/drivers/gpu/drm/i915/i915_irq.c > >> index d64d6895a2e5..bf4c15d0ea2b 100644 > >> --- a/drivers/gpu/drm/i915/i915_irq.c > >> +++ b/drivers/gpu/drm/i915/i915_irq.c > >> @@ -879,7 +879,7 @@ static void i915_digport_work_func(struct work_struct > >> *work) > >> /* > >> * Handle hotplug events outside the interrupt handler proper. > >> */ > >> -#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000) > >> +static void intel_hpd_irq_storm_disable(struct drm_i915_private > >> *dev_priv); > >> > >> static void i915_hotplug_work_func(struct work_struct *work) > >> { > >> @@ -890,7 +890,6 @@ static void i915_hotplug_work_func(struct work_struct > >> *work) > >> struct intel_connector *intel_connector; > >> struct intel_encoder *intel_encoder; > >> struct drm_connector *connector; > >> - bool hpd_disabled = false; > >> bool changed = false; > >> u32 hpd_event_bits; > >> > >> @@ -901,31 +900,9 @@ static void i915_hotplug_work_func(struct work_struct > >> *work) > >> > >> hpd_event_bits = dev_priv->hotplug.event_bits; > >> dev_priv->hotplug.event_bits = 0; > >> - list_for_each_entry(connector, _config->connector_list, head) { > > > > Random comment: We have piles of connector_list walking in probe codde, > > and DP MST adds/removes them without much thought really users of these. > > Dave? Do we need a connector_list spinlock? > > > > Just grabbing one of the modeset locks won't cut it I think since it'll > > serialize way too much. > > In my tree currently this code grabs mode_config->mutex, so it should > be fine wrt > the connector list disappearing, as removing connectors is protected > my mode_config->mutex > as well. Hm right I mixed things up and didn't realize this is protected by the mode_config->mutex. > if we do add a spinlock, we need to be careful about dropping it and > restarting > the loops etc, as I would guess there are a fair few paths we don't > want to descend > holding the spin lock, due to possible sleeping/scheduling etc. > > I think the intel_hpd_irq_reenable_work is the one that stands out to > me, the short > hotplug paths don't traverse connector lists deliberately to avoid the > problem. I think grabbing the mode_config->mutex in there should be fine. Looks like we can stretch just grabbing mode_config->mutex for a bit longer ;-) Jani, can you perhaps throw this in with your next round of hpd cleanup? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[Intel-gfx] [PATCH 2/5] drm/i915/irq: abstract irq storm hotplug disabling
On Thu, Jun 18, 2015 at 01:06:14PM +0300, Jani Nikula wrote: > Continue abstracting hotplug storm related functions to clarify the > code. This time, abstract hotplug irq storm related hotplug > disabling. While at it, clean up the loop iterating over connectors for > readability. > > Signed-off-by: Jani Nikula > --- > drivers/gpu/drm/i915/i915_irq.c | 77 > ++--- > 1 file changed, 50 insertions(+), 27 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index d64d6895a2e5..bf4c15d0ea2b 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -879,7 +879,7 @@ static void i915_digport_work_func(struct work_struct > *work) > /* > * Handle hotplug events outside the interrupt handler proper. > */ > -#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000) > +static void intel_hpd_irq_storm_disable(struct drm_i915_private *dev_priv); > > static void i915_hotplug_work_func(struct work_struct *work) > { > @@ -890,7 +890,6 @@ static void i915_hotplug_work_func(struct work_struct > *work) > struct intel_connector *intel_connector; > struct intel_encoder *intel_encoder; > struct drm_connector *connector; > - bool hpd_disabled = false; > bool changed = false; > u32 hpd_event_bits; > > @@ -901,31 +900,9 @@ static void i915_hotplug_work_func(struct work_struct > *work) > > hpd_event_bits = dev_priv->hotplug.event_bits; > dev_priv->hotplug.event_bits = 0; > - list_for_each_entry(connector, _config->connector_list, head) { Random comment: We have piles of connector_list walking in probe codde, and DP MST adds/removes them without much thought really users of these. Dave? Do we need a connector_list spinlock? Just grabbing one of the modeset locks won't cut it I think since it'll serialize way too much. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch