Re: [PATCH 2/3] drm/i915: Simplify for_each_fw_domain iterators

[Dave Gordon <david.s.gordon@intel.com>]

On 04/04/16 17:51, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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@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@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;
>
>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx