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

[Todd Previte]

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>

> 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>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20140624/cb2ac0c0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: postbox-contact.jpg
Type: image/jpeg
Size: 1291 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20140624/cb2ac0c0/attachment.jpg>