| Message ID | 6be56c568ce8498fb2dc3cc86fc516b185d8ec20.1432728144.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.nikula@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. -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
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.nikula@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
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");
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.nikula@intel.com> --- drivers/gpu/drm/i915/i915_irq.c | 40 ++++++++++++++++------------------------ 1 file changed, 16 insertions(+), 24 deletions(-)