[Intel-gfx] [PATCH 06/11] drm/i915: Introduce spt_irq_handler()
Jani Nikula
jani.nikula at linux.intel.com
Thu Aug 27 00:38:25 PDT 2015
On Thu, 27 Aug 2015, Paulo Zanoni <przanoni at gmail.com> wrote:
> 2015-08-12 12:44 GMT-03:00 <ville.syrjala at linux.intel.com>:
>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>
>> Starting from SPT the only interrupts living in the south are GMBUS and
>> HPD. What's worse some of the SPT specific new bits conflict with some
>> other bits on earlier PCH generations. So better not use the
>> cpt_irq_handler() for SPT+ anymore.
>>
>> Also kill the hand rolled port E handling with something more
>> standardish. This also avoids accidentally confusing port B and port E
>> long pulses since the bits occupy the same positions, just in different
>> registers.
>>
>> Also add a comment noting that the short pulse duration bits are
>> reserved on LPT+. The 2ms value we program is 0, so no issue wrt. the
>> MBZ in the spec.
>>
>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_irq.c | 123 +++++++++++++++++++++++++++-------------
>> 1 file changed, 83 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index d12106c..e2485bd 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1260,6 +1260,16 @@ static bool bxt_port_hotplug_long_detect(enum port port, u32 val)
>> }
>> }
>>
>> +static bool spt_port_hotplug2_long_detect(enum port port, u32 val)
>> +{
>> + switch (port) {
>> + case PORT_E:
>> + return val & PORTE_HOTPLUG_LONG_DETECT;
>> + default:
>> + return false;
>> + }
>> +}
>> +
>> static bool pch_port_hotplug_long_detect(enum port port, u32 val)
>> {
>> switch (port) {
>> @@ -1269,8 +1279,6 @@ static bool pch_port_hotplug_long_detect(enum port port, u32 val)
>> return val & PORTC_HOTPLUG_LONG_DETECT;
>> case PORT_D:
>> return val & PORTD_HOTPLUG_LONG_DETECT;
>> - case PORT_E:
>> - return val & PORTE_HOTPLUG_LONG_DETECT;
>> default:
>> return false;
>> }
>> @@ -1771,12 +1779,7 @@ 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;
>> -
>> - if (HAS_PCH_SPT(dev))
>> - hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_SPT;
>> - else
>> - hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
>> + u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_CPT;
>>
>> if (hotplug_trigger) {
>> u32 dig_hotplug_reg, pin_mask, long_mask;
>> @@ -1784,22 +1787,10 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>> dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
>> I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg);
>>
>> - if (HAS_PCH_SPT(dev)) {
>> - intel_get_hpd_pins(&pin_mask, &long_mask,
>> - hotplug_trigger,
>> - dig_hotplug_reg, hpd_spt,
>> - pch_port_hotplug_long_detect);
>> -
>> - /* detect PORTE HP event */
>> - dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG2);
>> - if (pch_port_hotplug_long_detect(PORT_E,
>> - dig_hotplug_reg))
>> - long_mask |= 1 << HPD_PORT_E;
>> - } else
>> - intel_get_hpd_pins(&pin_mask, &long_mask,
>> - hotplug_trigger,
>> - dig_hotplug_reg, hpd_cpt,
>> - pch_port_hotplug_long_detect);
>> + intel_get_hpd_pins(&pin_mask, &long_mask,
>> + hotplug_trigger,
>> + dig_hotplug_reg, hpd_cpt,
>> + pch_port_hotplug_long_detect);
>>
>> intel_hpd_irq_handler(dev, pin_mask, long_mask);
>> }
>> @@ -1833,6 +1824,42 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
>> cpt_serr_int_handler(dev);
>> }
>>
>> +static void spt_irq_handler(struct drm_device *dev, u32 pch_iir)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + u32 hotplug_trigger = pch_iir & SDE_HOTPLUG_MASK_SPT &
>> + ~SDE_PORTE_HOTPLUG_SPT;
>> + u32 hotplug2_trigger = pch_iir & SDE_PORTE_HOTPLUG_SPT;
>> +
>> + if (hotplug_trigger) {
>> + u32 dig_hotplug_reg, pin_mask, long_mask;
>> +
>> + dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG);
>> + I915_WRITE(PCH_PORT_HOTPLUG, dig_hotplug_reg);
>> +
>> + intel_get_hpd_pins(&pin_mask, &long_mask, hotplug_trigger,
>> + dig_hotplug_reg, hpd_spt,
>> + pch_port_hotplug_long_detect);
>> + intel_hpd_irq_handler(dev, pin_mask, long_mask);
>> + }
>> +
>> + if (hotplug2_trigger) {
>> + u32 dig_hotplug_reg, pin_mask, long_mask;
>> +
>> + dig_hotplug_reg = I915_READ(PCH_PORT_HOTPLUG2);
>> + I915_WRITE(PCH_PORT_HOTPLUG2, dig_hotplug_reg);
>> +
>> + intel_get_hpd_pins(&pin_mask, &long_mask, hotplug2_trigger,
>> + dig_hotplug_reg, hpd_spt,
>> + spt_port_hotplug2_long_detect);
>> +
>> + intel_hpd_irq_handler(dev, pin_mask, long_mask);
>> + }
>
> Instead of calling intel_hpd_irq_handler() twice, can't you just do
> something like:
> intel_hpd_irq_hanlder(dev, pin_mask1 | pin_mask2, long_mask1 | long_mask2) ?
And if you reverted
commit fd63e2a972c670887e5e8a08440111d3812c0996
Author: Imre Deak <imre.deak at intel.com>
Date: Tue Jul 21 15:32:44 2015 -0700
drm/i915: combine i9xx_get_hpd_pins and pch_get_hpd_pins
and added another version for spt, this would all be pretty and each of
the functions would do exactly what is required and you could have
meaningful, platform specific debug logging in each of the functions.
By focusing on deduplicating a few lines in get_hpd_pins, the callers
(which are platform specific code to begin with) get more complicated.
BR,
Jani.
>
> Everything else looks good (except for the fact that I would have
> split this patch in like 5 different patches).
>
>> +
>> + if (pch_iir & SDE_GMBUS_CPT)
>> + gmbus_irq_handler(dev);
>> +}
>> +
>> static void ilk_display_irq_handler(struct drm_device *dev, u32 de_iir)
>> {
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -2151,7 +2178,11 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg)
>> if (pch_iir) {
>> I915_WRITE(SDEIIR, pch_iir);
>> ret = IRQ_HANDLED;
>> - cpt_irq_handler(dev, pch_iir);
>> +
>> + if (HAS_PCH_SPT(dev_priv))
>> + spt_irq_handler(dev, pch_iir);
>> + else
>> + cpt_irq_handler(dev, pch_iir);
>> } else
>> DRM_ERROR("The master control interrupt lied (SDE)!\n");
>>
>> @@ -3033,9 +3064,6 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
>> if (HAS_PCH_IBX(dev)) {
>> hotplug_irqs = SDE_HOTPLUG_MASK;
>> enabled_irqs = intel_hpd_enabled_irqs(dev, hpd_ibx);
>> - } else if (HAS_PCH_SPT(dev)) {
>> - hotplug_irqs = SDE_HOTPLUG_MASK_SPT;
>> - enabled_irqs = intel_hpd_enabled_irqs(dev, hpd_spt);
>> } else {
>> hotplug_irqs = SDE_HOTPLUG_MASK_CPT;
>> enabled_irqs = intel_hpd_enabled_irqs(dev, hpd_cpt);
>> @@ -3045,9 +3073,8 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
>>
>> /*
>> * Enable digital hotplug on the PCH, and configure the DP short pulse
>> - * duration to 2ms (which is the minimum in the Display Port spec)
>> - *
>> - * This register is the same on all known PCH chips.
>> + * duration to 2ms (which is the minimum in the Display Port spec).
>> + * The pulse duration bits are reserved on LPT+.
>> */
>> hotplug = I915_READ(PCH_PORT_HOTPLUG);
>> hotplug &= ~(PORTD_PULSE_DURATION_MASK|PORTC_PULSE_DURATION_MASK|PORTB_PULSE_DURATION_MASK);
>> @@ -3055,13 +3082,27 @@ static void ibx_hpd_irq_setup(struct drm_device *dev)
>> hotplug |= PORTC_HOTPLUG_ENABLE | PORTC_PULSE_DURATION_2ms;
>> hotplug |= PORTB_HOTPLUG_ENABLE | PORTB_PULSE_DURATION_2ms;
>> I915_WRITE(PCH_PORT_HOTPLUG, hotplug);
>> +}
>> +
>> +static void spt_hpd_irq_setup(struct drm_device *dev)
>> +{
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> + u32 hotplug_irqs, hotplug, enabled_irqs;
>> +
>> + hotplug_irqs = SDE_HOTPLUG_MASK_SPT;
>> + enabled_irqs = intel_hpd_enabled_irqs(dev, hpd_spt);
>> +
>> + ibx_display_interrupt_update(dev_priv, hotplug_irqs, enabled_irqs);
>> +
>> + /* Enable digital hotplug on the PCH */
>> + hotplug = I915_READ(PCH_PORT_HOTPLUG);
>> + hotplug |= PORTD_HOTPLUG_ENABLE | PORTC_HOTPLUG_ENABLE |
>> + PORTB_HOTPLUG_ENABLE;
>> + I915_WRITE(PCH_PORT_HOTPLUG, hotplug);
>>
>> - /* enable SPT PORTE hot plug */
>> - if (HAS_PCH_SPT(dev)) {
>> - hotplug = I915_READ(PCH_PORT_HOTPLUG2);
>> - hotplug |= PORTE_HOTPLUG_ENABLE;
>> - I915_WRITE(PCH_PORT_HOTPLUG2, hotplug);
>> - }
>> + hotplug = I915_READ(PCH_PORT_HOTPLUG2);
>> + hotplug |= PORTE_HOTPLUG_ENABLE;
>> + I915_WRITE(PCH_PORT_HOTPLUG2, hotplug);
>> }
>>
>> static void bxt_hpd_irq_setup(struct drm_device *dev)
>> @@ -4166,10 +4207,12 @@ void intel_irq_init(struct drm_i915_private *dev_priv)
>> dev->driver->irq_uninstall = gen8_irq_uninstall;
>> dev->driver->enable_vblank = gen8_enable_vblank;
>> dev->driver->disable_vblank = gen8_disable_vblank;
>> - if (HAS_PCH_SPLIT(dev))
>> - dev_priv->display.hpd_irq_setup = ibx_hpd_irq_setup;
>> - else
>> + if (IS_BROXTON(dev))
>> dev_priv->display.hpd_irq_setup = bxt_hpd_irq_setup;
>> + else if (HAS_PCH_SPT(dev))
>> + dev_priv->display.hpd_irq_setup = spt_hpd_irq_setup;
>> + else
>> + 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_reset;
>> --
>> 2.4.6
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list