[Intel-gfx] [PATCH 2/2] drm/i915: rework digital port IRQ handling (v2)

Mon Jul 7 15:13:02 CEST 2014

On Tue, Jun 24, 2014 at 03:00:51PM -0700, Todd Previte wrote:
> This looks like it's good to go.
> 
> As an aside, I don't *think* any of the compliance testing stuff I'm working
> on cares whether it's short of long pulse (1.1a compliance), but it will be
> interesting to see if/when/where it might have an effect.
> 
> Reviewed-by: Todd Previte <tprevite at gmail.com>

Queued for -next, thanks for the patch.
-Daniel
> 
> >Dave Airlie <mailto:airlied at gmail.com>
> >Tuesday, June 17, 2014 6:29 PM
> >From: Dave Airlie <airlied at redhat.com>
> >
> >The digital ports from Ironlake and up have the ability to distinguish
> >between long and short HPD pulses. Displayport 1.1 only uses the short
> >form to request link retraining usually, so we haven't really needed
> >support for it until now.
> >
> >However with DP 1.2 MST we need to handle the short irqs on their
> >own outside the modesetting locking the long hpd's involve. This
> >patch adds the framework to distinguish between short/long to the
> >current code base, to lay the basis for future DP 1.2 MST work.
> >
> >This should mean we get better bisectability in case of regression
> >due to the new irq handling.
> >
> >v2: add GM45 support (untested, due to lack of hw)
> >
> >Signed-off-by: Dave Airlie <airlied at redhat.com>
> >---
> >drivers/gpu/drm/i915/i915_drv.h | 5 ++
> >drivers/gpu/drm/i915/i915_irq.c | 160
> >+++++++++++++++++++++++++++++++++++++--
> >drivers/gpu/drm/i915/intel_ddi.c | 3 +
> >drivers/gpu/drm/i915/intel_dp.c | 20 +++++
> >drivers/gpu/drm/i915/intel_drv.h | 2 +
> >5 files changed, 182 insertions(+), 8 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >b/drivers/gpu/drm/i915/i915_drv.h
> >index 8f68678..5fd5bf3 100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -1551,6 +1551,11 @@ struct drm_i915_private {
> >
> >struct i915_runtime_pm pm;
> >
> >+ struct intel_digital_port *hpd_irq_port[I915_MAX_PORTS];
> >+ u32 long_hpd_port_mask;
> >+ u32 short_hpd_port_mask;
> >+ struct work_struct dig_port_work;
> >+
> >/* Old dri1 support infrastructure, beware the dragons ya fools entering
> >* here! */
> >struct i915_dri1_state dri1;
> >diff --git a/drivers/gpu/drm/i915/i915_irq.c
> >b/drivers/gpu/drm/i915/i915_irq.c
> >index cbf31cb..9913c08 100644
> >--- a/drivers/gpu/drm/i915/i915_irq.c
> >+++ b/drivers/gpu/drm/i915/i915_irq.c
> >@@ -1096,6 +1096,53 @@ static bool intel_hpd_irq_event(struct drm_device
> >*dev,
> >return true;
> >}
> >
> >+static void i915_digport_work_func(struct work_struct *work)
> >+{
> >+ struct drm_i915_private *dev_priv =
> >+ container_of(work, struct drm_i915_private, dig_port_work);
> >+ unsigned long irqflags;
> >+ u32 long_port_mask, short_port_mask;
> >+ struct intel_digital_port *intel_dig_port;
> >+ int i, ret;
> >+ u32 old_bits = 0;
> >+
> >+ spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> >+ long_port_mask = dev_priv->long_hpd_port_mask;
> >+ dev_priv->long_hpd_port_mask = 0;
> >+ short_port_mask = dev_priv->short_hpd_port_mask;
> >+ dev_priv->short_hpd_port_mask = 0;
> >+ spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> >+
> >+ for (i = 0; i < I915_MAX_PORTS; i++) {
> >+ bool valid = false;
> >+ bool long_hpd = false;
> >+ intel_dig_port = dev_priv->hpd_irq_port[i];
> >+ if (!intel_dig_port || !intel_dig_port->hpd_pulse)
> >+ continue;
> >+
> >+ if (long_port_mask & (1 << i)) {
> >+ valid = true;
> >+ long_hpd = true;
> >+ } else if (short_port_mask & (1 << i))
> >+ valid = true;
> >+
> >+ if (valid) {
> >+ ret = intel_dig_port->hpd_pulse(intel_dig_port, long_hpd);
> >+ if (ret == true) {
> >+ /* if we get true fallback to old school hpd */
> >+ old_bits |= (1 << intel_dig_port->base.hpd_pin);
> >+ }
> >+ }
> >+ }
> >+
> >+ if (old_bits) {
> >+ spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> >+ dev_priv->hpd_event_bits |= old_bits;
> >+ spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> >+ schedule_work(&dev_priv->hotplug_work);
> >+ }
> >+}
> >+
> >/*
> >* Handle hotplug events outside the interrupt handler proper.
> >*/
> >@@ -1520,23 +1567,104 @@ static irqreturn_t gen8_gt_irq_handler(struct
> >drm_device *dev,
> >#define HPD_STORM_DETECT_PERIOD 1000
> >#define HPD_STORM_THRESHOLD 5
> >
> >+static int ilk_port_to_hotplug_shift(enum port port)
> >+{
> >+ switch (port) {
> >+ case PORT_A:
> >+ case PORT_E:
> >+ default:
> >+ return -1;
> >+ case PORT_B:
> >+ return 0;
> >+ case PORT_C:
> >+ return 8;
> >+ case PORT_D:
> >+ return 16;
> >+ }
> >+}
> >+
> >+static int g4x_port_to_hotplug_shift(enum port port)
> >+{
> >+ switch (port) {
> >+ case PORT_A:
> >+ case PORT_E:
> >+ default:
> >+ return -1;
> >+ case PORT_B:
> >+ return 17;
> >+ case PORT_C:
> >+ return 19;
> >+ case PORT_D:
> >+ return 21;
> >+ }
> >+}
> >+
> >+static inline enum port get_port_from_pin(enum hpd_pin pin)
> >+{
> >+ switch (pin) {
> >+ case HPD_PORT_B:
> >+ return PORT_B;
> >+ case HPD_PORT_C:
> >+ return PORT_C;
> >+ case HPD_PORT_D:
> >+ return PORT_D;
> >+ default:
> >+ return PORT_A; /* no hpd */
> >+ }
> >+}
> >+
> >static inline void intel_hpd_irq_handler(struct drm_device *dev,
> >u32 hotplug_trigger,
> >+ u32 dig_hotplug_reg,
> >const u32 *hpd)
> >{
> >struct drm_i915_private *dev_priv = dev->dev_private;
> >int i;
> >+ enum port port;
> >bool storm_detected = false;
> >+ bool queue_dig = false, queue_hp = false;
> >+ u32 dig_shift;
> >+ u32 dig_port_mask = 0;
> >
> >if (!hotplug_trigger)
> >return;
> >
> >- DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x\n",
> >- hotplug_trigger);
> >+ DRM_DEBUG_DRIVER("hotplug event received, stat 0x%08x, dig 0x%08x\n",
> >+ hotplug_trigger, dig_hotplug_reg);
> >
> >spin_lock(&dev_priv->irq_lock);
> >for (i = 1; i < HPD_NUM_PINS; i++) {
> >+ if (!(hpd[i] & hotplug_trigger))
> >+ continue;
> >+
> >+ port = get_port_from_pin(i);
> >+ if (port && dev_priv->hpd_irq_port[port]) {
> >+ bool long_hpd;
> >+
> >+ if (IS_G4X(dev)) {
> >+ dig_shift = g4x_port_to_hotplug_shift(port);
> >+ long_hpd = (hotplug_trigger >> dig_shift) & PORTB_HOTPLUG_LONG_DETECT;
> >+ } else {
> >+ dig_shift = ilk_port_to_hotplug_shift(port);
> >+ long_hpd = (dig_hotplug_reg >> dig_shift) & PORTB_HOTPLUG_LONG_DETECT;
> >+ }
> >+
> >+ DRM_DEBUG_DRIVER("digital hpd port %d %d\n", port, long_hpd);
> >+ /* for long HPD pulses we want to have the digital queue happen,
> >+ but we still want HPD storm detection to function. */
> >+ if (long_hpd) {
> >+ dev_priv->long_hpd_port_mask |= (1 << port);
> >+ dig_port_mask |= hpd[i];
> >+ } else {
> >+ /* for short HPD just trigger the digital queue */
> >+ dev_priv->short_hpd_port_mask |= (1 << port);
> >+ hotplug_trigger &= ~hpd[i];
> >+ }
> >+ queue_dig = true;
> >+ }
> >+ }
> >
> >+ for (i = 1; i < HPD_NUM_PINS; i++) {
> >if (hpd[i] & hotplug_trigger &&
> >dev_priv->hpd_stats[i].hpd_mark == HPD_DISABLED) {
> >/*
> >@@ -1556,7 +1684,11 @@ static inline void intel_hpd_irq_handler(struct
> >drm_device *dev,
> >dev_priv->hpd_stats[i].hpd_mark != HPD_ENABLED)
> >continue;
> >
> >- dev_priv->hpd_event_bits |= (1 << i);
> >+ if (!(dig_port_mask & hpd[i])) {
> >+ dev_priv->hpd_event_bits |= (1 << i);
> >+ queue_hp = true;
> >+ }
> >+
> >if (!time_in_range(jiffies, dev_priv->hpd_stats[i].hpd_last_jiffies,
> >dev_priv->hpd_stats[i].hpd_last_jiffies
> >+ msecs_to_jiffies(HPD_STORM_DETECT_PERIOD))) {
> >@@ -1585,7 +1717,10 @@ static inline void intel_hpd_irq_handler(struct
> >drm_device *dev,
> >* queue for otherwise the flush_work in the pageflip code will
> >* deadlock.
> >*/
> >- schedule_work(&dev_priv->hotplug_work);
> >+ if (queue_dig)
> >+ schedule_work(&dev_priv->dig_port_work);
> >+ if (queue_hp)
> >+ schedule_work(&dev_priv->hotplug_work);
> >}
> >
> >static void gmbus_irq_handler(struct drm_device *dev)
> >@@ -1818,11 +1953,11 @@ static void i9xx_hpd_irq_handler(struct drm_device
> >*dev)
> >if (IS_G4X(dev)) {
> >u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_G4X;
> >
> >- intel_hpd_irq_handler(dev, hotplug_trigger, hpd_status_g4x);
> >+ intel_hpd_irq_handler(dev, hotplug_trigger, 0, hpd_status_g4x);
> >} else {
> >u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
> >
> >- intel_hpd_irq_handler(dev, hotplug_trigger, hpd_status_i915);
> >+ intel_hpd_irq_handler(dev, hotplug_trigger, 0, hpd_status_i915);
> >}
> >
> >if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) &&
> >@@ -1913,8 +2048,12 @@ static void ibx_irq_handler(struct drm_device *dev,
> >u32 pch_iir)
> >struct drm_i915_private *dev_priv = dev->dev_private;
> >int pipe;
> >u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK;
> >+ u32 dig_hotplug_reg;
> >+
> >+ dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
> >+ I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg);
> >
> >- intel_hpd_irq_handler(dev, hotplug_trigger, hpd_ibx);
> >+ intel_hpd_irq_handler(dev, hotplug_trigger, dig_hotplug_reg, hpd_ibx);
> >
> >if (pch_iir & SDE_AUDIO_POWER_MASK) {
> >int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK) >>
> >@@ -2020,8 +2159,12 @@ static void cpt_irq_handler(struct drm_device *dev,
> >u32 pch_iir)
> >struct drm_i915_private *dev_priv = dev->dev_private;
> >int pipe;
> >u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
> >+ u32 dig_hotplug_reg;
> >+
> >+ dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
> >+ I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg);
> >
> >- intel_hpd_irq_handler(dev, hotplug_trigger, hpd_cpt);
> >+ intel_hpd_irq_handler(dev, hotplug_trigger, dig_hotplug_reg, hpd_cpt);
> >
> >if (pch_iir & SDE_AUDIO_POWER_MASK_CPT) {
> >int port = ffs((pch_iir & SDE_AUDIO_POWER_MASK_CPT) >>
> >@@ -4338,6 +4481,7 @@ void intel_irq_init(struct drm_device *dev)
> >struct drm_i915_private *dev_priv = dev->dev_private;
> >
> >INIT_WORK(&dev_priv->hotplug_work, i915_hotplug_work_func);
> >+ INIT_WORK(&dev_priv->dig_port_work, i915_digport_work_func);
> >INIT_WORK(&dev_priv->gpu_error.work, i915_error_work_func);
> >INIT_WORK(&dev_priv->rps.work, gen6_pm_rps_work);
> >INIT_WORK(&dev_priv->l3_parity.error_work, ivybridge_parity_work);
> >diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> >b/drivers/gpu/drm/i915/intel_ddi.c
> >index b17b9c7..a80cb3e 100644
> >--- a/drivers/gpu/drm/i915/intel_ddi.c
> >+++ b/drivers/gpu/drm/i915/intel_ddi.c
> >@@ -1705,6 +1705,9 @@ void intel_ddi_init(struct drm_device *dev, enum
> >port port)
> >intel_encoder->cloneable = 0;
> >intel_encoder->hot_plug = intel_ddi_hot_plug;
> >
> >+ intel_dig_port->hpd_pulse = intel_dp_hpd_pulse;
> >+ dev_priv->hpd_irq_port[port] = intel_dig_port;
> >+
> >if (init_dp)
> >dp_connector = intel_ddi_init_dp_connector(intel_dig_port);
> >
> >diff --git a/drivers/gpu/drm/i915/intel_dp.c
> >b/drivers/gpu/drm/i915/intel_dp.c
> >index d01bb43..f110522 100644
> >--- a/drivers/gpu/drm/i915/intel_dp.c
> >+++ b/drivers/gpu/drm/i915/intel_dp.c
> >@@ -3699,6 +3699,22 @@ intel_dp_hot_plug(struct intel_encoder
> >*intel_encoder)
> >intel_dp_check_link_status(intel_dp);
> >}
> >
> >+bool
> >+intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool
> >long_hpd)
> >+{
> >+ struct intel_dp *intel_dp = &intel_dig_port->dp;
> >+
> >+ if (long_hpd)
> >+ return true;
> >+
> >+ /*
> >+ * we'll check the link status via the normal hot plug path later -
> >+ * but for short hpds we should check it now
> >+ */
> >+ intel_dp_check_link_status(intel_dp);
> >+ return false;
> >+}
> >+
> >/* Return which DP Port should be selected for Transcoder DP control */
> >int
> >intel_trans_dp_port_sel(struct drm_crtc *crtc)
> >@@ -4273,6 +4289,7 @@ intel_dp_init_connector(struct intel_digital_port
> >*intel_dig_port,
> >void
> >intel_dp_init(struct drm_device *dev, int output_reg, enum port port)
> >{
> >+ struct drm_i915_private *dev_priv = dev->dev_private;
> >struct intel_digital_port *intel_dig_port;
> >struct intel_encoder *intel_encoder;
> >struct drm_encoder *encoder;
> >@@ -4328,6 +4345,9 @@ intel_dp_init(struct drm_device *dev, int
> >output_reg, enum port port)
> >intel_encoder->cloneable = 0;
> >intel_encoder->hot_plug = intel_dp_hot_plug;
> >
> >+ intel_dig_port->hpd_pulse = intel_dp_hpd_pulse;
> >+ dev_priv->hpd_irq_port[port] = intel_dig_port;
> >+
> >if (!intel_dp_init_connector(intel_dig_port, intel_connector)) {
> >drm_encoder_cleanup(encoder);
> >kfree(intel_dig_port);
> >diff --git a/drivers/gpu/drm/i915/intel_drv.h
> >b/drivers/gpu/drm/i915/intel_drv.h
> >index f1d5897..9666ec3 100644
> >--- a/drivers/gpu/drm/i915/intel_drv.h
> >+++ b/drivers/gpu/drm/i915/intel_drv.h
> >@@ -563,6 +563,7 @@ struct intel_digital_port {
> >u32 saved_port_bits;
> >struct intel_dp dp;
> >struct intel_hdmi hdmi;
> >+ bool (*hpd_pulse)(struct intel_digital_port *, bool);
> >};
> >
> >static inline int
> >@@ -830,6 +831,7 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp);
> >void intel_edp_psr_disable(struct intel_dp *intel_dp);
> >void intel_edp_psr_update(struct drm_device *dev);
> >void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
> >+bool intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool
> >long_hpd);
> >
> >/* intel_dsi.c */
> >bool intel_dsi_init(struct drm_device *dev);
> >Dave Airlie <mailto:airlied at gmail.com>
> >Tuesday, June 17, 2014 6:29 PM
> >Can we get these merged or even looked at?, they are blocking the whole
> >MST progress,
> >and I don't have any insight to secret Intel review process. :-)
> >
> >Dave.
> >
> >_______________________________________________
> >Intel-gfx mailing list
> >Intel-gfx at lists.freedesktop.org
> >http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Sent with Postbox <http://www.getpostbox.com>

> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch