Re: [PATCH 1/2] drm/i915: Compute the fw_domain id from the mask

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 12/05/2017 14:53, Chris Wilson wrote:
> Storing both the mask and the id is redundant as we can trivially
> compute one from the other. As the mask is more frequently used, remove
> the id.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
>  drivers/gpu/drm/i915/intel_uncore.c | 12 ++++++------
>  drivers/gpu/drm/i915/intel_uncore.h |  6 ++----
>  3 files changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index bd9abef40c66..a2c9c3e792e1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1468,7 +1468,7 @@ static int i915_forcewake_domains(struct seq_file *m, void *data)
>
>  	for_each_fw_domain(fw_domain, i915, tmp)
>  		seq_printf(m, "%s.wake_count = %u\n",
> -			   intel_uncore_forcewake_domain_to_str(fw_domain->id),
> +			   intel_uncore_forcewake_domain_to_str(fw_domain),
>  			   READ_ONCE(fw_domain->wake_count));
>
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index 08d7d08438c0..7eaa592aed26 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -40,8 +40,10 @@ static const char * const forcewake_domain_names[] = {
>  };
>
>  const char *
> -intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id)
> +intel_uncore_forcewake_domain_to_str(const struct intel_uncore_forcewake_domain *d)
>  {
> +	unsigned int id = ilog2(d->mask);
> +
>  	BUILD_BUG_ON(ARRAY_SIZE(forcewake_domain_names) != FW_DOMAIN_ID_COUNT);
>
>  	if (id >= 0 && id < FW_DOMAIN_ID_COUNT)
> @@ -77,7 +79,7 @@ fw_domain_wait_ack_clear(const struct drm_i915_private *i915,
>  			     FORCEWAKE_KERNEL) == 0,
>  			    FORCEWAKE_ACK_TIMEOUT_MS))
>  		DRM_ERROR("%s: timed out waiting for forcewake ack to clear.\n",
> -			  intel_uncore_forcewake_domain_to_str(d->id));
> +			  intel_uncore_forcewake_domain_to_str(d));
>  }
>
>  static inline void
> @@ -95,7 +97,7 @@ fw_domain_wait_ack(const struct drm_i915_private *i915,
>  			     FORCEWAKE_KERNEL),
>  			    FORCEWAKE_ACK_TIMEOUT_MS))
>  		DRM_ERROR("%s: timed out waiting for forcewake ack request.\n",
> -			  intel_uncore_forcewake_domain_to_str(d->id));
> +			  intel_uncore_forcewake_domain_to_str(d));
>  }
>
>  static inline void
> @@ -209,7 +211,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
>  	struct intel_uncore_forcewake_domain *domain =
>  	       container_of(timer, struct intel_uncore_forcewake_domain, timer);
>  	struct drm_i915_private *dev_priv =
> -		container_of(domain, struct drm_i915_private, uncore.fw_domain[domain->id]);
> +		container_of(domain, struct drm_i915_private, uncore.fw_domain[ilog2(domain->mask)]);

Not sure I see the benefit of removing one field and then needing this 
ilog2 in the timer callback.

Regards,

Tvrtko


>  	unsigned long irqflags;
>
>  	assert_rpm_device_not_suspended(dev_priv);
> @@ -1149,8 +1151,6 @@ static void fw_domain_init(struct drm_i915_private *dev_priv,
>  	d->reg_set = reg_set;
>  	d->reg_ack = reg_ack;
>
> -	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));
> diff --git a/drivers/gpu/drm/i915/intel_uncore.h b/drivers/gpu/drm/i915/intel_uncore.h
> index ff6fe2bb0ccf..5fec5fd4346c 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.h
> +++ b/drivers/gpu/drm/i915/intel_uncore.h
> @@ -93,8 +93,7 @@ struct intel_uncore {
>  	u32 fw_reset;
>
>  	struct intel_uncore_forcewake_domain {
> -		enum forcewake_domain_id id;
> -		enum forcewake_domains mask;
> +		unsigned int mask;
>  		unsigned int wake_count;
>  		struct hrtimer timer;
>  		i915_reg_t reg_set;
> @@ -123,8 +122,7 @@ void intel_uncore_resume_early(struct drm_i915_private *dev_priv);
>
>  u64 intel_uncore_edram_size(struct drm_i915_private *dev_priv);
>  void assert_forcewakes_inactive(struct drm_i915_private *dev_priv);
> -const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
> -
> +const char *intel_uncore_forcewake_domain_to_str(const struct intel_uncore_forcewake_domain *domain);
>  enum forcewake_domains
>  intel_uncore_forcewake_for_reg(struct drm_i915_private *dev_priv,
>  			       i915_reg_t reg, unsigned int op);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx