Detect and deal with Interrupt 'Storms' from noisy Hotplug Lines.
Subsystems (2)
cover Detect and deal with Interrupt 'Storms' from noisy Hotplug Lines.
Despite the many attempts to fix the issue with noisy hotplug interrupt lines we are still seeing systems that suffer from this:
Recently we encountered a rather large scale installation of Q35 systems which was hit by this issue rather severely: It seemed as if not all machines of the same model were hit equally bad, in the worst cased hotplug interrupt noise caused several 1000 interrupts / s. Those machines would not even boot, instead the interrupt handler and the scheduled workers would keep the CPU busy that eventually the watchdog would kick in and issue an NMI. Other machines only received severa 10s to 100s of interrupts per sec - those machines would run properly - just with an excessive system load. More thorough investigations seemed to indicate that this condition only happen at certain video modes.
On another system - a laptop - a hotplug interrupt 'storm' occurred when it was charging and the batteries were at certain charge levels. While the system was still running fine its load was high enough that the user noticed from the fan noise that a problem existed.
The latter system had a Sandybridge chipset, thus a totally different generation from the former.
All those cases seemed to have been caused by cross talk on badly routed hotplug signal lines (or voltage instabilities).
This led to the conclusion that instead of trying to work around these 'storms' for each individual system, there should be a generic way to detect such a condition and take appropriate action:
This patch series implements a hotplug 'storm' detection, disables the respective interrupt for the hotplug pin when this condition is detected and reverts to periodic output polling on the affected connector. After a grace period of 2 minutes it will reenable hotplug on the affected line. This will take care of cases in which this condition is only temporary. Should the 'storm' condtion persist, this cycle will start over again.
To implement this some rearrangements in the code were required:
- The interrupt status bit which signals a hotplug needed to be recorded for each connector.
- The interrupt enable functions needed to be separate, also they need to be able to enable interrupts for each hotplug line independently.
Egbert Eich (8):
drm/i915: Remove pch_rq_mask from struct drm_i915_private. drm/i915: Set hotplug_supported_flag for all chipset generations. drm/i915: Add hpd status bit to struct intel_connector. drm/i915: Add Hotplug IRQ Storm detection.
drm/i915: Move hotplug interrupt enable for i915/i965/valleyview into
a separate function.
drm/i915: Only enable hotplug irq when needed on Ironlake and later
chips.
drm/i915: When detecting a hotplug IRQ storm disable respective IRQs. drm/i915: Add Reenable Timer to turn Hotplug Detection back on.
drivers/gpu/drm/i915/i915_drv.h | 7 +- drivers/gpu/drm/i915/i915_irq.c | 475 +++++++++++++++++++++++++++---------- drivers/gpu/drm/i915/intel_crt.c | 3 +- drivers/gpu/drm/i915/intel_dp.c | 5 +- drivers/gpu/drm/i915/intel_drv.h | 11 + drivers/gpu/drm/i915/intel_hdmi.c | 5 +- drivers/gpu/drm/i915/intel_sdvo.c | 23 +- 7 files changed, 383 insertions(+), 146 deletions(-)
Signature · 1 line
1.7.7
PATCH 1/8 drm/i915: Remove pch_rq_mask from struct drm_i915_private.
This variable is only used locally in the irq postinstall functions for ivybridge and ironlake.
--- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/i915_irq.c | 10 ++++++---- 2 files changed, 6 insertions(+), 5 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +0 -1
@@ -665,7 +665,6 @@ typedef struct drm_i915_private {u32 pipestat[2 ];u32 irq_mask;u32 gt_irq_mask; -u32 pch_irq_mask;u32 hotplug_supported_mask;struct work_struct hotplug_work;
drivers/gpu/drm/i915/i915_irq.c +6 -4
@@ -1857,6 +1857,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)DE_PLANEA_FLIP_DONE |DE_PLANEB_FLIP_DONE ;u32 render_irqs;u32 hotplug_mask; +u32 pch_irq_mask; dev_priv->irq_mask = ~display_mask;
@@ -1897,10 +1898,10 @@ static int ironlake_irq_postinstall(struct drm_device *dev) SDE_AUX_MASK); } - dev_priv->pch_irq_mask = ~hotplug_mask; + pch_irq_mask = ~hotplug_mask;I915_WRITE (SDEIIR ,I915_READ (SDEIIR )); -I915_WRITE (SDEIMR , dev_priv->pch_irq_mask); +I915_WRITE (SDEIMR , pch_irq_mask);I915_WRITE (SDEIER , hotplug_mask);POSTING_READ (SDEIER );
@@ -1927,6 +1928,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)DE_PLANEA_FLIP_DONE_IVB ;u32 render_irqs;u32 hotplug_mask; +u32 pch_irq_mask; dev_priv->irq_mask = ~display_mask;
@@ -1954,10 +1956,10 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)SDE_PORTB_HOTPLUG_CPT |SDE_PORTC_HOTPLUG_CPT | SDE_PORTD_HOTPLUG_CPT); - dev_priv->pch_irq_mask = ~hotplug_mask; + pch_irq_mask = ~hotplug_mask;I915_WRITE (SDEIIR ,I915_READ (SDEIIR )); -I915_WRITE (SDEIMR , dev_priv->pch_irq_mask); +I915_WRITE (SDEIMR , pch_irq_mask);I915_WRITE (SDEIER , hotplug_mask);POSTING_READ (SDEIER );
Signature · 1 line
1.7.7
Daniel Vetter
AppliedRe: [PATCH 1/8] drm/i915: Remove pch_rq_mask from struct drm_i915_private.
Quoted 4 lines · Egbert Eich
This variable is only used locally in the irq postinstall functions for ivybridge and ironlake.
Signed-off-by Egbert Eich <[email protected]>
Applied to dinq with a minor conflict resolved, thanks for the patch. -Daniel
Quoted 70 lines
--- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/i915_irq.c | 10 ++++++---- 2 files changed, 6 insertions(+), 5 deletions(-)drivers/gpu/drm/i915/i915_drv.h +0
-1@@ -665,7 +665,6 @@ typedef struct drm_i915_private {u32 pipestat[2 ];u32 irq_mask;u32 gt_irq_mask; -u32 pch_irq_mask;u32 hotplug_supported_mask;struct work_struct hotplug_work;drivers/gpu/drm/i915/i915_irq.c +6
-4@@ -1857,6 +1857,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)DE_PLANEA_FLIP_DONE |DE_PLANEB_FLIP_DONE ;u32 render_irqs;u32 hotplug_mask; +u32 pch_irq_mask; dev_priv->irq_mask = ~display_mask;@@ -1897,10 +1898,10 @@ static int ironlake_irq_postinstall(struct drm_device *dev) SDE_AUX_MASK); } - dev_priv->pch_irq_mask = ~hotplug_mask; + pch_irq_mask = ~hotplug_mask;I915_WRITE (SDEIIR ,I915_READ (SDEIIR )); -I915_WRITE (SDEIMR , dev_priv->pch_irq_mask); +I915_WRITE (SDEIMR , pch_irq_mask);I915_WRITE (SDEIER , hotplug_mask);POSTING_READ (SDEIER );@@ -1927,6 +1928,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)DE_PLANEA_FLIP_DONE_IVB ;u32 render_irqs;u32 hotplug_mask; +u32 pch_irq_mask; dev_priv->irq_mask = ~display_mask;@@ -1954,10 +1956,10 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)SDE_PORTB_HOTPLUG_CPT |SDE_PORTC_HOTPLUG_CPT | SDE_PORTD_HOTPLUG_CPT); - dev_priv->pch_irq_mask = ~hotplug_mask; + pch_irq_mask = ~hotplug_mask;I915_WRITE (SDEIIR ,I915_READ (SDEIIR )); -I915_WRITE (SDEIMR , dev_priv->pch_irq_mask); +I915_WRITE (SDEIMR , pch_irq_mask);I915_WRITE (SDEIER , hotplug_mask);POSTING_READ (SDEIER );Signature · 6 lines
1.7.7 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
PATCH 2/8 drm/i915: Set hotplug_supported_flag for all chipset generations.
In reply to Egbert EichSo far the hotplug_supported_mask in the struct drm_i915_private is only used for pre-Ironlake chipsets.
This patch sets up the correct value for all generations.
--- drivers/gpu/drm/i915/i915_drv.h | 4 ++ drivers/gpu/drm/i915/i915_irq.c | 71 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_crt.c | 2 +- drivers/gpu/drm/i915/intel_dp.c | 4 +-- drivers/gpu/drm/i915/intel_hdmi.c | 4 +-- drivers/gpu/drm/i915/intel_sdvo.c | 12 +------ 6 files changed, 79 insertions(+), 18 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +4 -0
@@ -1322,6 +1322,10 @@ extern void intel_irq_init(struct drm_device *dev);extern void intel_gt_init (struct drm_device *dev);extern void intel_gt_reset (struct drm_device *dev); +extern u32 intel_crt_hotplug_int_status (struct drm_device *dev); +extern u32 intel_sdvo_hotplug_int_status (struct drm_device *dev,bool is_sdvo_b); +extern u32 intel_hotplug_int_status (struct drm_device *dev,enum port port); +void i915_error_state_free (struct kref *error_ref);void
drivers/gpu/drm/i915/i915_irq.c +71 -0
74 lines not on this page · raw message
drivers/gpu/drm/i915/intel_crt.c +1 -1
@@ -797,7 +797,7 @@ void intel_crt_init(struct drm_device *dev)*/ crt->force_hotplug_required =0 ; - dev_priv->hotplug_supported_mask |=CRT_HOTPLUG_INT_STATUS ; + dev_priv->hotplug_supported_mask |=intel_crt_hotplug_int_status (dev);/* * TODO: find a proper way to discover whether we need to set the
drivers/gpu/drm/i915/intel_dp.c +1 -3
@@ -2754,21 +2754,19 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, name ="DPDDC-A" ;break ;case PORT_B : - dev_priv->hotplug_supported_mask |=DPB_HOTPLUG_INT_STATUS ; name ="DPDDC-B" ;break ;case PORT_C : - dev_priv->hotplug_supported_mask |=DPC_HOTPLUG_INT_STATUS ; name ="DPDDC-C" ;break ;case PORT_D : - dev_priv->hotplug_supported_mask |=DPD_HOTPLUG_INT_STATUS ; name ="DPDDC-D" ;break ;default :WARN (1 ,"Invalid port %c\n" ,port_name (port));break ; } + dev_priv->hotplug_supported_mask |=intel_hotplug_int_status (dev, port);if (is_edp (intel_dp))intel_dp_init_panel_power_sequencer (dev, intel_dp);
drivers/gpu/drm/i915/intel_hdmi.c +1 -3
@@ -980,21 +980,19 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,switch (port) {case PORT_B : intel_hdmi->ddc_bus =GMBUS_PORT_DPB ; - dev_priv->hotplug_supported_mask |=HDMIB_HOTPLUG_INT_STATUS ;break ;case PORT_C : intel_hdmi->ddc_bus =GMBUS_PORT_DPC ; - dev_priv->hotplug_supported_mask |=HDMIC_HOTPLUG_INT_STATUS ;break ;case PORT_D : intel_hdmi->ddc_bus =GMBUS_PORT_DPD ; - dev_priv->hotplug_supported_mask |=HDMID_HOTPLUG_INT_STATUS ;break ;case PORT_A :/* Internal port only for eDP. */ default :BUG (); } + dev_priv->hotplug_supported_mask |=intel_hotplug_int_status (dev, port);if (!HAS_PCH_SPLIT (dev)) { intel_hdmi->write_infoframe = g4x_write_infoframe;
drivers/gpu/drm/i915/intel_sdvo.c +1 -11
@@ -2727,17 +2727,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob) } } - hotplug_mask =0 ; -if (IS_G4X (dev)) { - hotplug_mask = intel_sdvo->is_sdvob ? -SDVOB_HOTPLUG_INT_STATUS_G4X :SDVOC_HOTPLUG_INT_STATUS_G4X ; - }else if (IS_GEN4 (dev)) { - hotplug_mask = intel_sdvo->is_sdvob ? -SDVOB_HOTPLUG_INT_STATUS_I965 :SDVOC_HOTPLUG_INT_STATUS_I965 ; - }else { - hotplug_mask = intel_sdvo->is_sdvob ? -SDVOB_HOTPLUG_INT_STATUS_I915 :SDVOC_HOTPLUG_INT_STATUS_I915 ; - } + hotplug_mask =intel_sdvo_hotplug_int_status (dev, intel_sdvo->is_sdvob);drm_encoder_helper_add (&intel_encoder->base, &intel_sdvo_helper_funcs);
Signature · 1 line
1.7.7
Re: [PATCH 2/8] drm/i915: Set hotplug_supported_flag for all chipset generations.
Quoted 5 lines · On Thu, 10 Jan 2013 10:02:40 -0500 Egbert Eich <[email protected]> wrote:
So far the
hotplug_supported_maskin the structdrm_i915_privateis only used for pre-Ironlake chipsets.
This patch sets up the correct value for all generations.Signed-off-by Egbert Eich <[email protected]>
Quoted 8 lines
--- drivers/gpu/drm/i915/i915_drv.h | 4 ++ drivers/gpu/drm/i915/i915_irq.c | 71 +++++++++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_crt.c | 2 +- drivers/gpu/drm/i915/intel_dp.c | 4 +-- drivers/gpu/drm/i915/intel_hdmi.c | 4 +-- drivers/gpu/drm/i915/intel_sdvo.c | 12 +------ 6 files changed, 79 insertions(+), 18 deletions(-)
I like it.
Just a tiny style nitpick. I'd prefer:
+ /* SDVO is treated separately */
+ if (IS_IVYBRIDGE(dev) ||
+ IS_HASWELL(dev) ||
+ (HAS_PCH_SPLIT(dev) && HAS_PCH_CPT(dev))) {
to be
+ /* SDVO is treated separately */
+ if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev) ||
+ (HAS_PCH_SPLIT(dev) && HAS_PCH_CPT(dev))) {
instead because vertical space is precious. But that's definitely not worth delaying this patch over.
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
PATCH 3/8 drm/i915: Add hpd status bit to struct intel_connector.
In reply to Egbert EichTo be able to map an HPD interrupt to a connector add the hpd status bit to the intel_connector structure.
--- drivers/gpu/drm/i915/intel_crt.c | 3 ++- drivers/gpu/drm/i915/intel_dp.c | 3 ++- drivers/gpu/drm/i915/intel_drv.h | 3 +++ drivers/gpu/drm/i915/intel_hdmi.c | 3 ++- drivers/gpu/drm/i915/intel_sdvo.c | 11 ++++++++++- 5 files changed, 19 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/intel_crt.c +2 -1
@@ -797,7 +797,8 @@ void intel_crt_init(struct drm_device *dev)*/ crt->force_hotplug_required =0 ; - dev_priv->hotplug_supported_mask |=intel_crt_hotplug_int_status (dev); + intel_connector->hpd_status_bit =intel_crt_hotplug_int_status (dev); + dev_priv->hotplug_supported_mask |= intel_connector->hpd_status_bit;/* * TODO: find a proper way to discover whether we need to set the
drivers/gpu/drm/i915/intel_dp.c +2 -1
@@ -2766,7 +2766,8 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,WARN (1 ,"Invalid port %c\n" ,port_name (port));break ; } - dev_priv->hotplug_supported_mask |=intel_hotplug_int_status (dev, port); + intel_connector->hpd_status_bit =intel_hotplug_int_status (dev, port); + dev_priv->hotplug_supported_mask |= intel_connector->hpd_status_bit;if (is_edp (intel_dp))intel_dp_init_panel_power_sequencer (dev, intel_dp);
drivers/gpu/drm/i915/intel_drv.h +3 -0
@@ -191,6 +191,9 @@ struct intel_connector {/* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */ struct edid *edid; + +/* hpd status bit for this connector */ +u32 hpd_status_bit; };struct intel_crtc {
drivers/gpu/drm/i915/intel_hdmi.c +2 -1
@@ -992,7 +992,8 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,default :BUG (); } - dev_priv->hotplug_supported_mask |=intel_hotplug_int_status (dev, port); + intel_connector->hpd_status_bit =intel_hotplug_int_status (dev, port); + dev_priv->hotplug_supported_mask |= intel_connector->hpd_status_bit;if (!HAS_PCH_SPLIT (dev)) { intel_hdmi->write_infoframe = g4x_write_infoframe;
drivers/gpu/drm/i915/intel_sdvo.c +10 -1
@@ -2697,6 +2697,7 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)struct drm_i915_private *dev_priv = dev->dev_private;struct intel_encoder *intel_encoder;struct intel_sdvo *intel_sdvo; +struct drm_connector *connector;u32 hotplug_mask;int i;
@@ -2749,8 +2750,16 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)/* Only enable the hotplug irq if we need it, to work around noisy * hotplug lines. */ -if (intel_sdvo->hotplug_active) +if (intel_sdvo->hotplug_active) { dev_priv->hotplug_supported_mask |= hotplug_mask; +list_for_each_entry (connector, &dev->mode_config.connector_list, head) { +struct intel_connector *intel_connector =to_intel_connector (connector); +if (intel_connector->encoder == intel_encoder) { + intel_connector->hpd_status_bit = hotplug_mask; +break ; + } + } + }intel_sdvo_select_ddc_bus (dev_priv, intel_sdvo, sdvo_reg);
Signature · 1 line
1.7.7
Re: [PATCH 3/8] drm/i915: Add hpd status bit to struct intel_connector.
Quoted 11 lines · On Thu, 10 Jan 2013 10:02:41 -0500 Egbert Eich <[email protected]> wrote:
To be able to map an HPD interrupt to a connector add the hpd status bit to the
intel_connectorstructure.Signed-off-by Egbert Eich <[email protected]>--- drivers/gpu/drm/i915/intel_crt.c | 3 ++- drivers/gpu/drm/i915/intel_dp.c | 3 ++- drivers/gpu/drm/i915/intel_drv.h | 3 +++ drivers/gpu/drm/i915/intel_hdmi.c | 3 ++- drivers/gpu/drm/i915/intel_sdvo.c | 11 ++++++++++- 5 files changed, 19 insertions(+), 4 deletions(-)
Another nice simple addition.
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
PATCH 4/8 drm/i915: Add Hotplug IRQ Storm detection.
In reply to Egbert EichAdd a hotplug IRQ storm detection (triggered when a hotplug interrupt fires more than 5 times / sec).
Mask out this specific interrupt and revert to polling on the associated output.
Rationale:
Despite of the many attempts to fix the problem with noisy hotplug interrupt lines we are still seeing systems which have issues:
Once cause of noise seems to be bad routing of the hotplug line on the board: cross talk from other signals seems to cause erronous hotplug interrupts. This has been documented as an erratum for the the i945GM chipset and thus hotplug support was disabled for this chipset model but others seem to have this problem, too.
We have seen this issue on a G35 motherboard for example:
Even different motherboards of the same model seem to behave
differently: while some only see only around 10-100 interrupts/s others seem to see 5k or more.
We've also observed a dependency on the selected video mode.
Also on certain laptops interrupt noise seems to occur duing battery charging when the battery is at a certain charge levels.
Thus we add a simple algorithm here that detects an 'interrupt storm' condition.
--- drivers/gpu/drm/i915/i915_irq.c | 74 ++++++++++++++++++++++++++++++++------ drivers/gpu/drm/i915/intel_drv.h | 4 ++ 2 files changed, 67 insertions(+), 11 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +63 -11
@@ -285,11 +285,24 @@ static void i915_hotplug_work_func(struct work_struct *work) hotplug_work);struct drm_device *dev = dev_priv->dev;struct drm_mode_config *mode_config = &dev->mode_config; +struct drm_connector *connector;struct intel_encoder *encoder; +unsigned long irqflags;mutex_lock (&mode_config->mutex);DRM_DEBUG_KMS ("running encoder hotplug functions\n" ); +spin_lock_irqsave (&dev_priv->irq_lock, irqflags); +list_for_each_entry (connector, &mode_config->connector_list, head) { +struct intel_connector *intel_connector =to_intel_connector (connector); +if (intel_connector->hpd_mark_disabled) { + intel_connector->hpd_mark_disabled =0 ; + connector->polled =DRM_CONNECTOR_POLL_CONNECT + |DRM_CONNECTOR_POLL_DISCONNECT ; + } + } +spin_unlock_irqrestore (&dev_priv->irq_lock, irqflags); +list_for_each_entry (encoder, &mode_config->encoder_list, base.head)if (encoder->hot_plug) encoder->hot_plug (encoder);
@@ -300,6 +313,36 @@ static void i915_hotplug_work_func(struct work_struct *work)drm_helper_hpd_irq_event (dev); } +static inline void hotplug_irq_storm_detect (struct drm_device *dev,u32 hotplug_trigger) +{ +struct drm_mode_config *mode_config = &dev->mode_config; +drm_i915_private_t *dev_priv = dev->dev_private; +struct drm_connector *connector; +unsigned long irqflags; + +spin_lock_irqsave (&dev_priv->irq_lock, irqflags); + +list_for_each_entry (connector, &mode_config->connector_list, head) { +struct intel_connector *intel_connector =to_intel_connector (connector); + +if (hotplug_trigger & intel_connector->hpd_status_bit) { +if (jiffies > (intel_connector->last_hpd_jiffies +msecs_to_jiffies (1000 )) || + jiffies < intel_connector->last_hpd_jiffies) { + intel_connector->last_hpd_jiffies = jiffies; + intel_connector->hpd_cnt =0 ; + }else if (intel_connector->hpd_cnt >5 ) { + dev_priv->hotplug_supported_mask &= ~intel_connector->hpd_status_bit; + intel_connector->hpd_mark_disabled =1 ; +pr_warn ("HPD interrupt storm on connector %s disabling\n" , +drm_get_connector_name (connector)); + }else + intel_connector->hpd_cnt++; + } + } + +spin_unlock_irqrestore (&dev_priv->irq_lock, irqflags); +} +/* defined intel_pm.c */ extern spinlock_t mchdev_lock;
@@ -579,13 +622,14 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)/* Consume port. Then clear IIR or we'll miss events */ if (iir &I915_DISPLAY_PORT_INTERRUPT ) {u32 hotplug_status =I915_READ (PORT_HOTPLUG_STAT ); - +u32 hotplug_trigger = hotplug_status & dev_priv->hotplug_supported_mask;DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status); -if (hotplug_status & dev_priv->hotplug_supported_mask) +if (hotplug_trigger) { +hotplug_irq_storm_detect (dev, hotplug_trigger);queue_work (dev_priv->wq, &dev_priv->hotplug_work); - + }I915_WRITE (PORT_HOTPLUG_STAT , hotplug_status);I915_READ (PORT_HOTPLUG_STAT ); }
@@ -609,10 +653,12 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;int pipe; +u32 hotplug_trigger = pch_iir & dev_priv->hotplug_supported_mask; -if (pch_iir &SDE_HOTPLUG_MASK ) +if (hotplug_trigger) { +hotplug_irq_storm_detect (dev, hotplug_trigger);queue_work (dev_priv->wq, &dev_priv->hotplug_work); - + }if (pch_iir &SDE_AUDIO_POWER_MASK ) DRM_DEBUG_DRIVER("PCH audio power change on port %d\n", (pch_iir & SDE_AUDIO_POWER_MASK) >>
@@ -652,10 +698,12 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;int pipe; +u32 hotplug_trigger = pch_iir & dev_priv->hotplug_supported_mask; -if (pch_iir &SDE_HOTPLUG_MASK_CPT ) +if (hotplug_trigger) { +hotplug_irq_storm_detect (dev, hotplug_trigger);queue_work (dev_priv->wq, &dev_priv->hotplug_work); - + }if (pch_iir &SDE_AUDIO_POWER_MASK_CPT ) DRM_DEBUG_DRIVER("PCH audio power change on port %d\n", (pch_iir & SDE_AUDIO_POWER_MASK_CPT) >>
@@ -2368,13 +2416,15 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)if ((I915_HAS_HOTPLUG (dev)) && (iir &I915_DISPLAY_PORT_INTERRUPT )) {u32 hotplug_status =I915_READ (PORT_HOTPLUG_STAT ); +u32 hotplug_trigger = hotplug_status & dev_priv->hotplug_supported_mask;DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status); -if (hotplug_status & dev_priv->hotplug_supported_mask) +if (hotplug_trigger) { +hotplug_irq_storm_detect (dev, hotplug_trigger);queue_work (dev_priv->wq, &dev_priv->hotplug_work); - + }I915_WRITE (PORT_HOTPLUG_STAT , hotplug_status);POSTING_READ (PORT_HOTPLUG_STAT ); }
@@ -2606,13 +2656,15 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)/* Consume port. Then clear IIR or we'll miss events */ if (iir &I915_DISPLAY_PORT_INTERRUPT ) {u32 hotplug_status =I915_READ (PORT_HOTPLUG_STAT ); +u32 hotplug_trigger = hotplug_status & dev_priv->hotplug_supported_mask;DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status); -if (hotplug_status & dev_priv->hotplug_supported_mask) +if (hotplug_trigger) { +hotplug_irq_storm_detect (dev, hotplug_trigger);queue_work (dev_priv->wq, &dev_priv->hotplug_work); - + }I915_WRITE (PORT_HOTPLUG_STAT , hotplug_status);I915_READ (PORT_HOTPLUG_STAT ); }
drivers/gpu/drm/i915/intel_drv.h +4 -0
@@ -194,6 +194,10 @@ struct intel_connector {/* hpd status bit for this connector */ u32 hpd_status_bit; +/* bookkeeping for irq storm detection */ +int hpd_cnt; +unsigned long last_hpd_jiffies; +bool hpd_mark_disabled; };struct intel_crtc {
Signature · 1 line
1.7.7
Re: [PATCH 4/8] drm/i915: Add Hotplug IRQ Storm detection.
drivers/gpu/drm/i915/i915_irq.c static void i915_hotplug_work_func(struct work_struct *work) · line 330
Quoted 3 lines · On Thu, 10 Jan 2013 10:02:42 -0500 Egbert Eich <[email protected]> wrote:
drivers/gpu/drm/i915/i915_irq.c +3
-0+if (hotplug_trigger & intel_connector->hpd_status_bit) { +if (jiffies > (intel_connector->last_hpd_jiffies +msecs_to_jiffies (1000 )) || + jiffies < intel_connector->last_hpd_jiffies) {
Can you use time_after() here instead?
Otherwise this looks really nice. We should get it lots of testing and soak time.
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
PATCH 5/8 drm/i915: Move hotplug interrupt enable for i915/i965/valleyview into a separate function.
In reply to Egbert Eich--- drivers/gpu/drm/i915/i915_irq.c | 178 ++++++++++++++++++++++---------------- 1 files changed, 103 insertions(+), 75 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +103 -75
@@ -567,6 +567,32 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,queue_work (dev_priv->wq, &dev_priv->rps.work); } +static void valleyview_enable_hotplug_irq (struct drm_device *dev) +{ +drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; +u32 hotplug_en =I915_READ (PORT_HOTPLUG_EN ); + hotplug_en &= ~(HDMIB_HOTPLUG_INT_EN |HDMIC_HOTPLUG_INT_EN | +HDMID_HOTPLUG_INT_EN |SDVOC_HOTPLUG_INT_EN | +SDVOB_HOTPLUG_INT_EN |CRT_HOTPLUG_INT_EN ); + +/* Note HDMI and DP share bits */ +if (dev_priv->hotplug_supported_mask &HDMIB_HOTPLUG_INT_STATUS ) + hotplug_en |=HDMIB_HOTPLUG_INT_EN ; +if (dev_priv->hotplug_supported_mask &HDMIC_HOTPLUG_INT_STATUS ) + hotplug_en |=HDMIC_HOTPLUG_INT_EN ; +if (dev_priv->hotplug_supported_mask &HDMID_HOTPLUG_INT_STATUS ) + hotplug_en |=HDMID_HOTPLUG_INT_EN ; +if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_I915 ) + hotplug_en |=SDVOC_HOTPLUG_INT_EN ; +if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_I915 ) + hotplug_en |=SDVOB_HOTPLUG_INT_EN ; +if (dev_priv->hotplug_supported_mask &CRT_HOTPLUG_INT_STATUS ) { + hotplug_en |=CRT_HOTPLUG_INT_EN ; + hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ; + } +I915_WRITE (PORT_HOTPLUG_EN , hotplug_en); +} +static irqreturn_t valleyview_irq_handler (int irq,void *arg) {struct drm_device *dev = (struct drm_device *) arg;
@@ -2020,7 +2046,6 @@ static int valleyview_irq_postinstall(struct drm_device *dev) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;u32 enable_mask; -u32 hotplug_en =I915_READ (PORT_HOTPLUG_EN );u32 pipestat_enable =PLANE_FLIP_DONE_INT_EN_VLV ;u32 render_irqs;u16 msid;
@@ -2075,25 +2100,9 @@ static int valleyview_irq_postinstall(struct drm_device *dev)I915_WRITE (DPINVGTT ,DPINVGTT_STATUS_MASK );I915_WRITE (DPINVGTT ,DPINVGTT_EN_MASK );#endif -I915_WRITE (VLV_MASTER_IER ,MASTER_INTERRUPT_ENABLE ); -/* Note HDMI and DP share bits */ -if (dev_priv->hotplug_supported_mask &HDMIB_HOTPLUG_INT_STATUS ) - hotplug_en |=HDMIB_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &HDMIC_HOTPLUG_INT_STATUS ) - hotplug_en |=HDMIC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &HDMID_HOTPLUG_INT_STATUS ) - hotplug_en |=HDMID_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_I915 ) - hotplug_en |=SDVOC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_I915 ) - hotplug_en |=SDVOB_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &CRT_HOTPLUG_INT_STATUS ) { - hotplug_en |=CRT_HOTPLUG_INT_EN ; - hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ; - } -I915_WRITE (PORT_HOTPLUG_EN , hotplug_en); +valleyview_enable_hotplug_irq (dev);return 0 ; }
@@ -2298,6 +2307,34 @@ static void i915_irq_preinstall(struct drm_device * dev)POSTING_READ (IER ); } +static void i915_enable_hotplug_irq (struct drm_device *dev) +{ +drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; +u32 hotplug_en =I915_READ (PORT_HOTPLUG_EN ); + hotplug_en &= ~(HDMIB_HOTPLUG_INT_EN |HDMIC_HOTPLUG_INT_EN | +HDMID_HOTPLUG_INT_EN |SDVOC_HOTPLUG_INT_EN | +SDVOB_HOTPLUG_INT_EN |CRT_HOTPLUG_INT_EN ); + +if (dev_priv->hotplug_supported_mask &HDMIB_HOTPLUG_INT_STATUS ) + hotplug_en |=HDMIB_HOTPLUG_INT_EN ; +if (dev_priv->hotplug_supported_mask &HDMIC_HOTPLUG_INT_STATUS ) + hotplug_en |=HDMIC_HOTPLUG_INT_EN ; +if (dev_priv->hotplug_supported_mask &HDMID_HOTPLUG_INT_STATUS ) + hotplug_en |=HDMID_HOTPLUG_INT_EN ; +if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_I915 ) + hotplug_en |=SDVOC_HOTPLUG_INT_EN ; +if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_I915 ) + hotplug_en |=SDVOB_HOTPLUG_INT_EN ; +if (dev_priv->hotplug_supported_mask &CRT_HOTPLUG_INT_STATUS ) { + hotplug_en |=CRT_HOTPLUG_INT_EN ; + hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ; + } + +/* Ignore TV since it's buggy */ + +I915_WRITE (PORT_HOTPLUG_EN , hotplug_en); +} +static int i915_irq_postinstall (struct drm_device *dev) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
@@ -2335,28 +2372,8 @@ static int i915_irq_postinstall(struct drm_device *dev)I915_WRITE (IER , enable_mask);POSTING_READ (IER ); -if (I915_HAS_HOTPLUG (dev)) { -u32 hotplug_en =I915_READ (PORT_HOTPLUG_EN ); - -if (dev_priv->hotplug_supported_mask &HDMIB_HOTPLUG_INT_STATUS ) - hotplug_en |=HDMIB_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &HDMIC_HOTPLUG_INT_STATUS ) - hotplug_en |=HDMIC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &HDMID_HOTPLUG_INT_STATUS ) - hotplug_en |=HDMID_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_I915 ) - hotplug_en |=SDVOC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_I915 ) - hotplug_en |=SDVOB_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &CRT_HOTPLUG_INT_STATUS ) { - hotplug_en |=CRT_HOTPLUG_INT_EN ; - hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ; - } - -/* Ignore TV since it's buggy */ - -I915_WRITE (PORT_HOTPLUG_EN , hotplug_en); - } +if (I915_HAS_HOTPLUG (dev)) +i915_enable_hotplug_irq (dev);intel_opregion_enable_asle (dev);
@@ -2519,10 +2536,54 @@ static void i965_irq_preinstall(struct drm_device * dev)POSTING_READ (IER ); } -static int i965_irq_postinstall (struct drm_device *dev) +static void i965_enable_hotplug_irq (struct drm_device *dev) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;u32 hotplug_en; + + hotplug_en =I915_READ (PORT_HOTPLUG_EN ); + hotplug_en &= ~(HDMIB_HOTPLUG_INT_EN |HDMIC_HOTPLUG_INT_EN | +HDMID_HOTPLUG_INT_EN |SDVOC_HOTPLUG_INT_EN | +SDVOB_HOTPLUG_INT_EN |CRT_HOTPLUG_INT_EN ); + +/* Note HDMI and DP share hotplug bits */ +if (dev_priv->hotplug_supported_mask &HDMIB_HOTPLUG_INT_STATUS ) + hotplug_en |=HDMIB_HOTPLUG_INT_EN ; +if (dev_priv->hotplug_supported_mask &HDMIC_HOTPLUG_INT_STATUS ) + hotplug_en |=HDMIC_HOTPLUG_INT_EN ; +if (dev_priv->hotplug_supported_mask &HDMID_HOTPLUG_INT_STATUS ) + hotplug_en |=HDMID_HOTPLUG_INT_EN ; +if (IS_G4X (dev)) { +if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_G4X ) + hotplug_en |=SDVOC_HOTPLUG_INT_EN ; +if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_G4X ) + hotplug_en |=SDVOB_HOTPLUG_INT_EN ; + }else { +if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_I965 ) + hotplug_en |=SDVOC_HOTPLUG_INT_EN ; +if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_I965 ) + hotplug_en |=SDVOB_HOTPLUG_INT_EN ; + } +if (dev_priv->hotplug_supported_mask &CRT_HOTPLUG_INT_STATUS ) { + hotplug_en |=CRT_HOTPLUG_INT_EN ; + +/* Programming the CRT detection parameters tends +to generate a spurious hotplug event about three +seconds later. So just do it once. +*/ +if (IS_G4X (dev)) + hotplug_en |=CRT_HOTPLUG_ACTIVATION_PERIOD_64 ; + hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ; + } + +/* Ignore TV since it's buggy */ + +I915_WRITE (PORT_HOTPLUG_EN , hotplug_en); +} + +static int i965_irq_postinstall (struct drm_device *dev) +{ +drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;u32 enable_mask;u32 error_mask;
@@ -2563,40 +2624,7 @@ static int i965_irq_postinstall(struct drm_device *dev)I915_WRITE (IER , enable_mask);POSTING_READ (IER ); -/* Note HDMI and DP share hotplug bits */ - hotplug_en =0 ; -if (dev_priv->hotplug_supported_mask &HDMIB_HOTPLUG_INT_STATUS ) - hotplug_en |=HDMIB_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &HDMIC_HOTPLUG_INT_STATUS ) - hotplug_en |=HDMIC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &HDMID_HOTPLUG_INT_STATUS ) - hotplug_en |=HDMID_HOTPLUG_INT_EN ; -if (IS_G4X (dev)) { -if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_G4X ) - hotplug_en |=SDVOC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_G4X ) - hotplug_en |=SDVOB_HOTPLUG_INT_EN ; - }else { -if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_I965 ) - hotplug_en |=SDVOC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_I965 ) - hotplug_en |=SDVOB_HOTPLUG_INT_EN ; - } -if (dev_priv->hotplug_supported_mask &CRT_HOTPLUG_INT_STATUS ) { - hotplug_en |=CRT_HOTPLUG_INT_EN ; - -/* Programming the CRT detection parameters tends -to generate a spurious hotplug event about three -seconds later. So just do it once. -*/ -if (IS_G4X (dev)) - hotplug_en |=CRT_HOTPLUG_ACTIVATION_PERIOD_64 ; - hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ; - } - -/* Ignore TV since it's buggy */ - -I915_WRITE (PORT_HOTPLUG_EN , hotplug_en); +i965_enable_hotplug_irq (dev);intel_opregion_enable_asle (dev);
Signature · 1 line
1.7.7
Re: [PATCH 5/8] drm/i915: Move hotplug interrupt enable for i915/i965/valleyview into a separate function.
Quoted 10 lines · On Thu, 10 Jan 2013 10:02:43 -0500 Egbert Eich <[email protected]> wrote:
Signed-off-by Egbert Eich <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 178 ++++++++++++++++++++++---------------- 1 files changed, 103 insertions(+), 75 deletions(-)drivers/gpu/drm/i915/i915_irq.c +0
-0
drivers/gpu/drm/i915/i915_irq.c static void gen6_queue_rps_work(struct drm_i915_private *dev_priv, · line 570
Quoted 4 lines
drivers/gpu/drm/i915/i915_irq.c +1
-0@@ -567,6 +567,32 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,queue_work (dev_priv->wq, &dev_priv->rps.work); } +static void valleyview_enable_hotplug_irq (struct drm_device *dev)
Given how you use these later, _reset_hotplug_irq or _update_hotplug_irq might be a better name, but that's no biggie.
Seems like a nice cleanup by itself even.
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
PATCH 6/8 drm/i915: Only enable hotplug irq when needed on Ironlake and later chips.
In reply to Egbert Eich--- drivers/gpu/drm/i915/i915_irq.c | 89 ++++++++++++++++++++------------------ 1 files changed, 47 insertions(+), 42 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +47 -42
@@ -1910,19 +1910,56 @@ static void valleyview_irq_preinstall(struct drm_device *dev)* This register is the same on all known PCH chips. */ -static void ironlake_enable_pch_hotplug (struct drm_device *dev) +static void ironlake_enable_pch_hotplug (struct drm_device *dev,bool is_cpt) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;u32 hotplug; hotplug =I915_READ (PCH_PORT_HOTPLUG ); - hotplug &= ~(PORTD_PULSE_DURATION_MASK |PORTC_PULSE_DURATION_MASK |PORTB_PULSE_DURATION_MASK ); - hotplug |=PORTD_HOTPLUG_ENABLE | PORTD_PULSE_DURATION_2ms; - hotplug |=PORTC_HOTPLUG_ENABLE | PORTC_PULSE_DURATION_2ms; - hotplug |=PORTB_HOTPLUG_ENABLE | PORTB_PULSE_DURATION_2ms; + hotplug &= ~(PORTD_HOTPLUG_ENABLE |PORTC_HOTPLUG_ENABLE |PORTB_HOTPLUG_ENABLE | +PORTD_PULSE_DURATION_MASK |PORTC_PULSE_DURATION_MASK |PORTB_PULSE_DURATION_MASK ); +if (dev_priv->hotplug_supported_mask & (is_cpt ?SDE_PORTD_HOTPLUG_CPT :SDE_PORTD_HOTPLUG )) + hotplug |=PORTD_HOTPLUG_ENABLE | PORTD_PULSE_DURATION_2ms; +if (dev_priv->hotplug_supported_mask & (is_cpt ?SDE_PORTC_HOTPLUG_CPT :SDE_PORTC_HOTPLUG )) + hotplug |=PORTC_HOTPLUG_ENABLE | PORTC_PULSE_DURATION_2ms; +if (dev_priv->hotplug_supported_mask & (is_cpt ?SDE_PORTB_HOTPLUG_CPT :SDE_PORTB_HOTPLUG )) + hotplug |=PORTB_HOTPLUG_ENABLE | PORTB_PULSE_DURATION_2ms;I915_WRITE (PCH_PORT_HOTPLUG , hotplug); } +static void cpt_enable_hotplug_irq (struct drm_device *dev) +{ +drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; +u32 pch_irq_mask; + + pch_irq_mask = ~dev_priv->hotplug_supported_mask; + +I915_WRITE (SDEIIR ,I915_READ (SDEIIR )); +I915_WRITE (SDEIMR , pch_irq_mask); +I915_WRITE (SDEIER , dev_priv->hotplug_supported_mask); +POSTING_READ (SDEIER ); + +ironlake_enable_pch_hotplug (dev,1 ); +} + +static void ibx_enable_hotplug_irq (struct drm_device *dev) +{ +drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; +u32 hotplug_mask; +u32 pch_irq_mask; + + hotplug_mask = (dev_priv->hotplug_supported_mask |SDE_AUX_MASK ); + + pch_irq_mask = ~hotplug_mask; + +I915_WRITE (SDEIIR ,I915_READ (SDEIIR )); +I915_WRITE (SDEIMR , pch_irq_mask); +I915_WRITE (SDEIER , hotplug_mask); +POSTING_READ (SDEIER ); + +ironlake_enable_pch_hotplug (dev,0 ); +} +static int ironlake_irq_postinstall (struct drm_device *dev) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
@@ -1930,8 +1967,6 @@ static int ironlake_irq_postinstall(struct drm_device *dev)u32 display_mask =DE_MASTER_IRQ_CONTROL |DE_GSE |DE_PCH_EVENT |DE_PLANEA_FLIP_DONE |DE_PLANEB_FLIP_DONE ;u32 render_irqs; -u32 hotplug_mask; -u32 pch_irq_mask; dev_priv->irq_mask = ~display_mask;
@@ -1959,27 +1994,10 @@ static int ironlake_irq_postinstall(struct drm_device *dev)I915_WRITE (GTIER , render_irqs);POSTING_READ (GTIER ); -if (HAS_PCH_CPT (dev)) { - hotplug_mask = (SDE_CRT_HOTPLUG_CPT | -SDE_PORTB_HOTPLUG_CPT | -SDE_PORTC_HOTPLUG_CPT | -SDE_PORTD_HOTPLUG_CPT ); - }else { - hotplug_mask = (SDE_CRT_HOTPLUG | -SDE_PORTB_HOTPLUG | -SDE_PORTC_HOTPLUG | -SDE_PORTD_HOTPLUG | -SDE_AUX_MASK ); - } - - pch_irq_mask = ~hotplug_mask; - -I915_WRITE (SDEIIR ,I915_READ (SDEIIR )); -I915_WRITE (SDEIMR , pch_irq_mask); -I915_WRITE (SDEIER , hotplug_mask); -POSTING_READ (SDEIER ); - -ironlake_enable_pch_hotplug (dev); +if (HAS_PCH_CPT (dev)) +cpt_enable_hotplug_irq (dev); +else +ibx_enable_hotplug_irq (dev);if (IS_IRONLAKE_M (dev)) {/* Clear & enable PCU event interrupts */
@@ -2001,8 +2019,6 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)DE_PLANEB_FLIP_DONE_IVB |DE_PLANEA_FLIP_DONE_IVB ;u32 render_irqs; -u32 hotplug_mask; -u32 pch_irq_mask; dev_priv->irq_mask = ~display_mask;
@@ -2026,18 +2042,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)I915_WRITE (GTIER , render_irqs);POSTING_READ (GTIER ); - hotplug_mask = (SDE_CRT_HOTPLUG_CPT | -SDE_PORTB_HOTPLUG_CPT | -SDE_PORTC_HOTPLUG_CPT | -SDE_PORTD_HOTPLUG_CPT ); - pch_irq_mask = ~hotplug_mask; - -I915_WRITE (SDEIIR ,I915_READ (SDEIIR )); -I915_WRITE (SDEIMR , pch_irq_mask); -I915_WRITE (SDEIER , hotplug_mask); -POSTING_READ (SDEIER ); - -ironlake_enable_pch_hotplug (dev); +cpt_enable_hotplug_irq (dev);return 0 ; }
Signature · 1 line
1.7.7
PATCH 7/8 drm/i915: When detecting a hotplug IRQ storm disable respective IRQs.
In reply to Egbert EichThis patch disables hotplug interrupts if an 'interrupt storm' has been detected.
Noise on the interrupt line renders the hotplug interrupt useless:
each hotplug event causes the devices to be rescanned which will will only increase the system load.
Thus disable the hotplug interrupts and fall back to periodic device polling.
--- drivers/gpu/drm/i915/i915_irq.c | 25 +++++++++++++++++++------ 1 files changed, 19 insertions(+), 6 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +19 -6
@@ -313,12 +313,13 @@ static void i915_hotplug_work_func(struct work_struct *work)drm_helper_hpd_irq_event (dev); } -static inline void hotplug_irq_storm_detect (struct drm_device *dev,u32 hotplug_trigger) +static inline int hotplug_irq_storm_detect (struct drm_device *dev,u32 hotplug_trigger) {struct drm_mode_config *mode_config = &dev->mode_config;drm_i915_private_t *dev_priv = dev->dev_private;struct drm_connector *connector;unsigned long irqflags; +int ret =0 ;spin_lock_irqsave (&dev_priv->irq_lock, irqflags);
@@ -335,12 +336,15 @@ static inline void hotplug_irq_storm_detect(struct drm_device *dev, u32 hotplug_ intel_connector->hpd_mark_disabled =1 ;pr_warn ("HPD interrupt storm on connector %s disabling\n" ,drm_get_connector_name (connector)); + ret =1 ; }else intel_connector->hpd_cnt++; } }spin_unlock_irqrestore (&dev_priv->irq_lock, irqflags); + +return ret; }/* defined intel_pm.c */
@@ -652,7 +656,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status);if (hotplug_trigger) { -hotplug_irq_storm_detect (dev, hotplug_trigger); +if (hotplug_irq_storm_detect (dev, hotplug_trigger)) +valleyview_enable_hotplug_irq (dev);queue_work (dev_priv->wq, &dev_priv->hotplug_work); }
@@ -675,6 +680,8 @@ out:return ret; } +static void ibx_enable_hotplug_irq (struct drm_device *dev); +static void ibx_irq_handler (struct drm_device *dev,u32 pch_iir) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
@@ -682,7 +689,8 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)u32 hotplug_trigger = pch_iir & dev_priv->hotplug_supported_mask;if (hotplug_trigger) { -hotplug_irq_storm_detect (dev, hotplug_trigger); +if (hotplug_irq_storm_detect (dev, hotplug_trigger)) +ibx_enable_hotplug_irq (dev);queue_work (dev_priv->wq, &dev_priv->hotplug_work); }if (pch_iir &SDE_AUDIO_POWER_MASK )
@@ -720,6 +728,8 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)DRM_DEBUG_DRIVER ("PCH transcoder A underrun interrupt\n" ); } +static void cpt_enable_hotplug_irq (struct drm_device *dev); +static void cpt_irq_handler (struct drm_device *dev,u32 pch_iir) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
@@ -727,7 +737,8 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)u32 hotplug_trigger = pch_iir & dev_priv->hotplug_supported_mask;if (hotplug_trigger) { -hotplug_irq_storm_detect (dev, hotplug_trigger); +if (hotplug_irq_storm_detect (dev, hotplug_trigger)) +cpt_enable_hotplug_irq (dev);queue_work (dev_priv->wq, &dev_priv->hotplug_work); }if (pch_iir &SDE_AUDIO_POWER_MASK_CPT )
@@ -2443,7 +2454,8 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status);if (hotplug_trigger) { -hotplug_irq_storm_detect (dev, hotplug_trigger); +if (hotplug_irq_storm_detect (dev, hotplug_trigger)) +i915_enable_hotplug_irq (dev);queue_work (dev_priv->wq, &dev_priv->hotplug_work); }
@@ -2694,7 +2706,8 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status);if (hotplug_trigger) { -hotplug_irq_storm_detect (dev, hotplug_trigger); +if (hotplug_irq_storm_detect (dev, hotplug_trigger)) +i965_enable_hotplug_irq (dev);queue_work (dev_priv->wq, &dev_priv->hotplug_work); }
Signature · 1 line
1.7.7
PATCH 8/8 drm/i915: Add Reenable Timer to turn Hotplug Detection back on.
In reply to Egbert EichWe disable hoptplug detection when we encounter a hotplug event storm. Still hotplug detection is required on some outputs (like Display Port). The interrupt storm may be only temporary (on certain Dell Laptops for instance it happens at certain charging states of the system). Thus we enable it after a certain grace period (2 minutes). Should the interrupt storm persist it will be detected immediately and it will be disabled again.
--- drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_irq.c | 58 ++++++++++++++++++++++++++++++++++++-- drivers/gpu/drm/i915/intel_drv.h | 6 +++- 3 files changed, 62 insertions(+), 4 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +2 -0
@@ -668,6 +668,8 @@ typedef struct drm_i915_private {u32 hotplug_supported_mask;struct work_struct hotplug_work; +#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000) +struct timer_list hotplug_reenable_timer;int num_pipe;int num_pch_pll;
drivers/gpu/drm/i915/i915_irq.c +55 -3
116 lines not on this page · raw message
drivers/gpu/drm/i915/intel_drv.h +5 -1
@@ -197,7 +197,11 @@ struct intel_connector {/* bookkeeping for irq storm detection */ int hpd_cnt;unsigned long last_hpd_jiffies; -bool hpd_mark_disabled; +enum { +HPD_ENABLED =0 , +HPD_MARKED_DISABLED , +HPD_DISABLED , + } hpd_mark_disabled; };struct intel_crtc {
Signature · 1 line
1.7.7
Quoted 33 lines · Egbert Eich
Despite the many attempts to fix the issue with noisy hotplug interrupt lines we are still seeing systems that suffer from this:
Recently we encountered a rather large scale installation of Q35 systems which was hit by this issue rather severely: It seemed as if not all machines of the same model were hit equally bad, in the worst cased hotplug interrupt noise caused several 1000 interrupts / s. Those machines would not even boot, instead the interrupt handler and the scheduled workers would keep the CPU busy that eventually the watchdog would kick in and issue an NMI. Other machines only received severa 10s to 100s of interrupts per sec - those machines would run properly - just with an excessive system load. More thorough investigations seemed to indicate that this condition only happen at certain video modes.On another system - a laptop - a hotplug interrupt 'storm' occurred when it was charging and the batteries were at certain charge levels. While the system was still running fine its load was high enough that the user noticed from the fan noise that a problem existed.
The latter system had a Sandybridge chipset, thus a totally different generation from the former.All those cases seemed to have been caused by cross talk on badly routed hotplug signal lines (or voltage instabilities).
This led to the conclusion that instead of trying to work around these 'storms' for each individual system, there should be a generic way to detect such a condition and take appropriate action:This patch series implements a hotplug 'storm' detection, disables the respective interrupt for the hotplug pin when this condition is detected and reverts to periodic output polling on the affected connector. After a grace period of 2 minutes it will reenable hotplug on the affected line. This will take care of cases in which this condition is only temporary. Should the 'storm' condtion persist, this cycle will start over again.
Quoted 5 lines
To implement this some rearrangements in the code were required:
- The interrupt status bit which signals a hotplug needed to be recorded for each connector.
- The interrupt enable functions needed to be separate, also they need to be able to enable interrupts for each hotplug line independently.
Nice work, and we know that we need this since quite a while. But unfortunately we've not yet come around to implement something. Some high-level comments on how I think this should best be handled:
- imo dv_priv->hotplug_supported_mask should die - it leaks platform specific irq magic from i915_irq.c into every connector/encoder. And we have had the bugs and confusions to prove that it's not a good idea. I think it'd be better if we add a new HOTPLUG_PIN_FOO enum that encoders register interest in, and the platform code in i915_irq.c then maps from/to that. On a quick check we have hotplug pins for CRT, TV, SDVO_B&C and PORT_A-D (for DP&HDMI).
Also note that on PCH_SPLIT platforms port A is not in the same register, further platforms will make an even cuter mess of this ...
- I think the the hpd pin should be track in the encoder, not in the connector. The only encoders where there's not a 1:1 relationship (sdvo and ddi on hsw) want it there. Also, we already have the ->hot_plug callback in the encoder, which will be useful for later extensions.
- Since some encoders share the same hpd pin (HDMI&DP on pre-hsw) I think we should keep the noise statistic data in the device's dev_priv somewhere in an array, with one set for each hpd pin from the enum above.
- In 3.8 the drm hpd/polling helpers are much improved and don't randomly poll everything any more. So if a hpd connector isn't marked as OUTPUT_POLL, it wont ever get polled. Which means if you disable the hpd irq for it, we need to have our own poll work to do that for us. The long-term goal I have is to pimp the encoder->hot_plug callback also for this case, to avoid re-running the connector detect code on unrelated outputs (which can sometimes cause havoc).
Eventually a want a hpd interrupt to only run the ->hot_plug callbacks on encoders which are interested in that signal, hence this slight overkill ... Ofc, that requires that we move a lot of the ->detect logic into ->hot_plug, but that's the only way to do sane EDID cache and similar things on outputs where hpd should work (DP/HDMI).
- The math buff in me would like hpd stroms to gracefully degrade into polling at 10s or so. We could achieve that with irq source masking and scheduling the work item to do the hotplug handling with an (increasing) delay if there's too many interrupts from a given hpd pin. But that requires that we can mask hotplug interrupts properly, which seems to be impossible with the PORT_HOTPLUG regs on gmch/SoC platforms :( So I think your logic is nice enough ;-)
Yours, Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Daniel,
Quoted 5 lines · Daniel Vetter
Nice work, and we know that we need this since quite a while. But unfortunately we've not yet come around to implement something. Some high-level comments on how I think this should best be handled:
Quoted 7 lines
- imo
dv_priv->hotplug_supported_maskshould die - it leaks platform specific irq magic fromi915_irq.cinto every connector/encoder. And we have had the bugs and confusions to prove that it's not a good idea. I think it'd be better if we add a newHOTPLUG_PIN_FOOenum that encoders register interest in, and the platform code ini915_irq.cthen maps from/to that. On a quick check we have hotplug pins for CRT, TV, SDVO_B&C and PORT_A-D (for DP&HDMI).
I thought along the same lines, I just didn't want to go quite as far. Therefore I added functions in i915_irq.c to set these depending on the connector.
Quoted 3 lines
Also note that on
PCH_SPLITplatforms port A is not in the same register, further platforms will make an even cuter mess of this ...
Ok, I will look into that.
Quoted 9 lines
- I think the the hpd pin should be track in the encoder, not in the connector. The only encoders where there's not a 1:1 relationship (sdvo and ddi on hsw) want it there. Also, we already have the ->hot_plug callback in the encoder, which will be useful for later extensions.
- Since some encoders share the same hpd pin (HDMI&DP on pre-hsw) I think we should keep the noise statistic data in the device's
dev_privsomewhere in an array, with one set for each hpd pin from the enum above.
This would also be an option. I did notice that these pins are shared, it didn't cause any issues as always both connectors got flagged simultaniously. On the other hand calling the same disable/enable twice when traversing the connector list is sorta ugly.
Quoted 8 lines
- In 3.8 the drm hpd/polling helpers are much improved and don't randomly poll everything any more. So if a hpd connector isn't marked as
OUTPUT_POLL, it wont ever get polled. Which means if you disable the hpd irq for it, we need to have our own poll work to do that for us. The long-term goal I have is to pimp theencoder->hot_plugcallback also for this case, to avoid re-running the connector detect code on unrelated outputs (which can sometimes cause havoc).
I do change the state of the 'polled' member when I disable/reenable hotplug interrupts already. This part therefore should work fine already.
Quoted 4 lines
Eventually a want a hpd interrupt to only run the ->hot_plug callbacks on encoders which are interested in that signal, hence this slight overkill ... Ofc, that requires that we move a lot of the ->detect logic
This was exactly my question: we have all information at hand now to do this and I can easily add this. The downside is that if the information about the mapping is not accurate (ie if a vendor routes HPD lines differently) this connector will never light up :(
As it is now since we poll everything when an interrupt happened we can be sure that we catch all connectors even if the mapping in our tables don't reflect what's wired on the board.
I didn't just go ahead and implement this yet as I've gotten too pessimistic.
Quoted 2 lines
into ->hot_plug, but that's the only way to do sane EDID cache and similar things on outputs where hpd should work (DP/HDMI).
... But since you suggest this I will gladly add this :)
Quoted 8 lines
- The math buff in me would like hpd stroms to gracefully degrade into polling at 10s or so. We could achieve that with irq source masking and scheduling the work item to do the hotplug handling with an (increasing) delay if there's too many interrupts from a given hpd pin. But that requires that we can mask hotplug interrupts properly, which seems to be impossible with the
PORT_HOTPLUGregs on gmch/SoC platforms :( So I think your logic is nice enough ;-)
What you suggest would be possible with some small changes to my code I guess. I just fear if we do have an IRQ storm 10s would be too short - on a completely idle system this might be the prime source of wakeups.
I believe I can spare some hours to think about and work in your suggestions.
Cheers,
Egbert.
Quoted 32 lines · Egbert Eich
Hi Daniel,
Quoted 5 lines · Daniel Vetter
Nice work, and we know that we need this since quite a while. But unfortunately we've not yet come around to implement something. Some high-level comments on how I think this should best be handled:
Quoted 7 lines
- imo
dv_priv->hotplug_supported_maskshould die - it leaks platform specific irq magic fromi915_irq.cinto every connector/encoder. And we have had the bugs and confusions to prove that it's not a good idea. I think it'd be better if we add a newHOTPLUG_PIN_FOOenum that encoders register interest in, and the platform code ini915_irq.cthen maps from/to that. On a quick check we have hotplug pins for CRT, TV, SDVO_B&C and PORT_A-D (for DP&HDMI).I thought along the same lines, I just didn't want to go quite as far. Therefore I added functions in
i915_irq.cto set these depending on the connector.Quoted 3 lines
Also note that on
PCH_SPLITplatforms port A is not in the same register, further platforms will make an even cuter mess of this ...Ok, I will look into that.
Quoted 6 lines
- I think the the hpd pin should be track in the encoder, not in the connector. The only encoders where there's not a 1:1 relationship (sdvo and ddi on hsw) want it there. Also, we already have the ->hot_plug callback in the encoder, which will be useful for later extensions.
Quoted 8 lines
Quoted 3 lines
- Since some encoders share the same hpd pin (HDMI&DP on pre-hsw) I think we should keep the noise statistic data in the device's
dev_privsomewhere in an array, with one set for each hpd pin from the enum above.This would also be an option. I did notice that these pins are shared, it didn't cause any issues as always both connectors got flagged simultaniously. On the other hand calling the same disable/enable twice when traversing the connector list is sorta ugly.
Yeah, I mostly want to have a clear 1:1 relationship between interrupt lines and the statistics about the noise on them ...
Quoted 10 lines
Quoted 7 lines
- In 3.8 the drm hpd/polling helpers are much improved and don't randomly poll everything any more. So if a hpd connector isn't marked as
OUTPUT_POLL, it wont ever get polled. Which means if you disable the hpd irq for it, we need to have our own poll work to do that for us. The long-term goal I have is to pimp theencoder->hot_plugcallback also for this case, to avoid re-running the connector detect code on unrelated outputs (which can sometimes cause havoc).I do change the state of the 'polled' member when I disable/reenable hotplug interrupts already. This part therefore should work fine already.
Hm, I've missed that, despite looking for it in the patches. One thing to note is that the poll work will disable itself if there's no connector with one of the POLL flags set in 3.8, so I think you need to kick it again when polling. Another thing to keep in mind is that we have encoders with POLL and HDP connectors (sometimes on the same one) - SDVO is the prime example since polling seems to work, but not too reliably. Hence we need the polling as a backup. To correctly restore those flags I guess we need a saved_polled variable in intel_connector which we need to restore when enabling the the hpd line again.
Quoted 4 lines
Quoted 3 lines
Eventually a want a hpd interrupt to only run the ->hot_plug callbacks on encoders which are interested in that signal, hence this slight overkill ... Ofc, that requires that we move a lot of the ->detect logic
Quoted 8 lines
This was exactly my question: we have all information at hand now to do this and I can easily add this. The downside is that if the information about the mapping is not accurate (ie if a vendor routes HPD lines differently) this connector will never light up :(
As it is now since we poll everything when an interrupt happened we can be sure that we catch all connectors even if the mapping in our tables don't reflect what's wired on the board.
I didn't just go ahead and implement this yet as I've gotten too pessimistic.
I share your pessimism, and we certainly need tons of special cases to make this work. E.g. the sdvo case, but also DP->VGA dongles where the forwarded hotplug events are as unreliable as plain VGA. And that exercise in only calling the right hpd handlers is only really useful if we cache the EDID, since userspace will do a full scan after it receives the hotplug event anyway. My idea is that the ->hotplug callback will then only invalidate the edid and we still do a full scan through all connector->detect callbacks. But for those outputs with reliably hdp we won't touch the hw (and so also optimize away the delays when userspace does the same afterwards). Once a hpd storm is detect and we switch to polling, we'd need to mark that output as unreliable to disable all edid caching.
We could even try to cache the edid for unreliable outputs like VGA for a short time ...
Quoted 4 lines
Quoted 2 lines
into ->hot_plug, but that's the only way to do sane EDID cache and similar things on outputs where hpd should work (DP/HDMI).
... But since you suggest this I will gladly add this :)
Imo better in a follow-up series, since there's quite some prep work involved. And I also think that it makes more sense to implement EDID caching first (which in turn requires some code to detect hpd irq storms ...).
Quoted 11 lines
Quoted 7 lines
- The math buff in me would like hpd stroms to gracefully degrade into polling at 10s or so. We could achieve that with irq source masking and scheduling the work item to do the hotplug handling with an (increasing) delay if there's too many interrupts from a given hpd pin. But that requires that we can mask hotplug interrupts properly, which seems to be impossible with the
PORT_HOTPLUGregs on gmch/SoC platforms :( So I think your logic is nice enough ;-)What you suggest would be possible with some small changes to my code I guess. I just fear if we do have an IRQ storm 10s would be too short - on a completely idle system this might be the prime source of wakeups.
I've misread your code and didn't realize that you rely on the output poll work for disabled hpd lines. I think the 2m delay in trying to re-enable outputs is more than fair enough. If we start to do fancy things with the DP/HDMI short pulses (i.e. reconfiguring downstream DP ports) we might need to reconsider the tuning values a bit. But the current values look sane to me.
Quoted 2 lines
I believe I can spare some hours to think about and work in your suggestions.
Awesome!
Cheers, Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Daniel,
I've played around a bit now, and implemented your suggestions:
Quoted 22 lines · Daniel Vetter
Quoted 21 lines · Egbert Eich
Hi Daniel,
Quoted 5 lines · Daniel Vetter
Nice work, and we know that we need this since quite a while. But unfortunately we've not yet come around to implement something. Some high-level comments on how I think this should best be handled:
Quoted 7 lines
- imo
dv_priv->hotplug_supported_maskshould die - it leaks platform specific irq magic fromi915_irq.cinto every connector/encoder. And we have had the bugs and confusions to prove that it's not a good idea. I think it'd be better if we add a newHOTPLUG_PIN_FOOenum that encoders register interest in, and the platform code ini915_irq.cthen maps from/to that. On a quick check we have hotplug pins for CRT, TV, SDVO_B&C and PORT_A-D (for DP&HDMI).I thought along the same lines, I just didn't want to go quite as far. Therefore I added functions in
i915_irq.cto set these depending on the connector.Quoted 1 line
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Also note that on
PCH_SPLITplatforms port A is not in the same register, further platforms will make an even cuter mess of this ...Ok, I will look into that.
So far I have not seen anywhere where there's hotplug support for PORT_A. PORT_A is marked as an internal connector without any HPD.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
- I think the the hpd pin should be track in the encoder, not in the connector. The only encoders where there's not a 1:1 relationship (sdvo and ddi on hsw) want it there. Also, we already have the ->hot_plug callback in the encoder, which will be useful for later extensions.
For SDVO it is definitely much simpler to track this in the encoder. However in the IRQ handling code we always need to take the detour thru the connector as the drm code expects any hotplog related information in the connector.
To get the connector we always have to walk thru the connector list and obtain the associated encoder. Walking thru the encoder list isn't sufficient as there is no easy way from encoder to connector. I don't have a strong preference either way - in the code I'm currently playing around with I keep this information in struct intel_encoder.
Quoted 5 lines
Quoted 5 lines
Quoted 4 lines
- Since some encoders share the same hpd pin (HDMI&DP on pre-hsw) I think we should keep the noise statistic data in the device's
dev_privsomewhere in an array, with one set for each hpd pin from the enum above.
Quoted 7 lines
Quoted 4 lines
This would also be an option. I did notice that these pins are shared, it didn't cause any issues as always both connectors got flagged simultaniously. On the other hand calling the same disable/enable twice when traversing the connector list is sorta ugly.
Yeah, I mostly want to have a clear 1:1 relationship between interrupt lines and the statistics about the noise on them ...
Yup. That's easy to do. In fact in my original prove of concept implementation I had it that way. However I was not sure if it's appreciated to add such bookkeeping in such a global fashion.
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
- In 3.8 the drm hpd/polling helpers are much improved and don't randomly poll everything any more. So if a hpd connector isn't marked as
OUTPUT_POLL, it wont ever get polled. Which means if you disable the hpd irq for it, we need to have our own poll work to do that for us. The long-term goal I have is to pimp theencoder->hot_plugcallback also for this case, to avoid re-running the connector detect code on unrelated outputs (which can sometimes cause havoc).
Quoted 7 lines
Quoted 2 lines
I do change the state of the 'polled' member when I disable/reenable hotplug interrupts already. This part therefore should work fine already.
Hm, I've missed that, despite looking for it in the patches. One thing to note is that the poll work will disable itself if there's no connector with one of the POLL flags set in 3.8, so I think you need to kick it again when polling. Another thing to keep in mind is that we have encoders
Exactly. This is something I had missed. It's however easily fixed by calling drm_kms_helper_poll_enable() when changing the settings.
Quoted 5 lines
with POLL and HDP connectors (sometimes on the same one) - SDVO is the prime example since polling seems to work, but not too reliably. Hence we need the polling as a backup. To correctly restore those flags I guess we need a
saved_polledvariable inintel_connectorwhich we need to restore when enabling the the hpd line again.
I don't see this in the code (drm-intel-testing pulled last Friday). On any connector it is either the DRM_CONNECTOR_POLL_HPD or the DRM_CONNECTOR_POLL_CONNECT (mostly with the DRM_CONNECTOR_POLL_DISCONNECT flag) set but not both.
Of course it could be done like you suggest, ie. continue polling despite waiting for interrupts, but this begs the question if we should not resort to polling entirely: the only benefit of doing HDP would be that we would get informed about an output change more quickly.
Quoted 33 lines
Quoted 5 lines
Quoted 4 lines
Eventually a want a hpd interrupt to only run the ->hot_plug callbacks on encoders which are interested in that signal, hence this slight overkill ... Ofc, that requires that we move a lot of the ->detect logic
Quoted 8 lines
This was exactly my question: we have all information at hand now to do this and I can easily add this. The downside is that if the information about the mapping is not accurate (ie if a vendor routes HPD lines differently) this connector will never light up :(
As it is now since we poll everything when an interrupt happened we can be sure that we catch all connectors even if the mapping in our tables don't reflect what's wired on the board.
I didn't just go ahead and implement this yet as I've gotten too pessimistic.I share your pessimism, and we certainly need tons of special cases to make this work. E.g. the sdvo case, but also
DP->VGAdongles where the forwarded hotplug events are as unreliable as plain VGA. And that exercise in only calling the right hpd handlers is only really useful if we cache the EDID, since userspace will do a full scan after it receives the hotplug event anyway. My idea is that the ->hotplug callback will then only invalidate the edid and we still do a full scan through allconnector->detectcallbacks. But for those outputs with reliably hdp we won't touch the hw (and so also optimize away the delays when userspace does the same afterwards). Once a hpd storm is detect and we switch to polling, we'd need to mark that output as unreliable to disable all edid caching.We could even try to cache the edid for unreliable outputs like VGA for a short time ...
Quoted 3 lines
Quoted 2 lines
into ->hot_plug, but that's the only way to do sane EDID cache and similar things on outputs where hpd should work (DP/HDMI).
Quoted 6 lines
Quoted 1 line
... But since you suggest this I will gladly add this :)
Imo better in a follow-up series, since there's quite some prep work involved. And I also think that it makes more sense to implement EDID caching first (which in turn requires some code to detect hpd irq storms ...).
I had sent a patch for EDID caching on the DRM level to Dave last December. I received some comments and suggestions from Ville from Intel which I had worked in - however I have not seen any reaction from Dave, yet.
Maybe you want to take a look at this. It cannot cache in all situations where caching would be useful and possible. It still should do a fairly good job of caching EDID extension blocks. This is because it currently lacks any driver interface and thus only can do as much as you can do without a deeper knowledge of what's going on on the hardware level.
On the other hand I believe I can add selective probing quite easily.
Quoted 13 lines
Quoted 9 lines
Quoted 8 lines
- The math buff in me would like hpd stroms to gracefully degrade into polling at 10s or so. We could achieve that with irq source masking and scheduling the work item to do the hotplug handling with an (increasing) delay if there's too many interrupts from a given hpd pin. But that requires that we can mask hotplug interrupts properly, which seems to be impossible with the
PORT_HOTPLUGregs on gmch/SoC platforms :( So I think your logic is nice enough ;-)Quoted 3 lines
What you suggest would be possible with some small changes to my code I guess. I just fear if we do have an IRQ storm 10s would be too short - on a completely idle system this might be the prime source of wakeups.
Quoted 6 lines
I've misread your code and didn't realize that you rely on the output poll work for disabled hpd lines. I think the 2m delay in trying to re-enable outputs is more than fair enough. If we start to do fancy things with the DP/HDMI short pulses (i.e. reconfiguring downstream DP ports) we might need to reconsider the tuning values a bit. But the current values look sane to me.
Ok. It was an ad-hock choice trying to maintain a blanace between systems which exhibit this behavior only under certain circumstances and others which have it more or less permanently.
Quoted 5 lines
Quoted 3 lines
I believe I can spare some hours to think about and work in your suggestions.
Awesome!
:)
Cheers,
Egbert.
Quoted 23 lines · Egbert Eich
Quoted 22 lines · Daniel Vetter
Quoted 21 lines · Egbert Eich
Hi Daniel,
Quoted 5 lines · Daniel Vetter
Nice work, and we know that we need this since quite a while. But unfortunately we've not yet come around to implement something. Some high-level comments on how I think this should best be handled:
Quoted 7 lines
- imo
dv_priv->hotplug_supported_maskshould die - it leaks platform specific irq magic fromi915_irq.cinto every connector/encoder. And we have had the bugs and confusions to prove that it's not a good idea. I think it'd be better if we add a newHOTPLUG_PIN_FOOenum that encoders register interest in, and the platform code ini915_irq.cthen maps from/to that. On a quick check we have hotplug pins for CRT, TV, SDVO_B&C and PORT_A-D (for DP&HDMI).I thought along the same lines, I just didn't want to go quite as far. Therefore I added functions in
i915_irq.cto set these depending on the connector.Quoted 1 line
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Also note that on
PCH_SPLITplatforms port A is not in the same register, further platforms will make an even cuter mess of this ...Ok, I will look into that.
So far I have not seen anywhere where there's hotplug support for
PORT_A.PORT_Ais marked as an internal connector without any HPD.
Hm, I've thought the hw supports short dp pulses on eDP port A in case the panel needs our attention, but maybe I've mixed that up with the dp aux irq for port A. In any case, that's easy to add if we need it.
Quoted 11 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
- I think the the hpd pin should be track in the encoder, not in the connector. The only encoders where there's not a 1:1 relationship (sdvo and ddi on hsw) want it there. Also, we already have the ->hot_plug callback in the encoder, which will be useful for later extensions.
For SDVO it is definitely much simpler to track this in the encoder. However in the IRQ handling code we always need to take the detour thru the connector as the drm code expects any hotplog related information in the connector.
Yeah, once we start filtering irq handling to only the ports affect, the current code will get messy. Which is why I think we should move the actual hpd handling into encoder->hotplug (and no longer call every ->detect callback). But I'm not too sure about it, and it can be done later on anyway.
Quoted 5 lines
To get the connector we always have to walk thru the connector list and obtain the associated encoder. Walking thru the encoder list isn't sufficient as there is no easy way from encoder to connector. I don't have a strong preference either way - in the code I'm currently playing around with I keep this information in struct
intel_encoder.
Can't we just loop over all connectors and before we update the hotplug state check whether the attached intel encoder is interested in the hpd irq we're processing? Or do I miss something here?
Quoted 6 lines
Quoted 5 lines
with POLL and HDP connectors (sometimes on the same one) - SDVO is the prime example since polling seems to work, but not too reliably. Hence we need the polling as a backup. To correctly restore those flags I guess we need a
saved_polledvariable inintel_connectorwhich we need to restore when enabling the the hpd line again.
Quoted 8 lines
I don't see this in the code (drm-intel-testing pulled last Friday). On any connector it is either the
DRM_CONNECTOR_POLL_HPDor theDRM_CONNECTOR_POLL_CONNECT(mostly with theDRM_CONNECTOR_POLL_DISCONNECTflag) set but not both.
Of course it could be done like you suggest, ie. continue polling despite waiting for interrupts, but this begs the question if we should not resort to polling entirely: the only benefit of doing HDP would be that we would get informed about an output change more quickly.
I mean that different connectors which use the same hpd line could use both polling and non-polling modes since there's only one hpd line per sdvo encoder. So we need to be a bit careful with which part of our detect code handles which pieces of hw and that we don't lose anything when switching modes.
Quoted 3 lines
I had sent a patch for EDID caching on the DRM level to Dave last December. I received some comments and suggestions from Ville from Intel which I had worked in - however I have not seen any reaction from Dave, yet.
Hm, missed that, I'll take a look again.
Quoted 5 lines
Maybe you want to take a look at this. It cannot cache in all situations where caching would be useful and possible. It still should do a fairly good job of caching EDID extension blocks. This is because it currently lacks any driver interface and thus only can do as much as you can do without a deeper knowledge of what's going on on the hardware level.
Atm I'm less sure about what we really should do wrt edid caching. One ugly problem is boot-up, where a bunch of userspace pieces all want to figure out the optimal configuration, but where it's pretty pointless to re-query to hw all the time. So I wonder whether we shouldn't have temporal caching of even unreliable outputs (e.g. a few seconds), with an option for drivers to invalidate the cache on e.g. a hpd interrupt.
Another ugly issue is when people disconnect outputs slowly so that we get the hpd interrupt _before_ the cable is fully unplugged. We've tried to fix those by checking the relevant hpd line state, but on noisy lines that won't really work out well (and we already have the regression bugs to prove it's a bad idea). So imo that entire caching/re-probing topic needs more thought.
Cheers, Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 5 lines · Daniel Vetter
On Tue, Jan 22, 2013 at 2:22 PM, Egbert Eich <[email protected]> wrote:
Hm, I've thought the hw supports short dp pulses on eDP port A in case the panel needs our attention, but maybe I've mixed that up with the dp aux irq for port A. In any case, that's easy to add if we need it.
Yes, of course.
Quoted 8 lines
Quoted 7 lines
For SDVO it is definitely much simpler to track this in the encoder. However in the IRQ handling code we always need to take the detour thru the connector as the drm code expects any hotplog related information in the connector.
Quoted 5 lines
Yeah, once we start filtering irq handling to only the ports affect, the current code will get messy. Which is why I think we should move the actual hpd handling into
encoder->hotplug(and no longer call every ->detect callback). But I'm not too sure about it, and it can be done later on anyway.
Yeah, this looks easily doable.
Quoted 10 lines
Quoted 6 lines
To get the connector we always have to walk thru the connector list and obtain the associated encoder. Walking thru the encoder list isn't sufficient as there is no easy way from encoder to connector. I don't have a strong preference either way - in the code I'm currently playing around with I keep this information in struct
intel_encoder.Can't we just loop over all connectors and before we update the hotplug state check whether the attached intel encoder is interested in the hpd irq we're processing? Or do I miss something here?
No, this is exactly how I've implemented it now.
Quoted 16 lines
Quoted 7 lines
Quoted 6 lines
with POLL and HDP connectors (sometimes on the same one) - SDVO is the prime example since polling seems to work, but not too reliably. Hence we need the polling as a backup. To correctly restore those flags I guess we need a
saved_polledvariable inintel_connectorwhich we need to restore when enabling the the hpd line again.Quoted 8 lines
I don't see this in the code (drm-intel-testing pulled last Friday). On any connector it is either the
DRM_CONNECTOR_POLL_HPDor theDRM_CONNECTOR_POLL_CONNECT(mostly with theDRM_CONNECTOR_POLL_DISCONNECTflag) set but not both.
Of course it could be done like you suggest, ie. continue polling despite waiting for interrupts, but this begs the question if we should not resort to polling entirely: the only benefit of doing HDP would be that we would get informed about an output change more quickly.
Quoted 5 lines
I mean that different connectors which use the same hpd line could use both polling and non-polling modes since there's only one hpd line per sdvo encoder. So we need to be a bit careful with which part of our detect code handles which pieces of hw and that we don't lose anything when switching modes.
Ah, ok. Got it, will think about it. Thanks!
Quoted 6 lines
Quoted 4 lines
I had sent a patch for EDID caching on the DRM level to Dave last December. I received some comments and suggestions from Ville from Intel which I had worked in - however I have not seen any reaction from Dave, yet.
Hm, missed that, I'll take a look again.
I had sent it on dri-devel. If you want I can send this to you again.
Quoted 7 lines
Quoted 6 lines
Maybe you want to take a look at this. It cannot cache in all situations where caching would be useful and possible. It still should do a fairly good job of caching EDID extension blocks. This is because it currently lacks any driver interface and thus only can do as much as you can do without a deeper knowledge of what's going on on the hardware level.
Quoted 6 lines
Atm I'm less sure about what we really should do wrt edid caching. One ugly problem is boot-up, where a bunch of userspace pieces all want to figure out the optimal configuration, but where it's pretty pointless to re-query to hw all the time. So I wonder whether we shouldn't have temporal caching of even unreliable outputs (e.g. a few seconds), with an option for drivers to invalidate the cache on e.g. a hpd interrupt.
Definitely. I don't think it is unreasonable to believe that once we have a valid EDID that this will remain valid either until we receive an event from the hardware telling us that something has changed (hotplug event) or we reprobe and find out that something has definitely changed or our reprobe timeout has expired.
If there's no valid EDID it is of course reasonable to reprobe at the earliest convenience: the situation where there's no EDID at all can be detected very quickly.
EDID readout is a slow undertaking and EDID extension make it even take longer.
Thus in my code I compare the EDID base block and if this hasn't changed I use data from the cache. This of course will only work if monitors are not able to dynamically reconfigure their EDID blocks.
Quoted 7 lines
Another ugly issue is when people disconnect outputs slowly so that we get the hpd interrupt _before_ the cable is fully unplugged. We've tried to fix those by checking the relevant hpd line state, but on noisy lines that won't really work out well (and we already have the regression bugs to prove it's a bad idea). So imo that entire caching/re-probing topic needs more thought.
Grr. Would it help to simply delay the worker a few tenth of a second so that this will happen less likely?
Cheers,
Egbert.
cover Detect and deal with Interrupt 'Storms' from noisy Hotplug Lines.
In reply to Daniel VetterI've reworked my 'hotplug interrupt storm detection'-patches and included most of Daniel's suggestions.
I've looked into adding EDID caching but since this requires some larger scale changes and some changes outside of the Intel driver it seemed to be a good idea to propose those changes at a later time.
Egbert Eich (12):
DRM/i915: Remove valleyview_hpd_irq_setup.
DRM/I915: Add enum hpd_pin to intel_encoder.
DRM/i915: Convert HPD interrupts to make use of HPD pin assignment in
encoders.
DRM/i915: Remove i965_hpd_irq_setup.
DRM/i915: Get rid if the 'hotplug_supported_mask' in struct
drm_i915_private.
DRM/i915: Add HPD IRQ storm detection.
DRM/i915: (re)init HPD interrupt storm statistics. DRM/i915: Treat hpd_irq_setup() for ironake and older generations the
same way.
DRM/i915: Disable HPD interrupt on pin when irq storm is detected. DRM/i915: Add Reenable Timer to turn Hotplug Detection back on. DRM/i915: Add bit field to record which pins have received HPD
events.
DRM/i915: Only reprobe display on encoder which has received an HPD
event.
drivers/gpu/drm/i915/i915_drv.h | 26 +++- drivers/gpu/drm/i915/i915_irq.c | 404 ++++++++++++++++++++++++++----------- drivers/gpu/drm/i915/i915_reg.h | 32 +++- drivers/gpu/drm/i915/intel_crt.c | 10 +- drivers/gpu/drm/i915/intel_dp.c | 8 +- drivers/gpu/drm/i915/intel_drv.h | 5 + drivers/gpu/drm/i915/intel_hdmi.c | 8 +- drivers/gpu/drm/i915/intel_sdvo.c | 17 +- drivers/gpu/drm/i915/intel_tv.c | 2 +- 9 files changed, 372 insertions(+), 140 deletions(-)
Signature · 1 line
1.7.7
PATCH 1/12 DRM/i915: Remove valleyview_hpd_irq_setup.
It's basically identical to i915_hpd_irq_setup().
--- drivers/gpu/drm/i915/i915_irq.c | 26 +------------------------- 1 files changed, 1 insertions(+), 25 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +1 -25
@@ -2113,30 +2113,6 @@ static int valleyview_irq_postinstall(struct drm_device *dev)return 0 ; } -static void valleyview_hpd_irq_setup (struct drm_device *dev) -{ -drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; -u32 hotplug_en =I915_READ (PORT_HOTPLUG_EN ); - -/* Note HDMI and DP share bits */ -if (dev_priv->hotplug_supported_mask &PORTB_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTB_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &PORTC_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &PORTD_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTD_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_I915 ) - hotplug_en |=SDVOC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_I915 ) - hotplug_en |=SDVOB_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &CRT_HOTPLUG_INT_STATUS ) { - hotplug_en |=CRT_HOTPLUG_INT_EN ; - hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ; - } - -I915_WRITE (PORT_HOTPLUG_EN , hotplug_en); -} -static void valleyview_irq_uninstall (struct drm_device *dev) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
@@ -2835,7 +2811,7 @@ void intel_irq_init(struct drm_device *dev) dev->driver->irq_uninstall = valleyview_irq_uninstall; dev->driver->enable_vblank = valleyview_enable_vblank; dev->driver->disable_vblank = valleyview_disable_vblank; - dev_priv->display.hpd_irq_setup = valleyview_hpd_irq_setup; + dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup; }else if (IS_IVYBRIDGE (dev) ||IS_HASWELL (dev)) {/* Share pre & uninstall handlers with ILK/SNB */ dev->driver->irq_handler = ivybridge_irq_handler;
Signature · 1 line
1.7.7
Re: [PATCH v.2 01/12] DRM/i915: Remove valleyview_hpd_irq_setup.
Quoted 43 lines · On Mon, 25 Feb 2013 12:06:48 -0500 Egbert Eich <[email protected]> wrote:
It's basically identical to
i915_hpd_irq_setup().Signed-off-by Egbert Eich <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 26 +------------------------- 1 files changed, 1 insertions(+), 25 deletions(-)drivers/gpu/drm/i915/i915_irq.c +0
-24@@ -2113,30 +2113,6 @@ static int valleyview_irq_postinstall(struct drm_device *dev)return 0 ; } -static void valleyview_hpd_irq_setup (struct drm_device *dev) -{ -drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; -u32 hotplug_en =I915_READ (PORT_HOTPLUG_EN ); - -/* Note HDMI and DP share bits */ -if (dev_priv->hotplug_supported_mask &PORTB_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTB_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &PORTC_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &PORTD_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTD_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_I915 ) - hotplug_en |=SDVOC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_I915 ) - hotplug_en |=SDVOB_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &CRT_HOTPLUG_INT_STATUS ) { - hotplug_en |=CRT_HOTPLUG_INT_EN ; - hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ; - } - -I915_WRITE (PORT_HOTPLUG_EN , hotplug_en); -} -static void valleyview_irq_uninstall (struct drm_device *dev) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
drivers/gpu/drm/i915/i915_irq.c void intel_irq_init(struct drm_device *dev) · line 2817
Quoted 8 lines
drivers/gpu/drm/i915/i915_irq.c +1
-1@@ -2835,7 +2811,7 @@ void intel_irq_init(struct drm_device *dev) dev->driver->irq_uninstall = valleyview_irq_uninstall; dev->driver->enable_vblank = valleyview_enable_vblank; dev->driver->disable_vblank = valleyview_disable_vblank; - dev_priv->display.hpd_irq_setup = valleyview_hpd_irq_setup; + dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup; }else if (IS_IVYBRIDGE (dev) ||IS_HASWELL (dev)) {/* Share pre & uninstall handlers with ILK/SNB */ dev->driver->irq_handler = ivybridge_irq_handler;
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
PATCH 2/12 DRM/I915: Add enum hpd_pin to intel_encoder.
In reply to Egbert EichTo clean up hotplug support we add a new enum to intel_encoder:
enum hpd_pin. It allows the encoder to request a hpd line but leave the details which IRQ is responsible on which chipset generation to i915_irq.c.
This way requesting hotplug support will become really simple on the encoder/connector level.
--- drivers/gpu/drm/i915/i915_drv.h | 13 +++++++++++++ drivers/gpu/drm/i915/intel_crt.c | 2 ++ drivers/gpu/drm/i915/intel_dp.c | 4 ++++ drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_hdmi.c | 4 ++++ drivers/gpu/drm/i915/intel_sdvo.c | 3 +++ 6 files changed, 27 insertions(+), 0 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +13 -0
@@ -86,6 +86,19 @@ enum port { };#define port_name (p) ((p) + 'A') +enum hpd_pin { +HPD_NONE =0 , +HPD_PORT_A =HPD_NONE ,/* PORT_A is internal */ +HPD_TV =HPD_NONE ,/* TV is known to be unreliable */ +HPD_CRT , +HPD_SDVO_B , +HPD_SDVO_C , +HPD_PORT_B , +HPD_PORT_C , +HPD_PORT_D , +HPD_NUM_PINS +}; +#define I915_GEM_GPU_DOMAINS \ (I915_GEM_DOMAIN_RENDER | \ I915_GEM_DOMAIN_SAMPLER | \
drivers/gpu/drm/i915/intel_crt.c +2 -0
@@ -777,6 +777,8 @@ void intel_crt_init(struct drm_device *dev) crt->base.disable = intel_disable_crt; crt->base.enable = intel_enable_crt; +if (I915_HAS_HOTPLUG (dev)) + crt->base.hpd_pin =HPD_CRT ;if (HAS_DDI (dev)) crt->base.get_hw_state = intel_ddi_get_hw_state;else
drivers/gpu/drm/i915/intel_dp.c +4 -0
@@ -2835,18 +2835,22 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,/* Set up the DDC bus. */ switch (port) {case PORT_A : + intel_encoder->hpd_pin =HPD_PORT_A ; name ="DPDDC-A" ;break ;case PORT_B : dev_priv->hotplug_supported_mask |=PORTB_HOTPLUG_INT_STATUS ; + intel_encoder->hpd_pin =HPD_PORT_B ; name ="DPDDC-B" ;break ;case PORT_C : dev_priv->hotplug_supported_mask |=PORTC_HOTPLUG_INT_STATUS ; + intel_encoder->hpd_pin =HPD_PORT_C ; name ="DPDDC-C" ;break ;case PORT_D : dev_priv->hotplug_supported_mask |=PORTD_HOTPLUG_INT_STATUS ; + intel_encoder->hpd_pin =HPD_PORT_D ; name ="DPDDC-D" ;break ;default :
drivers/gpu/drm/i915/intel_drv.h +1 -0
@@ -168,6 +168,7 @@ struct intel_encoder {* it is connected to in the pipe parameter. */ bool (*get_hw_state)(struct intel_encoder *,enum pipe *pipe);int crtc_mask; +enum hpd_pin hpd_pin; };struct intel_panel {
drivers/gpu/drm/i915/intel_hdmi.c +4 -0
@@ -1022,17 +1022,21 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,switch (port) {case PORT_B : intel_hdmi->ddc_bus =GMBUS_PORT_DPB ; + intel_encoder->hpd_pin =HPD_PORT_B ; dev_priv->hotplug_supported_mask |=PORTB_HOTPLUG_INT_STATUS ;break ;case PORT_C : intel_hdmi->ddc_bus =GMBUS_PORT_DPC ; + intel_encoder->hpd_pin =HPD_PORT_C ; dev_priv->hotplug_supported_mask |=PORTC_HOTPLUG_INT_STATUS ;break ;case PORT_D : intel_hdmi->ddc_bus =GMBUS_PORT_DPD ; + intel_encoder->hpd_pin =HPD_PORT_D ; dev_priv->hotplug_supported_mask |=PORTD_HOTPLUG_INT_STATUS ;break ;case PORT_A : + intel_encoder->hpd_pin =HPD_PORT_A ;/* Internal port only for eDP. */ default :BUG ();
drivers/gpu/drm/i915/intel_sdvo.c +3 -0
@@ -2780,6 +2780,9 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob) SDVOB_HOTPLUG_INT_STATUS_I915 :SDVOC_HOTPLUG_INT_STATUS_I915 ; } +if (intel_sdvo->hotplug_active) + intel_encoder->hpd_pin =HPD_SDVO_B ?HPD_SDVO_B :HPD_SDVO_C ; +drm_encoder_helper_add (&intel_encoder->base, &intel_sdvo_helper_funcs); intel_encoder->disable = intel_disable_sdvo;
Signature · 1 line
1.7.7
Re: [PATCH v.2 02/12] DRM/I915: Add enum hpd_pin to intel_encoder.
Quoted 8 lines · On Mon, 25 Feb 2013 12:06:49 -0500 Egbert Eich <[email protected]> wrote:
To clean up hotplug support we add a new enum to
intel_encoder:
enumhpd_pin. It allows the encoder to request a hpd line but leave the details which IRQ is responsible on which chipset generation toi915_irq.c.
This way requesting hotplug support will become really simple on the encoder/connector level.Signed-off-by Egbert Eich <[email protected]>
Quoted 8 lines
--- drivers/gpu/drm/i915/i915_drv.h | 13 +++++++++++++ drivers/gpu/drm/i915/intel_crt.c | 2 ++ drivers/gpu/drm/i915/intel_dp.c | 4 ++++ drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_hdmi.c | 4 ++++ drivers/gpu/drm/i915/intel_sdvo.c | 3 +++ 6 files changed, 27 insertions(+), 0 deletions(-)
Even better than the last one I suppose...
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
PATCH 3/12 DRM/i915: Convert HPD interrupts to make use of HPD pin assignment in encoders.
In reply to Egbert EichThis allows to enable HPD interrupts for individual pins to only receive hotplug events from lines which are connected and working.
--- drivers/gpu/drm/i915/i915_irq.c | 160 ++++++++++++++++++++++++-------------- drivers/gpu/drm/i915/i915_reg.h | 32 ++++++++- 2 files changed, 132 insertions(+), 60 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +101 -59
@@ -36,6 +36,68 @@#include "i915_trace.h" #include "intel_drv.h" +static const u32 hpd_ibx[] = { +0 ,/* HPD_NONE */ +SDE_CRT_HOTPLUG ,/* HPD_CRT */ +SDE_SDVOB_HOTPLUG ,/* HPD_SDVO_B */ +0 ,/* HPD_SDVO_C */ +SDE_PORTB_HOTPLUG ,/* HPD_PORT_B */ +SDE_PORTC_HOTPLUG ,/* HPD_PORT_C */ +SDE_PORTD_HOTPLUG /* HPD_PORT_D */ +}; + +static const u32 hpd_cpt[] = { +0 ,/* HPD_NONE */ +SDE_CRT_HOTPLUG_CPT ,/* HPD_CRT */ +0 ,/* HPD_SDVO_B */ +0 ,/* HPD_SDVO_C */ +SDE_PORTB_HOTPLUG_CPT ,/* HPD_PORT_B */ +SDE_PORTC_HOTPLUG_CPT ,/* HPD_PORT_C */ +SDE_PORTD_HOTPLUG_CPT /* HPD_PORT_D */ +}; + +static const u32 hpd_mask_i915[] = { +0 ,/* HPD_NONE */ +CRT_HOTPLUG_INT_EN ,/* HPD_CRT */ +SDVOB_HOTPLUG_INT_EN ,/* HPD_SDVO_B */ +SDVOC_HOTPLUG_INT_EN ,/* HPD_SDVO_C */ +PORTB_HOTPLUG_INT_EN ,/* HPD_PORT_B */ +PORTC_HOTPLUG_INT_EN ,/* HPD_PORT_C */ +PORTD_HOTPLUG_INT_EN /* HPD_PORT_D */ +}; + +static const u32 hpd_status_gen4[] = { +0 ,/* HPD_NONE */ +CRT_HOTPLUG_INT_STATUS ,/* HPD_CRT */ +SDVOB_HOTPLUG_INT_STATUS_G4X ,/* HPD_SDVO_B */ +SDVOC_HOTPLUG_INT_STATUS_G4X ,/* HPD_SDVO_C */ +PORTB_HOTPLUG_INT_STATUS ,/* HPD_PORT_B */ +PORTC_HOTPLUG_INT_STATUS ,/* HPD_PORT_C */ +PORTD_HOTPLUG_INT_STATUS /* HPD_PORT_D */ +}; + +static const u32 hpd_status_i965[] = { +0 ,/* HPD_NONE */ +CRT_HOTPLUG_INT_STATUS ,/* HPD_CRT */ +SDVOB_HOTPLUG_INT_STATUS_I965 ,/* HPD_SDVO_B */ +SDVOC_HOTPLUG_INT_STATUS_I965 ,/* HPD_SDVO_C */ +PORTB_HOTPLUG_INT_STATUS ,/* HPD_PORT_B */ +PORTC_HOTPLUG_INT_STATUS ,/* HPD_PORT_C */ +PORTD_HOTPLUG_INT_STATUS /* HPD_PORT_D */ +}; + +static const u32 hpd_status_i915[] = {/* i915 and valleyview are the same */ +0 ,/* HPD_NONE */ +CRT_HOTPLUG_INT_STATUS ,/* HPD_CRT */ +SDVOB_HOTPLUG_INT_STATUS_I915 ,/* HPD_SDVO_B */ +SDVOC_HOTPLUG_INT_STATUS_I915 ,/* HPD_SDVO_C */ +PORTB_HOTPLUG_INT_STATUS ,/* HPD_PORT_B */ +PORTC_HOTPLUG_INT_STATUS ,/* HPD_PORT_C */ +PORTD_HOTPLUG_INT_STATUS /* HPD_PORT_D */ +}; + + +/* For display hotplug interrupt */ static void ironlake_enable_display_irq (drm_i915_private_t *dev_priv,u32 mask)
@@ -596,7 +658,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status); -if (hotplug_status & dev_priv->hotplug_supported_mask) +if (hotplug_status &HOTPLUG_INT_STATUS_I915 )queue_work (dev_priv->wq, &dev_priv->hotplug_work);
@@ -1940,17 +2002,21 @@ static void ibx_enable_hotplug(struct drm_device *dev)static void ibx_irq_postinstall (struct drm_device *dev) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; -u32 mask; - -if (HAS_PCH_IBX (dev)) - mask =SDE_HOTPLUG_MASK | -SDE_GMBUS | -SDE_AUX_MASK ; -else - mask =SDE_HOTPLUG_MASK_CPT | -SDE_GMBUS_CPT | -SDE_AUX_MASK_CPT ; +struct drm_mode_config *mode_config = &dev->mode_config; +struct intel_encoder *intel_encoder; +u32 mask =I915_READ (SDEIER ); +if (HAS_PCH_IBX (dev)) { + mask &= ~SDE_HOTPLUG_MASK ; +list_for_each_entry (intel_encoder, &mode_config->encoder_list, base.head) + mask |= hpd_ibx[intel_encoder->hpd_pin]; + mask |=SDE_GMBUS |SDE_AUX_MASK ; + }else { + mask &= ~SDE_HOTPLUG_MASK_CPT ; +list_for_each_entry (intel_encoder, &mode_config->encoder_list, base.head) + mask |= hpd_cpt[intel_encoder->hpd_pin]; + mask |=SDE_GMBUS_CPT |SDE_AUX_MASK_CPT ; + }I915_WRITE (SDEIIR ,I915_READ (SDEIIR ));I915_WRITE (SDEIMR , ~mask);I915_WRITE (SDEIER , mask);
@@ -2360,26 +2426,16 @@ static int i915_irq_postinstall(struct drm_device *dev)static void i915_hpd_irq_setup (struct drm_device *dev) { -drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; -u32 hotplug_en; -if (I915_HAS_HOTPLUG (dev)) { - hotplug_en =I915_READ (PORT_HOTPLUG_EN ); +drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; +struct drm_mode_config *mode_config = &dev->mode_config; +struct intel_encoder *encoder; +u32 hotplug_en =I915_READ (PORT_HOTPLUG_EN ); -if (dev_priv->hotplug_supported_mask &PORTB_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTB_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &PORTC_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &PORTD_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTD_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_I915 ) - hotplug_en |=SDVOC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_I915 ) - hotplug_en |=SDVOB_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &CRT_HOTPLUG_INT_STATUS ) { - hotplug_en |=CRT_HOTPLUG_INT_EN ; - hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ; - } + hotplug_en &= ~HOTPLUG_INT_EN_MASK ; +list_for_each_entry (encoder, &mode_config->encoder_list, base.head) + hotplug_en |= hpd_mask_i915[encoder->hpd_pin]; + hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ;/* Ignore TV since it's buggy */
@@ -2443,7 +2499,7 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status); -if (hotplug_status & dev_priv->hotplug_supported_mask) +if (hotplug_status &HOTPLUG_INT_STATUS_I915 )queue_work (dev_priv->wq, &dev_priv->hotplug_work);
@@ -2596,38 +2652,22 @@ static int i965_irq_postinstall(struct drm_device *dev)static void i965_hpd_irq_setup (struct drm_device *dev) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; +struct drm_mode_config *mode_config = &dev->mode_config; +struct intel_encoder *encoder;u32 hotplug_en;/* Note HDMI and DP share hotplug bits */ hotplug_en =0 ; -if (dev_priv->hotplug_supported_mask &PORTB_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTB_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &PORTC_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &PORTD_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTD_HOTPLUG_INT_EN ; -if (IS_G4X (dev)) { -if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_G4X ) - hotplug_en |=SDVOC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_G4X ) - hotplug_en |=SDVOB_HOTPLUG_INT_EN ; - }else { -if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_I965 ) - hotplug_en |=SDVOC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_I965 ) - hotplug_en |=SDVOB_HOTPLUG_INT_EN ; - } -if (dev_priv->hotplug_supported_mask &CRT_HOTPLUG_INT_STATUS ) { - hotplug_en |=CRT_HOTPLUG_INT_EN ; - -/* Programming the CRT detection parameters tends -to generate a spurious hotplug event about three -seconds later. So just do it once. -*/ -if (IS_G4X (dev)) - hotplug_en |=CRT_HOTPLUG_ACTIVATION_PERIOD_64 ; - hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ; - } +list_for_each_entry (encoder, &mode_config->encoder_list, base.head) +/* enable bits are the same for all generations */ + hotplug_en |= hpd_mask_i915[encoder->hpd_pin]; +/* Programming the CRT detection parameters tends +to generate a spurious hotplug event about three +seconds later. So just do it once. +*/ +if (IS_G4X (dev)) + hotplug_en |=CRT_HOTPLUG_ACTIVATION_PERIOD_64 ; + hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ;/* Ignore TV since it's buggy */
@@ -2690,7 +2730,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status); -if (hotplug_status & dev_priv->hotplug_supported_mask) +if (hotplug_status & (IS_G4X (dev) ? +HOTPLUG_INT_STATUS_G4X : +HOTPLUG_INT_STATUS_I965 ))queue_work (dev_priv->wq, &dev_priv->hotplug_work);
drivers/gpu/drm/i915/i915_reg.h +31 -1
@@ -1637,6 +1637,12 @@#define SDVOC_HOTPLUG_INT_EN (1 << 25)#define TV_HOTPLUG_INT_EN (1 << 18)#define CRT_HOTPLUG_INT_EN (1 << 9) +#define HOTPLUG_INT_EN_MASK (PORTB_HOTPLUG_INT_EN | \ + PORTC_HOTPLUG_INT_EN | \ + PORTD_HOTPLUG_INT_EN | \ + SDVOC_HOTPLUG_INT_EN | \ + SDVOB_HOTPLUG_INT_EN | \ + CRT_HOTPLUG_INT_EN)#define CRT_HOTPLUG_FORCE_DETECT (1 << 3)#define CRT_HOTPLUG_ACTIVATION_PERIOD_32 (0 << 8)/* must use period 64 on GM45 according to docs */
@@ -1675,6 +1681,26 @@#define SDVOB_HOTPLUG_INT_STATUS_I965 (3 << 2)#define SDVOC_HOTPLUG_INT_STATUS_I915 (1 << 7)#define SDVOB_HOTPLUG_INT_STATUS_I915 (1 << 6) +#define HOTPLUG_INT_STATUS_G4X (CRT_HOTPLUG_INT_STATUS | \ + SDVOB_HOTPLUG_INT_STATUS_G4X | \ + SDVOC_HOTPLUG_INT_STATUS_G4X | \ + PORTB_HOTPLUG_INT_STATUS | \ + PORTC_HOTPLUG_INT_STATUS | \ + PORTD_HOTPLUG_INT_STATUS) + +#define HOTPLUG_INT_STATUS_I965 (CRT_HOTPLUG_INT_STATUS | \ + SDVOB_HOTPLUG_INT_STATUS_I965 | \ + SDVOC_HOTPLUG_INT_STATUS_I965 | \ + PORTB_HOTPLUG_INT_STATUS | \ + PORTC_HOTPLUG_INT_STATUS | \ + PORTD_HOTPLUG_INT_STATUS) + +#define HOTPLUG_INT_STATUS_I915 (CRT_HOTPLUG_INT_STATUS | \ + SDVOB_HOTPLUG_INT_STATUS_I915 | \ + SDVOC_HOTPLUG_INT_STATUS_I915 | \ + PORTB_HOTPLUG_INT_STATUS | \ + PORTC_HOTPLUG_INT_STATUS | \ + PORTD_HOTPLUG_INT_STATUS)/* SDVO port control */ #define SDVOB 0x61140
@@ -3508,7 +3534,11 @@#define SDE_PORTC_HOTPLUG (1 << 9)#define SDE_PORTB_HOTPLUG (1 << 8)#define SDE_SDVOB_HOTPLUG (1 << 6) -#define SDE_HOTPLUG_MASK (0xf << 8) +#define SDE_HOTPLUG_MASK (SDE_CRT_HOTPLUG | \ + SDE_SDVOB_HOTPLUG | \ + SDE_PORTB_HOTPLUG | \ + SDE_PORTC_HOTPLUG | \ + SDE_PORTD_HOTPLUG)#define SDE_TRANSB_CRC_DONE (1 << 5)#define SDE_TRANSB_CRC_ERR (1 << 4)#define SDE_TRANSB_FIFO_UNDER (1 << 3)
Signature · 1 line
1.7.7
Re: [PATCH v.2 03/12] DRM/i915: Convert HPD interrupts to make use of HPD pin assignment in encoders.
Quoted 16 lines · Egbert Eich
This allows to enable HPD interrupts for individual pins to only receive hotplug events from lines which are connected and working.
Signed-off-by Egbert Eich <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 160 ++++++++++++++++++++++++-------------- drivers/gpu/drm/i915/i915_reg.h | 32 ++++++++- 2 files changed, 132 insertions(+), 60 deletions(-)drivers/gpu/drm/i915/i915_irq.c +0
-0@@ -36,6 +36,68 @@#include "i915_trace.h" #include "intel_drv.h"
drivers/gpu/drm/i915/i915_irq.c line 47
Quoted 10 lines
drivers/gpu/drm/i915/i915_irq.c +9
-0+static const u32 hpd_ibx[] = { +0 ,/* HPD_NONE */ +SDE_CRT_HOTPLUG ,/* HPD_CRT */ +SDE_SDVOB_HOTPLUG ,/* HPD_SDVO_B */ +0 ,/* HPD_SDVO_C */ +SDE_PORTB_HOTPLUG ,/* HPD_PORT_B */ +SDE_PORTC_HOTPLUG ,/* HPD_PORT_C */ +SDE_PORTD_HOTPLUG /* HPD_PORT_D */ +};
Trivial bikeshed, these would look neater as
static const u32 hpd_ibx[] = {
[HPD_CRT] = SDE_CRTC_HOTPLUG,
[HPD_SDVO_B] = SDE_SDVOB_HOTPLUG,
[HPD_PORT_B] = SDE_PORTB_HOTPLUG,
[HPD_PORT_C] = SDE_PORTC_HOTPLUG,
[HPD_PORT_D] = SDE_PORTD_HOTPLUG,
};
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
PATCH 3/12 DRM/i915: Convert HPD interrupts to make use of HPD pin assignment in encoders (v2)
This allows to enable HPD interrupts for individual pins to only receive hotplug events from lines which are connected and working.
v2: Restructured initailization of const arrays following a suggstion
by Chris Wilson <[email protected]>
--- drivers/gpu/drm/i915/i915_irq.c | 151 ++++++++++++++++++++++++--------------- drivers/gpu/drm/i915/i915_reg.h | 32 ++++++++- 2 files changed, 123 insertions(+), 60 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +92 -59
@@ -36,6 +36,59 @@#include "i915_trace.h" #include "intel_drv.h" +static const u32 hpd_ibx[] = { + [HPD_CRT ] =SDE_CRT_HOTPLUG , + [HPD_SDVO_B ] =SDE_SDVOB_HOTPLUG , + [HPD_PORT_B ] =SDE_PORTB_HOTPLUG , + [HPD_PORT_C ] =SDE_PORTC_HOTPLUG , + [HPD_PORT_D ] =SDE_PORTD_HOTPLUG +}; + +static const u32 hpd_cpt[] = { + [HPD_CRT ] =SDE_CRT_HOTPLUG_CPT , + [HPD_PORT_B ] =SDE_PORTB_HOTPLUG_CPT , + [HPD_PORT_C ] =SDE_PORTC_HOTPLUG_CPT , + [HPD_PORT_D ] =SDE_PORTD_HOTPLUG_CPT +}; + +static const u32 hpd_mask_i915[] = { + [HPD_CRT ] =CRT_HOTPLUG_INT_EN , + [HPD_SDVO_B ] =SDVOB_HOTPLUG_INT_EN , + [HPD_SDVO_C ] =SDVOC_HOTPLUG_INT_EN , + [HPD_PORT_B ] =PORTB_HOTPLUG_INT_EN , + [HPD_PORT_C ] =PORTC_HOTPLUG_INT_EN , + [HPD_PORT_D ] =PORTD_HOTPLUG_INT_EN +}; + +static const u32 hpd_status_gen4[] = { + [HPD_CRT ] =CRT_HOTPLUG_INT_STATUS , + [HPD_SDVO_B ] =SDVOB_HOTPLUG_INT_STATUS_G4X , + [HPD_SDVO_C ] =SDVOC_HOTPLUG_INT_STATUS_G4X , + [HPD_PORT_B ] =PORTB_HOTPLUG_INT_STATUS , + [HPD_PORT_C ] =PORTC_HOTPLUG_INT_STATUS , + [HPD_PORT_D ] =PORTD_HOTPLUG_INT_STATUS +}; + +static const u32 hpd_status_i965[] = { + [HPD_CRT ] =CRT_HOTPLUG_INT_STATUS , + [HPD_SDVO_B ] =SDVOB_HOTPLUG_INT_STATUS_I965 , + [HPD_SDVO_C ] =SDVOC_HOTPLUG_INT_STATUS_I965 , + [HPD_PORT_B ] =PORTB_HOTPLUG_INT_STATUS , + [HPD_PORT_C ] =PORTC_HOTPLUG_INT_STATUS , + [HPD_PORT_D ] =PORTD_HOTPLUG_INT_STATUS +}; + +static const u32 hpd_status_i915[] = {/* i915 and valleyview are the same */ + [HPD_CRT ] =CRT_HOTPLUG_INT_STATUS , + [HPD_SDVO_B ] =SDVOB_HOTPLUG_INT_STATUS_I915 , + [HPD_SDVO_C ] =SDVOC_HOTPLUG_INT_STATUS_I915 , + [HPD_PORT_B ] =PORTB_HOTPLUG_INT_STATUS , + [HPD_PORT_C ] =PORTC_HOTPLUG_INT_STATUS , + [HPD_PORT_D ] =PORTD_HOTPLUG_INT_STATUS +}; + + +/* For display hotplug interrupt */ static void ironlake_enable_display_irq (drm_i915_private_t *dev_priv,u32 mask)
@@ -596,7 +649,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status); -if (hotplug_status & dev_priv->hotplug_supported_mask) +if (hotplug_status &HOTPLUG_INT_STATUS_I915 )queue_work (dev_priv->wq, &dev_priv->hotplug_work);
@@ -1940,17 +1993,21 @@ static void ibx_enable_hotplug(struct drm_device *dev)static void ibx_irq_postinstall (struct drm_device *dev) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; -u32 mask; - -if (HAS_PCH_IBX (dev)) - mask =SDE_HOTPLUG_MASK | -SDE_GMBUS | -SDE_AUX_MASK ; -else - mask =SDE_HOTPLUG_MASK_CPT | -SDE_GMBUS_CPT | -SDE_AUX_MASK_CPT ; +struct drm_mode_config *mode_config = &dev->mode_config; +struct intel_encoder *intel_encoder; +u32 mask =I915_READ (SDEIER ); +if (HAS_PCH_IBX (dev)) { + mask &= ~SDE_HOTPLUG_MASK ; +list_for_each_entry (intel_encoder, &mode_config->encoder_list, base.head) + mask |= hpd_ibx[intel_encoder->hpd_pin]; + mask |=SDE_GMBUS |SDE_AUX_MASK ; + }else { + mask &= ~SDE_HOTPLUG_MASK_CPT ; +list_for_each_entry (intel_encoder, &mode_config->encoder_list, base.head) + mask |= hpd_cpt[intel_encoder->hpd_pin]; + mask |=SDE_GMBUS_CPT |SDE_AUX_MASK_CPT ; + }I915_WRITE (SDEIIR ,I915_READ (SDEIIR ));I915_WRITE (SDEIMR , ~mask);I915_WRITE (SDEIER , mask);
@@ -2360,26 +2417,16 @@ static int i915_irq_postinstall(struct drm_device *dev)static void i915_hpd_irq_setup (struct drm_device *dev) { -drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; -u32 hotplug_en; -if (I915_HAS_HOTPLUG (dev)) { - hotplug_en =I915_READ (PORT_HOTPLUG_EN ); +drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; +struct drm_mode_config *mode_config = &dev->mode_config; +struct intel_encoder *encoder; +u32 hotplug_en =I915_READ (PORT_HOTPLUG_EN ); -if (dev_priv->hotplug_supported_mask &PORTB_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTB_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &PORTC_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &PORTD_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTD_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_I915 ) - hotplug_en |=SDVOC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_I915 ) - hotplug_en |=SDVOB_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &CRT_HOTPLUG_INT_STATUS ) { - hotplug_en |=CRT_HOTPLUG_INT_EN ; - hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ; - } + hotplug_en &= ~HOTPLUG_INT_EN_MASK ; +list_for_each_entry (encoder, &mode_config->encoder_list, base.head) + hotplug_en |= hpd_mask_i915[encoder->hpd_pin]; + hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ;/* Ignore TV since it's buggy */
@@ -2443,7 +2490,7 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status); -if (hotplug_status & dev_priv->hotplug_supported_mask) +if (hotplug_status &HOTPLUG_INT_STATUS_I915 )queue_work (dev_priv->wq, &dev_priv->hotplug_work);
@@ -2596,38 +2643,22 @@ static int i965_irq_postinstall(struct drm_device *dev)static void i965_hpd_irq_setup (struct drm_device *dev) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; +struct drm_mode_config *mode_config = &dev->mode_config; +struct intel_encoder *encoder;u32 hotplug_en;/* Note HDMI and DP share hotplug bits */ hotplug_en =0 ; -if (dev_priv->hotplug_supported_mask &PORTB_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTB_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &PORTC_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &PORTD_HOTPLUG_INT_STATUS ) - hotplug_en |=PORTD_HOTPLUG_INT_EN ; -if (IS_G4X (dev)) { -if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_G4X ) - hotplug_en |=SDVOC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_G4X ) - hotplug_en |=SDVOB_HOTPLUG_INT_EN ; - }else { -if (dev_priv->hotplug_supported_mask &SDVOC_HOTPLUG_INT_STATUS_I965 ) - hotplug_en |=SDVOC_HOTPLUG_INT_EN ; -if (dev_priv->hotplug_supported_mask &SDVOB_HOTPLUG_INT_STATUS_I965 ) - hotplug_en |=SDVOB_HOTPLUG_INT_EN ; - } -if (dev_priv->hotplug_supported_mask &CRT_HOTPLUG_INT_STATUS ) { - hotplug_en |=CRT_HOTPLUG_INT_EN ; - -/* Programming the CRT detection parameters tends -to generate a spurious hotplug event about three -seconds later. So just do it once. -*/ -if (IS_G4X (dev)) - hotplug_en |=CRT_HOTPLUG_ACTIVATION_PERIOD_64 ; - hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ; - } +list_for_each_entry (encoder, &mode_config->encoder_list, base.head) +/* enable bits are the same for all generations */ + hotplug_en |= hpd_mask_i915[encoder->hpd_pin]; +/* Programming the CRT detection parameters tends +to generate a spurious hotplug event about three +seconds later. So just do it once. +*/ +if (IS_G4X (dev)) + hotplug_en |=CRT_HOTPLUG_ACTIVATION_PERIOD_64 ; + hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ;/* Ignore TV since it's buggy */
@@ -2690,7 +2721,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status); -if (hotplug_status & dev_priv->hotplug_supported_mask) +if (hotplug_status & (IS_G4X (dev) ? +HOTPLUG_INT_STATUS_G4X : +HOTPLUG_INT_STATUS_I965 ))queue_work (dev_priv->wq, &dev_priv->hotplug_work);
drivers/gpu/drm/i915/i915_reg.h +31 -1
@@ -1637,6 +1637,12 @@#define SDVOC_HOTPLUG_INT_EN (1 << 25)#define TV_HOTPLUG_INT_EN (1 << 18)#define CRT_HOTPLUG_INT_EN (1 << 9) +#define HOTPLUG_INT_EN_MASK (PORTB_HOTPLUG_INT_EN | \ + PORTC_HOTPLUG_INT_EN | \ + PORTD_HOTPLUG_INT_EN | \ + SDVOC_HOTPLUG_INT_EN | \ + SDVOB_HOTPLUG_INT_EN | \ + CRT_HOTPLUG_INT_EN)#define CRT_HOTPLUG_FORCE_DETECT (1 << 3)#define CRT_HOTPLUG_ACTIVATION_PERIOD_32 (0 << 8)/* must use period 64 on GM45 according to docs */
@@ -1675,6 +1681,26 @@#define SDVOB_HOTPLUG_INT_STATUS_I965 (3 << 2)#define SDVOC_HOTPLUG_INT_STATUS_I915 (1 << 7)#define SDVOB_HOTPLUG_INT_STATUS_I915 (1 << 6) +#define HOTPLUG_INT_STATUS_G4X (CRT_HOTPLUG_INT_STATUS | \ + SDVOB_HOTPLUG_INT_STATUS_G4X | \ + SDVOC_HOTPLUG_INT_STATUS_G4X | \ + PORTB_HOTPLUG_INT_STATUS | \ + PORTC_HOTPLUG_INT_STATUS | \ + PORTD_HOTPLUG_INT_STATUS) + +#define HOTPLUG_INT_STATUS_I965 (CRT_HOTPLUG_INT_STATUS | \ + SDVOB_HOTPLUG_INT_STATUS_I965 | \ + SDVOC_HOTPLUG_INT_STATUS_I965 | \ + PORTB_HOTPLUG_INT_STATUS | \ + PORTC_HOTPLUG_INT_STATUS | \ + PORTD_HOTPLUG_INT_STATUS) + +#define HOTPLUG_INT_STATUS_I915 (CRT_HOTPLUG_INT_STATUS | \ + SDVOB_HOTPLUG_INT_STATUS_I915 | \ + SDVOC_HOTPLUG_INT_STATUS_I915 | \ + PORTB_HOTPLUG_INT_STATUS | \ + PORTC_HOTPLUG_INT_STATUS | \ + PORTD_HOTPLUG_INT_STATUS)/* SDVO port control */ #define SDVOB 0x61140
@@ -3508,7 +3534,11 @@#define SDE_PORTC_HOTPLUG (1 << 9)#define SDE_PORTB_HOTPLUG (1 << 8)#define SDE_SDVOB_HOTPLUG (1 << 6) -#define SDE_HOTPLUG_MASK (0xf << 8) +#define SDE_HOTPLUG_MASK (SDE_CRT_HOTPLUG | \ + SDE_SDVOB_HOTPLUG | \ + SDE_PORTB_HOTPLUG | \ + SDE_PORTC_HOTPLUG | \ + SDE_PORTD_HOTPLUG)#define SDE_TRANSB_CRC_DONE (1 << 5)#define SDE_TRANSB_CRC_ERR (1 << 4)#define SDE_TRANSB_FIFO_UNDER (1 << 3)
Signature · 1 line
1.7.7
Re: [PATCH v.2 03/12] DRM/i915: Convert HPD interrupts to make use of HPD pin assignment in encoders.
In reply to Egbert EichQuoted 5 lines · On Mon, 25 Feb 2013 12:06:50 -0500 Egbert Eich <[email protected]> wrote:
This allows to enable HPD interrupts for individual pins to only receive hotplug events from lines which are connected and working.
Signed-off-by Egbert Eich <[email protected]>---
In reviewing the old one, I thought tables might be nice, but on seeing it I'm not sure. I think they're about the same. So I'm ok with either.
Signature · 1 line
Jesse Barnes, Intel Open Source Technology Center
PATCH 4/12 DRM/i915: Remove i965_hpd_irq_setup.
In reply to Egbert EichAfter
"Convert HPD interrupts to make use of HPD pin assignment in encoders." This function is now basically the same as i915_hpd_irq_setup().
Consolidating both functions in one requires one more check for I915_HAS_HOTPLUG(dev) in the i965 code path and one more check for IS_G4X(dev) in the i915 code path. These are considered harmless.
--- drivers/gpu/drm/i915/i915_irq.c | 53 +++++++++++++------------------------- 1 files changed, 18 insertions(+), 35 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +18 -35
@@ -2424,25 +2424,6 @@ static int i915_irq_postinstall(struct drm_device *dev)return 0 ; } -static void i915_hpd_irq_setup (struct drm_device *dev) -{ -if (I915_HAS_HOTPLUG (dev)) { -drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; -struct drm_mode_config *mode_config = &dev->mode_config; -struct intel_encoder *encoder; -u32 hotplug_en =I915_READ (PORT_HOTPLUG_EN ); - - hotplug_en &= ~HOTPLUG_INT_EN_MASK ; -list_for_each_entry (encoder, &mode_config->encoder_list, base.head) - hotplug_en |= hpd_mask_i915[encoder->hpd_pin]; - hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ; - -/* Ignore TV since it's buggy */ - -I915_WRITE (PORT_HOTPLUG_EN , hotplug_en); - } -} -static irqreturn_t i915_irq_handler (int irq,void *arg) {struct drm_device *dev = (struct drm_device *) arg;
@@ -2649,29 +2630,31 @@ static int i965_irq_postinstall(struct drm_device *dev)return 0 ; } -static void i965_hpd_irq_setup (struct drm_device *dev) +static void i915_hpd_irq_setup (struct drm_device *dev) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;struct drm_mode_config *mode_config = &dev->mode_config;struct intel_encoder *encoder;u32 hotplug_en; -/* Note HDMI and DP share hotplug bits */ - hotplug_en =0 ; -list_for_each_entry (encoder, &mode_config->encoder_list, base.head) +if (I915_HAS_HOTPLUG (dev)) { + hotplug_en =I915_READ (PORT_HOTPLUG_EN ); + hotplug_en &= ~HOTPLUG_INT_EN_MASK ; +/* Note HDMI and DP share hotplug bits */ /* enable bits are the same for all generations */ - hotplug_en |= hpd_mask_i915[encoder->hpd_pin]; -/* Programming the CRT detection parameters tends -to generate a spurious hotplug event about three -seconds later. So just do it once. -*/ -if (IS_G4X (dev)) - hotplug_en |=CRT_HOTPLUG_ACTIVATION_PERIOD_64 ; - hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ; - -/* Ignore TV since it's buggy */ +list_for_each_entry (intel_encoder, &mode_config->encoder_list, base.head) + hotplug_en |= hpd_mask_i915[intel_encoder->hpd_pin]; +/* Programming the CRT detection parameters tends +to generate a spurious hotplug event about three +seconds later. So just do it once. +*/ +if (IS_G4X (dev)) + hotplug_en |=CRT_HOTPLUG_ACTIVATION_PERIOD_64 ; + hotplug_en |=CRT_HOTPLUG_VOLTAGE_COMPARE_50 ; -I915_WRITE (PORT_HOTPLUG_EN , hotplug_en); +/* Ignore TV since it's buggy */ +I915_WRITE (PORT_HOTPLUG_EN , hotplug_en); + } }static irqreturn_t i965_irq_handler (int irq,void *arg)
@@ -2886,7 +2869,7 @@ void intel_irq_init(struct drm_device *dev) dev->driver->irq_postinstall = i965_irq_postinstall; dev->driver->irq_uninstall = i965_irq_uninstall; dev->driver->irq_handler = i965_irq_handler; - dev_priv->display.hpd_irq_setup = i965_hpd_irq_setup; + dev_priv->display.hpd_irq_setup = i915_hpd_irq_setup; } dev->driver->enable_vblank = i915_enable_vblank; dev->driver->disable_vblank = i915_disable_vblank;
Signature · 1 line
1.7.7
PATCH 5/12 DRM/i915: Get rid if the 'hotplug_supported_mask' in struct drm_i915_private.
In reply to Egbert EichNow since we have replaced the bits to show interest in hotplug IRQs we can go and nuke the 'hotplug_supported_mask'.
--- drivers/gpu/drm/i915/i915_drv.h | 1 - drivers/gpu/drm/i915/intel_crt.c | 2 -- drivers/gpu/drm/i915/intel_dp.c | 3 --- drivers/gpu/drm/i915/intel_hdmi.c | 3 --- drivers/gpu/drm/i915/intel_sdvo.c | 9 +++------ 5 files changed, 3 insertions(+), 15 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +0 -1
@@ -922,7 +922,6 @@ typedef struct drm_i915_private {u32 irq_mask;u32 gt_irq_mask; -u32 hotplug_supported_mask;struct work_struct hotplug_work;bool enable_hotplug_processing;
drivers/gpu/drm/i915/intel_crt.c +0 -2
@@ -800,8 +800,6 @@ void intel_crt_init(struct drm_device *dev)*/ crt->force_hotplug_required =0 ; - dev_priv->hotplug_supported_mask |=CRT_HOTPLUG_INT_STATUS ; -/* * TODO: find a proper way to discover whether we need to set the the * polarity and link reversal bits or not, instead of relying on the
drivers/gpu/drm/i915/intel_dp.c +0 -3
@@ -2839,17 +2839,14 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port, name ="DPDDC-A" ;break ;case PORT_B : - dev_priv->hotplug_supported_mask |=PORTB_HOTPLUG_INT_STATUS ; intel_encoder->hpd_pin =HPD_PORT_B ; name ="DPDDC-B" ;break ;case PORT_C : - dev_priv->hotplug_supported_mask |=PORTC_HOTPLUG_INT_STATUS ; intel_encoder->hpd_pin =HPD_PORT_C ; name ="DPDDC-C" ;break ;case PORT_D : - dev_priv->hotplug_supported_mask |=PORTD_HOTPLUG_INT_STATUS ; intel_encoder->hpd_pin =HPD_PORT_D ; name ="DPDDC-D" ;break ;
drivers/gpu/drm/i915/intel_hdmi.c +0 -3
@@ -1023,17 +1023,14 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,case PORT_B : intel_hdmi->ddc_bus =GMBUS_PORT_DPB ; intel_encoder->hpd_pin =HPD_PORT_B ; - dev_priv->hotplug_supported_mask |=PORTB_HOTPLUG_INT_STATUS ;break ;case PORT_C : intel_hdmi->ddc_bus =GMBUS_PORT_DPC ; intel_encoder->hpd_pin =HPD_PORT_C ; - dev_priv->hotplug_supported_mask |=PORTC_HOTPLUG_INT_STATUS ;break ;case PORT_D : intel_hdmi->ddc_bus =GMBUS_PORT_DPD ; intel_encoder->hpd_pin =HPD_PORT_D ; - dev_priv->hotplug_supported_mask |=PORTD_HOTPLUG_INT_STATUS ;break ;case PORT_A : intel_encoder->hpd_pin =HPD_PORT_A ;
drivers/gpu/drm/i915/intel_sdvo.c +3 -6
@@ -2780,6 +2780,9 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob) SDVOB_HOTPLUG_INT_STATUS_I915 :SDVOC_HOTPLUG_INT_STATUS_I915 ; } +/* Only enable the hotplug irq if we need it, to work around noisy +* hotplug lines. +*/ if (intel_sdvo->hotplug_active) intel_encoder->hpd_pin =HPD_SDVO_B ?HPD_SDVO_B :HPD_SDVO_C ;
@@ -2811,12 +2814,6 @@ bool intel_sdvo_init(struct drm_device *dev, uint32_t sdvo_reg, bool is_sdvob)*/ intel_sdvo->base.cloneable = false; -/* Only enable the hotplug irq if we need it, to work around noisy -* hotplug lines. -*/ -if (intel_sdvo->hotplug_active) - dev_priv->hotplug_supported_mask |= hotplug_mask; -intel_sdvo_select_ddc_bus (dev_priv, intel_sdvo, sdvo_reg);/* Set the input timing to the screen. Assume always input 0. */
Signature · 1 line
1.7.7
Re: [PATCH v.2 05/12] DRM/i915: Get rid if the 'hotplug_supported_mask' in struct drm_i915_private.
Quoted 4 lines · Egbert Eich
Now since we have replaced the bits to show interest in hotplug IRQs we can go and nuke the '
hotplug_supported_mask'.Signed-off-by Egbert Eich <[email protected]>
I've applied your patch up to this one. Patch four needed some manual convincing to fit correctly, but the new hpd infrastructure is now in place.
To get the actual hpd irq storm detection going I think it'd be good to resend the remaining patches on top of latest drm-intel-nightly. I'll try to yell at people a bit harder to review things more timely this time around.
Thanks for the patches.
Cheers, Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH v.2 05/12] DRM/i915: Get rid if the 'hotplug_supported_mask' in struct drm_i915_private.
Quoted 10 lines · Daniel Vetter
Quoted 4 lines · Egbert Eich
Now since we have replaced the bits to show interest in hotplug IRQs we can go and nuke the '
hotplug_supported_mask'.Signed-off-by Egbert Eich <[email protected]>I've applied your patch up to this one. Patch four needed some manual convincing to fit correctly, but the new hpd infrastructure is now in place.
Quoted 6 lines
To get the actual hpd irq storm detection going I think it'd be good to resend the remaining patches on top of latest drm-intel-nightly. I'll try to yell at people a bit harder to review things more timely this time around.
Thanks for the patches.
Thanks guys for reviewing the patches. I will try to spare some time tonight to look into the issues that came up.
I got dragged into other tasks as well so I didn't even find the time to follow up on the patches myself.
For now I need to go back and debug yet another issue with SDVO on GM45 :(
Cheers,
Egbert.
PATCH 6/12 DRM/i915: Add HPD IRQ storm detection.
In reply to Egbert EichAdd a hotplug IRQ storm detection (triggered when a hotplug interrupt fires more than 5 times / sec).
Mask out this specific interrupt and revert to polling on the associated output.
Rationale:
Despite of the many attempts to fix the problem with noisy hotplug interrupt lines we are still seeing systems which have issues:
Once cause of noise seems to be bad routing of the hotplug line on the board: cross talk from other signals seems to cause erronous hotplug interrupts. This has been documented as an erratum for the the i945GM chipset and thus hotplug support was disabled for this chipset model but others seem to have this problem, too.
We have seen this issue on a G35 motherboard for example:
Even different motherboards of the same model seem to behave
differently: while some only see only around 10-100 interrupts/s others seem to see 5k or more.
We've also observed a dependency on the selected video mode.
Also on certain laptops interrupt noise seems to occur duing battery charging when the battery is at a certain charge levels.
Thus we add a simple algorithm here that detects an 'interrupt storm' condition.
--- drivers/gpu/drm/i915/i915_drv.h | 9 +++++ drivers/gpu/drm/i915/i915_irq.c | 63 +++++++++++++++++++++++++++++++------- 2 files changed, 60 insertions(+), 12 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +9 -0
@@ -1059,6 +1059,15 @@ typedef struct drm_i915_private {/* Old dri1 support infrastructure, beware the dragons ya fools entering * here! */ struct i915_dri1_state dri1; +struct { +unsigned long hpd_last_jiffies; +int hpd_cnt; +enum { +HPD_ENABLED =0 , +HPD_DISABLED =1 , +HPD_MARK_DISABLED =2 + } hpd_mark; + } hpd_stats[HPD_NUM_PINS ]; } drm_i915_private_t;/* Iterate over initialised rings */
drivers/gpu/drm/i915/i915_irq.c +51 -12
118 lines not on this page · raw message
Signature · 1 line
1.7.7
Re: [PATCH v.2 06/12] DRM/i915: Add HPD IRQ storm detection.
Quoted 5 lines · Egbert Eich
Add a hotplug IRQ storm detection (triggered when a hotplug interrupt fires more than 5 times / sec).
Mask out this specific interrupt and revert to polling on the associated output.
Hmm, not convinced that just marking it as HPD_MARK_DISABLED does that. It certainly prevents the irq from sending the uevent, but we have not yet disabled the irq or enabled drm_kms_helper.poll for this connector. Though userspace polling is unaffected.
Quoted 15 lines
Rationale:
Despite of the many attempts to fix the problem with noisy hotplug interrupt lines we are still seeing systems which have issues:
Once cause of noise seems to be bad routing of the hotplug line on the board: cross talk from other signals seems to cause erronous hotplug interrupts. This has been documented as an erratum for the the i945GM chipset and thus hotplug support was disabled for this chipset model but others seem to have this problem, too.We have seen this issue on a G35 motherboard for example:
Even different motherboards of the same model seem to behave
differently: while some only see only around 10-100 interrupts/s others seem to see 5k or more.
We've also observed a dependency on the selected video mode.
Quoted 5 lines
Also on certain laptops interrupt noise seems to occur duing battery charging when the battery is at a certain charge levels.
Thus we add a simple algorithm here that detects an 'interrupt storm' condition.
Looks sane.
Quoted 14 lines
Signed-off-by Egbert Eich <[email protected]>--- drivers/gpu/drm/i915/i915_drv.h | 9 +++++ drivers/gpu/drm/i915/i915_irq.c | 63 +++++++++++++++++++++++++++++++------- 2 files changed, 60 insertions(+), 12 deletions(-)drivers/gpu/drm/i915/i915_drv.h +0
-0@@ -1059,6 +1059,15 @@ typedef struct drm_i915_private {/* Old dri1 support infrastructure, beware the dragons ya fools entering * here! */
drivers/gpu/drm/i915/i915_drv.h typedef struct drm_i915_private { · line 1070
Quoted 10 lines
drivers/gpu/drm/i915/i915_drv.h +9
-0struct i915_dri1_state dri1; +struct { +unsigned long hpd_last_jiffies; +int hpd_cnt; +enum { +HPD_ENABLED =0 , +HPD_DISABLED =1 , +HPD_MARK_DISABLED =2 + } hpd_mark; + } hpd_stats[HPD_NUM_PINS ];
Please don't put it below dri1 -- as it may then be confused with our legacy burden.
Quoted 16 lines
drivers/gpu/drm/i915/i915_drv.h +0
-0} drm_i915_private_t;/* Iterate over initialised rings */ drivers/gpu/drm/i915/i915_irq.c +5
-0@@ -587,6 +587,34 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,queue_work (dev_priv->wq, &dev_priv->rps.work); } +static inline void hotplug_irq_storm_detect (struct drm_device *dev, +u32 hotplug_trigger, +const u32 *hpd) +{ +drm_i915_private_t *dev_priv = dev->dev_private;
drivers/gpu/drm/i915/i915_irq.c static void gen6_queue_rps_work(struct drm_i915_private *dev_priv, · line 604
Quoted 10 lines
drivers/gpu/drm/i915/i915_irq.c +10
-0+unsigned long irqflags; +int i; + +spin_lock_irqsave (&dev_priv->irq_lock, irqflags); + +for (i =1 ; i <HPD_NUM_PINS ; i++) { +if ((hpd[i] & hotplug_trigger) && + dev_priv->hpd_stats[i].hpd_mark ==HPD_ENABLED ) { +if (jiffies > (dev_priv->hpd_stats[i].hpd_last_jiffies +msecs_to_jiffies (1000 )) || + jiffies < dev_priv->hpd_stats[i].hpd_last_jiffies) {
if (time_is_after(dev_priv->hpd_stats[i].hpd_last_jiffies + msecs_to_jiffies(1000)))
Though it would be simpler if we just stored the adjusted hpd_last_jiffies.
Quoted 12 lines
drivers/gpu/drm/i915/i915_irq.c +12
-0+ dev_priv->hpd_stats[i].hpd_last_jiffies = jiffies; + dev_priv->hpd_stats[i].hpd_cnt =0 ; + }else if (dev_priv->hpd_stats[i].hpd_cnt >5 ) { + dev_priv->hpd_stats[i].hpd_mark =HPD_MARK_DISABLED ; +DRM_DEBUG_KMS ("HPD interrupt storm detected on PIN %d\n" , i); + }else + dev_priv->hpd_stats[i].hpd_cnt++; + } + } + +spin_unlock_irqrestore (&dev_priv->irq_lock, irqflags); +}
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
PATCH 6/12 DRM/i915: Add HPD IRQ storm detection (v2)
Add a hotplug IRQ storm detection (triggered when a hotplug interrupt fires more than 5 times / sec).
Rationale:
Despite of the many attempts to fix the problem with noisy hotplug interrupt lines we are still seeing systems which have issues:
Once cause of noise seems to be bad routing of the hotplug line on the board: cross talk from other signals seems to cause erronous hotplug interrupts. This has been documented as an erratum for the the i945GM chipset and thus hotplug support was disabled for this chipset model but others seem to have this problem, too.
We have seen this issue on a G35 motherboard for example:
Even different motherboards of the same model seem to behave
differently: while some only see only around 10-100 interrupts/s others seem to see 5k or more.
We've also observed a dependency on the selected video mode.
Also on certain laptops interrupt noise seems to occur duing battery charging when the battery is at a certain charge levels.
Thus we add a simple algorithm here that detects an 'interrupt storm' condition.
v2: Fixed comment.
--- drivers/gpu/drm/i915/i915_drv.h | 9 +++++ drivers/gpu/drm/i915/i915_irq.c | 63 +++++++++++++++++++++++++++++++------- 2 files changed, 60 insertions(+), 12 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +9 -0
@@ -1059,6 +1059,15 @@ typedef struct drm_i915_private {/* Old dri1 support infrastructure, beware the dragons ya fools entering * here! */ struct i915_dri1_state dri1; +struct { +unsigned long hpd_last_jiffies; +int hpd_cnt; +enum { +HPD_ENABLED =0 , +HPD_DISABLED =1 , +HPD_MARK_DISABLED =2 + } hpd_mark; + } hpd_stats[HPD_NUM_PINS ]; } drm_i915_private_t;/* Iterate over initialised rings */
drivers/gpu/drm/i915/i915_irq.c +51 -12
118 lines not on this page · raw message
Signature · 1 line
1.7.7
Re: [PATCH v.2 06/12] DRM/i915: Add HPD IRQ storm detection (v2)
Quoted 39 lines · Egbert Eich
Add a hotplug IRQ storm detection (triggered when a hotplug interrupt fires more than 5 times / sec).
Rationale:
Despite of the many attempts to fix the problem with noisy hotplug interrupt lines we are still seeing systems which have issues:
Once cause of noise seems to be bad routing of the hotplug line on the board: cross talk from other signals seems to cause erronous hotplug interrupts. This has been documented as an erratum for the the i945GM chipset and thus hotplug support was disabled for this chipset model but others seem to have this problem, too.We have seen this issue on a G35 motherboard for example:
Even different motherboards of the same model seem to behave
differently: while some only see only around 10-100 interrupts/s others seem to see 5k or more.
We've also observed a dependency on the selected video mode.Also on certain laptops interrupt noise seems to occur duing battery charging when the battery is at a certain charge levels.
Thus we add a simple algorithm here that detects an 'interrupt storm' condition.
v2: Fixed comment.
Signed-off-by Egbert Eich <[email protected]>Acked-by Chris Wilson <[email protected]>--- drivers/gpu/drm/i915/i915_drv.h | 9 +++++ drivers/gpu/drm/i915/i915_irq.c | 63 +++++++++++++++++++++++++++++++------- 2 files changed, 60 insertions(+), 12 deletions(-)drivers/gpu/drm/i915/i915_drv.h +0
-0@@ -1059,6 +1059,15 @@ typedef struct drm_i915_private {/* Old dri1 support infrastructure, beware the dragons ya fools entering * here! */
drivers/gpu/drm/i915/i915_drv.h typedef struct drm_i915_private { · line 1070
Quoted 10 lines
drivers/gpu/drm/i915/i915_drv.h +9
-0struct i915_dri1_state dri1; +struct { +unsigned long hpd_last_jiffies; +int hpd_cnt; +enum { +HPD_ENABLED =0 , +HPD_DISABLED =1 , +HPD_MARK_DISABLED =2 + } hpd_mark; + } hpd_stats[HPD_NUM_PINS ];
It's still hidden in the dri1 dungeon ;-) Imo this would fit nicely next to the existing hotplug work and state tracking stuff. -Daniel
Quoted 134 lines
drivers/gpu/drm/i915/i915_drv.h +0
-0} drm_i915_private_t;/* Iterate over initialised rings */ drivers/gpu/drm/i915/i915_irq.c +51
-12@@ -578,6 +578,34 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,queue_work (dev_priv->wq, &dev_priv->rps.work); } +static inline void hotplug_irq_storm_detect (struct drm_device *dev, +u32 hotplug_trigger, +const u32 *hpd) +{ +drm_i915_private_t *dev_priv = dev->dev_private; +unsigned long irqflags; +int i; + +spin_lock_irqsave (&dev_priv->irq_lock, irqflags); + +for (i =1 ; i <HPD_NUM_PINS ; i++) { +if ((hpd[i] & hotplug_trigger) && + dev_priv->hpd_stats[i].hpd_mark ==HPD_ENABLED ) { +if (jiffies > (dev_priv->hpd_stats[i].hpd_last_jiffies +msecs_to_jiffies (1000 )) || + jiffies < dev_priv->hpd_stats[i].hpd_last_jiffies) { + dev_priv->hpd_stats[i].hpd_last_jiffies = jiffies; + dev_priv->hpd_stats[i].hpd_cnt =0 ; + }else if (dev_priv->hpd_stats[i].hpd_cnt >5 ) { + dev_priv->hpd_stats[i].hpd_mark =HPD_MARK_DISABLED ; +DRM_DEBUG_KMS ("HPD interrupt storm detected on PIN %d\n" , i); + }else + dev_priv->hpd_stats[i].hpd_cnt++; + } + } + +spin_unlock_irqrestore (&dev_priv->irq_lock, irqflags); +} +static void gmbus_irq_handler (struct drm_device *dev) {struct drm_i915_private *dev_priv = (drm_i915_private_t *) dev->dev_private;@@ -646,13 +674,15 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)/* Consume port. Then clear IIR or we'll miss events */ if (iir &I915_DISPLAY_PORT_INTERRUPT ) {u32 hotplug_status =I915_READ (PORT_HOTPLUG_STAT ); +u32 hotplug_trigger = hotplug_status &HOTPLUG_INT_STATUS_I915 ;DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status); -if (hotplug_status &HOTPLUG_INT_STATUS_I915 ) +if (hotplug_trigger) { +hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_status_i915);queue_work (dev_priv->wq, &dev_priv->hotplug_work); - + }I915_WRITE (PORT_HOTPLUG_STAT , hotplug_status);I915_READ (PORT_HOTPLUG_STAT ); }@@ -676,10 +706,12 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;int pipe; +u32 hotplug_trigger = pch_iir &SDE_HOTPLUG_MASK ; -if (pch_iir &SDE_HOTPLUG_MASK ) +if (hotplug_trigger) { +hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_ibx);queue_work (dev_priv->wq, &dev_priv->hotplug_work); - + }if (pch_iir &SDE_AUDIO_POWER_MASK ) DRM_DEBUG_DRIVER("PCH audio power change on port %d\n", (pch_iir & SDE_AUDIO_POWER_MASK) >>@@ -722,10 +754,12 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;int pipe; +u32 hotplug_trigger = pch_iir &SDE_HOTPLUG_MASK_CPT ; -if (pch_iir &SDE_HOTPLUG_MASK_CPT ) +if (hotplug_trigger) { +hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_cpt);queue_work (dev_priv->wq, &dev_priv->hotplug_work); - + }if (pch_iir &SDE_AUDIO_POWER_MASK_CPT ) DRM_DEBUG_DRIVER("PCH audio power change on port %d\n", (pch_iir & SDE_AUDIO_POWER_MASK_CPT) >>@@ -2468,13 +2502,15 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)if ((I915_HAS_HOTPLUG (dev)) && (iir &I915_DISPLAY_PORT_INTERRUPT )) {u32 hotplug_status =I915_READ (PORT_HOTPLUG_STAT ); +u32 hotplug_trigger = hotplug_status &HOTPLUG_INT_STATUS_I915 ;DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status); -if (hotplug_status &HOTPLUG_INT_STATUS_I915 ) +if (hotplug_trigger) { +hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_status_i915);queue_work (dev_priv->wq, &dev_priv->hotplug_work); - + }I915_WRITE (PORT_HOTPLUG_STAT , hotplug_status);POSTING_READ (PORT_HOTPLUG_STAT ); }@@ -2701,15 +2737,18 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)/* Consume port. Then clear IIR or we'll miss events */ if (iir &I915_DISPLAY_PORT_INTERRUPT ) {u32 hotplug_status =I915_READ (PORT_HOTPLUG_STAT ); +u32 hotplug_trigger = hotplug_status & (IS_G4X (dev) ? +HOTPLUG_INT_STATUS_G4X : +HOTPLUG_INT_STATUS_I965 );DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status); -if (hotplug_status & (IS_G4X (dev) ? -HOTPLUG_INT_STATUS_G4X : -HOTPLUG_INT_STATUS_I965 )) +if (hotplug_trigger) { +hotplug_irq_storm_detect (dev, hotplug_trigger, +IS_G4X (dev) ? hpd_status_gen4 : hpd_status_i965);queue_work (dev_priv->wq, &dev_priv->hotplug_work); - + }I915_WRITE (PORT_HOTPLUG_STAT , hotplug_status);I915_READ (PORT_HOTPLUG_STAT ); }Signature · 1 line
1.7.7
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
PATCH 6/12 DRM/i915: Add HPD IRQ storm detection (v3)
Add a hotplug IRQ storm detection (triggered when a hotplug interrupt fires more than 5 times / sec).
Rationale:
Despite of the many attempts to fix the problem with noisy hotplug interrupt lines we are still seeing systems which have issues:
Once cause of noise seems to be bad routing of the hotplug line on the board: cross talk from other signals seems to cause erronous hotplug interrupts. This has been documented as an erratum for the the i945GM chipset and thus hotplug support was disabled for this chipset model but others seem to have this problem, too.
We have seen this issue on a G35 motherboard for example:
Even different motherboards of the same model seem to behave
differently: while some only see only around 10-100 interrupts/s others seem to see 5k or more.
We've also observed a dependency on the selected video mode.
Also on certain laptops interrupt noise seems to occur duing battery charging when the battery is at a certain charge levels.
Thus we add a simple algorithm here that detects an 'interrupt storm' condition.
v2: Fixed comment.
v3: Reordered drm_i915_private: moved hpd state tracking to hotplug work stuff.
--- drivers/gpu/drm/i915/i915_drv.h | 9 +++++ drivers/gpu/drm/i915/i915_irq.c | 63 +++++++++++++++++++++++++++++++------- 2 files changed, 60 insertions(+), 12 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +9 -0
@@ -924,6 +924,15 @@ typedef struct drm_i915_private {struct work_struct hotplug_work;bool enable_hotplug_processing; +struct { +unsigned long hpd_last_jiffies; +int hpd_cnt; +enum { +HPD_ENABLED =0 , +HPD_DISABLED =1 , +HPD_MARK_DISABLED =2 + } hpd_mark; + } hpd_stats[HPD_NUM_PINS ];int num_pipe;int num_pch_pll;
drivers/gpu/drm/i915/i915_irq.c +51 -12
118 lines not on this page · raw message
Signature · 1 line
1.7.7
PATCH 10/12 DRM/i915: Add Reenable Timer to turn Hotplug Detection back on (v2).
We disable hoptplug detection when we encounter a hotplug event storm. Still hotplug detection is required on some outputs (like Display Port). The interrupt storm may be only temporary (on certain Dell Laptops for instance it happens at certain charging states of the system). Thus we enable it after a certain grace period (2 minutes). Should the interrupt storm persist it will be detected immediately and it will be disabled again.
v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state tracker.
--- drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_irq.c | 53 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 1 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +2 -0
@@ -933,6 +933,8 @@ typedef struct drm_i915_private { HPD_MARK_DISABLED = 2 } hpd_mark; } hpd_stats[HPD_NUM_PINS ]; +#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000) +struct timer_list hotplug_reenable_timer;int num_pipe;int num_pch_pll;
drivers/gpu/drm/i915/i915_irq.c +52 -1
96 lines not on this page · raw message
Signature · 1 line
1.7.7
Re: [PATCH v.2 10/12] DRM/i915: Add Reenable Timer to turn Hotplug Detection back on (v2).
Quoted 86 lines · Egbert Eich
We disable hoptplug detection when we encounter a hotplug event storm. Still hotplug detection is required on some outputs (like Display Port). The interrupt storm may be only temporary (on certain Dell Laptops for instance it happens at certain charging states of the system). Thus we enable it after a certain grace period (2 minutes). Should the interrupt storm persist it will be detected immediately and it will be disabled again.
v2: Reordered
drm_i915_private: movedhotplug_reenable_timerto hpd state tracker.Signed-off-by Egbert Eich <[email protected]>--- drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_irq.c | 53 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 1 deletions(-)drivers/gpu/drm/i915/i915_drv.h +2
-0@@ -933,6 +933,8 @@ typedef struct drm_i915_private { HPD_MARK_DISABLED = 2 } hpd_mark; } hpd_stats[HPD_NUM_PINS ]; +#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000) +struct timer_list hotplug_reenable_timer;int num_pipe;int num_pch_pll;drivers/gpu/drm/i915/i915_irq.c +12
-1@@ -367,8 +367,11 @@ static void i915_hotplug_work_func(struct work_struct *work) connector_disabled = true; } } -if (connector_disabled) +if (connector_disabled) {drm_kms_helper_poll_enable (dev);/* if there were no outputs to poll, poll is disabled */ +mod_timer (&dev_priv->hotplug_reenable_timer, + jiffies +msecs_to_jiffies (I915_REENABLE_HOTPLUG_DELAY )); + }spin_unlock_irqrestore (&dev_priv->irq_lock, irqflags);@@ -2244,6 +2247,8 @@ static void valleyview_irq_uninstall(struct drm_device *dev)if (!dev_priv)return ; +del_timer_sync (&dev_priv->hotplug_reenable_timer); +for_each_pipe (pipe)I915_WRITE (PIPESTAT (pipe),0xffff );@@ -2265,6 +2270,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)if (!dev_priv)return ; +del_timer_sync (&dev_priv->hotplug_reenable_timer); +I915_WRITE (HWSTAM ,0xffffffff );I915_WRITE (DEIMR ,0xffffffff );@@ -2603,6 +2610,8 @@ static void i915_irq_uninstall(struct drm_device * dev)drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;int pipe; +del_timer_sync (&dev_priv->hotplug_reenable_timer); +if (I915_HAS_HOTPLUG (dev)) {I915_WRITE (PORT_HOTPLUG_EN ,0 );I915_WRITE (PORT_HOTPLUG_STAT ,I915_READ (PORT_HOTPLUG_STAT ));@@ -2851,6 +2860,8 @@ static void i965_irq_uninstall(struct drm_device * dev)if (!dev_priv)return ; +del_timer_sync (&dev_priv->hotplug_reenable_timer); +I915_WRITE (PORT_HOTPLUG_EN ,0 );I915_WRITE (PORT_HOTPLUG_STAT ,I915_READ (PORT_HOTPLUG_STAT ));@@ -2866,6 +2877,44 @@ static void i965_irq_uninstall(struct drm_device * dev)I915_WRITE (IIR ,I915_READ (IIR )); }
drivers/gpu/drm/i915/i915_irq.c static void i965_irq_uninstall(struct drm_device * dev) · line 2889
Quoted 10 lines
drivers/gpu/drm/i915/i915_irq.c +10
-0+static void i915_reenable_hotplug_timer_func (unsigned long data) +{ +drm_i915_private_t *dev_priv = (drm_i915_private_t *)data; +struct drm_device *dev = dev_priv->dev; +struct drm_mode_config *mode_config = &dev->mode_config; +unsigned long irqflags; +int i; + +spin_lock_irqsave (&dev_priv->irq_lock, irqflags); +for (i =1 ; i <HPD_NUM_PINS ; i++) {
Maybe include a small comment to avoid people thinking the 'i = 1' is a typo.
drivers/gpu/drm/i915/i915_irq.c static void i965_irq_uninstall(struct drm_device * dev) · line 2890
Quoted 1 line
drivers/gpu/drm/i915/i915_irq.c +1
-0+if (dev_priv->hpd_stats[i].hpd_mark ==HPD_MARK_DISABLED ) {
This is getting quite deep. This could help a bit:
if (dev_priv->hpd_stats[i].hpd_mark != HPD_MARK_DISABLED)
continue;
Quoted 9 lines
drivers/gpu/drm/i915/i915_irq.c +9
-0+struct drm_connector *connector; + + dev_priv->hpd_stats[i].hpd_mark =HPD_ENABLED ; +list_for_each_entry (connector, &mode_config->connector_list, head) { +struct intel_connector *intel_connector =to_intel_connector (connector); + +if (intel_connector->encoder->hpd_pin == i) { +if (connector->polled != intel_connector->polled) + DRM_DEBUG_DRIVER("Reenabling HPD on connector %s\n",
drivers/gpu/drm/i915/i915_irq.c static void i965_irq_uninstall(struct drm_device * dev) · line 2909
Quoted 10 lines
drivers/gpu/drm/i915/i915_irq.c +10
-0+ drm_get_connector_name(connector)); + connector->polled = intel_connector->polled; +if (!connector->polled) + connector->polled =DRM_CONNECTOR_POLL_HPD ; + } + } + +if (IS_HASWELL (dev) || +IS_IVYBRIDGE (dev) || + (HAS_PCH_SPLIT (dev)))
IS_HASWELL() and IS_IVYBRIDGE() are superfluous.
Quoted 27 lines
drivers/gpu/drm/i915/i915_irq.c +10
-0+ibx_hpd_irq_setup (dev); + else +i915_hpd_irq_setup (dev ); + } + } +spin_unlock_irqrestore (&dev_priv->irq_lock, irqflags); +} +void intel_irq_init (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -2878,6 +2927,8 @@ void intel_irq_init(struct drm_device *dev)setup_timer (&dev_priv->gpu_error.hangcheck_timer, i915_hangcheck_elapsed, (unsigned long ) dev); +setup_timer (&dev_priv->hotplug_reenable_timer, i915_reenable_hotplug_timer_func, + (unsigned long ) dev_priv);pm_qos_add_request (&dev_priv->pm_qos,PM_QOS_CPU_DMA_LATENCY ,PM_QOS_DEFAULT_VALUE );Signature · 6 lines
1.7.7 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 2 lines
Ville Syrjälä Intel OTC
PATCH 10/12 DRM/i915: Add Reenable Timer to turn Hotplug Detection back on (v3).
We disable hoptplug detection when we encounter a hotplug event storm. Still hotplug detection is required on some outputs (like Display Port). The interrupt storm may be only temporary (on certain Dell Laptops for instance it happens at certain charging states of the system). Thus we enable it after a certain grace period (2 minutes). Should the interrupt storm persist it will be detected immediately and it will be disabled again.
v2: Reordered drm_i915_private: moved hotplug_reenable_timer to hpd state tracker.
v3: Clarified loop start value,
Removed superfluous test for Ivybridge and Haswell,
Restructured loop to avoid deep nesting (all suggested by Ville Syrjälä)
--- drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_irq.c | 53 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 1 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +2 -0
@@ -933,6 +933,8 @@ typedef struct drm_i915_private { HPD_MARK_DISABLED = 2 } hpd_mark; } hpd_stats[HPD_NUM_PINS ]; +#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000) +struct timer_list hotplug_reenable_timer;int num_pipe;int num_pch_pll;
drivers/gpu/drm/i915/i915_irq.c +52 -1
96 lines not on this page · raw message
Signature · 6 lines
1.7.7 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 11/12 DRM/i915: Add bit field to record which pins have received HPD events (v2)
In reply to Egbert EichThis way it is possible to limit 're'-detect() of displays to connectors which have received an HPD event.
v2: Reordered drm_i915_private: Move hpd_event_bits to hpd state tracking.
--- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++ 2 files changed, 11 insertions(+), 0 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +1 -0
@@ -933,6 +933,7 @@ typedef struct drm_i915_private { HPD_MARK_DISABLED = 2 } hpd_mark; } hpd_stats[HPD_NUM_PINS ]; +u32 hpd_event_bits;#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)struct timer_list hotplug_reenable_timer;
drivers/gpu/drm/i915/i915_irq.c +10 -0
@@ -344,6 +344,7 @@ static void i915_hotplug_work_func(struct work_struct *work)struct drm_connector *connector;unsigned long irqflags;bool connector_disabled = false; +u32 hpd_event_bits;/* HPD irq before everything is fully set up. */ if (!dev_priv->enable_hotplug_processing)
@@ -353,6 +354,9 @@ static void i915_hotplug_work_func(struct work_struct *work)DRM_DEBUG_KMS ("running encoder hotplug functions\n" );spin_lock_irqsave (&dev_priv->irq_lock, irqflags); + + hpd_event_bits = dev_priv->hpd_event_bits; + dev_priv->hpd_event_bits =0 ;list_for_each_entry (connector, &mode_config->connector_list, head) { intel_connector =to_intel_connector (connector); intel_encoder = intel_connector->encoder;
@@ -366,6 +370,10 @@ static void i915_hotplug_work_func(struct work_struct *work) | DRM_CONNECTOR_POLL_DISCONNECT; connector_disabled = true; } +if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) { +DRM_DEBUG_KMS ("Connector %s (pin %i) received hotplug event.\n" , +drm_get_connector_name (connector), intel_encoder->hpd_pin); + } }if (connector_disabled) {drm_kms_helper_poll_enable (dev);/* if there were no outputs to poll, poll is disabled */
@@ -620,12 +628,14 @@ static inline bool hotplug_irq_storm_detect(struct drm_device *dev,for (i =1 ; i <HPD_NUM_PINS ; i++) {if ((hpd[i] & hotplug_trigger) && dev_priv->hpd_stats[i].hpd_mark ==HPD_ENABLED ) { + dev_priv->hpd_event_bits |= (1 << i);if (jiffies > (dev_priv->hpd_stats[i].hpd_last_jiffies +msecs_to_jiffies (1000 )) || jiffies < dev_priv->hpd_stats[i].hpd_last_jiffies) { dev_priv->hpd_stats[i].hpd_last_jiffies = jiffies; dev_priv->hpd_stats[i].hpd_cnt =0 ; }else if (dev_priv->hpd_stats[i].hpd_cnt >5 ) { dev_priv->hpd_stats[i].hpd_mark =HPD_MARK_DISABLED ; + dev_priv->hpd_event_bits &= ~(1 << i);DRM_DEBUG_KMS ("HPD interrupt storm detected on PIN %d\n" , i); ret = true; }else
Signature · 1 line
1.7.7
PATCH 11/12 DRM/i915: Add bit field to record which pins have received HPD events (v3)
This way it is possible to limit 're'-detect() of displays to connectors which have received an HPD event.
v2: Reordered drm_i915_private: Move hpd_event_bits to hpd state tracking.
v3: Fix patch.
--- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_irq.c | 47 ++++++++++++++++++++++++++++++++++----- 2 files changed, 42 insertions(+), 6 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +1 -0
@@ -933,6 +933,7 @@ typedef struct drm_i915_private { HPD_MARK_DISABLED = 2 } hpd_mark; } hpd_stats[HPD_NUM_PINS ]; +u32 hpd_event_bits;#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)struct timer_list hotplug_reenable_timer;
drivers/gpu/drm/i915/i915_irq.c +41 -6
91 lines not on this page · raw message
Signature · 1 line
1.7.7
Re: [PATCH v.3 11/12] DRM/i915: Add bit field to record which pins have received HPD events (v3)
Quoted 6 lines · Egbert Eich
This way it is possible to limit 're'-detect() of displays to connectors which have received an HPD event.
v2: Reordered
drm_i915_private: Movehpd_event_bitsto hpd state tracking.
v3: Fix patch.
Oops, forget this patch. It's messed up.
Sorry,
Egbert.
PATCH 7/12 DRM/i915: (re)init HPD interrupt storm statistics.
In reply to Egbert EichWhen an encoder is shared on several connectors there is only one hotplug line, thus this line needs to be shared among these connectors.
If HPD detect only works reliably on a subset of those connectors, we want to poll the others. Thus we need to make sure that storm detection doesn't mess up the settings for those connectors. Therefore we store the settings in the intel_connector struct and restore them from there.
If nothing is set but the encoder has a hpd_pin set we assume this connector is hotplug capable.
On init/reset we make sure the polled state of the connectors is (re)set to the default value, the HPD interrupts are marked enabled.
--- drivers/gpu/drm/i915/i915_irq.c | 13 +++++++++++++ drivers/gpu/drm/i915/intel_crt.c | 6 ++---- drivers/gpu/drm/i915/intel_dp.c | 1 - drivers/gpu/drm/i915/intel_drv.h | 4 ++++ drivers/gpu/drm/i915/intel_hdmi.c | 1 - drivers/gpu/drm/i915/intel_sdvo.c | 5 ++--- drivers/gpu/drm/i915/intel_tv.c | 2 +- 7 files changed, 22 insertions(+), 10 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +13 -0
@@ -2918,7 +2918,20 @@ void intel_irq_init(struct drm_device *dev)void intel_hpd_init (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private; +struct drm_mode_config *mode_config = &dev->mode_config; +struct drm_connector *connector; +int i; +for (i =1 ; i <HPD_NUM_PINS ; i++) { + dev_priv->hpd_stats[i].hpd_cnt =0 ; + dev_priv->hpd_stats[i].hpd_mark =HPD_ENABLED ; + } +list_for_each_entry (connector, &mode_config->connector_list, head) { +struct intel_connector *intel_connector =to_intel_connector (connector); + connector->polled = intel_connector->polled; +if (!connector->polled &&I915_HAS_HOTPLUG (dev) && intel_connector->encoder->hpd_pin >HPD_NONE ) + connector->polled =DRM_CONNECTOR_POLL_HPD ; + }if (dev_priv->display.hpd_irq_setup) dev_priv->display.hpd_irq_setup (dev); }
drivers/gpu/drm/i915/intel_crt.c +2 -4
@@ -790,10 +790,8 @@ void intel_crt_init(struct drm_device *dev)drm_sysfs_connector_add (connector); -if (I915_HAS_HOTPLUG (dev)) - connector->polled =DRM_CONNECTOR_POLL_HPD ; -else - connector->polled =DRM_CONNECTOR_POLL_CONNECT ; +if (!I915_HAS_HOTPLUG (dev)) + intel_connector->polled =DRM_CONNECTOR_POLL_CONNECT ;/* * Configure the automatic hotplug detection stuff
drivers/gpu/drm/i915/intel_dp.c +0 -1
@@ -2816,7 +2816,6 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,drm_connector_init (dev, connector, &intel_dp_connector_funcs, type);drm_connector_helper_add (connector, &intel_dp_connector_helper_funcs); - connector->polled =DRM_CONNECTOR_POLL_HPD ; connector->interlace_allowed = true; connector->doublescan_allowed =0 ;
drivers/gpu/drm/i915/intel_drv.h +4 -0
@@ -198,6 +198,10 @@ struct intel_connector {/* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */ struct edid *edid; + +/* since POLL and HPD connectors may use the same HPD line keep the native +state of connector->polled in case hotplug storm detection changes it */ +u8 polled; };struct intel_crtc {
drivers/gpu/drm/i915/intel_hdmi.c +0 -1
@@ -1015,7 +1015,6 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port, DRM_MODE_CONNECTOR_HDMIA);drm_connector_helper_add (connector, &intel_hdmi_connector_helper_funcs); - connector->polled =DRM_CONNECTOR_POLL_HPD ; connector->interlace_allowed =1 ; connector->doublescan_allowed =0 ;
drivers/gpu/drm/i915/intel_sdvo.c +2 -3
@@ -2270,7 +2270,6 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device) connector = &intel_connector->base;if (intel_sdvo_get_hotplug_support (intel_sdvo) & intel_sdvo_connector->output_flag) { - connector->polled =DRM_CONNECTOR_POLL_HPD ; intel_sdvo->hotplug_active |= intel_sdvo_connector->output_flag;/* Some SDVO devices have one-shot hotplug interrupts. * Ensure that they get re-enabled when an interrupt happens.
@@ -2278,7 +2277,7 @@ intel_sdvo_dvi_init(struct intel_sdvo *intel_sdvo, int device) intel_encoder->hot_plug = intel_sdvo_enable_hotplug;intel_sdvo_enable_hotplug (intel_encoder); }else { - connector->polled =DRM_CONNECTOR_POLL_CONNECT |DRM_CONNECTOR_POLL_DISCONNECT ; + intel_connector->polled =DRM_CONNECTOR_POLL_CONNECT |DRM_CONNECTOR_POLL_DISCONNECT ; } encoder->encoder_type =DRM_MODE_ENCODER_TMDS ; connector->connector_type =DRM_MODE_CONNECTOR_DVID ;
@@ -2347,7 +2346,7 @@ intel_sdvo_analog_init(struct intel_sdvo *intel_sdvo, int device) intel_connector = &intel_sdvo_connector->base; connector = &intel_connector->base; - connector->polled =DRM_CONNECTOR_POLL_CONNECT ; + intel_connector->polled =DRM_CONNECTOR_POLL_CONNECT ; encoder->encoder_type =DRM_MODE_ENCODER_DAC ; connector->connector_type =DRM_MODE_CONNECTOR_VGA ;
drivers/gpu/drm/i915/intel_tv.c +1 -1
@@ -1613,7 +1613,7 @@ intel_tv_init(struct drm_device *dev)* * More recent chipsets favour HDMI rather than integrated S-Video. */ - connector->polled =DRM_CONNECTOR_POLL_CONNECT ; + intel_connector->polled =DRM_CONNECTOR_POLL_CONNECT ;drm_connector_init (dev, connector, &intel_tv_connector_funcs,DRM_MODE_CONNECTOR_SVIDEO );
Signature · 1 line
1.7.7
PATCH 8/12 DRM/i915: Treat hpd_irq_setup() for ironake and older generations the same way.
In reply to Egbert EichWhen switching to enabling HPD IRQs only for lines where needed and supported this will ensure that the right lines will be enabled on all generations when intel_hpd_init() is called.
--- drivers/gpu/drm/i915/i915_irq.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +5 -5
@@ -2033,7 +2033,7 @@ static void ibx_enable_hotplug(struct drm_device *dev)I915_WRITE (PCH_PORT_HOTPLUG , hotplug); } -static void ibx_irq_postinstall (struct drm_device *dev) +static void ibx_hpd_irq_setup (struct drm_device *dev) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;struct drm_mode_config *mode_config = &dev->mode_config;
@@ -2055,8 +2055,6 @@ static void ibx_irq_postinstall(struct drm_device *dev)I915_WRITE (SDEIMR , ~mask);I915_WRITE (SDEIER , mask);POSTING_READ (SDEIER ); - -ibx_enable_hotplug (dev); }static int ironlake_irq_postinstall (struct drm_device *dev)
@@ -2094,7 +2092,7 @@ static int ironlake_irq_postinstall(struct drm_device *dev)I915_WRITE (GTIER , render_irqs);POSTING_READ (GTIER ); -ibx_irq_postinstall (dev); +ibx_enable_hotplug (dev);if (IS_IRONLAKE_M (dev)) {/* Clear & enable PCU event interrupts */
@@ -2140,7 +2138,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)I915_WRITE (GTIER , render_irqs);POSTING_READ (GTIER ); -ibx_irq_postinstall (dev); +ibx_enable_hotplug (dev);return 0 ; }
@@ -2884,6 +2882,7 @@ void intel_irq_init(struct drm_device *dev) dev->driver->irq_uninstall = ironlake_irq_uninstall; dev->driver->enable_vblank = ivybridge_enable_vblank; dev->driver->disable_vblank = ivybridge_disable_vblank; + dev_priv->display.hpd_irq_setup = ibx_hpd_irq_setup; }else if (HAS_PCH_SPLIT (dev)) { dev->driver->irq_handler = ironlake_irq_handler; dev->driver->irq_preinstall = ironlake_irq_preinstall;
@@ -2891,6 +2890,7 @@ void intel_irq_init(struct drm_device *dev) dev->driver->irq_uninstall = ironlake_irq_uninstall; dev->driver->enable_vblank = ironlake_enable_vblank; dev->driver->disable_vblank = ironlake_disable_vblank; + dev_priv->display.hpd_irq_setup = ibx_hpd_irq_setup; }else {if (INTEL_INFO (dev)->gen ==2 ) { dev->driver->irq_preinstall = i8xx_irq_preinstall;
Signature · 1 line
1.7.7
PATCH 9/12 DRM/i915: Disable HPD interrupt on pin when irq storm is detected.
In reply to Egbert EichThis patch disables hotplug interrupts if an 'interrupt storm' has been detected.
Noise on the interrupt line renders the hotplug interrupt useless:
each hotplug event causes the devices to be rescanned which will will only increase the system load.
Thus disable the hotplug interrupts and fall back to periodic device polling.
--- drivers/gpu/drm/i915/i915_irq.c | 69 ++++++++++++++++++++++++++++++--------- 1 files changed, 53 insertions(+), 16 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +53 -16
@@ -96,7 +96,8 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */PORTD_HOTPLUG_INT_STATUS /* HPD_PORT_D */ }; - +static void ibx_hpd_irq_setup (struct drm_device *dev); +static void i915_hpd_irq_setup (struct drm_device *dev);/* For display hotplug interrupt */ static void
@@ -347,7 +348,11 @@ static void i915_hotplug_work_func(struct work_struct *work) hotplug_work);struct drm_device *dev = dev_priv->dev;struct drm_mode_config *mode_config = &dev->mode_config; -struct intel_encoder *encoder; +struct intel_connector *intel_connector; +struct intel_encoder *intel_encoder; +struct drm_connector *connector; +unsigned long irqflags; +bool connector_disabled = false;/* HPD irq before everything is fully set up. */ if (!dev_priv->enable_hotplug_processing)
@@ -356,9 +361,29 @@ static void i915_hotplug_work_func(struct work_struct *work)mutex_lock (&mode_config->mutex);DRM_DEBUG_KMS ("running encoder hotplug functions\n" ); -list_for_each_entry (encoder, &mode_config->encoder_list, base.head) -if (encoder->hot_plug) - encoder->hot_plug (encoder); +spin_lock_irqsave (&dev_priv->irq_lock, irqflags); +list_for_each_entry (connector, &mode_config->connector_list, head) { + intel_connector =to_intel_connector (connector); + intel_encoder = intel_connector->encoder; +if (intel_encoder->hpd_pin >HPD_NONE && + dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark ==HPD_MARK_DISABLED && + connector->polled ==DRM_CONNECTOR_POLL_HPD ) { +pr_warn ("HPD interrupt storm detected on connector %s disabling\n" , +drm_get_connector_name (connector)); + dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark =HPD_DISABLED ; + connector->polled =DRM_CONNECTOR_POLL_CONNECT + |DRM_CONNECTOR_POLL_DISCONNECT ; + connector_disabled = true; + } + } +if (connector_disabled) +drm_kms_helper_poll_enable (dev);/* if there were no outputs to poll, poll is disabled */ + +spin_unlock_irqrestore (&dev_priv->irq_lock, irqflags); + +list_for_each_entry (intel_encoder, &mode_config->encoder_list, base.head) +if (intel_encoder->hot_plug) + intel_encoder->hot_plug (encoder);mutex_unlock (&mode_config->mutex);
@@ -587,13 +612,14 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,queue_work (dev_priv->wq, &dev_priv->rps.work); } -static inline void hotplug_irq_storm_detect (struct drm_device *dev, +static inline bool hotplug_irq_storm_detect (struct drm_device *dev,u32 hotplug_trigger,const u32 *hpd) {drm_i915_private_t *dev_priv = dev->dev_private;unsigned long irqflags;int i; +bool ret = false;spin_lock_irqsave (&dev_priv->irq_lock, irqflags);
@@ -607,12 +633,15 @@ static inline void hotplug_irq_storm_detect(struct drm_device *dev, }else if (dev_priv->hpd_stats[i].hpd_cnt >5 ) { dev_priv->hpd_stats[i].hpd_mark =HPD_MARK_DISABLED ;DRM_DEBUG_KMS ("HPD interrupt storm detected on PIN %d\n" , i); + ret = true; }else dev_priv->hpd_stats[i].hpd_cnt++; } }spin_unlock_irqrestore (&dev_priv->irq_lock, irqflags); + +return ret; }static void gmbus_irq_handler (struct drm_device *dev)
@@ -688,7 +717,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status);if (hotplug_trigger) { -hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_status_i915); +if (hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_status_i915)) +i915_hpd_irq_setup (dev);queue_work (dev_priv->wq, &dev_priv->hotplug_work); }
@@ -718,7 +748,8 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)u32 hotplug_trigger = pch_iir &SDE_HOTPLUG_MASK ;if (hotplug_trigger) { -hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_ibx); +if (hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_ibx)) +ibx_hpd_irq_setup (dev);queue_work (dev_priv->wq, &dev_priv->hotplug_work); }if (pch_iir &SDE_AUDIO_POWER_MASK )
@@ -766,7 +797,8 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)u32 hotplug_trigger = pch_iir &SDE_HOTPLUG_MASK_CPT ;if (hotplug_trigger) { -hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_cpt); +if (hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_cpt)) +ibx_hpd_irq_setup (dev);queue_work (dev_priv->wq, &dev_priv->hotplug_work); }if (pch_iir &SDE_AUDIO_POWER_MASK_CPT )
@@ -2043,12 +2075,14 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)if (HAS_PCH_IBX (dev)) { mask &= ~SDE_HOTPLUG_MASK ;list_for_each_entry (intel_encoder, &mode_config->encoder_list, base.head) - mask |= hpd_ibx[intel_encoder->hpd_pin]; +if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark ==HPD_ENABLED ) + mask |= hpd_ibx[intel_encoder->hpd_pin]; mask |=SDE_GMBUS |SDE_AUX_MASK ; }else { mask &= ~SDE_HOTPLUG_MASK_CPT ;list_for_each_entry (intel_encoder, &mode_config->encoder_list, base.head) - mask |= hpd_cpt[intel_encoder->hpd_pin]; +if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark ==HPD_ENABLED ) + mask |= hpd_cpt[intel_encoder->hpd_pin]; mask |=SDE_GMBUS_CPT |SDE_AUX_MASK_CPT ; }I915_WRITE (SDEIIR ,I915_READ (SDEIIR ));
@@ -2514,7 +2548,8 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status);if (hotplug_trigger) { -hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_status_i915); +if (hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_status_i915)) +i915_hpd_irq_setup (dev);queue_work (dev_priv->wq, &dev_priv->hotplug_work); }
@@ -2668,7 +2703,7 @@ static void i915_hpd_irq_setup(struct drm_device *dev) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;struct drm_mode_config *mode_config = &dev->mode_config; -struct intel_encoder *encoder; +struct intel_encoder *intel_encoder;u32 hotplug_en;if (I915_HAS_HOTPLUG (dev)) {
@@ -2677,7 +2712,8 @@ static void i915_hpd_irq_setup(struct drm_device *dev)/* Note HDMI and DP share hotplug bits */ /* enable bits are the same for all generations */ list_for_each_entry (intel_encoder, &mode_config->encoder_list, base.head) - hotplug_en |= hpd_mask_i915[intel_encoder->hpd_pin]; +if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark ==HPD_ENABLED ) + hotplug_en |= hpd_mask_i915[intel_encoder->hpd_pin];/* Programming the CRT detection parameters tends to generate a spurious hotplug event about three seconds later. So just do it once.
@@ -2751,8 +2787,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status);if (hotplug_trigger) { -hotplug_irq_storm_detect (dev, hotplug_trigger, -IS_G4X (dev) ? hpd_status_gen4 : hpd_status_i965); +if (hotplug_irq_storm_detect (dev, hotplug_trigger, +IS_G4X (dev) ? hpd_status_gen4 : hpd_status_i965)) +i915_hpd_irq_setup (dev);queue_work (dev_priv->wq, &dev_priv->hotplug_work); }
Signature · 1 line
1.7.7
PATCH 9/12 DRM/i915: Disable HPD interrupt on pin when irq storm is detected (v2)
This patch disables hotplug interrupts if an 'interrupt storm' has been detected.
Noise on the interrupt line renders the hotplug interrupt useless:
each hotplug event causes the devices to be rescanned which will will only increase the system load.
Thus disable the hotplug interrupts and fall back to periodic device polling.
v2: Fixed cleanup typo.
--- drivers/gpu/drm/i915/i915_irq.c | 69 ++++++++++++++++++++++++++++++--------- 1 files changed, 53 insertions(+), 16 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +53 -16
@@ -87,7 +87,8 @@ static const u32 hpd_status_i915[] = { /* i915 and valleyview are the same */ [HPD_PORT_D ] =PORTD_HOTPLUG_INT_STATUS }; - +static void ibx_hpd_irq_setup (struct drm_device *dev); +static void i915_hpd_irq_setup (struct drm_device *dev);/* For display hotplug interrupt */ static void
@@ -338,7 +339,11 @@ static void i915_hotplug_work_func(struct work_struct *work) hotplug_work);struct drm_device *dev = dev_priv->dev;struct drm_mode_config *mode_config = &dev->mode_config; -struct intel_encoder *encoder; +struct intel_connector *intel_connector; +struct intel_encoder *intel_encoder; +struct drm_connector *connector; +unsigned long irqflags; +bool connector_disabled = false;/* HPD irq before everything is fully set up. */ if (!dev_priv->enable_hotplug_processing)
@@ -347,9 +352,29 @@ static void i915_hotplug_work_func(struct work_struct *work)mutex_lock (&mode_config->mutex);DRM_DEBUG_KMS ("running encoder hotplug functions\n" ); -list_for_each_entry (encoder, &mode_config->encoder_list, base.head) -if (encoder->hot_plug) - encoder->hot_plug (encoder); +spin_lock_irqsave (&dev_priv->irq_lock, irqflags); +list_for_each_entry (connector, &mode_config->connector_list, head) { + intel_connector =to_intel_connector (connector); + intel_encoder = intel_connector->encoder; +if (intel_encoder->hpd_pin >HPD_NONE && + dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark ==HPD_MARK_DISABLED && + connector->polled ==DRM_CONNECTOR_POLL_HPD ) { +pr_warn ("HPD interrupt storm detected on connector %s disabling\n" , +drm_get_connector_name (connector)); + dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark =HPD_DISABLED ; + connector->polled =DRM_CONNECTOR_POLL_CONNECT + |DRM_CONNECTOR_POLL_DISCONNECT ; + connector_disabled = true; + } + } +if (connector_disabled) +drm_kms_helper_poll_enable (dev);/* if there were no outputs to poll, poll is disabled */ + +spin_unlock_irqrestore (&dev_priv->irq_lock, irqflags); + +list_for_each_entry (intel_encoder, &mode_config->encoder_list, base.head) +if (intel_encoder->hot_plug) + intel_encoder->hot_plug (intel_encoder);mutex_unlock (&mode_config->mutex);
@@ -578,13 +603,14 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,queue_work (dev_priv->wq, &dev_priv->rps.work); } -static inline void hotplug_irq_storm_detect (struct drm_device *dev, +static inline bool hotplug_irq_storm_detect (struct drm_device *dev,u32 hotplug_trigger,const u32 *hpd) {drm_i915_private_t *dev_priv = dev->dev_private;unsigned long irqflags;int i; +bool ret = false;spin_lock_irqsave (&dev_priv->irq_lock, irqflags);
@@ -598,12 +624,15 @@ static inline void hotplug_irq_storm_detect(struct drm_device *dev, }else if (dev_priv->hpd_stats[i].hpd_cnt >5 ) { dev_priv->hpd_stats[i].hpd_mark =HPD_MARK_DISABLED ;DRM_DEBUG_KMS ("HPD interrupt storm detected on PIN %d\n" , i); + ret = true; }else dev_priv->hpd_stats[i].hpd_cnt++; } }spin_unlock_irqrestore (&dev_priv->irq_lock, irqflags); + +return ret; }static void gmbus_irq_handler (struct drm_device *dev)
@@ -679,7 +708,8 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status);if (hotplug_trigger) { -hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_status_i915); +if (hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_status_i915)) +i915_hpd_irq_setup (dev);queue_work (dev_priv->wq, &dev_priv->hotplug_work); }
@@ -709,7 +739,8 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)u32 hotplug_trigger = pch_iir &SDE_HOTPLUG_MASK ;if (hotplug_trigger) { -hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_ibx); +if (hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_ibx)) +ibx_hpd_irq_setup (dev);queue_work (dev_priv->wq, &dev_priv->hotplug_work); }if (pch_iir &SDE_AUDIO_POWER_MASK )
@@ -757,7 +788,8 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)u32 hotplug_trigger = pch_iir &SDE_HOTPLUG_MASK_CPT ;if (hotplug_trigger) { -hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_cpt); +if (hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_cpt)) +ibx_hpd_irq_setup (dev);queue_work (dev_priv->wq, &dev_priv->hotplug_work); }if (pch_iir &SDE_AUDIO_POWER_MASK_CPT )
@@ -2034,12 +2066,14 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)if (HAS_PCH_IBX (dev)) { mask &= ~SDE_HOTPLUG_MASK ;list_for_each_entry (intel_encoder, &mode_config->encoder_list, base.head) - mask |= hpd_ibx[intel_encoder->hpd_pin]; +if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark ==HPD_ENABLED ) + mask |= hpd_ibx[intel_encoder->hpd_pin]; mask |=SDE_GMBUS |SDE_AUX_MASK ; }else { mask &= ~SDE_HOTPLUG_MASK_CPT ;list_for_each_entry (intel_encoder, &mode_config->encoder_list, base.head) - mask |= hpd_cpt[intel_encoder->hpd_pin]; +if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark ==HPD_ENABLED ) + mask |= hpd_cpt[intel_encoder->hpd_pin]; mask |=SDE_GMBUS_CPT |SDE_AUX_MASK_CPT ; }I915_WRITE (SDEIIR ,I915_READ (SDEIIR ));
@@ -2505,7 +2539,8 @@ static irqreturn_t i915_irq_handler(int irq, void *arg)DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status);if (hotplug_trigger) { -hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_status_i915); +if (hotplug_irq_storm_detect (dev, hotplug_trigger, hpd_status_i915)) +i915_hpd_irq_setup (dev);queue_work (dev_priv->wq, &dev_priv->hotplug_work); }
@@ -2659,7 +2694,7 @@ static void i915_hpd_irq_setup(struct drm_device *dev) {drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;struct drm_mode_config *mode_config = &dev->mode_config; -struct intel_encoder *encoder; +struct intel_encoder *intel_encoder;u32 hotplug_en;if (I915_HAS_HOTPLUG (dev)) {
@@ -2668,7 +2703,8 @@ static void i915_hpd_irq_setup(struct drm_device *dev)/* Note HDMI and DP share hotplug bits */ /* enable bits are the same for all generations */ list_for_each_entry (intel_encoder, &mode_config->encoder_list, base.head) - hotplug_en |= hpd_mask_i915[intel_encoder->hpd_pin]; +if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark ==HPD_ENABLED ) + hotplug_en |= hpd_mask_i915[intel_encoder->hpd_pin];/* Programming the CRT detection parameters tends to generate a spurious hotplug event about three seconds later. So just do it once.
@@ -2742,8 +2778,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg)DRM_DEBUG_DRIVER ("hotplug event received, stat 0x%08x\n" , hotplug_status);if (hotplug_trigger) { -hotplug_irq_storm_detect (dev, hotplug_trigger, -IS_G4X (dev) ? hpd_status_gen4 : hpd_status_i965); +if (hotplug_irq_storm_detect (dev, hotplug_trigger, +IS_G4X (dev) ? hpd_status_gen4 : hpd_status_i965)) +i915_hpd_irq_setup (dev);queue_work (dev_priv->wq, &dev_priv->hotplug_work); }
Signature · 1 line
1.7.7
PATCH 10/12 DRM/i915: Add Reenable Timer to turn Hotplug Detection back on.
In reply to Egbert EichWe disable hoptplug detection when we encounter a hotplug event storm. Still hotplug detection is required on some outputs (like Display Port). The interrupt storm may be only temporary (on certain Dell Laptops for instance it happens at certain charging states of the system). Thus we enable it after a certain grace period (2 minutes). Should the interrupt storm persist it will be detected immediately and it will be disabled again.
--- drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_irq.c | 53 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 1 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +2 -0
@@ -1068,6 +1068,8 @@ typedef struct drm_i915_private { HPD_MARK_DISABLED = 2 } hpd_mark; } hpd_stats[HPD_NUM_PINS ]; +#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000) +struct timer_list hotplug_reenable_timer; } drm_i915_private_t;/* Iterate over initialised rings */
drivers/gpu/drm/i915/i915_irq.c +52 -1
96 lines not on this page · raw message
Signature · 1 line
1.7.7
Re: [PATCH v.2 10/12] DRM/i915: Add Reenable Timer to turn Hotplug Detection back on.
Quoted 9 lines · Egbert Eich
We disable hoptplug detection when we encounter a hotplug event storm. Still hotplug detection is required on some outputs (like Display Port). The interrupt storm may be only temporary (on certain Dell Laptops for instance it happens at certain charging states of the system). Thus we enable it after a certain grace period (2 minutes). Should the interrupt storm persist it will be detected immediately and it will be disabled again.
Signed-off-by Egbert Eich <[email protected]>
Oops, just noticed that you have an ibx_hpd_irq_setup patch, too ;-) I think I'll go with since that one's just gotten a bit a beating in review from Ville. Hopfully that's not too ugly to rebase over ... -Daniel
Quoted 128 lines
--- drivers/gpu/drm/i915/i915_drv.h | 2 + drivers/gpu/drm/i915/i915_irq.c | 53 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 54 insertions(+), 1 deletions(-)drivers/gpu/drm/i915/i915_drv.h +2
-0@@ -1068,6 +1068,8 @@ typedef struct drm_i915_private { HPD_MARK_DISABLED = 2 } hpd_mark; } hpd_stats[HPD_NUM_PINS ]; +#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000) +struct timer_list hotplug_reenable_timer; } drm_i915_private_t;/* Iterate over initialised rings */ drivers/gpu/drm/i915/i915_irq.c +52
-1@@ -376,8 +376,11 @@ static void i915_hotplug_work_func(struct work_struct *work) connector_disabled = true; } } -if (connector_disabled) +if (connector_disabled) {drm_kms_helper_poll_enable (dev);/* if there were no outputs to poll, poll is disabled */ +mod_timer (&dev_priv->hotplug_reenable_timer, + jiffies +msecs_to_jiffies (I915_REENABLE_HOTPLUG_DELAY )); + }spin_unlock_irqrestore (&dev_priv->irq_lock, irqflags);@@ -2253,6 +2256,8 @@ static void valleyview_irq_uninstall(struct drm_device *dev)if (!dev_priv)return ; +del_timer_sync (&dev_priv->hotplug_reenable_timer); +for_each_pipe (pipe)I915_WRITE (PIPESTAT (pipe),0xffff );@@ -2274,6 +2279,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)if (!dev_priv)return ; +del_timer_sync (&dev_priv->hotplug_reenable_timer); +I915_WRITE (HWSTAM ,0xffffffff );I915_WRITE (DEIMR ,0xffffffff );@@ -2612,6 +2619,8 @@ static void i915_irq_uninstall(struct drm_device * dev)drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;int pipe; +del_timer_sync (&dev_priv->hotplug_reenable_timer); +if (I915_HAS_HOTPLUG (dev)) {I915_WRITE (PORT_HOTPLUG_EN ,0 );I915_WRITE (PORT_HOTPLUG_STAT ,I915_READ (PORT_HOTPLUG_STAT ));@@ -2860,6 +2869,8 @@ static void i965_irq_uninstall(struct drm_device * dev)if (!dev_priv)return ; +del_timer_sync (&dev_priv->hotplug_reenable_timer); +I915_WRITE (PORT_HOTPLUG_EN ,0 );I915_WRITE (PORT_HOTPLUG_STAT ,I915_READ (PORT_HOTPLUG_STAT ));@@ -2875,6 +2886,44 @@ static void i965_irq_uninstall(struct drm_device * dev)I915_WRITE (IIR ,I915_READ (IIR )); } +static void i915_reenable_hotplug_timer_func (unsigned long data) +{ +drm_i915_private_t *dev_priv = (drm_i915_private_t *)data; +struct drm_device *dev = dev_priv->dev; +struct drm_mode_config *mode_config = &dev->mode_config; +unsigned long irqflags; +int i; + +spin_lock_irqsave (&dev_priv->irq_lock, irqflags); +for (i =1 ; i <HPD_NUM_PINS ; i++) { +if (dev_priv->hpd_stats[i].hpd_mark ==HPD_MARK_DISABLED ) { +struct drm_connector *connector; + + dev_priv->hpd_stats[i].hpd_mark =HPD_ENABLED ; +list_for_each_entry (connector, &mode_config->connector_list, head) { +struct intel_connector *intel_connector =to_intel_connector (connector); + +if (intel_connector->encoder->hpd_pin == i) { +if (connector->polled != intel_connector->polled) +DRM_DEBUG_DRIVER ("Reenabling HPD on connector %s\n" , +drm_get_connector_name (connector)); + connector->polled = intel_connector->polled; +if (!connector->polled) + connector->polled =DRM_CONNECTOR_POLL_HPD ; + } + } + +if (IS_HASWELL (dev) || +IS_IVYBRIDGE (dev) || + (HAS_PCH_SPLIT (dev))) +ibx_hpd_irq_setup (dev); +else +i915_hpd_irq_setup (dev); + } + } +spin_unlock_irqrestore (&dev_priv->irq_lock, irqflags); +} +void intel_irq_init (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private;@@ -2887,6 +2936,8 @@ void intel_irq_init(struct drm_device *dev)setup_timer (&dev_priv->gpu_error.hangcheck_timer, i915_hangcheck_elapsed, (unsigned long ) dev); +setup_timer (&dev_priv->hotplug_reenable_timer, i915_reenable_hotplug_timer_func, + (unsigned long ) dev_priv);pm_qos_add_request (&dev_priv->pm_qos,PM_QOS_CPU_DMA_LATENCY ,PM_QOS_DEFAULT_VALUE );Signature · 1 line
1.7.7
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
PATCH 11/12 DRM/i915: Add bit field to record which pins have received HPD events.
In reply to Egbert EichThis way it is possible to limit 're'-detect() of displays to connectors which have received an HPD event.
--- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_irq.c | 10 ++++++++++ 2 files changed, 11 insertions(+), 0 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +1 -0
@@ -1068,6 +1068,7 @@ typedef struct drm_i915_private { HPD_MARK_DISABLED = 2 } hpd_mark; } hpd_stats[HPD_NUM_PINS ]; +u32 hpd_event_bits;#define I915_REENABLE_HOTPLUG_DELAY (2*60*1000)struct timer_list hotplug_reenable_timer; } drm_i915_private_t;
drivers/gpu/drm/i915/i915_irq.c +10 -0
@@ -353,6 +353,7 @@ static void i915_hotplug_work_func(struct work_struct *work)struct drm_connector *connector;unsigned long irqflags;bool connector_disabled = false; +u32 hpd_event_bits;/* HPD irq before everything is fully set up. */ if (!dev_priv->enable_hotplug_processing)
@@ -362,6 +363,9 @@ static void i915_hotplug_work_func(struct work_struct *work)DRM_DEBUG_KMS ("running encoder hotplug functions\n" );spin_lock_irqsave (&dev_priv->irq_lock, irqflags); + + hpd_event_bits = dev_priv->hpd_event_bits; + dev_priv->hpd_event_bits =0 ;list_for_each_entry (connector, &mode_config->connector_list, head) { intel_connector =to_intel_connector (connector); intel_encoder = intel_connector->encoder;
@@ -375,6 +379,10 @@ static void i915_hotplug_work_func(struct work_struct *work) | DRM_CONNECTOR_POLL_DISCONNECT; connector_disabled = true; } +if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) { +DRM_DEBUG_KMS ("Connector %s (pin %i) received hotplug event.\n" , +drm_get_connector_name (connector), intel_encoder->hpd_pin); + } }if (connector_disabled) {drm_kms_helper_poll_enable (dev);/* if there were no outputs to poll, poll is disabled */
@@ -629,12 +637,14 @@ static inline bool hotplug_irq_storm_detect(struct drm_device *dev,for (i =1 ; i <HPD_NUM_PINS ; i++) {if ((hpd[i] & hotplug_trigger) && dev_priv->hpd_stats[i].hpd_mark ==HPD_ENABLED ) { + dev_priv->hpd_event_bits |= (1 << i);if (jiffies > (dev_priv->hpd_stats[i].hpd_last_jiffies +msecs_to_jiffies (1000 )) || jiffies < dev_priv->hpd_stats[i].hpd_last_jiffies) { dev_priv->hpd_stats[i].hpd_last_jiffies = jiffies; dev_priv->hpd_stats[i].hpd_cnt =0 ; }else if (dev_priv->hpd_stats[i].hpd_cnt >5 ) { dev_priv->hpd_stats[i].hpd_mark =HPD_MARK_DISABLED ; + dev_priv->hpd_event_bits &= ~(1 << i);DRM_DEBUG_KMS ("HPD interrupt storm detected on PIN %d\n" , i); ret = true; }else
Signature · 1 line
1.7.7
PATCH 12/12 DRM/i915: Only reprobe display on encoder which has received an HPD event.
In reply to Egbert EichInstead of calling into the DRM helper layer to poll all connectors for changes in connected displays probe only those connectors which have received a hotplug event.
--- drivers/gpu/drm/i915/i915_irq.c | 37 +++++++++++++++++++++++++++++++------ 1 files changed, 31 insertions(+), 6 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +31 -6
@@ -339,6 +339,24 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe, crtc); } +/** +* drm_helper_hpd_irq_single_connector_event() - call this function with mode_config.mutex lock held +*/ + +static int intel_hpd_irq_single_connector_event (struct drm_device *dev,struct drm_connector *connector) +{ +enum drm_connector_status old_status; + + old_status = connector->status; + + connector->status = connector->funcs->detect (connector, false); +DRM_DEBUG_KMS ("[CONNECTOR:%d:%s] status updated from %d to %d\n" , + connector->base.id, +drm_get_connector_name (connector), + old_status, connector->status); +return (old_status != connector->status); +} +/* * Handle hotplug events outside the interrupt handler proper. */
@@ -353,6 +371,7 @@ static void i915_hotplug_work_func(struct work_struct *work)struct drm_connector *connector;unsigned long irqflags;bool connector_disabled = false; +bool changed = false;u32 hpd_event_bits;/* HPD irq before everything is fully set up. */
@@ -392,14 +411,20 @@ static void i915_hotplug_work_func(struct work_struct *work)spin_unlock_irqrestore (&dev_priv->irq_lock, irqflags); -list_for_each_entry (intel_encoder, &mode_config->encoder_list, base.head) -if (intel_encoder->hot_plug) - intel_encoder->hot_plug (encoder); - +list_for_each_entry (connector, &mode_config->connector_list, head) { + intel_connector =to_intel_connector (connector); + intel_encoder = intel_connector->encoder; +if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) { +if (intel_encoder->hot_plug) + intel_encoder->hot_plug (intel_encoder); +if (intel_hpd_irq_single_connector_event (dev, connector)) + changed = true; + } + }mutex_unlock (&mode_config->mutex); -/* Just fire off a uevent and let userspace tell us what to do */ -drm_helper_hpd_irq_event (dev); +if (changed) +drm_kms_helper_hotplug_event (dev); }static void ironlake_handle_rps_change (struct drm_device *dev)
Signature · 1 line
1.7.7
PATCH 12/12 DRM/i915: Only reprobe display on encoder which has received an HPD event.
Instead of calling into the DRM helper layer to poll all connectors for changes in connected displays probe only those connectors which have received a hotplug event.
--- drivers/gpu/drm/i915/i915_irq.c | 37 +++++++++++++++++++++++++++++++------ 1 files changed, 31 insertions(+), 6 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +31 -6
@@ -330,6 +330,24 @@ static int i915_get_vblank_timestamp(struct drm_device *dev, int pipe, crtc); } +/** +* drm_helper_hpd_irq_single_connector_event() - call this function with mode_config.mutex lock held +*/ + +static int intel_hpd_irq_single_connector_event (struct drm_device *dev,struct drm_connector *connector) +{ +enum drm_connector_status old_status; + + old_status = connector->status; + + connector->status = connector->funcs->detect (connector, false); +DRM_DEBUG_KMS ("[CONNECTOR:%d:%s] status updated from %d to %d\n" , + connector->base.id, +drm_get_connector_name (connector), + old_status, connector->status); +return (old_status != connector->status); +} +/* * Handle hotplug events outside the interrupt handler proper. */
@@ -344,6 +362,7 @@ static void i915_hotplug_work_func(struct work_struct *work)struct drm_connector *connector;unsigned long irqflags;bool connector_disabled = false; +bool changed = false;u32 hpd_event_bits;/* HPD irq before everything is fully set up. */
@@ -383,14 +402,20 @@ static void i915_hotplug_work_func(struct work_struct *work)spin_unlock_irqrestore (&dev_priv->irq_lock, irqflags); -list_for_each_entry (intel_encoder, &mode_config->encoder_list, base.head) -if (intel_encoder->hot_plug) - intel_encoder->hot_plug (intel_encoder); - +list_for_each_entry (connector, &mode_config->connector_list, head) { + intel_connector =to_intel_connector (connector); + intel_encoder = intel_connector->encoder; +if (hpd_event_bits & (1 << intel_encoder->hpd_pin)) { +if (intel_encoder->hot_plug) + intel_encoder->hot_plug (intel_encoder); +if (intel_hpd_irq_single_connector_event (dev, connector)) + changed = true; + } + }mutex_unlock (&mode_config->mutex); -/* Just fire off a uevent and let userspace tell us what to do */ -drm_helper_hpd_irq_event (dev); +if (changed) +drm_kms_helper_hotplug_event (dev); }static void ironlake_handle_rps_change (struct drm_device *dev)
Signature · 1 line
1.7.7
Quoted 14 lines · Egbert Eich
I've reworked my 'hotplug interrupt storm detection'-patches and included most of Daniel's suggestions.
I've looked into adding EDID caching but since this requires some larger scale changes and some changes outside of the Intel driver it seemed to be a good idea to propose those changes at a later time.Egbert Eich (12):
DRM/i915: Removevalleyview_hpd_irq_setup.
DRM/I915: Add enumhpd_pintointel_encoder.
DRM/i915: Convert HPD interrupts to make use of HPD pin assignment in
encoders.
DRM/i915: Removei965_hpd_irq_setup.
DRM/i915: Get rid if the 'hotplug_supported_mask' in structdrm_i915_private.
Quoted 10 lines
DRM/i915: Add HPD IRQ storm detection.
DRM/i915: (re)init HPD interrupt storm statistics. DRM/i915: Treathpd_irq_setup()for ironake and older generations the
same way.
DRM/i915: Disable HPD interrupt on pin when irq storm is detected. DRM/i915: Add Reenable Timer to turn Hotplug Detection back on. DRM/i915: Add bit field to record which pins have received HPD
events.
DRM/i915: Only reprobe display on encoder which has received an HPD
event.
I'm happy with this series. It seems to do everything that we need to enable/disable hpd, and tidies up hotplug detection in the process.
Didn't spot anything wrong in the first pass, but I'd like to let the details soak in a bit before r-b.
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre