[Intel-gfx] [PATCH 2/3] drm/i915: Simplify for_each_fw_domain iterators
Dave Gordon
david.s.gordon at intel.com
Mon Apr 4 19:14:42 UTC 2016
On 04/04/16 17:51, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>
> 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 at intel.com>
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 at intel.com>
(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;
>
>
More information about the Intel-gfx
mailing list