On Thu, Aug 27, 2015 at 07:13:49PM +0300, Ville Syrjälä wrote:
> 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.
I think what I'll do is leave intel_get_hpd_pins() mostly intact, except
I'll move the pin_mask/long_mask 0 initialization out into the callers.
That way anyone can call it multiple times and accumulate all the bits to
the same two masks, and then call intel_hpd_irq_handler just once.
Going back to platform specific variants of the function didn't seem too
nice as I'd probably then need to add more variants for port A HPD etc.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
