* [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info @ 2017-10-24 10:41 Sagar Arun Kamble 2017-10-24 10:46 ` Michal Wajdeczko ` (3 more replies) 0 siblings, 4 replies; 12+ messages in thread From: Sagar Arun Kamble @ 2017-10-24 10:41 UTC (permalink / raw) To: intel-gfx PM interrupt register offsets are constant per platforms and saving those in device info is more appropriate than getting those through functions. This patch removes functions gen6_pm_iir/imr/ier and saves those offsets in device info. v2: Use INTEL_INFO() to access device info. (Chris) Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 5 +++++ drivers/gpu/drm/i915/i915_irq.c | 31 +++++++++---------------------- drivers/gpu/drm/i915/intel_device_info.c | 11 +++++++++++ 3 files changed, 25 insertions(+), 22 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 54b5d4c..2f77d26 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -888,6 +888,11 @@ struct intel_device_info { u16 degamma_lut_size; u16 gamma_lut_size; } color; + + /* PM interrupt register offsets */ + i915_reg_t pm_iir_offset; + i915_reg_t pm_imr_offset; + i915_reg_t pm_ier_offset; }; struct intel_display_error_state; diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index b1296a5..5d448af 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -306,21 +306,6 @@ void gen5_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask) ilk_update_gt_irq(dev_priv, mask, 0); } -static i915_reg_t gen6_pm_iir(struct drm_i915_private *dev_priv) -{ - return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IIR(2) : GEN6_PMIIR; -} - -static i915_reg_t gen6_pm_imr(struct drm_i915_private *dev_priv) -{ - return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IMR(2) : GEN6_PMIMR; -} - -static i915_reg_t gen6_pm_ier(struct drm_i915_private *dev_priv) -{ - return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IER(2) : GEN6_PMIER; -} - /** * snb_update_pm_irq - update GEN6_PMIMR * @dev_priv: driver private @@ -332,6 +317,7 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv, uint32_t enabled_irq_mask) { uint32_t new_val; + i915_reg_t reg = INTEL_INFO(dev_priv)->pm_imr_offset; WARN_ON(enabled_irq_mask & ~interrupt_mask); @@ -343,8 +329,8 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv, if (new_val != dev_priv->pm_imr) { dev_priv->pm_imr = new_val; - I915_WRITE(gen6_pm_imr(dev_priv), dev_priv->pm_imr); - POSTING_READ(gen6_pm_imr(dev_priv)); + I915_WRITE(reg, dev_priv->pm_imr); + POSTING_READ(reg); } } @@ -371,7 +357,7 @@ void gen6_mask_pm_irq(struct drm_i915_private *dev_priv, u32 mask) static void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 reset_mask) { - i915_reg_t reg = gen6_pm_iir(dev_priv); + i915_reg_t reg = INTEL_INFO(dev_priv)->pm_iir_offset; lockdep_assert_held(&dev_priv->irq_lock); @@ -385,7 +371,7 @@ static void gen6_enable_pm_irq(struct drm_i915_private *dev_priv, u32 enable_mas lockdep_assert_held(&dev_priv->irq_lock); dev_priv->pm_ier |= enable_mask; - I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier); + I915_WRITE(INTEL_INFO(dev_priv)->pm_ier_offset, dev_priv->pm_ier); gen6_unmask_pm_irq(dev_priv, enable_mask); /* unmask_pm_irq provides an implicit barrier (POSTING_READ) */ } @@ -396,7 +382,7 @@ static void gen6_disable_pm_irq(struct drm_i915_private *dev_priv, u32 disable_m dev_priv->pm_ier &= ~disable_mask; __gen6_mask_pm_irq(dev_priv, disable_mask); - I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier); + I915_WRITE(INTEL_INFO(dev_priv)->pm_ier_offset, dev_priv->pm_ier); /* though a barrier is missing here, but don't really need a one */ } @@ -417,7 +403,8 @@ void gen6_enable_rps_interrupts(struct drm_i915_private *dev_priv) spin_lock_irq(&dev_priv->irq_lock); WARN_ON_ONCE(rps->pm_iir); - WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & dev_priv->pm_rps_events); + WARN_ON_ONCE(I915_READ(INTEL_INFO(dev_priv)->pm_iir_offset) & + dev_priv->pm_rps_events); rps->interrupts_enabled = true; gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events); @@ -461,7 +448,7 @@ void gen9_enable_guc_interrupts(struct drm_i915_private *dev_priv) { spin_lock_irq(&dev_priv->irq_lock); if (!dev_priv->guc.interrupts_enabled) { - WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & + WARN_ON_ONCE(I915_READ(INTEL_INFO(dev_priv)->pm_iir_offset) & dev_priv->pm_guc_events); dev_priv->guc.interrupts_enabled = true; gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events); diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c index 875d428..d1a4911 100644 --- a/drivers/gpu/drm/i915/intel_device_info.c +++ b/drivers/gpu/drm/i915/intel_device_info.c @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv) info->sseu.has_subslice_pg ? "y" : "n"); DRM_DEBUG_DRIVER("has EU power gating: %s\n", info->sseu.has_eu_pg ? "y" : "n"); + + /* Initialize PM interrupt register offsets */ + if (INTEL_GEN(dev_priv) >= 8) { + info->pm_iir_offset = GEN8_GT_IIR(2); + info->pm_imr_offset = GEN8_GT_IMR(2); + info->pm_ier_offset = GEN8_GT_IER(2); + } else { + info->pm_iir_offset = GEN6_PMIIR; + info->pm_imr_offset = GEN6_PMIMR; + info->pm_ier_offset = GEN6_PMIER; + } } -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info 2017-10-24 10:41 [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info Sagar Arun Kamble @ 2017-10-24 10:46 ` Michal Wajdeczko 2017-10-24 16:34 ` Sagar Arun Kamble 2017-10-24 12:09 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/1] " Patchwork ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ messages in thread From: Michal Wajdeczko @ 2017-10-24 10:46 UTC (permalink / raw) To: intel-gfx, Sagar Arun Kamble On Tue, 24 Oct 2017 12:41:13 +0200, Sagar Arun Kamble <sagar.a.kamble@intel.com> wrote: > PM interrupt register offsets are constant per platforms and saving those > in device info is more appropriate than getting those through functions. > This patch removes functions gen6_pm_iir/imr/ier and saves those offsets > in device info. > > v2: Use INTEL_INFO() to access device info. (Chris) > > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 5 +++++ > drivers/gpu/drm/i915/i915_irq.c | 31 > +++++++++---------------------- > drivers/gpu/drm/i915/intel_device_info.c | 11 +++++++++++ > 3 files changed, 25 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index 54b5d4c..2f77d26 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -888,6 +888,11 @@ struct intel_device_info { > u16 degamma_lut_size; > u16 gamma_lut_size; > } color; > + > + /* PM interrupt register offsets */ > + i915_reg_t pm_iir_offset; > + i915_reg_t pm_imr_offset; > + i915_reg_t pm_ier_offset; > }; > struct intel_display_error_state; > diff --git a/drivers/gpu/drm/i915/i915_irq.c > b/drivers/gpu/drm/i915/i915_irq.c > index b1296a5..5d448af 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -306,21 +306,6 @@ void gen5_disable_gt_irq(struct drm_i915_private > *dev_priv, uint32_t mask) > ilk_update_gt_irq(dev_priv, mask, 0); > } > -static i915_reg_t gen6_pm_iir(struct drm_i915_private *dev_priv) > -{ > - return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IIR(2) : GEN6_PMIIR; > -} > - > -static i915_reg_t gen6_pm_imr(struct drm_i915_private *dev_priv) > -{ > - return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IMR(2) : GEN6_PMIMR; > -} > - > -static i915_reg_t gen6_pm_ier(struct drm_i915_private *dev_priv) > -{ > - return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IER(2) : GEN6_PMIER; > -} > - btw, if you keep these functions but modify them into: return INTEL_INFO(dev_priv)->pm_xxx_offset; then most of below changes will not be needed > /** > * snb_update_pm_irq - update GEN6_PMIMR > * @dev_priv: driver private > @@ -332,6 +317,7 @@ static void snb_update_pm_irq(struct > drm_i915_private *dev_priv, > uint32_t enabled_irq_mask) > { > uint32_t new_val; > + i915_reg_t reg = INTEL_INFO(dev_priv)->pm_imr_offset; s/reg/imr ? > WARN_ON(enabled_irq_mask & ~interrupt_mask); > @@ -343,8 +329,8 @@ static void snb_update_pm_irq(struct > drm_i915_private *dev_priv, > if (new_val != dev_priv->pm_imr) { > dev_priv->pm_imr = new_val; > - I915_WRITE(gen6_pm_imr(dev_priv), dev_priv->pm_imr); > - POSTING_READ(gen6_pm_imr(dev_priv)); > + I915_WRITE(reg, dev_priv->pm_imr); > + POSTING_READ(reg); > } > } > @@ -371,7 +357,7 @@ void gen6_mask_pm_irq(struct drm_i915_private > *dev_priv, u32 mask) > static void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 > reset_mask) > { > - i915_reg_t reg = gen6_pm_iir(dev_priv); > + i915_reg_t reg = INTEL_INFO(dev_priv)->pm_iir_offset; s/reg/iir ? > lockdep_assert_held(&dev_priv->irq_lock); > @@ -385,7 +371,7 @@ static void gen6_enable_pm_irq(struct > drm_i915_private *dev_priv, u32 enable_mas > lockdep_assert_held(&dev_priv->irq_lock); > dev_priv->pm_ier |= enable_mask; > - I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier); > + I915_WRITE(INTEL_INFO(dev_priv)->pm_ier_offset, dev_priv->pm_ier); > gen6_unmask_pm_irq(dev_priv, enable_mask); > /* unmask_pm_irq provides an implicit barrier (POSTING_READ) */ > } > @@ -396,7 +382,7 @@ static void gen6_disable_pm_irq(struct > drm_i915_private *dev_priv, u32 disable_m > dev_priv->pm_ier &= ~disable_mask; > __gen6_mask_pm_irq(dev_priv, disable_mask); > - I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier); > + I915_WRITE(INTEL_INFO(dev_priv)->pm_ier_offset, dev_priv->pm_ier); > /* though a barrier is missing here, but don't really need a one */ > } > @@ -417,7 +403,8 @@ void gen6_enable_rps_interrupts(struct > drm_i915_private *dev_priv) > spin_lock_irq(&dev_priv->irq_lock); > WARN_ON_ONCE(rps->pm_iir); > - WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & > dev_priv->pm_rps_events); > + WARN_ON_ONCE(I915_READ(INTEL_INFO(dev_priv)->pm_iir_offset) & > + dev_priv->pm_rps_events); Can you define separate iir_reg variable as in above functions to simplify this nested statement ? > rps->interrupts_enabled = true; > gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events); > @@ -461,7 +448,7 @@ void gen9_enable_guc_interrupts(struct > drm_i915_private *dev_priv) > { > spin_lock_irq(&dev_priv->irq_lock); > if (!dev_priv->guc.interrupts_enabled) { > - WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & > + WARN_ON_ONCE(I915_READ(INTEL_INFO(dev_priv)->pm_iir_offset) & > dev_priv->pm_guc_events); > dev_priv->guc.interrupts_enabled = true; > gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events); > diff --git a/drivers/gpu/drm/i915/intel_device_info.c > b/drivers/gpu/drm/i915/intel_device_info.c > index 875d428..d1a4911 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct > drm_i915_private *dev_priv) > info->sseu.has_subslice_pg ? "y" : "n"); > DRM_DEBUG_DRIVER("has EU power gating: %s\n", > info->sseu.has_eu_pg ? "y" : "n"); > + > + /* Initialize PM interrupt register offsets */ > + if (INTEL_GEN(dev_priv) >= 8) { > + info->pm_iir_offset = GEN8_GT_IIR(2); > + info->pm_imr_offset = GEN8_GT_IMR(2); > + info->pm_ier_offset = GEN8_GT_IER(2); > + } else { > + info->pm_iir_offset = GEN6_PMIIR; > + info->pm_imr_offset = GEN6_PMIMR; > + info->pm_ier_offset = GEN6_PMIER; > + } > } _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info 2017-10-24 10:46 ` Michal Wajdeczko @ 2017-10-24 16:34 ` Sagar Arun Kamble 0 siblings, 0 replies; 12+ messages in thread From: Sagar Arun Kamble @ 2017-10-24 16:34 UTC (permalink / raw) To: Michal Wajdeczko, intel-gfx On 10/24/2017 4:16 PM, Michal Wajdeczko wrote: > On Tue, 24 Oct 2017 12:41:13 +0200, Sagar Arun Kamble > <sagar.a.kamble@intel.com> wrote: > >> PM interrupt register offsets are constant per platforms and saving >> those >> in device info is more appropriate than getting those through functions. >> This patch removes functions gen6_pm_iir/imr/ier and saves those offsets >> in device info. >> >> v2: Use INTEL_INFO() to access device info. (Chris) >> >> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 5 +++++ >> drivers/gpu/drm/i915/i915_irq.c | 31 >> +++++++++---------------------- >> drivers/gpu/drm/i915/intel_device_info.c | 11 +++++++++++ >> 3 files changed, 25 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index 54b5d4c..2f77d26 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -888,6 +888,11 @@ struct intel_device_info { >> u16 degamma_lut_size; >> u16 gamma_lut_size; >> } color; >> + >> + /* PM interrupt register offsets */ >> + i915_reg_t pm_iir_offset; >> + i915_reg_t pm_imr_offset; >> + i915_reg_t pm_ier_offset; >> }; >> struct intel_display_error_state; >> diff --git a/drivers/gpu/drm/i915/i915_irq.c >> b/drivers/gpu/drm/i915/i915_irq.c >> index b1296a5..5d448af 100644 >> --- a/drivers/gpu/drm/i915/i915_irq.c >> +++ b/drivers/gpu/drm/i915/i915_irq.c >> @@ -306,21 +306,6 @@ void gen5_disable_gt_irq(struct drm_i915_private >> *dev_priv, uint32_t mask) >> ilk_update_gt_irq(dev_priv, mask, 0); >> } >> -static i915_reg_t gen6_pm_iir(struct drm_i915_private *dev_priv) >> -{ >> - return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IIR(2) : GEN6_PMIIR; >> -} >> - >> -static i915_reg_t gen6_pm_imr(struct drm_i915_private *dev_priv) >> -{ >> - return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IMR(2) : GEN6_PMIMR; >> -} >> - >> -static i915_reg_t gen6_pm_ier(struct drm_i915_private *dev_priv) >> -{ >> - return INTEL_GEN(dev_priv) >= 8 ? GEN8_GT_IER(2) : GEN6_PMIER; >> -} >> - > > btw, if you keep these functions but modify them into: > > return INTEL_INFO(dev_priv)->pm_xxx_offset; > > then most of below changes will not be needed Yes. Will keep these functions. > >> /** >> * snb_update_pm_irq - update GEN6_PMIMR >> * @dev_priv: driver private >> @@ -332,6 +317,7 @@ static void snb_update_pm_irq(struct >> drm_i915_private *dev_priv, >> uint32_t enabled_irq_mask) >> { >> uint32_t new_val; >> + i915_reg_t reg = INTEL_INFO(dev_priv)->pm_imr_offset; > > s/reg/imr ? will remove this change since we are keeping gen6_pm* functions. > >> WARN_ON(enabled_irq_mask & ~interrupt_mask); >> @@ -343,8 +329,8 @@ static void snb_update_pm_irq(struct >> drm_i915_private *dev_priv, >> if (new_val != dev_priv->pm_imr) { >> dev_priv->pm_imr = new_val; >> - I915_WRITE(gen6_pm_imr(dev_priv), dev_priv->pm_imr); >> - POSTING_READ(gen6_pm_imr(dev_priv)); >> + I915_WRITE(reg, dev_priv->pm_imr); >> + POSTING_READ(reg); >> } >> } >> @@ -371,7 +357,7 @@ void gen6_mask_pm_irq(struct drm_i915_private >> *dev_priv, u32 mask) >> static void gen6_reset_pm_iir(struct drm_i915_private *dev_priv, u32 >> reset_mask) >> { >> - i915_reg_t reg = gen6_pm_iir(dev_priv); >> + i915_reg_t reg = INTEL_INFO(dev_priv)->pm_iir_offset; > > s/reg/iir ? will remove this as well. > >> lockdep_assert_held(&dev_priv->irq_lock); >> @@ -385,7 +371,7 @@ static void gen6_enable_pm_irq(struct >> drm_i915_private *dev_priv, u32 enable_mas >> lockdep_assert_held(&dev_priv->irq_lock); >> dev_priv->pm_ier |= enable_mask; >> - I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier); >> + I915_WRITE(INTEL_INFO(dev_priv)->pm_ier_offset, dev_priv->pm_ier); >> gen6_unmask_pm_irq(dev_priv, enable_mask); >> /* unmask_pm_irq provides an implicit barrier (POSTING_READ) */ >> } >> @@ -396,7 +382,7 @@ static void gen6_disable_pm_irq(struct >> drm_i915_private *dev_priv, u32 disable_m >> dev_priv->pm_ier &= ~disable_mask; >> __gen6_mask_pm_irq(dev_priv, disable_mask); >> - I915_WRITE(gen6_pm_ier(dev_priv), dev_priv->pm_ier); >> + I915_WRITE(INTEL_INFO(dev_priv)->pm_ier_offset, dev_priv->pm_ier); >> /* though a barrier is missing here, but don't really need a one */ >> } >> @@ -417,7 +403,8 @@ void gen6_enable_rps_interrupts(struct >> drm_i915_private *dev_priv) >> spin_lock_irq(&dev_priv->irq_lock); >> WARN_ON_ONCE(rps->pm_iir); >> - WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & >> dev_priv->pm_rps_events); >> + WARN_ON_ONCE(I915_READ(INTEL_INFO(dev_priv)->pm_iir_offset) & >> + dev_priv->pm_rps_events); > > Can you define separate iir_reg variable as in above functions to > simplify > this nested statement ? and this as well. > >> rps->interrupts_enabled = true; >> gen6_enable_pm_irq(dev_priv, dev_priv->pm_rps_events); >> @@ -461,7 +448,7 @@ void gen9_enable_guc_interrupts(struct >> drm_i915_private *dev_priv) >> { >> spin_lock_irq(&dev_priv->irq_lock); >> if (!dev_priv->guc.interrupts_enabled) { >> - WARN_ON_ONCE(I915_READ(gen6_pm_iir(dev_priv)) & >> + WARN_ON_ONCE(I915_READ(INTEL_INFO(dev_priv)->pm_iir_offset) & >> dev_priv->pm_guc_events); >> dev_priv->guc.interrupts_enabled = true; >> gen6_enable_pm_irq(dev_priv, dev_priv->pm_guc_events); >> diff --git a/drivers/gpu/drm/i915/intel_device_info.c >> b/drivers/gpu/drm/i915/intel_device_info.c >> index 875d428..d1a4911 100644 >> --- a/drivers/gpu/drm/i915/intel_device_info.c >> +++ b/drivers/gpu/drm/i915/intel_device_info.c >> @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct >> drm_i915_private *dev_priv) >> info->sseu.has_subslice_pg ? "y" : "n"); >> DRM_DEBUG_DRIVER("has EU power gating: %s\n", >> info->sseu.has_eu_pg ? "y" : "n"); >> + >> + /* Initialize PM interrupt register offsets */ >> + if (INTEL_GEN(dev_priv) >= 8) { >> + info->pm_iir_offset = GEN8_GT_IIR(2); >> + info->pm_imr_offset = GEN8_GT_IMR(2); >> + info->pm_ier_offset = GEN8_GT_IER(2); >> + } else { >> + info->pm_iir_offset = GEN6_PMIIR; >> + info->pm_imr_offset = GEN6_PMIMR; >> + info->pm_ier_offset = GEN6_PMIER; >> + } >> } _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [v2,1/1] drm/i915: Save PM interrupt register offsets in device info 2017-10-24 10:41 [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info Sagar Arun Kamble 2017-10-24 10:46 ` Michal Wajdeczko @ 2017-10-24 12:09 ` Patchwork 2017-10-24 13:03 ` ✓ Fi.CI.IGT: " Patchwork 2017-10-24 16:43 ` [PATCH v2 1/1] " Chris Wilson 3 siblings, 0 replies; 12+ messages in thread From: Patchwork @ 2017-10-24 12:09 UTC (permalink / raw) To: Sagar Arun Kamble; +Cc: intel-gfx == Series Details == Series: series starting with [v2,1/1] drm/i915: Save PM interrupt register offsets in device info URL : https://patchwork.freedesktop.org/series/32526/ State : success == Summary == Series 32526v1 series starting with [v2,1/1] drm/i915: Save PM interrupt register offsets in device info https://patchwork.freedesktop.org/api/1.0/series/32526/revisions/1/mbox/ Test drv_module_reload: Subgroup basic-no-display: dmesg-warn -> INCOMPLETE (fi-cfl-s) fdo#103206 fdo#103206 https://bugs.freedesktop.org/show_bug.cgi?id=103206 fi-bdw-5557u total:289 pass:268 dwarn:0 dfail:0 fail:0 skip:21 time:447s fi-bdw-gvtdvm total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:451s fi-blb-e6850 total:289 pass:223 dwarn:1 dfail:0 fail:0 skip:65 time:370s fi-bsw-n3050 total:289 pass:243 dwarn:0 dfail:0 fail:0 skip:46 time:518s fi-bwr-2160 total:289 pass:183 dwarn:0 dfail:0 fail:0 skip:106 time:262s fi-bxt-dsi total:289 pass:259 dwarn:0 dfail:0 fail:0 skip:30 time:494s fi-bxt-j4205 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:503s fi-byt-j1900 total:289 pass:253 dwarn:1 dfail:0 fail:0 skip:35 time:498s fi-byt-n2820 total:289 pass:249 dwarn:1 dfail:0 fail:0 skip:39 time:486s fi-cfl-s total:287 pass:253 dwarn:2 dfail:0 fail:0 skip:31 fi-cnl-y total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:635s fi-elk-e7500 total:289 pass:229 dwarn:0 dfail:0 fail:0 skip:60 time:422s fi-gdg-551 total:289 pass:178 dwarn:1 dfail:0 fail:1 skip:109 time:251s fi-glk-1 total:289 pass:261 dwarn:0 dfail:0 fail:0 skip:28 time:581s fi-glk-dsi total:289 pass:258 dwarn:0 dfail:0 fail:1 skip:30 time:492s fi-hsw-4770 total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:453s fi-hsw-4770r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:428s fi-ilk-650 total:289 pass:228 dwarn:0 dfail:0 fail:0 skip:61 time:434s fi-ivb-3520m total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:491s fi-ivb-3770 total:289 pass:260 dwarn:0 dfail:0 fail:0 skip:29 time:460s fi-kbl-7500u total:289 pass:264 dwarn:1 dfail:0 fail:0 skip:24 time:491s fi-kbl-7560u total:289 pass:270 dwarn:0 dfail:0 fail:0 skip:19 time:572s fi-kbl-7567u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:477s fi-kbl-r total:289 pass:262 dwarn:0 dfail:0 fail:0 skip:27 time:588s fi-pnv-d510 total:289 pass:222 dwarn:1 dfail:0 fail:0 skip:66 time:544s fi-skl-6260u total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:455s fi-skl-6700hq total:289 pass:263 dwarn:0 dfail:0 fail:0 skip:26 time:646s fi-skl-6700k total:289 pass:265 dwarn:0 dfail:0 fail:0 skip:24 time:519s fi-skl-6770hq total:289 pass:269 dwarn:0 dfail:0 fail:0 skip:20 time:503s fi-skl-gvtdvm total:289 pass:266 dwarn:0 dfail:0 fail:0 skip:23 time:451s fi-snb-2520m total:289 pass:250 dwarn:0 dfail:0 fail:0 skip:39 time:565s fi-snb-2600 total:289 pass:249 dwarn:0 dfail:0 fail:0 skip:40 time:421s 5c82a37eff83ab4e60e760fbaf03db5ba0563497 drm-tip: 2017y-10m-23d-18h-06m-28s UTC integration manifest 21074be0b1a8 drm/i915: Save PM interrupt register offsets in device info == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6155/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* ✓ Fi.CI.IGT: success for series starting with [v2,1/1] drm/i915: Save PM interrupt register offsets in device info 2017-10-24 10:41 [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info Sagar Arun Kamble 2017-10-24 10:46 ` Michal Wajdeczko 2017-10-24 12:09 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/1] " Patchwork @ 2017-10-24 13:03 ` Patchwork 2017-10-24 16:43 ` [PATCH v2 1/1] " Chris Wilson 3 siblings, 0 replies; 12+ messages in thread From: Patchwork @ 2017-10-24 13:03 UTC (permalink / raw) To: Sagar Arun Kamble; +Cc: intel-gfx == Series Details == Series: series starting with [v2,1/1] drm/i915: Save PM interrupt register offsets in device info URL : https://patchwork.freedesktop.org/series/32526/ State : success == Summary == Test kms_busy: Subgroup extended-modeset-hang-oldfb-with-reset-render-C: pass -> DMESG-WARN (shard-hsw) fdo#102249 +1 Test drv_module_reload: Subgroup basic-reload-inject: dmesg-warn -> PASS (shard-hsw) fdo#102707 Test perf: Subgroup polling: fail -> PASS (shard-hsw) fdo#102252 fdo#102249 https://bugs.freedesktop.org/show_bug.cgi?id=102249 fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707 fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252 shard-hsw total:2540 pass:1433 dwarn:2 dfail:0 fail:8 skip:1097 time:9224s == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6155/shards.html _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info 2017-10-24 10:41 [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info Sagar Arun Kamble ` (2 preceding siblings ...) 2017-10-24 13:03 ` ✓ Fi.CI.IGT: " Patchwork @ 2017-10-24 16:43 ` Chris Wilson 2017-10-24 17:48 ` Jani Nikula 3 siblings, 1 reply; 12+ messages in thread From: Chris Wilson @ 2017-10-24 16:43 UTC (permalink / raw) To: Sagar Arun Kamble, intel-gfx Quoting Sagar Arun Kamble (2017-10-24 11:41:13) > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c > index 875d428..d1a4911 100644 > --- a/drivers/gpu/drm/i915/intel_device_info.c > +++ b/drivers/gpu/drm/i915/intel_device_info.c > @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv) > info->sseu.has_subslice_pg ? "y" : "n"); > DRM_DEBUG_DRIVER("has EU power gating: %s\n", > info->sseu.has_eu_pg ? "y" : "n"); > + > + /* Initialize PM interrupt register offsets */ > + if (INTEL_GEN(dev_priv) >= 8) { > + info->pm_iir_offset = GEN8_GT_IIR(2); > + info->pm_imr_offset = GEN8_GT_IMR(2); > + info->pm_ier_offset = GEN8_GT_IER(2); > + } else { > + info->pm_iir_offset = GEN6_PMIIR; > + info->pm_imr_offset = GEN6_PMIMR; > + info->pm_ier_offset = GEN6_PMIER; > + } If you are going to take another pass at this, move these into the static tables in i915_pci.c Updating GEN6_FEATURES and GEN8_FEATURES will then percolate into individual platform defines. -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info 2017-10-24 16:43 ` [PATCH v2 1/1] " Chris Wilson @ 2017-10-24 17:48 ` Jani Nikula 2017-10-24 20:26 ` Tvrtko Ursulin 0 siblings, 1 reply; 12+ messages in thread From: Jani Nikula @ 2017-10-24 17:48 UTC (permalink / raw) To: Chris Wilson, Sagar Arun Kamble, intel-gfx On Tue, 24 Oct 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Sagar Arun Kamble (2017-10-24 11:41:13) >> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c >> index 875d428..d1a4911 100644 >> --- a/drivers/gpu/drm/i915/intel_device_info.c >> +++ b/drivers/gpu/drm/i915/intel_device_info.c >> @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv) >> info->sseu.has_subslice_pg ? "y" : "n"); >> DRM_DEBUG_DRIVER("has EU power gating: %s\n", >> info->sseu.has_eu_pg ? "y" : "n"); >> + >> + /* Initialize PM interrupt register offsets */ >> + if (INTEL_GEN(dev_priv) >= 8) { >> + info->pm_iir_offset = GEN8_GT_IIR(2); >> + info->pm_imr_offset = GEN8_GT_IMR(2); >> + info->pm_ier_offset = GEN8_GT_IER(2); >> + } else { >> + info->pm_iir_offset = GEN6_PMIIR; >> + info->pm_imr_offset = GEN6_PMIMR; >> + info->pm_ier_offset = GEN6_PMIER; >> + } > > If you are going to take another pass at this, move these into the > static tables in i915_pci.c > > Updating GEN6_FEATURES and GEN8_FEATURES will then percolate into > individual platform defines. Like I wrote in reply to v1, I'm not convinced we should do this at all. What makes *these* registers so important they must be in device info? What makes most of i915_reg.h so unimportant they don't deserve the same treatment? Where do you draw the line? I'd draw the line at, no registers at device info. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info 2017-10-24 17:48 ` Jani Nikula @ 2017-10-24 20:26 ` Tvrtko Ursulin 2017-10-25 7:45 ` Jani Nikula 0 siblings, 1 reply; 12+ messages in thread From: Tvrtko Ursulin @ 2017-10-24 20:26 UTC (permalink / raw) To: Jani Nikula, Chris Wilson, Sagar Arun Kamble, intel-gfx On 24/10/17 18:48, Jani Nikula wrote: > On Tue, 24 Oct 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> Quoting Sagar Arun Kamble (2017-10-24 11:41:13) >>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c >>> index 875d428..d1a4911 100644 >>> --- a/drivers/gpu/drm/i915/intel_device_info.c >>> +++ b/drivers/gpu/drm/i915/intel_device_info.c >>> @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv) >>> info->sseu.has_subslice_pg ? "y" : "n"); >>> DRM_DEBUG_DRIVER("has EU power gating: %s\n", >>> info->sseu.has_eu_pg ? "y" : "n"); >>> + >>> + /* Initialize PM interrupt register offsets */ >>> + if (INTEL_GEN(dev_priv) >= 8) { >>> + info->pm_iir_offset = GEN8_GT_IIR(2); >>> + info->pm_imr_offset = GEN8_GT_IMR(2); >>> + info->pm_ier_offset = GEN8_GT_IER(2); >>> + } else { >>> + info->pm_iir_offset = GEN6_PMIIR; >>> + info->pm_imr_offset = GEN6_PMIMR; >>> + info->pm_ier_offset = GEN6_PMIER; >>> + } >> >> If you are going to take another pass at this, move these into the >> static tables in i915_pci.c >> >> Updating GEN6_FEATURES and GEN8_FEATURES will then percolate into >> individual platform defines. > > Like I wrote in reply to v1, I'm not convinced we should do this at all. > > What makes *these* registers so important they must be in device info? > What makes most of i915_reg.h so unimportant they don't deserve the same > treatment? Where do you draw the line? > > I'd draw the line at, no registers at device info. I suggested to Sagar this change during review so feel responsible to chime in. So in general I just find the amount of times our driver asks itself what it's running on a bit tasteless. :( I did quick and dirty check by bumping a counter in all the IS_this|or|that checks, all which can be known at driver probe time, and wired it up to the PMU so I can check their frequency. The annotated perf stat output: root@e31:~# perf stat -a -e i915/whoami/ -I 1000 # time counts unit events # idle system no X running 1.000298100 10 i915/whoami/ 2.000750955 8 i915/whoami/ 3.001104193 10 i915/whoami/ 4.001333433 10 i915/whoami/ 5.001703162 10 i915/whoami/ 6.002122721 10 i915/whoami/ # starting X now.. 7.002266228 2,203 i915/whoami/ 8.002392598 4,682 i915/whoami/ 9.002764398 0 i915/whoami/ 10.003027119 0 i915/whoami/ 11.003486048 42 i915/whoami/ # X idling.. 12.003854660 0 i915/whoami/ 13.004221680 0 i915/whoami/ 14.004622571 0 i915/whoami/ 15.004968110 0 i915/whoami/ 16.005372363 0 i915/whoami/ 17.005778034 0 i915/whoami/ 18.005941970 0 i915/whoami/ 19.006313427 0 i915/whoami/ 20.006676048 0 i915/whoami/ 21.007059927 0 i915/whoami/ 22.007507818 0 i915/whoami/ 23.007887628 0 i915/whoami/ 24.008207035 0 i915/whoami/ 25.008580496 0 i915/whoami/ # time counts unit events 26.008949236 0 i915/whoami/ 27.009433473 0 i915/whoami/ # gfxbench trex starting up 28.009677600 2,605 i915/whoami/ 29.009941972 716 i915/whoami/ 30.010127588 2,190 i915/whoami/ 31.010249535 46 i915/whoami/ 32.010383565 36 i915/whoami/ 33.010527674 0 i915/whoami/ # trex running 34.010760584 4,709 i915/whoami/ 35.011079891 5,381 i915/whoami/ 36.011280234 5,306 i915/whoami/ 37.011719986 5,505 i915/whoami/ 38.012017531 5,386 i915/whoami/ 39.012529241 5,687 i915/whoami/ 40.012922986 6,009 i915/whoami/ 41.013120143 5,791 i915/whoami/ 42.013399982 5,296 i915/whoami/ 43.013712979 5,349 i915/whoami/ 44.014107375 5,127 i915/whoami/ 45.014553950 5,387 i915/whoami/ 46.014953020 5,364 i915/whoami/ 47.015243748 4,738 i915/whoami/ 48.015560460 4,788 i915/whoami/ 49.015867395 4,927 i915/whoami/ 50.016152690 4,886 i915/whoami/ So.. I am not saying these particular registers are mega important, and not even saying that these 5k/s conditionals are measurable (either as branches or increased code size effect), but overall the situation is a bit of.. bleurgh from the elegance point of view. :( If we have register sets which are 100% mutually exclusive, then I see them as candidates to put them in some object at probe time. It doesn't have to be device_info but I don't see why we wouldn't do it. It is just a different flavour of the vfunc approach after all. Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info 2017-10-24 20:26 ` Tvrtko Ursulin @ 2017-10-25 7:45 ` Jani Nikula 2017-10-26 10:06 ` Tvrtko Ursulin 0 siblings, 1 reply; 12+ messages in thread From: Jani Nikula @ 2017-10-25 7:45 UTC (permalink / raw) To: Tvrtko Ursulin, Chris Wilson, Sagar Arun Kamble, intel-gfx On Tue, 24 Oct 2017, Tvrtko Ursulin <tursulin@ursulin.net> wrote: > On 24/10/17 18:48, Jani Nikula wrote: >> On Tue, 24 Oct 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: >>> Quoting Sagar Arun Kamble (2017-10-24 11:41:13) >>>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c >>>> index 875d428..d1a4911 100644 >>>> --- a/drivers/gpu/drm/i915/intel_device_info.c >>>> +++ b/drivers/gpu/drm/i915/intel_device_info.c >>>> @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv) >>>> info->sseu.has_subslice_pg ? "y" : "n"); >>>> DRM_DEBUG_DRIVER("has EU power gating: %s\n", >>>> info->sseu.has_eu_pg ? "y" : "n"); >>>> + >>>> + /* Initialize PM interrupt register offsets */ >>>> + if (INTEL_GEN(dev_priv) >= 8) { >>>> + info->pm_iir_offset = GEN8_GT_IIR(2); >>>> + info->pm_imr_offset = GEN8_GT_IMR(2); >>>> + info->pm_ier_offset = GEN8_GT_IER(2); >>>> + } else { >>>> + info->pm_iir_offset = GEN6_PMIIR; >>>> + info->pm_imr_offset = GEN6_PMIMR; >>>> + info->pm_ier_offset = GEN6_PMIER; >>>> + } >>> >>> If you are going to take another pass at this, move these into the >>> static tables in i915_pci.c >>> >>> Updating GEN6_FEATURES and GEN8_FEATURES will then percolate into >>> individual platform defines. >> >> Like I wrote in reply to v1, I'm not convinced we should do this at all. >> >> What makes *these* registers so important they must be in device info? >> What makes most of i915_reg.h so unimportant they don't deserve the same >> treatment? Where do you draw the line? >> >> I'd draw the line at, no registers at device info. > > I suggested to Sagar this change during review so feel responsible to > chime in. > > So in general I just find the amount of times our driver asks itself > what it's running on a bit tasteless. :( > > I did quick and dirty check by bumping a counter in all the > IS_this|or|that checks, all which can be known at driver probe time, and > wired it up to the PMU so I can check their frequency. The annotated > perf stat output: > > root@e31:~# perf stat -a -e i915/whoami/ -I 1000 > # time counts unit events > > # idle system no X running > > 1.000298100 10 i915/whoami/ > > 2.000750955 8 i915/whoami/ > > 3.001104193 10 i915/whoami/ > > 4.001333433 10 i915/whoami/ > > 5.001703162 10 i915/whoami/ > > 6.002122721 10 i915/whoami/ > > > # starting X now.. > > 7.002266228 2,203 i915/whoami/ > > 8.002392598 4,682 i915/whoami/ > > 9.002764398 0 i915/whoami/ > > 10.003027119 0 i915/whoami/ > > 11.003486048 42 i915/whoami/ > > > # X idling.. > > 12.003854660 0 i915/whoami/ > > 13.004221680 0 i915/whoami/ > > 14.004622571 0 i915/whoami/ > > 15.004968110 0 i915/whoami/ > > 16.005372363 0 i915/whoami/ > > 17.005778034 0 i915/whoami/ > > 18.005941970 0 i915/whoami/ > > 19.006313427 0 i915/whoami/ > > 20.006676048 0 i915/whoami/ > > 21.007059927 0 i915/whoami/ > > 22.007507818 0 i915/whoami/ > > 23.007887628 0 i915/whoami/ > > 24.008207035 0 i915/whoami/ > > 25.008580496 0 i915/whoami/ > > # time counts unit events > 26.008949236 0 i915/whoami/ > > 27.009433473 0 i915/whoami/ > > > # gfxbench trex starting up > > 28.009677600 2,605 i915/whoami/ > > 29.009941972 716 i915/whoami/ > > 30.010127588 2,190 i915/whoami/ > > 31.010249535 46 i915/whoami/ > > 32.010383565 36 i915/whoami/ > > 33.010527674 0 i915/whoami/ > > > # trex running > > 34.010760584 4,709 i915/whoami/ > > 35.011079891 5,381 i915/whoami/ > > 36.011280234 5,306 i915/whoami/ > > 37.011719986 5,505 i915/whoami/ > > 38.012017531 5,386 i915/whoami/ > > 39.012529241 5,687 i915/whoami/ > > 40.012922986 6,009 i915/whoami/ > > 41.013120143 5,791 i915/whoami/ > > 42.013399982 5,296 i915/whoami/ > > 43.013712979 5,349 i915/whoami/ > > 44.014107375 5,127 i915/whoami/ > > 45.014553950 5,387 i915/whoami/ > > 46.014953020 5,364 i915/whoami/ > > 47.015243748 4,738 i915/whoami/ > > 48.015560460 4,788 i915/whoami/ > > 49.015867395 4,927 i915/whoami/ > > 50.016152690 4,886 i915/whoami/ > > > So.. I am not saying these particular registers are mega important, and > not even saying that these 5k/s conditionals are measurable (either as > branches or increased code size effect), but overall the situation is a > bit of.. bleurgh from the elegance point of view. :( > > If we have register sets which are 100% mutually exclusive, then I see > them as candidates to put them in some object at probe time. It doesn't > have to be device_info but I don't see why we wouldn't do it. It is just > a different flavour of the vfunc approach after all. I think to fix something that is inelegant, you have to have a plan to actually improve things in the long run. IMO adding a few random registers to device info without a plan is less elegant and less consistent than the status quo. We currently have at least three ways to index pipe/port/transcoder/etc based registers. Combine that with storing some register offsets in device info, you'll have six ways. There's a chance we'll end up adding the register offsets to device info both statically and dynamically. We're already struggling with guiding new contributors to defining registers in the existing schemes. Now, I'm sure we could spend weeks on end devising a plan how to move register offsets to device info or another structure, working out the details and bikeshedding. After that, we could do weeks and weeks of busywork converting registers, causing conflicts in all the work in our internal trees and developers' own branches, not to mention making bug fix and feature backports more painful. I have a pretty strong feeling this is not a good use of our time. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info 2017-10-25 7:45 ` Jani Nikula @ 2017-10-26 10:06 ` Tvrtko Ursulin 2017-10-26 12:50 ` Jani Nikula 0 siblings, 1 reply; 12+ messages in thread From: Tvrtko Ursulin @ 2017-10-26 10:06 UTC (permalink / raw) To: Jani Nikula, Tvrtko Ursulin, Chris Wilson, Sagar Arun Kamble, intel-gfx On 25/10/2017 08:45, Jani Nikula wrote: > On Tue, 24 Oct 2017, Tvrtko Ursulin <tursulin@ursulin.net> wrote: >> On 24/10/17 18:48, Jani Nikula wrote: >>> On Tue, 24 Oct 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: >>>> Quoting Sagar Arun Kamble (2017-10-24 11:41:13) >>>>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c >>>>> index 875d428..d1a4911 100644 >>>>> --- a/drivers/gpu/drm/i915/intel_device_info.c >>>>> +++ b/drivers/gpu/drm/i915/intel_device_info.c >>>>> @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv) >>>>> info->sseu.has_subslice_pg ? "y" : "n"); >>>>> DRM_DEBUG_DRIVER("has EU power gating: %s\n", >>>>> info->sseu.has_eu_pg ? "y" : "n"); >>>>> + >>>>> + /* Initialize PM interrupt register offsets */ >>>>> + if (INTEL_GEN(dev_priv) >= 8) { >>>>> + info->pm_iir_offset = GEN8_GT_IIR(2); >>>>> + info->pm_imr_offset = GEN8_GT_IMR(2); >>>>> + info->pm_ier_offset = GEN8_GT_IER(2); >>>>> + } else { >>>>> + info->pm_iir_offset = GEN6_PMIIR; >>>>> + info->pm_imr_offset = GEN6_PMIMR; >>>>> + info->pm_ier_offset = GEN6_PMIER; >>>>> + } >>>> >>>> If you are going to take another pass at this, move these into the >>>> static tables in i915_pci.c >>>> >>>> Updating GEN6_FEATURES and GEN8_FEATURES will then percolate into >>>> individual platform defines. >>> >>> Like I wrote in reply to v1, I'm not convinced we should do this at all. >>> >>> What makes *these* registers so important they must be in device info? >>> What makes most of i915_reg.h so unimportant they don't deserve the same >>> treatment? Where do you draw the line? >>> >>> I'd draw the line at, no registers at device info. >> >> I suggested to Sagar this change during review so feel responsible to >> chime in. >> >> So in general I just find the amount of times our driver asks itself >> what it's running on a bit tasteless. :( >> >> I did quick and dirty check by bumping a counter in all the >> IS_this|or|that checks, all which can be known at driver probe time, and >> wired it up to the PMU so I can check their frequency. The annotated >> perf stat output: >> >> root@e31:~# perf stat -a -e i915/whoami/ -I 1000 >> # time counts unit events >> >> # idle system no X running >> >> 1.000298100 10 i915/whoami/ >> >> 2.000750955 8 i915/whoami/ >> >> 3.001104193 10 i915/whoami/ >> >> 4.001333433 10 i915/whoami/ >> >> 5.001703162 10 i915/whoami/ >> >> 6.002122721 10 i915/whoami/ >> >> >> # starting X now.. >> >> 7.002266228 2,203 i915/whoami/ >> >> 8.002392598 4,682 i915/whoami/ >> >> 9.002764398 0 i915/whoami/ >> >> 10.003027119 0 i915/whoami/ >> >> 11.003486048 42 i915/whoami/ >> >> >> # X idling.. >> >> 12.003854660 0 i915/whoami/ >> >> 13.004221680 0 i915/whoami/ >> >> 14.004622571 0 i915/whoami/ >> >> 15.004968110 0 i915/whoami/ >> >> 16.005372363 0 i915/whoami/ >> >> 17.005778034 0 i915/whoami/ >> >> 18.005941970 0 i915/whoami/ >> >> 19.006313427 0 i915/whoami/ >> >> 20.006676048 0 i915/whoami/ >> >> 21.007059927 0 i915/whoami/ >> >> 22.007507818 0 i915/whoami/ >> >> 23.007887628 0 i915/whoami/ >> >> 24.008207035 0 i915/whoami/ >> >> 25.008580496 0 i915/whoami/ >> >> # time counts unit events >> 26.008949236 0 i915/whoami/ >> >> 27.009433473 0 i915/whoami/ >> >> >> # gfxbench trex starting up >> >> 28.009677600 2,605 i915/whoami/ >> >> 29.009941972 716 i915/whoami/ >> >> 30.010127588 2,190 i915/whoami/ >> >> 31.010249535 46 i915/whoami/ >> >> 32.010383565 36 i915/whoami/ >> >> 33.010527674 0 i915/whoami/ >> >> >> # trex running >> >> 34.010760584 4,709 i915/whoami/ >> >> 35.011079891 5,381 i915/whoami/ >> >> 36.011280234 5,306 i915/whoami/ >> >> 37.011719986 5,505 i915/whoami/ >> >> 38.012017531 5,386 i915/whoami/ >> >> 39.012529241 5,687 i915/whoami/ >> >> 40.012922986 6,009 i915/whoami/ >> >> 41.013120143 5,791 i915/whoami/ >> >> 42.013399982 5,296 i915/whoami/ >> >> 43.013712979 5,349 i915/whoami/ >> >> 44.014107375 5,127 i915/whoami/ >> >> 45.014553950 5,387 i915/whoami/ >> >> 46.014953020 5,364 i915/whoami/ >> >> 47.015243748 4,738 i915/whoami/ >> >> 48.015560460 4,788 i915/whoami/ >> >> 49.015867395 4,927 i915/whoami/ >> >> 50.016152690 4,886 i915/whoami/ >> >> >> So.. I am not saying these particular registers are mega important, and >> not even saying that these 5k/s conditionals are measurable (either as >> branches or increased code size effect), but overall the situation is a >> bit of.. bleurgh from the elegance point of view. :( >> >> If we have register sets which are 100% mutually exclusive, then I see >> them as candidates to put them in some object at probe time. It doesn't >> have to be device_info but I don't see why we wouldn't do it. It is just >> a different flavour of the vfunc approach after all. > > I think to fix something that is inelegant, you have to have a plan to > actually improve things in the long run. IMO adding a few random > registers to device info without a plan is less elegant and less > consistent than the status quo. > > We currently have at least three ways to index pipe/port/transcoder/etc > based registers. Combine that with storing some register offsets in > device info, you'll have six ways. There's a chance we'll end up adding > the register offsets to device info both statically and > dynamically. We're already struggling with guiding new contributors to > defining registers in the existing schemes. > > Now, I'm sure we could spend weeks on end devising a plan how to move > register offsets to device info or another structure, working out the > details and bikeshedding. After that, we could do weeks and weeks of > busywork converting registers, causing conflicts in all the work in our > internal trees and developers' own branches, not to mention making bug > fix and feature backports more painful. > > I have a pretty strong feeling this is not a good use of our time. I can only read here a dislike of a big rework (which I did not suggest to start with), and dislike of the piecemeal changes. So basically preference for a status quo. And there will be more and more of such checks. So today it is 5k/sec, in a year it might be more. So to clarify. Do you actually oppose some subsystem/area moving some registers to any data structure, or just to device info? Do you have a suggestion on what we could do? Or you simply think this is a complete non-issue? Regards, Tvrtko _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info 2017-10-26 10:06 ` Tvrtko Ursulin @ 2017-10-26 12:50 ` Jani Nikula 2017-10-26 13:24 ` Jani Nikula 0 siblings, 1 reply; 12+ messages in thread From: Jani Nikula @ 2017-10-26 12:50 UTC (permalink / raw) To: Tvrtko Ursulin, Tvrtko Ursulin, Chris Wilson, Sagar Arun Kamble, intel-gfx On Thu, 26 Oct 2017, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > On 25/10/2017 08:45, Jani Nikula wrote: >> On Tue, 24 Oct 2017, Tvrtko Ursulin <tursulin@ursulin.net> wrote: >>> On 24/10/17 18:48, Jani Nikula wrote: >>>> On Tue, 24 Oct 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: >>>>> Quoting Sagar Arun Kamble (2017-10-24 11:41:13) >>>>>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c >>>>>> index 875d428..d1a4911 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_device_info.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_device_info.c >>>>>> @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv) >>>>>> info->sseu.has_subslice_pg ? "y" : "n"); >>>>>> DRM_DEBUG_DRIVER("has EU power gating: %s\n", >>>>>> info->sseu.has_eu_pg ? "y" : "n"); >>>>>> + >>>>>> + /* Initialize PM interrupt register offsets */ >>>>>> + if (INTEL_GEN(dev_priv) >= 8) { >>>>>> + info->pm_iir_offset = GEN8_GT_IIR(2); >>>>>> + info->pm_imr_offset = GEN8_GT_IMR(2); >>>>>> + info->pm_ier_offset = GEN8_GT_IER(2); >>>>>> + } else { >>>>>> + info->pm_iir_offset = GEN6_PMIIR; >>>>>> + info->pm_imr_offset = GEN6_PMIMR; >>>>>> + info->pm_ier_offset = GEN6_PMIER; >>>>>> + } >>>>> >>>>> If you are going to take another pass at this, move these into the >>>>> static tables in i915_pci.c >>>>> >>>>> Updating GEN6_FEATURES and GEN8_FEATURES will then percolate into >>>>> individual platform defines. >>>> >>>> Like I wrote in reply to v1, I'm not convinced we should do this at all. >>>> >>>> What makes *these* registers so important they must be in device info? >>>> What makes most of i915_reg.h so unimportant they don't deserve the same >>>> treatment? Where do you draw the line? >>>> >>>> I'd draw the line at, no registers at device info. >>> >>> I suggested to Sagar this change during review so feel responsible to >>> chime in. >>> >>> So in general I just find the amount of times our driver asks itself >>> what it's running on a bit tasteless. :( >>> >>> I did quick and dirty check by bumping a counter in all the >>> IS_this|or|that checks, all which can be known at driver probe time, and >>> wired it up to the PMU so I can check their frequency. The annotated >>> perf stat output: >>> >>> root@e31:~# perf stat -a -e i915/whoami/ -I 1000 >>> # time counts unit events >>> >>> # idle system no X running >>> >>> 1.000298100 10 i915/whoami/ >>> >>> 2.000750955 8 i915/whoami/ >>> >>> 3.001104193 10 i915/whoami/ >>> >>> 4.001333433 10 i915/whoami/ >>> >>> 5.001703162 10 i915/whoami/ >>> >>> 6.002122721 10 i915/whoami/ >>> >>> >>> # starting X now.. >>> >>> 7.002266228 2,203 i915/whoami/ >>> >>> 8.002392598 4,682 i915/whoami/ >>> >>> 9.002764398 0 i915/whoami/ >>> >>> 10.003027119 0 i915/whoami/ >>> >>> 11.003486048 42 i915/whoami/ >>> >>> >>> # X idling.. >>> >>> 12.003854660 0 i915/whoami/ >>> >>> 13.004221680 0 i915/whoami/ >>> >>> 14.004622571 0 i915/whoami/ >>> >>> 15.004968110 0 i915/whoami/ >>> >>> 16.005372363 0 i915/whoami/ >>> >>> 17.005778034 0 i915/whoami/ >>> >>> 18.005941970 0 i915/whoami/ >>> >>> 19.006313427 0 i915/whoami/ >>> >>> 20.006676048 0 i915/whoami/ >>> >>> 21.007059927 0 i915/whoami/ >>> >>> 22.007507818 0 i915/whoami/ >>> >>> 23.007887628 0 i915/whoami/ >>> >>> 24.008207035 0 i915/whoami/ >>> >>> 25.008580496 0 i915/whoami/ >>> >>> # time counts unit events >>> 26.008949236 0 i915/whoami/ >>> >>> 27.009433473 0 i915/whoami/ >>> >>> >>> # gfxbench trex starting up >>> >>> 28.009677600 2,605 i915/whoami/ >>> >>> 29.009941972 716 i915/whoami/ >>> >>> 30.010127588 2,190 i915/whoami/ >>> >>> 31.010249535 46 i915/whoami/ >>> >>> 32.010383565 36 i915/whoami/ >>> >>> 33.010527674 0 i915/whoami/ >>> >>> >>> # trex running >>> >>> 34.010760584 4,709 i915/whoami/ >>> >>> 35.011079891 5,381 i915/whoami/ >>> >>> 36.011280234 5,306 i915/whoami/ >>> >>> 37.011719986 5,505 i915/whoami/ >>> >>> 38.012017531 5,386 i915/whoami/ >>> >>> 39.012529241 5,687 i915/whoami/ >>> >>> 40.012922986 6,009 i915/whoami/ >>> >>> 41.013120143 5,791 i915/whoami/ >>> >>> 42.013399982 5,296 i915/whoami/ >>> >>> 43.013712979 5,349 i915/whoami/ >>> >>> 44.014107375 5,127 i915/whoami/ >>> >>> 45.014553950 5,387 i915/whoami/ >>> >>> 46.014953020 5,364 i915/whoami/ >>> >>> 47.015243748 4,738 i915/whoami/ >>> >>> 48.015560460 4,788 i915/whoami/ >>> >>> 49.015867395 4,927 i915/whoami/ >>> >>> 50.016152690 4,886 i915/whoami/ >>> >>> >>> So.. I am not saying these particular registers are mega important, and >>> not even saying that these 5k/s conditionals are measurable (either as >>> branches or increased code size effect), but overall the situation is a >>> bit of.. bleurgh from the elegance point of view. :( >>> >>> If we have register sets which are 100% mutually exclusive, then I see >>> them as candidates to put them in some object at probe time. It doesn't >>> have to be device_info but I don't see why we wouldn't do it. It is just >>> a different flavour of the vfunc approach after all. >> >> I think to fix something that is inelegant, you have to have a plan to >> actually improve things in the long run. IMO adding a few random >> registers to device info without a plan is less elegant and less >> consistent than the status quo. >> >> We currently have at least three ways to index pipe/port/transcoder/etc >> based registers. Combine that with storing some register offsets in >> device info, you'll have six ways. There's a chance we'll end up adding >> the register offsets to device info both statically and >> dynamically. We're already struggling with guiding new contributors to >> defining registers in the existing schemes. >> >> Now, I'm sure we could spend weeks on end devising a plan how to move >> register offsets to device info or another structure, working out the >> details and bikeshedding. After that, we could do weeks and weeks of >> busywork converting registers, causing conflicts in all the work in our >> internal trees and developers' own branches, not to mention making bug >> fix and feature backports more painful. >> >> I have a pretty strong feeling this is not a good use of our time. > > I can only read here a dislike of a big rework (which I did not suggest > to start with), and dislike of the piecemeal changes. Any change would have to be piecemeal anyway. I don't have a dislike for that per se. I'm just saying that adding some registers to some data structures on a whim leads to an ugly inconsistent end result, and it gets cargo-culted to more and more places, uncontrolled. The driver will become harder to maintain. The changes must be done piecemeal, but there needs to be a plan where we want to take all this in the long term. And that plan is going to be an awful bikeshed fest. > So basically preference for a status quo. And there will be more and > more of such checks. So today it is 5k/sec, in a year it might be > more. Even with cached register offsets you'll anyway be doing 5k fetches of the cached offsets per second. Sure you'll save a branch and couple of immediates in code, but I can't imagine it being a huge penalty. > So to clarify. Do you actually oppose some subsystem/area moving some > registers to any data structure, or just to device info? > > Do you have a suggestion on what we could do? Or you simply think this > is a complete non-issue? This is not a non-issue, but, to be quite honest, I'd rather see people go to bugzilla and fix a dozen actual issues that are hitting CI or end users out there, today. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info 2017-10-26 12:50 ` Jani Nikula @ 2017-10-26 13:24 ` Jani Nikula 0 siblings, 0 replies; 12+ messages in thread From: Jani Nikula @ 2017-10-26 13:24 UTC (permalink / raw) To: Tvrtko Ursulin, Tvrtko Ursulin, Chris Wilson, Sagar Arun Kamble, intel-gfx On Thu, 26 Oct 2017, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Thu, 26 Oct 2017, Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: >> On 25/10/2017 08:45, Jani Nikula wrote: >>> On Tue, 24 Oct 2017, Tvrtko Ursulin <tursulin@ursulin.net> wrote: >>>> On 24/10/17 18:48, Jani Nikula wrote: >>>>> On Tue, 24 Oct 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: >>>>>> Quoting Sagar Arun Kamble (2017-10-24 11:41:13) >>>>>>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c >>>>>>> index 875d428..d1a4911 100644 >>>>>>> --- a/drivers/gpu/drm/i915/intel_device_info.c >>>>>>> +++ b/drivers/gpu/drm/i915/intel_device_info.c >>>>>>> @@ -462,4 +462,15 @@ void intel_device_info_runtime_init(struct drm_i915_private *dev_priv) >>>>>>> info->sseu.has_subslice_pg ? "y" : "n"); >>>>>>> DRM_DEBUG_DRIVER("has EU power gating: %s\n", >>>>>>> info->sseu.has_eu_pg ? "y" : "n"); >>>>>>> + >>>>>>> + /* Initialize PM interrupt register offsets */ >>>>>>> + if (INTEL_GEN(dev_priv) >= 8) { >>>>>>> + info->pm_iir_offset = GEN8_GT_IIR(2); >>>>>>> + info->pm_imr_offset = GEN8_GT_IMR(2); >>>>>>> + info->pm_ier_offset = GEN8_GT_IER(2); >>>>>>> + } else { >>>>>>> + info->pm_iir_offset = GEN6_PMIIR; >>>>>>> + info->pm_imr_offset = GEN6_PMIMR; >>>>>>> + info->pm_ier_offset = GEN6_PMIER; >>>>>>> + } >>>>>> >>>>>> If you are going to take another pass at this, move these into the >>>>>> static tables in i915_pci.c >>>>>> >>>>>> Updating GEN6_FEATURES and GEN8_FEATURES will then percolate into >>>>>> individual platform defines. >>>>> >>>>> Like I wrote in reply to v1, I'm not convinced we should do this at all. >>>>> >>>>> What makes *these* registers so important they must be in device info? >>>>> What makes most of i915_reg.h so unimportant they don't deserve the same >>>>> treatment? Where do you draw the line? >>>>> >>>>> I'd draw the line at, no registers at device info. >>>> >>>> I suggested to Sagar this change during review so feel responsible to >>>> chime in. >>>> >>>> So in general I just find the amount of times our driver asks itself >>>> what it's running on a bit tasteless. :( >>>> >>>> I did quick and dirty check by bumping a counter in all the >>>> IS_this|or|that checks, all which can be known at driver probe time, and >>>> wired it up to the PMU so I can check their frequency. The annotated >>>> perf stat output: >>>> >>>> root@e31:~# perf stat -a -e i915/whoami/ -I 1000 >>>> # time counts unit events >>>> >>>> # idle system no X running >>>> >>>> 1.000298100 10 i915/whoami/ >>>> >>>> 2.000750955 8 i915/whoami/ >>>> >>>> 3.001104193 10 i915/whoami/ >>>> >>>> 4.001333433 10 i915/whoami/ >>>> >>>> 5.001703162 10 i915/whoami/ >>>> >>>> 6.002122721 10 i915/whoami/ >>>> >>>> >>>> # starting X now.. >>>> >>>> 7.002266228 2,203 i915/whoami/ >>>> >>>> 8.002392598 4,682 i915/whoami/ >>>> >>>> 9.002764398 0 i915/whoami/ >>>> >>>> 10.003027119 0 i915/whoami/ >>>> >>>> 11.003486048 42 i915/whoami/ >>>> >>>> >>>> # X idling.. >>>> >>>> 12.003854660 0 i915/whoami/ >>>> >>>> 13.004221680 0 i915/whoami/ >>>> >>>> 14.004622571 0 i915/whoami/ >>>> >>>> 15.004968110 0 i915/whoami/ >>>> >>>> 16.005372363 0 i915/whoami/ >>>> >>>> 17.005778034 0 i915/whoami/ >>>> >>>> 18.005941970 0 i915/whoami/ >>>> >>>> 19.006313427 0 i915/whoami/ >>>> >>>> 20.006676048 0 i915/whoami/ >>>> >>>> 21.007059927 0 i915/whoami/ >>>> >>>> 22.007507818 0 i915/whoami/ >>>> >>>> 23.007887628 0 i915/whoami/ >>>> >>>> 24.008207035 0 i915/whoami/ >>>> >>>> 25.008580496 0 i915/whoami/ >>>> >>>> # time counts unit events >>>> 26.008949236 0 i915/whoami/ >>>> >>>> 27.009433473 0 i915/whoami/ >>>> >>>> >>>> # gfxbench trex starting up >>>> >>>> 28.009677600 2,605 i915/whoami/ >>>> >>>> 29.009941972 716 i915/whoami/ >>>> >>>> 30.010127588 2,190 i915/whoami/ >>>> >>>> 31.010249535 46 i915/whoami/ >>>> >>>> 32.010383565 36 i915/whoami/ >>>> >>>> 33.010527674 0 i915/whoami/ >>>> >>>> >>>> # trex running >>>> >>>> 34.010760584 4,709 i915/whoami/ >>>> >>>> 35.011079891 5,381 i915/whoami/ >>>> >>>> 36.011280234 5,306 i915/whoami/ >>>> >>>> 37.011719986 5,505 i915/whoami/ >>>> >>>> 38.012017531 5,386 i915/whoami/ >>>> >>>> 39.012529241 5,687 i915/whoami/ >>>> >>>> 40.012922986 6,009 i915/whoami/ >>>> >>>> 41.013120143 5,791 i915/whoami/ >>>> >>>> 42.013399982 5,296 i915/whoami/ >>>> >>>> 43.013712979 5,349 i915/whoami/ >>>> >>>> 44.014107375 5,127 i915/whoami/ >>>> >>>> 45.014553950 5,387 i915/whoami/ >>>> >>>> 46.014953020 5,364 i915/whoami/ >>>> >>>> 47.015243748 4,738 i915/whoami/ >>>> >>>> 48.015560460 4,788 i915/whoami/ >>>> >>>> 49.015867395 4,927 i915/whoami/ >>>> >>>> 50.016152690 4,886 i915/whoami/ >>>> >>>> >>>> So.. I am not saying these particular registers are mega important, and >>>> not even saying that these 5k/s conditionals are measurable (either as >>>> branches or increased code size effect), but overall the situation is a >>>> bit of.. bleurgh from the elegance point of view. :( >>>> >>>> If we have register sets which are 100% mutually exclusive, then I see >>>> them as candidates to put them in some object at probe time. It doesn't >>>> have to be device_info but I don't see why we wouldn't do it. It is just >>>> a different flavour of the vfunc approach after all. >>> >>> I think to fix something that is inelegant, you have to have a plan to >>> actually improve things in the long run. IMO adding a few random >>> registers to device info without a plan is less elegant and less >>> consistent than the status quo. >>> >>> We currently have at least three ways to index pipe/port/transcoder/etc >>> based registers. Combine that with storing some register offsets in >>> device info, you'll have six ways. There's a chance we'll end up adding >>> the register offsets to device info both statically and >>> dynamically. We're already struggling with guiding new contributors to >>> defining registers in the existing schemes. >>> >>> Now, I'm sure we could spend weeks on end devising a plan how to move >>> register offsets to device info or another structure, working out the >>> details and bikeshedding. After that, we could do weeks and weeks of >>> busywork converting registers, causing conflicts in all the work in our >>> internal trees and developers' own branches, not to mention making bug >>> fix and feature backports more painful. >>> >>> I have a pretty strong feeling this is not a good use of our time. >> >> I can only read here a dislike of a big rework (which I did not suggest >> to start with), and dislike of the piecemeal changes. > > Any change would have to be piecemeal anyway. I don't have a dislike for > that per se. I'm just saying that adding some registers to some data > structures on a whim leads to an ugly inconsistent end result, and it > gets cargo-culted to more and more places, uncontrolled. The driver will > become harder to maintain. The changes must be done piecemeal, but there > needs to be a plan where we want to take all this in the long term. > > And that plan is going to be an awful bikeshed fest. > >> So basically preference for a status quo. And there will be more and >> more of such checks. So today it is 5k/sec, in a year it might be >> more. > > Even with cached register offsets you'll anyway be doing 5k fetches of > the cached offsets per second. Sure you'll save a branch and couple of > immediates in code, but I can't imagine it being a huge penalty. > >> So to clarify. Do you actually oppose some subsystem/area moving some >> registers to any data structure, or just to device info? >> >> Do you have a suggestion on what we could do? Or you simply think this >> is a complete non-issue? > > This is not a non-issue, but, to be quite honest, I'd rather see people > go to bugzilla and fix a dozen actual issues that are hitting CI or end > users out there, today. As to the registers in question which apparently do get accessed a lot, I'd go with what Ville suggests in [1]. Fits our existing models, doesn't introduce anything new, addresses the issue. BR, Jani. [1] http://mid.mail-archive.com/20171024175559.GG10981@intel.com > > > BR, > Jani. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-10-26 13:23 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-24 10:41 [PATCH v2 1/1] drm/i915: Save PM interrupt register offsets in device info Sagar Arun Kamble 2017-10-24 10:46 ` Michal Wajdeczko 2017-10-24 16:34 ` Sagar Arun Kamble 2017-10-24 12:09 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/1] " Patchwork 2017-10-24 13:03 ` ✓ Fi.CI.IGT: " Patchwork 2017-10-24 16:43 ` [PATCH v2 1/1] " Chris Wilson 2017-10-24 17:48 ` Jani Nikula 2017-10-24 20:26 ` Tvrtko Ursulin 2017-10-25 7:45 ` Jani Nikula 2017-10-26 10:06 ` Tvrtko Ursulin 2017-10-26 12:50 ` Jani Nikula 2017-10-26 13:24 ` Jani Nikula
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.