On 04/04/16 17:51, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
As the vast majority of users does not use the domain id variable,
"do not use"
we can eliminate it from the iterator and also change the latter
using the same principle as was recently done for for_each_engine.
For a couple of callers which do need the domain id, or the domain
mask, store the later in the domain itself at init time and use
both through it.
"For a couple of callers which do need the domain mask, store it
in the domain array (which already has the domain id), then both
can be retrieved thence." ?
Result is clearer code and smaller generated binary, especially
in the tight fw get/put loops. Also, relationship between domain
id and mask is no longer assumed in the macro.
"in the macro or elsewhere" ?
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
A few typos & clarifications above, plus a minor quibble about a macro
name (which you haven't actually changed, but might as well fix).
Otherwise LGTM, so
Reviewed-by: Dave Gordon <david.s.gordon@xxxxxxxxx>
(even if you don't change the macro name)
---
drivers/gpu/drm/i915/i915_debugfs.c | 5 ++---
drivers/gpu/drm/i915/i915_drv.h | 17 +++++++-------
drivers/gpu/drm/i915/intel_uncore.c | 45 +++++++++++++++++--------------------
3 files changed, 32 insertions(+), 35 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index a2e3af02c292..06fce014d0b4 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1464,12 +1464,11 @@ static int i915_forcewake_domains(struct seq_file *m, void *data)
struct drm_device *dev = node->minor->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_uncore_forcewake_domain *fw_domain;
- int i;
spin_lock_irq(&dev_priv->uncore.lock);
- for_each_fw_domain(fw_domain, dev_priv, i) {
+ for_each_fw_domain(fw_domain, dev_priv) {
seq_printf(m, "%s.wake_count = %u\n",
- intel_uncore_forcewake_domain_to_str(i),
+ intel_uncore_forcewake_domain_to_str(fw_domain->id),
fw_domain->wake_count);
}
spin_unlock_irq(&dev_priv->uncore.lock);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7d4c704d7d75..160f980f0368 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -665,6 +665,7 @@ struct intel_uncore {
struct intel_uncore_forcewake_domain {
struct drm_i915_private *i915;
enum forcewake_domain_id id;
+ enum forcewake_domains mask;
At present this mask will always have only one bit set, but I suppose
there might be some utility in allowing multiple bits (virtual domains?)
unsigned wake_count;
struct hrtimer timer;
i915_reg_t reg_set;
@@ -679,14 +680,14 @@ struct intel_uncore {
};
/* Iterate over initialised fw domains */
-#define for_each_fw_domain_mask(domain__, mask__, dev_priv__, i__) \
- for ((i__) = 0, (domain__) = &(dev_priv__)->uncore.fw_domain[0]; \
- (i__) < FW_DOMAIN_ID_COUNT; \
- (i__)++, (domain__) = &(dev_priv__)->uncore.fw_domain[i__]) \
- for_each_if (((mask__) & (dev_priv__)->uncore.fw_domains) & (1 << (i__)))
-
-#define for_each_fw_domain(domain__, dev_priv__, i__) \
- for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__, i__)
+#define for_each_fw_domain_mask(domain__, mask__, dev_priv__) \
+ for ((domain__) = &(dev_priv__)->uncore.fw_domain[0]; \
+ (domain__) < &(dev_priv__)->uncore.fw_domain[FW_DOMAIN_ID_COUNT]; \
+ (domain__)++) \
+ for_each_if ((mask__) & (domain__)->mask)
+
For consistency with the for_each_engine() macros, the name ought to be
"for_each_fw_domain_mask*ed*()", showing that we're iterating over a
subset selected by the 'mask' parameter, rather than iterating over all
possible masks.
.Dave.
+#define for_each_fw_domain(domain__, dev_priv__) \
+ for_each_fw_domain_mask(domain__, FORCEWAKE_ALL, dev_priv__)
#define CSR_VERSION(major, minor) ((major) << 16 | (minor))
#define CSR_VERSION_MAJOR(version) ((version) >> 16)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 76ac990de354..49edd641b434 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -111,9 +111,8 @@ static void
fw_domains_get(struct drm_i915_private *dev_priv, enum forcewake_domains fw_domains)
{
struct intel_uncore_forcewake_domain *d;
- enum forcewake_domain_id id;
- for_each_fw_domain_mask(d, fw_domains, dev_priv, id) {
+ for_each_fw_domain_mask(d, fw_domains, dev_priv) {
fw_domain_wait_ack_clear(d);
fw_domain_get(d);
fw_domain_wait_ack(d);
@@ -124,9 +123,8 @@ static void
fw_domains_put(struct drm_i915_private *dev_priv, enum forcewake_domains fw_domains)
{
struct intel_uncore_forcewake_domain *d;
- enum forcewake_domain_id id;
- for_each_fw_domain_mask(d, fw_domains, dev_priv, id) {
+ for_each_fw_domain_mask(d, fw_domains, dev_priv) {
fw_domain_put(d);
fw_domain_posting_read(d);
}
@@ -136,10 +134,9 @@ static void
fw_domains_posting_read(struct drm_i915_private *dev_priv)
{
struct intel_uncore_forcewake_domain *d;
- enum forcewake_domain_id id;
/* No need to do for all, just do for first found */
- for_each_fw_domain(d, dev_priv, id) {
+ for_each_fw_domain(d, dev_priv) {
fw_domain_posting_read(d);
break;
}
@@ -149,12 +146,11 @@ static void
fw_domains_reset(struct drm_i915_private *dev_priv, enum forcewake_domains fw_domains)
{
struct intel_uncore_forcewake_domain *d;
- enum forcewake_domain_id id;
if (dev_priv->uncore.fw_domains == 0)
return;
- for_each_fw_domain_mask(d, fw_domains, dev_priv, id)
+ for_each_fw_domain_mask(d, fw_domains, dev_priv)
fw_domain_reset(d);
fw_domains_posting_read(dev_priv);
@@ -256,7 +252,6 @@ void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
unsigned long irqflags;
struct intel_uncore_forcewake_domain *domain;
int retry_count = 100;
- enum forcewake_domain_id id;
enum forcewake_domains fw = 0, active_domains;
/* Hold uncore.lock across reset to prevent any register access
@@ -266,7 +261,7 @@ void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
while (1) {
active_domains = 0;
- for_each_fw_domain(domain, dev_priv, id) {
+ for_each_fw_domain(domain, dev_priv) {
if (hrtimer_cancel(&domain->timer) == 0)
continue;
@@ -275,9 +270,9 @@ void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
spin_lock_irqsave(&dev_priv->uncore.lock, irqflags);
- for_each_fw_domain(domain, dev_priv, id) {
+ for_each_fw_domain(domain, dev_priv) {
if (hrtimer_active(&domain->timer))
- active_domains |= (1 << id);
+ active_domains |= domain->mask;
}
if (active_domains == 0)
@@ -294,9 +289,9 @@ void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
WARN_ON(active_domains);
- for_each_fw_domain(domain, dev_priv, id)
+ for_each_fw_domain(domain, dev_priv)
if (domain->wake_count)
- fw |= 1 << id;
+ fw |= domain->mask;
if (fw)
dev_priv->uncore.funcs.force_wake_put(dev_priv, fw);
@@ -418,16 +413,15 @@ static void __intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
enum forcewake_domains fw_domains)
{
struct intel_uncore_forcewake_domain *domain;
- enum forcewake_domain_id id;
if (!dev_priv->uncore.funcs.force_wake_get)
return;
fw_domains &= dev_priv->uncore.fw_domains;
- for_each_fw_domain_mask(domain, fw_domains, dev_priv, id) {
+ for_each_fw_domain_mask(domain, fw_domains, dev_priv) {
if (domain->wake_count++)
- fw_domains &= ~(1 << id);
+ fw_domains &= ~domain->mask;
}
if (fw_domains)
@@ -485,14 +479,13 @@ static void __intel_uncore_forcewake_put(struct drm_i915_private *dev_priv,
enum forcewake_domains fw_domains)
{
struct intel_uncore_forcewake_domain *domain;
- enum forcewake_domain_id id;
if (!dev_priv->uncore.funcs.force_wake_put)
return;
fw_domains &= dev_priv->uncore.fw_domains;
- for_each_fw_domain_mask(domain, fw_domains, dev_priv, id) {
+ for_each_fw_domain_mask(domain, fw_domains, dev_priv) {
if (WARN_ON(domain->wake_count == 0))
continue;
@@ -546,12 +539,11 @@ void intel_uncore_forcewake_put__locked(struct drm_i915_private *dev_priv,
void assert_forcewakes_inactive(struct drm_i915_private *dev_priv)
{
struct intel_uncore_forcewake_domain *domain;
- enum forcewake_domain_id id;
if (!dev_priv->uncore.funcs.force_wake_get)
return;
- for_each_fw_domain(domain, dev_priv, id)
+ for_each_fw_domain(domain, dev_priv)
WARN_ON(domain->wake_count);
}
@@ -727,15 +719,14 @@ static inline void __force_wake_auto(struct drm_i915_private *dev_priv,
enum forcewake_domains fw_domains)
{
struct intel_uncore_forcewake_domain *domain;
- enum forcewake_domain_id id;
if (WARN_ON(!fw_domains))
return;
/* Ideally GCC would be constant-fold and eliminate this loop */
- for_each_fw_domain_mask(domain, fw_domains, dev_priv, id) {
+ for_each_fw_domain_mask(domain, fw_domains, dev_priv) {
if (domain->wake_count) {
- fw_domains &= ~(1 << id);
+ fw_domains &= ~domain->mask;
continue;
}
@@ -1156,6 +1147,12 @@ static void fw_domain_init(struct drm_i915_private *dev_priv,
d->i915 = dev_priv;
d->id = domain_id;
+ BUILD_BUG_ON(FORCEWAKE_RENDER != (1 << FW_DOMAIN_ID_RENDER));
+ BUILD_BUG_ON(FORCEWAKE_BLITTER != (1 << FW_DOMAIN_ID_BLITTER));
+ BUILD_BUG_ON(FORCEWAKE_MEDIA != (1 << FW_DOMAIN_ID_MEDIA));
+
+ d->mask = 1 << domain_id;
+
hrtimer_init(&d->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
d->timer.function = intel_uncore_fw_release_timer;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
