* [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
* ✓ 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: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
* 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.