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 jani.nikula@intel.com
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, &mode_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
On 22 June 2015 at 23:02, Daniel Vetter daniel@ffwll.ch 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 jani.nikula@intel.com
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, &mode_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.
On Tue, Jun 23, 2015 at 10:29:27AM +1000, Dave Airlie wrote:
On 22 June 2015 at 23:02, Daniel Vetter daniel@ffwll.ch 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 jani.nikula@intel.com
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, &mode_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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Dave Airlie