On Wed, May 27, 2015 at 02:41:29PM +0200, Daniel Vetter wrote:
> On Wed, May 27, 2015 at 03:03:43PM +0300, Jani Nikula wrote:
> > Add platform specific functions to decipher the register contents
> > instead of just returning the shift value. Use macros instead of magic
> > numbers to look at the register bits.
> >
> > As a side effect, if an unsupported port is passed, consistently return
> > false (indicating short hotplug) instead of returning -1 which was used
> > for shifting.
> >
> > Signed-off-by: Jani Nikula <jani.nik...@intel.com>
>
> Not convinced. Currently we have a strange mix of passing register+decode
> tables to intel_hpd_irq_handler for hpds and decoding the long vs. short
> stuff internally. That already doesn't work with things like port A which
> tends to be (at least on some platforms) in a totally different register.
> And it splits up the definitions for the hpd bits from the long vs. short
> bits unecessarily. I even suspect we might have some bugs with correctly
> clearing those additional status registers sometimes.
>
> Imo if we clean this up we should pull it all together into one place,
> i.e. platform code does all the register reading&decoding and passes
> decode bitmasks down to generic handlers. Or something along those lines
> at least.
Forgot to add: Merged all the other patches to dinq, thanks.
-Daniel
> -Daniel
>
> > ---
> > drivers/gpu/drm/i915/i915_irq.c | 40
> > ++++++++++++++++------------------------
> > 1 file changed, 16 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index 91cb0b6ec47b..90cc248691d7 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1375,35 +1375,31 @@ static irqreturn_t gen8_gt_irq_handler(struct
> > drm_i915_private *dev_priv,
> > #define HPD_STORM_DETECT_PERIOD 1000
> > #define HPD_STORM_THRESHOLD 5
> >
> > -static int pch_port_to_hotplug_shift(enum port port)
> > +static bool pch_port_hotplug_long_detect(enum port port, u32 val)
> > {
> > switch (port) {
> > - case PORT_A:
> > - case PORT_E:
> > - default:
> > - return -1;
> > case PORT_B:
> > - return 0;
> > + return val & PORTB_HOTPLUG_LONG_DETECT;
> > case PORT_C:
> > - return 8;
> > + return val & PORTC_HOTPLUG_LONG_DETECT;
> > case PORT_D:
> > - return 16;
> > + return val & PORTD_HOTPLUG_LONG_DETECT;
> > + default:
> > + return false;
> > }
> > }
> >
> > -static int i915_port_to_hotplug_shift(enum port port)
> > +static bool i915_port_hotplug_long_detect(enum port port, u32 val)
> > {
> > switch (port) {
> > - case PORT_A:
> > - case PORT_E:
> > - default:
> > - return -1;
> > case PORT_B:
> > - return 17;
> > + return val & PORTB_HOTPLUG_INT_LONG_PULSE;
> > case PORT_C:
> > - return 19;
> > + return val & PORTC_HOTPLUG_INT_LONG_PULSE;
> > case PORT_D:
> > - return 21;
> > + return val & PORTD_HOTPLUG_INT_LONG_PULSE;
> > + default:
> > + return false;
> > }
> > }
> >
> > @@ -1431,7 +1427,6 @@ static void intel_hpd_irq_handler(struct drm_device
> > *dev,
> > 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)
> > @@ -1451,13 +1446,10 @@ static void intel_hpd_irq_handler(struct drm_device
> > *dev,
> > if (!port || !dev_priv->hotplug.irq_port[port])
> > continue;
> >
> > - if (!HAS_GMCH_DISPLAY(dev_priv)) {
> > - dig_shift = pch_port_to_hotplug_shift(port);
> > - long_hpd = (dig_hotplug_reg >> dig_shift) &
> > PORTB_HOTPLUG_LONG_DETECT;
> > - } else {
> > - dig_shift = i915_port_to_hotplug_shift(port);
> > - long_hpd = (hotplug_trigger >> dig_shift) &
> > PORTB_HOTPLUG_LONG_DETECT;
> > - }
> > + if (HAS_GMCH_DISPLAY(dev_priv))
> > + long_hpd = i915_port_hotplug_long_detect(port,
> > hotplug_trigger);
> > + else
> > + long_hpd = pch_port_hotplug_long_detect(port,
> > dig_hotplug_reg);
> >
> > DRM_DEBUG_DRIVER("digital hpd port %c - %s\n", port_name(port),
> > long_hpd ? "long" : "short");
> > --
> > 2.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
