On Thu, Aug 27, 2015 at 10:38:25AM +0300, Jani Nikula wrote:
> On Thu, 27 Aug 2015, Paulo Zanoni <przanoni@xxxxxxxxx> wrote:
> > 2015-08-12 12:44 GMT-03:00 <ville.syrjala@xxxxxxxxxxxxxxx>:
> >> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >>
> >> 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@xxxxxxxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxx>
> 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.
OK, let me have a look at this mess in a bit more detail and see what can
be done. Paulo's idea at least is simple enough to do, so I'll do at
least that.
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> >
> > --
> > Paulo Zanoni
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
